Bug 27850 - Panning by emulating Mouse Wheel events breaks Google Maps
Summary: Panning by emulating Mouse Wheel events breaks Google Maps
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Brian Weinstein
URL:
Keywords: InRadar
Depends on: 28023
Blocks: 26486
  Show dependency treegraph
 
Reported: 2009-07-30 12:28 PDT by Brian Weinstein
Modified: 2009-09-03 01:13 PDT (History)
1 user (show)

See Also:


Attachments
Use Scroll instead of Mouse Wheel Events - Frames Broken (9.45 KB, patch)
2009-07-30 12:35 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff
Panning Not Using Mouse Events (10.70 KB, patch)
2009-08-13 10:37 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Weinstein 2009-07-30 12:28:23 PDT
When you try to pan in google maps, the expected behavior would be pan the map, like you are dragging of the current behavior, which will zoom with WebKit.

This patch will be broken into two parts - 
1) Change from emulating mouse wheel events for panning to just calling scroll methods
2) Before allowing pan gestures, make sure the element that is being panned doesn't have any drag listeners, if it does, instead of panning, we want our finger motion to be a drag instead of a pan.
Comment 1 Brian Weinstein 2009-07-30 12:30:04 PDT
<rdar://problem/7106061>
Comment 2 Brian Weinstein 2009-07-30 12:35:44 PDT
Created attachment 33804 [details]
Use Scroll instead of Mouse Wheel Events - Frames Broken
Comment 3 Brian Weinstein 2009-08-13 10:37:32 PDT
Created attachment 34757 [details]
Panning Not Using Mouse Events
Comment 4 Ada Chan 2009-08-18 14:08:08 PDT
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?
Comment 5 Brian Weinstein 2009-09-02 11:28:49 PDT
This was landed in http://trac.webkit.org/changeset/47594.