Bug 121711 – Handle panning gestures messages properly on Windows

• Component: UI Events
• Status: RESOLVED FIXED

[Roger Fong]

This bug addresses a couple issues:

1. Two finger panning in one direction can sometimes cause some unexpected scrolling in the other direction when the gesture first begins(directions meaning horizontal and vertical)
2. Single finger horizontal panning should only be disabled when attempting to select text
3. Scrolling via panning should be clamped, other wise we can scroll contents completely out of the scrollview.
4. Horizontal Overpan should work
5. Overpan should work only when the main document is scrolled

[Roger Fong]

Each of these fixes by itself is rather small.
I'm not sure if I should just break this up into 5 smaller bugs.

[Roger Fong]

<rdar://problem/12712495>

[Roger Fong]

Created attachment 212245 [details]
patch

[Brent Fulgham]

Comment on attachment 212245 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212245&action=review

Looks great!  We might want to add a FIXME for Right-to-left languages if that's something we are likely to encounter for scroll bar support.

> Source/WebKit/win/WebView.cpp:1611
> +            || (view->horizontalScrollbar() && (gestureBeginPoint.y > (webViewRect.bottom - view->horizontalScrollbar()->theme()->scrollbarThickness())));  

This might not be right for a right-to-left environment.

> Source/WebKit/win/WebView.cpp:1640
> +        // Disable single-fingered horizontal panning only if the target node text.

... target node *is* text?

> Source/WebKit/win/WebView.cpp:1741
> +        UpdatePanningFeedbackPtr()(m_viewWindow, xpan, ypan, gi.dwFlags & GF_INERTIA);

Nice!

[Roger Fong]

(In reply to comment #4)
> (From update of attachment 212245 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212245&action=review
> 
> Looks great!  We might want to add a FIXME for Right-to-left languages if that's something we are likely to encounter for scroll bar support.
> 
> > Source/WebKit/win/WebView.cpp:1611
> > +            || (view->horizontalScrollbar() && (gestureBeginPoint.y > (webViewRect.bottom - view->horizontalScrollbar()->theme()->scrollbarThickness())));  
> 
> This might not be right for a right-to-left environment.
> 

I'll run a quick test.

[Roger Fong]

Committed: http://trac.webkit.org/changeset/156219

Setting dir=rtl on the document doesn't flip the position of the vertical scrollbar.

Perhaps that's actually controlled by the HWND? If that's the case, GetWindowRect might actually take the flipped-ness into account.

Will investigate further later.

[Roger Fong]

(In reply to comment #6)
> Committed: http://trac.webkit.org/changeset/156219
> 
> Setting dir=rtl on the document doesn't flip the position of the vertical scrollbar.
> 
> Perhaps that's actually controlled by the HWND? If that's the case, GetWindowRect might actually take the flipped-ness into account.
> 
> Will investigate further later.

I revoke that statement. It definitely won't work. Even if GetWindowRect flips the left and right values appropriately I'll need to add the scrollbar width, not subtract.

[Ryosuke Niwa]

This patch caused 5 tests to hit assertions on Mac:
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20%28Tests%29/builds/13132

[Alexey Proskuryakov]

Created attachment 212253 [details]
follow-up fix

> This patch caused 5 tests to hit assertions on Mac:
> http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20%28Tests%29/builds/13132

That warrants either an immediate rollout or an immediate fix. It's not OK to keep crashes in the tree over weekend.

[Alexey Proskuryakov]

EWS didn't catch this because the patch in the bug is not quite what got landed.

[Alexey Proskuryakov]

Landed the follow-up fix in r156223.

[Brent Fulgham]

Comment on attachment 212253 [details]
follow-up fix

View in context: https://bugs.webkit.org/attachment.cgi?id=212253&action=review

> Source/WebCore/rendering/RenderLayer.cpp:-2181
> -        *scrolledView = &renderer().view().frameView();

I should have caught this when I reviewed. :(