Closed Bug 1417601 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Web Painting, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: miko, Assigned: miko)

References

Details

Attachments

(2 files)

This is an existing bug that a patch for bug 1415485 uncovered.
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 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.
Attachment #8929226 - Flags: review?(matt.woodrow) → review+
Pushed by mikokm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1b2c62ece7a3
Override dirty rect for OOF frames with containing block outside a modified stacking context r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/1b2c62ece7a3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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
Attachment #8929226 - Flags: approval-mozilla-beta?
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+.
Attachment #8929226 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1417907
No longer depends on: 1417907
You need to log in before you can comment on or make changes to this bug.