Bug 146043 - CSS scroll snap: defining snap points on axis that does not scroll does not work properly
Summary: CSS scroll snap: defining snap points on axis that does not scroll does not w...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-06-16 20:46 PDT by Brent Fulgham
Modified: 2015-06-17 11:33 PDT (History)
11 users (show)

See Also:


Attachments
Patch (8.62 KB, patch)
2015-06-16 21:43 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (9.22 KB, patch)
2015-06-17 10:49 PDT, Brent Fulgham
simon.fraser: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (537.50 KB, application/zip)
2015-06-17 11:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (569.68 KB, application/zip)
2015-06-17 11:25 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2015-06-16 20:46:00 PDT
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.

Comment 1 Brent Fulgham 2015-06-16 21:30:39 PDT
<rdar://problem/20125511>
Comment 2 Brent Fulgham 2015-06-16 21:43:34 PDT
Created attachment 254999 [details]
Patch
Comment 3 Simon Fraser (smfr) 2015-06-17 09:19:32 PDT
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().
Comment 4 Brent Fulgham 2015-06-17 09:37:27 PDT
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.
Comment 5 Brent Fulgham 2015-06-17 10:49:32 PDT
Created attachment 255022 [details]
Patch
Comment 6 Build Bot 2015-06-17 11:21:56 PDT
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
Comment 7 Build Bot 2015-06-17 11:21:59 PDT
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
Comment 8 Build Bot 2015-06-17 11:25:31 PDT
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
Comment 9 Build Bot 2015-06-17 11:25:34 PDT
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
Comment 10 Brent Fulgham 2015-06-17 11:26:17 PDT
(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.
Comment 11 Brent Fulgham 2015-06-17 11:33:03 PDT
Committed r185658: <http://trac.webkit.org/changeset/185658>