SMIL animations of SVG <view> element have no effect
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 13 Sep 2019 18:21:14 +0000 (18:21 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 13 Sep 2019 18:21:14 +0000 (18:21 +0000)
https://bugs.webkit.org/show_bug.cgi?id=94469

Patch by Nikolas Zimmermann <zimmermann@kde.org> on 2019-09-13
Reviewed by Said Abou-Hallawa.

Source/WebCore:

SMIL animations of the attributes associated with SVGViewElement work fine, but without any
visual effect. When loading an SVG document with a given fragment identifier
(e.g. test.svg#customView) where 'customView' references to an embedded SVGViewElement, the
viewBox/preserveAspectRatio settings should be taken from the SVGViewElement. Currently
there is no link between the SVGViewElement and the SVGSVGElement. The settings from the
SVGViewElement are only pasrsed onco in SVGSVGElement::scrollToFragment(). Dynamic updates
of the 'viewBox' and 'preserveAspectRatio' attributes of the SVGViewElement thus have no
visual effect, since the SVGSVGElement does not re-evaluates its viewBox.

Store a RefPtr to the currently used SVGViewElement in SVGSVGElement, and a WeakPtr back
to the SVGSVGElement that currently references the SVGViewElement. This allows us to
propagate SVGViewElement attribute changes to SVGSVGElement and re-evaluate the viewBox
stored in SVGSVGElement and trigger visual updates.

Tests: svg/custom/animation-on-view-element.html
       svg/custom/multiple-view-elements.html

* svg/SVGSVGElement.cpp:
(WebCore::SVGSVGElement::scrollToFragment):
* svg/SVGSVGElement.h:
* svg/SVGViewElement.cpp:
(WebCore::SVGViewElement::svgAttributeChanged): Add missing implementation, tracked by
webkit.org/b/196554. Correctly handle SVGFitToViewBox property changes. Update the viewBox
stored in the SVGSVGElement, that references the SVGViewElement. Afterwards invalidate the
renderer associated with the SVGSVGElement, which properly triggers visual updates.
* svg/SVGViewElement.h:

LayoutTests:

Add new layout tests to verify that dynamic modifications of the <view> element cause
visual updates. Previously SVGSVGElement was never notified about changes of the
SVGViewElement and thus did not update the stored viewBox.

* svg/custom/animation-on-view-element-expected.html: Added.
* svg/custom/animation-on-view-element.html: Added. This is a new reftest
demonstrating that animations of SVG <view> elements now behave as expected.
* svg/custom/multiple-view-elements-expected.html: Added.
* svg/custom/multiple-view-elements.html: Added.
* svg/dom/SVGViewSpec-multiple-views-expected.txt:
* svg/dom/SVGViewSpec-multiple-views.html: Extend to cover dynamic modifications.

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

13 files changed:
LayoutTests/ChangeLog
LayoutTests/svg/custom/animation-on-view-element-expected.html [new file with mode: 0644]
LayoutTests/svg/custom/animation-on-view-element.html [new file with mode: 0644]
LayoutTests/svg/custom/multiple-view-elements-expected.html [new file with mode: 0644]
LayoutTests/svg/custom/multiple-view-elements.html [new file with mode: 0644]
LayoutTests/svg/custom/resources/animation-on-view-element.svg [new file with mode: 0644]
LayoutTests/svg/dom/SVGViewSpec-multiple-views-expected.txt
LayoutTests/svg/dom/SVGViewSpec-multiple-views.html
Source/WebCore/ChangeLog
Source/WebCore/svg/SVGSVGElement.cpp
Source/WebCore/svg/SVGSVGElement.h
Source/WebCore/svg/SVGViewElement.cpp
Source/WebCore/svg/SVGViewElement.h

index 57f6a79..45c1fb3 100644 (file)
@@ -1,3 +1,22 @@
+2019-09-13  Nikolas Zimmermann  <zimmermann@kde.org>
+
+        SMIL animations of SVG <view> element have no effect
+        https://bugs.webkit.org/show_bug.cgi?id=94469
+
+        Reviewed by Said Abou-Hallawa.
+
+        Add new layout tests to verify that dynamic modifications of the <view> element cause
+        visual updates. Previously SVGSVGElement was never notified about changes of the
+        SVGViewElement and thus did not update the stored viewBox.
+
+        * svg/custom/animation-on-view-element-expected.html: Added.
+        * svg/custom/animation-on-view-element.html: Added. This is a new reftest
+        demonstrating that animations of SVG <view> elements now behave as expected.
+        * svg/custom/multiple-view-elements-expected.html: Added.
+        * svg/custom/multiple-view-elements.html: Added.
+        * svg/dom/SVGViewSpec-multiple-views-expected.txt:
+        * svg/dom/SVGViewSpec-multiple-views.html: Extend to cover dynamic modifications.
+
 2019-09-13  Russell Epstein  <repstein@apple.com>
 
         Unreviewed, rolling out r249709.
diff --git a/LayoutTests/svg/custom/animation-on-view-element-expected.html b/LayoutTests/svg/custom/animation-on-view-element-expected.html
new file mode 100644 (file)
index 0000000..2360a8c
--- /dev/null
@@ -0,0 +1,17 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <script>
+        if (window.testRunner)
+            testRunner.waitUntilDone();
+
+        function loaded() {
+            if (window.testRunner)
+                testRunner.notifyDone();
+        }
+    </script>
+</head>
+<body>
+  <iframe style="border: none" onload="loaded()" width='200' height='200' src='resources/animation-on-view-element.svg'></iframe>
+</body>
+</html>
diff --git a/LayoutTests/svg/custom/animation-on-view-element.html b/LayoutTests/svg/custom/animation-on-view-element.html
new file mode 100644 (file)
index 0000000..3dd4daa
--- /dev/null
@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <script>
+        if (window.testRunner)
+            testRunner.waitUntilDone();
+
+        function loaded() {
+            var svg = document.getElementById("svgFrame").getSVGDocument().documentElement;
+            svg.setCurrentTime(4); // Past end of the animation in the embedded SVG.
+            if (window.testRunner)
+                testRunner.notifyDone();
+        }
+    </script>
+</head>
+<body>
+  <iframe id="svgFrame" style="border: none" onload="loaded()" width='200' height='200' src='resources/animation-on-view-element.svg#customView'></iframe>
+</body>
+</html>
diff --git a/LayoutTests/svg/custom/multiple-view-elements-expected.html b/LayoutTests/svg/custom/multiple-view-elements-expected.html
new file mode 100644 (file)
index 0000000..2cb394a
--- /dev/null
@@ -0,0 +1,37 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <script>
+        if (window.testRunner)
+            testRunner.waitUntilDone();
+
+        function loaded() {
+            if (window.testRunner)
+                testRunner.notifyDone();
+        }
+    </script>
+</head>
+<body onload="loaded()">
+  <div style="display: inline">
+    <svg xmlns="http://www.w3.org/2000/svg" width="200" height="200">
+      <rect x="0" y="0" width="200" height="200" fill="green"/>
+    </svg>
+  </div>
+  <div style="display: inline">
+    <svg xmlns="http://www.w3.org/2000/svg" width="200" height="200">
+      <rect x="0" y="0" width="200" height="200" fill="green"/>
+    </svg>
+  </div>
+  <br/>
+  <div style="display: inline">
+    <svg xmlns="http://www.w3.org/2000/svg" width="200" height="200">
+      <rect x="0" y="0" width="200" height="200" fill="green"/>
+    </svg>
+  </div>
+  <div style="display: inline">
+    <svg xmlns="http://www.w3.org/2000/svg" width="200" height="200">
+      <rect x="0" y="0" width="200" height="200" fill="green"/>
+    </svg>
+  </div>
+</body>
+</html>
diff --git a/LayoutTests/svg/custom/multiple-view-elements.html b/LayoutTests/svg/custom/multiple-view-elements.html
new file mode 100644 (file)
index 0000000..8ba3399
--- /dev/null
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <script>
+        if (window.testRunner)
+            testRunner.waitUntilDone();
+
+        var elementsLoaded = 0;
+        function loaded() {
+            if (window.testRunner && ++elementsLoaded == 4)
+                testRunner.notifyDone();
+        }
+    </script>
+</head>
+<body>
+  <iframe id="svgFrame1" style="border: none" onload="loaded()" width='200' height='200' src='../dom/resources/multiple-view-elements.svg#view1'></iframe>
+  <iframe id="svgFrame2" style="border: none" onload="loaded()" width='200' height='200' src='../dom/resources/multiple-view-elements.svg#view2'></iframe>
+  <br/>
+  <iframe id="svgFrame3" style="border: none" onload="loaded()" width='200' height='200' src='../dom/resources/multiple-view-elements.svg#svgView(viewBox(5 0 10 15);transform(scale(2 2)))'></iframe>
+  <iframe id="svgFrame4" style="border: none" onload="loaded()" width='200' height='200' src='../dom/resources/multiple-view-elements.svg#svgView(viewBox(0 0 10 15);transform(scale(2 2));preserveAspectRatio(xMinYMax meet))'></iframe>
+</body>
+</html>
diff --git a/LayoutTests/svg/custom/resources/animation-on-view-element.svg b/LayoutTests/svg/custom/resources/animation-on-view-element.svg
new file mode 100644 (file)
index 0000000..4da9a80
--- /dev/null
@@ -0,0 +1,16 @@
+<svg xmlns="http://www.w3.org/2000/svg" width="200" height="200">
+  <g style="fill: none; stroke-width: 24; stroke-linecap: round">
+    <g style="stroke: #c9ecba">
+      <line x1="20" y1="20" x2="75" y2="20"/>
+      <line x1="47" y1="20" x2="47" y2="94"/>
+    </g>
+    <g style="stroke: #b0f2f4">
+      <path style="stroke-linejoin: round" d="m 167.5,76 1,74 -46,0 -0.5,-74"/>
+      <path style="stroke-linejoin: square" d="m 75,76,0,74,45,0"/>
+    </g>
+  </g>
+
+  <view id="customView" viewBox="60 85 121 74">
+    <animate attributeName="viewBox" dur="2s" begin="1s" to="0 0 200 200" fill="freeze"/>
+  </view>
+</svg>
index bffbd97..49d52da 100644 (file)
@@ -58,6 +58,15 @@ PASS currentView.preserveAspectRatioString is "xMidYMax slice"
 PASS currentView.preserveAspectRatio.baseVal.align is SVGPreserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMIDYMAX
 PASS currentView.preserveAspectRatio.baseVal.meetOrSlice is SVGPreserveAspectRatio.SVG_MEETORSLICE_SLICE
 
+Test dynamic modifications of the 'view2' element
+
+Check viewBox value after modification
+PASS currentView.viewBox.baseVal.x is 10
+PASS currentView.viewBox.baseVal.y is 10
+PASS currentView.viewBox.baseVal.width is 30
+PASS currentView.viewBox.baseVal.height is 30
+PASS currentView.viewBoxString is "10 10 30 30"
+
 Loading external SVG resources/multiple-view-elements.svg#svgView(viewBox(0 0 10 15);transform(scale(2 2));preserveAspectRatio(xMinYMax meet))...
 
 Verify that no load was performed, but only a different view was set on the same document
index 62a3349..cc54b98 100644 (file)
@@ -98,6 +98,18 @@ function testSecondViewElement() {
     shouldBe("currentView.preserveAspectRatio.baseVal.align", "SVGPreserveAspectRatio.SVG_PRESERVEASPECTRATIO_XMIDYMAX");
     shouldBe("currentView.preserveAspectRatio.baseVal.meetOrSlice", "SVGPreserveAspectRatio.SVG_MEETORSLICE_SLICE");
 
+    debug("");
+    debug("Test dynamic modifications of the 'view2' element");
+    iframeElement.contentDocument.documentElement.getElementById("view2").setAttribute("viewBox", "10 10 30 30");
+
+    debug("");
+    debug("Check viewBox value after modification");
+    shouldBe("currentView.viewBox.baseVal.x", "10");
+    shouldBe("currentView.viewBox.baseVal.y", "10");
+    shouldBe("currentView.viewBox.baseVal.width", "30");
+    shouldBe("currentView.viewBox.baseVal.height", "30");
+    shouldBeEqualToString("currentView.viewBoxString", "10 10 30 30");
+
     nextView = "#svgView(viewBox(0 0 10 15);transform(scale(2 2));preserveAspectRatio(xMinYMax meet))";
     debug("");
     debug("Loading external SVG " + externalSVGFileName + nextView + "...");
index 76568ea..699cd54 100644 (file)
@@ -1,3 +1,37 @@
+2019-09-13  Nikolas Zimmermann  <zimmermann@kde.org>
+
+        SMIL animations of SVG <view> element have no effect
+        https://bugs.webkit.org/show_bug.cgi?id=94469
+
+        Reviewed by Said Abou-Hallawa.
+
+        SMIL animations of the attributes associated with SVGViewElement work fine, but without any
+        visual effect. When loading an SVG document with a given fragment identifier
+        (e.g. test.svg#customView) where 'customView' references to an embedded SVGViewElement, the
+        viewBox/preserveAspectRatio settings should be taken from the SVGViewElement. Currently
+        there is no link between the SVGViewElement and the SVGSVGElement. The settings from the
+        SVGViewElement are only pasrsed onco in SVGSVGElement::scrollToFragment(). Dynamic updates
+        of the 'viewBox' and 'preserveAspectRatio' attributes of the SVGViewElement thus have no
+        visual effect, since the SVGSVGElement does not re-evaluates its viewBox.
+
+        Store a RefPtr to the currently used SVGViewElement in SVGSVGElement, and a WeakPtr back
+        to the SVGSVGElement that currently references the SVGViewElement. This allows us to
+        propagate SVGViewElement attribute changes to SVGSVGElement and re-evaluate the viewBox
+        stored in SVGSVGElement and trigger visual updates.
+
+        Tests: svg/custom/animation-on-view-element.html
+               svg/custom/multiple-view-elements.html
+
+        * svg/SVGSVGElement.cpp:
+        (WebCore::SVGSVGElement::scrollToFragment):
+        * svg/SVGSVGElement.h:
+        * svg/SVGViewElement.cpp:
+        (WebCore::SVGViewElement::svgAttributeChanged): Add missing implementation, tracked by
+        webkit.org/b/196554. Correctly handle SVGFitToViewBox property changes. Update the viewBox
+        stored in the SVGSVGElement, that references the SVGViewElement. Afterwards invalidate the
+        renderer associated with the SVGSVGElement, which properly triggers visual updates.
+        * svg/SVGViewElement.h:
+
 2019-09-13  Brent Fulgham  <bfulgham@apple.com>
 
         [FTW] ImageBuffer/ImageBufferData is highly inefficient
index d757eac..47f0c3c 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2004, 2005, 2006 Nikolas Zimmermann <zimmermann@kde.org>
+ * Copyright (C) 2004, 2005, 2006, 2019 Nikolas Zimmermann <zimmermann@kde.org>
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2010 Rob Buis <buis@kde.org>
  * Copyright (C) 2007-2019 Apple Inc. All rights reserved.
  * Copyright (C) 2014 Adobe Systems Incorporated. All rights reserved.
@@ -666,6 +666,19 @@ bool SVGSVGElement::scrollToFragment(const String& fragmentIdentifier)
     // attributes on the closest ancestor "svg" element.
     if (auto* viewElement = findViewAnchor(fragmentIdentifier)) {
         if (auto* rootElement = findRootAnchor(viewElement)) {
+            if (rootElement->m_currentViewElement) {
+                ASSERT(rootElement->m_currentViewElement->targetElement() == rootElement);
+
+                // If the viewElement has changed, remove the link from the SVGViewElement to the previously selected SVGSVGElement.
+                if (rootElement->m_currentViewElement != viewElement)
+                    rootElement->m_currentViewElement->resetTargetElement();
+            }
+
+            if (rootElement->m_currentViewElement != viewElement) {
+                rootElement->m_currentViewElement = viewElement;
+                rootElement->m_currentViewElement->setTargetElement(*rootElement);
+            }
+
             rootElement->inheritViewAttributes(*viewElement);
             if (auto* renderer = rootElement->renderer())
                 RenderSVGResource::markForLayoutAndParentResourceInvalidation(*renderer);
index d46b428..41b1e7c 100644 (file)
@@ -36,6 +36,7 @@ class SVGMatrix;
 class SVGNumber;
 class SVGRect;
 class SVGTransform;
+class SVGViewElement;
 class SVGViewSpec;
 
 class SVGSVGElement final : public SVGGraphicsElement, public SVGExternalResourcesRequired, public SVGFitToViewBox, public SVGZoomAndPan {
@@ -127,6 +128,8 @@ public:
     SVGAnimatedLength& widthAnimated() { return m_width; }
     SVGAnimatedLength& heightAnimated() { return m_height; }
 
+    void inheritViewAttributes(const SVGViewElement&);
+
 private:
     SVGSVGElement(const QualifiedName&, Document&);
     virtual ~SVGSVGElement();
@@ -149,7 +152,6 @@ private:
 
     AffineTransform localCoordinateSpaceTransform(SVGLocatable::CTMScope) const override;
     RefPtr<Frame> frameForCurrentScale() const;
-    void inheritViewAttributes(const SVGViewElement&);
     Ref<NodeList> collectIntersectionOrEnclosureList(SVGRect&, SVGElement*, bool (*checkFunction)(SVGElement&, SVGRect&));
 
     SVGViewElement* findViewAnchor(const String& fragmentIdentifier) const;
@@ -159,6 +161,7 @@ private:
     bool m_useCurrentView { false };
     Ref<SMILTimeContainer> m_timeContainer;
     RefPtr<SVGViewSpec> m_viewSpec;
+    RefPtr<SVGViewElement> m_currentViewElement;
     String m_currentViewFragmentIdentifier;
 
     Ref<SVGPoint> m_currentTranslate { SVGPoint::create() };
index 273382c..662f065 100644 (file)
@@ -22,7 +22,9 @@
 #include "config.h"
 #include "SVGViewElement.h"
 
+#include "RenderSVGResource.h"
 #include "SVGNames.h"
+#include "SVGSVGElement.h"
 #include "SVGStringList.h"
 #include <wtf/IsoMallocInlines.h>
 
@@ -61,4 +63,24 @@ void SVGViewElement::parseAttribute(const QualifiedName& name, const AtomString&
     SVGZoomAndPan::parseAttribute(name, value);
 }
 
+void SVGViewElement::svgAttributeChanged(const QualifiedName& attrName)
+{
+    // We ignore changes to SVGNames::viewTargetAttr, which is deprecated and unused in WebCore.
+    if (PropertyRegistry::isKnownAttribute(attrName))
+        return;
+
+    if (SVGFitToViewBox::isKnownAttribute(attrName)) {
+        if (m_targetElement) {
+            m_targetElement->inheritViewAttributes(*this);
+            if (auto* renderer = m_targetElement->renderer())
+                RenderSVGResource::markForLayoutAndParentResourceInvalidation(*renderer);
+        }
+
+        return;
+    }
+
+    SVGElement::svgAttributeChanged(attrName);
+    SVGExternalResourcesRequired::svgAttributeChanged(attrName);
+}
+
 }
index 538f864..fe37e91 100644 (file)
@@ -24,6 +24,7 @@
 #include "SVGElement.h"
 #include "SVGExternalResourcesRequired.h"
 #include "SVGFitToViewBox.h"
+#include "SVGSVGElement.h"
 #include "SVGStringList.h"
 #include "SVGZoomAndPan.h"
 
@@ -39,19 +40,24 @@ public:
 
     Ref<SVGStringList> viewTarget() { return m_viewTarget.copyRef(); }
 
+    const SVGSVGElement* targetElement() const { return m_targetElement.get(); }
+    void setTargetElement(const SVGSVGElement& targetElement) { m_targetElement = makeWeakPtr(targetElement); }
+    void resetTargetElement() { m_targetElement = nullptr; }
+
 private:
     SVGViewElement(const QualifiedName&, Document&);
 
     using PropertyRegistry = SVGPropertyOwnerRegistry<SVGViewElement, SVGElement, SVGExternalResourcesRequired, SVGFitToViewBox>;
     const SVGPropertyRegistry& propertyRegistry() const final { return m_propertyRegistry; }
 
-    // FIXME(webkit.org/b/196554): svgAttributeChanged missing.
     void parseAttribute(const QualifiedName&, const AtomString&) final;
+    void svgAttributeChanged(const QualifiedName&) override;
 
     bool rendererIsNeeded(const RenderStyle&) final { return false; }
 
     PropertyRegistry m_propertyRegistry { *this };
     Ref<SVGStringList> m_viewTarget { SVGStringList::create(this) };
+    WeakPtr<SVGSVGElement> m_targetElement { nullptr };
 };
 
 } // namespace WebCore