1908242 - Sticky elements randomly jump inside a sticky panel while scrolling

Status: VERIFIED FIXED

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

Description

[sunsay]

Attachment: firefox_41VKHG3pBb.mp4

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/124.0.0.0 YaBrowser/24.6.0.0 Safari/537.36

Steps to reproduce:

I've got a sticky header (or a footer) inside of a sticky panel (div).
The panel sometimes dynamically changes its position from sticky to relative and vice versa.
Once it changes its position to sticky, Firefox can't keep up sticky items inside of it, and they randomly jump out of their supposed positions while user scrolls down or up: https://jsbin.com/gamariyimu/edit?html,output

Actual results:

Sticky elements randomly jump inside a sticky panel while scrolling

Expected results:

Sticky elements should remain sticky as it happens when panel's position is sticky from the start and not being changed dynamically

Comment 1

[Botond Ballo [:botond]]

Did a quick check with mozregression. This bug reproduces as far back as I can test (around Firefox 74; earlier versions do not handle some of the script on this page). It happens with WebRender disabled, and also with APZ disabled.

==> The issue occurs during display list building or earlier. Moving to Web Painting component.

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4e0a6c11be1bdd3f766991967e680ce6704b70d7&tochange=cec079fedf8f48576568d3f66d835bbf5bb6f79c

Suspect: ff238eaf45c0e29c686aad03732799c96c04c866 Emilio Cobos Álvarez — Bug 1611462 - Optimize position changes better. r=dholbert

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

:emilio, since you are the author of the regressor, bug 1611462, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Reduced test-case

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1908242 - Make sure that sticky items are processed in the right order. r=TYLin,#layout

We rely on positioning ancestors before descendants to get the right
position. This was very implicitly enforced before the regressing bug,
by reframing on position changes (due to the order
StickyScrollContainer::AddFrame was called, ancestors before children).

In order to support dynamic sticky changes without reframing, we need to
handle the case of an existing ancestor switching position to sticky
dynamically.

Luckily we already have the right data-structure for that, so the change
is rather trivial.

Write a testharness test for this because APZ does get the position
right, so it's more future-proof against regressions to just test the
raw layout values.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Thanks for the excellent test-case sunsay!

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1908242 - Minor clean-ups to StickyScrollContainer. r=TYLin,#layout

Remove one totally unused function, and some useless virtual keywords.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1611462

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The component has been changed since the backlog priority was decided, so we're resetting it.
For more information, please visit BugBot documentation.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7aaa257ce970
Minor clean-ups to StickyScrollContainer. r=TYLin,layout-reviewers

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/67a9d7321ab6
Make sure that sticky items are processed in the right order. r=TYLin

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/47199 for changes under testing/web-platform/tests

Comment 14

[tszentpeteri]

https://hg.mozilla.org/mozilla-central/rev/7aaa257ce970

Comment 15

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/67a9d7321ab6

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox129 to wontfix.

For more information, please visit BugBot documentation.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9413351 [details]
Bug 1908242 - Minor clean-ups to StickyScrollContainer. r=TYLin,#layout

Beta/Release Uplift Approval Request

• User impact if declined: Position jumps on scrolling.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 0, or bug 1873231
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively straight-forward patch.
• String changes made/needed: none
• Is Android affected?: Yes

Comment 18

[Donal Meehan [:dmeehan]]

:emilio what do you think about ESR128? Should you add an uplift request there too?
We are at the very start of the ESR cycle so ESR128 is not at full rollout yet.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9413351 [details]
Bug 1908242 - Minor clean-ups to StickyScrollContainer. r=TYLin,#layout

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: see above. If it applies cleanly it should be a relatively safe uplift.
• User impact if declined: see above
• Fix Landed on Version: 130
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): See above

Comment 20

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

Upstream PR merged by moz-wptsync-bot

Comment 21

[Donal Meehan [:dmeehan]]

Comment on attachment 9413351 [details]
Bug 1908242 - Minor clean-ups to StickyScrollContainer. r=TYLin,#layout

Approved for 129.0b7

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/90e4710516c0
https://hg.mozilla.org/releases/mozilla-beta/rev/a8e865b7d543

Comment 23

[Rares Doghi, Desktop QA]

Verified as fixed in our latest Beta 129.0b7 and our latest Nightly 130.0a1 (2024-07-22)

Comment 24

[Donal Meehan [:dmeehan]]

Comment on attachment 9413351 [details]
Bug 1908242 - Minor clean-ups to StickyScrollContainer. r=TYLin,#layout

Approved for 128.1esr.

Comment 25

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/6c401250eae3
https://hg.mozilla.org/releases/mozilla-esr128/rev/028e071b9448

Comment 26

[Rares Doghi, Desktop QA]

Verified as fixed in our latest build 128.1.0esr