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
Daniel Bates
2020-03-14 12:16:26 PDT
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! |