Deal with reftest-no-display-list failure in retained-dl-style-change-stacking-context-3.html with apz.allow_zooming=true
Categories
(Core :: Panning and Zooming, task, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox80 | --- | fixed |
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(3 files)
https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&&revision=0e45b7474920f8df1f984cf857d71349cbec4633 is a recent try push with apz.allow_zooming=true on desktop. It shows a failure in the retained-dl-style-change-stacking-context-3.html test because there is an element here that has its display items rebuilt when it shouldn't, as a result of a sibling element mutating.
I investigated this locally, and it looks like this might be intentional. In particular, when we walk the display list after the mutation, the dirty rect is initially (correctly) set to just the mutated element. However when we descend into the out-of-flow fixed-pos container, we run this code when apz.allow_zooming=true which then expands the dirty rect to be larger. That larger dirty rect is intersected with the size of the fixed-pos container here but it still means the entire fixed-pos div is inside the dirty rect, and so the display list for the reftest-no-display-list item gets rebuilt.
The ComputeVisibleRectForFrame code dates back a while and I'm wondering if it doesn't properly handle the partial display list builds that the retained-DL code does. Maybe we need to make that dirty-rect expansion conditional on not being in a partial update.
|
Assignee | |
Comment 1•5 years ago
|
||
Ugh. All my debugging was based on the buggy version of the test, where second div is inside the fixed-pos item. The corrected version of the test (with the missing </div> replaced so that the page structure reflects the indentation of the elements) still fails, so I need to debug again.
|
|
Assignee | |
Comment 2•5 years ago
|
||
Actually I think it's an indentation bug. If I add a </div> there it fails even on current m-c with no other changes. Phew.
|
|
Assignee | |
Comment 3•5 years ago
|
||
This retains the structure of the page, but fixes the indentation and adds a
missing closing tag. Note that if we treat the indentation as correct and move
the 'second' div outside the position-fixed div, then the test fails. So if that
is what the test is intending to be testing, we should handle that in a new bug
where we add a new copy of this test with that structure, since the current
test is not testing that at all.
|
|
Assignee | |
Comment 4•5 years ago
|
||
Depends on D82214
|
|
Assignee | |
Comment 5•5 years ago
|
||
When doing a display list build, there's some code that expands the dirty
and visible rects for out-of-flow items to include the entire visual viewport
or displayport, because out-of-flow items are specially positioned in the
compositor and may become visible without the main thread knowing about it.
However, this happens even during partial updates using retained display lists,
which I believe is undesirable because the preceding non-partial update should
already have painted all the right things. The partial update should be
restricted to the part of the OOF item that actually need repainting, and the
merging of the partial update into the full display list should produce a final
display list that covers everything.
Depends on D82215
|
|
Assignee | |
Comment 6•5 years ago
|
||
New try push with the updated part 1: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=a91c7f7d124eef31a2dbcd3c491e138f353a0949
|
||
Comment 8•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/58866232298c
https://hg.mozilla.org/mozilla-central/rev/aaf49a6cdf1a
https://hg.mozilla.org/mozilla-central/rev/ff1a0c64ae96
Description
•