Bug 89454 – Store hit-test rect in HitTestPoint as Rect

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

[Allan Sandfeld Jensen]

HitTestPoint represents a rectangle used for hit-testing, this rectangle is currently stored as a point plus padding values. The padding value are very rarely used though, while the rectangle they span is constantly used, which requires it to be recalculated all the time.

[Allan Sandfeld Jensen]

Created attachment 148300 [details]
Patch

[Antonio Gomes]

Comment on attachment 148300 [details]
Patch

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

Patch looks good. I have one question:

> Source/WebCore/page/EventHandler.cpp:2488
> +    IntRect touchRect(touchCenter - touchRadius, touchRadius + touchRadius);

I think the formula is a bit different:

 187 // Formula:
188 // x = p.x() - rightPadding
189 // y = p.y() - topPadding
190 // width = leftPadding + rightPadding + 1
191 // height = topPadding + bottomPadding + 1
192 inline IntRect HitTestPoint::rectForPoint(const LayoutPoint& point) const
193 {
194  return rectForPoint(point, m_topPadding, m_rightPadding, m_bottomPadding, m_leftPadding);
195 } 

does it return different rects now in some cases?

[Allan Sandfeld Jensen]

(In reply to comment #2)
> (From update of attachment 148300 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148300&action=review
> 
> Patch looks good. I have one question:
> 
> > Source/WebCore/page/EventHandler.cpp:2488
> > +    IntRect touchRect(touchCenter - touchRadius, touchRadius + touchRadius);
> 
> I think the formula is a bit different:
> 
>  187 // Formula:
> 188 // x = p.x() - rightPadding
> 189 // y = p.y() - topPadding
> 190 // width = leftPadding + rightPadding + 1
> 191 // height = topPadding + bottomPadding + 1
> 192 inline IntRect HitTestPoint::rectForPoint(const LayoutPoint& point) const
> 193 {
> 194  return rectForPoint(point, m_topPadding, m_rightPadding, m_bottomPadding, m_leftPadding);
> 195 } 
> 
> does it return different rects now in some cases?

In this particular case the radius has originally  been calculated as rect.width()/2 and rect.height()/2, so the most correct thing is to calculate back to a rectangle by just adding left and right padding, and top and bottom padding together.

The formula has not been changed anywhere else. 

Unrelated to this bug though, I do hope at some point to move away from the formula of adding 1 to width and height, since I don't see the center-point as a whole pixel, but as an infinitely small point. Once you add transformations it is just odd if the point itself changes size. But I have deliberately left this unchanged for now.

[Allan Sandfeld Jensen]

Created attachment 148766 [details]
Patch

Re-attaching the original patch to rerun EWS tests after prerequisite patch has landed.

[WebKit Review Bot]

Comment on attachment 148766 [details]
Patch

Attachment 148766 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13029106

New failing tests:
transforms/3d/hit-testing/perspective-clipped.html
fast/forms/search-transformed.html
fast/dom/elementFromPoint-relative-to-viewport.html
WebFrameTest.DivAutoZoomParamsTest

[WebKit Review Bot]

Created attachment 148785 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Allan Sandfeld Jensen]

Created attachment 148995 [details]
Patch

Move rect when moving point

[WebKit Review Bot]

Comment on attachment 148995 [details]
Patch

Attachment 148995 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13067013

New failing tests:
transforms/3d/hit-testing/perspective-clipped.html
fast/dom/elementFromPoint-relative-to-viewport.html

[WebKit Review Bot]

Created attachment 149001 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Allan Sandfeld Jensen]

Created attachment 149033 [details]
Patch

Avoid changing corner-case behavior for point hit-testing, and eliminate code complication.

[Allan Sandfeld Jensen]

(In reply to comment #8)
> (From update of attachment 148995 [details])
> Attachment 148995 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13067013
> 
> New failing tests:
> transforms/3d/hit-testing/perspective-clipped.html
> fast/dom/elementFromPoint-relative-to-viewport.html

I should note these two tests depends on the 1x1 expansion of a hit-test point when subpixel layout is enabled.

[Darin Adler]

Comment on attachment 149033 [details]
Patch

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

These changes all look OK, but things seem a bit mixed up. And there are coding style mistakes and typos that should be fixed before landing.

> Source/WebCore/rendering/HitTestResult.cpp:69
>      // If all padding values passed in are zero then it is not a rect based hit test.
>      m_isRectBased = topPadding || rightPadding || bottomPadding || leftPadding;
> +    m_boundingRect = rectForPoint(centerPoint, topPadding, rightPadding, bottomPadding, leftPadding);

Why are these not done as initializers rather than assignment? The code generated will be slightly inferior this way and it seems unnecessary.

> Source/WebCore/rendering/HitTestResult.cpp:105
> +template<typename SomeRect>
> +bool hitTestPointIntersects(const HitTestPoint& hitTestPoint, const SomeRect &rect)
> +{
> +    // FIXME: When the hit-test is not rect-based we should use rect.contains(m_point).
> +    // That does change some corner case tests though.
> +
> +    // First check if rect even intersects our bounding rect.
> +    if (!rect.intersects(hitTestPoint.boundingBox()))
> +        return false;
> +
> +    // FIXME: Implement quad-based interesection test to handle transformed hittest points.
> +    return true;
> +
> +}

The name SomeRect here is not conventional terminology. Maybe RectType would be a better name.

The & is positioned wrong and should be const SomeRect& rect.

The word intersection is spelled wrong as “interesection”. Since hittest is not a word it should be “hit test”, and it should not be “hit-test” above, and it should be “rect based”, not “rect-based”.

> Source/WebCore/rendering/HitTestResult.cpp:107
> +bool HitTestPoint::intersects(const LayoutRect &rect) const

Same formatting mistake.

> Source/WebCore/rendering/HitTestResult.cpp:109
> +    return hitTestPointIntersects<LayoutRect>(*this, rect);

There is no reason to specify the type here explicitly as <LayoutRect>. The compiler selects the appropriate type for the template automatically.

> Source/WebCore/rendering/HitTestResult.cpp:112
> +bool HitTestPoint::intersects(const FloatRect &rect) const

Ditto.

> Source/WebCore/rendering/HitTestResult.cpp:114
> +    return hitTestPointIntersects<FloatRect>(*this, rect);

Ditto.

> Source/WebCore/rendering/HitTestResult.h:63
> -    void setPoint(const LayoutPoint& p) { m_point = p; }
> +    void setPoint(const LayoutPoint& p) { m_boundingRect.move(roundedIntPoint(p) - roundedIntPoint(m_point)); m_point = p; }

Is it correct to round twice here rather than doing the math and then rounding? Is this covered by a test case? Perhaps this function is too long to have it inside the class definition. We could put it somewhere below.

> Source/WebCore/rendering/HitTestResult.h:81
>      LayoutPoint m_point;
>  
> -    int m_topPadding;
> -    int m_rightPadding;
> -    int m_bottomPadding;
> -    int m_leftPadding;
> +    IntRect m_boundingRect;

I think it’s confusing to have a LayoutPoint named m_point and a IntRect named m_boundingRect. Because of the strangely different naming and types it’s unclear how these two relate to each other. Further, it’s unclear how m_isRectBased works. Is one of these ignored completely in one of the two cases?

[Allan Sandfeld Jensen]

(In reply to comment #12)
> > Source/WebCore/rendering/HitTestResult.h:63
> > -    void setPoint(const LayoutPoint& p) { m_point = p; }
> > +    void setPoint(const LayoutPoint& p) { m_boundingRect.move(roundedIntPoint(p) - roundedIntPoint(m_point)); m_point = p; }
> 
> Is it correct to round twice here rather than doing the math and then rounding? Is this covered by a test case? Perhaps this function is too long to have it inside the class definition. We could put it somewhere below.
> 
Rounding twice is necessary to ensure the rect will be in the same place it would be if it had been declared in this spot with the original padding. Rounding the distance could generate different results when sub-pixel layout is enabled. Ideally the boundingBox will move to using layoutRect later, but that changes a few test cases, so I will do that in a separate bug and patch.

> > Source/WebCore/rendering/HitTestResult.h:81
> >      LayoutPoint m_point;
> >  
> > -    int m_topPadding;
> > -    int m_rightPadding;
> > -    int m_bottomPadding;
> > -    int m_leftPadding;
> > +    IntRect m_boundingRect;
> 
> I think it’s confusing to have a LayoutPoint named m_point and a IntRect named m_boundingRect. Because of the strangely different naming and types it’s unclear how these two relate to each other. Further, it’s unclear how m_isRectBased works. Is one of these ignored completely in one of the two cases?

The boundingRect is named this way because this is a patch preparing for the patch in bug #85792, where the area is stored in a FloatQuad and the bounding rect is the cached result of the FloatQuads bounding box.

m_isRectBased is currently needed instead of just using m_boundingRect.isEmpty() because the rect is expanded by 1x1 below and left of the m_point, so even point based hit testing has a non empty area..

[Allan Sandfeld Jensen]

Created attachment 149170 [details]
Patch

[WebKit Review Bot]

Comment on attachment 149170 [details]
Patch

Attachment 149170 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13085294

[Early Warning System Bot]

Comment on attachment 149170 [details]
Patch

Attachment 149170 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13033689

[Early Warning System Bot]

Comment on attachment 149170 [details]
Patch

Attachment 149170 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13084258

[Gustavo Noronha (kov)]

Comment on attachment 149170 [details]
Patch

Attachment 149170 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13032664

[Build Bot]

Comment on attachment 149170 [details]
Patch

Attachment 149170 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13085295

[Gyuyoung Kim]

Comment on attachment 149170 [details]
Patch

Attachment 149170 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13030617

[Build Bot]

Comment on attachment 149170 [details]
Patch

Attachment 149170 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13037598

[Allan Sandfeld Jensen]

Created attachment 149173 [details]
Patch

[WebKit Review Bot]

Comment on attachment 149173 [details]
Patch

Clearing flags on attachment: 149173

Committed r121096: <http://trac.webkit.org/changeset/121096>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.