Skip to content
Permalink
Browse files
Recompute transforms on SVG containers if bounds have changed during … 
…layout

https://bugs.webkit.org/show_bug.cgi?id=245918
<rdar://100647287>

Reviewed by Nikolas Zimmermann.

The transform-origin depends on the size of the shape, but <g> (LegacyRenderSVGContainer) only gets the final bounds after laying out the children and updating the cached boundaries.
Make sure we recompute the transforms again after calling updateCachedBoundaries(), but only recompute when transformReferenceBoxRect() was changed.

Test: css/css-transforms/transform-box/fill-box-002.html

* LayoutTests/TestExpectations:
* Source/WebCore/rendering/svg/LegacyRenderSVGContainer.cpp:
(WebCore::LegacyRenderSVGContainer::layout):
* Source/WebCore/rendering/svg/LegacyRenderSVGTransformableContainer.cpp:
(WebCore::LegacyRenderSVGTransformableContainer::calculateLocalTransform):
* Source/WebCore/rendering/svg/LegacyRenderSVGTransformableContainer.h:

Canonical link: https://commits.webkit.org/255060@main
  • Loading branch information
@nt1m
nt1m committed Oct 1, 2022
1 parent 6f687d4 commit 3c6b4ac4654c5fd4fcb312b5b6dc616b8dcb6f73
Show file tree
Hide file tree
Showing 4 changed files with 11 additions and 2 deletions.
1 LayoutTests/TestExpectations
View file
@@ -4198,7 +4198,6 @@ webkit.org/b/230080 imported/w3c/web-platform-tests/css/css-transforms/rotateY-1
webkit.org/b/230080 imported/w3c/web-platform-tests/css/css-transforms/scale-transform-overlap.html [ ImageOnlyFailure ]
webkit.org/b/230080 imported/w3c/web-platform-tests/css/css-transforms/transform-background-007.html [ ImageOnlyFailure ]
webkit.org/b/230080 imported/w3c/web-platform-tests/css/css-transforms/transform-background-008.html [ ImageOnlyFailure ]
webkit.org/b/230080 imported/w3c/web-platform-tests/css/css-transforms/transform-box/fill-box-002.html [ ImageOnlyFailure ]
webkit.org/b/230080 imported/w3c/web-platform-tests/css/css-transforms/transform-root-bg-001.html [ ImageOnlyFailure ]
webkit.org/b/230080 imported/w3c/web-platform-tests/css/css-transforms/transform-root-bg-002.html [ ImageOnlyFailure ]
webkit.org/b/230080 imported/w3c/web-platform-tests/css/css-transforms/transform-root-bg-003.html [ ImageOnlyFailure ]
5 Source/WebCore/rendering/svg/LegacyRenderSVGContainer.cpp
View file
@@ -79,7 +79,10 @@ void LegacyRenderSVGContainer::layout()
if (m_needsBoundariesUpdate || updatedTransform) {
updateCachedBoundaries();
m_needsBoundariesUpdate = false;


// New bounds can affect transforms, so recompute them here if needed.
calculateLocalTransform();

// If our bounds changed, notify the parents.
LegacyRenderSVGModelObject::setNeedsBoundariesUpdate();
}
6 Source/WebCore/rendering/svg/LegacyRenderSVGTransformableContainer.cpp
View file
@@ -63,6 +63,12 @@ bool LegacyRenderSVGTransformableContainer::calculateLocalTransform()
m_lastTranslation = translation;
}

auto referenceBoxRect = transformReferenceBoxRect();
if (referenceBoxRect != m_lastTransformReferenceBoxRect) {
m_lastTransformReferenceBoxRect = referenceBoxRect;
m_needsTransformUpdate = true;
}

This comment has been minimized.

Sign in to view

Copy link
@shallawa

shallawa Oct 3, 2022

Contributor

It is strange that we run this code even if m_needsTransformUpdate is true. Also I think we should run it only if the layout depends on the referenceBoxRect(). I think this happens only if transform-origin attribute has a percentage lengths.

This comment has been minimized.

Sign in to view

Copy link
@nt1m

nt1m Oct 3, 2022

Author Member

It is strange that we run this code even if m_needsTransformUpdate is true.

We do want to update m_lastTransformReferenceBoxRect regardless.

Also I think we should run it only if the layout depends on the referenceBoxRect(). I think this happens only if transform-origin attribute has a percentage lengths.

This is actually the majority of cases, since the default value of transform-origin is center (50% 50%), so adding this optimization won't help too much.

This comment has been minimized.

Sign in to view

Copy link
@shallawa

shallawa Oct 3, 2022 •
edited

Contributor

I meant something like this may have been clearer:

if (!m_needsTransformUpdate && isReferenceBoxRectNeeded())
    m_needsTransformUpdate = true;

SVGGraphicsElement::animatedLocalTransform() which is called below calls transformReferenceBoxRect() also.

This comment has been minimized.

Sign in to view

Copy link
@nt1m

nt1m Oct 3, 2022

Author Member

I see, I do think storing m_lastTransformReferenceBoxRect is useful regardless, because in many cases, the bounds won't actually change during layout, so this optimization is valuable.

    m_lastTransformReferenceBoxRect = transformReferenceBoxRect();
    if (!m_needsTransformUpdate && isReferenceBoxRectNeeded())
        m_needsTransformUpdate = true;

where isReferenceBoxRectNeeded() would check both for the percentage-based transform-origin/transform and the m_lastTransformReferenceBoxRect having been changed.

I filed https://bugs.webkit.org/show_bug.cgi?id=245978 for this.

This comment has been minimized.

Sign in to view

Copy link
@nikolaszimmermann

nikolaszimmermann Oct 5, 2022 •
edited

Contributor

It is strange that we run this code even if m_needsTransformUpdate is true. Also I think we should run it only if the layout depends on the referenceBoxRect(). I think this happens only if transform-origin attribute has a percentage lengths.

Good catch - for m_needsTransformUpdate we can skip the logic, if the "transform reference box" is recomputed and cached anyhow after computing the transform later on.

Regarding the second statement: transform-origin also depends on the location of the "transform reference box" (our transformReferenceBoxRect()), even if fixed units are used for the transform-origin-x|y|z components of the shorthand. For percentage sized x/y/z components, there's an additional dependency on the size of the "transform reference box".

FloatPoint3D RenderStyle::computeTransformOrigin(const FloatRect& boundingBox) const
{
    FloatPoint3D originTranslate;
    originTranslate.setXY(boundingBox.location() + floatPointForLengthPoint(transformOriginXY(), boundingBox.size()));
    originTranslate.setZ(transformOriginZ());
    return originTranslate;
}

However one could omit the whole "transform reference box" bookkeeping and potential recomputations, if the transform itself is not affected by the transform-origin (affectedByTransformOrigin() -- if false, transform is invariant under transform-origin change, e.g. for 'transform: translate(....);'). For LBSE additional considerations are needed as SVG transforms also play a role, instead of only CSS transforms (where 'affectedByTransformOrigin' from RenderStyle is sufficient to determine if the transform is affected by transform-origin or not)

This comment has been minimized.

Sign in to view

Copy link
@nikolaszimmermann

nikolaszimmermann Oct 5, 2022 •
edited

Contributor

It is strange that we run this code even if m_needsTransformUpdate is true.

We do want to update m_lastTransformReferenceBoxRect regardless.

Also I think we should run it only if the layout depends on the referenceBoxRect(). I think this happens only if transform-origin attribute has a percentage lengths.

This is actually the majority of cases, since the default value of transform-origin is center (50% 50%), so adding this optimization won't help too much.

That's not the default for SVG, which is transform-origin: 0 0. and transform-box is view-box, which means that for SVG by default the top-left corner of the viewport is the transform reference point, whereas for a CSS box the default is indeed center (50% 50%) and thus resolving is performed against the local coordinate system of the box -- completely different for SVG (by default).

m_didTransformToRootUpdate = m_needsTransformUpdate || SVGRenderSupport::transformToRootChanged(parent());
if (!m_needsTransformUpdate)
return false;
1 Source/WebCore/rendering/svg/LegacyRenderSVGTransformableContainer.h
View file
@@ -47,6 +47,7 @@ class LegacyRenderSVGTransformableContainer final : public LegacyRenderSVGContai
bool m_didTransformToRootUpdate : 1;
AffineTransform m_localTransform;
FloatSize m_lastTranslation;
FloatRect m_lastTransformReferenceBoxRect;
};

} // namespace WebCore

0 comments on commit 3c6b4ac

Please sign in to comment.