Closed Bug 1551806 Opened 1 year ago Closed 1 year ago

Don't try to snap somewhere when there is no valid snap point exists

Categories

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

68 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- unaffected
firefox67 --- unaffected
firefox67.0.1 --- unaffected
firefox68 + fixed

People

(Reporter: jsnajdr, Assigned: hiro)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(1 file)

Steps to reproduce:
Open the demo page at https://jsnajdr.github.io/ff-snap.html

It contains a carousel of "hosting service plans" that can be scrolled horizontally either by mouse or touchpad, or with the "left" and "right" buttons that set the scroller element's contentLeft property.

Issue #1:
When scrolling with Mac touchpad or Magic Mouse, the carousel snaps back to the original position instead of scrolling. See this screen capture video:

https://cld.wthms.co/MLmUVf

It depends a lot on how fast I do the scroll gesture.

Issue #2:
If the carousel element has a 'scroll-snap-type: x mandatory' style, and the plan elements don't have 'scroll-snap-align' style set, then scrolling with the buttons doesn't work at all. The 'el.scrollLeft = x' assignment doesn't have any affect.

Test on the demo page by unchecking the "Set scroll-snap-align on plan elements" checkbox and click on the left/right buttons. Nothing will happen.

I agree that not setting 'scroll-snap-align' is a weird situation, but we managed to arrive at it (in a bit more complex incarnation) in a production app that works correctly in Safari and Chrome, but is broken in Firefox.

Issue #3:
When clicking the left/right buttons, the 'scrollLeft' transition is animated: it's advanced in several steps inside requestAnimationFrame callbacks. However, after clicking, the scroll is not animated. It happens all at once. Chrome behaves the same way. But in Safari, the animation works.

Regressed by: 1531228

(In reply to Jarda Snajdr [:jsnajdr] from comment #0)

Steps to reproduce:
Open the demo page at https://jsnajdr.github.io/ff-snap.html

It contains a carousel of "hosting service plans" that can be scrolled horizontally either by mouse or touchpad, or with the "left" and "right" buttons that set the scroller element's contentLeft property.

Issue #1:
When scrolling with Mac touchpad or Magic Mouse, the carousel snaps back to the original position instead of scrolling. See this screen capture video:

https://cld.wthms.co/MLmUVf

It depends a lot on how fast I do the scroll gesture.

This sounds like the same issue as bug 1551801.

Issue #3:
When clicking the left/right buttons, the 'scrollLeft' transition is animated: it's advanced in several steps inside requestAnimationFrame callbacks. However, after clicking, the scroll is not animated. It happens all at once. Chrome behaves the same way. But in Safari, the animation works.

I think this is the right behavior, it's a bug in Safari. Setting scrollLeft should snap to the snap position, IIUC.

Issue #2:
If the carousel element has a 'scroll-snap-type: x mandatory' style, and the plan elements don't have 'scroll-snap-align' style set, then scrolling with the buttons doesn't work at all. The 'el.scrollLeft = x' assignment doesn't have any affect.

Test on the demo page by unchecking the "Set scroll-snap-align on plan elements" checkbox and click on the left/right buttons. Nothing will happen.

I agree that not setting 'scroll-snap-align' is a weird situation, but we managed to arrive at it (in a bit more complex incarnation) in a production app that works correctly in Safari and Chrome, but is broken in Firefox.

I will check the spec in such cases whether we should be able to scroll arbitrary positions or not.

Flags: needinfo?(hikezoe)

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

Test on the demo page by unchecking the "Set scroll-snap-align on plan elements" checkbox and click on the left/right buttons. Nothing will happen.

I agree that not setting 'scroll-snap-align' is a weird situation, but we managed to arrive at it (in a bit more complex incarnation) in a production app that works correctly in Safari and Chrome, but is broken in Firefox.

I will check the spec in such cases whether we should be able to scroll arbitrary positions or not.

Ok, great. It's a bug in our implementation. From the definition of the mandatory in the spec;

If a valid snap position exists then the scroll container must snap at the termination of a scroll (if none exist then no snapping occurs).

Would you mind if I re-use this bug only for the issue?

Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe)
Priority: -- → P2

I think this is the right behavior, it's a bug in Safari. Setting scrollLeft should snap to the snap position, IIUC.

That might be open to multiple interpretations. When specifying snap strictness, the spec [1] says things like:

the scroll container is required to be snapped to a snap position when there are no active scrolling operations.

or

the scroll container may snap to a snap position at the termination of a scroll,

When there is rAF loop that sets scrollLeft, does that mean that there is "active scrolling operation" in progress? Safari apparently does something like:

el.onScrollChange( debounce( snapToPosition, someTimeout ) )

I.e., it sets the scrollLeft exactly as requested and does the snapping only after the changes stop coming ("termination of a scroll").

[1] https://drafts.csswg.org/css-scroll-snap-1/#snap-strictness

Would you mind if I re-use this bug only for the issue?

No problem. The first issue is already covered by bug 1551801, although you commented there you can't reproduce the issue? Maybe my demo shared in this bug could help?

I think however that performing the scrollLeft animation correctly even in presence of snap points is a valid and desirable feature request. Currently we work around that (in Chrome, too) by removing the snap CSS classes before animating and adding them back after. Should I file a new issue for that?

Thank you!

(In reply to Jarda Snajdr [:jsnajdr] from comment #3)

That might be open to multiple interpretations. When specifying snap strictness, the spec [1] says things like:

the scroll container is required to be snapped to a snap position when there are no active scrolling operations.

or

the scroll container may snap to a snap position at the termination of a scroll,

When there is rAF loop that sets scrollLeft, does that mean that there is "active scrolling operation" in progress? Safari apparently does something like:

I don't think so. Setting scrollLeft is one of programatic scroll operations I believe. From the spec;

the author can request a particular bias for the scrollport to land on a snap position after scrolling operations (including programmatic scrolls such as the scrollTo() method).

Given that the result of the web-platform tests Safari doesn't do snap operations on any programatic scroll calls (most of the tests use scrollTo).

(In reply to Jarda Snajdr [:jsnajdr] from comment #4)

Would you mind if I re-use this bug only for the issue?

No problem. The first issue is already covered by bug 1551801, although you commented there you can't reproduce the issue? Maybe my demo shared in this bug could help?

Unfortunately it doesn't help, since as far as I can tell, it's a MaxOSX only issue, and I don't have any MacOSX machines.

I think however that performing the scrollLeft animation correctly even in presence of snap points is a valid and desirable feature request. Currently we work around that (in Chrome, too) by removing the snap CSS classes before animating and adding them back after. Should I file a new issue for that?

If you believe it's a desirable feature in terms of web development you can file an issue on the spec, here https://github.com/w3c/csswg-drafts/issues. That's said, I am not sure I understand what the scrollLeft operations in the test case try to do, it seems to me that using smooth scroll will result similar effects. Something like this;

carousel.scrollBy({ left: 100, behavior: 'smooth' });

Summary: With scroll-snap-v1 enabled, scrolling on macOS and setting scrollLeft doesn't work as expected → Don't try to snap somewhere when there is no valid snap point exists
OS: macOS → All
Hardware: Desktop → All

Given that the result of the web-platform tests Safari doesn't do snap operations on any programatic scroll calls (most of the tests use scrollTo).

That's right, I verified that Safari ignores the snap points during programmatic scroll. That's the only reason why our scroll animation works there.

I am not sure I understand what the scrollLeft operations in the test case try to do, it seems to me that using smooth scroll will result similar effects.

The browser support for smooth scroll is not great though. Safari and IE/Edge don't have it. The scrollLeft operations in the test case basically implement a polyfill for smooth scrolling. That polyfill unfortunately doesn't work with scroll snap.

I think that if we switch to element.scrollTo({behavior:'smooth'}) where we can and polyfill where we can't, we can make our Plans Carousel perfect on Firefox and near-perfect on other browsers. Thanks for a helpful conversation and a quick patch!

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bf19271bb2f96d4db3b00247d18ed812adf4939

There was a bug on RTL scroll container, and the bug makes a test case in scroll-snap-type-on-root-element.html failure. I filed bug 1552089 for the failure and will fix soon.

Here is a new try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3072624f7118436ace55eabdbc19bbdd1cf15d68

(In reply to Jarda Snajdr [:jsnajdr] from comment #7)

I think that if we switch to element.scrollTo({behavior:'smooth'}) where we can and polyfill where we can't, we can make our Plans Carousel perfect on Firefox and near-perfect on other browsers.

That's good to hear. :) Thanks for filing this bug, that was really great to know this issue before shipping the new scroll snap. :)

Looking at the patch and seeing that the HasSnapPositions method checks both X and Y axes, I wonder: if the snap points are defined, but on a different axis, will your patch still work?

.parent { scroll-snap-type: x mandatory } /* snap on the inline axis /
.child { scroll-snap-align: start none } /
no snap points on the inline axis, only block */

Good point! It doesn't work at all. :/ Thank you!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb88aa7f79a480e38d67b682c237f01f1171bf16

Also your test case noticed me that the current implementation doesn't work for cases where a scroll container has scroll-snap-type: x mandatory and its children have scroll-snap-align: none start but the scroll container is only able to scroll y-axis by user interactions. It's bit hard to fix these cases since even if the container is not scrollable in a certain direction, say overflow-x: none, scrollTo can be moved to arbitrary scroll position. I will file a new bug for this issue, probably we will ship the new scroll snap without fixing the issue.

From the spec [1];
If a valid snap position exists then the scroll container must snap at the
termination of a scroll (if none exist then no snapping occurs).

Both of test cases in this commit fail without this change.

[1] https://drafts.csswg.org/css-scroll-snap-1/#valdef-scroll-snap-type-mandatory

Has Regression Range: --- → yes
Has STR: --- → yes
Keywords: regression
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ef3c6d8bb498
Don't try to snap if there is no valid snap positions for the scroll-snap v1 implementation. r=botond
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16910 for changes under testing/web-platform/tests
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.