1530253 - Support re-snapping for scroll snap

Status: RESOLVED FIXED

(Core :: Layout: Scrolling and Overflow, enhancement, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

The spec clearly says;

If the content or layout of the document changes (e.g. content is added, moved, deleted, resized) such that the content of a snapport changes, the UA must re-evaluate the resulting scroll position, and re-snap if required. If the scroll container was snapped before the content change and that same snap position still exists (e.g. its associated element was not deleted), the scroll container must be re-snapped to that same snap position after the content change.

But as far as I can tell Chrome doesn't re-snap yet, so I am going to defer this feature for now.

Comment 1

[Cameron McCormack (:heycam)]

Per https://twitter.com/tabatkins/status/1235329611925540864 Chrome is supporting this now.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Emilio has already raised a spec issue that is that whether re-snap is prior than scroll-anchoring.
https://github.com/w3c/csswg-drafts/issues/4830

Comment 3

[Hristo Dimitrov]

The spec seems to be settled as of recently based on the issue above.

Comment 4

[Bruno Stasse]

Both Chrome and Safari have been supporting this part of the spec for quite some time now. Since the issue with scroll-anchoring has been resolved in the spec, it would be great to see this come to Firefox as well.

That would remove the need to manually (and imperfectly) scroll back to the snapping point after detecting a layout change (or not doing it at all and exposing the user to undesirable situations).

Comment 5

[Alex Thayer [:alexical] (she/her)]

How difficult is this to implement, Hiro? I think this would greatly simplify our manual scroll snapping inside devtools.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

In most cases where re-snapping should happen the implementation shouldn't be hard, we can just re-evaluate snap points when it's necessary (e.g. the scrollport size is changed). One case it's unclear to me is that if there's an on-going async scrolling, I'd suppose no re-snap should happen during the async scrolling, but I am not sure, I'd hope there's at least one wpt for the case.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Note that we have a plan to do this in this H2, FWIW.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

I am going to summarize about re-snapping feature, what the spec definition is, what we need to do is.

• We need to maintain the snap target(s) where we snapped on the last scroll
  operation
• It's possible that there are multiple snap targets at the same position
• It's also possible that the last snap point is associated with two different
  snap targets, for example, the point on X Axis is associated to a
  scroll-snap-align: none start element, on Y Axis it's associated to a
  scroll-snap-align: start none element
• In the above case if snapping to the combination point (x, y) results both
  elements are outside of the scrollport, we need to choose a snap point on
  either X or Y axis, then re-evaluate the nearest snap point from there,
  there's a wpt for this case
• If one of the last snapped targets has focused when re-snapping, we need to
  snap to the focused one
• The spec doesn't define priories if there's no focused element among the last
  snapped targets so that we can choose an arbitrary re-snap point
• Re-snapping needs to supercede scroll anchoring,
• It's possible that snapping happens in APZ, thus we need to inform the last
  snapped targets done by APZ to the main-thread
• There are also possibilities that an async scroll operation with snap points
  is triggered on the main-thread, e.g. async Element.scrollBy calls, thus
  we need to inform the snap points calculated on the main-thread into APZ as well

I am going to defer the last two bullet points into one or two follow-up bugs.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1530253 - Introduce a new struct wrapping various varibles in CalcSnapPoints. r?botond

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1530253 - Assign an id to each scroll snap target for tracking the last snapped element. r?botond!,emilio!

Depends on D148855

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1530253 - Make GetSnapPointForDestination return the last snapped target id(s). r?botond

Depends on D148856

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1530253 - Replace internal ScrollFrameHelper::ScrollTo calls with ScrollFrameHelper::ScrollToWithOrigin. r?botond

There are some call sites of ScrollTo involving will a GetSnapPointForDestination
call. For re-snapping, we need to inform not only the snap point but also the
snap target id(s). To do it, in subsequent patches in this commit series, we will
change only ScrollToWithOrigin arguments rather than both ScrollTo and
ScrollToWithOrigin.

Depends on D148857

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1530253 - Add a wrapper struct representing various arguments for ScrollToWithOrigin. r?botond

Depends on D148858

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1530253 - Give the snap target ids to ScrollToWithOrigin. r?botond

The snap targt ids will be the last snap target ids we use for re-snapping.

Depends on D148859

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1530253 - Store the last snap target ids in the case of instant scroll oparations on the main-thread. r?botond!,emilio!

Depends on D148860

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1530253 - Implement the logic to choose the best snap point for re-snap. r?botond

Depends on D148861

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1530253 - Trigger re-snap on reflows and in RecomputePosition. r?botond!,emilio!

The basic machinery to trigger re-snap is same as what scroll anchoring does,
queueing a triggering request and flush the queued requests after reflows
finished.

One caveat is that when the last scroll operation snapped to a point, we need to
clobber scroll anchoring.

Depends on D148862

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1530253 - Trigger resnap on scroll-snap-type or scroll-snap-align changes. r?emilio

Depends on D148863

Comment 19

[Hiroyuki Ikezoe (:hiro)]

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

• It's possible that snapping happens in APZ, thus we need to inform the last
  snapped targets done by APZ to the main-thread
• There are also possibilities that an async scroll operation with snap points
  is triggered on the main-thread, e.g. async Element.scrollBy calls, thus
  we need to inform the snap points calculated on the main-thread into APZ as well

I am going to defer the last two bullet points into one or two follow-up bugs.

Filed bug 1774537 for these two.

Comment 20

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

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

Comment 21

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/440819c40a31
https://hg.mozilla.org/mozilla-central/rev/e5c5cef3d9a9
https://hg.mozilla.org/mozilla-central/rev/7fd7115f1456
https://hg.mozilla.org/mozilla-central/rev/c081156c1e7d
https://hg.mozilla.org/mozilla-central/rev/8d6b0fdd42be
https://hg.mozilla.org/mozilla-central/rev/2d636857c002
https://hg.mozilla.org/mozilla-central/rev/8ca5a9f803a2
https://hg.mozilla.org/mozilla-central/rev/15df89bfc0ff
https://hg.mozilla.org/mozilla-central/rev/cf0b9acadae8
https://hg.mozilla.org/mozilla-central/rev/ab55b2968618

Comment 22

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

Upstream PR merged by moz-wptsync-bot

Comment 23

[Šime Vidas]

This change broke my CSS scroll snap user styles for Twitter’s timeline.

I currently apply the following user styles to the URL https://twitter.com/home via the Stylus browser extension:

html {
    scroll-snap-type: y proximity !important;
}

[aria-label="Timeline: Your Home Timeline"] article, [role="progressbar"] {
    scroll-snap-align: start !important;
}

[aria-label="New Tweets are available."] {
    display: none !important;
}

[aria-label="Home timeline"] > :first-child {
    position: static !important;
    scroll-snap-align: start !important;
}

I have recorded a video of the issue and posted it on YouTube: https://www.youtube.com/watch?v=6RodhbOVy90

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Thanks Šime for reporting the issue. It looks like you are trying to scroll by mouse wheel or some such? It realized me that there's an issue that when a new mouse wheel started, it won't clobber the last snap target id(s) so that if new contents get added, it will snap to the original position.

I am going to address the issue in bug 1774537. If it takes too much time, we will back out patches for this bug.

Comment 25

[Šime Vidas]

Sorry for not mentioning this detail. When I try to scroll with the Spacebar key, nothing happens (the page doesn’t scroll at all), and when I try to scroll with the laptop’s trackpad (vertical two-finger swipe), I get the glitchy behavior seen in the video that I linked above.

Comment 26

[Dianna Smith [:diannaS]]

:hiro do you think this needs a relnote for fx104?

Comment 27

[Hiroyuki Ikezoe (:hiro)]

I was actually unsure whether this kind of changes should be noted in release notes or not, so I asked some folks, our conclusion is, given that there's a spec section about this feature, so that it's probably worth noting in the release note.

Note that bug 1312165 is somewhat relevant with this bug, which was fixed in 103 cycle, I believe it should be noted in the release note.

Comment 28

[Šime Vidas]

As I wrote in comment 23 above, I use the Stylus add-on to add CSS Scroll Snap to the Twitter timeline. I’m testing in Firefox Nightly, and the new re-snapping behavior is making it impossible to view the content of tweets that are taller than my viewport.

I have recorded a video of the issue here: https://www.youtube.com/watch?v=bSJyg7DKqAc

This happens when I scroll using my laptop’s trackpad or the Space bar key.

The user styles that I add to the Twitter timeline:

html {
    scroll-snap-type: y proximity !important;
}

article {
    scroll-snap-align: start !important;
    scroll-margin: 53px !important;
}

As you can see, the code couldn’t be simpler. I apply proximity snapping to the page and then snap to the start of the tweets in the timeline (which are <article> elements). The scroll margin is there because of Twitter’s sticky header.

I am not sure why Firefox is re-snapping so aggressively (as my linked video shows). Maybe Twitter is mutating the DOM too often (due to the virtual scroller). If possible, I would like to be able to *disable re-snapping. I prefer the behavior in Firefox release, which doesn’t have re-snapping yet.

Comment 29

[Botond Ballo [:botond]]

Šime, I assume you tested in the 2022-07-16 nightly, which is the first to include the fix for bug 1774537?

If so, could you kindly file comment 28 as a new bug, and mark it as "regressed by" this one?

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Here is a description about re-snapping for the rel note.

"Firefox now supports re-snapping, it tries to keep the last snap position after any content/layout changes."