Bug 201565

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

Created attachment 378427 [details] animateMotion + animate This test case shows the difference between using the <animateMotion> and <animate> elements.

I am working on this.

This WPT is also affected by this bug: https://wpt.fyi/results/svg/animations/animateMotion-fill-remove.html?label=master&label=experimental&aligned

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.

Created attachment 378743 [details] Patch

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"/>.

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

(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 :-)

Created attachment 378951 [details] Patch

Comment on attachment 378951 [details] Patch This looks great. Thanks!

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

<rdar://problem/55448916>