1905247 - scroller with 'scroll-snap-align' on descendants refuses to scroll until you give it extremely out-of-bounds scrollLeft values

Status: RESOLVED FIXED

(Core :: Layout: Scrolling and Overflow, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:

1. Load attached testcase.
2. Try clicking the buttons to see which ones cause the scrollable div to advance its position.

ACTUAL RESULTS:
The buttons up-to-and-including 200 are ineffective. You have to set scrollLeft to something beyond 200 in order to get the div to scroll.

EXPECTED RESULTS:
The buttons at 51 and greater should all generate a scroll.

Firefox gives ACTUAL REULTS (and does so in Nightlies going back as far as we have support for scroll-snap-align from bug 1373835).
Chromium and WebKit give EXPECTED RESULTS.

I'm not an expert on scroll-snapping; "expected results" are based on comparison to other browsers and my intuition based on the fact that the scrollable area has a scrollLeftMax of 100 (which is the scrollLeft that we arrive at when Firefox does actually scroll here).

This testcase is reduced from the apple store website example in bug 1850485, and I think this might be the root of the issue there. The scrolling in that bug happens dynamically via several steps (starting with a transitioned transform), but I suspect it ends up tripping this bug towards the end of the scripted scroll operation and inadvertently resetting the scroll position as a result.

hiro, could you take a look?

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screencast showing ACTUAL RESULTS in Firefox vs. EXPECTED RESULTS in Chrome (clicking buttons until I find one that changes the scroll position)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (simplified a bit)

This is the same as testcase 1, but with some unnecessary complexity removed. Now this uses inline-block elements instead of flexbox, and I've removed an unnecessary layer of nesting.

actual/expected results are still as-described in comment 0.

Comment 3

[Daniel Holbert [:dholbert]]

It seems like we're applying the scroll-snap-align:start scrolling restriction too strictly.

We seem to be refusing to advance the scroll position unless we're advancing it by a particular threshold, where the threshold is: enough to make the second scroll-snap-align:start item flush with the start of the scroller (which is the 200px vs. 201px boundary in this testcase; that explains why that distance is the magic number for us)

This makes sense in general, but it doesn't seem to make sense in this specific case where our scrollable overflow (and scrollLeftMax) is smaller than that threshold, and so we literally can't scroll by 200px or by 201px or anything close to it. The fact that we require a scrollLeft = 201px in order to actually be able to advance the scroll position to a much smaller value seems wrong.

It seems like other browsers have special handling for this case; it looks like their scroll threshold is min(scroll-snap-align-threshold, 0.5*scrollLeftMax) (i.e. min(200px,50px) for this testcase) -- or something like that. This means that scrolling to the scrollLeftMax value (or something more than halfway to it) -- e.g. anything from 51px-100px in these testcases -- will get you to the maximum scroll position in those browsers.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 3

Here's a testcase with just 2 items of different sizes, where the first item is 610px wide, and scroller.scrollLeftMax is 10 (i.e. we've only got 10px of scrollable overflow).

Chrome snaps to the leftmost scroll position (0) if you set scrollLeft to 5 or below, and to the rightmost scroll position (10) if you set scrollLeft to 6 or above.

Firefox snaps to the leftmost scroll position (0) if you set scrollLeft 305 or below, and to the rightmost scroll position (10) if you set scrollLeft to 306 or above.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Thanks Daniel for the detailed analysis. You are totally right.

We've done for such clamping for scroll-snap-align: end and scroll-snap-align: center, but somehow I missed it for scroll-snap-align: start. :/

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1905247 - Convert logicalScrollRange into `writingMode`. r?emilio

logicalScrollRange is used to clamp each candidate snap position inside the
scroll range. If we used aWritingModeOnScroller for logicalScrollRange,
clamping will be done in a wrong axis in some cases.

With the next commit but without this change running
scroll-snap-writing-mode-000.html [1] causes a lot of
"ASSERTION: writing-mode mismatch" warnings.

[1] https://searchfox.org/mozilla-central/rev/16a5f0e868a01d3f3a4a7364206bef2a64e80735/testing/web-platform/tests/css/css-scroll-snap/snap-after-initial-layout/scroll-snap-writing-mode-000.html

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1905247 - Clamp `scroll-snap-align: start` snap points inside the scroll range. r?emilio

This change is basically equivalent with what we did for bug 1788029 but for
scroll-snap-align: start cases.

The test case was originally written by Daniel Holbert.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/667e49b7e734
Convert logicalScrollRange into `writingMode`. r=emilio
https://hg.mozilla.org/integration/autoland/rev/9345fa484aa6
Clamp `scroll-snap-align: start` snap points inside the scroll range. r=emilio

Comment 9

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/46960 for changes under testing/web-platform/tests

Comment 10

[tszentpeteri]

Backed out for causing wpt failures on unreachable-snap-positions-003.html.

• Backout link
• Push with failures
• Failure Log:

[task 2024-07-02T08:05:21.452Z] 08:05:21     INFO - TEST-PASS | /css/css-scroll-snap/unreachable-snap-positions-002.html | Unreachable snap point with `scroll-snap-align: center` in RTL 
[task 2024-07-02T08:05:21.452Z] 08:05:21     INFO - TEST-UNEXPECTED-FAIL | /css/css-scroll-snap/unreachable-snap-positions-003.html | Snaps to the positions defined by the element as much as possible - assert_equals: expected 10 but got 0
[task 2024-07-02T08:05:21.452Z] 08:05:21     INFO - @http://web-platform.test:8000/css/css-scroll-snap/unreachable-snap-positions-003.html:36:16
[task 2024-07-02T08:05:21.452Z] 08:05:21     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:2622:25
[task 2024-07-02T08:05:21.452Z] 08:05:21     INFO - test@http://web-platform.test:8000/resources/testharness.js:633:30
[task 2024-07-02T08:05:21.452Z] 08:05:21     INFO - @http://web-platform.test:8000/css/css-scroll-snap/unreachable-snap-positions-003.html:34:5
[task 2024-07-02T08:05:21.518Z] 08:05:21     INFO - TEST-OK | /css/css-scroll-snap/unreachable-snap-positions-003.html | took 546ms
[task 2024-07-02T08:05:21.894Z] 08:05:21     INFO - STDOUT: cleanup aborted: Unable to remount device
[task 2024-07-02T08:05:22.013Z] 08:05:22     INFO - STDOUT: cleanup aborted: Unable to remount device
[task 2024-07-02T08:05:22.013Z] 08:05:22     INFO - Closing logging queue
[task 2024-07-02T08:05:22.014Z] 08:05:22     INFO - queue closed
[task 2024-07-02T08:05:22.031Z] 08:05:22     INFO - Setting up ssl
[task 2024-07-02T08:05:22.221Z] 08:05:22     INFO - certutil | b''
[task 2024-07-02T08:05:22.354Z] 08:05:22     INFO - certutil | b''
[task 2024-07-02T08:05:22.362Z] 08:05:22     INFO - certutil | b'\nCertificate Nickname                                         Trust Attributes\n                                                             SSL,S/MIME,JAR/XPI\n\nweb-platform-tests                                           CT,, \n'
[task 2024-07-02T08:05:22.899Z] 08:05:22     INFO - adb Granting important runtime permissions to org.mozilla.geckoview.test_runner
[task 2024-07-02T08:05:24.090Z] 08:05:24     INFO - adb launch_application: am start -W -n org.mozilla.geckoview.test_runner/org.mozilla.geckoview.test_runner.TestRunnerActivity -a android.intent.action.MAIN --es env0 MOZ_CRASHREPORTER=1 --es env1 MOZ_CRASHREPORTER_NO_REPORT=1 --es env2 MOZ_CRASHREPORTER_SHUTDOWN=1 --es env3 MOZ_HIDE_RESULTS_TABLE=1 --es env4 MOZ_IN_AUTOMATION=1 --es env5 MOZ_LOG=signaling:3,mtransport:4,DataChannel:4,jsep:4 --es env6 R_LOG_LEVEL=6 --es env7 R_LOG_DESTINATION=stderr --es env8 R_LOG_VERBOSE=1 --es env9 MOZ_PROCESS_LOG=/tmp/tmpt8k4z9fhpidlog --es env10 MOZ_DISABLE_NONLOCAL_CONNECTIONS=1 --es arg0 -no-remote --es arg1 -profile --es arg2 /data/local/tmp/test_root/profile --es arg3 --marionette --es arg4 about:blank
[task 2024-07-02T08:05:25.614Z] 08:05:25     INFO - Starting runner
[task 2024-07-02T08:05:26.463Z] 08:05:26     INFO - TEST-START | /css/css-scroll-snap/unreachable-snap-positions-004.html

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 12

[Hiroyuki Ikezoe (:hiro)]

The max-width: 800px element wasn't laid out on mobile as expected. I did add initial-scale=0.5 meta viewport to make the test work on mobile.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed6df9c57c61
Convert logicalScrollRange into `writingMode`. r=emilio
https://hg.mozilla.org/integration/autoland/rev/4734497618a2
Clamp `scroll-snap-align: start` snap points inside the scroll range. r=emilio

Comment 14

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/ed6df9c57c61
https://hg.mozilla.org/mozilla-central/rev/4734497618a2

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot