SVG <animateMotion> does not reset the element to its first animation frame if its...
authorzimmermann@webkit.org <zimmermann@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Sep 2019 19:23:56 +0000 (19:23 +0000)
committerzimmermann@webkit.org <zimmermann@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Sep 2019 19:23:56 +0000 (19:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=201565

Reviewed by Said Abou-Hallawa.

Source/WebCore:

Fix fill="remove" support for <animateMotion>: properly trigger visual updates.
Motion animations are implemented by provding an additional transformation on
SVGGraphicsElements: the supplementalTransform().

When an <animateMotion> element specifies fill="remove" the supplemental transform is
correctly reset, when the animation is finished. However, nobody is notified about the
change of transformation matrix. Fix by marking the target elements renderer as
setNeedsTransformUpdate() and call markForLayoutAndParentResourceInvalidation().

<animate> elements are not affected by the bug:
SVGAnimateElementBase::clearAnimatedType()
 -> invokes SVGAttributeAnimator::stop()
    -> invokes applyAnimatedPropertyChange() [via SVGAnimatedPropertyAnimator]
    -> invokes SVGXXXElement::svgAttributeChanged().

When animating e.g. the 'transform' attribute SVGGraphicsElement::svgAttributeChanged()
will use exactly the same mechanism to trigger visul updates: first call
setNeedsTransformUpdate() on the associated renderer, followd by a call to
markForLayoutAndParentResourceInvalidation().

--> Both code paths are now consistent.
Also fixes http://web-platform-tests.live/svg/animations/animateMotion-fill-remove.html.

Tests: svg/animations/fill-remove-support.html
       svg/animations/animateMotion-remove-freeze-use.svg

* svg/SVGAnimateMotionElement.cpp:
(WebCore::SVGAnimateMotionElement::clearAnimatedType): Call applyResultsToTarget().
(WebCore::SVGAnimateMotionElement::calculateAnimatedValue): Stop calling
setNeedsTransformUpdate() on the target elements renderer, since applyResultsToTarget()
is responsible for modifications of the renderer state.
(WebCore::SVGAnimateMotionElement::applyResultsToTarget): Always mark the renderer
as setNeedsTransformUpdate() when applying the results, before the call to
RenderSVGResource::markForLayoutAndParentResourceInvalidation(). This is more consistent
with respect to the code path that updates the SVGElements instances (<use> support).

LayoutTests:

Add a layout test covering fill="remove" support <animate> and <animateMotion>.
Add another layout test covering both fill="remove" and fill="freeze" for
<animateMotion> on SVG elements and their instances (<use>).

* svg/animations/animateMotion-remove-freeze-use-expected.svg: Added.
* svg/animations/animateMotion-remove-freeze-use.svg: Added.
* svg/animations/fill-remove-support-expected.txt: Added.
* svg/animations/fill-remove-support.html: Added.
* svg/animations/resources/fill-remove-support.svg: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@249974 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/svg/animations/animateMotion-remove-freeze-use-expected.svg [new file with mode: 0644]
LayoutTests/svg/animations/animateMotion-remove-freeze-use.svg [new file with mode: 0644]
LayoutTests/svg/animations/fill-remove-support-expected.txt [new file with mode: 0644]
LayoutTests/svg/animations/fill-remove-support.html [new file with mode: 0644]
LayoutTests/svg/animations/resources/fill-remove-support.svg [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/svg/SVGAnimateMotionElement.cpp

index 44720f7..975a48a 100644 (file)
@@ -1,3 +1,20 @@
+2019-09-17  Nikolas Zimmermann  <zimmermann@kde.org>
+
+        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
+
+        Reviewed by Said Abou-Hallawa.
+
+        Add a layout test covering fill="remove" support <animate> and <animateMotion>.
+        Add another layout test covering both fill="remove" and fill="freeze" for
+        <animateMotion> on SVG elements and their instances (<use>).
+
+        * svg/animations/animateMotion-remove-freeze-use-expected.svg: Added.
+        * svg/animations/animateMotion-remove-freeze-use.svg: Added.
+        * svg/animations/fill-remove-support-expected.txt: Added.
+        * svg/animations/fill-remove-support.html: Added.
+        * svg/animations/resources/fill-remove-support.svg: Added.
+
 2019-09-17  Russell Epstein  <repstein@apple.com>
 
         REGRESSION (macOS): Many webgpu/whlsl* tests are flaky failures.
diff --git a/LayoutTests/svg/animations/animateMotion-remove-freeze-use-expected.svg b/LayoutTests/svg/animations/animateMotion-remove-freeze-use-expected.svg
new file mode 100644 (file)
index 0000000..4498d6c
--- /dev/null
@@ -0,0 +1,6 @@
+<svg xmlns="http://www.w3.org/2000/svg">
+    <rect x="10" y="10" width="100" height="100" fill="green"/>
+    <rect x="210" y="120" width="100" height="100" fill="green"/>
+    <rect x="10" y="230" width="100" height="100" fill="green"/>
+    <rect x="210" y="340" width="100" height="100" fill="green"/>
+</svg>
diff --git a/LayoutTests/svg/animations/animateMotion-remove-freeze-use.svg b/LayoutTests/svg/animations/animateMotion-remove-freeze-use.svg
new file mode 100644 (file)
index 0000000..8fdea00
--- /dev/null
@@ -0,0 +1,25 @@
+<svg xmlns="http://www.w3.org/2000/svg" onload="loaded()">
+    <path id="path" d="M10,0 L200,0" fill="none"/>
+    <rect id="rect-remove" x="10" y="10" width="100" height="100" fill="green">
+        <animateMotion begin="0.5s" dur="2s" fill="remove">
+            <mpath href="#path"/>
+        </animateMotion>
+    </rect>
+    <rect id="rect-freeze" x="10" y="120" width="100" height="100" fill="green">
+        <animateMotion begin="0.5s" dur="2s" fill="freeze">
+            <mpath href="#path"/>
+        </animateMotion>
+    </rect>
+    <use href="#rect-remove" y="220"/>
+    <use href="#rect-freeze" y="220"/>
+<script>
+if (window.testRunner)
+    testRunner.waitUntilDone();
+
+function loaded() {
+    document.documentElement.setCurrentTime(5);
+    if (window.testRunner)
+        testRunner.notifyDone();
+}
+</script>
+</svg>
diff --git a/LayoutTests/svg/animations/fill-remove-support-expected.txt b/LayoutTests/svg/animations/fill-remove-support-expected.txt
new file mode 100644 (file)
index 0000000..2d6f452
--- /dev/null
@@ -0,0 +1,60 @@
+SVG 1.1 dynamic animation tests
+
+
+This tests fill=remove for animate and animateMotion
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+
+Check state before/after animation
+PASS animateMotionRect.getBoundingClientRect().x is 10
+PASS animateMotionRect.getBBox().x is 10
+PASS animateRect.getBBox().x is 10
+PASS animateRect.getBoundingClientRect().x is 10
+PASS animateRect.x.animVal.value is 10
+PASS animateRect.x.baseVal.value is 10
+
+Check state right after the beginning of the animation
+PASS animateMotionRect.getBoundingClientRect().x is 20
+PASS animateMotionRect.getBBox().x is 10
+PASS animateRect.getBBox().x is 20
+PASS animateRect.getBoundingClientRect().x is 20
+PASS animateRect.x.animVal.value is 20
+PASS animateRect.x.baseVal.value is 10
+
+Check state in the middle of the animation
+PASS animateMotionRect.getBoundingClientRect().x is 115
+PASS animateMotionRect.getBBox().x is 10
+PASS animateRect.getBBox().x is 115
+PASS animateRect.getBoundingClientRect().x is 115
+PASS animateRect.x.animVal.value is 115
+PASS animateRect.x.baseVal.value is 10
+
+Check state just before the end of the animation
+PASS animateMotionRect.getBoundingClientRect().x is 210
+PASS animateMotionRect.getBBox().x is 10
+PASS animateRect.getBBox().x is 210
+PASS animateRect.getBoundingClientRect().x is 210
+PASS animateRect.x.animVal.value is 210
+PASS animateRect.x.baseVal.value is 10
+
+Check state before/after animation
+PASS animateMotionRect.getBoundingClientRect().x is 10
+PASS animateMotionRect.getBBox().x is 10
+PASS animateRect.getBBox().x is 10
+PASS animateRect.getBoundingClientRect().x is 10
+PASS animateRect.x.animVal.value is 10
+PASS animateRect.x.baseVal.value is 10
+
+Check state before/after animation
+PASS animateMotionRect.getBoundingClientRect().x is 10
+PASS animateMotionRect.getBBox().x is 10
+PASS animateRect.getBBox().x is 10
+PASS animateRect.getBoundingClientRect().x is 10
+PASS animateRect.x.animVal.value is 10
+PASS animateRect.x.baseVal.value is 10
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/svg/animations/fill-remove-support.html b/LayoutTests/svg/animations/fill-remove-support.html
new file mode 100644 (file)
index 0000000..878b420
--- /dev/null
@@ -0,0 +1,82 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../../resources/js-test-pre.js"></script>
+<script src="../dynamic-updates/resources/SVGTestCase.js"></script>
+<script src="resources/SVGAnimationTestCase.js"></script>
+</head>
+<body onload="runSMILTest()">
+<h1>SVG 1.1 dynamic animation tests</h1>
+<p id="description"></p>
+<div id="console"></div>
+<script>
+description("This tests fill=remove for animate and animateMotion");
+embedSVGTestCase("resources/fill-remove-support.svg");
+
+// Setup animation test
+function sampleAnimateBeforeOrAfter() {
+    debug("");
+    debug("Check state before/after animation");
+    shouldBeCloseEnough("animateMotionRect.getBoundingClientRect().x", "10");
+    shouldBeCloseEnough("animateMotionRect.getBBox().x", "10");
+    shouldBeCloseEnough("animateRect.getBBox().x", "10");
+    shouldBeCloseEnough("animateRect.getBoundingClientRect().x", "10");
+    shouldBeCloseEnough("animateRect.x.animVal.value", "10");
+    shouldBe("animateRect.x.baseVal.value", "10");
+}
+
+function sampleAnimateBegin() {
+    debug("");
+    debug("Check state right after the beginning of the animation");
+    shouldBeCloseEnough("animateMotionRect.getBoundingClientRect().x", "20");
+    shouldBeCloseEnough("animateMotionRect.getBBox().x", "10");
+    shouldBeCloseEnough("animateRect.getBBox().x", "20");
+    shouldBeCloseEnough("animateRect.getBoundingClientRect().x", "20");
+    shouldBeCloseEnough("animateRect.x.animVal.value", "20");
+    shouldBe("animateRect.x.baseVal.value", "10");
+}
+
+function sampleAnimateMiddle() {
+    debug("");
+    debug("Check state in the middle of the animation");
+    shouldBeCloseEnough("animateMotionRect.getBoundingClientRect().x", "115");
+    shouldBeCloseEnough("animateMotionRect.getBBox().x", "10");
+    shouldBeCloseEnough("animateRect.getBBox().x", "115");
+    shouldBeCloseEnough("animateRect.getBoundingClientRect().x", "115");
+    shouldBeCloseEnough("animateRect.x.animVal.value", "115");
+    shouldBe("animateRect.x.baseVal.value", "10");
+}
+
+function sampleAnimateEnd() {
+    debug("");
+    debug("Check state just before the end of the animation");
+    shouldBeCloseEnough("animateMotionRect.getBoundingClientRect().x", "210");
+    shouldBeCloseEnough("animateMotionRect.getBBox().x", "10");
+    shouldBeCloseEnough("animateRect.getBBox().x", "210");
+    shouldBeCloseEnough("animateRect.getBoundingClientRect().x", "210");
+    shouldBeCloseEnough("animateRect.x.animVal.value", "210");
+    shouldBe("animateRect.x.baseVal.value", "10");
+}
+
+function executeTest() {
+    animateMotionRect = rootSVGElement.ownerDocument.getElementById("animateMotionRect");
+    animateRect = rootSVGElement.ownerDocument.getElementById("animateRect");
+
+    const expectedValues = [
+        // [animationId, time, sampleCallback]
+        [ "animate", -0.5,    sampleAnimateBeforeOrAfter ],
+        [ "animate",  0.0,    sampleAnimateBegin         ],
+        [ "animate",  1.0,    sampleAnimateMiddle        ],
+        [ "animate",  1.9999, sampleAnimateEnd           ],
+        [ "animate",  2.0,    sampleAnimateBeforeOrAfter ],
+        [ "animate",  3.0,    sampleAnimateBeforeOrAfter ]
+    ];
+
+    runAnimationTest(expectedValues);
+}
+
+window.animationStartsImmediately = true;
+var successfullyParsed = true;
+</script>
+</body>
+</html>
diff --git a/LayoutTests/svg/animations/resources/fill-remove-support.svg b/LayoutTests/svg/animations/resources/fill-remove-support.svg
new file mode 100644 (file)
index 0000000..0bc35ce
--- /dev/null
@@ -0,0 +1,11 @@
+<svg xmlns="http://www.w3.org/2000/svg">
+    <path id="path" d="M10,0 L200,0" fill="none"/>
+    <rect id="animateMotionRect" x="10" y="10" width="100" height="100" fill="green">
+        <animateMotion begin="0.5s" dur="2s" fill="remove">
+            <mpath href="#path"/>
+        </animateMotion>
+    </rect>
+    <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>
+</svg>
index 7c1c870..51db715 100644 (file)
@@ -1,3 +1,46 @@
+2019-09-17  Nikolas Zimmermann  <zimmermann@kde.org>
+
+        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
+
+        Reviewed by Said Abou-Hallawa.
+
+        Fix fill="remove" support for <animateMotion>: properly trigger visual updates.
+        Motion animations are implemented by provding an additional transformation on
+        SVGGraphicsElements: the supplementalTransform().
+
+        When an <animateMotion> element specifies fill="remove" the supplemental transform is
+        correctly reset, when the animation is finished. However, nobody is notified about the
+        change of transformation matrix. Fix by marking the target elements renderer as
+        setNeedsTransformUpdate() and call markForLayoutAndParentResourceInvalidation().
+
+        <animate> elements are not affected by the bug:
+        SVGAnimateElementBase::clearAnimatedType()
+         -> invokes SVGAttributeAnimator::stop()
+            -> invokes applyAnimatedPropertyChange() [via SVGAnimatedPropertyAnimator]
+            -> invokes SVGXXXElement::svgAttributeChanged().
+
+        When animating e.g. the 'transform' attribute SVGGraphicsElement::svgAttributeChanged()
+        will use exactly the same mechanism to trigger visul updates: first call
+        setNeedsTransformUpdate() on the associated renderer, followd by a call to
+        markForLayoutAndParentResourceInvalidation().
+
+        --> Both code paths are now consistent.
+        Also fixes http://web-platform-tests.live/svg/animations/animateMotion-fill-remove.html.
+
+        Tests: svg/animations/fill-remove-support.html
+               svg/animations/animateMotion-remove-freeze-use.svg
+
+        * svg/SVGAnimateMotionElement.cpp:
+        (WebCore::SVGAnimateMotionElement::clearAnimatedType): Call applyResultsToTarget().
+        (WebCore::SVGAnimateMotionElement::calculateAnimatedValue): Stop calling
+        setNeedsTransformUpdate() on the target elements renderer, since applyResultsToTarget()
+        is responsible for modifications of the renderer state.
+        (WebCore::SVGAnimateMotionElement::applyResultsToTarget): Always mark the renderer
+        as setNeedsTransformUpdate() when applying the results, before the call to
+        RenderSVGResource::markForLayoutAndParentResourceInvalidation(). This is more consistent
+        with respect to the code path that updates the SVGElements instances (<use> support).
+
 2019-09-17  Andy Estes  <aestes@apple.com>
 
         [Cocoa] Add a WKA extension point
index 1dac660..2f2beb8 100644 (file)
@@ -156,6 +156,7 @@ void SVGAnimateMotionElement::clearAnimatedType(SVGElement* targetElement)
         return;
     if (AffineTransform* transform = targetElement->supplementalTransform())
         transform->makeIdentity();
+    applyResultsToTarget();
 }
 
 bool SVGAnimateMotionElement::calculateToAtEndOfDurationValue(const String& toAtEndOfDurationString)
@@ -212,13 +213,11 @@ void SVGAnimateMotionElement::calculateAnimatedValue(float percentage, unsigned
     auto targetElement = makeRefPtr(this->targetElement());
     if (!targetElement)
         return;
+
     AffineTransform* transform = targetElement->supplementalTransform();
     if (!transform)
         return;
 
-    if (RenderObject* targetRenderer = targetElement->renderer())
-        targetRenderer->setNeedsTransformUpdate();
-
     if (!isAdditive())
         transform->makeIdentity();
 
@@ -253,8 +252,10 @@ void SVGAnimateMotionElement::applyResultsToTarget()
     if (!targetElement)
         return;
 
-    if (RenderElement* renderer = targetElement->renderer())
+    if (RenderElement* renderer = targetElement->renderer()) {
+        renderer->setNeedsTransformUpdate();
         RenderSVGResource::markForLayoutAndParentResourceInvalidation(*renderer);
+    }
 
     AffineTransform* targetSupplementalTransform = targetElement->supplementalTransform();
     if (!targetSupplementalTransform)