Bug 85849 - Incorrect rect-based hit-test result for culled-inline elements
: Incorrect rect-based hit-test result for culled-inline elements
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Raymes Khoury
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-07 17:53 PDT by Raymes Khoury
Modified: 2012-08-01 12:05 PDT (History)
13 users (show)

See Also:


Attachments
example (188 bytes, text/html)
2012-05-07 17:54 PDT, Raymes Khoury
no flags Details
Patch (5.51 KB, patch)
2012-05-14 09:57 PDT, Raymes Khoury
no flags Details | Formatted Diff | Diff
Patch (7.85 KB, patch)
2012-05-14 14:47 PDT, Raymes Khoury
no flags Details | Formatted Diff | Diff
Patch (7.93 KB, patch)
2012-05-14 17:06 PDT, Raymes Khoury
no flags Details | Formatted Diff | Diff
Patch (7.51 KB, patch)
2012-05-21 16:54 PDT, Raymes Khoury
no flags Details | Formatted Diff | Diff
Patch (6.57 KB, patch)
2012-05-21 16:59 PDT, Raymes Khoury
no flags Details | Formatted Diff | Diff
Patch (7.32 KB, patch)
2012-06-05 16:52 PDT, Raymes Khoury
no flags Details | Formatted Diff | Diff
Patch (7.49 KB, patch)
2012-06-06 09:24 PDT, Raymes Khoury
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Raymes Khoury 2012-05-07 17:53:12 PDT
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.
Comment 1 Raymes Khoury 2012-05-07 17:54:19 PDT
Created attachment 140640 [details]
example
Comment 2 Raymes Khoury 2012-05-11 14:51:32 PDT
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
Comment 3 Eric Seidel 2012-05-11 14:55:34 PDT
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!)
Comment 4 Raymes Khoury 2012-05-14 09:57:15 PDT
Created attachment 141745 [details]
Patch
Comment 5 Raymes Khoury 2012-05-14 10:01:18 PDT
Attached is a layout test which currently fails. Also a patch which "fixes" the problem.
Comment 6 Dave Hyatt 2012-05-14 11:52:53 PDT
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.
Comment 7 Raymes Khoury 2012-05-14 11:56:58 PDT
Is it possible and sufficient to check whether the node is a text node before examining parents?
Comment 8 Raymes Khoury 2012-05-14 14:47:59 PDT
Created attachment 141795 [details]
Patch
Comment 9 Ryosuke Niwa 2012-05-14 14:53:33 PDT
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.
Comment 10 Raymes Khoury 2012-05-14 14:59:14 PDT
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?
Comment 11 Raymes Khoury 2012-05-14 14:59:32 PDT
@rniwa: Thanks. I'm assuming the patch will need some further changes so I will include those changes in the next patch.
Comment 12 Darin Adler 2012-05-14 16:03:22 PDT
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.
Comment 13 Raymes Khoury 2012-05-14 17:06:29 PDT
Created attachment 141817 [details]
Patch
Comment 14 Raymes Khoury 2012-05-14 17:07:57 PDT
Thanks. Addressed #9 and #12.
Comment 15 Raymes Khoury 2012-05-16 08:52:57 PDT
Ping
Comment 16 Adam Barth 2012-05-16 20:09:22 PDT
> 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.
Comment 17 Dave Hyatt 2012-05-18 11:03:20 PDT
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.
Comment 18 Raymes Khoury 2012-05-21 16:54:08 PDT
Created attachment 143135 [details]
Patch
Comment 19 Raymes Khoury 2012-05-21 16:59:43 PDT
Created attachment 143139 [details]
Patch
Comment 20 Raymes Khoury 2012-05-21 17:03:18 PDT
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.
Comment 21 Raymes Khoury 2012-05-23 09:04:32 PDT
Ping
Comment 22 Dave Hyatt 2012-05-24 13:02:56 PDT
(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.
Comment 23 Dave Hyatt 2012-05-24 13:04:07 PDT
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>
Comment 24 Raymes Khoury 2012-05-24 16:40:10 PDT
(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.
Comment 25 Levi Weintraub 2012-06-04 16:41:43 PDT
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.
Comment 26 Raymes Khoury 2012-06-05 16:52:17 PDT
Created attachment 145896 [details]
Patch
Comment 27 Raymes Khoury 2012-06-05 16:54:08 PDT
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.
Comment 28 Levi Weintraub 2012-06-05 17:07:14 PDT
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.
Comment 29 Raymes Khoury 2012-06-06 09:24:55 PDT
Created attachment 146044 [details]
Patch
Comment 30 Levi Weintraub 2012-06-07 09:53:25 PDT
Comment on attachment 146044 [details]
Patch

LGTM.
Comment 31 WebKit Review Bot 2012-06-07 10:02:34 PDT
Comment on attachment 146044 [details]
Patch

Clearing flags on attachment: 146044

Committed r119733: <http://trac.webkit.org/changeset/119733>
Comment 32 WebKit Review Bot 2012-06-07 10:02:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 33 Antonio Gomes 2012-08-01 12:02:25 PDT
please next time CC me on bugs changing the rect-based hit testing system. Thanks
Comment 34 Adam Barth 2012-08-01 12:05:11 PDT
> 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