SVG: hit testing region for <text> elements is incorrect
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 4 Nov 2015 10:40:17 +0000 (10:40 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 4 Nov 2015 10:40:17 +0000 (10:40 +0000)
https://bugs.webkit.org/show_bug.cgi?id=150838

Patch by Antoine Quint <graouts@apple.com> on 2015-11-04
Reviewed by Dean Jackson.

Source/WebCore:

Hit testing for SVG <text> elements was using the same code as hit testing
for CSS-rendered elements. However, in SVG, text elements should only hit
test based on their character cells, not the rectangular bounds of the
element, see section 16.6 of the SVG 1.1 specification:

http://www.w3.org/TR/SVG11/interact.html#PointerEventsProperty

So we now hit test each SVGTextFragment of each SVGInlineTextBox
that is a child of an SVGRootInlineBox to correctly find whether the
provided HitTestLocation is contained within a character cell.

Tests: svg/hittest/text-dominant-baseline-hanging.svg
       svg/hittest/text-multiple-dx-values.svg
       svg/hittest/text-with-multiple-tspans.svg
       svg/hittest/text-with-text-node-and-content-elements.svg
       svg/hittest/text-with-text-node-only.svg
       svg/hittest/text-with-text-path.svg

* rendering/RootInlineBox.h:
Remove the final keyword since nodeAtPoint() may now be subclassed as
implemented in SVGRootInlineBox.

* rendering/svg/SVGInlineTextBox.cpp:
(WebCore::SVGInlineTextBox::nodeAtPoint):
Iterate over the SVGTextFragments to look for a fragment containing the
provided HitTestLocation.

* rendering/svg/SVGRootInlineBox.cpp:
(WebCore::SVGRootInlineBox::nodeAtPoint):
* rendering/svg/SVGRootInlineBox.h:
Override RootInlineBox::nodeAtPoint() to delegate hit testing to the
children inline boxes.

LayoutTests:

* svg/hittest/text-dominant-baseline-hanging-expected.svg: Added.
* svg/hittest/text-dominant-baseline-hanging.svg: Added.
* svg/hittest/text-multiple-dx-values-expected.svg: Added.
* svg/hittest/text-multiple-dx-values.svg: Added.
* svg/hittest/text-with-multiple-tspans-expected.svg: Added.
* svg/hittest/text-with-multiple-tspans.svg: Added.
* svg/hittest/text-with-text-node-and-content-elements-expected.svg: Added.
* svg/hittest/text-with-text-node-and-content-elements.svg: Added.
* svg/hittest/text-with-text-node-only-expected.svg: Added.
* svg/hittest/text-with-text-node-only.svg: Added.
* svg/hittest/text-with-text-path-expected.svg: Added.
* svg/hittest/text-with-text-path.svg: Added.

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

18 files changed:
LayoutTests/ChangeLog
LayoutTests/svg/hittest/text-dominant-baseline-hanging-expected.svg [new file with mode: 0644]
LayoutTests/svg/hittest/text-dominant-baseline-hanging.svg [new file with mode: 0644]
LayoutTests/svg/hittest/text-multiple-dx-values-expected.svg [new file with mode: 0644]
LayoutTests/svg/hittest/text-multiple-dx-values.svg [new file with mode: 0644]
LayoutTests/svg/hittest/text-with-multiple-tspans-expected.svg [new file with mode: 0644]
LayoutTests/svg/hittest/text-with-multiple-tspans.svg [new file with mode: 0644]
LayoutTests/svg/hittest/text-with-text-node-and-content-elements-expected.svg [new file with mode: 0644]
LayoutTests/svg/hittest/text-with-text-node-and-content-elements.svg [new file with mode: 0644]
LayoutTests/svg/hittest/text-with-text-node-only-expected.svg [new file with mode: 0644]
LayoutTests/svg/hittest/text-with-text-node-only.svg [new file with mode: 0644]
LayoutTests/svg/hittest/text-with-text-path-expected.svg [new file with mode: 0644]
LayoutTests/svg/hittest/text-with-text-path.svg [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RootInlineBox.h
Source/WebCore/rendering/svg/SVGInlineTextBox.cpp
Source/WebCore/rendering/svg/SVGRootInlineBox.cpp
Source/WebCore/rendering/svg/SVGRootInlineBox.h

index 01d7950..f96ca17 100644 (file)
@@ -1,3 +1,23 @@
+2015-11-04  Antoine Quint  <graouts@apple.com>
+
+        SVG: hit testing region for <text> elements is incorrect
+        https://bugs.webkit.org/show_bug.cgi?id=150838
+
+        Reviewed by Dean Jackson.
+
+        * svg/hittest/text-dominant-baseline-hanging-expected.svg: Added.
+        * svg/hittest/text-dominant-baseline-hanging.svg: Added.
+        * svg/hittest/text-multiple-dx-values-expected.svg: Added.
+        * svg/hittest/text-multiple-dx-values.svg: Added.
+        * svg/hittest/text-with-multiple-tspans-expected.svg: Added.
+        * svg/hittest/text-with-multiple-tspans.svg: Added.
+        * svg/hittest/text-with-text-node-and-content-elements-expected.svg: Added.
+        * svg/hittest/text-with-text-node-and-content-elements.svg: Added.
+        * svg/hittest/text-with-text-node-only-expected.svg: Added.
+        * svg/hittest/text-with-text-node-only.svg: Added.
+        * svg/hittest/text-with-text-path-expected.svg: Added.
+        * svg/hittest/text-with-text-path.svg: Added.
+
 2015-11-04  Xabier Rodriguez Calvar  <calvaris@igalia.com>
 
         Unreviewed.
diff --git a/LayoutTests/svg/hittest/text-dominant-baseline-hanging-expected.svg b/LayoutTests/svg/hittest/text-dominant-baseline-hanging-expected.svg
new file mode 100644 (file)
index 0000000..7232c4d
--- /dev/null
@@ -0,0 +1,3 @@
+<svg width="500" height="150" viewBox="0 0 1000 300" xmlns="http://www.w3.org/2000/svg">
+    <rect width="100%" height="100%" fill="green" />
+</svg>
diff --git a/LayoutTests/svg/hittest/text-dominant-baseline-hanging.svg b/LayoutTests/svg/hittest/text-dominant-baseline-hanging.svg
new file mode 100644 (file)
index 0000000..27df312
--- /dev/null
@@ -0,0 +1,28 @@
+<svg width="500" height="150" viewBox="0 0 1000 300" xmlns="http://www.w3.org/2000/svg">
+    <rect id="background" width="100%" height="100%" fill="red" />
+    <text id="text" dominant-baseline="hanging" x="10" y="10">Foo</text>
+    <defs>
+        <style type="text/css">
+        <![CDATA[
+            @font-face {
+                font-family: Ahem;
+                src: url(../../resources/Ahem.ttf);
+            }
+            text {
+                font-family: Ahem;
+                font-size: 40px;
+            }
+            ]]>
+        </style>
+        <script type="text/javascript">
+        <![CDATA[
+            // The point at 15,15 is contained within the text's bounding box.
+            var text = document.getElementById("text");
+            if (document.elementFromPoint(15, 15) === text) {
+                document.getElementById("background").setAttribute("fill", "green");
+                text.remove();
+            }
+        ]]>
+        </script>
+    </defs>
+</svg>
diff --git a/LayoutTests/svg/hittest/text-multiple-dx-values-expected.svg b/LayoutTests/svg/hittest/text-multiple-dx-values-expected.svg
new file mode 100644 (file)
index 0000000..7232c4d
--- /dev/null
@@ -0,0 +1,3 @@
+<svg width="500" height="150" viewBox="0 0 1000 300" xmlns="http://www.w3.org/2000/svg">
+    <rect width="100%" height="100%" fill="green" />
+</svg>
diff --git a/LayoutTests/svg/hittest/text-multiple-dx-values.svg b/LayoutTests/svg/hittest/text-multiple-dx-values.svg
new file mode 100644 (file)
index 0000000..56dd6a7
--- /dev/null
@@ -0,0 +1,31 @@
+<svg width="500" height="150" viewBox="0 0 1000 300" xmlns="http://www.w3.org/2000/svg">
+    <rect id="background" width="100%" height="100%" fill="red" />
+    <text id="text" x="10" y="1em" dx="0 50 100">Foo</text>
+    <defs>
+        <style type="text/css">
+        <![CDATA[
+            @font-face {
+                font-family: Ahem;
+                src: url(../../resources/Ahem.ttf);
+            }
+            text {
+                font-family: Ahem;
+                font-size: 40px;
+            }
+            ]]>
+        </style>
+        <script type="text/javascript">
+        <![CDATA[
+            // The point at 15,15 is contained within the text's bounding box.
+            // The point at 30,15 is contained in between the text's character cells
+            // and thus should return the background.
+            var text = document.getElementById("text");
+            var background = document.getElementById("background");
+            if (document.elementFromPoint(15, 15) === text && document.elementFromPoint(35, 15) === background) {
+                background.setAttribute("fill", "green");
+                text.remove();
+            }
+        ]]>
+        </script>
+    </defs>
+</svg>
diff --git a/LayoutTests/svg/hittest/text-with-multiple-tspans-expected.svg b/LayoutTests/svg/hittest/text-with-multiple-tspans-expected.svg
new file mode 100644 (file)
index 0000000..4be92d6
--- /dev/null
@@ -0,0 +1,3 @@
+<svg xmlns="http://www.w3.org/2000/svg" width="300" height="150" viewBox="0 0 300 150">
+    <rect width="100%" height="100%" fill="green" />
+</svg>
\ No newline at end of file
diff --git a/LayoutTests/svg/hittest/text-with-multiple-tspans.svg b/LayoutTests/svg/hittest/text-with-multiple-tspans.svg
new file mode 100644 (file)
index 0000000..85e0a2b
--- /dev/null
@@ -0,0 +1,36 @@
+<svg xmlns="http://www.w3.org/2000/svg" width="300" height="150" viewBox="0 0 300 150">
+    <rect width="100%" height="100%" fill="red" />
+    <text id="text">
+        <tspan x="10" dy="1em">Foo</tspan>
+        <tspan x="10" dy="1.2em">Foo bar baz</tspan>
+    </text>
+    <defs>
+        <style type="text/css">
+        <![CDATA[
+            @font-face {
+                font-family: Ahem;
+                src: url(../../resources/Ahem.ttf);
+            }
+            text {
+                font-family: Ahem;
+                font-size: 40px;
+            }
+            ]]>
+        </style>
+        <script type="text/javascript">
+        <![CDATA[
+            if (window.testRunner)
+                testRunner.waitUntilDone();
+
+            setTimeout(function() {
+                // The point at 170,30 is contained within the text's bounding box
+                // but not in a painted part so the element should be the background.
+                document.elementFromPoint(170, 30).setAttribute("fill", "green");
+                document.getElementById("text").remove();
+                if (window.testRunner)
+                    testRunner.notifyDone();
+            }, 50);
+        ]]>
+        </script>
+    </defs>
+</svg>
\ No newline at end of file
diff --git a/LayoutTests/svg/hittest/text-with-text-node-and-content-elements-expected.svg b/LayoutTests/svg/hittest/text-with-text-node-and-content-elements-expected.svg
new file mode 100644 (file)
index 0000000..7232c4d
--- /dev/null
@@ -0,0 +1,3 @@
+<svg width="500" height="150" viewBox="0 0 1000 300" xmlns="http://www.w3.org/2000/svg">
+    <rect width="100%" height="100%" fill="green" />
+</svg>
diff --git a/LayoutTests/svg/hittest/text-with-text-node-and-content-elements.svg b/LayoutTests/svg/hittest/text-with-text-node-and-content-elements.svg
new file mode 100644 (file)
index 0000000..cd956a7
--- /dev/null
@@ -0,0 +1,36 @@
+<svg width="500" height="150" viewBox="0 0 1000 300" xmlns="http://www.w3.org/2000/svg" onclick="click(evt)">
+    <rect id="background" width="100%" height="100%" fill="red" />
+    <text id="text" x="10" y="1em">Foo <tspan id="tspan">Bar</tspan> Baz</text>
+    <defs>
+        <style type="text/css">
+        <![CDATA[
+            @font-face {
+                font-family: Ahem;
+                src: url(../../resources/Ahem.ttf);
+            }
+            text {
+                font-family: Ahem;
+                font-size: 40px;
+            }
+            ]]>
+        </style>
+        <script type="text/javascript">
+        <![CDATA[
+            if (window.testRunner)
+                testRunner.waitUntilDone();
+
+            var text = document.getElementById("text");
+            setTimeout(function() {
+                // The point at 15,15 is contained within the word "Foo", a text node.
+                // The point at 120,15 is contained within the word "Bar", a <tspan>.
+                if (document.elementFromPoint(15, 15) === text && document.elementFromPoint(110, 15) === document.getElementById("tspan")) {
+                    document.getElementById("background").setAttribute("fill", "green");
+                    text.remove();
+                    if (window.testRunner)
+                        testRunner.notifyDone();
+                }
+            }, 50);
+        ]]>
+        </script>
+    </defs>
+</svg>
diff --git a/LayoutTests/svg/hittest/text-with-text-node-only-expected.svg b/LayoutTests/svg/hittest/text-with-text-node-only-expected.svg
new file mode 100644 (file)
index 0000000..7232c4d
--- /dev/null
@@ -0,0 +1,3 @@
+<svg width="500" height="150" viewBox="0 0 1000 300" xmlns="http://www.w3.org/2000/svg">
+    <rect width="100%" height="100%" fill="green" />
+</svg>
diff --git a/LayoutTests/svg/hittest/text-with-text-node-only.svg b/LayoutTests/svg/hittest/text-with-text-node-only.svg
new file mode 100644 (file)
index 0000000..4ba9cbe
--- /dev/null
@@ -0,0 +1,28 @@
+<svg width="500" height="150" viewBox="0 0 1000 300" xmlns="http://www.w3.org/2000/svg">
+    <rect id="background" width="100%" height="100%" fill="red" />
+    <text id="text" x="10" y="1em">Foo</text>
+    <defs>
+        <style type="text/css">
+        <![CDATA[
+            @font-face {
+                font-family: Ahem;
+                src: url(../../resources/Ahem.ttf);
+            }
+            text {
+                font-family: Ahem;
+                font-size: 40px;
+            }
+            ]]>
+        </style>
+        <script type="text/javascript">
+        <![CDATA[
+            // The point at 15,15 is contained within the text's bounding box.
+            var text = document.getElementById("text");
+            if (document.elementFromPoint(15, 15) === text) {
+                document.getElementById("background").setAttribute("fill", "green");
+                text.remove();
+            }
+        ]]>
+        </script>
+    </defs>
+</svg>
diff --git a/LayoutTests/svg/hittest/text-with-text-path-expected.svg b/LayoutTests/svg/hittest/text-with-text-path-expected.svg
new file mode 100644 (file)
index 0000000..7f7e634
--- /dev/null
@@ -0,0 +1,3 @@
+<svg width="500" height="150" viewBox="0 0 1000 300" xmlns="http://www.w3.org/2000/svg">
+    <rect id="background" width="100%" height="100%" fill="green" />
+</svg>
diff --git a/LayoutTests/svg/hittest/text-with-text-path.svg b/LayoutTests/svg/hittest/text-with-text-path.svg
new file mode 100644 (file)
index 0000000..45920fe
--- /dev/null
@@ -0,0 +1,33 @@
+<svg width="500" height="150" viewBox="0 0 1000 300" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
+    <rect id="background" width="100%" height="100%" fill="red" />
+    <text id="text">
+        <textPath xlink:href="#path">This is text on a path</textPath>
+    </text>
+    <defs>
+        <path id="path"
+              d="M 100 200 
+              C 200 100 300   0 400 100
+              C 500 200 600 300 700 200
+              C 800 100 900 100 900 100" />
+        <style type="text/css">
+        <![CDATA[
+            @font-face {
+               font-family: Ahem;
+               src: url(../../resources/Ahem.ttf);
+            }
+            text {
+               font-family: Ahem;
+               font-size: 40px;
+            }
+        ]]>
+        </style>
+        <script type="text/javascript">
+        <![CDATA[
+            // The point at 150,75 is contained within the text's bounding box
+            // but not in a painted part so the element should be the background.
+            document.elementFromPoint(150, 75).setAttribute("fill", "green");
+            document.getElementById("text").remove();
+        ]]>
+        </script>
+    </defs>
+</svg>
index 6123d59..bbbf0fe 100644 (file)
@@ -1,3 +1,43 @@
+2015-11-04  Antoine Quint  <graouts@apple.com>
+
+        SVG: hit testing region for <text> elements is incorrect
+        https://bugs.webkit.org/show_bug.cgi?id=150838
+
+        Reviewed by Dean Jackson.
+
+        Hit testing for SVG <text> elements was using the same code as hit testing
+        for CSS-rendered elements. However, in SVG, text elements should only hit
+        test based on their character cells, not the rectangular bounds of the
+        element, see section 16.6 of the SVG 1.1 specification:
+        
+        http://www.w3.org/TR/SVG11/interact.html#PointerEventsProperty
+        
+        So we now hit test each SVGTextFragment of each SVGInlineTextBox
+        that is a child of an SVGRootInlineBox to correctly find whether the
+        provided HitTestLocation is contained within a character cell. 
+
+        Tests: svg/hittest/text-dominant-baseline-hanging.svg
+               svg/hittest/text-multiple-dx-values.svg
+               svg/hittest/text-with-multiple-tspans.svg
+               svg/hittest/text-with-text-node-and-content-elements.svg
+               svg/hittest/text-with-text-node-only.svg
+               svg/hittest/text-with-text-path.svg
+
+        * rendering/RootInlineBox.h:
+        Remove the final keyword since nodeAtPoint() may now be subclassed as
+        implemented in SVGRootInlineBox.
+        * rendering/svg/SVGInlineTextBox.cpp:
+        (WebCore::SVGInlineTextBox::nodeAtPoint):
+        Iterate over the SVGTextFragments to look for a fragment containing the
+        provided HitTestLocation.
+        * rendering/svg/SVGRootInlineBox.cpp:
+        (WebCore::SVGRootInlineBox::nodeAtPoint):
+        * rendering/svg/SVGRootInlineBox.h:
+        Override RootInlineBox::nodeAtPoint() to delegate hit testing to the
+        children inline boxes.
+
 2015-11-04  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [GStreamer] Use RunLoop::Timer for ready state timer in MediaPlayerPrivateGStreamer
index 52a3d51..9868b00 100644 (file)
@@ -119,7 +119,7 @@ public:
     virtual LayoutUnit lineHeight() const override final;
 
     virtual void paint(PaintInfo&, const LayoutPoint&, LayoutUnit lineTop, LayoutUnit lineBottom) override;
-    virtual bool nodeAtPoint(const HitTestRequest&, HitTestResult&, const HitTestLocation& locationInContainer, const LayoutPoint& accumulatedOffset, LayoutUnit lineTop, LayoutUnit lineBottom, HitTestAction) override final;
+    virtual bool nodeAtPoint(const HitTestRequest&, HitTestResult&, const HitTestLocation& locationInContainer, const LayoutPoint& accumulatedOffset, LayoutUnit lineTop, LayoutUnit lineBottom, HitTestAction) override;
 
     using InlineBox::hasSelectedChildren;
     using InlineBox::setHasSelectedChildren;
index ef8f18e..69f2960 100644 (file)
@@ -685,9 +685,25 @@ bool SVGInlineTextBox::nodeAtPoint(const HitTestRequest& request, HitTestResult&
             boxOrigin.moveBy(accumulatedOffset);
             FloatRect rect(boxOrigin, size());
             if (locationInContainer.intersects(rect)) {
-                renderer().updateHitTestResult(result, locationInContainer.point() - toLayoutSize(accumulatedOffset));
-                if (!result.addNodeToRectBasedTestResult(&renderer().textNode(), request, locationInContainer, rect))
-                    return true;
+
+                float scalingFactor = renderer().scalingFactor();
+                ASSERT(scalingFactor);
+                
+                float baseline = renderer().scaledFont().fontMetrics().floatAscent() / scalingFactor;
+
+                AffineTransform fragmentTransform;
+                for (auto& fragment : m_textFragments) {
+                    FloatQuad fragmentQuad(FloatRect(fragment.x, fragment.y - baseline, fragment.width, fragment.height));
+                    fragment.buildFragmentTransform(fragmentTransform);
+                    if (!fragmentTransform.isIdentity())
+                        fragmentQuad = fragmentTransform.mapQuad(fragmentQuad);
+                    
+                    if (fragmentQuad.containsPoint(locationInContainer.point())) {
+                        renderer().updateHitTestResult(result, locationInContainer.point() - toLayoutSize(accumulatedOffset));
+                        if (!result.addNodeToRectBasedTestResult(&renderer().textNode(), request, locationInContainer, rect))
+                            return true;
+                    }
+                }
              }
         }
     }
index 49bab64..be67d74 100644 (file)
@@ -209,6 +209,18 @@ InlineBox* SVGRootInlineBox::closestLeafChildForPosition(const LayoutPoint& poin
     return closestLeaf ? closestLeaf : lastLeaf;
 }
 
+bool SVGRootInlineBox::nodeAtPoint(const HitTestRequest& request, HitTestResult& result, const HitTestLocation& locationInContainer, const LayoutPoint& accumulatedOffset, LayoutUnit lineTop, LayoutUnit lineBottom, HitTestAction hitTestAction)
+{
+    for (InlineBox* leaf = firstLeafChild(); leaf; leaf = leaf->nextLeafChild()) {
+        if (!leaf->isSVGInlineTextBox())
+            continue;
+        if (leaf->nodeAtPoint(request, result, locationInContainer, accumulatedOffset, lineTop, lineBottom, hitTestAction))
+            return true;
+    }
+
+    return false;
+}
+
 static inline void swapItemsInLayoutAttributes(SVGTextLayoutAttributes* firstAttributes, SVGTextLayoutAttributes* lastAttributes, unsigned firstPosition, unsigned lastPosition)
 {
     SVGCharacterDataMap::iterator itFirst = firstAttributes->characterDataMap().find(firstPosition + 1);
index bca9ffd..8511648 100644 (file)
@@ -47,6 +47,8 @@ public:
 
     InlineBox* closestLeafChildForPosition(const LayoutPoint&);
 
+    virtual bool nodeAtPoint(const HitTestRequest&, HitTestResult&, const HitTestLocation& locationInContainer, const LayoutPoint& accumulatedOffset, LayoutUnit lineTop, LayoutUnit lineBottom, HitTestAction) override final;
+
 private:
     virtual bool isSVGRootInlineBox() const override { return true; }
     void reorderValueLists(Vector<SVGTextLayoutAttributes*>&);