Open Bug 1730749 Opened 4 years ago Updated 1 month ago

Resolve conflicts between position:sticky and the ASR model

Categories

(Core :: Web Painting, task, P2)

task

Tracking

()

ASSIGNED

People

(Reporter: mstange, Assigned: botond)

References

(Blocks 3 open bugs)

Details

Attachments

(19 files, 1 obsolete file)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

We keep having trouble with position:sticky and ASR-related assertions. That's because, at the moment, position:sticky doesn't really play well with the ASR model and sort of only works by accident - during the ASR work I eventually ran out of steam and then hacked position:sticky into it just enough to pass the tests.

I think I figured out how to make the two play well together.

The biggest trouble at the moment is that there's no natural choice for the ASR we put on the position:sticky container display item, especially if the position:sticky frame contains fixed descendants - does the sticky item scroll with the "scrolled ASR" or with the "fixed ASR"? It kind of transitions between both. Having a "sticky ASR" would let us set that ASR on the sticky container item, and on the items inside it (but not any fixed descendants). Then the next question.is where that sticky ASR should be located in the ASR tree. Sticky elements can change between being scrolled and fixed, so you'd think the sticky ASR node could be between the fixed ASR and the scrolled ASR. But that would mean having to insert an ASR node above the scrolled ASR once we detect sticky items inside the scroll frame. So instead, I think we can make the sticky ASR a child of the scrolled ASR:

  • [A} Outer ASR
    • [B] Scrolled ASR
      • [C] Sticky ASR

Then the sticky ASR is modeled like a nested scroll frame. While the sticky item is in the scrolled state, C acts like a scroll frame that's not being scrolled, so the sticky item just moves with B. But whenever the sticky item is in the "fixed" state, conceptually, C "reverses" any scrolling of B so that the sticky items stay fixed within A.
The actual sticky positioning calculation can stay the way it is now - there is no need to model a scroll position of C.
Having C be a child of B is valid because C has finite bounds with respect to B: A sticky element's position with respect to the scrolled ASR is always constrained by its parent element, so in this example, the sticky element always has finite bounds with respect to B (even during its fixed state). This allows us, for example, to compute valid bounds for the opacity item in this example:

<div class="scrollFrame">
  <div class="opacity">
    <div class="sticky"></div>
  </div>
</div>

The opacity item gets ASR B, the sticky items get ASR C. The opacity item's bounds are with respect to ASR B; they're the bounds of the sticky contents +- the "sticky adjustment range" given by the size of the .opacity element. (Calculating these bounds might be a bit challenging but should be doable.)

And in this example:

<div class="scrollFrame">
  <div class="opacity">
    <div class="sticky">
      <div class="fixed"></div>
      <div class="contentsOfSticky"></div>
    </div>
  </div>
</div>

The ASR of the opacity item and of the fixed container item are ASR A, and the ASR for the contentsOfSticky items are the sticky ASR C. We don't have to calculate a container ASR for the sticky container item because there is no sticky container item any more.

So then the steps for this bug are:

  • Make sticky elements create ASRs for themselves.
  • Maybe rename ASRs to spatial nodes at this point.

Edit 2024-03-31: I've deleted the part about removing the sticky container item. I think having a sticky container item is fine and I see no advantages to be gained from removing it.

Severity: -- → S3
Priority: -- → P2
Blocks: 1791232

Wanted to share an update for anyone who might be keeping an eye on this bug:

Starting around June, Dan and I have started looking at this refactoring, with close guidance from Markus. We've been working on it as a secondary project in parallel with more pressing primary projects, but we've been making steady progress, and plan to continue doing so in the coming months. We'll share further updates (including a draft patch once we have one worth sharing) as things progress.

One alteration we're looking to make to the design outlined in comment 0 so far, is that we're looking to keep nsDisplayStickyPosition for the time being (leaving its removal for a future, no-functional-changes follow-up refactoring).

Thanks @botond! We're following this very closely, currently we have exclusions on FF for some effects that require this composition to work properly.

Just want to point out that we managed to fix this issue in one case by slapping a will-change: transform on a wrapper element that's under the clipping element and parent of the sticky element: https://jsbin.com/wonopujena/edit?css,output

Dan and I have been continuing to work on this refactoring as a secondary project, making a bit of progress each week. I've uploaded a WIP patch of our changes so far. At this stage, the patch causes a lot of test failures and is not really ready for use or testing yet; I'm mostly sharing it for potential feedback from other developers.

See Also: → 1941024
Blocks: 1941024
See Also: 1941024

Sharing another update:

Over the past few months, Dan and I have been continuing to plug away at this as a secondary project, and fixed many issues in our WIP patch, greening up a lot of tests, though there is more left to be done.

We're now planning to accelerate our efforts and focus more on this project as we try to get it across the finish line.

As we'll be working on this asynchronously, we'll be posting more frequent updates to this Bugzilla ticket.


We've recently re-worked part of the patch to make it easier to support retained display lists (specifically, we no longer have sticky ASRs store a pointer to the corresponding sticky display item). I fixed a few issues in the revised implementation approach, and kicked off a new CI run here; that will show us what test failures remain to be fixed.

(In reply to Botond Ballo [:botond] from comment #6)

We've recently re-worked part of the patch to make it easier to support retained display lists (specifically, we no longer have sticky ASRs store a pointer to the corresponding sticky display item). I fixed a few issues in the revised implementation approach, and kicked off a new CI run here; that will show us what test failures remain to be fixed.

The CI run is looking fairly good. Remaining test failures are:

  • test_group_overscroll_handoff.html
  • some reftest-snapshot tests
  • a couple of view-transitions web platform reftests

We started investigating the reftest-snapshot test failures. These all appear to have the same failure mode: there is a part of a sticky element that should be clipped out but isn't.

Here are some display list dumps for layout/reftests/position-sticky/top-6.html, one of the failing tests, with and without our patches.

Without:

SolidColor p=0x7f5e6059e4d8 f=0x7f5e605f2020() key=48 bounds(0,0,48000,60000) componentAlpha(0,0,0,0) clip() asr() clipChain() uniform  (opaque 0,0,48000,60000) reuse-state(None) (rgba 255,255,255,255)
SolidColor p=0x7f5e6059e020 f=0x7f5e605f20d0() key=48 bounds(0,0,48000,60000) componentAlpha(0,0,0,0) clip() asr() clipChain() uniform  (opaque 0,0,48000,60000) reuse-state(None) (rgba 255,255,255,255)
BackgroundColor p=0x7f5e6059e0d8 f=0x7f5e605f2f70( id:contain) key=6 bounds(480,-18420,47040,20400) componentAlpha(0,0,0,0) clip(480,480,47040,6000) asr() clipChain(0x7f5e6059e188 <480,480,47040,6000> [root asr]) uniform  (opaque 480,-18420,47040,20400) hitTestInfo(0x0) hitTestArea(0,0,0,0) reuse-state(None) (rgba 0.501961,0.501961,0.501961,1) backgroundRect(x=480, y=-18420, w=47040, h=20400)
Border p=0x7f5e6059e1e0 f=0x7f5e605f2f70( id:contain) key=9 bounds(480,-18420,47040,20400) componentAlpha(0,0,0,0) clip(480,480,47040,6000) asr() clipChain(0x7f5e6059e188 <480,480,47040,6000> [root asr])  reuse-state(None)
StickyPosition p=0x7f5e6059e348 f=0x7f5e605f3030( id:sticky) key=51 bounds(480,480,47040,300) componentAlpha(0,0,0,0) clip() asr() clipChain(0x7f5e6059e188 <480,480,47040,6000> [root asr])  (opaque 480,180,47040,600) reuse-state(None) (flags 0x0) (scrolltarget 0)
  BackgroundColor p=0x7f5e6059e288 f=0x7f5e605f3030( id:sticky) key=6 bounds(480,180,47040,600) componentAlpha(0,0,0,0) clip(480,480,47040,6000) asr() clipChain(0x7f5e6059e188 <480,480,47040,6000> [root asr]) uniform  (opaque 480,180,47040,600) hitTestInfo(0x0) hitTestArea(0,0,0,0) reuse-state(None) (rgba 0,0,0,1) backgroundRect(x=480, y=180, w=47040, h=600)

With:

SolidColor p=0x7ff533cf3550 f=0x7ff533c7e020(Viewport(-1)) key=48 bounds(0,0,48000,60000) componentAlpha(0,0,0,0) clip() asr(null) clipChain() uniform  (opaque 0,0,48000,60000) reuse-state(None) (rgba 255,255,255,255)
SolidColor p=0x7ff533cf3470 f=0x7ff533c7e0d8(Canvas(html)(-1)) key=48 bounds(0,0,48000,60000) componentAlpha(0,0,0,0) clip() asr(null) clipChain() uniform  (opaque 0,0,48000,60000) reuse-state(None) (rgba 255,255,255,255)
BackgroundColor p=0x7ff533cf3020 f=0x7ff533c7efe0(Block(div id=contain)(3) id:contain) key=6 bounds(480,-18420,47040,20400) componentAlpha(0,0,0,0) clip(480,480,47040,6000) asr(null) clipChain(0x7ff533cf30e8 <480,480,47040,6000> [root asr]) uniform  (opaque 480,-18420,47040,20400) hitTestInfo(0x0) hitTestArea(0,0,0,0) reuse-state(None) (rgba 0.501961,0.501961,0.501961,1) backgroundRect(x=480, y=-18420, w=47040, h=20400)
Border p=0x7ff533cf3140 f=0x7ff533c7efe0(Block(div id=contain)(3) id:contain) key=9 bounds(480,-18420,47040,20400) componentAlpha(0,0,0,0) clip(480,480,47040,6000) asr(null) clipChain(0x7ff533cf30e8 <480,480,47040,6000> [root asr])  reuse-state(None)
StickyPosition p=0x7ff533cf32d8 f=0x7ff533c7f0a8(Block(div id=sticky)(1) id:sticky) key=51 bounds(480,480,47040,6000) componentAlpha(0,0,0,0) clip() asr(null) clipChain(0x7ff533cf30e8 <480,480,47040,6000> [root asr])  (opaque 480,180,47040,600) reuse-state(None) (flags 0x0) (scrolltarget 0)
  BackgroundColor p=0x7ff533cf3200 f=0x7ff533c7f0a8(Block(div id=sticky)(1) id:sticky) key=6 bounds(480,180,47040,600) componentAlpha(0,0,0,0) clip() asr(sticky <0x7ff533c7f0a8>) clipChain(0x7ff533cf30e8 <480,480,47040,6000> [root asr]) uniform  (opaque 480,180,47040,600) hitTestInfo(0x0) hitTestArea(0,0,0,0) reuse-state(None) (rgba 0,0,0,1) backgroundRect(x=480, y=180, w=47040, h=600)

The first one was taken in a release build, so there are no frame names, but aside from that, a noticeable difference is that the BackgroundColor child of the StickyPosition item has clip(480,480,47040,6000) without our patches but clip() with our patches. I suspect this is what causes the rendering difference.

(In reply to Botond Ballo [:botond] from comment #8)

the BackgroundColor child of the StickyPosition item has clip(480,480,47040,6000) without our patches but clip() with our patches

That in turn is downstream of the BackgroundColor item having a different ASR.

Without our patches we have:

asr() clipChain(0x7f5e6059e188 <480,480,47040,6000> [root asr])

With our patches:

asr(sticky <0x7ff533c7f0a8>) clipChain(0x7ff533cf30e8 <480,480,47040,6000> [root asr])

nsDisplayItem::GetClip() returns "the clip in the item's clip chain that's associated with thte item's ASR". Without our patches, the item has a null ASR, so the <480,480,47040,6000> in the clip chain matches; with our patches, the item has a sticky ASR so it no longer matches.

The clip in question is the scroll port clip of an overflow: hidden subframe containing the sticky element. This subframe is not active (it does not get an ASR), so its scroll port clip gets associated with the parent ASR, which in this case is the root.

So, I think the issue (or, at least, an issue) here is that we give the sticky item an ASR when its enclosing scroll frame does not have one. I think we should maybe only give ASRs to sticky items which are "active", in the sense that the scroll frame with respect to which they are sticky is active (gets an ASR)?

Discussed this with Markus.

(In reply to Botond Ballo [:botond] from comment #10)

So, I think the issue (or, at least, an issue) here is that we give the sticky item an ASR when its enclosing scroll frame does not have one. I think we should maybe only give ASRs to sticky items which are "active", in the sense that the scroll frame with respect to which they are sticky is active (gets an ASR)?

Markus confirmed that we should only give ASRs to "active" sticky items.

However, it's not entirely clear that doing so would be a correctness problem (rather than just doing needless work from a performance point of view).

The fact that this is breaking rendering suggests that there may be a latent bug in the draw-snapshot codepath. Specifically, nsDisplayList::Paint should probably be applying all the clips in an item's clip chain, not just the item's GetClip().

So, next steps here:

  1. Only create ASRs for "active" sticky items (ones for which shouldFlatten=false).
  2. Update nsDisplayList::Paint to apply all clips in an item's clip chain.

If (1) is sufficient to fix all reftest-snapshot failures, (2) can be left as a follow-up improvement.

(In reply to Botond Ballo [:botond] from comment #11)

So, next steps here:

  1. Only create ASRs for "active" sticky items (ones for which shouldFlatten=false).

We've implemented this, and it does cause all reftest-snapshot tests in the position-sticky directory to pass locally. New CI run to see what remains to be fixed here.

(In reply to Botond Ballo [:botond] from comment #12)

New CI run to see what remains to be fixed here.

Here is a newer push with the patch series rebased onto a more recent baseline. Relative to the state of things in comment 7, we're not only passing reftest-snapshot tests, but the view-transitions web platform reftest failures are gone as well (thanks, I'm guessing to work on view transitions that has happened in parallel)!

So, it seems only the failure of test_group_overscroll_handoff.html remains to be fixed.

(In reply to Botond Ballo [:botond] from comment #13)

So, it seems only the failure of test_group_overscroll_handoff.html remains to be fixed.

I've spent some time trying to debug this failure, and it's a very strange one.

The test hangs early during the execution of helper_position_sticky_scroll_handoff.html, during the waitUntilApzStable call.

The hang happens consistently, but the place where it occurs changes intermittently between two places. The first is during promiseAllPaintsDone, where the code in paint_listener.js waits for a MozAfterPaint event that never comes. I guess we are somehow breaking the paint listener mechanism / its platform implementation on the affected page, which is somewhat surprising, though it's the less surprising of the two hangs.

The more surprising hang is in promiseFocus, more specifically in this executeSoon call. It seems the callback is just never called. I tried tracing the implementation but got very quickly into the weeds of EventQueue / TaskController stuff I don't understand well.

My next step is probably to debug the first hang (MozAfterPaint never arrives) further, as it seems more tractable, though if anyone has suggestions for how to debug "executeSoon never calls its callback", I would be curious to hear it.

It turns out the hangs discussed in the previous comment have a less mysterious cause than it seemed: the first time we try to paint helper_position_sticky_scroll_handoff.html, we get into an infinite loop during display list building. (I haven't delved into the details yet, but we somehow end up with an ASR which is set as its own parent, and then this loop iterates indefinitely.)

Sometimes the first paint of the page is already scheduled by the time promiseFocus gets to the executeSoon call, other times it only gets scheduled afterwards, hence the two different manifestations of the hang.

(In reply to Botond Ballo [:botond] from comment #15)

the first time we try to paint helper_position_sticky_scroll_handoff.html, we get into an infinite loop during display list building. [...] we somehow end up with an ASR which is set as its own parent, and then this loop iterates indefinitely.)

The issue is that this page contains an element which is position: sticky and overflow: auto; this results in a frame which is both a sticky frame, and a scroll frame. Since we now create ASRs for both frame types, this frame has two associated ASRs. However, we cache ASRs on frames in a frame property, and so the two ASRs get mixed up. At some point, we end up querying what we expect to be the scroll ASR, but get back the sticky ASR because that has been written into the frame property more recently.

(In reply to Botond Ballo [:botond] from comment #16)

The issue is that this page contains an element which is position: sticky and overflow: auto; this results in a frame which is both a sticky frame, and a scroll frame. Since we now create ASRs for both frame types, this frame has two associated ASRs. However, we cache ASRs on frames in a frame property, and so the two ASRs get mixed up. At some point, we end up querying what we expect to be the scroll ASR, but get back the sticky ASR because that has been written into the frame property more recently.

Fixed this by using a second frame property to store sticky ASRs.

New CI run here. We are now passing test_group_overscroll_handoff.html on most platforms, but it's still failing on Linux with fission disabled. However, this is a different failure: it's the helper_position_fixed_scroll_handoff-5.html subtest that appears to be hanging.

(In reply to Botond Ballo [:botond] from comment #17)

New CI run here. We are now passing test_group_overscroll_handoff.html on most platforms, but it's still failing on Linux with fission disabled. However, this is a different failure: it's the helper_position_fixed_scroll_handoff-5.html subtest that appears to be hanging.

That's likely bug 1966348 and so will be fixed by updating to tip.

(In reply to Timothy Nikkel (:tnikkel) from comment #18)

(In reply to Botond Ballo [:botond] from comment #17)

New CI run here. We are now passing test_group_overscroll_handoff.html on most platforms, but it's still failing on Linux with fission disabled. However, this is a different failure: it's the helper_position_fixed_scroll_handoff-5.html subtest that appears to be hanging.

That's likely bug 1966348 and so will be fixed by updating to tip.

Indeed, that failure was unrelated to these patches. With the fix for that included, we now have a green CI run!

No one creates ASRs of this kind yet (that will happen in a later patch).

Consumer code is adjusted to check for the ASR kind as appropriate.
To make this easier, two helper functions GetNearestScrollASR()
and GetNearestScrollASRViewId() are added.

Assignee: nobody → botond
Status: NEW → ASSIGNED

The implementation only needs the display item's frame and its
"per frame key". In some cases these inputs can be known before
the display item instance itself is available. A later patch in
this series makes use of this.

This paves the way for moving the code that creates the animation id
to earlier during WebRender display list building, where we won't
have the sticky item available to store the animation id on it.

This allows the code that checks the flag to avoid needing the
sticky display item.

A frame can be both a scroll frame and sticky, in which case it has
two associated ASRs. Using distinct frame properties allows retaining
information about both.

This is needed because the sticky item's own ASR may not be the sticky ASR,
but the item needs to know the sticky ASR to look up the corresponding
spatial id in CreateWebRenderCommands.

The current implementation relies of mSpaceAndClipChain, but that assumes
WebRender display list building has actually entered the sticky display
item.

A later patch will move this call to earlier during WebRender display
list building (ClipManager), where we'll need to provide the parent
spatial id explicitly.

This is conceptually similar to the existing mapping of ViewIDs to spatial ids
for scroll ASRs, and the two could be unified in the future.

The patch also generalizes ClipManager::GetScrollLayer into GetSpatialId.

In nsDisplayStickyPosition::CreateWebRenderCommands, we use the new mapping,
in preparation for moving the DefineStickyFrame call out of there.

This patch generalizes ClipManager::DefineScrollLayers into
DefineSpatialNodes, and introduces ClipManager::DefineStickyNode to
handle creating a spatial node for a sticky ASR.

For ease of review, this patch temporarily assumes that we can get hold of
the sticky display item in ClipManager, and contains a stub function for
doing so. That allows the initial implementation of DefineStickyNode to
mostly be cut-and-paste from nsDisplayStickyPosition::CreateWebRenderCommands.

The next patch will revise the implementation of DefineStickyNode to
avoid the dependency on the sticky display item.

This is no longer used now that the creation of the sticky animation id
happens in ClipManager.

The use of this override for sticky content was essentially a workaround
for not having sticky ASRs. As we now have them, we get the desired
spatial id the usual way, mapping from ASR to spatial id.

To avoid sticky content checkerboarding when we've scrolled farther
than the displayport, we do not apply the displayport clip to it.

This was previously implemented in a hacky way in ClipManager, but
the introduction of sticky ASRs complicates that implementation.

Instead, this patch implements the omission of displayport clips
for sticky content in the Gecko display list itself.

Attachment #9441089 - Attachment is obsolete: true

Note, the patches implementing this refactoring were coauthored by Dan Robertson; I'm not sure what's the best way to reflect that in the metadata (the original commit authorship information on the patches did not survive patch splitting / combination and moz-phab).

Blocks: 1966010
Attachment #9495401 - Attachment description: Bug 1730749 - Introduce a new kind of ASR for sticky content. r=mstange → Bug 1730749 - Introduce a new kind of ASR for sticky content. r=mstange,tnikkel
Attachment #9495402 - Attachment description: Bug 1730749 - Add a static version of nsDisplayStickyPosition::ShouldGetStickyAnimationId() for use without a display item instance. r=mstange → Bug 1730749 - Add a static version of nsDisplayStickyPosition::ShouldGetStickyAnimationId() for use without a display item instance. r=mstange,tnikkel
Attachment #9495403 - Attachment description: Bug 1730749 - Add a static version of nsDisplayItem::GetPerFrameKey() for use without a display item instance. r=mstange → Bug 1730749 - Add a static version of nsDisplayItem::GetPerFrameKey() for use without a display item instance. r=mstange,tnikkel
Attachment #9495404 - Attachment description: Bug 1730749 - Allow using WebRenderUserData without a display item. r=mstange → Bug 1730749 - Allow using WebRenderUserData without a display item. r=mstange,tnikkel
Attachment #9495405 - Attachment description: Bug 1730749 - Reimplement nsDisplayStickyPosition::UpdateScrollData to get the animation id from WebRenderUserData instead. r=mstange → Bug 1730749 - Reimplement nsDisplayStickyPosition::UpdateScrollData to get the animation id from WebRenderUserData instead. r=mstange,tnikkel
Attachment #9495406 - Attachment description: Bug 1730749 - Store the 'should flatten' flag on StickyScrollContainer. r=mstange → Bug 1730749 - Store the 'should flatten' flag on StickyScrollContainer. r=mstange,tnikkel
Attachment #9495407 - Attachment description: Bug 1730749 - Create ASRs for sticky frames in BuildDisplayListForStackingContext. r=mstange → Bug 1730749 - Create ASRs for sticky frames in BuildDisplayListForStackingContext. r=mstange,tnikkel
Attachment #9495408 - Attachment description: Bug 1730749 - Use a distinct frame property for sticky ASRs. r=mstange → Bug 1730749 - Use a distinct frame property for sticky ASRs. r=mstange,tnikkel
Attachment #9495409 - Attachment description: Bug 1730749 - Store the sticky ASR on sticky display items. r=mstange → Bug 1730749 - Store the sticky ASR on sticky display items. r=mstange,tnikkel
Attachment #9495410 - Attachment description: Bug 1730749 - Allow the caller of DisplayListBuilder::DefineStickyFrame to specify the parent spatial id explicitly. r=mstange → Bug 1730749 - Allow the caller of DisplayListBuilder::DefineStickyFrame to specify the parent spatial id explicitly. r=mstange,tnikkel
Attachment #9495411 - Attachment description: Bug 1730749 - Maintain a mapping of sticky ASRs to spatial ids inside DisplayListBuilder. r=mstange → Bug 1730749 - Maintain a mapping of sticky ASRs to spatial ids inside DisplayListBuilder. r=mstange,tnikkel
Attachment #9495412 - Attachment description: Bug 1730749 - Handle sticky ASRs in ClipManager. r=mstange → Bug 1730749 - Handle sticky ASRs in ClipManager. r=mstange,tnikkel
Attachment #9495413 - Attachment description: Bug 1730749 - Remove the dependency on the sticky display item in ClipManager::DefineStickyNode. r=mstange → Bug 1730749 - Remove the dependency on the sticky display item in ClipManager::DefineStickyNode. r=mstange,tnikkel
Attachment #9495414 - Attachment description: Bug 1730749 - Remove nsDisplayItem::mWrStickyAnimationId. r=mstange → Bug 1730749 - Remove nsDisplayItem::mWrStickyAnimationId. r=mstange,tnikkel
Attachment #9495415 - Attachment description: Bug 1730749 - Remove the ASR override for sticky items. r=mstange → Bug 1730749 - Remove the ASR override for sticky items. r=mstange,tnikkel
Attachment #9495416 - Attachment description: Bug 1730749 - Rework handling of displayport clips for sticky content. r=mstange → Bug 1730749 - Rework handling of displayport clips for sticky content. r=mstange,tnikkel
Attachment #9495417 - Attachment description: Bug 1730749 - Relax the 'finite bounds' assertion for sticky content. r=mstange → Bug 1730749 - Relax the 'finite bounds' assertion for sticky content. r=mstange,tnikkel
Attachment #9495414 - Attachment description: Bug 1730749 - Remove nsDisplayItem::mWrStickyAnimationId. r=mstange,tnikkel → Bug 1730749 - Remove nsDisplayStickyPosition::mWrStickyAnimationId. r=mstange,tnikkel
Attachment #9495409 - Attachment description: Bug 1730749 - Store the sticky ASR on sticky display items. r=mstange,tnikkel → Bug 1730749 - Introduce ActiveScrolledRoot::GetStickyASRForFrame(). r=mstange,tnikkel

We need to retain sticky ASRs for:

  • looking up the ASR from the frame
  • sharing the ASR between continuations of a sticky element

We need to retain scroll ASRs for consistency (if we retain
sticky ASRs but not scroll ASRs, the retained sticky ASRs will
point to stale scroll ASR parents).

This is needed to avoid upsetting the assumptions made by the code to
track container ASRs. Without this, layout/base/crashtests/1464641.html
triggers the assertion in the PickAncestor() call in
SetCurrentActiveScrolledRoot().

I posted an updated version of the patch series. Here is a summary of the updates:

  • [New patch D264156]: ASRs are now always stored in a frame property, not just when retained-DL is enabled. This is needed in part due to the next bullet point, but also for the existing "Introduce ActiveScrolledRoot::GetStickyASRForFrame()" patch to work as intended.
  • [New patch D264157]: If a sticky element creates multiple continuation frames, they now share a sticky ASR. More details in the commit message.
  • [Changes to patch D254147]: Sticky ASRs are now created earlier in BuildDisplayListForStackingContext, such that the they're already installed on the display list builder by the time the containerASRTracker is created.
  • [Change to patch D254157]: The conditions under which the 'finite clip' assertion is relaxed were narrowed.

(The latest update to the patch series is just a rebase.)

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: