| Summary: | SVG: hit testing region for <text> elements is incorrect | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||
| Component: | SVG | Assignee: | Antoine Quint <graouts> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | commit-queue, graouts, jonlee, sabouhallawa, tobi, webkit-bug-importer, zimmermann | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| URL: | http://codepen.io/TobiReif/pen/ghELn | ||||||||
| Attachments: |
|
||||||||
|
Description
Antoine Quint
2015-11-03 07:51:33 PST
Created attachment 264692 [details]
Patch
1. What exactly are you trying to fix? Can you please attach a test case which shows a real life bug that you want to fix? 2. You consider the current SVG <text> element hit testing is wrong. What specs did you follow for fixing this bug? 3. What is exactly the definition of the painted contents of a SVG <text> element? Do you mean it the area which is not covered by text fragments? Where can I get the definition? 4. I think this fix is not following the standard hit testing. Every element has a rectangular bounds. When running the hit testing we scan the tree from top to bottom as long as the element bounds contain the hit point. In your patch, you are bypassing the text element itself as if it does not exist. So if non of the text fragments matches the hit point, the parent of the <text> element will be the matched element. 5. How about this example: <svg xmlns="http://www.w3.org/2000/svg"> <rect id="background" width="100%" height="100%" fill="red" /> <text id="text" font-family="Verdana" font-size="42.5"> <tspan x="10" dy="1em">Foo</tspan> <tspan x="10" dy="1em">Foo bar baz</tspan> </text> <script> var text = document.getElementById("text"); var rect = text.getBoundingClientRect(); alert("width =" + rect.width + ", " + "height =" + rect.height); </script> </svg> If I run it, it alerts the width and the height of the <text> element bounding box including the non painted contents area. So why if I call elementFromPoint() with a point inside text.getBoundingClientRect() but not covered by a text fragment I get the red <rect> element? 6. You mentioned that SVG hit testing for SVG <text> element is using the same code as hit testing for CSS-rendered elements. How about < foreignObject>? Why does this example behalf different from your tests? <svg xmlns="http://www.w3.org/2000/svg"> <rect id="background" width="100%" height="100%" fill="red" /> <foreignObject width="100%" height="100%" requiredExtensions="http://www.w3.org/1999/xhtml"> <body xmlns="http://www.w3.org/1999/xhtml"> <p style="font-family:Verdana; font-size:42.5px;">Foo</p> <p style="font-family:Verdana; font-size:42.5px;">Foo bar baz</p> </body> </foreignObject> </svg> 7. How about the <text> element position list? In this example: <svg xmlns="http://www.w3.org/2000/svg" width="300" height="150" viewBox="0 0 300 150"> <rect id="background" width="100%" height="100%" fill="red" /> <text id="text" font-family="Verdana" font-size="24"> <tspan x="10 50 100" y="1em">Foo</tspan> </text> </svg> Three InlineTextBoxes will be created; one box for each character. Why if I ask for a point between two characters , I get the red <rect>? (In reply to comment #3) > 1. What exactly are you trying to fix? Can you please attach a test case > which shows a real life bug that you want to fix? This bug originated from an internal report, where the original test case is http://codepen.io/TobiReif/pen/ghELn. I've added this URL to this bug's URL field. > 2. You consider the current SVG <text> element hit testing is wrong. What > specs did you follow for fixing this bug? SVG spec section 16.6 (http://www.w3.org/TR/SVG11/interact.html#PointerEventsProperty) which starts with "For text elements, hit-testing is performed on a character cell basis" and goes on to explain how different values of pointer-events will affect hit-testing. > 3. What is exactly the definition of the painted contents of a SVG <text> > element? Do you mean it the area which is not covered by text fragments? > Where can I get the definition? See above. > 4. I think this fix is not following the standard hit testing. Every element > has a rectangular bounds. When running the hit testing we scan the tree from > top to bottom as long as the element bounds contain the hit point. In your > patch, you are bypassing the text element itself as if it does not exist. So > if non of the text fragments matches the hit point, the parent of the <text> > element will be the matched element. I haven't tested <text> element without a <tspan> or <textPath>, I'll have to get back to you tomorrow about the behaviour in this case. But at any rate, your comment that "this fix is not following the standard hit testing" is correct, I believe SVG <text> and its descendants break from the CSS / box model norm. > 5. How about this example: > > <svg xmlns="http://www.w3.org/2000/svg"> > <rect id="background" width="100%" height="100%" fill="red" /> > <text id="text" font-family="Verdana" font-size="42.5"> > <tspan x="10" dy="1em">Foo</tspan> > <tspan x="10" dy="1em">Foo bar baz</tspan> > </text> > <script> > var text = document.getElementById("text"); > var rect = text.getBoundingClientRect(); > alert("width =" + rect.width + ", " + "height =" + rect.height); > </script> > </svg> > > If I run it, it alerts the width and the height of the <text> element > bounding box including the non painted contents area. So why if I call > elementFromPoint() with a point inside text.getBoundingClientRect() but not > covered by a text fragment I get the red <rect> element? I'd fully expect that, yes. If you hit-test anything outside of the character bounds then the <text> element wouldn't hit-test positively. > 6. You mentioned that SVG hit testing for SVG <text> element is using the > same code as hit testing for CSS-rendered elements. How about < > foreignObject>? Why does this example behalf different from your tests? > > <svg xmlns="http://www.w3.org/2000/svg"> > <rect id="background" width="100%" height="100%" fill="red" /> > <foreignObject width="100%" height="100%" > requiredExtensions="http://www.w3.org/1999/xhtml"> > <body xmlns="http://www.w3.org/1999/xhtml"> > <p style="font-family:Verdana; font-size:42.5px;">Foo</p> > <p style="font-family:Verdana; font-size:42.5px;">Foo bar baz</p> > </body> > </foreignObject> > </svg> That would be because none of those elements are "graphics elements" as defined in http://www.w3.org/TR/SVG11/intro.html#TermGraphicsElement and would thus fall under the CSS hit-testing model which would work with the element's bounds rather than the character bounds. > 7. How about the <text> element position list? In this example: > > <svg xmlns="http://www.w3.org/2000/svg" width="300" height="150" viewBox="0 > 0 300 150"> > <rect id="background" width="100%" height="100%" fill="red" /> > <text id="text" font-family="Verdana" font-size="24"> > <tspan x="10 50 100" y="1em">Foo</tspan> > </text> > </svg> > > Three InlineTextBoxes will be created; one box for each character. Why if I > ask for a point between two characters , I get the red <rect>? Yes, I believe that would be the correct behaviour. Comment on attachment 264692 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264692&action=review r=me assuming more tests > Source/WebCore/ChangeLog:10 > + for CSS-rendered elements. However, in SVG, text elements should only hit > + test based on their painted content, not the rectangular bounds of the You might want to be more specific, in that it is the bounds of each painted character. > Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:690 > + ASSERT(scalingFactor); I see this assertion in more places in this file. I wonder if it is useful. It seems it is initialized to 1: m_scalingFactor(1) > Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:697 > + unsigned textFragmentsSize = m_textFragments.size(); > + for (unsigned i = 0; i < textFragmentsSize; ++i) { > + const SVGTextFragment& fragment = m_textFragments.at(i); Since you only use i to access the array, you can make this a modern for loop. for (auto& fragment : m_textFragments) { > Source/WebCore/rendering/svg/SVGRootInlineBox.cpp:214 > + for (InlineBox* leaf = firstLeafChild(); leaf; leaf = leaf->nextLeafChild()) { I wish we had an iterator for this, so we could use a modern loop. > LayoutTests/ChangeLog:11 > + * svg/hittest/text-with-multiple-tspans-expected.svg: Added. > + * svg/hittest/text-with-multiple-tspans.svg: Added. > + > + Testing the hit testing region of an SVG <text> element with two <tspan> children. As Said mentioned, there probably should be tests for a bare <text> > LayoutTests/svg/hittest/text-with-multiple-tspans.svg:10 > + // The point at 125,30 is contained within the text's bounding box > + // but not in a painted part so the element should be the background. Is this guaranteed even if the font is missing? > LayoutTests/svg/hittest/text-with-text-path.svg:16 > + // 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. Ditto. I wonder if Verdana is always available. Maybe you should use the Ahem font via a @font-face? *** Bug 150274 has been marked as a duplicate of this bug. *** (In reply to comment #4) > (In reply to comment #3) > > 4. I think this fix is not following the standard hit testing. Every element > > has a rectangular bounds. When running the hit testing we scan the tree from > > top to bottom as long as the element bounds contain the hit point. In your > > patch, you are bypassing the text element itself as if it does not exist. So > > if non of the text fragments matches the hit point, the parent of the <text> > > element will be the matched element. > > I haven't tested <text> element without a <tspan> or <textPath>, I'll have > to get back to you tomorrow about the behaviour in this case. But at any > rate, your comment that "this fix is not following the standard hit testing" > is correct, I believe SVG <text> and its descendants break from the CSS / > box model norm. I've tested this and the patch correctly handles <text> elements with only text nodes as content, or mixing text nodes and content elements. I'll add tests for that purpose. > > 7. How about the <text> element position list? In this example: > > > > <svg xmlns="http://www.w3.org/2000/svg" width="300" height="150" viewBox="0 > > 0 300 150"> > > <rect id="background" width="100%" height="100%" fill="red" /> > > <text id="text" font-family="Verdana" font-size="24"> > > <tspan x="10 50 100" y="1em">Foo</tspan> > > </text> > > </svg> > > > > Three InlineTextBoxes will be created; one box for each character. Why if I > > ask for a point between two characters , I get the red <rect>? > > Yes, I believe that would be the correct behaviour. I also tested this case and the patch makes it work as you outlined, clicking outside of the character cells hit the background <rect>. Created attachment 264781 [details]
Patch for landing
(In reply to comment #5) > Comment on attachment 264692 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=264692&action=review > > r=me assuming more tests > > > Source/WebCore/ChangeLog:10 > > + for CSS-rendered elements. However, in SVG, text elements should only hit > > + test based on their painted content, not the rectangular bounds of the > > You might want to be more specific, in that it is the bounds of each painted > character. I've updated the ChangeLog when landing, with reference to the SVG 1.1 spec. > > Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:690 > > + ASSERT(scalingFactor); > > I see this assertion in more places in this file. I wonder if it is useful. > > It seems it is initialized to 1: m_scalingFactor(1) In doubt, I left it in. > > Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:697 > > + unsigned textFragmentsSize = m_textFragments.size(); > > + for (unsigned i = 0; i < textFragmentsSize; ++i) { > > + const SVGTextFragment& fragment = m_textFragments.at(i); > > Since you only use i to access the array, you can make this a modern for > loop. > > for (auto& fragment : m_textFragments) { Changed in landing patch. > > LayoutTests/ChangeLog:11 > > + * svg/hittest/text-with-multiple-tspans-expected.svg: Added. > > + * svg/hittest/text-with-multiple-tspans.svg: Added. > > + > > + Testing the hit testing region of an SVG <text> element with two <tspan> children. > > As Said mentioned, there probably should be tests for a bare <text> I've added tests for the cases where we have a <text> element with a single text node, one with a single text node and dominant-baseline="hanging", one with a text node, a <tspan> and another text node, and one with multiple "dx" values. > > LayoutTests/svg/hittest/text-with-multiple-tspans.svg:10 > > + // The point at 125,30 is contained within the text's bounding box > > + // but not in a painted part so the element should be the background. > > Is this guaranteed even if the font is missing? I updated to use the Ahem font for all new tests. > > LayoutTests/svg/hittest/text-with-text-path.svg:16 > > + // 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. > > Ditto. I wonder if Verdana is always available. In that case it would have worked regardless of the font since the text is laid out on a path that should simply not draw text in that location. Comment on attachment 264781 [details] Patch for landing Clearing flags on attachment: 264781 Committed r192020: <http://trac.webkit.org/changeset/192020> All reviewed patches have been landed. Closing bug. |