1400982 - Fix clip chain building in ScrollingLayersHelper

Status: RESOLVED WORKSFORME

(Core :: Graphics: WebRender, enhancement)

Description

[Kartikaya Gupta (email:[e-mail])]

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

Comment 1

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

This is waiting on https://github.com/servo/webrender/issues/1777, I'm not actively working on it at the moment.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

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.