1833758 - Consider adding a way to allow scroll anchor adjustments in scroll event handlers behind a pref

Status: RESOLVED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

We had a couple of times of discussion about mitigating bug 1779404 (facebook jumpy scrolling issue) in APZ meetings and figured out a way. The way is;

1. Add a pref to allow scroll anchor adjustments in scroll event handlers
2. If adjustments in scroll event handlers happen frequently, say 10 times adjustments in 500ms, disable further adjustments just like the heuristic for bug 1592474

I originally raised only 1) with a hidden pref named like "this_pref_may_your_browser_hang" or some such. Then Botond came up with 2). With 2) the pref should not be hidden?

Emilo, does this mitigation sound reasonable? If so, I will give it a shot.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I mean, the number of adjustments is not so relevant, it's how much it advances what's relevant right? I wonder if bug 1592474 allows us to just allow scroll anchor adjustments in scroll handlers...

The other thing we should look into is at not generating a scroll event when scroll anchoring undoes the scrolling, that's effectively this piece of chromium work: https://github.com/w3c/csswg-drafts/issues/4239#issuecomment-575828404

I think that's probably a better avenue to fix this.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)

I mean, the number of adjustments is not so relevant, it's how much it advances what's relevant right? I wonder if bug 1592474 allows us to just allow scroll anchor adjustments in scroll handlers...

Yeah indeed. In the specific case of the infinite event bug, it's indeed 0 advances in scroll event handlers. I will figure out DisablingHeuristic can be applicable for scroll event handler cases. The big difference from the current DisablingHeuristic is scroll event handler cases involves user scrolling, thus we don't reset the heuristic in UserScrolled(). Maybe we can have two DisablingHeuristics?

The other thing we should look into is at not generating a scroll event when scroll anchoring undoes the scrolling, that's effectively this piece of chromium work: https://github.com/w3c/csswg-drafts/issues/4239#issuecomment-575828404

I think that's probably a better avenue to fix this.

Honestly I am not a big fan of the change, from a comment by David Bokan in one of the relevant changes;

This means a scroll that ends up where it started before a lifecycle doesn't dispatch an event. e.g: https://output.jsbin.com/rumuges. Why early out at all?

OTOH, if the scroll interpretation really is "rendered a new scroll position" this makes sense but it might be an interop/compat issue..

I am also concerned it.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1833758 - Add a pref not to reset max consecutive adjustment count during running APZ async scroll. r?emilio

With this pref we can avoid bug 1561450 without disabling any scroll adjustments
in scroll event handlers because in the specific case of the bug the scroll
adjustment in question is zero length such as

  element.style.display = "block";
  element.offsetTop // flush layout
  element.style.display = "none";

so it can be caught by our existing consecutive adjustment heuristic. Thus with
the default layout.css.scroll-anchoring.max-consecutive-adjustments value, as of
now it's 10, the case of bug 1561450 will stop firing scroll events after
5 (= 10/2) additional scroll event observations.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4adf1e463466
Add a pref not to reset max consecutive adjustment count during running APZ async scroll. r=emilio

Comment 5

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

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

Comment 6

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/4adf1e463466

Comment 7

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

Upstream PR merged by moz-wptsync-bot