cc: Fix touch hit-testing for overlapping layers.

cc/trees/layer_tree_host_common.cc

 // Copyright 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/trees/layer_tree_host_common.h"
 
 #include <algorithm>
 
 #include "base/debug/trace_event.h"
 #include "cc/base/math_util.h"
@@ skipping 2359 matching lines @@
   }
 
   // This can potentially return NULL, which means the screen_space_point did
   // not successfully hit test any layers, not even the root layer.
   return found_layer;
 }
 
 LayerImpl* LayerTreeHostCommon::FindLayerThatIsHitByPointInTouchHandlerRegion(
     gfx::PointF screen_space_point,
     const LayerImplList& render_surface_layer_list) {
-  LayerImpl* found_layer = NULL;
+  // First find out which layer was hit from the saved list of visible layers
+  // in the most recent frame.
+  LayerImpl* layer_impl = LayerTreeHostCommon::FindLayerThatIsHitByPoint(
+      screen_space_point,
+      render_surface_layer_list);
 
-  typedef LayerIterator<LayerImpl,
-                        LayerImplList,
-                        RenderSurfaceImpl,
-                        LayerIteratorActions::FrontToBack> LayerIteratorType;
-  LayerIteratorType end = LayerIteratorType::End(&render_surface_layer_list);
-
-  for (LayerIteratorType
-           it = LayerIteratorType::Begin(&render_surface_layer_list);
-       it != end;
-       ++it) {
-    // We don't want to consider render_surfaces for hit testing.
-    if (!it.represents_itself())
-      continue;
-
-    LayerImpl* current_layer = (*it);
-
-    if (!LayerHasTouchEventHandlersAt(screen_space_point, current_layer))
-      continue;

[enne (OOO), 2013/10/18 19:18:38]

I would have expected the fix here to be turning this continue into a return
NULL statement.

-
-    found_layer = current_layer;
-    break;
+  // Walk up the hierarchy and look for a layer with a touch event handler

[enne (OOO), 2013/10/18 19:18:38]

Can you help me understand why you need to walk up the hierarchy? It seems like
there could be sibling layers with touch handlers that could also be hit by that
point.

[sadrul, 2013/10/18 19:57:44]

Consider this example: http://jsfiddle.net/zcjJe/3/

The child div (green) extends out of the parent div (yellow), and into the gray
box. The yellow div has a touch event handler. When tapping on the green box
that overlaps with the gray box, but is outside of the yellow box, still should
trigger the touch handler on the parent (since the JS event propagates through
the ancestor nodes). This is why it is necessary to go up the tree.

If a non-touch layer is on top of a touch layer, then touching the non-touch
layer should not trigger the touch-event handler on the touch-layer (except for
crbug.com/269598 cases, but we need to fix that properly anyway).

Does that address your concerns?

[enne (OOO), 2013/10/21 19:52:04]

I understand that you need to do *a* walk and not just stop at the first layer
you hit.  However, I'm a little confused about what walk needs to be done.  Can
you clarify?

You're talking about propagating JS events, but this can't be a DOM tree walk,
because the GraphicsLayers are constructed via the RenderLayer z-order lists and
a RenderLayer's real parent will be a sibling in the GraphicsLayer tree if its
parent is not a stacking context.

As per my previous comments, I was expecting this to be a draw-order walk, but
maybe that's not right either.

(Does cc::Layer need to know an event propagation layer id?)

[sadrul, 2013/10/21 21:36:46]

I had to look at the history of the change to figure out why this is done like
this: https://chromiumcodereview.appspot.com/11534024

From what I can see, this was done since at that time, the touch-event handling
regions were being set on the root layer. So each individual layer wouldn't have
the hit-region, and so it was necessary to walk up the tree.

Now that that is not the case (thanks to
https://chromiumcodereview.appspot.com/11534024 etc.), we probably don't need to
walk up the tree at all anymore. A quick test that removes the loop below
doesn't seem to break any of the cc-tests. I can explore this farther, but I
would rather change the runtime-behaviour in a subsequent CL, and keep this CL
to simply make sure we test the same code we run.
You are right. I wasn't making sense.

+  // region that the given point hits.
+  for (; layer_impl; layer_impl = layer_impl->parent()) {
+    if (LayerTreeHostCommon::LayerHasTouchEventHandlersAt(screen_space_point,
+                                                          layer_impl))
+      break;
   }
-
-  // This can potentially return NULL, which means the screen_space_point did
-  // not successfully hit test any layers, not even the root layer.
-  return found_layer;
+  return layer_impl;
 }
 
 bool LayerTreeHostCommon::LayerHasTouchEventHandlersAt(
     gfx::PointF screen_space_point,
     LayerImpl* layer_impl) {
   if (layer_impl->touch_event_handler_region().IsEmpty())
     return false;
 
   if (!PointHitsRegion(screen_space_point,
                        layer_impl->screen_space_transform(),
                        layer_impl->touch_event_handler_region(),
                        layer_impl->contents_scale_x(),
                        layer_impl->contents_scale_y()))
     return false;
 
   // At this point, we think the point does hit the touch event handler region
   // on the layer, but we need to walk up the parents to ensure that the layer
   // was not clipped in such a way that the hit point actually should not hit
   // the layer.
   if (PointIsClippedBySurfaceOrClipRect(screen_space_point, layer_impl))
     return false;
 
   return true;
 }
 }  // namespace cc