1869540 - Elements inside a container are rendered on top of the positioned element with the higher z-index

Status: ASSIGNED

(Core :: Web Painting, defect, P1)

Description

[yuzvik.oleg]

Attachment: firefox-bug-reduced-demo.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36

Steps to reproduce:

Tried to open a modal dialog (with overlay) by clicking on a button in a web app I am working on

Actual results:

The dialog and overlay were shown but some elements on the page were painted on top of the overlay even though the overlay is positioned and has high z-index set

Expected results:

elements that were painted on top of the overlay should have been painted under the overlay instead

Comment 1

[yuzvik.oleg]

Attachment: firefox-bug-demo-reduced.html

Comment 2

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

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

For more information, please visit BugBot documentation.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

This reproduces also just with contain: paint, not container-type in particular.

With a bit more reduced / backward-compatible test-case, I got to this regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4a692c812a3fe2f893d2a6e25b9490b38415c907&tochange=0969fd6383cea58141d70fbb2d0895ccb476af6c

Unfortunately builds are too old to bisect further and there are a couple suspects (bug 1535507, bug 205202). If I had to guess it'd be the first of those two.

Alice, by any chance, wouldn't you have builds for that range?

Thanks in any case for doing the initial bisection!

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Test-case without container queries.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Ah, bug 1535507 most definitely. Specially given this also reproduces with things like will-change: perspective and so.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

If I comment out https://searchfox.org/mozilla-central/rev/1e1dc727b9670d7d9117f0622caad387935004c3/layout/painting/RetainedDisplayListBuilder.cpp#78-79 then the bug gets fixed. But not familiar with that code.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Tim do you know who might be familiar with RDL nowadays?

Comment 8

[Alice0775 White]

Regression window w/ Test-case without container queries.:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=365ff23dd4c93ef26743e3fa339b5fac3bf15b66&tochange=2bf6bddf9a8a5c52222273ee787bfca7e6c97394

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Thanks Alice!

As a workaround, adding a no-op transform like scale: 1 or so fixes it (though that's of course not amazing).

Comment 10

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

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

Comment 11

[yuzvik.oleg]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #9)

Thanks Alice!

As a workaround, adding a no-op transform like scale: 1 or so fixes it (though that's of course not amazing).

Thanks for the workaround, Emilio! I added transform: scale(1,1) (not scale: 1 because it doesn't seem to work in JSX's style property) to the container (outermost element with container-type property set) and the problem is gone.

Comment 12

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

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

Comment 13

[Timothy Nikkel (:tnikkel)]

I think I know what the problem is, from the commit of the regressing patch:

"If the frame supports it (stacking context + containing block for fixed), and a descendant was
modified, we would have created an override dirty region with just the area of that descendant.
In the case where no descendants are modified, we should use an empty rect, rather than
the area inherited from our parent.

This fixes the case where we forcibly build position:fixed frames (since they might async
scroll differently to the rest of the page), but we only need to build the container item,
not the whole frame subtree within it."

The nsDisplayContainer that contains the contain: paint div here is getting this optimization, it is getting an empty dirty rect on a partial update, which causes us to not build any of its children. The optimization works for fixed pos because we always create a fixed container item in BuildDisplayListForStackingContext. For non-fixed pos we don't always build a nsDisplayContainer, that decision is made here

https://searchfox.org/mozilla-central/rev/5d6e8d45a5cf4bdfa02b6187bde53f2d42f40be2/layout/generic/nsIFrame.cpp#4322

In this case the list is empty so we don't build anything. Forcing us to build an empty nsDisplayContainer there if we hit the "empty dirty rect" optimization fixes the bug. But I'm not sure if that alone is a landable fix because the logic in WrapInWrapList tries to avoid creating container items if it can, so that might change the display item we get there without marking it modified in a partial update, which we cannot do. I'll think more on this.

Comment 14

[Kelsey Gilbert [:jgilbert]]

@tnikkel:

1. How hard is this to fix?
2. Can you prioritize fixing it?

Comment 15

[Bob Hood [:bhood]]

Keeping this on Tim's radar while he's addressing higher-priority issues.

Comment 16

[Timothy Nikkel (:tnikkel)]

(In reply to Kelsey Gilbert [:jgilbert] from comment #14)

@tnikkel:

1. How hard is this to fix?

Probably not that hard, the next step is in comment 13, I just need to spend some time reading the code and thinking through it.

2. Can you prioritize fixing it?

Yes, I definitely want to come back to this and finish it up. I've been treating this as an S3 since this issue was discovered over 4 years after the regressing bug landed, so it's probably not a very common issue. Is the S2 bump coming from bug 1865374 (which this bug blocks)? I'm not sure that bug qualifies for S2.

Comment 17

[Kelsey Gilbert [:jgilbert]]

Well there is a workaround in comments 9 and 11, so it's probably not dire, but this is still pretty clear-cut rendering correctness issue.
Up to you, but if you're happy with S3 that's fine too.

Comment 18

[Bob Hood [:bhood]]

Tim, let's evaluate the P1 priority for this one at our next Priority review.