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..8b7e768944965c3952a5ae3cc50b03327fc8eaa6 100644
--- a/Source/core/rendering/InlineFlowBox.cpp
+++ b/Source/core/rendering/InlineFlowBox.cpp
@@ -1028,42 +1028,48 @@ 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()) {
+    // Check all of children including culled inlines.

[pdr., 2014/12/10 07:16:19]

These comments helped me a lot, thank you :)

I tried to rephrase what the algorithm does in a simpler way. What do you think
about this comment instead?
// 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.

[Miyoung Shin(g), 2014/12/12 14:46:57]

thank you. it's perfect.
Should I replace all of comment ? or remain Behavior description ? 

[pdr., 2014/12/12 21:21:08]

Lets replace the entire comment.

+    // We need to account for culled inline parents of the hit-tested nodes,
+    // so that they may also get included in not only area-based hit-tests but also point-based ones.
+    // And we should consider of running the minimal loops for hit-test
+
+    // Behaviour of hit-test is as follows:
+    // - the main loop is the first loop and the sub loop is the second loop.
+    //   the main loop is for inline boxes and the sub loop is for render object of culled inline.
+    // - the main loop tries to hit-test all of inline box, if one of them is hit, then return.
+    //   although one of them is not hit, culled parent of inline box can be hit. so we enter the sub loop.
+    // - the sub loop tries to hit-test the culled parent of inline box.
+    //   However, if there is a sibling, we yield own parent to sibling becase we should check first the children before parent.
+    //   the direction of sibling depends on BIDI direction.
+    InlineBox* prev;
+    for (InlineBox* curr = lastChild(); curr; curr = prev) {
+        prev = curr->prevOnLine();
         if (curr->renderer().isText() || !curr->boxModelObject()->hasSelfPaintingLayer()) {

[pdr., 2014/12/10 07:16:19]

I think I understand what this is doing now. If you look at
InlineFlowBoxPainter::paint, this check is just mirroring how the inlines are
painted. I think we can replace this check with the following:

// Layers will handle hit testing themselves.
if (curr->boxModelObject() && curr->boxModelObject()->hasSelfPaintingLayer())
    continue;

[Miyoung Shin(g), 2014/12/12 14:46:57]

Ok, I will try.

-            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;
-            }
             if (curr->nodeAtPoint(request, result, locationInContainer, accumulatedOffset, lineTop, lineBottom)) {
                 renderer().updateHitTestResult(result, locationInContainer.point() - toLayoutSize(accumulatedOffset));
                 return true;
             }
+
+            RenderObject* culledParent = &curr->renderer();
+            if (!prev || curr->renderer() != prev->renderer()) {

[pdr., 2014/12/10 07:16:19]

Can you help me understand the "curr->renderer() != prev->renderer()" check?
What is this trying to check for? It may make your algorithm a little easier to
read if you refactor this like so:

// Short comment here explaining what the check is doing.
if (prev && curr->renderer() == prev->renderer())
    continue;

[Miyoung Shin(g), 2014/12/12 14:46:57]

current inlinebox and next inlinbox in the main loop can be the same renderer.
ex) 
<span>
text1 
text2
<span>
if text2 is the current inlinebox, then text1 would be the next inlinebox in the
main loop, and they are the same renderer.

"curr->renderer() != prev->renderer() is to check if renderer is changed. 
if not change(text2 -> text1), we should yield the hit-test to next inlinebox.
OK

[pdr., 2014/12/12 21:21:08]

Ah, I see. Should this check be moved up then, so it's the first statement in
the for loop?

[Miyoung Shin(g), 2014/12/14 06:54:56]

No, we should call inlinebox::nodeAtPoint for each inlinebox before checking
this condition.

+                while (culledParent) {
+                    if (culledParent->style()->isLeftToRightDirection()) {

[pdr., 2014/12/10 07:16:19]

Imagine a case where you have three culled inline siblings and you are trying to
hit test the middle one. Wont this check will prevent you from ever hit testing
the middle one? Can you add a test of this case? (or, just add a few more tests
to nodesFromRect-culled-inlines-between-silblings.html)

Additionally, the bidi check here doesn't make sense to me because the text
direction should be unrelated to hit test order. Can you help me understand a
case where we do need to consider the direction?

[Miyoung Shin(g), 2014/12/12 14:46:57]

Ah... you're right. I was back on track.
I should reconsider this case and I will add the test to
nodesFromRect-culled-inlines-between-silblings.html for this case.
It is for the element's direction, not for text.
we arrange inlineboxes due to bidi derection.

ex) in editing/selection/caret-at-bidi-boundary.html
<dt><span dir="rtl">12<b>קל43</b></span>ab</dt>

inlinebox flow is: 
   ab
-> 12
-> קל43 

if there isn't rtl, then the flow is,
   ab
-> קל43 
-> 12

in case of '12' with rtl, we should yield the hit-test to next sibling('קל43'),
so I need to consider the direction.

[pdr., 2014/12/12 21:21:08]

I still don't really understand why the order matters. Won't hitTestCulledInline
return true for one of them and false for the other? How did the old code handle
this properly without checking direction?

[Miyoung Shin(g), 2014/12/14 06:54:56]

I realized we don't need to care three culled inline siblings you said.
Culled inlines that needs the hit-test have a child inline box and we can do the
hit-test for them in next loop.
If culled inlines don't have a child inline box, we don't need the hit-test for
them, because they don't have any layout.
The old code has the problem that go up to the parent even if we should stop.
Because old code calls isDescendantOf for whether we should stop but it couldn't
check the some case as I explained in PPT.
Also isDescendantOf is a little expensive.

if we check a direction of current renderobject, we could know time when we stop
without isDescendantOf.

+                        if (culledParent->previousSibling())
+                            break;
+                    } else if (culledParent->nextSibling()) {
+                        break;
+                    }
+                    culledParent = culledParent->parent();
+                    if (culledParent == renderer())
+                        break;
+
+                    if (culledParent && culledParent->isRenderInline() && toRenderInline(culledParent)->hitTestCulledInline(request, result, locationInContainer, accumulatedOffset)) {

[pdr., 2014/12/10 07:16:19]

In the worst case, we'll iterate up the culled inline renderers until we hit the
parent renderer. Is it ever possible for culledParent to be null? We may just
want to add ASSERT(currentParent) right under "culledParent =
culledParent->parent();"

[Miyoung Shin(g), 2014/12/12 14:46:57]

I think, it's impossible to be null. I will add ASSERT(culledParent).

+                        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();