<g> wrapping <symbol> causes display of hidden <symbol>
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 26 Feb 2016 20:58:32 +0000 (20:58 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 26 Feb 2016 20:58:32 +0000 (20:58 +0000)
https://bugs.webkit.org/show_bug.cgi?id=154576

Patch by Said Abou-Hallawa <sabouhallawa@apple.com> on 2016-02-26
Reviewed by Darin Adler.
Source/WebCore:

The SVGSymbolElement is allowed in the shadow tree of an SVGUseElement
only if it subtree root element. Any descendant SVGSymbolElement should
be removed from the subtree because it is a hidden container. If the cloned
subtree includes an SVGUseElement which references an SVGSymbolElement,
the same rule will be applied to the descendant SVGUseElement. The goal
is to remove all the descendant SVGSymbolElements from the cloned target
because these SVGSymbolElements will be expanded to SVGSVGElements and
hence become visible.

* svg/SVGUseElement.cpp:
(WebCore::disassociateAndRemoveClones): A helper function which removes
cloned SVGElements and their subtrees from their parents and disassociate
them from their originals.

(WebCore::removeDisallowedElementsFromSubtree): Use disassociateAndRemoveClones().

(WebCore::removeSymbolElementsFromSubtree): Removes all the descendant
SVGSymbolElements from the cloned subtree. It does not remove the root
element itself if it is an SVGSymbolElement because this one will be
expanded to an SVGSVGElement which is exactly what we need.

(WebCore::SVGUseElement::cloneTarget): Call removeSymbolElementsFromSubtree()
to remove the descendant SVGSymbolElements from the cloned subtree before
appending it to the container shadow root.

LayoutTests:

Ensure the <symbol> element is not displayed when it's wrapped in a <g>
element and this <g> element is referenced by a <use> element.

* platform/gtk/svg/custom/use-on-g-containing-symbol-expected.png: Removed.
* platform/gtk/svg/custom/use-on-g-containing-symbol-expected.txt: Removed.
* platform/ios-simulator/svg/custom/use-on-g-containing-symbol-expected.txt: Removed.
* platform/mac/svg/custom/use-on-g-containing-symbol-expected.png: Removed.
* platform/mac/svg/custom/use-on-g-containing-symbol-expected.txt: Removed.
* platform/win/svg/custom/use-on-g-containing-symbol-expected.txt: Removed.
* svg/custom/use-on-g-containing-symbol-expected.svg: Added.
* svg/custom/use-on-g-containing-symbol.svg:
The original test was wrong. It had the following definition:
    "<g id='symbol'><symbol>...</symbol></g>"
And it was expecting to have the symbol drawn if the <g> element was
referenced like that
    "<use xlink:href='#symbol'/>"
FireFox does not render anything for this <use> element which is correct.
With this patch, this test failed so it had to be modified to test the right
behavior. Also it is now converted to a ref test.

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

LayoutTests/ChangeLog
LayoutTests/platform/gtk/svg/custom/use-on-g-containing-symbol-expected.png [deleted file]
LayoutTests/platform/gtk/svg/custom/use-on-g-containing-symbol-expected.txt [deleted file]
LayoutTests/platform/ios-simulator/svg/custom/use-on-g-containing-symbol-expected.txt [deleted file]
LayoutTests/platform/mac/svg/custom/use-on-g-containing-symbol-expected.png [deleted file]
LayoutTests/platform/mac/svg/custom/use-on-g-containing-symbol-expected.txt [deleted file]
LayoutTests/platform/win/svg/custom/use-on-g-containing-symbol-expected.txt [deleted file]
LayoutTests/svg/custom/use-on-g-containing-symbol-expected.svg [new file with mode: 0644]
LayoutTests/svg/custom/use-on-g-containing-symbol.svg
Source/WebCore/ChangeLog
Source/WebCore/svg/SVGUseElement.cpp

index 401f7cf..28daa8d 100644 (file)
@@ -1,3 +1,30 @@
+2016-02-26  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        <g> wrapping <symbol> causes display of hidden <symbol>
+        https://bugs.webkit.org/show_bug.cgi?id=154576
+
+        Reviewed by Darin Adler.
+
+        Ensure the <symbol> element is not displayed when it's wrapped in a <g>
+        element and this <g> element is referenced by a <use> element.
+
+        * platform/gtk/svg/custom/use-on-g-containing-symbol-expected.png: Removed.
+        * platform/gtk/svg/custom/use-on-g-containing-symbol-expected.txt: Removed.
+        * platform/ios-simulator/svg/custom/use-on-g-containing-symbol-expected.txt: Removed.
+        * platform/mac/svg/custom/use-on-g-containing-symbol-expected.png: Removed.
+        * platform/mac/svg/custom/use-on-g-containing-symbol-expected.txt: Removed.
+        * platform/win/svg/custom/use-on-g-containing-symbol-expected.txt: Removed.
+        * svg/custom/use-on-g-containing-symbol-expected.svg: Added.
+        * svg/custom/use-on-g-containing-symbol.svg:
+        The original test was wrong. It had the following definition:
+            "<g id='symbol'><symbol>...</symbol></g>"
+        And it was expecting to have the symbol drawn if the <g> element was
+        referenced like that
+            "<use xlink:href='#symbol'/>"
+        FireFox does not render anything for this <use> element which is correct.
+        With this patch, this test failed so it had to be modified to test the right
+        behavior. Also it is now converted to a ref test.
+
 2016-02-26  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r197167.
diff --git a/LayoutTests/platform/gtk/svg/custom/use-on-g-containing-symbol-expected.png b/LayoutTests/platform/gtk/svg/custom/use-on-g-containing-symbol-expected.png
deleted file mode 100644 (file)
index 7c08fd8..0000000
Binary files a/LayoutTests/platform/gtk/svg/custom/use-on-g-containing-symbol-expected.png and /dev/null differ
diff --git a/LayoutTests/platform/gtk/svg/custom/use-on-g-containing-symbol-expected.txt b/LayoutTests/platform/gtk/svg/custom/use-on-g-containing-symbol-expected.txt
deleted file mode 100644 (file)
index 84bcf12..0000000
+++ /dev/null
@@ -1,17 +0,0 @@
-layer at (0,0) size 800x600
-  RenderView at (0,0) size 800x600
-layer at (0,0) size 800x600
-  RenderSVGRoot {svg} at (20,45) size 488x82
-    RenderSVGHiddenContainer {defs} at (0,0) size 0x0
-      RenderSVGContainer {g} at (0,0) size 0x0
-        RenderSVGHiddenContainer {symbol} at (0,0) size 0x0
-          RenderSVGRect {rect} at (0,0) size 125x25 [stroke={[type=SOLID] [color=#000080] [stroke width=5.00]}] [fill={[type=SOLID] [color=#FF0000]}] [x=0.00] [y=0.00] [width=60.00] [height=10.00]
-    RenderSVGContainer {g} at (245,45) size 130x30 [transform={m=((1.00,0.00)(0.00,1.00)) t=(125.00,25.00)}]
-      RenderSVGRect {rect} at (245,45) size 130x30 [stroke={[type=SOLID] [color=#000080] [stroke width=5.00]}] [fill={[type=SOLID] [color=#FF0000]}] [x=0.00] [y=0.00] [width=60.00] [height=10.00]
-    RenderSVGText {text} at (10,45) size 244x19 contains 1 chunk(s)
-      RenderSVGInlineText {#text} at (0,0) size 244x18
-        chunk 1 text run 1 at (10.00,60.00) startOffset 0 endOffset 38 width 244.00: "The two objects should look identical."
-    RenderSVGContainer {use} at (45,45) size 130x30 [transform={m=((1.00,0.00)(0.00,1.00)) t=(25.00,25.00)}]
-      RenderSVGContainer {g} at (45,45) size 130x30
-        RenderSVGViewportContainer {svg} at (45,45) size 130x30
-          RenderSVGRect {rect} at (45,45) size 130x30 [stroke={[type=SOLID] [color=#000080] [stroke width=5.00]}] [fill={[type=SOLID] [color=#FF0000]}] [x=0.00] [y=0.00] [width=60.00] [height=10.00]
diff --git a/LayoutTests/platform/ios-simulator/svg/custom/use-on-g-containing-symbol-expected.txt b/LayoutTests/platform/ios-simulator/svg/custom/use-on-g-containing-symbol-expected.txt
deleted file mode 100644 (file)
index db8f9ec..0000000
+++ /dev/null
@@ -1,17 +0,0 @@
-layer at (0,0) size 800x600
-  RenderView at (0,0) size 800x600
-layer at (0,0) size 800x600
-  RenderSVGRoot {svg} at (20,45) size 489x82
-    RenderSVGHiddenContainer {defs} at (0,0) size 0x0
-      RenderSVGContainer {g} at (0,0) size 0x0
-        RenderSVGHiddenContainer {symbol} at (0,0) size 0x0
-          RenderSVGRect {rect} at (0,0) size 125x25 [stroke={[type=SOLID] [color=#000080] [stroke width=5.00]}] [fill={[type=SOLID] [color=#FF0000]}] [x=0.00] [y=0.00] [width=60.00] [height=10.00]
-    RenderSVGContainer {g} at (245,45) size 130x30 [transform={m=((1.00,0.00)(0.00,1.00)) t=(125.00,25.00)}]
-      RenderSVGRect {rect} at (245,45) size 130x30 [stroke={[type=SOLID] [color=#000080] [stroke width=5.00]}] [fill={[type=SOLID] [color=#FF0000]}] [x=0.00] [y=0.00] [width=60.00] [height=10.00]
-    RenderSVGText {text} at (10,45) size 245x19 contains 1 chunk(s)
-      RenderSVGInlineText {#text} at (0,0) size 245x18
-        chunk 1 text run 1 at (10.00,60.00) startOffset 0 endOffset 38 width 244.40: "The two objects should look identical."
-    RenderSVGContainer {use} at (45,45) size 130x30 [transform={m=((1.00,0.00)(0.00,1.00)) t=(25.00,25.00)}]
-      RenderSVGContainer {g} at (45,45) size 130x30
-        RenderSVGViewportContainer {svg} at (45,45) size 130x30
-          RenderSVGRect {rect} at (45,45) size 130x30 [stroke={[type=SOLID] [color=#000080] [stroke width=5.00]}] [fill={[type=SOLID] [color=#FF0000]}] [x=0.00] [y=0.00] [width=60.00] [height=10.00]
diff --git a/LayoutTests/platform/mac/svg/custom/use-on-g-containing-symbol-expected.png b/LayoutTests/platform/mac/svg/custom/use-on-g-containing-symbol-expected.png
deleted file mode 100644 (file)
index afc748b..0000000
Binary files a/LayoutTests/platform/mac/svg/custom/use-on-g-containing-symbol-expected.png and /dev/null differ
diff --git a/LayoutTests/platform/mac/svg/custom/use-on-g-containing-symbol-expected.txt b/LayoutTests/platform/mac/svg/custom/use-on-g-containing-symbol-expected.txt
deleted file mode 100644 (file)
index 30e6074..0000000
+++ /dev/null
@@ -1,17 +0,0 @@
-layer at (0,0) size 800x600
-  RenderView at (0,0) size 800x600
-layer at (0,0) size 800x600
-  RenderSVGRoot {svg} at (20,45) size 489x83
-    RenderSVGHiddenContainer {defs} at (0,0) size 0x0
-      RenderSVGContainer {g} at (0,0) size 0x0
-        RenderSVGHiddenContainer {symbol} at (0,0) size 0x0
-          RenderSVGRect {rect} at (0,0) size 125x25 [stroke={[type=SOLID] [color=#000080] [stroke width=5.00]}] [fill={[type=SOLID] [color=#FF0000]}] [x=0.00] [y=0.00] [width=60.00] [height=10.00]
-    RenderSVGContainer {g} at (245,45) size 130x30 [transform={m=((1.00,0.00)(0.00,1.00)) t=(125.00,25.00)}]
-      RenderSVGRect {rect} at (245,45) size 130x30 [stroke={[type=SOLID] [color=#000080] [stroke width=5.00]}] [fill={[type=SOLID] [color=#FF0000]}] [x=0.00] [y=0.00] [width=60.00] [height=10.00]
-    RenderSVGText {text} at (10,45) size 245x19 contains 1 chunk(s)
-      RenderSVGInlineText {#text} at (0,0) size 245x19
-        chunk 1 text run 1 at (10.00,60.00) startOffset 0 endOffset 38 width 244.40: "The two objects should look identical."
-    RenderSVGContainer {use} at (45,45) size 130x30 [transform={m=((1.00,0.00)(0.00,1.00)) t=(25.00,25.00)}]
-      RenderSVGContainer {g} at (45,45) size 130x30
-        RenderSVGViewportContainer {svg} at (45,45) size 130x30
-          RenderSVGRect {rect} at (45,45) size 130x30 [stroke={[type=SOLID] [color=#000080] [stroke width=5.00]}] [fill={[type=SOLID] [color=#FF0000]}] [x=0.00] [y=0.00] [width=60.00] [height=10.00]
diff --git a/LayoutTests/platform/win/svg/custom/use-on-g-containing-symbol-expected.txt b/LayoutTests/platform/win/svg/custom/use-on-g-containing-symbol-expected.txt
deleted file mode 100644 (file)
index aefda4a..0000000
+++ /dev/null
@@ -1,17 +0,0 @@
-layer at (0,0) size 800x600
-  RenderView at (0,0) size 800x600
-layer at (0,0) size 800x600
-  RenderSVGRoot {svg} at (20,45) size 488x83
-    RenderSVGHiddenContainer {defs} at (0,0) size 0x0
-      RenderSVGContainer {g} at (0,0) size 0x0
-        RenderSVGHiddenContainer {symbol} at (0,0) size 0x0
-          RenderSVGRect {rect} at (0,0) size 125x25 [stroke={[type=SOLID] [color=#000080] [stroke width=5.00]}] [fill={[type=SOLID] [color=#FF0000]}] [x=0.00] [y=0.00] [width=60.00] [height=10.00]
-    RenderSVGContainer {g} at (245,45) size 130x30 [transform={m=((1.00,0.00)(0.00,1.00)) t=(125.00,25.00)}]
-      RenderSVGRect {rect} at (245,45) size 130x30 [stroke={[type=SOLID] [color=#000080] [stroke width=5.00]}] [fill={[type=SOLID] [color=#FF0000]}] [x=0.00] [y=0.00] [width=60.00] [height=10.00]
-    RenderSVGText {text} at (10,45) size 244x19 contains 1 chunk(s)
-      RenderSVGInlineText {#text} at (0,0) size 244x19
-        chunk 1 text run 1 at (10.00,60.00) startOffset 0 endOffset 38 width 244.00: "The two objects should look identical."
-    RenderSVGContainer {use} at (45,45) size 130x30 [transform={m=((1.00,0.00)(0.00,1.00)) t=(25.00,25.00)}]
-      RenderSVGContainer {g} at (45,45) size 130x30
-        RenderSVGViewportContainer {svg} at (45,45) size 130x30
-          RenderSVGRect {rect} at (45,45) size 130x30 [stroke={[type=SOLID] [color=#000080] [stroke width=5.00]}] [fill={[type=SOLID] [color=#FF0000]}] [x=0.00] [y=0.00] [width=60.00] [height=10.00]
diff --git a/LayoutTests/svg/custom/use-on-g-containing-symbol-expected.svg b/LayoutTests/svg/custom/use-on-g-containing-symbol-expected.svg
new file mode 100644 (file)
index 0000000..ba842a0
--- /dev/null
@@ -0,0 +1,8 @@
+<svg version="1.2" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
+    <symbol id="rect-symbol"> 
+        <rect x="0" y="0" width="100" height="100" fill="green"/>
+    </symbol> 
+    <use xlink:href="#rect-symbol" transform="translate(10,10)"/>
+    <use xlink:href="#rect-symbol" transform="translate(120,10)"/>
+    <use xlink:href="#rect-symbol" transform="translate(230,10)"/>
+</svg>
\ No newline at end of file
index ef7addc..3a761cf 100644 (file)
@@ -1,19 +1,10 @@
-<?xml version="1.0" standalone="no"?>
-<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
-<svg width="800" height="600" viewBox="0 0 400 300" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
-<defs>
-    <g id="symbol">
-        <symbol overflow="visible">
-            <rect stroke-width="5px" fill="red" stroke="navy" width="60" height="10"/>
+<svg version="1.2" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
+    <use xlink:href="#rect-group" transform="translate(110, 0)"></use>
+    <g id="rect-group">
+        <symbol id="rect-symbol"> 
+            <rect x="0" y="0" width="100" height="100" fill="currentColor"/>
         </symbol>
+        <use xlink:href="#rect-symbol" color="green" transform="translate(10,10)"/>
     </g>
-</defs>
-
-<g transform="translate(125 25)">
-    <rect stroke-width="5px" fill="red" stroke="navy" width="60" height="10"/>
-</g>
-
-<text x="10" y="60">The two objects should look identical.</text>
-<use x="25" y="25" xlink:href="#symbol"/>
+    <use xlink:href="#rect-group" transform="translate(220, 0)"/>
 </svg>
-
index cc9898c..3bcd027 100644 (file)
@@ -1,3 +1,35 @@
+2016-02-26  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        <g> wrapping <symbol> causes display of hidden <symbol>
+        https://bugs.webkit.org/show_bug.cgi?id=154576
+
+        Reviewed by Darin Adler.
+        
+        The SVGSymbolElement is allowed in the shadow tree of an SVGUseElement
+        only if it subtree root element. Any descendant SVGSymbolElement should
+        be removed from the subtree because it is a hidden container. If the cloned
+        subtree includes an SVGUseElement which references an SVGSymbolElement,
+        the same rule will be applied to the descendant SVGUseElement. The goal
+        is to remove all the descendant SVGSymbolElements from the cloned target
+        because these SVGSymbolElements will be expanded to SVGSVGElements and
+        hence become visible.
+
+        * svg/SVGUseElement.cpp:
+        (WebCore::disassociateAndRemoveClones): A helper function which removes
+        cloned SVGElements and their subtrees from their parents and disassociate
+        them from their originals.
+        
+        (WebCore::removeDisallowedElementsFromSubtree): Use disassociateAndRemoveClones().
+        
+        (WebCore::removeSymbolElementsFromSubtree): Removes all the descendant
+        SVGSymbolElements from the cloned subtree. It does not remove the root
+        element itself if it is an SVGSymbolElement because this one will be
+        expanded to an SVGSVGElement which is exactly what we need.
+        
+        (WebCore::SVGUseElement::cloneTarget): Call removeSymbolElementsFromSubtree()
+        to remove the descendant SVGSymbolElements from the cloned subtree before
+        appending it to the container shadow root.
+
 2016-02-26  Olivier Blin  <olivier.blin@softathome.com>
 
         Initialize LocaleICU data members in header
index 11d6c98..99142f8 100644 (file)
@@ -310,6 +310,15 @@ RenderElement* SVGUseElement::rendererClipChild() const
     return targetClone->renderer();
 }
 
+static inline void disassociateAndRemoveClones(const Vector<Element*>& clones)
+{
+    for (auto& clone : clones) {
+        for (auto& descendant : descendantsOfType<SVGElement>(*clone))
+            descendant.setCorrespondingElement(nullptr);
+        clone->parentNode()->removeChild(*clone);
+    }
+}
+
 static void removeDisallowedElementsFromSubtree(SVGElement& subtree)
 {
     // Remove disallowed elements after the fact rather than not cloning them in the first place.
@@ -330,11 +339,20 @@ static void removeDisallowedElementsFromSubtree(SVGElement& subtree)
         }
         ++it;
     }
-    for (auto* element : disallowedElements) {
-        for (auto& descendant : descendantsOfType<SVGElement>(*element))
-            descendant.setCorrespondingElement(nullptr);
-        element->parentNode()->removeChild(*element);
-    }
+
+    disassociateAndRemoveClones(disallowedElements);
+}
+
+static void removeSymbolElementsFromSubtree(SVGElement& subtree)
+{
+    // Symbol elements inside the subtree should not be cloned for two reasons: 1) They are invisible and
+    // don't need to be cloned to get correct rendering. 2) expandSymbolElementsInShadowTree will turn them
+    // into <svg> elements, which is correct for symbol elements directly referenced by use elements,
+    // but incorrect for ones that just happen to be in a subtree.
+    Vector<Element*> symbolElements;
+    for (auto& descendant : descendantsOfType<SVGSymbolElement>(subtree))
+        symbolElements.append(&descendant);
+    disassociateAndRemoveClones(symbolElements);
 }
 
 static void associateClonesWithOriginals(SVGElement& clone, SVGElement& original)
@@ -409,6 +427,7 @@ void SVGUseElement::cloneTarget(ContainerNode& container, SVGElement& target) co
     Ref<SVGElement> targetClone = static_cast<SVGElement&>(target.cloneElementWithChildren(document()).get());
     associateClonesWithOriginals(targetClone.get(), target);
     removeDisallowedElementsFromSubtree(targetClone.get());
+    removeSymbolElementsFromSubtree(targetClone.get());
     transferSizeAttributesToTargetClone(targetClone.get());
     container.appendChild(WTFMove(targetClone));
 }