Bug 209110 - Separate out adding node to hit test result from determining whether hit testing should continue
Summary: Separate out adding node to hit test result from determining whether hit test...
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on: 209107
Blocks:
  Show dependency treegraph
 
Reported: 2020-03-14 12:16 PDT by Daniel Bates
Modified: 2020-03-14 21:12 PDT (History)
13 users (show)

See Also:


Attachments
Patch (25.59 KB, patch)
2020-03-14 13:48 PDT, Daniel Bates
beidson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.