[css-scroll-anchoring] Should not apply anchoring adjustments from scroll event handlers

[emilio]

Most of the most annoying scroll anchoring issues I've fixed on Firefox over the last few months are due to the adjustments accidentally triggered from scroll event listeners. Some examples off the top of my head:

• https://bugzilla.mozilla.org/show_bug.cgi?id=1568778
• https://bugzilla.mozilla.org/show_bug.cgi?id=1543599

These adjustments trigger other scroll event listeners, which in turn generally either confuse the page or let the page burn a lot of CPU for no good reason.

The spec has the concept of suppression triggers to prevent most of these. Those are mostly heuristics, and are not equally implemented across browser.

In https://bugzilla.mozilla.org/attachment.cgi?id=9087497 I came up with a reduced test-case for something that we saw in the wild, and for which we don't want to suppress the adjustment in the more general case of it not being triggered by a scroll event listener. The test-case locks up scrolling in Firefox and Chrome once you scroll to the bottom.

Other than not doing scroll anchoring adjustments every time anyone updates layout (which may or may not be implementable), the other reasonable solution I can think of is to not adjust scroll positions of an element if we're processing an scroll event listener for that target.

That's what I'm going to implement in https://bugzilla.mozilla.org/show_bug.cgi?id=1561450, I think, and I'm going to investigate removing other suppression triggers which should be unnecessary after that change as followup work.

cc @skobes @eqrion @dholbert

[css-meeting-bot]

The CSS Working Group just discussed Scroll Anchoring.

The full IRC log of that discussion

<fantasai> Topic: Scroll Anchoring
<myles> emilio: Is this microphone on?
<astearns> github: https://github.com//issues/4239
<myles> emilio: The spec sepcifices a few situations where if a given style chagne happens, you don't apply any scrolling adjustments in thecontainer. This is done to avoid manual sticky effects. This is done to prevent them to breaking.
<myles> emilio: even with this heuristic, we have still found either broken pages that for example, once you scroll to the bottom you can't scroll back up, or they do things that in the regular cases you want scroll anchoring to apply, or pages that get burn a bunch of CPU in infinite scroll event listeners because scrolling triggers more scrolling listeners. This is possible in both chrome and firefox.
<myles> emilio: What I did to fix it is to never apply any adjustment for scroll anchoring if you are processing a scroll event listener for htat node.
<myles> emilio: When you fire the scroll event listener, you remember the target of the event, and if the element matches, you don't re-fire
<myles> emilio: this is mainly to prevent bad scroll events. I'm just interested in feedback from implementors
<myles> <missed>: Chrome implementors are in agreement
<myles> emilio: Ian Scopes is no longer working on this
<myles> eae: No one is working on it now
<myles> chrishtr: What about it are you itnerested in?
<heycam> s/Scopes/Skobes/
<TabAtkins> s/Ian Skobes/Steve Kobes/
<heycam> s/Ian Skobes/Steve Kobes/
<myles> emilio: scroll anchoring restores positions so the offset relative to the scroller is preserved. There are cases where that's undesirable, or break,s if the page is doing stuff like scroll effects. So if you change a static position thing to be below, the anchor node goes up, and then you need another scroll event, and the page realizes that the thing is no longer sticking to the viewport, et.c
<myles> emilio: Chrome fixed this by, when changing from in-flow to out-of-flow, don't do the scroll adjustment.
<myles> eae: Yes, that's how it works today.
<myles> emilio: Even with this heuristic, pages burn CPU by firing infinite scroll events, which is not great. Pages that get locked up on scrolling if you get to the bottom.
<myles> emilio: Those cases are not easy to identify generically, because a particular style change happened. The style didn't change, it's a case that scrolling wants to fix. The only thing that's happening is a scroll event listener. Instead of Chrome's heuristics, it would be simpler to not adjust scroll offsets if you're doing scroll anchoring if it's happening during a scroll event listener
<myles> eae: We tried that and rejected it, but I don't remember why. Our current behavior is the one that worked the best in the best number of cases.
<myles> chrishtr: If something dirties layout that changes scroll offset in a scroll event listener... you want that readback to not take into account anchoring?
<myles> emilio: Yes.
<myles> eae: That sounds reasonable. If we can't figure out why we can't do that, we should try it agian and see if it's workable
<myles> chrishtr: Is the code doing this loop synchronously?
<myles> emilio: I know pages in Chrome that burn CPU because the scroll offset keeps changing
<myles> emilio: It changes back and forth, and triggers two scroll events
<myles> eae: We try to detect that and short-circuit if we go back and forth more than a few times, but it may not always work.
<myles> atotic: If you have feedbakc, that would be great, because we're using a heuristic now
<myles> chrishtr: If layout is dirtied and syncrhonously read back in teh same handler .... what does reading it back have to do with ....
<smfr> q+
<myles> emilio: While the page is measurin gsomething, so if you insert something, measure it, then remove it, that measurement triggers a scrolloffset update. If you measure again, you'd get the opposite scrolloffset update. That needs to update the scroll offset
<myles> chrishtr: Are you proposing that forced layouts don't do this?
<astearns> ack smfr
<myles> emilio: I'm proposing that Scroll offsets don't do anchoring
<myles> smfr: Is the time that scroll anchoring is applied defined in terms of HTML5 event loop.
<myles> emilio: no. This is an issue. Mostly for this heuristic, if we can remove it, we don't need that, but the main issue is that it's udnefined when this calculation occurs. We are aware of this.
<myles> emilio: Scroll anchoring runs after updating styles but before updating layout.
<myles> emilio: In order to have hte tree in the state you want it to, you have to run it every layout
<myles> smfr: Scrolls triggered by scroll anchoring fire scroll events?
<myles> emilio: yes
<myles> smfr: that's necessary?
<myles> smfr: yes
<myles> emilio: yes
<myles> chrishtr: What you're proposing is worth investigating. Chrome can invest engineers to page back in our history here.
<myles> eae: If you're impelmenting now, we shoudl revesolve this
<myles> astearns: Do we need a resolution? Should we change the spec?
<myles> emilio: These heuristics are still implemented in Chrome and Firefox. I want to remove them in Firefox.
<myles> emilio: I don't see any reason to remove them now.
<myles> chrishtr: We need more investigation.
<smfr> q+
<myles> dbaron: You talked about, in a certain case, not doing scroll anchoring. Are there are reasons to record but defer the adjustment until later?
<myles> dbaron: I haven't thought about it that much. It feels like not doing it sometimes runs the risk of dropping something that you might have wanted
<myles> emilio: That may be another approach. And then we can move it to the "update the rendering steps" in HTML5
<myles> emilio: The main reason case where you don't want scroll anchroning adjustments is when you're reacting to scroll events yourself
<myles> atotic: Part of the problem in Chrome is that, by the time we're in layout, we don't know which handler cuased it. If you can make it work in Firefox, maybe it's possible in Chrome. But we don't have anyone working on it
<myles> emilio: Yeah, but you can set a flag....
<myles> atotic: We'd have to pass the flag around....
<myles> emilio: Yeah, it's possible
<myles> smfr: High level comment about spec: The spec is unusual because it describes a behavior which is making up for the fact that web developers often make mistakes and cause scroll jank. With specs like this, we need to be careful. We risk a scenario where some UAs implement this, papering over the bad stuff, and authors may only test in UAs that implemented it. In other UAs whcih haven't implemented in it, there's a lot more scroll jank than the other UAs. This
<myles> spec increases the disparity between user agents. We need to be careful not to make too many of these in the future
<myles> astearns: There are lots of CSS features which are improving the rendering of a page. If a browser doens't implement that feature, that browser will look worse.
<myles> smfr: It's invisible to authors. The browser jsut fixes up content. It's easy for authors to create scroll-jank if they don't test in chrome
<myles> iank_: there is precidence. Text autosizing behavior is automatic.
<myles> astearns: Can we move on?

[skobes-chromium]

Is this proposal specifically limited to scroll handlers that force synchronous layout (e.g. reading back offsetTop or scrollTop after changing the DOM) before yielding?

Based on https://phabricator.services.mozilla.com/D43233 it sounds like yes. Blink could also implement this fairly easily.

Some historical context: the sticky header feedback loops that we originally encountered didn't necessarily force synchronous layout. They would just touch DOM and yield. Then the lifecycle update performs the scroll anchoring adjustment, but we are no longer "inside" the listener.

We thought about a more general solution, in which the adjustment would be suppressed if the movement of the anchor node occurred "because of" a scroll event listener which finished in the past. We concluded that tracking causality in this manner was too difficult.

The cases that motivated http://bit.ly/sanaclap ("suppression triggers" in the spec) would not have been solved by merely suppressing adjustments during sync layouts inside scroll listeners.

But it might make sense to do this, if it fixes additional cases that are not addressed by the suppression triggers.

On the other hand it is weird for the behavior to be different because the scroll listener did a layout-forcing readback before yielding. Usually a readback is a nop.

[emilio]

Is this proposal specifically limited to scroll handlers that force synchronous layout (e.g. reading back offsetTop or scrollTop after changing the DOM) before yielding?

Yes, basically.

Some historical context: the sticky header feedback loops that we originally encountered didn't necessarily force synchronous layout. They would just touch DOM and yield. Then the lifecycle update performs the scroll anchoring adjustment, but we are no longer "inside" the listener.
We thought about a more general solution, in which the adjustment would be suppressed if the movement of the anchor node occurred "because of" a scroll event listener which finished in the past. We concluded that tracking causality in this manner was too difficult.

I see, that's fair.

The cases that motivated http://bit.ly/sanaclap ("suppression triggers" in the spec) would not have been solved by merely suppressing adjustments during sync layouts inside scroll listeners.

But it might make sense to do this, if it fixes additional cases that are not addressed by the suppression triggers.

It does, see above. But you're right we probably cannot remove the existing triggers.

On the other hand it is weird for the behavior to be different because the scroll listener did a layout-forcing readback before yielding. Usually a readback is a nop.

Well, it is when scroll anchoring isn't suppressed when reading back also changes scroll positions, right? At least that's my mental model of it.

[emilio]

@skobes interesting, that document mentions running adjustments during the animation frame, that's one of the things we thought about too... see https://bugzilla.mozilla.org/show_bug.cgi?id=1529702.

That would also fix this issue of scroll listeners doing readback, fwiw...

[skobes-chromium]

Chrome does defer scroll anchoring adjustments to a well-defined point in the frame lifecycle. I think this is also in the spec, which defines the "suppression window" with reference to the event loop.

However, readback of scroll position forces the adjustment to occur (there is a test that verifies this). There is some discussion on the review thread of https://codereview.chromium.org/2404393003 about when to flush the adjustment.

[emilio]

Sure, so does Gecko, but reading back layout-dependent values goes through all the lifecycle steps including applying adjustments, unless I'm misreading the blink code:

https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/dom/document.cc?l=3038&rcl=85bca75346768d2f213696319acdd149467a8bab

So doing stuff like getBoundingClientRect(), or offsetHeight, or whatever else would also perform adjustments.

[chrishtr]

I currently don't think we should update the spec to disallow scroll anchoring adjustments from scroll event handlers. I think it's ultimately too arbitrary and restrictive.

In addition, I have been able to find solutions to a few of the cases Emilio added to the WPT test suite without resorting to disabling scroll anchoring during scroll event handlers. See:

https://chromium-review.googlesource.com/c/chromium/src/+/1902767
https://chromium-review.googlesource.com/c/chromium/src/+/1928243
https://chromium-review.googlesource.com/c/chromium/src/+/1929439

[emilio]

So one of the diffs (https://chromium.googlesource.com/chromium/src.git/+/6a9dd6d26726f346345cf1970c2bc3af3ec1afbd) changes the expectations of http://w3c-test.org/html/interaction/focus/the-autofocus-attribute/update-the-rendering.html... What's the reasoning for that? https://html.spec.whatwg.org/multipage/webappapis.html#update-the-rendering seems it defines the order that was in the test...

Also, that commit means that scroll events from scroll anchoring are not dispatched directly, but at the end of a frame if something else has not changed the offset back... right? If so, that is observable and we should probably put that in the spec.

[chrishtr]

The CL causes scroll offsets due to scroll anchoring and clamping (but not other causes, such as explicitly setting scroll offset via script) to occur after the layout part of the update-the-rendering steps have completed. Therefore the scroll events corresponding to them are sent at the subsequent update-the-rendering event loop opportunity.

i.e. the sequence is:

1. Web app dirties layout
2. Update-the-rendering runs (frame N)
   2a. Various things, including firing any scroll events
   2b. Layout
   2c. Enqueue scroll event
   ...
3. Update-the-rendering runs (frame N+1)
   3a. Fire scroll event enqueued by 2c
   ...

Before my CL, a scroll anchor or clamp related to a forced layout would enqueue a scroll event right then and there. But I don't think anything in the spec requires this. Instead we can enqueue it after layout during update-the-rendering, resulting in it being fired in the next frame (N+1 in my example above).

[chrishtr]

I think we should go ahead and close this and not change the specs. @emilio ok by you?

[emilio]

Yeah, I'm ok with this, sorry for the lag.