RESOLVED FIXED Bug 211787
Perspective should not be affected by transform-origin
https://bugs.webkit.org/show_bug.cgi?id=211787
Summary Perspective should not be affected by transform-origin
Simon Fraser (smfr)
Reported 2020-05-12 10:45:02 PDT
Created attachment 399143 [details] Test Transform origin affects perspective, and should not.
Attachments
Test (2.51 KB, text/html)
2020-05-12 10:45 PDT, Simon Fraser (smfr)
no flags
Patch, v1 (34.68 KB, patch)
2022-04-04 13:38 PDT, Nikolas Zimmermann
no flags
Patch, v2 (33.21 KB, patch)
2022-04-05 15:00 PDT, Nikolas Zimmermann
no flags
Patch, v3 (32.25 KB, patch)
2022-04-07 16:06 PDT, Nikolas Zimmermann
no flags
Patch, v4 (32.23 KB, patch)
2022-04-08 03:56 PDT, Nikolas Zimmermann
no flags
Patch, v5 (34.56 KB, patch)
2022-04-10 15:57 PDT, Nikolas Zimmermann
ews-feeder: commit-queue-
Patch, v6 (35.42 KB, patch)
2022-04-10 16:04 PDT, Nikolas Zimmermann
no flags
Patch, v7 (35.42 KB, patch)
2022-04-11 05:58 PDT, Nikolas Zimmermann
simon.fraser: review+
Radar WebKit Bug Importer
Comment 1 2020-05-12 10:45:21 PDT
Simon Fraser (smfr)
Comment 2 2020-05-12 21:57:26 PDT
This also affects perspective on scrollers with visible scrollbars.
Nikolas Zimmermann
Comment 3 2022-04-04 13:38:19 PDT
Created attachment 456620 [details] Patch, v1
Nikolas Zimmermann
Comment 4 2022-04-04 13:40:30 PDT
For LBSE I need proper transform-box/origin support for both composited & non-composited element. Therefore I gave this a shot: looks promising, perspective is no longer dependent on the transform-origin/transform-box. This depends on the earlier cleanup patches from bugs 237554, 237701. I can mark this for review, once they were landed.
Nikolas Zimmermann
Comment 5 2022-04-04 15:32:46 PDT
- 'perspective-origin' <length> values are now resolved against the 'transform reference box' (== the reference box according to 'transform-box', e.g. content-box / view-box / border-box / ...). Until now we've always used 'border-box'. - Teach RenderLayerBacking about 'transform-box': composited elements ignored 'transform-box' so far, when computing the anchor points that encode 'transform-origin' information as relative values. - Update the GraphicsLayer geometry upon 'transform-box' changes (fixes 'transform-box' animations, covered by a new WPT test). - The computed perspective transformation, that is applied to children of an element that defines a CSS 'perspective', is now invariant under 'transform-origin' / 'transform-box' changes. This now matches Blink/Gecko behaviour. Blink/Gecko currently do not support e.g. 'content-box' as value for 'transform-box' -- other than that our compositing + perspective/scrolling/transform-box testcases now match between Safari/Firefox/Chromium, however only WebKit provides proper 'content-box' support. This aligns our support for composited / non-composited elements (assigning 'will-change: transform' to elements that specify a non-default 'transform-box' no longer leads to surprises as in 'why does the element jump from its nominal position when compositing is active') ... I think we should re-sync /css/css-transforms WPT tests first, to assess the current WPT CSS transforms state and how this patch affects WPT results -- I have had issues with a fresh WPT sync (hundreds of failures without this patch applied, so that needs to be checked first, I did not investigate yet).
Nikolas Zimmermann
Comment 6 2022-04-05 15:00:51 PDT
Created attachment 456755 [details] Patch, v2
Nikolas Zimmermann
Comment 7 2022-04-07 16:06:39 PDT
Created attachment 456980 [details] Patch, v3
Nikolas Zimmermann
Comment 8 2022-04-07 16:07:10 PDT
Now that both 237554 and 237701 are in, it's tempting to see what EWS thinks about the patch :-) Let's try.
Nikolas Zimmermann
Comment 9 2022-04-08 03:56:47 PDT
Created attachment 457041 [details] Patch, v4
Nikolas Zimmermann
Comment 10 2022-04-10 15:57:37 PDT
Created attachment 457214 [details] Patch, v5
Nikolas Zimmermann
Comment 11 2022-04-10 16:04:53 PDT
Created attachment 457215 [details] Patch, v6
Nikolas Zimmermann
Comment 12 2022-04-11 05:58:26 PDT
Created attachment 457251 [details] Patch, v7
Nikolas Zimmermann
Comment 13 2022-04-20 00:55:02 PDT
Simon, did you have any chance to have a look at it?
Nikolas Zimmermann
Comment 14 2022-05-06 01:18:44 PDT
Yikes, it's pending review since almost a month.... all experts busy? :-)
Simon Fraser (smfr)
Comment 15 2022-05-16 13:29:10 PDT
Comment on attachment 457251 [details] Patch, v7 View in context: https://bugs.webkit.org/attachment.cgi?id=457251&action=review > Source/WebCore/rendering/RenderLayer.cpp:1815 > + // To circumvent this, explicitely remove the transform-origin dependency in the perspective matrix. > + auto transformOrigin = transformOriginForPainting(); If this function isn't returning the "perspectiveTransform", but rather something that makes assumptions about platform-specific rendering, perhaps it needs to be renamed? > Source/WebCore/rendering/RenderLayer.cpp:1824 > +FloatPoint3D RenderLayer::transformOriginForPainting() const Unclear how this relates to the other kind of "transform origin". Is it just about snapping? > Source/WebCore/rendering/RenderLayer.h:743 > // Note that this transform has the perspective-origin baked in. Is this comment still accurate?
Nikolas Zimmermann
Comment 16 2022-05-18 05:10:01 PDT
(In reply to Simon Fraser (smfr) from comment #15) > Comment on attachment 457251 [details] > Patch, v7 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=457251&action=review > > > Source/WebCore/rendering/RenderLayer.cpp:1815 > > + // To circumvent this, explicitely remove the transform-origin dependency in the perspective matrix. > > + auto transformOrigin = transformOriginForPainting(); > > If this function isn't returning the "perspectiveTransform", but rather > something that makes assumptions about platform-specific rendering, perhaps > it needs to be renamed? It's still the "vanilla" perspective transform matrix, plus an additional component that embeds it into the parent coordinate system, and that is indeed platform-dependent, as you pointed out -- however consistent for all our platforms at present. How about re-using the GraphicsLayer terminology and call it "childrenTransform()"? I'll try that. > > > Source/WebCore/rendering/RenderLayer.cpp:1824 > > +FloatPoint3D RenderLayer::transformOriginForPainting() const > > Unclear how this relates to the other kind of "transform origin". Is it just > about snapping? Yeah, it snaps, if necessary. How about "transformOriginPixelSnappedIfNeeded"? I'll go for that for now. > > > Source/WebCore/rendering/RenderLayer.h:743 > > // Note that this transform has the perspective-origin baked in. > > Is this comment still accurate? Yeah, we removed the transform-origin dependency, but perspective-origin is still included (and that's ok). I will upload the new iteration of this patch as GitHub PR.
Nikolas Zimmermann
Comment 17 2022-05-18 05:13:42 PDT
EWS
Comment 18 2022-05-21 16:17:25 PDT
Committed r294615 (250841@main): <https://commits.webkit.org/250841@main> Reviewed commits have been landed. Closing PR #720 and removing active labels.
Note You need to log in before you can comment on or make changes to this bug.