Bug 146043 – CSS scroll snap: defining snap points on axis that does not scroll does not work properly

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Brent Fulgham]

We get incorrect behavior if we define a document with a horizontally-scrolling container, but only define scroll-snap-points in the Y-axis.

In this configuration, the scroll-snap implementation builds both horizontal and vertical snap points. Since there are no snap points defined on the horizontal axis, we end up with only the default [0, max] snap points. This causes us to snap all the way to the far end of the container.

The fix is probabaly in AxisScrollSnapOffsets.cpp in updateFromStyle. I think that cases where there are no defined scroll-snap points, we shouldn't create a set of points that only contains the beginning and end points of the container.


[Brent Fulgham]

<rdar://problem/20125511>

[Brent Fulgham]

Created attachment 254999 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 254999 [details]
Patch

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

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:119
> +    if (snapOffsets.size() == 1 && !snapOffsets.first()) {
> +        snapOffsets.clear();
> +        return;
> +    }

Doesn't this miss the case where the content has a valid offset at 0?

> LayoutTests/css3/scroll-snap/scroll-snap-mismatch.html:43
> +            debug("Scroll-snap offsets for 'badHorizontalTarget': " + window.internals.scrollSnapOffsets(badHorizontalTarget));
> +
> +            var horizontalTarget = document.getElementById('horizontalTarget');
> +            debug("Scroll-snap offsets for 'horizontalTarget': " + window.internals.scrollSnapOffsets(horizontalTarget));

I don't think you should really use debug() for actual output. You should be using shouldBe().

[Brent Fulgham]

Comment on attachment 254999 [details]
Patch

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

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:119
>> +    }
> 
> Doesn't this miss the case where the content has a valid offset at 0?

I guess you could have some poorly authored markup where you really did specify a single offset at 0. I'll revise this to handle that case -- though the end result will be the gross behavior Jon found.

>> LayoutTests/css3/scroll-snap/scroll-snap-mismatch.html:43
>> +            debug("Scroll-snap offsets for 'horizontalTarget': " + window.internals.scrollSnapOffsets(horizontalTarget));
> 
> I don't think you should really use debug() for actual output. You should be using shouldBe().

Oh! I'll switch to that.

[Brent Fulgham]

Created attachment 255022 [details]
Patch

[Build Bot]

Comment on attachment 255022 [details]
Patch

Attachment 255022 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5163604022132736

New failing tests:
css3/scroll-snap/scroll-snap-mismatch.html

[Build Bot]

Created attachment 255025 [details]
Archive of layout-test-results from ews100 for mac-mavericks

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

[Build Bot]

Comment on attachment 255022 [details]
Patch

Attachment 255022 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6072427691900928

New failing tests:
css3/scroll-snap/scroll-snap-mismatch.html

[Build Bot]

Created attachment 255026 [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 #9)
> Created attachment 255026 [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

I used improper syntax for "shouldBe()". I'll fix while landing.

[Brent Fulgham]

Committed r185658: <http://trac.webkit.org/changeset/185658>