Closed
Bug 1426386
Opened 7 years ago
Closed 7 years ago
Don't combine the offset of a stacking context into its transform, if it has one
Categories
(Core :: Graphics: WebRender, enhancement, P1)
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: kats, Assigned: kats)
References
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
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
Assignee | ||
Comment 5•7 years ago
|
||
By the way, https://bugzilla.mozilla.org/show_bug.cgi?id=1424686#c6 has some more context on this bug, it might be useful.
Assignee | ||
Updated•7 years ago
|
See Also: → https://github.com/servo/webrender/pull/2221
Updated•7 years ago
|
Blocks: stage-wr-trains
Priority: -- → P1
Comment 6•7 years ago
|
||
mozreview-review |
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 7•7 years ago
|
||
mozreview-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 8•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 9•7 years ago
|
||
(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
Assignee | ||
Comment 10•7 years ago
|
||
(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."
Assignee | ||
Comment 11•7 years ago
|
||
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
Second patch updated per comment 11. Markus, does the above discussion answer your questions?
Flags: needinfo?(mstange)
Assignee | ||
Comment 16•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•7 years ago
|
||
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
Comment 21•7 years ago
|
||
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
Assignee | ||
Comment 22•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 26•7 years ago
|
||
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
Comment 27•7 years ago
|
||
Backed out 3 changesets (bug 1426386) for R2 failing in /layout/reftests/position-sticky/transformed-2.html on Windows
push where the failures started: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4416e585cf770d8d94128d7d2b159502a219971e&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&selectedJob=156651552
failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=156651552&repo=autoland&lineNumber=52740
backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=929bf53b16fb1e01acdcdfcb740ee4ce73eb2fda&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success
Flags: needinfo?(bugmail)
Assignee | ||
Comment 28•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 32•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 36•7 years ago
|
||
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.
Comment 37•7 years ago
|
||
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 38•7 years ago
|
||
mozreview-review |
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
Comment 39•7 years ago
|
||
bugherder |
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: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Assignee | ||
Comment 40•7 years ago
|
||
Yay! I'll spin out another bug for the clang-tidy warnings.
You need to log in
before you can comment on or make changes to this bug.
Description
•