[webkit-reviews] review granted: [Bug 85849] Incorrect rect-based hit-test result for culled-inline elements : [Attachment 145896] Patch

[bugzilla-daemon at webkit.org]

Levi Weintraub <leviw at chromium.org> has granted Raymes Khoury
<raymes at google.com>'s request for review:
Bug 85849: Incorrect rect-based hit-test result for culled-inline elements
https://bugs.webkit.org/show_bug.cgi?id=85849

Attachment 145896: Patch
https://bugs.webkit.org/attachment.cgi?id=145896&action=review

------- Additional Comments from Levi Weintraub <leviw at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=145896&action=review


I think this is an improvement over the current behavior. r=me if you change
the test as I've suggested.

> Source/WebCore/rendering/HitTestResult.cpp:662
> -    if (node->renderer()->isInline()) {
> +    bool regionFilled = rect.contains(rectForPoint(pointInContainer));
> +    // FIXME: This code (incorrectly) attempts to correct for culled inline
nodes. See https://bugs.webkit.org/show_bug.cgi?id=85849.
> +    if (node->renderer()->isInline() && !regionFilled) {

It's sad to see this code duplication :( Any reason the IntRect version can't
just call the FloatRect one?

> LayoutTests/fast/dom/nodesFromRect-culled-inlines.html:36
> +	 /* FIXME: This fails due to:
https://bugs.webkit.org/show_bug.cgi?id=88376
> +	 /* check(0, 98, 0, 1, 2, 0, [e.img, e.span]); */

This will probably just get forgotten if it's commented out. I'd much rather
see the test go in enabled but failing with a comment in the LayoutTests
ChangeLog about how its expected.