Bug 148155 - Scroll snapping should trigger when receiving a momentum end wheel event
Summary: Scroll snapping should trigger when receiving a momentum end wheel event
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 148326
  Show dependency treegraph
 
Reported: 2015-08-18 18:29 PDT by Wenson Hsieh
Modified: 2015-08-21 14:35 PDT (History)
3 users (show)

See Also:


Attachments
Patch (5.26 KB, patch)
2015-08-18 23:06 PDT, Wenson Hsieh
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2015-08-18 18:29:21 PDT
Scroll snapping should trigger when receiving a wheel event indicating momentum end. This will allow our failing scroll snapping layout tests to pass, since asynchronous momentum wheel events are currently being coalesced. However, in theory, if a user were to flick extremely lightly on the trackpad and generate fewer than 3 wheel momentum events before letting go, scroll snapping would not occur.
Comment 1 Wenson Hsieh 2015-08-18 18:29:55 PDT
<rdar://problem/22336880>
Comment 2 Wenson Hsieh 2015-08-18 23:06:25 PDT
Created attachment 259361 [details]
Patch
Comment 3 Alexey Proskuryakov 2015-08-19 08:53:47 PDT
Comment on attachment 259361 [details]
Patch

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

> Source/WebCore/platform/cocoa/ScrollController.mm:571
> +        else {
> +            snapState.clearInitialWheelDeltaWindow();
> +            snapState.m_shouldOverrideWheelEvent = false;
> +        }

It's not obvious to me why this is inside an "else". beginScrollSnapAnimation does set these variables, but not in all code paths.
Comment 4 Wenson Hsieh 2015-08-19 09:05:12 PDT
Comment on attachment 259361 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/platform/cocoa/ScrollController.mm:571
>> +        }
> 
> It's not obvious to me why this is inside an "else". beginScrollSnapAnimation does set these variables, but not in all code paths.

Good point. I should be able to lift the logic to reset the delta window and override state out of the else clause, since we should be doing this anyway even if we don't trigger a glide here.
Comment 5 Wenson Hsieh 2015-08-19 11:14:00 PDT
Committed r188641: <http://trac.webkit.org/changeset/188641>