Closed Bug 1400982 Opened 7 years ago Closed 7 years ago

Fix clip chain building in ScrollingLayersHelper

Categories

(Core :: Graphics: WebRender, enhancement)

Other Branch
enhancement
Not set
normal

Tracking

()

RESOLVED WORKSFORME
mozilla57
Tracking Status
firefox57 --- unaffected

People

(Reporter: kats, Unassigned)

References

Details

(Whiteboard: [gfx-noted])

I'm looking into why layout/reftests/async-scrolling/sticky-pos-scrollable-2.html is failing in layers-free, and I think there's a bug in the existing ScrollingLayersHelper code to build the scroll/clip chains. We have a display list like so: StickyPosition ... asr(<A>) clipChain(<B> [A] <C> [root asr]) ... BackgroundColor ... asr(<A>) clipChain(<D> [A] <C> [root asr]) At the time we process the BackgroundColor item, the stuff on the WR stack is like so (top of stack is at the top of the list): Sticky id=6 (the sticky clip from the StickyPosition) Clip for B ScrollLayer for A Clip for C When we go into ScrollingLayersHelper for the BackgroundColor item, the call at [1] is a no-op because the leafmost ASR (A) is already the topmost scroll id on the stack. We then go into [2]. aItem->GetClipChain points to clip D. D hasn't been defined yet, so we recurse at [3] with D's parent, which is C. However, the topmost clip on the stack is actually clip B, so instead of bailing out at [4] we continue on and re-push C onto the stack at [5]. So after ScrollingLayersHelper is done building all the things we end up with a stack that looks like this: Clip for D Clip for C Sticky id=6 Clip for B ScrollLayer for A Clip for C and that's when we push the rect for the blue div. Since the WR clip chain for the rect is D -> C, it's not scrolled by A (and also not inside the sticky clip), so the rect doesn't async scroll at all. [1] http://searchfox.org/mozilla-central/rev/05c4c3bc0cfb9b0fc66bdfc8c47cac674e45f151/gfx/layers/wr/ScrollingLayersHelper.cpp#120 [2] http://searchfox.org/mozilla-central/rev/05c4c3bc0cfb9b0fc66bdfc8c47cac674e45f151/gfx/layers/wr/ScrollingLayersHelper.cpp#128 [3] http://searchfox.org/mozilla-central/rev/05c4c3bc0cfb9b0fc66bdfc8c47cac674e45f151/gfx/layers/wr/ScrollingLayersHelper.cpp#222 [4] http://searchfox.org/mozilla-central/rev/05c4c3bc0cfb9b0fc66bdfc8c47cac674e45f151/gfx/layers/wr/ScrollingLayersHelper.cpp#217 [5] http://searchfox.org/mozilla-central/rev/05c4c3bc0cfb9b0fc66bdfc8c47cac674e45f151/gfx/layers/wr/ScrollingLayersHelper.cpp#240
What would be better is if we could explicitly set the parent of a particular clip in webrender. Then we wouldn't have to jump so many hoops in terms of making sure the stack is just right before defining a new clip.
Actually explicitly setting the parent is orthogonal. It might help a bit but I think the fundamental problem here is that we're carrying a clip on the stack while recursing when we shouldn't be. In the example in comment 0, the clip B is supposed to be local to the sticky item, but we're pushing it on the stack and leaving there while we process the nested list. Instead I think what we want is for the "sticky id=6" clip to be outside clip B, and we should pop off B when we're done with the sticky item and about to recurse into the child. That way the sticky clip (which is tightly-bound to ScrollLayer A) remains on the stack but clip B does not. Then the child BackgroundColor can push clip D on top of the sticky item and push its rect.
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [gfx-noted][wr-mvp][triage] → [wr-mvp] [gfx-noted]
Target Milestone: --- → mozilla57
I looked a little bit more at sticky positioning today, this time using layout/reftests/position-sticky/top-3.html as my test page. I bypassed the above clip-nested problem by increment/decrementing mMaskClipCount in Push/PopStickyFrame - this ensures that any clips that we push inside a sticky frame will be defined anew. In theory it should fix the problem but is perhaps not optimal. It didn't work, and I managed to repro a problem in the standalone scrolling.rs example app too. Filed that as https://github.com/servo/webrender/issues/1751.
This is waiting on https://github.com/servo/webrender/issues/1777, I'm not actively working on it at the moment.
Assignee: bugmail → nobody
Status: ASSIGNED → NEW
Priority: P1 → P2
The fix for servo/webrender#1777 landed as part of the WR update in bug 1407213. So this should be unblocked now, I'll pick it up eventually.
Depends on: 1407213
Both layout/reftests/async-scrolling/sticky-pos-scrollable-2.html and layout/reftests/position-sticky/top-3.html (and most of the other sticky tests) are passing now. Mostly they were fixed by removing extraneous clips in bug 1403697, and the proper WR fix should make sure that sticky behaves correctly even if we bring back the extraneous clips.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
Priority: P2 → --
Whiteboard: [wr-mvp] [gfx-noted] → [gfx-noted]
You need to log in before you can comment on or make changes to this bug.