Bug 209110

Summary: Separate out adding node to hit test result from determining whether hit testing should continue
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebCore Misc.Assignee: Daniel Bates <dbates>
Status: ASSIGNED ---    
Severity: Normal CC: ahmad.saleem792, beidson, dino, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, kondapallykalyan, mmaxfield, pdr, sabouhallawa, schenney, sergio
Priority: P2    
Version: WebKit Local Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=209112
https://bugs.webkit.org/show_bug.cgi?id=99656
https://bugs.webkit.org/show_bug.cgi?id=89990
Bug Depends on: 209107    
Bug Blocks:    
Attachments:
Description Flags
Patch beidson: review+

Description Daniel Bates 2020-03-14 12:16:26 PDT
Separate out adding node to hit test result from determining whether hit testing should continue.

Motivation:

HitTestResult::m_listBasedTestResult must stay in sync with HitTestResult::m_innerNode: if code sets an inner node then it MUST also add that node to the list based result set, if list based hit testing is being performed. Currently only convention keeps them in sync and you will see code like:

[[
if (visibleToHitTesting() && action == HitTestForeground && locationInContainer.intersects(boundsRect)) {
    updateHitTestResult(result, locationInContainer.point() - toLayoutSize(adjustedLocation)); <--- this updates HitTestResult::m_innerNode
    if (result.addNodeToListBasedTestResult(element(), request, locationInContainer, boundsRect) == HitTestProgress::Stop) <-- this HitTestResult::m_listBasedTestResult 
        return true;
}
]]
<https://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderBox.cpp?rev=258400#L1252>

scattered all over in the hit testing portion of the rendering code. My goal is to move result.addNodeToListBasedTestResult() inside updateHitTestResult() to ensure this invariant.
Comment 1 Daniel Bates 2020-03-14 13:48:58 PDT
Created attachment 393591 [details]
Patch
Comment 2 Daniel Bates 2020-03-14 15:50:10 PDT
Dang it! I hit a snag that may prevent me from moving addNodeToListBasedTestResult() into updateHitTestResult() in RenderSVGContainer::nodeAtFloatPoint():


[[
...       
    for (RenderObject* child = lastChild(); child; child = child->previousSibling()) {
        if (child->nodeAtFloatPoint(request, result, localPoint, hitTestAction)) {
            updateHitTestResult(result, LayoutPoint(localPoint));
            if (result.addNodeToListBasedTestResult(child->node(), request, localPoint) == HitTestProgress::Stop)
                return true;
        }
    }
...
]]
<https://trac.webkit.org/browser/trunk/Source/WebCore/rendering/svg/RenderSVGContainer.cpp?rev=258400#L174>


This code is not like other code. updateHitTestResult() will call RenderObject::updateHitTestResult(), which will set inner node to RenderObject::node(), but addNodeToListBasedTestResult() is adding child->node() to the set.
Comment 3 Daniel Bates 2020-03-14 21:12:40 PDT
(In reply to Daniel Bates from comment #2)
> Dang it! I hit a snag that may prevent me from moving
> addNodeToListBasedTestResult() into updateHitTestResult() in
> RenderSVGContainer::nodeAtFloatPoint():
> 
> 
> [[
> ...       
>     for (RenderObject* child = lastChild(); child; child =
> child->previousSibling()) {
>         if (child->nodeAtFloatPoint(request, result, localPoint,
> hitTestAction)) {
>             updateHitTestResult(result, LayoutPoint(localPoint));
>             if (result.addNodeToListBasedTestResult(child->node(), request,
> localPoint) == HitTestProgress::Stop)


From code inspection and thinking, I am leaning towards: it is a bug to pass child->node() here. The child should have added done that itself in its nodeAtFloatPoint.

Having said that, this bug likely has 0 visual manifestations because the node list is a ListHashSet. That means, if the child added itself as I expect with this design then adding the child again (a duplicate) will be ignored since ListHashSet guarantees unique elements.

As I type this, I am led to the conclusion that I am not snagged: it is ok to proceed and move the addNode...() code into updateHitTestResult(). Moreover, it will actually make the code correct. Anyway, I am going to try it BEFORE landing this patch just in case some misreading of the code reveals another problem. This way I avoid having to revert this patch.
>                 return true;
>         }
>     }
> ...
> ]]
> <https://trac.webkit.org/browser/trunk/Source/WebCore/rendering/svg/
> RenderSVGContainer.cpp?rev=258400#L174>
> 
> 
> This code is not like other code. updateHitTestResult() will call
> RenderObject::updateHitTestResult(), which will set inner node to
> RenderObject::node(), but addNodeToListBasedTestResult() is adding
> child->node() to the set.
Comment 4 Ahmad Saleem 2022-10-25 08:50:40 PDT
I checked via BugID and it seems this r+ patch didn't landed and it seems that old code is still present:

e.g.,

https://github.com/WebKit/WebKit/blob/2b778871c42d1772cfe9a97af335b3361d446439/Source/WebCore/rendering/svg/RenderSVGRoot.cpp#L504

Is this something needed anymore? Thanks!