Hit test SVG line boxes

Hit test SVG line boxes

This patch is a partial merge of http://wkrev.com/192020 by Antoine
Quint <graouts@apple.com>;.

Hit testing for SVG <text> elements was using the same code as hit
testing for regular HTML 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

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.

This patch is almost identical to the original patch but the tests have
been rewritten to be text-based reftests and to not rely on timeouts.

BUG=551058
Committed: https://crrev.com/e0b3579267bd648ded7ab13d2708296da5f92c02
Cr-Commit-Position: refs/heads/master@{#360197}

[pdr., 2015-11-17 06:06:37]

Description was changed from

==========
Hit test SVG line boxes

This patch is a partial merge of http://wkrev.com/192020 by Antoine
Quint <graouts@apple.com>.

Hit testing for SVG <text> elements was using the same code as hit
testing for regular HTML 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

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.

This patch is almost identical to the original patch but the tests have
been largely rewritten to be text-based reftests and to not rely on
timeouts.

BUG=551058
==========

to

==========
Hit test SVG line boxes

This patch is a partial merge of http://wkrev.com/192020 by Antoine
Quint <graouts@apple.com>.

Hit testing for SVG <text> elements was using the same code as hit
testing for regular HTML 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

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.

This patch is almost identical to the original patch but the tests have
been rewritten to be text-based reftests and to not rely on timeouts.

BUG=551058
==========

[pdr., 2015-11-17 06:08:08]

pdr@chromium.org changed reviewers:
+ fs@opera.com

[pdr., 2015-11-17 06:08:18]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-17 06:08:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1454623002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1454623002/20001

[commit-bot: I haz the power, 2015-11-17 07:26:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-17 07:26:23]

Dry run: This issue passed the CQ dry run.

[fs, 2015-11-17 09:47:49]

LGTM w/ questions, test comments and nits. But no ponies. Definitely no ponies.

https://codereview.chromium.org/1454623002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/hittest/text-dominant-baseline-hanging.svg
(right):

https://codereview.chromium.org/1454623002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/hittest/text-dominant-baseline-hanging.svg:7:
font-family: Ahem;
I guess we should use the ahem.js helper or there will be flakes
(eventually?)...

https://codereview.chromium.org/1454623002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/hittest/text-dominant-baseline-hanging.svg:22:
// The point at 15,15 is contained within the text's bounding box.
This goes for the comments in all tests pretty much...

"...is contained within the <object>'s bounding box." is a fairly confusing
statement for a CL that makes sure the "bounding box" is not what's used to test
for intersections. More accurate would be something like:

"...is contained within a glyph cell of <object>."

(Preferably even which glyph cell, but I suppose we can punt on that level of
fidelity...)

https://codereview.chromium.org/1454623002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/hittest/text-with-text-node-and-content-elements.svg
(right):

https://codereview.chromium.org/1454623002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/hittest/text-with-text-node-and-content-elements.svg:1:
<svg onload="runTest()" width="500" height="150" viewBox="0 0 1000 300"
xmlns="http://www.w3.org/2000/svg" onclick="click(evt)">
Drop onclick.

https://codereview.chromium.org/1454623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/line/SVGInlineTextBox.cpp
(right):

https://codereview.chromium.org/1454623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/line/SVGInlineTextBox.cpp:274:
FloatRect fragmentRect(fragment.x, fragment.y - baseline, fragment.width,
fragment.height);
You did not make this a (Float)Quad as in the original patch - was that
intentional, and are you planning on landing that later on? (It should make a
difference for cases like textPath and rotated glyphs.) Ripe for optimization
that one is though (w/ FloatQuad having a more expensive iset-test.)

[pdr., 2015-11-17 21:51:22]

The CQ bit was checked by pdr@chromium.org

[pdr., 2015-11-17 21:51:23]

Thanks!

https://codereview.chromium.org/1454623002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/hittest/text-dominant-baseline-hanging.svg
(right):

https://codereview.chromium.org/1454623002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/hittest/text-dominant-baseline-hanging.svg:7:
font-family: Ahem;
On 2015/11/17 at 09:47:49, fs wrote:
> I guess we should use the ahem.js helper or there will be flakes
(eventually?)...

Done

https://codereview.chromium.org/1454623002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/hittest/text-dominant-baseline-hanging.svg:22:
// The point at 15,15 is contained within the text's bounding box.
On 2015/11/17 at 09:47:49, fs wrote:
> This goes for the comments in all tests pretty much...
> 
> "...is contained within the <object>'s bounding box." is a fairly confusing
statement for a CL that makes sure the "bounding box" is not what's used to test
for intersections. More accurate would be something like:
> 
> "...is contained within a glyph cell of <object>."
> 
> (Preferably even which glyph cell, but I suppose we can punt on that level of
fidelity...)

Done

https://codereview.chromium.org/1454623002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/hittest/text-with-text-node-and-content-elements.svg
(right):

https://codereview.chromium.org/1454623002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/hittest/text-with-text-node-and-content-elements.svg:1:
<svg onload="runTest()" width="500" height="150" viewBox="0 0 1000 300"
xmlns="http://www.w3.org/2000/svg" onclick="click(evt)">
On 2015/11/17 at 09:47:49, fs wrote:
> Drop onclick.

Done

https://codereview.chromium.org/1454623002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/line/SVGInlineTextBox.cpp
(right):

https://codereview.chromium.org/1454623002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/line/SVGInlineTextBox.cpp:274:
FloatRect fragmentRect(fragment.x, fragment.y - baseline, fragment.width,
fragment.height);
On 2015/11/17 at 09:47:49, fs wrote:
> You did not make this a (Float)Quad as in the original patch - was that
intentional, and are you planning on landing that later on? (It should make a
difference for cases like textPath and rotated glyphs.) Ripe for optimization
that one is though (w/ FloatQuad having a more expensive iset-test.)

Good catch. This was a mistake.

I was planning on doing the trick where the point is transformed instead of the
rect and got bogged down in updating the tests. I've reverted this back to
FloatQuad as in the original patch, and added a new test, rotated-text.svg,
which failed without this fix and passes with it.

[pdr., 2015-11-17 21:51:23]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/1454623002/#ps60001
(title: "Add forgotten test. Sorry, test!")

[commit-bot: I haz the power, 2015-11-17 21:51:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1454623002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1454623002/60001

[commit-bot: I haz the power, 2015-11-17 23:14:16]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-17 23:15:58]

Patchset 4 (id:??) landed as
https://crrev.com/e0b3579267bd648ded7ab13d2708296da5f92c02
Cr-Commit-Position: refs/heads/master@{#360197}