Remove mouse-related hit tests during a GestureTap

Remove mouse-related hit tests during a GestureTap

This further reduces the total number of hit tests in trivial tap scenarios
from 9 to 6.  It looks like all the work we do with un-hit-tested mouse
events is really mouse specific and probably not something we want in the
tap case.  So we can bump our emulation down to a lower level without
having to otherwise refactor the existing code.

This change is observable by web applications.  For example, if a
mousedown event handler modifies the DOM, then previously the mouseup and
click events would be sent to the new element under the point.  Now
all the events that happen as a result of a tap will be sent to the same
node.  Since they all happen at the same conceptual moment, this change
is probably a net improvement.  But we'll need to watch for any
scenarios that regress.

BUG=381728
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178098

[Rick Byers, 2014-06-25 18:14:53]

Zeeshan, when you're done with https://codereview.chromium.org/338543003, please
take a look at this CL which builds upon it further.

Rick

[Zeeshan Qureshi, 2014-06-26 06:06:53]

lgtm, so this would stop the dispatch if the target node is deleted on
mousedown?

[Rick Byers, 2014-06-26 21:05:28]

On 2014/06/26 06:06:53, Zeeshan Qureshi wrote:
> lgtm, so this would stop the dispatch if the target node is deleted on
> mousedown?

Good quesiton.  HitTestResult uses a RefPtr to hold the target Node now, so
(just like for targets removed during a touch event sequence) events can be
dispatched to an element that has been removed from the DOM.  This means, for
example, that a document-wide listener could see a mousedown without a matching
mouseup (where with a real mouse you'd hit-test again so get a mouseenter /
mouseup).

I've added a test to capture this behavior and ensure no-one adds code that
assumes the target of a mouse event is actually in the DOM.  Can you please take
a look at the new test?

Rick

[Zeeshan Qureshi, 2014-06-26 21:20:51]

Looks good.

https://codereview.chromium.org/353893002/diff/20001/LayoutTests/fast/events/...
File LayoutTests/fast/events/touch/gesture/gesture-tap-div-removed.html (right):

https://codereview.chromium.org/353893002/diff/20001/LayoutTests/fast/events/...
LayoutTests/fast/events/touch/gesture/gesture-tap-div-removed.html:57:
description("Verifies that deleting the element being tapped during the event
sequence doesn't cause any problems.  Succeeds if the expected events are
delivered to the div and we don't crash.");
It might be helpful to mention that we expect the events to be delivered but not
bubbled (aside from not crashing).

[Rick Byers, 2014-06-26 21:29:01]

Thanks.

https://codereview.chromium.org/353893002/diff/20001/LayoutTests/fast/events/...
File LayoutTests/fast/events/touch/gesture/gesture-tap-div-removed.html (right):

https://codereview.chromium.org/353893002/diff/20001/LayoutTests/fast/events/...
LayoutTests/fast/events/touch/gesture/gesture-tap-div-removed.html:57:
description("Verifies that deleting the element being tapped during the event
sequence doesn't cause any problems.  Succeeds if the expected events are
delivered to the div and we don't crash.");
On 2014/06/26 21:20:50, Zeeshan Qureshi wrote:
> It might be helpful to mention that we expect the events to be delivered but
not
> bubbled (aside from not crashing).

Done.

[Rick Byers, 2014-07-02 17:03:51]

I've been trying to polish this up while on vacation, but the interaction with
touch text selection on Android is quite tricky and apparently untested (at
least in blink tests).  I've got things mostly working now, except the OSK
doesn't come up when you tap on an input field - I'm not sure why yet.  ZeeshanQ
has experience with this and has volunteered to investigate from here.  Thanks
Zeeshan!

[Zeeshan Qureshi, 2014-07-05 03:16:25]

On 2014/07/02 17:03:51, Rick Byers wrote:
> I've been trying to polish this up while on vacation, but the interaction with
> touch text selection on Android is quite tricky and apparently untested (at
> least in blink tests).  I've got things mostly working now, except the OSK
> doesn't come up when you tap on an input field - I'm not sure why yet. 
ZeeshanQ
> has experience with this and has volunteered to investigate from here.  Thanks
> Zeeshan!

This was because we needed to provide the editable shadow div to set the focus
properly but touch adjustment was walking up the tree and returning the input
element associated with it. I've fixed the touch adjustment behaviour in a
separate CL (https://codereview.chromium.org/370843002/) and should be able
to land this unchanged after that.

[Rick Byers, 2014-07-14 17:35:57]

On 2014/07/05 03:16:25, Zeeshan Qureshi wrote:
> On 2014/07/02 17:03:51, Rick Byers wrote:
> > I've been trying to polish this up while on vacation, but the interaction
with
> > touch text selection on Android is quite tricky and apparently untested (at
> > least in blink tests).  I've got things mostly working now, except the OSK
> > doesn't come up when you tap on an input field - I'm not sure why yet. 
> ZeeshanQ
> > has experience with this and has volunteered to investigate from here. 
Thanks
> > Zeeshan!
> 
> This was because we needed to provide the editable shadow div to set the focus
> properly but touch adjustment was walking up the tree and returning the input
> element associated with it. I've fixed the touch adjustment behaviour in a
> separate CL (https://codereview.chromium.org/370843002/) and should be able
> to land this unchanged after that.

Thanks Zeeshan.  I've tested this out now and it seems to be working fine to me
too.  I've improved the test and made a couple other tweaks since you last
looked at this change.  Can you take another look please?

[Zeeshan Qureshi, 2014-07-14 19:08:42]

Looks good, one nit though:

https://codereview.chromium.org/353893002/diff/160001/LayoutTests/fast/events...
File LayoutTests/fast/events/touch/gesture/focus-selectionchange-on-tap.html
(right):

https://codereview.chromium.org/353893002/diff/160001/LayoutTests/fast/events...
LayoutTests/fast/events/touch/gesture/focus-selectionchange-on-tap.html:58: //
Run any pending tasks before resolving the promise.
Is this for the |selectionchange| event to be fired? Could we just thread that
as a promise as well?

[Rick Byers, 2014-07-14 19:40:00]

https://codereview.chromium.org/353893002/diff/160001/LayoutTests/fast/events...
File LayoutTests/fast/events/touch/gesture/focus-selectionchange-on-tap.html
(right):

https://codereview.chromium.org/353893002/diff/160001/LayoutTests/fast/events...
LayoutTests/fast/events/touch/gesture/focus-selectionchange-on-tap.html:58: //
Run any pending tasks before resolving the promise.
On 2014/07/14 19:08:42, Zeeshan Qureshi wrote:
> Is this for the |selectionchange| event to be fired? Could we just thread that
> as a promise as well?

Yes, and I think the focus event is also fired as a separate task too.  Plus we
want to catch anything else that (incorrectly) happens as a direct result of the
events we sent.  By posting a task to invoke 'resolve' we're effectively
threading all pending tasks into the promise here.  Make sense?

[Zeeshan Qureshi, 2014-07-14 19:46:05]

https://codereview.chromium.org/353893002/diff/160001/LayoutTests/fast/events...
File LayoutTests/fast/events/touch/gesture/focus-selectionchange-on-tap.html
(right):

https://codereview.chromium.org/353893002/diff/160001/LayoutTests/fast/events...
LayoutTests/fast/events/touch/gesture/focus-selectionchange-on-tap.html:58: //
Run any pending tasks before resolving the promise.
On 2014/07/14 19:39:59, Rick Byers wrote:
> On 2014/07/14 19:08:42, Zeeshan Qureshi wrote:
> > Is this for the |selectionchange| event to be fired? Could we just thread
that
> > as a promise as well?
> 
> Yes, and I think the focus event is also fired as a separate task too.  Plus
we
> want to catch anything else that (incorrectly) happens as a direct result of
the
> events we sent.  By posting a task to invoke 'resolve' we're effectively
> threading all pending tasks into the promise here.  Make sense?

Yep, I was thinking more of moving it to an explicit promise but we'd still need
this.

[Rick Byers, 2014-07-14 19:47:56]

The CQ bit was checked by rbyers@chromium.org

[commit-bot: I haz the power, 2014-07-14 19:48:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rbyers@chromium.org/353893002/160001

[commit-bot: I haz the power, 2014-07-14 19:52:29]

Change committed as 178098

[Zeeshan Qureshi, 2014-07-14 19:55:42]

On 2014/07/14 19:52:29, I haz the power (commit-bot) wrote:
> Change committed as 178098

That was the fastest commit I've seen!