Bug 89990

Summary: NodesFromRect doesn't work on SVG root elements
Product: WebKit Reporter: Allan Sandfeld Jensen <allan.jensen>
Component: Layout and RenderingAssignee: Allan Sandfeld Jensen <allan.jensen>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, morrita, pdr, tonikitoo, webkit.review.bot, zimmermann
Priority: P2    
Version: 420+   
Hardware: All   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=209110
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Allan Sandfeld Jensen 2012-06-26 10:19:56 PDT
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.
Comment 1 Allan Sandfeld Jensen 2012-06-26 10:24:55 PDT
Created attachment 149555 [details]
Patch
Comment 2 Allan Sandfeld Jensen 2012-06-26 10:28:25 PDT
Created attachment 149557 [details]
Patch

Fix typo in comment
Comment 3 Philip Rogers 2012-06-26 21:33:37 PDT
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?
Comment 4 Allan Sandfeld Jensen 2012-06-27 02:15:49 PDT
(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?
Comment 5 Allan Sandfeld Jensen 2012-06-27 05:10:01 PDT
(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.
Comment 6 Antonio Gomes 2012-07-11 08:40:15 PDT
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?
Comment 7 Allan Sandfeld Jensen 2012-07-11 08:48:43 PDT
(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.
Comment 8 Allan Sandfeld Jensen 2012-07-11 09:16:32 PDT
Created attachment 151714 [details]
Patch
Comment 9 Antonio Gomes 2012-07-11 09:56:38 PDT
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.
Comment 10 Allan Sandfeld Jensen 2012-07-11 10:01:32 PDT
(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.
Comment 11 WebKit Review Bot 2012-07-11 10:09:19 PDT
Comment on attachment 151714 [details]
Patch

Clearing flags on attachment: 151714

Committed r122339: <http://trac.webkit.org/changeset/122339>
Comment 12 WebKit Review Bot 2012-07-11 10:09:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Antonio Gomes 2012-07-11 10:17:58 PDT
(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?