Bug 145148

Summary: Scroll-snap animations should not start on axes with zero-delta
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: Layout and RenderingAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch dino: review+

Description Brent Fulgham 2015-05-18 15:55:53 PDT 
I noticed a bug in two-dimensional scroll snap regions as follows:

1. Begin a vertical scroll-snap gesture.
2. Scroll region being a vertical scroll animation.
3. Interrupt the vertical scroll with a horizontal gesture.
4. A horizontal scroll will begin.
5. Suddenly, the horizontal scroll will end and a vertical scroll will start.
6. The remainder of the original vertical scroll completes.

This is happening because of a flaw in the gesture handling code.

a. Vertical gestures are tracked, and a vertical target is selected.
b. Horizontal gesture interrupts animation, clearing the wheel tracking state.
c. Horizontal gesture is tracked, causing non-zero X delta values to be tracked, as well as an equivalent set of zero Y delta values.
d. When the horizontal gesture enters the inertial scrolling phase, WebKit activates the scroll snap logic.
e. WebKit first checks for vertical scrolling. It sees that it has a valid count of wheel events, and triggers the start of a vertical scroll snap animation.
f. The vertical scroll animation begins overriding wheel events.
g. Later, WebKit checks for horizontal scrolling. However, it sees that wheel events are being overridden, and stops.
h. Consequently, the horizontal snap is "converted" to a vertical snap.

The fix is to block starting the scroll snap animation on an axis if the user gesture involved zero delta in the direction of the axis.
Comment 1 Brent Fulgham 2015-05-18 15:58:29 PDT 
Created attachment 253349 [details]
Patch
Comment 2 WebKit Commit Bot 2015-05-18 16:00:29 PDT 
Attachment 253349 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Radar WebKit Bug Importer 2015-05-18 16:33:33 PDT 
<rdar://problem/21009187>
Comment 4 Radar WebKit Bug Importer 2015-05-18 16:33:35 PDT 
<rdar://problem/21009192>
Comment 5 Brent Fulgham 2015-05-19 12:40:14 PDT 
I don't have a good non-flaky automatic test for this. It's pretty easy to test manually.
Comment 6 Dean Jackson 2015-05-19 13:14:25 PDT 
Comment on attachment 253349 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

Oops.

> Source/WebCore/ChangeLog:14
> +        * platform/cocoa/ScrollController.mm:
> +        (WebCore::ScrollController::processWheelEventForScrollSnapOnAxis): Don't begin a scroll sn

This sentence does not e
Comment 7 Brent Fulgham 2015-05-19 14:05:09 PDT 
Comment on attachment 253349 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests (OOPS!).
> 
> Oops.

Oops!

>> Source/WebCore/ChangeLog:14
>> +        (WebCore::ScrollController::processWheelEventForScrollSnapOnAxis): Don't begin a scroll sn
> 
> This sentence does not e

Oops!
Comment 8 Brent Fulgham 2015-05-19 14:27:39 PDT 
Committed r184589: <http://trac.webkit.org/changeset/184589>