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.
Created attachment 149555 [details] Patch
Created attachment 149557 [details] Patch Fix typo in comment
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?
(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?
(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 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?
(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.
Created attachment 151714 [details] Patch
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.
(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 on attachment 151714 [details] Patch Clearing flags on attachment: 151714 Committed r122339: <http://trac.webkit.org/changeset/122339>
All reviewed patches have been landed. Closing bug.
(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?