cc: Fix touch hit-testing for overlapping layers.

cc: Fix touch hit-testing for overlapping layers (in tests).

If a touch-layer is below a non-touch layer, and the two layers overlap,
then the overlapped region should not be treated as a touch-handling region.
This change updates FindLayerThatIsHitByPointInTouchHandlerRegion() to do the
right thing (which is currently used only in tests), and uses this function from
LayerTreeHostImpl::HaveTouchEventHandlersAt() (instead of having its own code).

BUG=none
R=enne@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230578

[Rick Byers, 2013-10-17 21:05:02]

On 2013/10/17 19:21:03, sadrul wrote:

I agree this seems like the right thing to do, and is in fact the original way
Yusuf wrote it: https://chromiumcodereview.appspot.com/11402002.  Looks like it
was changed in https://chromiumcodereview.appspot.com/11534024 to accommodate
the fact that webkit only put rects on the root layer (which was a concession at
the time to be consistent to iOS).  Now that that is fixed, I agree we should be
going back to the original design.  Thanks!

Note however that it may expose bugs (like http://crbug.com/269598) that would
have been previously masked.  I wish we had a good way of finding sites whose
behavior would change as a result.  Once I land crbug.com/257461 we'll at least
get one side of the equation (how many false positives we're getting in
practice).  We could in theory add a metric to track false negatives - not sure
it's worth it though.  For now we should just keep our eyes out for reports of
regressions here.

Rick

[sadrul, 2013-10-18 11:28:11]

On 2013/10/17 21:05:02, Rick Byers wrote:
> On 2013/10/17 19:21:03, sadrul wrote:
> 
> I agree this seems like the right thing to do, and is in fact the original way
> Yusuf wrote it: https://chromiumcodereview.appspot.com/11402002.  Looks like
it
> was changed in https://chromiumcodereview.appspot.com/11534024 to accommodate
> the fact that webkit only put rects on the root layer (which was a concession
at
> the time to be consistent to iOS).  Now that that is fixed, I agree we should
be
> going back to the original design.  Thanks!
> 
> Note however that it may expose bugs (like http://crbug.com/269598) that would
> have been previously masked.  I wish we had a good way of finding sites whose
> behavior would change as a result.  Once I land crbug.com/257461 we'll at
least
> get one side of the equation (how many false positives we're getting in
> practice).  We could in theory add a metric to track false negatives - not
sure
> it's worth it though.  For now we should just keep our eyes out for reports of
> regressions here.

Note that this doesn't actually change the current behaviour at runtime.
Instead, it is moving the code we currently use at runtime (from
HaveTouchEventHandlersAt) into the code we currently use only during tests
(FindLayerThatIsHitByPointInTouchHandlerRegion), so that we are testing the code
we actually use at runtime.

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

https://codereview.chromium.org/27718006/diff/10001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (left):

https://codereview.chromium.org/27718006/diff/10001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:2399: continue;
I would have expected the fix here to be turning this continue into a return
NULL statement.

https://codereview.chromium.org/27718006/diff/10001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/27718006/diff/10001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:2386: // Walk up the hierarchy and look for a
layer with a touch event handler
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:43]

https://codereview.chromium.org/27718006/diff/10001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/27718006/diff/10001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:2386: // Walk up the hierarchy and look for a
layer with a touch event handler
On 2013/10/18 19:18:38, enne wrote:
> 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.

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?

[sadrul, 2013-10-21 18:49:04]

ping

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

https://codereview.chromium.org/27718006/diff/10001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/27718006/diff/10001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:2386: // Walk up the hierarchy and look for a
layer with a touch event handler
On 2013/10/18 19:57:44, sadrul wrote:
> On 2013/10/18 19:18:38, enne wrote:
> > 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.
> 
> 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.

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]

https://codereview.chromium.org/27718006/diff/10001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/27718006/diff/10001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:2386: // Walk up the hierarchy and look for a
layer with a touch event handler
On 2013/10/21 19:52:04, enne wrote:
> On 2013/10/18 19:57:44, sadrul wrote:
> > On 2013/10/18 19:18:38, enne wrote:
> > > 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.
> > 
> > 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.
> 
> 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?

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'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.

You are right. I wasn't making sense.

> 
> 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?)

[enne (OOO), 2013-10-22 23:18:00]

On 2013/10/21 21:36:46, sadrul wrote:

> 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.

lgtm since this is ultimately just a test change.  Is there a bug for doing the
right thing with overlapping touch handlers in a tree?  Can you add a TODO with
that bug number as a comment in
LayerTreeHostCommon::FindLayerThatIsHitByPointInTouchHandlerRegion?

[sadrul, 2013-10-24 00:58:20]

On 2013/10/22 23:18:00, enne wrote:
> On 2013/10/21 21:36:46, sadrul wrote:
> 
> > 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.
> 
> lgtm since this is ultimately just a test change.  Is there a bug for doing
the
> right thing with overlapping touch handlers in a tree?  Can you add a TODO
with
> that bug number as a comment in
> LayerTreeHostCommon::FindLayerThatIsHitByPointInTouchHandlerRegion?

Done. Thanks!

[sadrul, 2013-10-24 01:03:06]

Committed patchset #4 manually as r230578 (presubmit successful).