Closed Bug 1426386 Opened 2 years ago Closed 2 years ago

Don't combine the offset of a stacking context into its transform, if it has one

Categories

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

Other Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted][triage])

Attachments

(3 files)

Right now we do this thing when building the WR display list where if we encounter a gecko stacking context, we set the origin of the corresponding WR stacking context to (0,0) and instead include the gecko stacking context's offset (from the parent stacking context) as a translation component on the WR stacking context's transform. This mostly works fine, but falls down in at least one case: the case where there is a position:sticky item such that there is a transformed stacking context between the sticky item and ancestor scrolling element. This is one of the problems in bug 1424686.
These patches apply on top of bug 1422057. Conceptually they're independent and can be trivially rebased without those patches, but I wrote them in that order and haven't tested these in isolation.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20f106f688e43571803d78e1ac7f9504effe9c9d
By the way, https://bugzilla.mozilla.org/show_bug.cgi?id=1424686#c6 has some more context on this bug, it might be useful.
Comment on attachment 8937994 [details]
Bug 1426386 - Expose WR stacking context bounds via StackingContextHelper.

https://reviewboard.mozilla.org/r/208724/#review215788
Attachment #8937994 - Flags: review?(mstange) → review+
Comment on attachment 8937995 [details]
Bug 1426386 - Split the transform sent to webrender back into the transform and positioning components.

https://reviewboard.mozilla.org/r/208726/#review215792

::: layout/painting/nsDisplayList.cpp:8531
(Diff revision 1)
>  
>  Matrix4x4
> -nsDisplayTransform::GetTransformForRendering()
> +nsDisplayTransform::GetTransformForRendering(LayoutDevicePoint* aOutOrigin)
>  {
>    if (!mFrame->HasPerspective() || mTransformGetter || mIsTransformSeparator) {
> +    if (!mTransformGetter && !mIsTransformSeparator && aOutOrigin) {

I don't know enough about our implementation of transforms to be able to tell whether this condition makes sense.

Also, why don't we need to do anything when the frame has perspective?

::: layout/painting/nsDisplayList.cpp:8595
(Diff revision 1)
>                                              mozilla::wr::IpcResourceUpdateQueue& aResources,
>                                              const StackingContextHelper& aSc,
>                                              WebRenderLayerManager* aManager,
>                                              nsDisplayListBuilder* aDisplayListBuilder)
>  {
> -  Matrix4x4 newTransformMatrix = GetTransformForRendering();
> +  bool isReferenceFrame = mFrame->IsTransformed();

What exactly determines whether this nsDisplayTransform becomes a WR reference frame? Is this determined in Gecko code or in WebRender code?

If it's determined in WebRender code, do Gecko and WebRender come to the same conclusion for an element with transform:none;will-change:transform;? How about for an element with a CSS transform animation which is currently resting at transform:none?
Attachment #8937995 - Flags: review?(mstange) → review+
Comment on attachment 8937996 [details]
Bug 1426386 - Add reftests for position:sticky items inside a transform.

https://reviewboard.mozilla.org/r/208728/#review215794
Attachment #8937996 - Flags: review?(mstange) → review+
(In reply to Markus Stange [:mstange] from comment #7)
> >    if (!mFrame->HasPerspective() || mTransformGetter || mIsTransformSeparator) {
> > +    if (!mTransformGetter && !mIsTransformSeparator && aOutOrigin) {
> 
> I don't know enough about our implementation of transforms to be able to
> tell whether this condition makes sense.

So I came up with this condition based on the code in the GetTransform function. That is, we need !mTransformGetter and !mIsTransformSeparator in order to enter the line at [1] in GetTransform, where we need avoid the OFFSET_BY_ORIGIN part of the arguments. In effect you can think of this as inlining the codepath in GetTransform that uses OFFSET_BY_ORIGIN, and then omitting OFFSET_BY_ORIGIN if aOutOrigin is provided.

> Also, why don't we need to do anything when the frame has perspective?

Honestly, not sure. I tried to make my change minimal rather than comprehensive because I have a poor understanding of nsDisplayTransform as well. I believe we still have some open bugs/failing reftests on WR handling of perspectives so we can deal with this later.

> ::: layout/painting/nsDisplayList.cpp:8595
> What exactly determines whether this nsDisplayTransform becomes a WR
> reference frame? Is this determined in Gecko code or in WebRender code?

My understanding is that this is determined in WR code - anytime we push a WR stacking context with a nonempty transform it becomes a stacking context.

> If it's determined in WebRender code, do Gecko and WebRender come to the
> same conclusion for an element with transform:none;will-change:transform;?
> How about for an element with a CSS transform animation which is currently
> resting at transform:none?

Good questions. I'll think about this some more and do some more experimenting. I've also forgotten/paged out some of the details I had in mind when I wrote these patches but I don't think I really considered these questions specifically anyway.

[1] https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/layout/painting/nsDisplayList.cpp#8439
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> it becomes a stacking context.

this should say "it becomes a reference frame."
So after thinking about this some more, I realized that we should be able to *always* separate the transform from the offset when creating a WR stacking context from an nsDisplayTransform. In the case where the offset is empty, there's no difference in behaviour.

If the offset is non-empty and the transform is empty, we will change from creating a spurious WR reference frame (because we would previously have been setting a transform consisting of the offset) to just creating a WR stacking context with an offset. This seems more correct and efficient, since WR reference frames are more expensive IIRC.

If the offset is non-empty and the transform is also non-empty, we will split what we were previously providing as the transform into the two separate components (transform and offset) which is what I wanted to fix this bug. So that's fine too.

I kicked off a try push [1] to verify this theory and it looks green. The additional change in this try push is at [2].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4c7a64fc00a9f69a9dda1e6710a27e81655d1a5
[2] https://hg.mozilla.org/try/rev/4050bd68998d3e5e3aa853c437460e1bdaf31f5a
Second patch updated per comment 11. Markus, does the above discussion answer your questions?
Flags: needinfo?(mstange)
Rebased: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf044620744e34706219f40bbc996c4dc75fd4b0

I'm going to go ahead and land this, can deal with any issues in follow-ups.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b14272ea68fd
Expose WR stacking context bounds via StackingContextHelper. r=mstange
https://hg.mozilla.org/integration/autoland/rev/a233b4ffcfc9
Split the transform sent to webrender back into the transform and positioning components. r=mstange
https://hg.mozilla.org/integration/autoland/rev/ba85e9043c8a
Add reftests for position:sticky items inside a transform. r=mstange
Backout by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3332de80d146
Backed out 3 changesets backed out for failing reftest layout/reftests/position-sticky/transformed-2.html on Android on a CLOSED TREE
Urgh. The "failure" is just fuzziness (2,4) on the end of the scrollbars on Android. It's not relevant to the test so I'll just fuzz it.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5402e62aaa71
Expose WR stacking context bounds via StackingContextHelper. r=mstange
https://hg.mozilla.org/integration/autoland/rev/1e675415a7a0
Split the transform sent to webrender back into the transform and positioning components. r=mstange
https://hg.mozilla.org/integration/autoland/rev/4416e585cf77
Add reftests for position:sticky items inside a transform. r=mstange
Why are we even running non-e10s reftests on Windows anymore? Ah well, I just need to skip-if(!asyncPan) the test.
Flags: needinfo?(bugmail)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d42cdc209720a01401657e7a573bdfae1061d4f3 shows that skip-if(!asyncPan) resolved the failure on windows.

However there's also an android perma-fail (which got filed as bug 1430787) from one of the new reftests that I need to fix before I can re-land this. I'll do that work in bug 1430787 since looks like it's a pre-existing bug in the code.
I'm marking the new tests skip-if on android for bug 1430787. Here's a recent try push with that change:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ea093e9d6272658a0f66e0b55ad99dc07143383

The only issue remaining here is that the OS X and Linux test-verify jobs are orange due to leaks. However I haven't been able to reproduce this locally, and the tests running as part of the regular reftest jobs are green. I believe this leakage to be a result of the fact that test-verify runs really quickly and might be done while the safebrowsing code is still in the middle of doing it's thing. I've filed bug 1432246 for this issue.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2254ae8cf040
Expose WR stacking context bounds via StackingContextHelper. r=mstange
https://hg.mozilla.org/integration/autoland/rev/be2548e479a6
Split the transform sent to webrender back into the transform and positioning components. r=mstange
https://hg.mozilla.org/integration/autoland/rev/3dea91961847
Add reftests for position:sticky items inside a transform. r=mstange
Comment on attachment 8937994 [details]
Bug 1426386 - Expose WR stacking context bounds via StackingContextHelper.

https://reviewboard.mozilla.org/r/208724/#review220316


Static analysis found 2 defects in this patch.
 - 2 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/painting/nsDisplayList.cpp:6756
(Diff revision 6)
>                                              mozilla::layers::WebRenderLayerManager* aManager,
>                                              nsDisplayListBuilder* aDisplayListBuilder)
>  {
>    nsTArray<mozilla::wr::WrFilterOp> filters;
> -  StackingContextHelper sc(aSc, aBuilder, filters, nullptr, 0, nullptr, nullptr,
> -                           nullptr, nsCSSRendering::GetGFXBlendMode(mBlendMode));
> +  StackingContextHelper sc(aSc, aBuilder, filters, LayoutDeviceRect(), nullptr,
> +                           0, nullptr, nullptr, nullptr,

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

                           0, nullptr, nullptr, nullptr,
                           ^
                           nullptr

::: layout/painting/nsDisplayList.cpp:9933
(Diff revision 6)
>      }
>    }
>  
>    float opacity = mFrame->StyleEffects()->mOpacity;
> -  StackingContextHelper sc(aSc, aBuilder, wrFilters, nullptr, 0, opacity != 1.0f && mHandleOpacity ? &opacity : nullptr);
> +  StackingContextHelper sc(aSc, aBuilder, wrFilters, LayoutDeviceRect(), nullptr,
> +                           0, opacity != 1.0f && mHandleOpacity ? &opacity : nullptr);

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

                           0, opacity != 1.0f && mHandleOpacity ? &opacity : nullptr);
                           ^
                           nullptr
https://hg.mozilla.org/mozilla-central/rev/2254ae8cf040
https://hg.mozilla.org/mozilla-central/rev/be2548e479a6
https://hg.mozilla.org/mozilla-central/rev/3dea91961847
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Yay! I'll spin out another bug for the clang-tidy warnings.
Dropping obsolete needinfo.
Flags: needinfo?(mstange)
No longer depends on: 1477449
Depends on: 1498962
You need to log in before you can comment on or make changes to this bug.