Closed Bug 1428993 Opened 2 years ago Closed 2 years ago

Graphical glitch with retained display lists

Categories

(Core :: Web Painting, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: birtles, Assigned: miko)

References

Details

Attachments

(5 files, 1 obsolete file)

See forthcoming test.

If you hover the quarter-circles in the bottom of the test they should enlarge and cover the white area but most of the time they incorrectly go underneath the white area. Reproduces for me in Nightly on Linux and Windows. Does not reproduce if I disable retained display lists.
Attached file CSS for test
Attached file Test (obsolete) —
Reproduces slightly more readily on the red circle than the green.
Sorry, the CSS file is massive. I reduced the HTML file pretty far, but the CSS could use a lot more reduction. The more I reduced the HTML, however, the less likely it would reproduce so I thought I'd post the test in its complex-but-reproduces-very-easily state.
Attached file Test
Just updating the test title
Attachment #8940979 - Attachment is obsolete: true
Thank you for the report. I can reproduce this locally on macOS.
Assignee: nobody → mikokm
Status: NEW → ASSIGNED
Attached file simplified.html
Simplified testcase
Priority: -- → P2
Comment on attachment 8943025 [details]
Bug 1428993 - Part 2: Override dirty rect for stacking contexts between OOF frame placeholder and the containing block

https://reviewboard.mozilla.org/r/213288/#review219088

::: layout/painting/RetainedDisplayListBuilder.cpp:646
(Diff revision 1)
>  
> +    // If the current frame is an OOF frame, DisplayListBuildingData needs to be
> +    // set on all the ancestor stacking contexts of the  placeholder frame, up
> +    // to the containing block of the OOF frame. This is done to ensure that the
> +    // content that might be behind the OOF frame is built for merging.
> +    nsIFrame* placeholder = previousFrame->HasAnyStateBits(NS_FRAME_OUT_OF_FLOW)

We could move this whole placeholder handling block into a helper, and put it before the TransformFrameRectToAncestor call to avoid needing previousFrame.

::: layout/painting/RetainedDisplayListBuilder.cpp:647
(Diff revision 1)
> +    // If the current frame is an OOF frame, DisplayListBuildingData needs to be
> +    // set on all the ancestor stacking contexts of the  placeholder frame, up
> +    // to the containing block of the OOF frame. This is done to ensure that the
> +    // content that might be behind the OOF frame is built for merging.
> +    nsIFrame* placeholder = previousFrame->HasAnyStateBits(NS_FRAME_OUT_OF_FLOW)
> +                          ? do_QueryFrame(previousFrame->GetPlaceholderFrame())

Do we need do_QueryFrame here? We should only need it when we're casting to a derived type, nsPlaceholderFrame -> nsIFrame should just work.
Attachment #8943025 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8943365 [details]
Bug 1428993 - Part 1: Split RetainedDisplayListBuilder::ComputeRebuildRegion() and PreProcessDisplayList() into multiple functions

https://reviewboard.mozilla.org/r/213684/#review219452
Attachment #8943365 - Flags: review?(matt.woodrow) → review+
Pushed by mikokm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7f88ebf8d0d1
Part 1: Split RetainedDisplayListBuilder::ComputeRebuildRegion() and PreProcessDisplayList() into multiple functions r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/080c07796f4c
Part 2: Override dirty rect for stacking contexts between OOF frame placeholder and the containing block r=mattwoodrow
Pushed by mikokm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2eb0a49e3192
Part 1: Split RetainedDisplayListBuilder::ComputeRebuildRegion() and PreProcessDisplayList() into multiple functions r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/2c2e56a87ad1
Part 2: Override dirty rect for stacking contexts between OOF frame placeholder and the containing block r=mattwoodrow
The failing reftest layout/reftests/display-list/1428993-1.html is now disabled on Android due to weird behavior around the button edges. The second added reftest layout/reftests/display-list/1428993-2.html tests the same functionality.
Flags: needinfo?(mikokm)
https://hg.mozilla.org/mozilla-central/rev/2eb0a49e3192
https://hg.mozilla.org/mozilla-central/rev/2c2e56a87ad1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Attachment 8940980 [details] still doesn't seem to be fixed for me in the latest nightly.
Flags: needinfo?(nerli)
Sorry, I meant to direct that needinfo to Miko!
Flags: needinfo?(nerli) → needinfo?(mikokm)
(In reply to Brian Birtles (:birtles) from comment #20)
> Attachment 8940980 [details] still doesn't seem to be fixed for me in the
> latest nightly.

Thank you again Brian.

It seems that there are some additional problems that the simplified testcase did not catch. I will investigate this again, and file a new bug for possible follow-up work.
Flags: needinfo?(mikokm)
Thanks Miko!

Please CC me on any follow-up bugs.
Depends on: 1432553
Depends on: 1446667
Depends on: 1453601
You need to log in before you can comment on or make changes to this bug.