1424686 - regression: Wrong position of elements on tjournal.ru

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P1)

Description

[sviter33]

Attachment: hashtag block with WR.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20171210220040

Steps to reproduce:

Go to https://tjournal.ru/



Actual results:

With webrender on hashtag block rendering incorrect (see attach)


Expected results:

With webrender off all elements of the page is rendering correctly

Comment 1

[Darkspirit]

https://tjournal.ru/
That box jumps down after a few seconds and looks like on the screenshot. Does not happen without WebRender.

mozregression --good 2017-10-10 --bad 2017-12-10 --pref "layers.acceleration.force-enabled:true" "gfx.webrender.enabled:true" "gfx.webrendest.enabled:true" "gfx.webrender.layers-free:true" "gfx.webrender.blob-images:true" "image.mem.shared:true" "layout.display-list.retain:false"
> 6:11.27 INFO: Last good revision: b73eab93e81995fb0643b8ab8d4a004bd795ef58
> 6:11.27 INFO: First bad revision: 184c3c90cf23323a822f979d4cff1e10d629e245
> 6:11.27 INFO: Pushlog:
> https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b73eab93e81995fb0643b8ab8d4a004bd795ef58&tochange=184c3c90cf23323a822f979d4cff1e10d629e245

> 184c3c90cf23	Kartikaya Gupta — Bug 1410527 - Update how we tell WR about position:sticky elements. r=mstange

Comment 2

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

Thanks, I'll take a look.

Comment 3

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

I noticed that if I remove the "transform:translateZ(0)" on the sidebar_twister ancestor, that fixes the issue. That transform property triggers the creation of an nsDisplayTransform display item with a nonempty transform matrix, and so I suspect that WR is treating the sticky item as relative to that reference frame instead. I'll need to investigate more to wrap my head around this.

Comment 4

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

It might actually be the opposite problem: gecko treats the transformed nsDisplayTransform as a reference frame and so the bounds of the StickyPosition item inside it have a top-left corner of (0,0). WR interprets the (0,0) as relative to the viewport and so moves the thing down by 70px in order to maintain the sticky effect. This turns out to be 70px too much. Not yet sure what the best fix is.

Comment 5

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

Attachment: Simple test case

This is a simplified version that demonstrates the behaviour difference between WR (incorrect) and non-WR (correct). Still trying to wrap my head around the different scenarios that are possible here.

Comment 6

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

I spent almost the whole day yesterday thinking about this. Here's a braindump, using the attachment from comment 5 as the testcase:

- there are two reference frames in the content area. The first is the root one at (0,0). The second is the one at the top-left corner of the green div (assuming scroll position 0). This is positioned at y=70 during layout, but has an explicit y=+50 transform, so it appears at y=120.
- when gecko sends WR a display list, it includes a stacking context for the second reference frame; this stacking context has a y=120 translation transform. conceptually this is the transform that is needed to take items inside the second reference frame and bring them into the root reference frame. the rect for the green item therefore has coordinates at (0,0) in the untransformed space inside this stacking context.
- because the green item is sticky, we also send along the margin information for it. in this case top:70px is the margin. note that on the gecko side this is relative to the scrolling ancestor, which in this case is equivalent to the root reference frame. this also ignores the transformation effect of the second reference frame. in other words, conceptually what we want to do is resolve the "sticky" top:70px behaviour change of the green rect first, and *then* apply the y+50 transform afterwards.
- on the WR side, the information received amounts to "there is a green rect at 0,0 inside a stacking context with transform y=+120, and this green rect is sticky with a top-margin of 70px". given this information, WR is actually doing the right thing, i.e. it resolves the top-margin-sticky of 70px with respect to the ancestor scrollframe at y=0, and *then* applies the y+120 transform. But the visual result is different, because the green rect is now at y=190.

- the problem, i believe, is that nsDisplayTransform is combining its positioning information (y=70) with its transform (y+50) into a single transform [1] when we send it over to WR. WR therefore has no way to tell how much of that transform is actual CSS transform and how much is layout positioning. When WR is doing its sticky calculations, it takes care to adjust the scrollframe coordinates by layout positioning amounts [2] when entering a new reference frame. this is correct because WR coordinates are all in "untransformed" spaces relative to their own reference frame. But it turns out this adjustment is always a no-op because gecko never sends a nonzero layout positioning amount.

- the solution, therefore, is to actually send the layout positioning and transform components of a reference frame as separate things to WR. the stacking context helper API already allows this, but we always pass in a zero origin for the stacking context [3]. We would need to expose this via StackingContextHelper and take advantage of it in nsDisplayTransform::CreateWebRenderCommands to provide both components separately.

[1] https://searchfox.org/mozilla-central/rev/281d99b3d342b71d0653b872345761372d38f5c1/layout/painting/nsDisplayList.cpp#8320
[2] https://searchfox.org/mozilla-central/rev/9d920555ec81f1c9d4b7fa3b08e23eb88efb60e1/gfx/webrender/src/clip_scroll_node.rs#662-664
[3] https://searchfox.org/mozilla-central/rev/9d920555ec81f1c9d4b7fa3b08e23eb88efb60e1/gfx/layers/wr/StackingContextHelper.cpp#47

Comment 7

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> - the solution, therefore, is to actually send the layout positioning and
> transform components of a reference frame as separate things to WR. the
> stacking context helper API already allows this, but we always pass in a
> zero origin for the stacking context [3]. We would need to expose this via
> StackingContextHelper and take advantage of it in
> nsDisplayTransform::CreateWebRenderCommands to provide both components
> separately.
> 

I hacked this up and it seems to work as expected. I did find a small bug in the WR-side code, so I put a patch for that at https://github.com/servo/webrender/pull/2221

I'll need to clean up my hacky WIP so it's more reliable.

Comment 8

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

Even with the gecko-side fix the behaviour isn't completely correct; the background color items for the tags don't move properly and that's because their clip chains are different from the text. The background items have additional clip items for the rounded corners and I think we need servo/webrender#1964 to be fixed in order to make that work.

Comment 9

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

Try push with fixes and reftests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b05ec9e35da945ccd77d1c9b2e87a23ce8257437

As mentioned in comment 8 it doesn't fix this bug completely so I'll probably spin it out into a separate bug and leave this one open as depending on the clipchain work.

Comment 10

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ee4096e04d6cb2c1b6b7b131eb248df5b37aaf5

Comment 11

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

I split out the fix described in comment 6 onwards into bug 1426386. The remaining problem here will require the clip chain API changes which is being tracked by bug 1409442 on the gecko side and various issues on the WR side.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Once bug 1409442 and bug 1426386 are fixed we expect this to go away?

Comment 13

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

Yes. bug 1409442 requires both a WR-side change and a nontrivial Gecko-side change.

Comment 14

[sviter33]

This is still an issue for me.

Comment 15

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

This should be fixed with bug 1377187 (which is the "nontrival Gecko-side change" referred to in comment 13). That's on autoland now so hopefully it'll be in the next nightly.

Comment 16

[Darkspirit]

https://hg.mozilla.org/integration/autoland/shortlog/6a55c1fa635e
> mozregression --repo autoland --launch 07db52945b1f --pref gfx.webrender.all:true startup.homepage_welcome_url:'https://tjournal.ru'
bad

> mozregression --repo autoland --launch 976c2ea507b9 --pref gfx.webrender.all:true startup.homepage_welcome_url:'https://tjournal.ru'
looks good

Yay! Thanks!

Comment 17

[Darkspirit]

Fixed in Nightly 62 x64 20180508231737 de_DE @ Debian Testing.