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+

Daniel Bates
Reported 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.
Attachments
Patch (25.59 KB, patch)
2020-03-14 13:48 PDT, Daniel Bates 		beidson: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Daniel Bates
Comment 1 2020-03-14 13:48:58 PDT
Created attachment 393591 [details] Patch
Daniel Bates
Comment 2 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.
Daniel Bates
Comment 3 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.
Ahmad Saleem
Comment 4 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!
Note You need to log in before you can comment on or make changes to this bug.