Bug 85849 – Incorrect rect-based hit-test result for culled-inline elements

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Raymes Khoury]

Attached is an example which is causing incorrect rect-based hit tests for us. When performing a rect-based hit-test anywhere on the <img>, the enclosing span is also included in the hit-test result and this seems incorrect to me. The code causing this to occur was added as part of a CL implementing line box culling: https://bugs.webkit.org/show_bug.cgi?id=57916. In particular the following lines of HitTestResult.cpp:

 if (node->renderer()->isInline()) {
       for (RenderObject* curr = node->renderer()->parent(); curr; curr = curr->parent()) {
           if (!curr->isRenderInline())
               break;
           
           // We need to make sure the nodes for culled inlines get included.
           RenderInline* currInline = toRenderInline(curr);
           if (currInline->alwaysCreateLineBoxes())
               break;
           
           if (currInline->visibleToHitTesting() && currInline->node())
               mutableRectBasedTestResult().add(currInline->node()->shadowAncestorNode());
       }
   }

I'm not sure why this code is necessary or how to properly fix the observed problem.

[Raymes Khoury]

Created attachment 140640 [details]
example

[Raymes Khoury]

The fix is needed for pepper flash (which uses rect-based hit tests for checking whether the flash settings dialogue is visible to the user). This is blocking http://code.google.com/p/chromium/issues/detail?id=127185

[Eric Seidel (no email)]

My understanding is that rect-based hit testing was added by the Qt folks for their mobile port.  I don't know if any other port uses it.  (But my understanding could be wrong!)

[Raymes Khoury]

Created attachment 141745 [details]
Patch

[Raymes Khoury]

Attached is a layout test which currently fails. Also a patch which "fixes" the problem.

[Dave Hyatt]

Comment on attachment 141745 [details]
Patch

Although blindly adding is wrong, not adding the inlines at all is also wrong. In the common case of text inside culled line boxes, the right thing is happening currently, and this patch will break text in order to get replaced elements like images (that jut out of the inline) working.

[Raymes Khoury]

Is it possible and sufficient to check whether the node is a text node before examining parents?

[Raymes Khoury]

Created attachment 141795 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 141795 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        Modified code which blindly adds culled inlines to rect-based hit-test
> +        results so that it only does so for text nodes.

This should appear below "Reviewed by" line followed by a blank line.
And this line should be replaced by the actual bug title.

> LayoutTests/ChangeLog:7
> +        Added 2 tests for rect-based hit-tests which test:
> +        1) The case when a replaced element within a culled inline is in the
> +        hit-test result.
> +        2) The case when a text node within a culled inline is in the hit-test
> +        result.

Again, this long description should appear below "Reviewed by" line followed by a blank line,
and this line should be replaced by the bug title.

[Raymes Khoury]

Uploaded a second patch which only adds parent elements if the the node is text, as well as another layout test.

Why is it that when adding a text node, we want to add ancestors which are culled inlines? My best guess is it's because if we are doing a hit-test which includes text then it will usually cover a portion of the background of that text. But that would lead to two questions: 1) Why do we only do this for culled inlines? 2) Why do we examine all ancestors that are culled inlines, and not just the parent?

[Raymes Khoury]

@rniwa: Thanks. I'm assuming the patch will need some further changes so I will include those changes in the next patch.

[Darin Adler]

Comment on attachment 141795 [details]
Patch

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

> Source/WebCore/rendering/HitTestResult.cpp:585
> +    if (node->renderer()->isInline() && node->renderer()->isText()) {

Why keep the check for isInline? All text will be inline.

[Raymes Khoury]

Created attachment 141817 [details]
Patch

[Raymes Khoury]

Thanks. Addressed #9 and #12.

[Raymes Khoury]

Ping

[Adam Barth]

> Ping

Unfortunately, I don't know enough about hit testing to review this patch.  It's probably better for Eric or dhyatt to review your patch.

[Dave Hyatt]

Comment on attachment 141817 [details]
Patch

This patch is closer, but now it will do the wrong thing with images/plug-ins, etc. when the mouse actually is inside where the enclosing inlines would be.

This is a performance optimization essentially to avoid creating line boxes for inline flows that are detected to match some enclosing parent. What this means is the answer you give on a hit test has to be identical to the answer that would have been given if the performance optimization were not in effect.

What is checked in currently is wrong, and all the patches that have been produced are also wrong. However, the latest patch is probably the "least wrong" of all of them, so in that sense it's an improvement. I don't mind landing this change, but it needs to be accompanied by a follow-up bug and FIXMEs in the code, so that we can actually fix this the right way in the future.

The right way to fix the bug is that you have to actually determine where the culled inline is (we have code elsewhere that does this sort of thing, e.g.,the rect/quad collection methods in RenderInline) and do an accurate test for intersection.

[Raymes Khoury]

Created attachment 143135 [details]
Patch

[Raymes Khoury]

Created attachment 143139 [details]
Patch

[Raymes Khoury]

Thanks David. 

Note that rect-based hit-testing should halt/return when a node fills the entire hit-test region. Child nodes are searched before parents. Hence the code here which adds culled inlines to the test result is wrong for several reasons:
1) It assumes ancestor culled-inline nodes satisfy the hit-test condition (that is, they are greater than or equal to the size of the node being added to the hit-test result) which AFAICT is not necessarily true.
2) It adds all culled-inline ancestors, even when one of these might fill the entire hit-test region and hence stop the hit-test.

I updated the patch to partially address (2). If the node being added to the hit-test result fills the entire hit-test region, then we don't add any culled inline ancestors, otherwise we add them all. This fixes the particular case that we care about for pepper flash.

I briefly looked into exactly what would be required for a real fix but I'm still not certain where the change should be made and it seems it would require some refactoring effort. It's probably better left for someone who better understands the hit-test code.

[Raymes Khoury]

Ping

[Dave Hyatt]

(In reply to comment #20)
> Thanks David. 
> 
> Note that rect-based hit-testing should halt/return when a node fills the entire hit-test region. Child nodes are searched before parents. Hence the code here which adds culled inlines to the test result is wrong for several reasons:
> 1) It assumes ancestor culled-inline nodes satisfy the hit-test condition (that is, they are greater than or equal to the size of the node being added to the hit-test result) which AFAICT is not necessarily true.

<!doctype html>
<span style="font-size:36px;background-color:yellow;"><img height=100 width=100 style="background-color:black"></span>

I put backgrounds in so you can see that an ancestor inline can in fact occupy an area that does not overlap with the image. It is possible for the hit test region to contain both nodes.

> 2) It adds all culled-inline ancestors, even when one of these might fill the entire hit-test region and hence stop the hit-test.
> 

This is true. In fact if you're culled by definition, any ancestors will occupy the same range, so at most you would only be including one culled inline ancestor, and the only reason you would include it would be because of the descender. To find out the dimensions of this descent you could just crawl up to the first ancestor that is not culled (typically the root line box) and then compare the placement/dimensions of that box.

[Dave Hyatt]

The ascent can stick out as well of course, e.g.,


<!doctype html>
<span style="font-size:36px;background-color:yellow;"><img height=20 width=20 style="background-color:black"></span>

[Raymes Khoury]

(In reply to comment #22)
> (In reply to comment #20)
> > Thanks David. 
> > 
> > Note that rect-based hit-testing should halt/return when a node fills the entire hit-test region. Child nodes are searched before parents. Hence the code here which adds culled inlines to the test result is wrong for several reasons:
> > 1) It assumes ancestor culled-inline nodes satisfy the hit-test condition (that is, they are greater than or equal to the size of the node being added to the hit-test result) which AFAICT is not necessarily true.
> 
> <!doctype html>
> <span style="font-size:36px;background-color:yellow;"><img height=100 width=100 style="background-color:black"></span>
> 
> I put backgrounds in so you can see that an ancestor inline can in fact occupy an area that does not overlap with the image. It is possible for the hit test region to contain both nodes.

I agree. But in this case, the hit-test will only contain (<span>, <img>) if the hit-test rect extends over the yellow region. If the black region completely covers the hit-test rect then only the <img> will be returned in the results. 
When the <span> is culled (e.g. by removing the bg color) the current behavior is for it to be added to the test result as well (regardless of the hit-test rect) which is wrong. My patch fixes this particular case.

> 
> > 2) It adds all culled-inline ancestors, even when one of these might fill the entire hit-test region and hence stop the hit-test.
> > 
> 
> This is true. In fact if you're culled by definition, any ancestors will occupy the same range, so at most you would only be including one culled inline ancestor, and the only reason you would include it would be because of the descender. To find out the dimensions of this descent you could just crawl up to the first ancestor that is not culled (typically the root line box) and then compare the placement/dimensions of that box.

I believe you are saying that a full fix to the problem could work as follows:
1) First check if the node currently being added to the test result completely fills the hit-test rect. If it does, then we are done (this is my current patch).
2) If not, traverse up the tree to the first non-culled ancestor.
3) Check whether that ancestor intersects the hit-test region. If so, add the deepest culled inline to the hit-test result and we are done. (I guess do the intersection testing using the approach here http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderLineBoxList.cpp&exact_package=chromium&q=RenderLineBoxList%20hitTest&type=cs&l=277)

However, I'm worried about the side-effects of implementing (3) in addNodeToRectBasedHitTestResult. For example, addNodeToRectBasedHitTestResult is supposed to return false if the node being added fully intersects the hit test rectangle (signalling the stop condition). This assumption would be altered if it returned false when the node /or one of its ancestors/ filled the region. There are other subtleties that I'm unsure of.

My preference would be to commit my current patch and defer the full fix to someone who understands the implications. But let me know.

[Levi Weintraub]

It sounds like we can agree that this is an improvement over the current logic. I'd love to see the cases that Hyatt mentioned also put in as test cases and a bug filed to track the work towards getting this code fully fixed.

[Raymes Khoury]

Created attachment 145896 [details]
Patch

[Raymes Khoury]

I integrated in the test cases suggested by Hyatt. There is one case that currently fails due to the bug, which I have commented out. I filed https://bugs.webkit.org/show_bug.cgi?id=88376 for a full fix to the problem.

[Levi Weintraub]

Comment on attachment 145896 [details]
Patch

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.

[Raymes Khoury]

Created attachment 146044 [details]
Patch

[Levi Weintraub]

Comment on attachment 146044 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 146044 [details]
Patch

Clearing flags on attachment: 146044

Committed r119733: <http://trac.webkit.org/changeset/119733>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antonio Gomes]

please next time CC me on bugs changing the rect-based hit testing system. Thanks

[Adam Barth]

> please next time CC me on bugs changing the rect-based hit testing system. Thanks

You might be able to configure a watchlist regular expression to get CCed automagically:

http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/watchlist