Move height/width implementation for use element from RenderSVGViewportContainer...
authordarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 24 Jan 2015 19:55:58 +0000 (19:55 +0000)
committerdarin@apple.com <darin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 24 Jan 2015 19:55:58 +0000 (19:55 +0000)
https://bugs.webkit.org/show_bug.cgi?id=140826

Reviewed by Anders Carlsson.

Source/WebCore:

Tests: svg/animations/use-animate-width-and-height.html
       svg/custom/use-attribute-invalidations.html
       svg/custom/use-dynamic-attribute-setting.html

This is an adaptation of work Rob Buis did in Blink:

    http://src.chromium.org/viewvc/blink?view=revision&revision=173258

The goal here is to reduce use of SVGElementInstance since we are going to
remove it. The tests Rob added to Blink (which I believe I improved a bit here)
meant we had to fix quite a few bugs in the implementation of the width/height
logic rather than just moving it. Even so, this could use even more test coverage
since there is separate logic for <symbol> and <svg>, three different code paths
(animation/attribute setting, initial creation, and one other), and also
distinct issues for attributes not set at all, attributes set to values that
can't be parsed, and attributes set with different units.

* rendering/svg/RenderSVGViewportContainer.cpp:
(WebCore::RenderSVGViewportContainer::calcViewport): Removed the old logic.

* svg/SVGSVGElement.cpp:
(WebCore::SVGSVGElement::SVGSVGElement): Use ASCIILiteral to more efficiently
create strings from ASCII literals here.
(WebCore::SVGSVGElement::parseAttribute): Default to 100%, not 0, when the
width or height property are either not set or not successfully parsed. Without
this change, one of the SVG tests starts failing.

* svg/SVGUseElement.cpp:
(WebCore::updateWidthAndHeight): Added. The tricky part here is that we have
to copy width and height attributes only if they were successfully parsed, and
also we need to copy the current animating values, not the original attribute
strings. Kind of messy, but I wanted to adapt Rob's solution for the time being,
rather than inventing something new.
(WebCore::SVGUseElement::svgAttributeChanged): Call updateWidthAndHeight.
This is used both when actual attribute changes occur and also when animation
changes the current value.
(WebCore::SVGUseElement::buildShadowAndInstanceTree): Call updateWidthAndHeight.
This is used when the shadow elements are first created.
(WebCore::SVGUseElement::expandUseElementsInShadowTree): Call updateWidthAndHeight.
This was in Rob's patch, but I am not sure we have sufficient test coverage.

LayoutTests:

* platform/mac/svg/custom/relative-sized-shadow-tree-content-with-symbol-expected.png: Old test
result was expecting failure. New one expects success.
* platform/mac/svg/custom/relative-sized-shadow-tree-content-with-symbol-expected.txt: Ditto.
* svg/animations/use-animate-width-and-height-expected.txt: Added.
* svg/animations/use-animate-width-and-height.html: Added.
* svg/custom/use-attribute-invalidations-expected.html: Added.
* svg/custom/use-attribute-invalidations.html: Added.
* svg/custom/use-dynamic-attribute-setting-expected.html: Added.
* svg/custom/use-dynamic-attribute-setting.html: Added.

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

13 files changed:
LayoutTests/ChangeLog
LayoutTests/platform/mac/svg/custom/relative-sized-shadow-tree-content-with-symbol-expected.png
LayoutTests/platform/mac/svg/custom/relative-sized-shadow-tree-content-with-symbol-expected.txt
LayoutTests/svg/animations/use-animate-width-and-height-expected.txt [new file with mode: 0644]
LayoutTests/svg/animations/use-animate-width-and-height.html [new file with mode: 0644]
LayoutTests/svg/custom/use-attribute-invalidations-expected.html [new file with mode: 0644]
LayoutTests/svg/custom/use-attribute-invalidations.html [new file with mode: 0644]
LayoutTests/svg/custom/use-dynamic-attribute-setting-expected.html [new file with mode: 0644]
LayoutTests/svg/custom/use-dynamic-attribute-setting.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp
Source/WebCore/svg/SVGSVGElement.cpp
Source/WebCore/svg/SVGUseElement.cpp

index 0314396c17b48af82a2d29d5a6615dd384570c6a..b202fb1adb33ed7a8619202d425916ddd331a973 100644 (file)
@@ -1,3 +1,20 @@
+2015-01-24  Darin Adler  <darin@apple.com>
+
+        Move height/width implementation for use element from RenderSVGViewportContainer to SVGUseElement
+        https://bugs.webkit.org/show_bug.cgi?id=140826
+
+        Reviewed by Anders Carlsson.
+
+        * platform/mac/svg/custom/relative-sized-shadow-tree-content-with-symbol-expected.png: Old test
+        result was expecting failure. New one expects success.
+        * platform/mac/svg/custom/relative-sized-shadow-tree-content-with-symbol-expected.txt: Ditto.
+        * svg/animations/use-animate-width-and-height-expected.txt: Added.
+        * svg/animations/use-animate-width-and-height.html: Added.
+        * svg/custom/use-attribute-invalidations-expected.html: Added.
+        * svg/custom/use-attribute-invalidations.html: Added.
+        * svg/custom/use-dynamic-attribute-setting-expected.html: Added.
+        * svg/custom/use-dynamic-attribute-setting.html: Added.
+
 2015-01-23  Brent Fulgham  <bfulgham@apple.com>
 
         [Win] Unreviewed gardening. Add Windows baseline for
index 4cf4781904740ef48eedf6899b85f3081ccf0791..b0e602d807d287bcd7fc015d35bc71c2d690969f 100644 (file)
Binary files a/LayoutTests/platform/mac/svg/custom/relative-sized-shadow-tree-content-with-symbol-expected.png and b/LayoutTests/platform/mac/svg/custom/relative-sized-shadow-tree-content-with-symbol-expected.png differ
index 539aff600ec52944e49b7e13b3fa435489487793..3c48a75f4f5a2082d7a5adf67615953a34bf6d9f 100644 (file)
@@ -8,13 +8,13 @@ layer at (0,0) size 800x478
           text run at (0,0) width 744: "The svg area contained in the div element (red box), should fill out the whole area (two green rectangles, first: (0,0)-"
           text run at (0,18) width 686: "(50%,50%), second: (50%,50%)-(100%,100%)), especially after resizing the content box to a different size"
       RenderBlock {div} at (0,52) size 402x402 [border: (1px solid #FF0000)]
-        RenderSVGRoot {svg} at (9,69) size 400x400
+        RenderSVGRoot {svg} at (9,69) size 200x200
           RenderSVGHiddenContainer {defs} at (0,0) size 0x0
             RenderSVGHiddenContainer {symbol} at (0,0) size 0x0
               RenderSVGRect {rect} at (9,69) size 0x0 [fill={[type=SOLID] [color=#008000]}] [x=0.00] [y=0.00] [width=0.00] [height=0.00]
               RenderSVGRect {rect} at (9,69) size 100x100 [fill={[type=SOLID] [color=#008000]}] [x=0.00] [y=0.00] [width=100.00] [height=100.00]
-          RenderSVGContainer {use} at (9,69) size 400x400
-            RenderSVGViewportContainer {svg} at (9,69) size 400x400
-              RenderSVGRect {rect} at (209,269) size 200x200 [fill={[type=SOLID] [color=#008000]}] [x=100.00] [y=100.00] [width=100.00] [height=100.00]
-              RenderSVGRect {rect} at (9,69) size 200x200 [fill={[type=SOLID] [color=#008000]}] [x=0.00] [y=0.00] [width=100.00] [height=100.00]
+          RenderSVGContainer {use} at (9,69) size 200x200
+            RenderSVGViewportContainer {svg} at (9,69) size 200x200
+              RenderSVGRect {rect} at (109,169) size 100x100 [fill={[type=SOLID] [color=#008000]}] [x=100.00] [y=100.00] [width=100.00] [height=100.00]
+              RenderSVGRect {rect} at (9,69) size 100x100 [fill={[type=SOLID] [color=#008000]}] [x=0.00] [y=0.00] [width=100.00] [height=100.00]
         RenderText {#text} at (0,0) size 0x0
diff --git a/LayoutTests/svg/animations/use-animate-width-and-height-expected.txt b/LayoutTests/svg/animations/use-animate-width-and-height-expected.txt
new file mode 100644 (file)
index 0000000..2aa4254
--- /dev/null
@@ -0,0 +1,49 @@
+Test animation of use element width/height
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS use.width.animVal.value is 100
+PASS use.width.baseVal.value is 100
+PASS use.height.animVal.value is 100
+PASS use.height.baseVal.value is 100
+PASS use.getAttribute('width') is '100'
+PASS use.getAttribute('height') is '100'
+PASS shadowRoot.firstChild.width.animVal.value is 100
+PASS shadowRoot.firstChild.height.animVal.value is 100
+PASS use.width.animVal.value is 105
+PASS use.width.baseVal.value is 100
+PASS use.height.animVal.value is 105
+PASS use.height.baseVal.value is 100
+FAIL use.getAttribute('width') should be 105. Was 100.
+FAIL use.getAttribute('height') should be 105. Was 100.
+PASS shadowRoot.firstChild.width.animVal.value is 105
+PASS shadowRoot.firstChild.height.animVal.value is 105
+PASS use.width.animVal.value is 115
+PASS use.width.baseVal.value is 100
+PASS use.height.animVal.value is 115
+PASS use.height.baseVal.value is 100
+FAIL use.getAttribute('width') should be 115. Was 100.
+FAIL use.getAttribute('height') should be 115. Was 100.
+PASS shadowRoot.firstChild.width.animVal.value is 115
+PASS shadowRoot.firstChild.height.animVal.value is 115
+PASS use.width.animVal.value is 125
+PASS use.width.baseVal.value is 100
+PASS use.height.animVal.value is 125
+PASS use.height.baseVal.value is 100
+FAIL use.getAttribute('width') should be 125. Was 100.
+FAIL use.getAttribute('height') should be 125. Was 100.
+PASS shadowRoot.firstChild.width.animVal.value is 125
+PASS shadowRoot.firstChild.height.animVal.value is 125
+PASS use.width.animVal.value is 135
+PASS use.width.baseVal.value is 100
+PASS use.height.animVal.value is 135
+PASS use.height.baseVal.value is 100
+FAIL use.getAttribute('width') should be 135. Was 100.
+FAIL use.getAttribute('height') should be 135. Was 100.
+PASS shadowRoot.firstChild.width.animVal.value is 135
+PASS shadowRoot.firstChild.height.animVal.value is 135
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/svg/animations/use-animate-width-and-height.html b/LayoutTests/svg/animations/use-animate-width-and-height.html
new file mode 100644 (file)
index 0000000..cdc0ad2
--- /dev/null
@@ -0,0 +1,143 @@
+<!DOCTYPE HTML>
+<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()">
+<p id="description"></p>
+<div id="console"></div>
+<script>
+
+description("Test animation of use element width/height");
+createSVGTestCase();
+
+// Set up test document.
+
+var symbol = createSVGElement("symbol");
+symbol.setAttribute("id", "symbol");
+
+var rect = createSVGElement("rect");
+rect.setAttribute("id", "rect");
+rect.setAttribute("x", "10");
+rect.setAttribute("y", "10");
+rect.setAttribute("width", "100%");
+rect.setAttribute("height", "100%");
+rect.setAttribute("fill", "green");
+rect.setAttribute("onclick", "executeTest()");
+symbol.appendChild(rect);
+rootSVGElement.appendChild(symbol);
+
+var use = createSVGElement("use");
+use.setAttribute("id", "use");
+use.setAttributeNS('http://www.w3.org/1999/xlink', 'xlink:href', '#symbol');
+use.setAttribute("x", "0");
+use.setAttribute("y", "0");
+use.setAttribute("width", "100");
+use.setAttribute("height", "100");
+rootSVGElement.appendChild(use);
+
+var animate = createSVGElement("animate");
+animate.setAttribute("id", "animate");
+animate.setAttribute("attributeName", "width");
+animate.setAttribute("attributeType", "XML");
+animate.setAttribute("begin", "1s");
+animate.setAttribute("dur", "10s");
+animate.setAttribute("from", "100");
+animate.setAttribute("to", "200");
+use.appendChild(animate);
+
+var animate2 = createSVGElement("animate");
+animate2.setAttribute("id", "animate2");
+animate2.setAttribute("attributeName", "height");
+animate2.setAttribute("attributeType", "XML");
+animate2.setAttribute("begin", "1s");
+animate2.setAttribute("dur", "10s");
+animate2.setAttribute("from", "100");
+animate2.setAttribute("to", "200");
+use.appendChild(animate2);
+
+var shadowRoot = internals.shadowRoot(use);
+
+function sample1()
+{
+    // Check initial/end conditions
+    shouldBe("use.width.animVal.value", "100");
+    shouldBe("use.width.baseVal.value", "100");
+    shouldBe("use.height.animVal.value", "100");
+    shouldBe("use.height.baseVal.value", "100");
+    shouldBe("use.getAttribute('width')", "'100'");
+    shouldBe("use.getAttribute('height')", "'100'");
+    shouldBe("shadowRoot.firstChild.width.animVal.value", "100");
+    shouldBe("shadowRoot.firstChild.height.animVal.value", "100");
+}
+
+function sample2()
+{
+    shouldBe("use.width.animVal.value", "105");
+    shouldBe("use.width.baseVal.value", "100");
+    shouldBe("use.height.animVal.value", "105");
+    shouldBe("use.height.baseVal.value", "100");
+    shouldBe("use.getAttribute('width')", "'105'");
+    shouldBe("use.getAttribute('height')", "'105'");
+    shouldBe("shadowRoot.firstChild.width.animVal.value", "105");
+    shouldBe("shadowRoot.firstChild.height.animVal.value", "105");
+}
+
+function sample3()
+{
+    shouldBe("use.width.animVal.value", "115");
+    shouldBe("use.width.baseVal.value", "100");
+    shouldBe("use.height.animVal.value", "115");
+    shouldBe("use.height.baseVal.value", "100");
+    shouldBe("use.getAttribute('width')", "'115'");
+    shouldBe("use.getAttribute('height')", "'115'");
+    shouldBe("shadowRoot.firstChild.width.animVal.value", "115");
+    shouldBe("shadowRoot.firstChild.height.animVal.value", "115");
+}
+
+function sample4() {
+    shouldBe("use.width.animVal.value", "125");
+    shouldBe("use.width.baseVal.value", "100");
+    shouldBe("use.height.animVal.value", "125");
+    shouldBe("use.height.baseVal.value", "100");
+    shouldBe("use.getAttribute('width')", "'125'");
+    shouldBe("use.getAttribute('height')", "'125'");
+    shouldBe("shadowRoot.firstChild.width.animVal.value", "125");
+    shouldBe("shadowRoot.firstChild.height.animVal.value", "125");
+}
+
+function sample5()
+{
+    shouldBe("use.width.animVal.value", "135");
+    shouldBe("use.width.baseVal.value", "100");
+    shouldBe("use.height.animVal.value", "135");
+    shouldBe("use.height.baseVal.value", "100");
+    shouldBe("use.getAttribute('width')", "'135'");
+    shouldBe("use.getAttribute('height')", "'135'");
+    shouldBe("shadowRoot.firstChild.width.animVal.value", "135");
+    shouldBe("shadowRoot.firstChild.height.animVal.value", "135");
+}
+
+function executeTest()
+{
+    const expectedValues = [
+        // [animationId, time, sampleCallback]
+        ["animate2", 0.0,   sample1],
+        ["animate2", 0.5,   sample2],
+        ["animate2", 1.5,   sample3],
+        ["animate2", 2.5,   sample4],
+        ["animate2", 3.5,   sample5]
+    ];
+
+    runAnimationTest(expectedValues);
+}
+
+window.clickX = 50;
+window.clickY = 50;
+
+var successfullyParsed = true;
+</script>
+</body>
+</html>
diff --git a/LayoutTests/svg/custom/use-attribute-invalidations-expected.html b/LayoutTests/svg/custom/use-attribute-invalidations-expected.html
new file mode 100644 (file)
index 0000000..88ef571
--- /dev/null
@@ -0,0 +1,17 @@
+<!DOCTYPE HTML>
+<html>
+<body>
+<p>This test passes if there are three 100x100px green squares in a diagonal line below:</p>
+<svg width="300" height="300">
+    <svg width="100" height="100">
+        <rect width="100%" height="100%" fill="green"/>
+    </svg>
+    <svg x="100" y="100" width="100" height="100">
+        <rect width="100%" height="100%" fill="green"/>
+    </svg>
+    <svg x="200" y="200" width="100" height="100">
+        <rect width="100%" height="100%" fill="green"/>
+    </svg>
+</svg>
+</body>
+</html>
diff --git a/LayoutTests/svg/custom/use-attribute-invalidations.html b/LayoutTests/svg/custom/use-attribute-invalidations.html
new file mode 100644 (file)
index 0000000..98b6838
--- /dev/null
@@ -0,0 +1,27 @@
+<!DOCTYPE HTML>
+<html>
+<body>
+<p>This test passes if there are three 100x100px green squares in a diagonal line below:</p>
+<svg width="300" height="300">
+    <defs>
+        <symbol id="symbol" width="100">
+            <rect width="100%" height="100%" fill="green"/>
+        </symbol>
+    </defs>
+    <svg x="0" y="0" width="100" height="100">
+        <use id="use1" xlink:href="#symbol"/>
+    </svg>
+    <use id="use2" xlink:href="#symbol" x="100" y="100" width="10" height="100"/>
+    <use id="use3" xlink:href="#symbol" x="200" y="200" width="100" height="10"/>
+</svg>
+<script>
+    // Force layout since the intention is to test behavior of changes *after* layout has occurred.
+    // FIXME: Find a clean reliable way to do this; Blink tests were using requestAnimationFrame,
+    // which seems like it would be racy and wrong, but perhaps they have some rationale for that.
+    document.body.offsetTop;
+
+    document.getElementById('use2').removeAttribute('width');
+    document.getElementById('use3').setAttribute('height', '100');
+</script>
+</body>
+</html>
diff --git a/LayoutTests/svg/custom/use-dynamic-attribute-setting-expected.html b/LayoutTests/svg/custom/use-dynamic-attribute-setting-expected.html
new file mode 100644 (file)
index 0000000..8183eeb
--- /dev/null
@@ -0,0 +1,14 @@
+<!DOCTYPE HTML>
+<html>
+<body>
+<p>This test passes if there are two 100x100px green squares in a diagonal line below:</p>
+<svg width="300" height="300">
+    <svg width="100" height="100">
+        <rect width="100%" height="100%" fill="green"/>
+    </svg>
+    <svg x="100" y="100" width="100" height="100">
+        <rect width="100%" height="100%" fill="green"/>
+    </svg>
+</svg>
+</body>
+</html>
diff --git a/LayoutTests/svg/custom/use-dynamic-attribute-setting.html b/LayoutTests/svg/custom/use-dynamic-attribute-setting.html
new file mode 100644 (file)
index 0000000..cd1e2a0
--- /dev/null
@@ -0,0 +1,24 @@
+<!DOCTYPE HTML>
+<html>
+<body>
+<p>This test passes if there are two 100x100px green squares in a diagonal line below:</p>
+<svg width="300" height="300">
+    <defs>
+        <svg id="svg" width="10" height="10">
+            <rect width="100%" height="100%" fill="green"/>
+        </svg>
+    </defs>
+    <use id="use1" xlink:href="#svg" x="0" y="0" width="100" height="100"/>
+    <use id="use2" xlink:href="#svg" x="100" y="100" width="10" height="10"/>
+</svg>
+<script>
+    // Force layout since the intention is to test behavior of changes *after* layout has occurred.
+    // FIXME: Find a clean reliable way to do this; Blink tests were using requestAnimationFrame,
+    // which seems like it would be racy and wrong, but perhaps they have some rationale for that.
+    document.body.offsetTop;
+
+    document.getElementById('use2').setAttribute('width', '100');
+    document.getElementById('use2').setAttribute('height', '100');
+</script>
+</body>
+</html>
index 9d1116c5e93d8b6c628b3a3e294b3f79d43f9563..009b89e34738b59d4545deccda73568aa8013e86 100644 (file)
@@ -1,3 +1,51 @@
+2015-01-24  Darin Adler  <darin@apple.com>
+
+        Move height/width implementation for use element from RenderSVGViewportContainer to SVGUseElement
+        https://bugs.webkit.org/show_bug.cgi?id=140826
+
+        Reviewed by Anders Carlsson.
+
+        Tests: svg/animations/use-animate-width-and-height.html
+               svg/custom/use-attribute-invalidations.html
+               svg/custom/use-dynamic-attribute-setting.html
+
+        This is an adaptation of work Rob Buis did in Blink:
+
+            http://src.chromium.org/viewvc/blink?view=revision&revision=173258
+
+        The goal here is to reduce use of SVGElementInstance since we are going to
+        remove it. The tests Rob added to Blink (which I believe I improved a bit here)
+        meant we had to fix quite a few bugs in the implementation of the width/height
+        logic rather than just moving it. Even so, this could use even more test coverage
+        since there is separate logic for <symbol> and <svg>, three different code paths
+        (animation/attribute setting, initial creation, and one other), and also
+        distinct issues for attributes not set at all, attributes set to values that
+        can't be parsed, and attributes set with different units.
+
+        * rendering/svg/RenderSVGViewportContainer.cpp:
+        (WebCore::RenderSVGViewportContainer::calcViewport): Removed the old logic.
+
+        * svg/SVGSVGElement.cpp:
+        (WebCore::SVGSVGElement::SVGSVGElement): Use ASCIILiteral to more efficiently
+        create strings from ASCII literals here.
+        (WebCore::SVGSVGElement::parseAttribute): Default to 100%, not 0, when the
+        width or height property are either not set or not successfully parsed. Without
+        this change, one of the SVG tests starts failing.
+
+        * svg/SVGUseElement.cpp:
+        (WebCore::updateWidthAndHeight): Added. The tricky part here is that we have
+        to copy width and height attributes only if they were successfully parsed, and
+        also we need to copy the current animating values, not the original attribute
+        strings. Kind of messy, but I wanted to adapt Rob's solution for the time being,
+        rather than inventing something new.
+        (WebCore::SVGUseElement::svgAttributeChanged): Call updateWidthAndHeight.
+        This is used both when actual attribute changes occur and also when animation
+        changes the current value.
+        (WebCore::SVGUseElement::buildShadowAndInstanceTree): Call updateWidthAndHeight.
+        This is used when the shadow elements are first created.
+        (WebCore::SVGUseElement::expandUseElementsInShadowTree): Call updateWidthAndHeight.
+        This was in Rob's patch, but I am not sure we have sufficient test coverage.
+
 2015-01-24  Chris Dumez  <cdumez@apple.com>
 
         Provide implementation for WTF::DefaultHash<bool>
index 526075abf2df443f6a8db824b74c4e9fa2cd90ca..85826783fc1d16a46c8f1575dea4aeeac9d1f576 100644 (file)
@@ -58,62 +58,17 @@ void RenderSVGViewportContainer::applyViewportClip(PaintInfo& paintInfo)
 
 void RenderSVGViewportContainer::calcViewport()
 {
-    SVGSVGElement& svg = svgSVGElement();
-    FloatRect oldViewport = m_viewport;
-
-    SVGLengthContext lengthContext(&svg);
-    m_viewport = FloatRect(svg.x().value(lengthContext), svg.y().value(lengthContext), svg.width().value(lengthContext), svg.height().value(lengthContext));
-
-    SVGElement* correspondingElement = svg.correspondingElement();
-    if (correspondingElement && svg.isInShadowTree()) {
-        const HashSet<SVGElementInstance*>& instances = correspondingElement->instancesForElement();
-        ASSERT(!instances.isEmpty());
-
-        SVGUseElement* useElement = 0;
-        const HashSet<SVGElementInstance*>::const_iterator end = instances.end();
-        for (HashSet<SVGElementInstance*>::const_iterator it = instances.begin(); it != end; ++it) {
-            const SVGElementInstance* instance = (*it);
-            ASSERT(instance->correspondingElement()->hasTagName(SVGNames::svgTag) || instance->correspondingElement()->hasTagName(SVGNames::symbolTag));
-            if (instance->shadowTreeElement() == &svg) {
-                ASSERT(correspondingElement == instance->correspondingElement());
-                useElement = instance->directUseElement();
-                if (!useElement)
-                    useElement = instance->correspondingUseElement();
-                break;
-            }
-        }
-
-        ASSERT(useElement);
-        bool isSymbolElement = correspondingElement->hasTagName(SVGNames::symbolTag);
-
-        // Spec (<use> on <symbol>): This generated 'svg' will always have explicit values for attributes width and height.
-        // If attributes width and/or height are provided on the 'use' element, then these attributes
-        // will be transferred to the generated 'svg'. If attributes width and/or height are not specified,
-        // the generated 'svg' element will use values of 100% for these attributes.
-
-        // Spec (<use> on <svg>): If attributes width and/or height are provided on the 'use' element, then these
-        // values will override the corresponding attributes on the 'svg' in the generated tree.
-
-        SVGLengthContext lengthContext(&svg);
-        if (useElement->hasAttribute(SVGNames::widthAttr))
-            m_viewport.setWidth(useElement->width().value(lengthContext));
-        else if (isSymbolElement && svg.hasAttribute(SVGNames::widthAttr)) {
-            SVGLength containerWidth(LengthModeWidth, "100%");
-            m_viewport.setWidth(containerWidth.value(lengthContext));
-        }
-
-        if (useElement->hasAttribute(SVGNames::heightAttr))
-            m_viewport.setHeight(useElement->height().value(lengthContext));
-        else if (isSymbolElement && svg.hasAttribute(SVGNames::heightAttr)) {
-            SVGLength containerHeight(LengthModeHeight, "100%");
-            m_viewport.setHeight(containerHeight.value(lengthContext));
-        }
-    }
-
-    if (oldViewport != m_viewport) {
-        setNeedsBoundariesUpdate();
-        setNeedsTransformUpdate();
-    }
+    SVGSVGElement& element = svgSVGElement();
+    SVGLengthContext lengthContext(&element);
+    FloatRect newViewport(element.x().value(lengthContext), element.y().value(lengthContext), element.width().value(lengthContext), element.height().value(lengthContext));
+
+    if (m_viewport == newViewport)
+        return;
+
+    m_viewport = newViewport;
+
+    setNeedsBoundariesUpdate();
+    setNeedsTransformUpdate();
 }
 
 bool RenderSVGViewportContainer::calculateLocalTransform() 
index 6871987d303ad487e8ab9e55b1dc1e844c3ad925..9ed136e65718b27d6791072b9ff99c667b293c0d 100644 (file)
@@ -84,8 +84,8 @@ inline SVGSVGElement::SVGSVGElement(const QualifiedName& tagName, Document& docu
     : SVGGraphicsElement(tagName, document)
     , m_x(LengthModeWidth)
     , m_y(LengthModeHeight)
-    , m_width(LengthModeWidth, "100%")
-    , m_height(LengthModeHeight, "100%") 
+    , m_width(LengthModeWidth, ASCIILiteral("100%"))
+    , m_height(LengthModeHeight, ASCIILiteral("100%"))
     , m_useCurrentView(false)
     , m_zoomAndPan(SVGZoomAndPanMagnify)
     , m_timeContainer(SMILTimeContainer::create(this))
@@ -257,11 +257,23 @@ void SVGSVGElement::parseAttribute(const QualifiedName& name, const AtomicString
         setXBaseValue(SVGLength::construct(LengthModeWidth, value, parseError));
     else if (name == SVGNames::yAttr)
         setYBaseValue(SVGLength::construct(LengthModeHeight, value, parseError));
-    else if (name == SVGNames::widthAttr)
-        setWidthBaseValue(SVGLength::construct(LengthModeWidth, value, parseError, ForbidNegativeLengths));
-    else if (name == SVGNames::heightAttr)
-        setHeightBaseValue(SVGLength::construct(LengthModeHeight, value, parseError, ForbidNegativeLengths));
-    else if (SVGLangSpace::parseAttribute(name, value)
+    else if (name == SVGNames::widthAttr) {
+        SVGLength length = SVGLength::construct(LengthModeWidth, value, parseError, ForbidNegativeLengths);
+        if (parseError != NoError || value.isEmpty()) {
+            // FIXME: This is definitely the correct behavior for a missing/removed attribute.
+            // Not sure it's correct for the empty string or for something that can't be parsed.
+            length = SVGLength(LengthModeWidth, ASCIILiteral("100%"));
+        }
+        setWidthBaseValue(length);
+    } else if (name == SVGNames::heightAttr) {
+        SVGLength length = SVGLength::construct(LengthModeHeight, value, parseError, ForbidNegativeLengths);
+        if (parseError != NoError || value.isEmpty()) {
+            // FIXME: This is definitely the correct behavior for a removed attribute.
+            // Not sure it's correct for the empty string or for something that can't be parsed.
+            length = SVGLength(LengthModeHeight, ASCIILiteral("100%"));
+        }
+        setHeightBaseValue(length);
+    } else if (SVGLangSpace::parseAttribute(name, value)
                || SVGExternalResourcesRequired::parseAttribute(name, value)
                || SVGFitToViewBox::parseAttribute(this, name, value)
                || SVGZoomAndPan::parseAttribute(this, name, value)) {
index ce9db35749e60150516a1c771b19d3333bc6f772..833425a74fae53fe6946f47ce029f8c5aeffd5de 100644 (file)
@@ -213,6 +213,26 @@ Document* SVGUseElement::externalDocument() const
     return 0;
 }
 
+static void updateWidthAndHeight(SVGElement& shadowElement, const SVGUseElement& useElement, const SVGElement& originalElement)
+{
+    // FIXME: The check for valueInSpecifiedUnits being non-zero below is a workaround for the fact
+    // that we currently have no good way to tell whether a particular animatable attribute is a value
+    // indicating it was unspecified, or specified but could not be parsed. Would be nice to fix that some day.
+    if (is<SVGSymbolElement>(shadowElement)) {
+        // Spec (<use> on <symbol>): This generated 'svg' will always have explicit values for attributes width and height.
+        // If attributes width and/or height are provided on the 'use' element, then these attributes
+        // will be transferred to the generated 'svg'. If attributes width and/or height are not specified,
+        // the generated 'svg' element will use values of 100% for these attributes.
+        shadowElement.setAttribute(SVGNames::widthAttr, (useElement.widthIsValid() && useElement.width().valueInSpecifiedUnits()) ? AtomicString(useElement.width().valueAsString()) : "100%");
+        shadowElement.setAttribute(SVGNames::heightAttr, (useElement.heightIsValid() && useElement.height().valueInSpecifiedUnits()) ? AtomicString(useElement.height().valueAsString()) : "100%");
+    } else if (is<SVGSVGElement>(shadowElement)) {
+        // Spec (<use> on <svg>): If attributes width and/or height are provided on the 'use' element, then these
+        // values will override the corresponding attributes on the 'svg' in the generated tree.
+        shadowElement.setAttribute(SVGNames::widthAttr, (useElement.widthIsValid() && useElement.width().valueInSpecifiedUnits()) ? AtomicString(useElement.width().valueAsString()) : originalElement.getAttribute(SVGNames::widthAttr));
+        shadowElement.setAttribute(SVGNames::heightAttr, (useElement.heightIsValid() && useElement.height().valueInSpecifiedUnits()) ? AtomicString(useElement.height().valueAsString()) : originalElement.getAttribute(SVGNames::heightAttr));
+    }
+}
+
 void SVGUseElement::svgAttributeChanged(const QualifiedName& attrName)
 {
     if (!isSupportedAttribute(attrName)) {
@@ -222,13 +242,16 @@ void SVGUseElement::svgAttributeChanged(const QualifiedName& attrName)
 
     SVGElementInstance::InvalidationGuard invalidationGuard(this);
 
-    auto renderer = this->renderer();
-    if (attrName == SVGNames::xAttr
-        || attrName == SVGNames::yAttr
-        || attrName == SVGNames::widthAttr
-        || attrName == SVGNames::heightAttr) {
+    if (attrName == SVGNames::xAttr || attrName == SVGNames::yAttr || attrName == SVGNames::widthAttr || attrName == SVGNames::heightAttr) {
         updateRelativeLengthsInformation();
-        if (renderer)
+        if (m_targetElementInstance) {
+            // FIXME: It's unnecessarily inefficient to do this work any time we change "x" or "y".
+            // FIXME: It's unnecessarily inefficient to update both width and height each time either is changed.
+            ASSERT(m_targetElementInstance->shadowTreeElement());
+            ASSERT(m_targetElementInstance->correspondingElement());
+            updateWidthAndHeight(*m_targetElementInstance->shadowTreeElement(), *this, *m_targetElementInstance->correspondingElement());
+        }
+        if (auto* renderer = this->renderer())
             RenderSVGResource::markForLayoutAndParentResourceInvalidation(*renderer);
         return;
     }
@@ -429,6 +452,10 @@ void SVGUseElement::buildShadowAndInstanceTree(SVGElement* target)
     // shadow tree elements <-> instances in the instance tree.
     associateInstancesWithShadowTreeElements(shadowTreeRootElement->firstChild(), m_targetElementInstance.get());
 
+    ASSERT(m_targetElementInstance->shadowTreeElement());
+    ASSERT(m_targetElementInstance->correspondingElement() == target);
+    updateWidthAndHeight(*m_targetElementInstance->shadowTreeElement(), *this, *target);
+
     // If no shadow tree element is present, this means that the reference root
     // element was removed, as it is disallowed (ie. <use> on <foreignObject>)
     // Do NOT leave an inconsistent instance tree around, instead destruct it.
@@ -651,7 +678,7 @@ void SVGUseElement::expandUseElementsInShadowTree(Node* element)
 
         if (target && !isDisallowedElement(*target)) {
             RefPtr<Element> newChild = target->cloneElementWithChildren(document());
-            ASSERT(newChild->isSVGElement());
+            updateWidthAndHeight(downcast<SVGElement>(*newChild), use, *target);
             cloneParent->appendChild(newChild.release());
         }