1788029 - scroll-snap is broken after version 104 update

Status: VERIFIED FIXED

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

Description

[evromalarkey]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:104.0) Gecko/20100101 Firefox/104.0

Steps to reproduce:

Updated to Firefox 104.

On mobile there is a drawer hamburger menu on https://ui.newlogic.cz/ done via scroll-snap. Works in all major browsers without problem, except Firefox version 104+ (worked in prior versions). Broken on mobile and desktop.

This UI framework is used on multiple websites, and since Firefox 104 all these website have broken menu!

You can try this in https://github.com/lubomirblazekcz/scrollsnap-drawer this demo works in all major browsers and Firefox 103.

To see how is the snaping is buggy now I've created a demo branch here https://github.com/lubomirblazekcz/scrollsnap-drawer/tree/firefox104

Actual results:

Nothing.

Expected results:

Menu should be visible.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Layout: Scrolling and Overflow' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[evromalarkey]

Try also the https://developer.mozilla.org/en-US/docs/Web/CSS/scroll-snap-stop demo, scroll-snap-type: x mandatory; looks to be broken on mobile.

Comment 3

[Dan Robertson (:dlrobertson)]

Thanks for the bug report! I think this is a duplicate of bug 1783936. I've been able to reproduce the issue on beta and stable with the examples you provided. I have not been able to reproduce on nightly, can you confirm that you also cannot reproduce the issue on a build from https://nightly.mozilla.org?

Comment 4

[evromalarkey]

I've tried the nightly and unfortunetly no, if you try the scrollsnap-drawer demo on the master branch then it's not working. :/Drawer no longer opens, this works in all major browsers.

Comment 5

[Dan Robertson (:dlrobertson)]

(In reply to evromalarkey from comment #4)

I've tried the nightly and unfortunetly no, if you try the scrollsnap-drawer demo on the master branch then it's not working. :/Drawer no longer opens, this works in all major browsers.

Thanks, I can now reproduce the issue! I've reduced the regression rang to 307dd529e2a3dc6f6191a40d35c1c9e9fd94f727 to 1f99e4a5113539ba2b85689d04f66fd0261db0a8. I suspect bug 1530253 is the regressor.

Comment 6

[Dan Robertson (:dlrobertson)]

Confirmed bug 1530253 as the regressor. Bisected bug 1530253 patchset to D148863(cf0b9acadae8ded1c2c334f6b43b459317a4eb42).

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1530253

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: A simplified test case

I believe attaching file represents what the underlying problem of this bug is. When you open the file, you'll see on Firefox the initial scroll position isn't zero, on Chrome the initial scroll position is zero.

The underlying problem is here in the calculation of each snap candidate position for snap-align:end on X axis. The calculation is;

        inlineDirectionPosition.emplace(logicalTargetRect.IEnd(writingMode) -   
                                          containerISize);

The logicalTargetRect's IEnd is 21600 in app units and the containerISize is 79200 in app units, thus the result of the calculation is negative even though the container's wiring-mode is not RTL. We need to clamp the calculation into the scrollable range, i.e. in LTR cases it should be [0, ...], in RTL cases it should be [..., 0].

Comment 9

[Hiroyuki Ikezoe (:hiro)]

evromalarkey, thanks for reporting this bug. I could see the original issue you reported, but I can't see any issues in the MDN's scroll-snap-stop page. Can you describe more about the issue you saw in the page? It's probably a different issue than this bug. Thanks!

Comment 10

[evromalarkey]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)

evromalarkey, thanks for reporting this bug. I could see the original issue you reported, but I can't see any issues in the MDN's scroll-snap-stop page. Can you describe more about the issue you saw in the page? It's probably a different issue than this bug. Thanks!

The MDN scroll-snap-stop page was propably the bug with https://bugzilla.mozilla.org/show_bug.cgi?id=1783936, I thought it was related, since this started happening too after update.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

I believe this is a bug in Gecko. That's said the spec is unclear to me so I raised a spec issue; https://github.com/w3c/csswg-drafts/issues/7698

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Johannes Odland told me in the spec issue, it's spec-ed. :)

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1788029 - Clamp snap points inside the scroll range. r?#layout

The spec clearly describes this behavior;

https://drafts.csswg.org/css-scroll-snap/#unreachable

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/720b7fa0ff41
Clamp snap points inside the scroll range. r=emilio

Comment 15

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

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

Comment 16

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/720b7fa0ff41

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:hiro, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox105 to wontfix.

For more information, please visit auto_nag documentation.

Comment 18

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

Upstream PR merged by moz-wptsync-bot

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9293416 [details]
Bug 1788029 - Clamp snap points inside the scroll range. r?#layout

Beta/Release Uplift Approval Request

• User impact if declined: Scroll snap misbehaves
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Comment 0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively straight forward
• String changes made/needed: none
• Is Android affected?: Yes

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9293416 [details]
Bug 1788029 - Clamp snap points inside the scroll range. r?#layout

Approved for 105.0b9.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/93722cd445b0

Comment 22

[evromalarkey]

I am trying this in the current Nightly and I see the bug is still present. Is it really fixed? Or is it not public yet?

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Yeah, right. there's something I've been missing. I will file a follow-up bug once after I identified the problem.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

That's probably due to bug 1772272.

Comment 25

[Peter Magyari (Desktop QA)]

I have verified the fix for the initial scroll position issue using the test-case from Comment 8 on Firefox Nightly 106.0a1(20220911214030) and Firefox Beta 105.0b9(20220908185813)

Hiroyuki, will you file a new bug regarding the original issue or will that be tracked in Bug 1772272?

Comment 26

[Hiroyuki Ikezoe (:hiro)]

(In reply to Peter_M from comment #25)

I have verified the fix for the initial scroll position issue using the test-case from Comment 8 on Firefox Nightly 106.0a1(20220911214030) and Firefox Beta 105.0b9(20220908185813)

Hiroyuki, will you file a new bug regarding the original issue or will that be tracked in Bug 1772272?

Yeah, as far as I can tell the rest of the issue why the original test case doesn't work is bug 1772272. And actually it's the case actually what I was concerned in bug 1772272 comment 0. The original test case changes transform property and at the same time it also sets transition: transform so that at the time the computed style value of the transform is basically unchanged, thus it won't snap at all. I wonder how Chrome handles this case, need to investigate.