Bug 121711 - Handle panning gestures messages properly on Windows
Summary: Handle panning gestures messages properly on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-09-20 13:47 PDT by Roger Fong
Modified: 2013-09-20 23:13 PDT (History)
9 users (show)

See Also:


Attachments
patch (11.00 KB, patch)
2013-09-20 17:00 PDT, Roger Fong
bfulgham: review+
Details | Formatted Diff | Diff
follow-up fix (2.07 KB, patch)
2013-09-20 22:18 PDT, Alexey Proskuryakov
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roger Fong 2013-09-20 13:47:31 PDT
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
Comment 1 Roger Fong 2013-09-20 13:48:17 PDT
Each of these fixes by itself is rather small.
I'm not sure if I should just break this up into 5 smaller bugs.
Comment 2 Roger Fong 2013-09-20 14:50:38 PDT
<rdar://problem/12712495>
Comment 3 Roger Fong 2013-09-20 17:00:35 PDT
Created attachment 212245 [details]
patch
Comment 4 Brent Fulgham 2013-09-20 17:17:12 PDT
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!
Comment 5 Roger Fong 2013-09-20 18:34:27 PDT
(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.
Comment 6 Roger Fong 2013-09-20 20:25:31 PDT
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.
Comment 7 Roger Fong 2013-09-20 21:10:03 PDT
(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.
Comment 8 Ryosuke Niwa 2013-09-20 22:01:35 PDT
This patch caused 5 tests to hit assertions on Mac:
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20%28Tests%29/builds/13132
Comment 9 Alexey Proskuryakov 2013-09-20 22:18:45 PDT
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.
Comment 10 Alexey Proskuryakov 2013-09-20 22:21:09 PDT
EWS didn't catch this because the patch in the bug is not quite what got landed.
Comment 11 Alexey Proskuryakov 2013-09-20 22:34:25 PDT
Landed the follow-up fix in r156223.
Comment 12 Brent Fulgham 2013-09-20 23:13:07 PDT
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. :(