1417601 - OOF placeholders might be skipped in nested stacking context when retaining a display list

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Miko Mynttinen]

This is an existing bug that a patch for bug 1415485 uncovered.

Comment 1

[Miko Mynttinen]

Attachment: testcase-off-frames-retaining.html

Comment 2

[Miko Mynttinen]

When a modified frame is within a stacking context, we try to create a display list with empty dirty rect to contain the changes to that stacking context. The empty dirty rect causes us to not mark OOF frames [1], unless we first call ForceDescendIntoIfVisible() on them. We do this in PreProcessDisplayList [2]. Unfortunately this fails, if the placeholder for the OOF frame is inside another stacking context that is a descendant of the stacking context that the modified frame is in.
 
[1] https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/layout/painting/nsDisplayList.cpp#1183
[2] https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/layout/painting/RetainedDisplayListBuilder.cpp#118

Comment 3

[Miko Mynttinen]

Attachment: Bug 1417601 - Override dirty rect for OOF frames with containing block outside a modified stacking context

Review commit: https://reviewboard.mozilla.org/r/200502/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200502/

Comment 4

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8929226 [details]
Bug 1417601 - Override dirty rect for OOF frames with containing block outside a modified stacking context

https://reviewboard.mozilla.org/r/200502/#review205648

::: layout/painting/nsDisplayList.cpp:1120
(Diff revision 1)
>    return FindAnimatedGeometryRootFor(aItem->Frame());
>  }
>  
> +static bool
> +AnyAncestorOrPlaceholderIsModified(nsIFrame* aFrame,
> +                                   nsIFrame* aStopAtFrame = nullptr)

So, for DisplayListClipState we refer to 'ContainingBlockDescendants' and 'ContentDescendants' to distinguish between taking placeholder replacements into account or not.

We could do something similar here, maybe AnyContentAncestorModified?

::: layout/painting/nsDisplayList.cpp:1193
(Diff revision 1)
> +  // have an empty dirty rect set.
> +  // If the nearest stacking context for the modified frame is an ancestor of
> +  // of it, and if the stacking context is a descendant of the containing block
> +  // of this OOF frame, we override the dirty rect to ensure that the frame will
> +  // get marked.
> +  if (hasEmptyDirtyRect &&

I think we always want to set dirty = visible if we have a modified ancestor, even if the dirty rect isn't empty.

I think it might be possible to have an unrelated invalidation that gives a dirty rect that partial covers us, but one of our descendants would be excluded by it.

Comment 5

[Miko Mynttinen]

Comment on attachment 8929226 [details]
Bug 1417601 - Override dirty rect for OOF frames with containing block outside a modified stacking context

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200502/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b2c62ece7a3
Override dirty rect for OOF frames with containing block outside a modified stacking context r=mattwoodrow

Comment 7

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/1b2c62ece7a3

Comment 8

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8929226 [details]
Bug 1417601 - Override dirty rect for OOF frames with containing block outside a modified stacking context

Approval Request Comment
[Feature/Bug causing the regression]: bug 1352499. This is code that is preffed off, but we want to run a shield study enabling the pref.
[User impact if declined]: None, preffed off code.
[Is this code covered by automated tests?]: Yes, when the pref is enabled.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Code is preffed off.
[String changes made/needed]: None

Comment 9

[Gerry Chang [:gchang]]

Comment on attachment 8929226 [details]
Bug 1417601 - Override dirty rect for OOF frames with containing block outside a modified stacking context

Take this to support the shield study in 58. Beta58+.

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f49b7adf88e6