2010481 - ":hover" interrupts "position:sticky" when scrolling with the mousewheel

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P2)

Description

[DF]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:147.0) Gecko/20100101 Firefox/147.0

Steps to reproduce:

147.0 (64-bit), Windows 10
build ID: 20260105210555

Since updating to the latest release version, I've been experiencing an issue with sticky elements becoming temporarily frozen when that element and an element with ":hover" styles have been moused over in a specific sequence.

Use the following HTML and steps to reproduce the bug:

<html><head>
	<style>a:hover{color:#F00;}</style>
</head>
<body style="height:3000px;">
	<nav style="position:sticky;top:0;">STICKY NAV</nav>
	<a>HOVER ELEMENT</a>
</body></html>

1) Mouse over HOVER ELEMENT, then STICKY NAV, then HOVER ELEMENT, in this exact sequence.
2a) Move the cursor down so it doesn't touch either element, and scroll down using the mousewheel. The sticky element will not follow the screen until having scrolled down far enough (distance varies with viewport height).
OR
2b) Keep the cursor over HOVER ELEMENT and scroll down using the mousewheel. The sticky element will not follow the screen until scrolling has stopped (more obvious with smooth scrolling enabled).
3) After step 2a, doing any of the following will unfreeze and bring the STICKY NAV back on screen (not an exhaustive list):

• Mousing over HOVER ELEMENT again,
• Mousing over the scroll bar,
• Using the arrow keys to scroll,
• Changing focus to or from the Firefox window from or to another.

The HOVER ELEMENT can be any type of element (not just <a>), and the hover style can be anything as well (not just color). The height style added to the body is only so we're actually able to scroll down, but otherwise is of no consequence to this issue. Doesn't matter either if the ":hover" style is in a style tag or a CSS file, or whether it's applied to an element or class.

The issue persists when using a new Firefox profile. I did not test the issue on a Nightly build.

Comment 1

[Alice0775 White]

Attachment: reportor's_testcase.html

Comment 2

[Alice0775 White]

The sticky element will not follow the screen even if wheel scrolling has been stopped.
screencast: https://youtu.be/01T2Obldu2w

Regression window:
https://hg-edge.mozilla.org/integration/autoland/pushloghtml?fromchange=a55e6a8352a8395f09179cf38ab75a7ad69e1d25&tochange=1e6f6deb5ae17dfbda69db09f882c80faf69e0c0

Comment 3

[BugBot [:suhaib / :marco]]

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

For more information, please visit BugBot documentation.

Comment 4

[Botond Ballo [:botond]]

Thanks for the report! I will investigate.

Comment 5

[Botond Ballo [:botond]]

It looks like this is related to retained display lists.

The bug is triggered when the following happens:

1. We build a (Gecko) display list for painting. We correctly treat the sticky element as being "active" (shouldFlatten=false), and save that information on the StickyScrollContainer here.
2. We build a display list for event handling. In the repro steps, this is triggered by the hover. For event handling, we don't consider scroll frames or sticky elements as being active, and we overwrite the StickyScrollContainer flag to shouldFlatten=true (same place in the code as the first step).
3. We get a paint that reuses the display list that was built in step (1) and retained. In nsDisplayStickyPosition::CreateWebRenderCommands, we read the flag on the StickyScrollContainer which is now false, and consequently treat the sticky item as inactive in WebRender (we don't assign its stacking context a distinct spatial id).
4. We async scroll the WebRender display list built in step (3), resulting in the sticky item incorrectly moving together with the rest of the scroll frame's contents.

Comment 6

[Timothy Nikkel (:tnikkel)]

Can we refactor so that we only read and write that value in retained builds? In other types of builds we compute it but don't store/read from that place?

Comment 7

[Botond Ballo [:botond]]

Attachment: Bug 2010481 - Do not write to StickyScrollContainer::mShouldFlatten if we're not painting to the window. r=mstange

This value is only read if we're painting to the window.

If we write false to it during a hit test, with retained display lists
the next paint to the window can incorrectly use that value.

Comment 8

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tnikkel) from comment #6)

Can we refactor so that we only read and write that value in retained builds? In other types of builds we compute it but don't store/read from that place?

Yeah, I have a simple fix along those lines (now posted). Just need to figure out how to write a test that exercises this.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/1da2ae459617
https://hg.mozilla.org/integration/autoland/rev/70f2a0bbad42
Do not write to StickyScrollContainer::mShouldFlatten if we're not painting to the window. r=layout-reviewers,tnikkel

Comment 10

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/70f2a0bbad42

Comment 11

[BugBot [:suhaib / :marco]]

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

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox148 to wontfix.

For more information, please visit BugBot documentation.

Comment 13

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: Users can sometimes experience temporary jumps of position:sticky content (e.g. headers, sidebars) during scrolling. Affected sites include Facebook.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: Patch is small and fairly straightforward
• String changes made/needed: none
• Is Android affected?: no

Comment 14

[Botond Ballo [:botond]]

Attachment: Bug 2010481 - Do not write to StickyScrollContainer::mShouldFlatten if we're not painting to the window.

This value is only read if we're painting to the window.

If we write false to it during a hit test, with retained display lists
the next paint to the window can incorrectly use that value.

Original Revision: https://phabricator.services.mozilla.com/D280348

Comment 15

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/bb7e845518d6
https://hg.mozilla.org/releases/mozilla-beta/rev/4d458a515c37

Comment 16

[Ryan VanderMeulen [:RyanVM]]

I've seen this pop up in various Reddit threads too. Should we nominate this for dot release uplift also?

Comment 17

[Botond Ballo [:botond]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)

I've seen this pop up in various Reddit threads too. Should we nominate this for dot release uplift also?

+1, given that we haven't seen any issues with this fix, and the release is still several weeks off. I've nominated it: https://phabricator.services.mozilla.com/D281131.

Comment 18

[Phabricator Automation]

firefox-release Uplift Approval Request

• User impact if declined: Users can sometimes experience temporary jumps of position:sticky content (e.g. headers, sidebars) during scrolling. Affected sites include Facebook and there are various reports on reddit of users noticing this.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: Patch is small and fairly straightforward
• String changes made/needed: none
• Is Android affected?: no

Comment 19

[Botond Ballo [:botond]]

Attachment: Bug 2010481 - Do not write to StickyScrollContainer::mShouldFlatten if we're not painting to the window.

This value is only read if we're painting to the window.

If we write false to it during a hit test, with retained display lists
the next paint to the window can incorrectly use that value.

Original Revision: https://phabricator.services.mozilla.com/D280348

Comment 20

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/a4f75c9053d6
https://hg.mozilla.org/releases/mozilla-release/rev/cb9ce381d8d9