[webkit-reviews] review granted: [Bug 201565] SVG <animateMotion> does not reset the element to its first animation frame if its fill is "remove" : [Attachment 378743] Patch

[bugzilla-daemon at webkit.org]

Said Abou-Hallawa <sabouhallawa at apple.com> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 201565: SVG <animateMotion> does not reset the element to its first
animation frame if its fill is "remove"
https://bugs.webkit.org/show_bug.cgi?id=201565

Attachment 378743: Patch

https://bugs.webkit.org/attachment.cgi?id=378743&action=review




--- Comment #7 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 378743
  --> https://bugs.webkit.org/attachment.cgi?id=378743
Patch

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

> Source/WebCore/svg/SVGAnimateMotionElement.cpp:160
> +    if (RenderObject* targetRenderer = targetElement->renderer())
> +	   targetRenderer->setNeedsTransformUpdate();

I think setNeedsTransformUpdate() should be moved to applyResultsToTarget()
before calling markForLayoutAndParentResourceInvalidation():

    if (RenderElement* renderer = targetElement->renderer()) {
	renderer->setNeedsTransformUpdate();
       
RenderSVGResource::markForLayoutAndParentResourceInvalidation(*renderer);
    }

I think it makes more sense when setNeedsTransformUpdate() is paired with
markForLayoutAndParentResourceInvalidation(). And this is what we do for the
other <use> instances in applyResultsToTarget().

> LayoutTests/svg/animations/resources/fill-remove-support.svg:10
> +    <rect id="animateRect" x="10" y="120" width="100" height="100"
fill="green">
> +	   <animate id="animate" attributeType="XML" attributeName="x"
from="20" to="210" begin="0.5s" dur="2s" fill="remove" />
> +    </rect>

Why do we need to test the <animate> element with this patch? The code change
does not affect it. I think we should the following cases instead:

1. An SVG element with <animateMotion fill="freeze">
2. A <use> element which references the animated SVG element, e.g. <use
href="animateMotionRect"/>.