Bug 89448 – Use HitTestPoint instead of LayoutPoint for nodeAtPoint

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Allan Sandfeld Jensen]

The HitTestPoint class introduced in bug #85965, is supposed to replace LayoutPoint as the point tested in nodeAtPoint. This is necessary to fix area-based hittesting on transformed elements, bug #85792.

[Allan Sandfeld Jensen]

Created attachment 148293 [details]
Patch

[Eric Seidel (no email)]

The answers to https://bugs.webkit.org/show_bug.cgi?id=85792#c27 will make reviewing this patch trivial. :)

You'll likely find eae, leviw and smfr to be good reviewers for these sorts of patches as they're familiar with both the subpixel rendering support and rendering tree transformations/hittesting.  I'm also happy to help with these reviews.

[Eric Seidel (no email)]

Comment on attachment 148293 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148293&action=review

Thank you again for hte long explanations both on the bugs, and over IRC.

I have real concern about the long-term readability of this code due to the really port class, function and variable namings.  These new objects are more than points, and naming them HitTestPoint is just confusing.

HitRegion would be one possible suggested replacement.

In eithe rcase, this patch is OK, but we really need to fix this code to read more clearly before we're done here.

> Source/WebCore/rendering/InlineFlowBox.cpp:969
> -    if (!overflowRect.intersects(result.rectForPoint(pointInContainer)))
> +    if (!pointInContainer.intersects(overflowRect))

So we're going to remove rectForPoint in this new model?

> Source/WebCore/rendering/RenderLayer.cpp:3631
> -    if (bgRect.intersects(hitTestArea) && isSelfPaintingLayer()) {
> +    if (bgRect.intersects(hitTestPoint) && isSelfPaintingLayer()) {

Seems like a sad name change. :(

> Source/WebCore/rendering/RenderLayer.h:118
> +    bool intersects(const HitTestPoint&);

Unclear to me if this what it's going to look like in the end.  It seems that clipRect and HitTestPoint both represent possibly non-rectilinear shapes.

> Source/WebCore/rendering/RenderLineBoxList.cpp:287
> +    LayoutPoint point = pointInContainer.point();

This and many similar renames could hav been avoided by changing the argument to something else and using a pointInContainer local which was actually a LayoutPOint.

[Allan Sandfeld Jensen]

(In reply to comment #3)
> I have real concern about the long-term readability of this code due to the really port class, function and variable namings.  These new objects are more than points, and naming them HitTestPoint is just confusing.
> 
> HitRegion would be one possible suggested replacement.
> 
The term point here is a point the same way a touch-point is, since touch points also has an area, but I can see how it might be confusing. 

Unfortunately Region is also a really overloaded name in WebKit rendering terms, so maybe HitTestArea or TouchPoint would work?

[Eric Seidel (no email)]

I guess I dont' like TouchPoint either, since it's not a point. :)

It would be nice if while you're in here you came up with a cohenent vision for what hit testing should look like.  it appears to me that he hit testing code hasn't had someone come and love it in a long time. :)

I would be fine with HitTestArea or HitArea.  Region is definitely an overloaded term, but I'm not sure that HitRegion makes that overloading any worse. :)

[WebKit Review Bot]

Comment on attachment 148293 [details]
Patch

Clearing flags on attachment: 148293

Committed r120824: <http://trac.webkit.org/changeset/120824>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.