Bug 149681 – Latch does not clear when a scroll snap animation is triggered

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Brent Fulgham]

The blog post about the new scroll snap feature identified a bug in the latching logic.

Steps to reproduce:
1. Create a web page with two (or more) <div> elements holding content with scroll snap style turned on.
2. Perform a trackpad or mouse wheel move in the top element to trigger a scroll snap "glide" so that it animates to its second position.
3. Perform another trackpad or mouse wheel move (in the top element) to trigger a scroll snap "snap back" to the second position.
4. Move the mouse pointer to the lower <div> element.
5. Perform a trackpad or mouse wheel move in the bottom element to move the div content.

The bottom div should scroll (or scroll-snap), but the bug causes the top element to hold latch state. This causes the top div to move instead of the div the user is interacting with.

[Brent Fulgham]

<rdar://problem/22733922>

[Brent Fulgham]

Created attachment 262197 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 262197 [details]
Patch

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

> LayoutTests/platform/efl/TestExpectations:2618
> +tiled-drawing/scrolling/latched-div-with-scroll-snap.html [ Failure ]

Why not just skip this test in LayoutTests/TestExpectations and enable it in the mac-wk2 TestExpectations?

> LayoutTests/tiled-drawing/scrolling/latched-div-with-scroll-snap.html:153
> +        function scrollInSecondDiv()
> +        {
> +            debug("Testing that latch moves to bottom div:");
> +            var topDivTarget = document.getElementById("topTarget");
> +            divScrollPositionBeforeSecondaryMove = topDivTarget.scrollLeft;
> +
> +            var divTarget = document.getElementById("bottomTarget");
> +
> +            var windowPosition = locationInWindowCoordinates(divTarget);
> +            var startPosX = windowPosition.x + 0.5 * divTarget.clientWidth;
> +            var startPosY = windowPosition.y + 0.5 * divTarget.clientHeight;
> + 
> +            eventSender.mouseMoveTo(startPosX, startPosY);
> +            eventSender.mouseScrollByWithWheelAndMomentumPhases(-1, 0, 'began', 'none');
> +            eventSender.mouseScrollByWithWheelAndMomentumPhases(-1, 0, 'changed', 'none');
> +            eventSender.mouseScrollByWithWheelAndMomentumPhases(-1, 0, 'changed', 'none');
> +            eventSender.mouseScrollByWithWheelAndMomentumPhases(-1, 0, 'changed', 'none');
> +            eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, 'ended', 'none');
> +            eventSender.mouseScrollByWithWheelAndMomentumPhases(-1, 0, 'none', 'begin');
> +            eventSender.mouseScrollByWithWheelAndMomentumPhases(-1, 0, 'none', 'continue');
> +            eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, 'none', 'end');
> +            eventSender.callAfterScrollingCompletes(function() { return checkForSecondaryScrollGlide(); });           
> +        }
> +
> +        function checkForScrollSnap()
> +        {
> +            var divTarget = document.getElementById("topTarget");
> +
> +            var actualPosition = divTarget.scrollLeft;
> +
> +            // The div should have snapped back to the previous position
> +            if (actualPosition != divScrollPositionBeforeSnap)
> +                testFailed("div did not snap back to proper location for " + targetLabel +". Expected " + divScrollPositionBeforeSnap + ", but got " + actualPosition);
> +            else
> +                testPassed("div honored snap points.");
> +
> +            setTimeout(function() { scrollInSecondDiv() }, 0);
> +        }
> +
> +        function scrollSnapTest()
> +        {
> +            var divTarget = document.getElementById("topTarget");
> + 
> +            divScrollPositionBeforeSnap = divTarget.scrollLeft;
> +
> +            var windowPosition = locationInWindowCoordinates(divTarget);
> +            var startPosX = windowPosition.x + 0.5 * divTarget.clientWidth;
> +            var startPosY = windowPosition.y + 0.5 * divTarget.clientHeight;
> +
> +            eventSender.mouseMoveTo(startPosX, startPosY); // Make sure we are just outside the iFrame
> +            eventSender.mouseScrollByWithWheelAndMomentumPhases(-1, 0, 'began', 'none');
> +            eventSender.mouseScrollByWithWheelAndMomentumPhases(-1, 0, 'changed', 'none');
> +            eventSender.mouseScrollByWithWheelAndMomentumPhases(-1, 0, 'changed', 'none');
> +            eventSender.mouseScrollByWithWheelAndMomentumPhases(0, 0, 'ended', 'none');
> +            eventSender.callAfterScrollingCompletes(function() { return checkForScrollSnap(); });
> +        }
> +
> +        function checkForScrollGlide(targetLabel)
> +        {
> +            var divTarget = document.getElementById("topTarget");
> +
> +            var actualPosition = divTarget.scrollLeft;
> +            var expectedPosition = divTarget.clientWidth;
> +
> +            // The div should have scrolled (glided) to the next snap point.
> +            if (actualPosition == expectedPosition)
> +                testPassed("div scrolled to next window.");
> +            else
> +                testFailed("div did not honor snap points. Expected " + expectedPosition + ", but got " + actualPosition);

So much code!

> LayoutTests/tiled-drawing/scrolling/latched-div-with-scroll-snap.html:190
> +                message.innerHTML = "<p>This test is better run under DumpRenderTree.<br/>To manually test it, place the mouse pointer<br/>"

DumpRenderTree, not WTR?

[Brent Fulgham]

Comment on attachment 262197 [details]
Patch

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

>> LayoutTests/platform/efl/TestExpectations:2618
>> +tiled-drawing/scrolling/latched-div-with-scroll-snap.html [ Failure ]
> 
> Why not just skip this test in LayoutTests/TestExpectations and enable it in the mac-wk2 TestExpectations?

Oh, I don't even need to do that -- you already skipped it (and enabled in WK2) a long time ago!

>> LayoutTests/tiled-drawing/scrolling/latched-div-with-scroll-snap.html:153
>> +                testFailed("div did not honor snap points. Expected " + expectedPosition + ", but got " + actualPosition);
> 
> So much code!

I combined the glide gesture into a function, which helps. But there's not much way around the need to interact with two divs using different gestures to reproduce the problem.

>> LayoutTests/tiled-drawing/scrolling/latched-div-with-scroll-snap.html:190
>> +                message.innerHTML = "<p>This test is better run under DumpRenderTree.<br/>To manually test it, place the mouse pointer<br/>"
> 
> DumpRenderTree, not WTR?

Whoops! I'll fix.

[Build Bot]

Comment on attachment 262197 [details]
Patch

Attachment 262197 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/228979

New failing tests:
tiled-drawing/scrolling/fast-scroll-iframe-latched-select.html

[Build Bot]

Created attachment 262202 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Brent Fulgham]

(In reply to comment #6)
> Created attachment 262202 [details]
> Archive of layout-test-results from ews105 for mac-mavericks-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-wk2-ews.
> Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

Rats. It looks like <select> elements go through this same code, so I can't just blindly clear the latch.

[Brent Fulgham]

Created attachment 262264 [details]
Patch v2 (Only clear latching when scroll snap is started).

[Brent Fulgham]

The original patch would clear the latched state any time the user end gesture was encountered, which caused the latch to clear during momentum or other operations.

Instead, we only want to clear the latch when the scroll snap animation starts, since it hijacks the normal wheel handling that would clear the latch at the appropriate moment.

This updated patch passes all tests now.

[Brent Fulgham]

Committed r190423: <http://trac.webkit.org/changeset/190423>