Bug 27850

<rdar://problem/7106061>

Created attachment 33804 [details] Use Scroll instead of Mouse Wheel Events - Frames Broken

Created attachment 34757 [details] Panning Not Using Mouse Events

Comment on attachment 34757 [details] Panning Not Using Mouse Events I'm not super familiar with this code, so I suggest having another person review it. Here are my comments so far: > + HitTestRequest request(HitTestRequest::ReadOnly); > + HitTestResult result(m_page->mainFrame()->view()->screenToContents(gestureBeginPoint)); > + > + m_page->mainFrame()->document()->renderView()->layer()->hitTest(request, result); Do we know for sure that m_page->mainFrame()->document() will not return null? > + m_gestureTargetNode = result.innerNode(); > + if (result.scrollbar()) > + hitScrollbar = true; You can move the declaration of hitScrollbar down here and set it directly to result.scrollbar(). > + > + while (m_gestureTargetNode && (m_gestureTargetNode->isContainerNode()) && !hitScrollbar) { Don't need the extra parentheses around m_gestureTargetNode->isContainerNode(). > + Frame* childFrame = EventHandler::subframeForTargetNode(m_gestureTargetNode); > + if (!childFrame) > + break; > + > + HitTestResult result(childFrame->view()->screenToContents(gestureBeginPoint)); > + > + childFrame->document()->renderView()->layer()->hitTest(request, result); > + m_gestureTargetNode = result.innerNode(); > + if (result.scrollbar()) > + hitScrollbar = true; > + } If hitScrollbar is true and the gesture is blocked, does it make more sense to reset m_gestureTargetNode to 0? > + > + RenderObject* renderer = 0; > + if (m_gestureTargetNode) > + renderer = m_gestureTargetNode->renderer(); > + while (renderer) { > + if (renderer->isBox() && toRenderBox(renderer)->canBeScrolledAndHasScrollableArea()) { > + canBeScrolled = true; > + break; > + } > + renderer = renderer->parent(); > + } It might be cleaner to rewrite the above as a for loop, and keep renderer local within that loop. > > - if (eventHandlerPoint.x() > view->visibleWidth() || scrollbarTest.scrollbar()) { > - // We are in the scrollbar, turn off panning, need to be able to drag the scrollbar > + RECT webViewRect; > + GetWindowRect(m_viewWindow, &webViewRect); > + > + if ((view->verticalScrollbar() && gestureBeginPoint.x > (webViewRect.right - view->verticalScrollbar()->theme()->scrollbarThickness())) > + || hitScrollbar) { > + // If we didn't hit a scrollbar in our hit testing, the only other scrollbar it would be > + // possible to hit would be the WebView scrollbar, so we can check if our x coordinate is > + // outside of the webview, where the scollbar vertical is. > + dwPanWant = GC_PAN | GC_PAN_WITH_INERTIA | GC_PAN_WITH_GUTTER; > + dwPanBlock = GC_PAN_WITH_SINGLE_FINGER_HORIZONTALLY | GC_PAN_WITH_SINGLE_FINGER_VERTICALLY; > + } else if (!canBeScrolled) { > + // We can't scroll on the frame/page we are in. > dwPanWant = GC_PAN | GC_PAN_WITH_INERTIA | GC_PAN_WITH_GUTTER; > dwPanBlock = GC_PAN_WITH_SINGLE_FINGER_HORIZONTALLY | GC_PAN_WITH_SINGLE_FINGER_VERTICALLY; > } else { Could combine the above two "if" together. > + // Someone in our DOM Hierarchy can scroll, and we're not on a scrollbar, let them pan. > dwPanWant = GC_PAN | GC_PAN_WITH_SINGLE_FINGER_VERTICALLY | GC_PAN_WITH_INERTIA | GC_PAN_WITH_GUTTER; > dwPanBlock = GC_PAN_WITH_SINGLE_FINGER_HORIZONTALLY; > } > > bool WebView::gesture(WPARAM wParam, LPARAM lParam) > @@ -1372,21 +1414,18 @@ bool WebView::gesture(WPARAM wParam, LPA > m_lastPanX = gi.ptsLocation.x; > m_lastPanY = gi.ptsLocation.y; > > - CloseGestureInfoHandlePtr()(gestureHandle); > + return DefWindowProc(gi.hwndTarget, WM_GESTURE, wParam, lParam); Are we sure we should not call CloseGestureInfoHandlePtr()(gestureHandle) in this case? Can we just return false for fallback behavior? > break; Double-check release build to make sure it doesn't complain about the above line being unreachable. > + bool didUpdatePanning = true; > // FIXME: Support Horizontal Window Bounce > + // FIXME: If the user starts panning down after a window bounce has started, the window doesn't bounce back > + // until they release their finger. > if (vertScrollbar->currentPos() == 0) > - UpdatePanningFeedbackPtr()(m_viewWindow, 0, m_yOverpan, gi.dwFlags & GF_INERTIA); > + didUpdatePanning = UpdatePanningFeedbackPtr()(gi.hwndTarget, 0, m_yOverpan, gi.dwFlags & GF_INERTIA); > else if (vertScrollbar->currentPos() >= vertScrollbar->maximum()) > - UpdatePanningFeedbackPtr()(m_viewWindow, 0, m_yOverpan, gi.dwFlags & GF_INERTIA); > + didUpdatePanning = UpdatePanningFeedbackPtr()(gi.hwndTarget, 0, m_yOverpan, gi.dwFlags & GF_INERTIA); > What is didUpdatePanning for? You didn't refer to it later in the code. > CloseGestureInfoHandlePtr()(gestureHandle); > break; > } > default: > - // We have encountered an unknown gesture - return false to pass it to DefWindowProc > - CloseGestureInfoHandlePtr()(gestureHandle); > + return DefWindowProc(gi.hwndTarget, WM_GESTURE, wParam, lParam); Are we sure we should not call CloseGestureInfoHandlePtr()(gestureHandle) in this case? Can we just return false for fallback behavior? > break; > } > > Index: WebKit/win/WebView.h > =================================================================== > --- WebKit/win/WebView.h (revision 47167) > +++ WebKit/win/WebView.h (working copy) > @@ -922,6 +922,7 @@ protected: > OwnPtr<HashSet<WebCore::String> > m_embeddedViewMIMETypes; > > //Variables needed to store gesture information > + WebCore::Node* m_gestureTargetNode; You need to initialize m_gestureTargetNode in the WebView constructor. Another question to ask is how m_gestureTargetNode is maintained. From what you explained to me, we could get a gesture notify followed by a bunch of gestures. What if m_gestureTargetNode gets deleted in the middle of the gesture calls?

This was landed in http://trac.webkit.org/changeset/47594.