Fallback to root layer if hit-testing does not hit anything in iframe

Source/core/paint/DeprecatedPaintLayer.cpp

Index: Source/core/paint/DeprecatedPaintLayer.cpp
diff --git a/Source/core/paint/DeprecatedPaintLayer.cpp b/Source/core/paint/DeprecatedPaintLayer.cpp
index e959ac12c71119335e8658c459a34e4855746927..ed879ff8d76b99b1e901582a5e70746d6c722a5c 100644
--- a/Source/core/paint/DeprecatedPaintLayer.cpp
+++ b/Source/core/paint/DeprecatedPaintLayer.cpp
@@ -1503,7 +1503,20 @@ bool DeprecatedPaintLayer::hitTest(HitTestResult& result)
         // should not use RequestType for it. Perhaps best for it to be done at a higher level. See
         // http://crbug.com/505825
         LayoutPoint hitPoint = hitTestLocation.point();
-        if (!request.isChildFrameHitTest() && ((request.active() || request.release()) || (request.move() && hitTestArea.contains(hitPoint.x(), hitPoint.y()))) && isRootLayer()) {
+        bool fallback = false;
+        if (!request.isChildFrameHitTest()) {
+            if (((request.active() || request.release()) || (request.move() && hitTestArea.contains(hitPoint.x(), hitPoint.y()))) && isRootLayer())
+                fallback = true;
+        } else {
+            // Falling back to iframe's root layer is more restrict:
+            // 1) It can't be rect based test. Rect based test is list based hit-testing, which should search
+            //    all the elements overlapping the test rect. If we set iframe is hit due to fallback, main frame
+            //    will stop hit testing search, and the result list will be incomplete.

[Rick Byers, 2015/08/17 15:18:29]

So what does this mean for a tap that occurs entirely inside an iframe but
outside of any layer?  It would be weird if tapping hit through to the parent
document but clicking didn't in that case.  I assume we wouldn't see a problem
today due to our point-based fallback (http://crbug.com/398914).  Please add a
test for this scenario so that if we ever fix that fallback we'll know we also
need to fix this case somehow.

[Yufeng Shen (Slow to review), 2015/08/18 16:05:21]

ah, right, if the hit-rect (for tap) is totally within the bounds of the iframe,
we should also do the fallback. I removed the condition of 
"!hitTestLocation.isRectBasedTest()", but use
"hitTestArea.contains(LayoutRect(hitRect)", which covers both the 
point based and rect based inclusion test. I dropped the "request.move()"
condition, not sure if that will cause any problem.

Note: point-based re-hit-test for adjusted position of gesture event
would not change the captured frame, I think. Because point-based
re-hit-test seems only calling hitTestResultInFrame(), which does not
set HitTestRequest::AllowChildFrameContent (hitTestResultAtPoint() does).
So if touch adjustment sets the hit-test node to be NodeA in DoucmentA,
point-based re-test would not descend into iframes but stick to DocumentA, 
though NodeA may be changed in the re-test.

Anyway, I added a the case of tap and click in the layout test.

+            // 2) The hitPoint must be within the visible rect of the iframe.
+            if (!hitTestLocation.isRectBasedTest() && (request.active() || request.release() || request.move()) && hitTestArea.contains(hitPoint.x(), hitPoint.y()) && isRootLayer())

[Rick Byers, 2015/08/17 15:18:29]

Can you just avoid checking the hit test flags (release, move active) entirely? 
Most hit tests should be one of those three, and it's really wrong that we pay
attention to the flags at all (again I believe Dave has a bug for this
somewhere?).

[Yufeng Shen (Slow to review), 2015/08/18 16:05:21]

Done.

+                fallback = true;
+        }

[Rick Byers, 2015/08/17 15:18:29]

I think you could more clearly separate the confusing mix of hacky heuristics
and reasonable behavior here if you reworked the condition as follows:

if (!insideLayer && isRootLayer()) {

  // If we didn't hit any layers but are still inside the document
  // bounds, then we should fallback to hitting the document.
  if (hitTestArea.contains(hitPoint.x(), hitPoint.y()) {

     // Except avoid early termination of rect-based hit test 
     // search in iframe case ...
    if (!(request.isChildFrameHitTest() &&
          hitTestLocation.isRectBasedTest()))
      fallback = true;

  // Mouse dragging outside the main document should also be
  // delivered to the document
  // TODO(dtapuska): Capture behavior inconsistent with iframes crbug.com/XXXX
  } else if ((request.active() || request.release()) &&
             !request.isChildFrameHitTest() {
    fallback = true;
  }
}

If this formulation works, then you can also get rid of the hack Majid added in
https://codereview.chromium.org/1210253003/diff/80001/Source/core/input/Event...
(but the argument that this mouse-drag specific fallback case belongs in
EventHandler, not DeprecatedPaintLayer is still true and maybe something you
want to try moving in a subsequent CL).

[Yufeng Shen (Slow to review), 2015/08/18 16:05:21]

done.
Majid's CL adds a test case where wheel event is outside even the window bounds,
which is relying on the "... else if ((request.active() ... " condition to
 capture but not the condition "if (hitTestArea.contains(LayoutRect(hitRect))"
which this CL is focusing on, so I can't really remove the active flag hack for
wheel event hit-test.

[Rick Byers, 2015/08/19 19:23:19]

I don't think part of the test is important, in fact I'd probably argue it's a
bug too (blink shouldn't be getting wheel events outside of the window entirely,
and if it does we probably shouldn't be scrolling).  So I think you can just
update the test to not expect any events in the case where the co-ordinates are
entirely outside the wheel.  Majid, do you agree?

[majidvp, 2015/08/19 21:02:30]

Actually, it is there for cases where the event is outside the root layer but
inside window which can happen. See for example http://output.jsbin.com/kapogi

There is an implicit assumption here that the hit-point is inside the window but
I think that may not be valid for child frame hit testing which is a bug.

[majidvp, 2015/08/20 16:29:35]

I took a second look and I think Rick is correct and that part of the test
(dispatch-mouse-events-to-window-always.html) which generates coordinate outside
window is a bug and can be removed.

+        if (fallback) {
             layoutObject()->updateHitTestResult(result, toLayoutView(layoutObject())->flipForWritingMode(hitTestLocation.point()));
             insideLayer = this;