Bug 237590 – Extract transform-origin handling out of RenderStyle::applyTransform()

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Nikolas Zimmermann]

RenderStyle::applyTransform() implements the algorithm given in CSS Transforms Module Level 2 (https://www.w3.org/TR/css-transforms-2/#ctm)
that yields the "current transformation matrix". It is used e.g. in RenderLayer::updateTransform() to compute the layer transformation matrix, that's used for rendering.
    
Make it possible to re-use all steps of the algorithm for SVG, by allowing to override the CSS transform by an external matrix that's used instead (SVG 2D transform).

[Nikolas Zimmermann]

Created attachment 454086 [details]
Patch, v1

[Rob Buis]

Comment on attachment 454086 [details]
Patch, v1

View in context: https://bugs.webkit.org/attachment.cgi?id=454086&action=review

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1536
> +TransformationMatrix& TransformationMatrix::multiplyAffineTransform(const AffineTransform& mat)

We probably should not abbreviate here, so either 'matrix' or 'affineTransform'.

[Simon Fraser (smfr)]

Comment on attachment 454086 [details]
Patch, v1

View in context: https://bugs.webkit.org/attachment.cgi?id=454086&action=review

> Source/WebCore/rendering/style/RenderStyle.cpp:1474
> +    if (transformOperations.size()) {
> +        for (auto& operation : transformOperations.operations())
> +            operation->apply(transform, boundingBox.size());
> +    } else if (externalAffineTransformationSource) {
> +        // SVG may provide additional non-CSS transformation operations.
> +        transform.multiplyAffineTransform(externalAffineTransformationSource());
> +    }

It's weird that this is either-or. From the function signature I would expect that the result of externalAffineTransformationSource would get multiplied with the style's transforms. Especially since this function DOES use the individual transform properties.

[Simon Fraser (smfr)]

Comment on attachment 454086 [details]
Patch, v1

View in context: https://bugs.webkit.org/attachment.cgi?id=454086&action=review

>> Source/WebCore/rendering/style/RenderStyle.cpp:1474
>> +    }
> 
> It's weird that this is either-or. From the function signature I would expect that the result of externalAffineTransformationSource would get multiplied with the style's transforms. Especially since this function DOES use the individual transform properties.

One option would be to add another function that knows how to apply/unapply transform origin, and call it from this function, and to add a third function for the custom SVG case.

[Nikolas Zimmermann]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 454086 [details]
> Patch, v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454086&action=review
> 
> > Source/WebCore/rendering/style/RenderStyle.cpp:1474
> > +    if (transformOperations.size()) {
> > +        for (auto& operation : transformOperations.operations())
> > +            operation->apply(transform, boundingBox.size());
> > +    } else if (externalAffineTransformationSource) {
> > +        // SVG may provide additional non-CSS transformation operations.
> > +        transform.multiplyAffineTransform(externalAffineTransformationSource());
> > +    }
> 
> It's weird that this is either-or. From the function signature I would
> expect that the result of externalAffineTransformationSource would get
> multiplied with the style's transforms. Especially since this function DOES
> use the individual transform properties.

Good catch, the intention was to only look at 'externalAffineTransformSource' if neither the transform property nor the individual transform properties are given, but the "if (transformOperatations.size())" case only catches the transform attribute, not the rest.

[Nikolas Zimmermann]

(In reply to Simon Fraser (smfr) from comment #4)
> > It's weird that this is either-or. From the function signature I would expect that the result of externalAffineTransformationSource would get multiplied with the style's transforms. Especially since this function DOES use the individual transform properties.
> 
> One option would be to add another function that knows how to apply/unapply
> transform origin, and call it from this function, and to add a third
> function for the custom SVG case.

+1 that sounds much nicer.

[Nikolas Zimmermann]

Created attachment 454212 [details]
Patch, v2

[Nikolas Zimmermann]

Created attachment 454217 [details]
Patch, v3

[Nikolas Zimmermann]

(In reply to Rob Buis from comment #2)
> Comment on attachment 454086 [details]
> Patch, v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454086&action=review
> 
> > Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1536
> > +TransformationMatrix& TransformationMatrix::multiplyAffineTransform(const AffineTransform& mat)
> 
> We probably should not abbreviate here, so either 'matrix' or
> 'affineTransform'.

Missed that comment, I can apply s/mat/matrix/ before landing.

[Nikolas Zimmermann]

Created attachment 454348 [details]
Patch, v4

[Nikolas Zimmermann]

(In reply to Nikolas Zimmermann from comment #9)
> (In reply to Rob Buis from comment #2)
> > Comment on attachment 454086 [details]
> > Patch, v1
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=454086&action=review
> > 
> > > Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1536
> > > +TransformationMatrix& TransformationMatrix::multiplyAffineTransform(const AffineTransform& mat)
> > 
> > We probably should not abbreviate here, so either 'matrix' or
> > 'affineTransform'.
> 
> Missed that comment, I can apply s/mat/matrix/ before landing.
Even easier, the changes are not needed here anymore, leaving this out for another LBSE patch.

[Nikolas Zimmermann]

Simon, can you have another look?
This slightly deviates from your proposal -- it might be helpful to look at this patch and also at https://bugs.webkit.org/show_bug.cgi?id=237711, which is the patch that enables SVG/CSS transform support for LBSE (RenderLayerModelObject::applySVGTransform() makes use of the new methods introduced here: applyTransformOrigin/unapplyTransformOrigin/affectedByTransformOrigin/etc.)

The patch on ticket 237711 depends on all of these tickets: 237553 (done), 237554 (r?), 237590 (this one), 237701 (depends on 2357554).

Once these are in, I can proceed to the patches that enable transform-box support for composited elements -- which is trivial after the cleanups are all in.

[Radar WebKit Bug Importer]

<rdar://problem/90248514>

[Simon Fraser (smfr)]

Comment on attachment 454348 [details]
Patch, v4

View in context: https://bugs.webkit.org/attachment.cgi?id=454348&action=review

> Source/WebCore/rendering/style/RenderStyle.cpp:1508
> +    // 8. Translate by the negated computed X, Y and Z values of transform-origin. (see unapplyTransformOrigin)

Weird to have this dangling comment here.

[Nikolas Zimmermann]

(In reply to Simon Fraser (smfr) from comment #14)
> Comment on attachment 454348 [details]
> Patch, v4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454348&action=review
> 
> > Source/WebCore/rendering/style/RenderStyle.cpp:1508
> > +    // 8. Translate by the negated computed X, Y and Z values of transform-origin. (see unapplyTransformOrigin)
> 
> Weird to have this dangling comment here.

I've added (see unapplyTransformOrigin) -- modified to:
+    // 8. Translate by the negated computed X, Y and Z values of transform-origin.
+    // (implemented in unapplyTransformOrigin)

before landing. I hope that's better :-)

[Nikolas Zimmermann]

Committed r291338 (248477@trunk): <https://commits.webkit.org/248477@trunk>