1791232 - Element with position:sticky inside a container with clip-path or overflow:clip is not painting correctly on scroll

Status: NEW

(Core :: Web Painting, defect)

Description

[maggotfish]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:104.0) Gecko/20100101 Firefox/104.0

Steps to reproduce:

See example: https://codepen.io/tombigel/pen/xxEPxvg?editors=0100

I have an element with position: sticky inside a parent with clip-path: inset(0), or same with overflow: clip.

Actual results:

when scrolling it's not painting it correctly as I scroll.

Expected results:

Should always paint the clipped area of the sticky positioned element.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Test-case from reporter.

Comment 2

[Timothy Nikkel (:tnikkel)]

Seems to date back to our initial implementation of sticky.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

So, position: fixed used to be similarly broken, and bug 1382534 seemed to fix it: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e81094853e1d7035db30a54deaf60a0108924b85&tochange=b2f459b88cab5525c785a7fa70a01be3e9cdcb23

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: More reduced test-case.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

In the reduced test-case, something like this helps make the bug not reproduce:

diff --git a/layout/painting/nsDisplayList.cpp b/layout/painting/nsDisplayList.cpp
index 6949daddfe8c8..e47cb5a929103 100644
--- a/layout/painting/nsDisplayList.cpp
+++ b/layout/painting/nsDisplayList.cpp
@@ -1585,10 +1585,7 @@ static bool IsStickyFrameActive(nsDisplayListBuilder* aBuilder,
   MOZ_ASSERT(aFrame->StyleDisplay()->mPosition ==
              StylePositionProperty::Sticky);
 
-  StickyScrollContainer* stickyScrollContainer =
-      StickyScrollContainer::GetStickyScrollContainerForFrame(aFrame);
-  return stickyScrollContainer &&
-         stickyScrollContainer->ScrollFrame()->IsMaybeAsynchronouslyScrolled();
+  return true;
 }
 
 bool nsDisplayListBuilder::IsAnimatedGeometryRoot(nsIFrame* aFrame,

I'm unsure what that means in practice tho, not familiar with AGRs... Even with that the bug still repros when the window is unfocused. Tim / Botond (when you're back, no rush), do you know what is the right fix here?

Comment 6

[Botond Ballo [:botond]]

In bug 1382534, the problem was that a mask display item had an incorrect ASR (active scrolled root), and the fix was to give it a different ASR (the rest of the work in that bug was about computing a correct clip to go with the ASR).

However, the codepath modified in comment 5 seems not to be related to ASR assignments, but rather only used by retained-DL? Which makes me wonder what the behaviour is with retained-DL disabled.

Comment 7

[Timothy Nikkel (:tnikkel)]

It reproduces with retained DL disabled. The code in comment 5 doesn't seem only related to retained DL?

Comment 8

[Botond Ballo [:botond]]

The only call chain of IsStickyFrameActive that I could find using Searchfox is:

RetainedDisplayListBuilder::ProcessFrame
  nsDisplayListBuilder::FindAnimatedGeometryRootFrameFor
    nsDisplayListBuilder::IsAnimatedGeometryRoot

Comment 9

[maggotfish]

pin @emilio any update on this?

Comment 10

[Botond Ballo [:botond]]

What I'm seeing in local testing is:

• The change in comment 5 does not fix the bug (or change anything about when in reproduces, as far as I could tell) for me.
• The bug does reproduce with retained DL disabled for me.

One note: the bug reproduces reliably if you click on the page (presumably causing a repaint) after scrolling down and before scrolling back up.

Comment 11

[Botond Ballo [:botond]]

Attachment: Further reduced test case

Here is a testcase that's further reduced still.

One note: the bug reproduces reliably if you click on the page (presumably causing a repaint) after scrolling down and before scrolling back up.

Slight clarification here: the bug reproduces reliably if you click below the green area after scrolling down and before scrolling back up.

Comment 12

[Botond Ballo [:botond]]

Attachment: Display list with correct rendering

Here is the Gecko display list when the page is scrolled to the top after initially loading the page (rendering is correct).

Comment 13

[Botond Ballo [:botond]]

Attachment: Display list with incorrect rendering

Here is the display list with the page scrolled to the top after first scrolling down and clicking below the green area, i.e. the last time the page was painted was when it was scrolled down. Here, rendering is incorrect.

Comment 14

[Botond Ballo [:botond]]

I'm not spotting anything particularly surprising about the display lists, the only things that differ are vertical offsets reflecting the fact that in the second case we were scrolled down at the time of the paint. There are no differences related to ASRs or clips (beyond their offsets).

Comment 15

[Botond Ballo [:botond]]

One thing I've noticed is that async scrolling after the repaint does scroll the green content, but how much it scrolls it is limited, with the limit related to the value of the margin-bottom: as the margin-bottom approaches 0, it's less limited (such that at 0 the problem goes away completely), whereas as the margin-bottom approaches -90vh (90vh being the height of the parent comp element), it's more limited (such that at 90vh you can cause the green area to not be rendered at all even when scrolled to the top, if you've scrolled it completely out of view at the time of the repaint).

This is making me think there is a clip that should be scrolling in the compositor but isn't.

Comment 16

[Botond Ballo [:botond]]

Attachment: Further reduced test case

The previous comment was based on a slight misunderstanding of what the page is doing.

I'm uploading a revised testcase which uses a gradient rather than a solid color as the sticky element's background, which makes it more obvious whether the sticky element is itself being scrolled vs. staying in place and just having its clip be scrolled.

If you scroll this page, you can see there are two distinct phases: initially, only the clip scrolls (the background stays in place), and then at one point (once the margin-box of the sticky element, which is only 40vh high due to the margin-bottom: -50vh, reaches the bottom edge of the #comp element which is the sticky element's containing block), the sticky element itself starts to scroll out of view.

Similarly, when scrolling back up, first the sticky element scrolls partially into view, and then the clip starts scrolling to reveal more of it.

The rendering bug occurs if a repaint was last triggered at the time the clip was partially clipping away the sticky element. After such a repaint, async scrolling successfully moves the clip but any additional area revealed in this way is blank (until the next repaint).

So, given that, I think a revised diagnosis is more along the lines of us applying a clip to a display item prematurely during painting, not recognizing that the clip can scroll asynchronously and we should paint the entire item.

Comment 17

[Botond Ballo [:botond]]

Discussed this with Markus today.

The problem here is that the StickyPosition display item's ASR is the root scroll frame's ASR (this is how we currently model position:sticky in the ASR model), and the clip that applies to this item from the overflow:clip is also the root scroll frame's ASR (since that clip scrolls with the root scroll frame).

As a result, we think that the clip and item scroll together, and so if the clip obscures part of the item at display list building time, it will continue to obscure that part of the item even after async scrolling.

However, this is not in fact the case -- while the clip scrolls with the root scroll frame, the sticky element only does so when it's not in its "stuck" position, and therefore it doesn't really scroll together with the clip.

Unfortunately this does not seem like an easy fix -- it seems like fixing this would require the refactor described in bug 1730749.

Comment 18

[Botond Ballo [:botond]]

I'll also mention that I discovered two reftests in our test suite that exercise a very similar scenario:

position-sticky-scrolled-clip-1.html
position-sticky-scrolled-clip-2.html

They use the clip property rather than overflow:clip, but the effect at the display list level is the same: we have a sticky display item with a scrolled clip that sometimes moves relative to the item.

The first test is only testing that the clip can in fact be scrolled in the compositor, and it passes. The second test is basically testing the same scenario discussed in this bug, and it's marked as a known failure, and has been ever since it was added in bug 1298218, the bug that introduced the ASR model.

Comment 19

[Botond Ballo [:botond]]

Canceling needinfo on Timothy as the original question has been substantially answered.

(But perhaps you might find the diagnosis here, and the additional motivation for the refactor described in bug 1730749, an interesting consideration to keep in mind as part of the ongoing/planned display list refactoring work?)

Comment 20

[maggotfish]

Thanks so much for this Botond! We are watching this with great anticipation (:

Comment 21

[Botond Ballo [:botond]]

(In reply to maggotfish from comment #20)

We are watching this with great anticipation (:

As mentioned at the end of comment 17, this is unfortunately not likely to be an easy fix as it depends on a refactor (bug 1730749) that will require a larger amount of work.

To help inform the prioritization of this work, I have a few questions for you:

• Does this issue affect a website currently in production? If so, can you identify it, and/or give a sense of its size (order-of-magnitude estimate of # of users or # of visits)?
• Are you aware of other affected websites?
• Are you aware of any workarounds you can employ?

Thanks!

Comment 22

[maggotfish]

As mentioned at the end of comment 17, this is unfortunately not likely to be an easy fix as it depends on a refactor (bug 1730749) that will require a larger amount of work.

Yes, it's understood (: but still wanted to add an optimistic tone.

Does this issue affect a website currently in production? If so, can you identify it, and/or give a sense of its size (order-of-magnitude estimate of # of users or # of visits)?

No that we know of. This is mostly blocking us from replacing our current scroll effects technique with a better one that should allow us adding more features and possibly improve the effects' performance.

Are you aware of other affected websites?

No.

Are you aware of any workarounds you can employ?

Alternatively, if Gecko ships the new scroll-animations per spec we may be able to avoid resorting to this technique.

Thanks!

Comment 23

[Botond Ballo [:botond]]

(In reply to maggotfish from comment #22)

Alternatively, if Gecko ships the new scroll-animations per spec we may be able to avoid resorting to this technique.

Support for scroll-driven animations is definitely planned, with significant implementation work done already. The tracking issue for that is bug 1676779.

Comment 25

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

The following field has been copied from a duplicate bug:

Field	Value	Source
Regressed by	bug 1382534	bug 1820221

For more information, please visit auto_nag documentation.

Comment 26

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

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

Comment 27

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

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

Comment 28

[maggotfish]

Hey guys, it's been over 10 months now, is there any progress?
We really need this fixed as we plan on rolling out a feature that depends on this scenario.
See example here: https://codepen.io/ydaniv/pen/vYbpxrr
For now our implementation uses fixedpos which is problematic since it breaks on different scenarios where a containing block is forced on an ancestor of that element.

Currently this is working smoothly on Blink and WebKit.
Gecko's paint is still lagging.

Thanks!

Comment 29

[Botond Ballo [:botond]]

I have successfully advocated for adding the refactoring that this depends on, bug 1730749, to our team's roadmap. However, we have not gotten to it yet, and there are a few more items ahead of it. I'm hopeful that we'll get to it in 2024 H1.

Comment 30

[Botond Ballo [:botond]]

(Also, just correcting the bug flags, as this is not a regression; this has never worked. The relationship to bug 1382534 is described in bug 1820221 comment 5.)

Comment 31

[maggotfish]

Also cross-commenting here, since this was my original issue:
Just want to point out that we managed to fix this issue in one case by slapping a will-change: transform on a wrapper element that's under the clipping element and parent of the sticky element: https://jsbin.com/wonopujena/edit?css,output