Bug 89990 – NodesFromRect doesn't work on SVG root elements

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Allan Sandfeld Jensen]

The SVG root elements implements nodeAtPoint in a way that doesn't support rect-based hit testing. 

Note that SVG elements in general doesn't support rect-based hit testing, but this bug only tracks the issue on the root element as the first step.

[Allan Sandfeld Jensen]

Created attachment 149555 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 149557 [details]
Patch

Fix typo in comment

[Philip Rogers]

Comment on attachment 149557 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149557&action=review

A few drive-by comments from a non-reviewer.

This is a cool API and I didn't even know we had it. Thanks for tackling it in SVG :)

It may be easiest for you to split the test into two (or more) tests: one just testing the SVG root hit testing with your new code (that passes), and a second (or more) testing elements in SVG that will be marked as fail until noteAtFloatPoint is updated. I think much more rigorous testing will be needed for the rect/circle/path/etc case, such as transform=translate or scale.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:-423
> -    // Note: For now, we're ignoring hits to border and padding for <svg>

Isn't this comment still valid?

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:439
> +    if (visibleToHitTesting() && hitTestAction == HitTestBlockBackground) {

This will be faster if the conditions are reversed.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:444
> +        LayoutRect boundsRect(accumulatedOffset + location(), size());

There are 4 (soon to be 5!) bounding rects of various types in SVG elements. Could "hitTestArea" be used here instead of boundsRect?

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:446
> +            updateHitTestResult(result, pointInBorderBox);

Is using pointInBorderBox here correct?

[Allan Sandfeld Jensen]

(In reply to comment #3)
> (From update of attachment 149557 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149557&action=review
> 
> A few drive-by comments from a non-reviewer.
> 
> This is a cool API and I didn't even know we had it. Thanks for tackling it in SVG :)
> 
> It may be easiest for you to split the test into two (or more) tests: one just testing the SVG root hit testing with your new code (that passes), and a second (or more) testing elements in SVG that will be marked as fail until noteAtFloatPoint is updated. I think much more rigorous testing will be needed for the rect/circle/path/etc case, such as transform=translate or scale.
> 
I do expect more tests to be added as SVG elements get better support. It just not that easy to make a rect-based hit test of SVG root-elements that wouldn't be affected by such improvements. 

> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:-423
> > -    // Note: For now, we're ignoring hits to border and padding for <svg>
> 
> Isn't this comment still valid?
> 
No, hits in the border and padding are with this change now considered hits on the SVG root element background. 

> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:444
> > +        LayoutRect boundsRect(accumulatedOffset + location(), size());
> 
> There are 4 (soon to be 5!) bounding rects of various types in SVG elements. Could "hitTestArea" be used here instead of boundsRect?
> 
The terminology used here is the same as what is used in other implementations of nodeAtPoint. It might not be suitable for real SVG elements, but the SVG root is treated more like a normal Render object than a SVG element.

> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:446
> > +            updateHitTestResult(result, pointInBorderBox);
> 
> Is using pointInBorderBox here correct?

Well, I think it is now consistent with other nodeAtPoint implementations. Is there any reason you think it should be different in this case?

[Allan Sandfeld Jensen]

(In reply to comment #4)
> (In reply to comment #3)
> > > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:446
> > > +            updateHitTestResult(result, pointInBorderBox);
> > 
> > Is using pointInBorderBox here correct?
> 
> Well, I think it is now consistent with other nodeAtPoint implementations. Is there any reason you think it should be different in this case?

I have checked. In WebCore the only place that uses the localPoint is renderWidget, which assumes it is offset from the border-box. It might not be the true local-point, but it is what is currently assumed.

[Antonio Gomes]

Comment on attachment 149557 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149557&action=review

looks good generally.

> Source/WebCore/ChangeLog:8
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: fast/dom/nodesFromRect-svg.html

please be more descriptive here.

>> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:439
>> +    if (visibleToHitTesting() && hitTestAction == HitTestBlockBackground) {
> 
> This will be faster if the conditions are reversed.

are you changing this?

[Allan Sandfeld Jensen]

(In reply to comment #6)
> (From update of attachment 149557 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149557&action=review
> 
> looks good generally.
> 
> > Source/WebCore/ChangeLog:8
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Test: fast/dom/nodesFromRect-svg.html
> 
> please be more descriptive here.
> 
Right.

> >> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:439
> >> +    if (visibleToHitTesting() && hitTestAction == HitTestBlockBackground) {
> > 
> > This will be faster if the conditions are reversed.
> 
> are you changing this?

Of course,  I just wanted to be sure the rest was okay.

[Allan Sandfeld Jensen]

Created attachment 151714 [details]
Patch

[Antonio Gomes]

Comment on attachment 151714 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151714&action=review

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:445
> +        if (pointInContainer.intersects(boundsRect)) {

I found it hard to read as a 'point' when it is actually an area.

[Allan Sandfeld Jensen]

(In reply to comment #9)
> (From update of attachment 151714 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151714&action=review
> 
> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:445
> > +        if (pointInContainer.intersects(boundsRect)) {
> 
> I found it hard to read as a 'point' when it is actually an area.

True, but it is consistent with current use in other nodeAtPoint implementations. We should probably updated them all collectively at some point to better names.

[WebKit Review Bot]

Comment on attachment 151714 [details]
Patch

Clearing flags on attachment: 151714

Committed r122339: <http://trac.webkit.org/changeset/122339>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antonio Gomes]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 151714 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=151714&action=review
> > 
> > > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:445
> > > +        if (pointInContainer.intersects(boundsRect)) {
> > 
> > I found it hard to read as a 'point' when it is actually an area.
> 
> True, but it is consistent with current use in other nodeAtPoint implementations. We should probably updated them all collectively at some point to better names.

lets file a bug about it?