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.
Created attachment 393591 [details] Patch
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.
(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.
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!