Bug 239717 – [LBSE] Fix origin of transformations in SVG

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Ticket 237711 laid the groundwork for SVG transforms in LBSE, however there are still issues:

- SVG transforms specified alone on SVG elements, w/o any additional style/perspective related property such as transform-box / transform-origin / ...
  does not trigger layer creation (StyleAdjuster needs to consult SVG transforms as well, as special case, to determine if the animatedLocalTransform() is non-identity). This was missing.

- Switching on/off transforms for a SVG element, was not tracking updates to both 'HasSVGTransform' / 'HasTransformRelatedProperty' flags correctly, fix that.

And finally an important change with respect to LBSE downstream. In previous approach, the role of 'offsetFromAncestor' was changed for SVG, in e.g. paintLayerByApplyingTransform(). It did not measure the offsetFromAncestor in the same coordinate space as CSS. CSS layout does not consider transformations, everything is laid out as it's untransformed, and transforms are merely a painting effect. This is true for SVG on shape-level, but not for containers, since SVG containers are the union of the bounds of the children, mapped through the child transformations.

When SVGContainerLayout positions container children relative to itself, to "mimic" nested absolute positioned CSS boxes, it subtracts the container objectBoundingBox() location from the locations of the children. That is easy to implement, one doesn't need new geometry but can rely on objectBoundingBox(). However this makes SVG container 'internal boundaries' dependent on the transformations of the children. Therefore one needs to relayout the whole tree upwards, if the transformation changes in a non-accelerated way (SVGTransformList manipulation, SVG DOM, SMIL <animateTransform>, ...). Furthermore one needs to adapt RenderLayer to change the transformation style from "M * T" to "T * M", where T is the translation due to offsetFromAncestor() and M the user-defined transformation matrix.

This works fine, but there is a less-intrusive approach:
Compute a new geometry 'objectBoundingBoxWithoutTransformations' that is the same as objectBoundingBox(), but does not map contains children through their transformations. Change SVGContainerLayout to subtract that location instead of objectBoundingBox() location. This way the boundaries stay independent of the transformation. Hit-testing / painting via layers, still does respect the transformations, if present. This slightly changed approach, is way less confusing in the end, since the "M * T" logic (first translate to offsetFromAncestor, then apply transformation matrix, can stay as-is).

Patch for this approach will follow, that fixes most transform tests in LBSE, that are testable at present.

[Nikolas Zimmermann]

Created attachment 458267 [details]
Patch, v1

[Rob Buis]

Comment on attachment 458267 [details]
Patch, v1

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

> Source/WebCore/rendering/RenderLayer.cpp:3549
> +#endif

I think a lambda is nicer here.

> Source/WebCore/style/StyleAdjuster.cpp:374
> +#endif

I think a lambda is nicer here.

[Rob Buis]

Comment on attachment 458267 [details]
Patch, v1

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

> Source/WebCore/rendering/RenderElement.cpp:2437
> +        if (isSVGLayerAwareRenderer() && !isSVGRoot() && document().settings().layerBasedSVGEngineEnabled())

It seems a bit more logical to do the isSVGRoot check last.

[Nikolas Zimmermann]

Created attachment 458305 [details]
Patch, v2

[Nikolas Zimmermann]

(In reply to Rob Buis from comment #2)
> > Source/WebCore/rendering/RenderLayer.cpp:3549
> > +#endif
> 
> I think a lambda is nicer here.
Good idea, fixed.

> > Source/WebCore/style/StyleAdjuster.cpp:374
> > +#endif
> 
> I think a lambda is nicer here.
Fixed as well.

(In reply to Rob Buis from comment #3)
> > Source/WebCore/rendering/RenderElement.cpp:2437
> > +        if (isSVGLayerAwareRenderer() && !isSVGRoot() && document().settings().layerBasedSVGEngineEnabled())
> 
> It seems a bit more logical to do the isSVGRoot check last.

Hmm, I disagree. isSVGLayerAwareRenderer() contains an explicit '||.isSVGRoot()' condition, so it returns true for RenderSVGRoot. I really meant to check here: "all layer aware renderers, except RenderSVGRoot" (--> "isSVGLayerAwareRenderer() && !isSVGRoot()"). Note, that if 'isSVGRoot()' evaluates to true, that means LBSE must be enabled, otherwise we'd see a LegacyRenderSVGRoot, not a RenderSVGRoot.

You could argue about testing: "if (document().settings().layerBasedSVGEngineEnabled() && isSVGLayerAwareRenderer() && !isSVGRoot())" though ;-) I left it as-is, if you're ok with it.

[Nikolas Zimmermann]

Created attachment 458310 [details]
Patch, v3

[Nikolas Zimmermann]

Created attachment 458314 [details]
Patch, v4

[Rob Buis]

Comment on attachment 458314 [details]
Patch, v4

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

> Source/WebCore/rendering/RenderObject.h:377
> +    // Their laid out in such a way that transformations do NOT affect layout, as in HTML/CSS world, but take affect during

Their -> They are

[Nikolas Zimmermann]

Created attachment 458434 [details]
Patch for landing

[EWS]

Committed r293504 (250035@main): <https://commits.webkit.org/250035@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 458434 [details].

[Radar WebKit Bug Importer]

<rdar://problem/92390068>