1368551 - Make layout/reftests/async-scrolling/split-opacity-layers-1.html pass on linux64-qr with APZ enabled

Status: RESOLVED FIXED

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

Description

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

With WR+APZ enabled, the layout/reftests/async-scrolling/split-opacity-layers-1.html reftest fails [1]. It looks like one of the purple divs is 10 pixels off from where it should be.

I was able to modify the scrolling example in webrender to reproduce a similar scenario. It looks like webrender internally treats a "None" transform differently from a "Some(PropertyBinding::Value(LayoutTransform::identity()))" transform. Right now we pretty much always use the identity transform on stacking contexts but maybe we shouldn't be? See bug 1345577.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=68c5c545d47a35f45b6af5c79dcf2e6e59c00bf3&selectedJob=102461411

Comment 1

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

I filed servo/webrender#1304 for the issue since it seems like unexpected behaviour in WR.

Comment 2

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

Hacking it up so that identity transforms get turned into "None" fixed the test for me locally. I did a try push to see what else changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b8d2a2cfb0db0016aee29e08af1b913df67ed10

Comment 3

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

The patch fixes the split-opacity-layers-1.html test as expected, and doesn't appear to affect any other test. All the other failures are exactly the same as without this patch.

Comment 4

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

I realized that we have another PushStackingContext codepath which this didn't affect. I should fix that one too.

Comment 5

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

Attachment: Bug 1368551 - Remove unneccessary overload of PushStackingContext.

This overload is only called in one place, so we can just inline it. It
makes the API a little simpler.

Review commit: https://reviewboard.mozilla.org/r/144640/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144640/

Comment 6

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

Attachment: Bug 1368551 - Don't send transforms to WR in push_stacking_context if they are identity.

If we set a transform in push_stacking_context, it changes the internal
WebRender behaviour to make that stacking context a reference frame, and
things inside it are positioned differently. This is true even if the
transform is an identity transform.

In most cases we are hitting this and sending an identity transform
through, when in fact we want to be sending a None value to WebRender so
that it doesn't create reference frames. This is a partial fix, a proper
fix will be done in bug 1345577 by separating the CSS transform from the
other transforms that FrameLayerBuilder invents.

Review commit: https://reviewboard.mozilla.org/r/144642/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144642/

Comment 7

[Peter Chang[:pchang]]

Comment on attachment 8873170 [details]
Bug 1368551 - Remove unneccessary overload of PushStackingContext.

https://reviewboard.mozilla.org/r/144640/#review148618

Comment 8

[Peter Chang[:pchang]]

Comment on attachment 8873171 [details]
Bug 1368551 - Don't send transforms to WR in push_stacking_context if they are identity.

https://reviewboard.mozilla.org/r/144642/#review148626

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/85e598624e0f
Remove unneccessary overload of PushStackingContext. r=pchang
https://hg.mozilla.org/projects/graphics/rev/67f689dd202d
Don't send transforms to WR in push_stacking_context if they are identity. r=pchang

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85e598624e0f
https://hg.mozilla.org/mozilla-central/rev/67f689dd202d