We need to account for culled inline parents of the hit-tested nodes.(Reland)

Source/core/rendering/InlineFlowBox.cpp

Index: Source/core/rendering/InlineFlowBox.cpp
diff --git a/Source/core/rendering/InlineFlowBox.cpp b/Source/core/rendering/InlineFlowBox.cpp
index 10918f9e4886f4037c2b3e46bb0a46be044f9f21..8792bdbd2da3b9ab59a00a4ff9b54300a3da0256 100644
--- a/Source/core/rendering/InlineFlowBox.cpp
+++ b/Source/core/rendering/InlineFlowBox.cpp
@@ -1028,42 +1028,46 @@ bool InlineFlowBox::nodeAtPoint(const HitTestRequest& request, HitTestResult& re
     if (!locationInContainer.intersects(overflowRect))
         return false;
 
-    // Check children first.
-    // We need to account for culled inline parents of the hit-tested nodes, so that they may also get included in area-based hit-tests.
-    RenderObject* culledParent = 0;
-    for (InlineBox* curr = lastChild(); curr; curr = curr->prevOnLine()) {
-        if (curr->renderer().isText() || !curr->boxModelObject()->hasSelfPaintingLayer()) {
-            RenderObject* newParent = 0;
-            // Culled parents are only relevant for area-based hit-tests, so ignore it in point-based ones.
-            if (locationInContainer.isRectBasedTest()) {
-                newParent = curr->renderer().parent();
-                if (newParent == renderer())
-                    newParent = 0;
-            }
-            // Check the culled parent after all its children have been checked, to do this we wait until
-            // we are about to test an element with a different parent.
-            if (newParent != culledParent) {
-                if (!newParent || !newParent->isDescendantOf(culledParent)) {
-                    while (culledParent && culledParent != renderer() && culledParent != newParent) {
-                        if (culledParent->isRenderInline() && toRenderInline(culledParent)->hitTestCulledInline(request, result, locationInContainer, accumulatedOffset))
-                            return true;
-                        culledParent = culledParent->parent();
-                    }
-                }
-                culledParent = newParent;
+    // We need to hit test both our inline children (InlineBoxes) and culled inlines
+    // (RenderObjects). We check our inlines in the same order as line layout but
+    // for each inline we additionally need to hit test its culled inline parents.
+    // While hit testing culled inline parents, we can stop once we reach
+    // a non-inline parent or a culled inline associated with a different inline box.
+    InlineBox* prev;
+    for (InlineBox* curr = lastChild(); curr; curr = prev) {

[pdr., 2014/12/14 22:06:48]

This code is so much easier to understand than the old code!

+        prev = curr->prevOnLine();
+        // Layers will handle hit testing themselves.
+        if (curr->boxModelObject() && curr->boxModelObject()->hasSelfPaintingLayer())
+            continue;
+
+        if (curr->nodeAtPoint(request, result, locationInContainer, accumulatedOffset, lineTop, lineBottom)) {
+            renderer().updateHitTestResult(result, locationInContainer.point() - toLayoutSize(accumulatedOffset));
+            return true;
+        }
+
+        RenderObject* culledParent = &curr->renderer();
+        // If current inlinebox's renderer and previous inlinebox's renderer is same,

[pdr., 2014/12/14 22:06:48]

Minor nit:
"// If [the] current inlinebox's renderer and [the] previous inlinebox's
renderer"
"// [are] the same, we should yield the hit=test to [the] previous inlinebox."

[Miyoung Shin(g), 2014/12/15 13:01:15]

Thanks.

+        // we should yield the hit-test to previous inlinebox.
+        if (prev && curr->renderer() == prev->renderer())
+            continue;
+
+        while (true) {
+            if (culledParent->style()->isLeftToRightDirection()) {

[pdr., 2014/12/14 22:06:48]

I still don't understand the direction part here. Maybe a test would make this
clearer--can you add a test for this (maybe
nodesFromRect-culled-inlines-between-silblings-bidi.html) that fails without the
isLeftToRightDirection check but passes with it?

[pdr., 2014/12/14 22:06:48]

I still don't understand the direction part here. Maybe a test would make this
clearer--can you add a test for this (maybe
nodesFromRect-culled-inlines-between-silblings-bidi.html) that fails without the
isLeftToRightDirection check but passes with it?

[Miyoung Shin(g), 2014/12/15 13:01:15]

OK. I will add nodesFromRect-culled-inlines-between-silblings-bidi.html

+                if (culledParent->previousSibling())

[pdr., 2014/12/14 22:06:48]

This essentially checks if we can early-out because the inlinebox descendant of
the previous sibling will handle walking up this chain of culled inlines. This
is based on the assumption that the previous sibling must have inline box
descendants which seems to be true most of the time. Unfortunately, in the case
of text nodes we can have a previous sibling that does not have an inline box
descendant. You can see this in the js fiddle I posted before:
http://jsfiddle.net/progers/1pk6tqb8/1/embedded/result. Please add a testcase
for this.

We could solve this by adding a check such as:
if (culledParent->previousSibling() &&
culledParent->previousSibling()->hasInlineBoxes())
But I wasn't able to find a function to do that. RenderObject::childrenInline or
RenderObject::slowFirstChild() might work here.

Another option might be to add an additional check like this:
if (culledParent->previousSibling() && prev &&
prev->renderer().isDescendantOf(culledParent->previousSibling()))

[Miyoung Shin(g), 2014/12/15 13:01:15]

I'm sorry. I think I get it now.
I thought that event-on-culled_inline.html covers the test case you posted in
the js fiddle.
I realized that I have to add a space in my test case.

before :
<span id="clickme2"><em id="em">
after:
<span id="clickme2"> <em id="em">

After changing TC, I found my problem and I caught up what you say.
And event-on-culled_inline.html will cover your tc after updating the patch.
Thank you. The second option works well.

+                    break;
+            } else if (culledParent->nextSibling()) {
+                break;
             }
-            if (curr->nodeAtPoint(request, result, locationInContainer, accumulatedOffset, lineTop, lineBottom)) {
-                renderer().updateHitTestResult(result, locationInContainer.point() - toLayoutSize(accumulatedOffset));
+
+            culledParent = culledParent->parent();
+            ASSERT(culledParent);
+            if (culledParent == renderer())
+                break;
+
+            if (culledParent->isRenderInline() && toRenderInline(culledParent)->hitTestCulledInline(request, result, locationInContainer, accumulatedOffset))
                 return true;
-            }
         }
     }
-    // Check any culled ancestor of the final children tested.
-    while (culledParent && culledParent != renderer()) {
-        if (culledParent->isRenderInline() && toRenderInline(culledParent)->hitTestCulledInline(request, result, locationInContainer, accumulatedOffset))
-            return true;
-        culledParent = culledParent->parent();
-    }
 
     // Now check ourselves. Pixel snap hit testing.
     LayoutRect frameRect = roundedFrameRect();