1856088 - Scroll anchoring causes jumps (probably due to incorrect relative scroll position updates)

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P2)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: A test case

While I was trying to create a test case for bug 1779404, I happened to create another variant of test cases causing scroll position jumps. This test case isn't fixed by the fix for bug 1779404, so I suppose there's another underlying issue on our relative scroll position update stuff.

As far as I can tell with the fix for bug 1779404 I don't see any scroll position jumps on Facebook, so I am assuming this case is unrelated to the facebook case. Setting P3:S3 for the reason, but if we received new reports about scroll jumps on Facebook, we will raise this severity.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Another test case

Comment 2

[Hiroyuki Ikezoe (:hiro)]

This is tough.

After chatting with Botond and Timothy on Matrix, I thought using pure scroll position update for scroll anchor adjustment is promising. But in fact it's not, it's not at all.

For example in the cases such as bug 1852818, a scroll anchor adjustment happens in a setTimeout callback due to expanding an element's height above the anchor, and then in a scroll event handler a scroll anchor adjustment happens due to restoring the expanded element. Say something like; the first adjustment +500px pure relative update and the second one is -500px adjustment. They are going to be handled in a single NotifyLayersUpdated call. Then unfortunately we clamp the scroll offset on every update, so that the first update doesn't result +500px change, but the second one result -500px. We might want to defer the clamp later? It won't work if there's a Element.scrollBy call in between the scroll anchor adjustments, we may end up having more scroll related metrics in each ScrollPositionUpdate and use them in each updating functions. I don't think that's a good idea though.

So I'd rather introduce a new scroll position update type, it's an absolute update but it will be merged into the running async scroll animation. That's will be most stable in terms of scroll anchoring because scroll anchor adjustment wants literally to scroll the given destination, i.e. the anchor element position.

I've implemented it locally and it works fine as far as I can tell. And it looks also fine on a try run.

Also note that I implemented it behind a new pref so that we can disable it if it were problematic.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1856088 - Dump some metrics in the failure messages in helper_scroll_anchoring_on_wheel.html. r?botond

This test is a test case where a scroll anchor adjustment happens while an
async scroll animation is running. Without the dumped metrics it's hard to tell
what's wrong. In fact the test fails if we did use ScrollUpdateType::Absolute
for scroll anchoring

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1856088 - Handle scroll anchor adjustments as an absolute scroll position updates behind a pref. r?botond

Depends on D190887

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Bumping up the priority.

Comment 6

[Brinke Guthrie]

any progress on this? It's been going on for ages.

Comment 7

[Botond Ballo [:botond]]

(In reply to Brinke Guthrie from comment #6)

any progress on this? It's been going on for ages.

There is a patch posted for this. I did a round of review on it today. It should be merged in the near future, and the fix will appear in Firefox 121 and possibly be backported to Firefox 120.

Note that this is just one of several recent fixes for these kinds of symptoms. Others are:

• bug 1833758 (fixed in Firefox 115)
• bug 1852818 (fixed in Firefox 119)
• bug 1779404 (fixed in Firefox 119)

So, depending on what version you were using when you encountered these symptoms, it's possible they are already fixed by one of the above changes.

Comment 8

[Brinke Guthrie]

(In reply to Botond Ballo [:botond] from comment #7)

(In reply to Brinke Guthrie from comment #6)

any progress on this? It's been going on for ages.

There is a patch posted for this. I did a round of review on it today. It should be merged in the near future, and the fix will appear in Firefox 121 and possibly be backported to Firefox 120.

Note that this is just one of several recent fixes for these kinds of symptoms. Others are:

• bug 1833758 (fixed in Firefox 115)
• bug 1852818 (fixed in Firefox 119)
• bug 1779404 (fixed in Firefox 119)

So, depending on what version you were using when you encountered these symptoms, it's possible they are already fixed by one of the above changes.

Wait- what's the current FF build then? It's not in the next revision?

Comment 9

[Botond Ballo [:botond]]

(In reply to Brinke Guthrie from comment #8)

Wait- what's the current FF build then? It's not in the next revision?

Currently, 119 is released, 120 is in beta, and 121 is under active development.

When this bugfix is merged, it goes on the development ("nightly") branch, and by default will be released in 121, but there is a possibility of backporting a fix to the beta branch (in which case the fix would be released in 120). This fix may be a good candidate for backport since we're early in the release cycle, but that's a call for the patch author and release managers to make.

What site are you experiencing these symptoms on, and with what Firefox version?

Comment 10

[Brinke Guthrie]

(In reply to Botond Ballo [:botond] from comment #9)

(In reply to Brinke Guthrie from comment #8)

Wait- what's the current FF build then? It's not in the next revision?

Currently, 119 is released, 120 is in beta, and 121 is under active development.

When this bugfix is merged, it goes on the development ("nightly") branch, and by default will be released in 121, but there is a possibility of backporting a fix to the beta branch (in which case the fix would be released in 120). This fix may be a good candidate for backport since we're early in the release cycle, but that's a call for the patch author and release managers to make.

What site are you experiencing these symptoms on, and with what Firefox version?

Facebook has bounced like a ping pong ball on all FF versions for a couple of years now.

Comment 11

[Botond Ballo [:botond]]

(In reply to Brinke Guthrie from comment #10)

Facebook has bounced like a ping pong ball on all FF versions for a couple of years now.

Ok, so the first thing I would suggest is trying the very recently released 119, which contains a fix for bug 1779404, which was specifically about Facebook.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8229b3ad6bb
Dump some metrics in the failure messages in helper_scroll_anchoring_on_wheel.html. r=botond
https://hg.mozilla.org/integration/autoland/rev/7a509d3a2778
Handle scroll anchor adjustments as an absolute scroll position updates behind a pref. r=botond

Comment 13

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

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

Comment 14

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/b8229b3ad6bb
https://hg.mozilla.org/mozilla-central/rev/7a509d3a2778

Comment 15

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

Upstream PR merged by moz-wptsync-bot

Comment 16

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #11)

(In reply to Brinke Guthrie from comment #10)

Facebook has bounced like a ping pong ball on all FF versions for a couple of years now.

Ok, so the first thing I would suggest is trying the very recently released 119, which contains a fix for bug 1779404, which was specifically about Facebook.

Based on bug 1858022 comment 16, it looks like bug 1779404 did not fix the issue you were seeing.

It would be interesting to see if change made in this bug fixes it. When you get a chance, could you give the latest Nightly version (https://www.mozilla.org/en-US/firefox/channel/desktop/#nightly) a try?

Comment 17

[Brinke Guthrie]

ah, don't do nightly on my Mac.

Comment 18

[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-firefox120 to wontfix.

For more information, please visit BugBot documentation.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

I am not going to uplift this change since this change didn't fix the facebook jumply scrolling issue completely and it's unclear to me whether it's mitigated or not.