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

Source/core/rendering/InlineFlowBox.cpp

 /*
  * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 1010 matching lines @@
 }
 
 bool InlineFlowBox::nodeAtPoint(const HitTestRequest& request, HitTestResult& result, const HitTestLocation& locationInContainer, const LayoutPoint& accumulatedOffset, LayoutUnit lineTop, LayoutUnit lineBottom)
 {
     LayoutRect overflowRect(visualOverflowRect(lineTop, lineBottom));
     flipForWritingMode(overflowRect);
     overflowRect.moveBy(accumulatedOffset);
     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;
+    // 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;
             }
-            // 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));
+
+            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();
     LayoutUnit minX = frameRect.x();
     LayoutUnit minY = frameRect.y();
     LayoutUnit width = frameRect.width();
     LayoutUnit height = frameRect.height();
 
     // Constrain our hit testing to the line top and bottom if necessary.
     bool noQuirksMode = renderer().document().inNoQuirksMode();
@@ skipping 275 matching lines @@
         ASSERT(child->prevOnLine() == prev);
         prev = child;
     }
     ASSERT(prev == m_lastChild);
 #endif
 }
 
 #endif
 
 } // namespace blink