SVG <animateMotion> does not reset the element to its first animation frame if its fill is "remove"

[Said Abou-Hallawa]

Created attachment 378249 [details]
test case

Open the attached test case.

Result: The green rectangle should animate for 2 seconds then it stays in its last animated position.
Expected: After the green rectangle finishes animation, it should move back to its original position.


This is an SVG test case. It has a <rect> element which has an <animateMotion> element as a child. The <animateMotion> has an attribute fill="remove". The expected behavior is resetting the rectangle back to its original position once the animation finishes.

[Said Abou-Hallawa]

Related WPT tests:

https://wpt.fyi/results/svg/animations/animateMotion-still.html?label=master&label=experimental&aligned

[Said Abou-Hallawa]

Created attachment 378427 [details]
animateMotion + animate

This test case shows the difference between using the <animateMotion> and <animate> elements.

[Nikolas Zimmermann]

I am working on this.

[Said Abou-Hallawa]

This WPT is also affected by this bug:

https://wpt.fyi/results/svg/animations/animateMotion-fill-remove.html?label=master&label=experimental&aligned

[Nikolas Zimmermann]

I have a testcase for svg/animations that samples the animation for both <animate> / <animateMotion>: before-the-animation / middle-of-animation / before-end-of-animation / past-end-of-animation, and is currently not passing.

I found the origin of the bug and will prepare a patch now.

[Nikolas Zimmermann]

Created attachment 378743 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 378743 [details]
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"/>.

[Said Abou-Hallawa]

Created attachment 378847 [details]
animateMotion-remove-freeze-use

[Nikolas Zimmermann]

(In reply to Said Abou-Hallawa from comment #7)
> 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().

I thought about it and there are even more compelling reasons why it should be paired, and why calling setNeedsTransformUpdate() in calculateAnimatedValue() (what we are doing at the moment) is flawed:

To assure that the implementation is symmetric between <animate> and <animateMotion>
in terms of updating the underlying value, updating the renderers "dirty state" etc, I decided
to compare the current call paths that lead to visual updates:

Let's examine animating "x" of a <rect> using <animate>:

1. An animation frame update is triggered by the timer in SMILTimeContainer.
2. The following call chain is triggered:

-> SMILTimeContainer::timerFired
   -> SMILTimeContainer::updateAnimations
      |--> SVGSMILElement::progress
      |  |--> SVGAnimationElement::updateAnimation
      |     |--> SVGAnimateElementBase::calculateAnimatedValue
      |        |--> SVGAnimatedLengthAnimator::animate
      |             Modifies m_x of SVGRectElement, does NOT modify the target elements renderer.
      |
      |--> SVGAnimateElementBase::applyResultsToTarget
         |--> SVGAnimatedPropertyAnimator<SVGAnimatedValueProperty<SVGLength>, SVGAnimationLengthFunction>::apply
            |--> SVGAttributeAnimator::applyAnimatedPropertyChange
               |--> SVGAttributeAnimator::applyAnimatedPropertyChange
                  |--> SVGRectElement::svgAttributeChanged
                     | Updates of the attribute changes are propagated to the instances (<use>), by utilizing InstanceInvalidationGuard.
                     |
                     |--> SVGElement::invalidateSVGPresentationAttributeStyle
                        |--> Element::invalidateStyle
                             Since xAttr is part of the presentation attribute style, setNeedsLayout() is called
                             on the RenderSVGRect, and thus the <rect> is relayouted during the next layout phase.
                             RenderSVGRect::updateShapeFromElement() rebuilds the geometry and/or creates a path if necessary.

Now let's examine moving a rect along x direction using <animateMotion>:

1. An animation frame update is triggered by the timer in SMILTimeContainer.
2. The following call chain is triggered:

-> SMILTimeContainer::timerFired
   -> SMILTimeContainer::updateAnimations
      |--> SVGSMILElement::progress
      |  |--> SVGAnimationElement::updateAnimation
      |     |--> SVGAnimateMotionElement::calculateAnimatedValue
      |        |  Prepares a new supplementalTransform - resets the existing one.
      |        |
      |        |--> RenderObject::setNeedsTransformUpdate
      |        |    Explicitely sets the target elements renderer dirty: needs transform update.
      |        |
      |        |--> SVGAnimateMotionElement::buildTransformForProgress
      |        |    Calculates the new supplemental transform.
      |
      |--> SVGAnimateMotionElement::applyResultsToTarget
         |--> RenderSVGResource::markForLayoutAndParentResourceInvalidation
              Explicitely marks the target elements renderer for layout.
              Propagates the supplementalTransform changes to all instances of the target element (<use>) and marks all instance renders for layout. 


Its obvious that <animateMotion> is different than <animate>: The updates of the supplementalTransform() and the modification of
the render tree are done at the same time. These phases should be decoupled, as you implicitly suggested with your comment.


> > 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:
I wanted to document the different behavior of <animate> and <animateMotion>, that's why I decided to test both in the same testcase. Not ok?

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

I agree, these two testcases will be helpful.
I will adapt my patch and come back to you :-)

[Nikolas Zimmermann]

Created attachment 378951 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 378951 [details]
Patch

This looks great. Thanks!

[Nikolas Zimmermann]

Committed r249974: <https://trac.webkit.org/changeset/249974>

[Radar WebKit Bug Importer]

<rdar://problem/55448916>