1738781 - Scroll anchoring selects a suboptimal anchor if contents are dirty when anchor selection happens.

Status: VERIFIED FIXED

(Core :: Layout, task)

Description

[Emilio Cobos Álvarez (:emilio)]

Attachment: Testcase

On the attached test-case:

• Scroll to the middle of the page (where the buttons are).
• Click "Replace all"
• Click "Insert"

ER:

• Buttons don't jump.

AR:

• Scroll position jumps.

This is kind of unfortunate, because scroll anchoring works on a dirty layout tree by design, and by the point we try to select an anchor we haven't reflowed the new contents yet, so we choose the better anchor available (which is #content).

However after reflow we could improve our anchor selection.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1738781 - Invalidate anchor after layout if we can find a better one. r=dholbert

This is very similar to code Blink has dealing with
content-visibility: auto:

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/scroll_anchor.cc;l=658-659;drc=fd8802b593110ea18a97ef044f8a40dd24a622ec

I'm not sure yet why they don't have this problem for other types of
content (maybe they run two layouts of the scrolled content to determine
scrollbars or something, so this just works sorta-by-chance for them?).

I'm building Chromium to debug that a bit and pushing to try next (will
send the test-case as a WPT in a follow-up patch if there are no new
passes), but I think this is a sensible fix, wdyt?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Test-case that also reproduces in Chrome

If you perform the STR, but click Replace all, then insert, then the problem reproduces in Chromium as well.

The reason why it doesn't if you click both buttons separately is because Chrome invalidates the anchor on focus changes (that is, when you click the button), via Node::NotifyPriorityScrollAnchorStatusChanged().

Blink implemented the same behavior as I just did for content-visibility: auto... Chris, do you have opinions on this? I think recomputed the anchor if at the time of computation it had dirty children makes some amount of sense to me, it guarantees that we find an optimal anchor without depending on layout timing.

That said for now I could just implement the "priority candidates" bits from the relevant spec PR and punt on this, since it'd paper over most these issues...

Comment 3

[Emilio Cobos Álvarez (:emilio)]

So I think invalidating the anchor on any focus change like blink does is incorrect, and since the button is not really a "priority node" as per the scroll anchoring spec it should probably not be invalidated in that case...

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e402011abaa3
Invalidate anchor after layout if we can find a better one. r=dholbert

Comment 5

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

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

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/e402011abaa3

Comment 7

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

Upstream PR merged by moz-wptsync-bot

Comment 8

[Catalin Sasca, Desktop QA [:csasca]]

Reproduced the issue on Firefox 95.0a1 (2021-11-01) under macOS 11.6.1 by using the testcase provided in Comment 0.

The issue is fixed on Firefox 96.0b4 and Firefox 97.0a1 (2021-12-13). Tests were performed on macOS 11.6.1, Ubuntu 20.04 and Windows 11.