Bug 53767 – SVG <use> element is not repositioned when moved to x=0 y=0 through script

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Alexis Deveria]

SVG <use> elements will appear with x="0" y="0" statically and can moved using any other values in JS, but exactly x=0 and y=0 results in no visual difference. Same problem when removing both x and y attributes.

[Alexis Deveria]

Created attachment 81217 [details]
Example of the problem, the green rect should cover the red one after one second.

[Rob Buis]

Created attachment 101798 [details]
Patch

[Dirk Schulze]

Comment on attachment 101798 [details]
Patch

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

> Source/WebCore/rendering/svg/SVGShadowTreeElements.cpp:53
> +    m_yOffset = y;
> +    if (renderer())
> +        static_cast<RenderSVGTransformableContainer*>(renderer())->setNeedsTransformUpdate();

Si it is not only 0,0 affected but every other position as well? Because according your patch I don't see a reason why just 0,0 should be affected.

[Nikolas Zimmermann]

Comment on attachment 101798 [details]
Patch

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

>> Source/WebCore/rendering/svg/SVGShadowTreeElements.cpp:53
>> +        static_cast<RenderSVGTransformableContainer*>(renderer())->setNeedsTransformUpdate();
> 
> Si it is not only 0,0 affected but every other position as well? Because according your patch I don't see a reason why just 0,0 should be affected.

The more interessting question is why would you need a "transform update" if transform did not change?

Note that RenderSVGShadowTreeRootContainer inherits from RenderSVGTransformableContainer, and
bool RenderSVGTransformableContainer::calculateLocalTransform()

contains special code to apply the container offset to the local transform. This should work even _without_ touching setNeedsTransformUpdate.
If it doesn't there's another bug that you're hiding by this workaround.

r- for this reason.

[Nikolas Zimmermann]

Good point Dirk - why is only x/y=0 affected.
Here's the answer:

bool RenderSVGTransformableContainer::calculateLocalTransform()
{
    SVGStyledTransformableElement* element = static_cast<SVGStyledTransformableElement*>(node());
......

    FloatSize translation = static_cast<SVGShadowTreeContainerElement*>(element)->containerTranslation();
    if (!translation.width() && !translation.height())
        return needsUpdate;

if translation width/height (corresponds to x/y) is 0 we'll ignore updating the transform.
We need to store the previous translation, and compare it with the current translation, if they are equal just "return needsUpdate" otherwhise go on....

That explains why only x=0&&y=0 failed here, and avoids the setNeedsTransformUpdate hack from your current patch...

[Rob Buis]

Hi guys,

(In reply to comment #5)
> Good point Dirk - why is only x/y=0 affected.
> Here's the answer:
> 
> bool RenderSVGTransformableContainer::calculateLocalTransform()
> {
>     SVGStyledTransformableElement* element = static_cast<SVGStyledTransformableElement*>(node());
> ......
> 
>     FloatSize translation = static_cast<SVGShadowTreeContainerElement*>(element)->containerTranslation();
>     if (!translation.width() && !translation.height())
>         return needsUpdate;
> 
> if translation width/height (corresponds to x/y) is 0 we'll ignore updating the transform.
> We need to store the previous translation, and compare it with the current translation, if they are equal just "return needsUpdate" otherwhise go on....
> 
> That explains why only x=0&&y=0 failed here, and avoids the setNeedsTransformUpdate hack from your current patch...

Was just about to hint at the same code (just woken up) :)
Will have a look at it again.
Cheers,

Rob.

[Rob Buis]

Created attachment 101814 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 101814 [details]
Patch

Excellent patch, r=me!

[Rob Buis]

Landed in r91644.