[Android] Always precede Tap gesture events with TapDown

[Android] Always precede Tap gesture events with TapDown

The gesture detector generates TapDown events before Tap events, but
there there remain cases where synthetic Tap events are not preceded
by such TapDown events. Insert the expected TapDown for these synthetic
cases, also validating this event ordering in the
GestureEventStreamValidator.

A consistent ordering here provides the render process, and any downstream
gesture listener, more context to reason about and react to the gesture
event stream.

BUG=418402
Committed: https://crrev.com/ed1dafa7db5fcd150a757d1237c83a81666ec47f
Cr-Commit-Position: refs/heads/master@{#304611}

[jdduke (slow), 2014-11-14 23:23:00]

jdduke@chromium.org changed reviewers:
+ rbyers@chromium.org

[jdduke (slow), 2014-11-14 23:23:00]

rbyers@: PTAL, per offline discussion.

Tim, can you think of any cases where Aura might insert a synthetic tap gesture
outside of the normal gesture detection pipeline? I also wonder if we should be
more strict with longpress ordering, but I'd like to keep this change focused on
tap and we can address other events later.

[jdduke (slow), 2014-11-14 23:23:40]

Oops, actually adding tdresser@.

Tim, can you think of any cases where Aura might insert a synthetic tap gesture
outside of the normal gesture detection pipeline? I also wonder if we should be
more strict with longpress ordering, but I'd like to keep this change focused on
tap and we can address other events later.

[tdresser, 2014-11-17 18:49:57]

On 2014/11/14 23:23:40, jdduke wrote:
> Oops, actually adding tdresser@.
> 
> Tim, can you think of any cases where Aura might insert a synthetic tap
gesture
> outside of the normal gesture detection pipeline? I also wonder if we should
be
> more strict with longpress ordering, but I'd like to keep this change focused
on
> tap and we can address other events later.

I'm a little surprised that it isn't breaking in a bunch of tests, but I can't
think of any cases where that _should_ happen in Aura. 

I wouldn't be surprised if there were some bugs which allow it to happen, but
hopefully this will eventually help us find them!

[Rick Byers, 2014-11-17 22:55:24]

LGTM, thanks!

https://codereview.chromium.org/724313003/diff/1/content/browser/android/cont...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/724313003/diff/1/content/browser/android/cont...
content/browser/android/content_view_core_impl.cc:1034:
WebInputEvent::GestureTapDown, time_ms, x, y);
What about GestureShowPress?  Shouldn't Tap always be preceded by that too?

[jdduke (slow), 2014-11-17 23:45:21]

https://codereview.chromium.org/724313003/diff/1/content/browser/android/cont...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/724313003/diff/1/content/browser/android/cont...
content/browser/android/content_view_core_impl.cc:1034:
WebInputEvent::GestureTapDown, time_ms, x, y);
On 2014/11/17 22:55:24, Rick Byers wrote:
> What about GestureShowPress?  Shouldn't Tap always be preceded by that too?

Hmm, you tell me :) I believe we insert ShowPress before Tap in the
TouchDispositionGestureFilter, but I don't think we validate the ordering
anywhere, and I'm not sure if we should (we can if it's worth it). I guess I
don't understand the side effects of TapDown+ShowPress well enough to know which
is critical. Inserting ShowPress before Tap has always felt weird =/ (i.e., why
can't Blink do it implicitly?).

[Zeeshan Qureshi, 2014-11-17 23:49:06]

zeeshanq@chromium.org changed reviewers:
+ zeeshanq@chromium.org

[Zeeshan Qureshi, 2014-11-17 23:49:06]

https://codereview.chromium.org/724313003/diff/1/content/browser/android/cont...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/724313003/diff/1/content/browser/android/cont...
content/browser/android/content_view_core_impl.cc:1034:
WebInputEvent::GestureTapDown, time_ms, x, y);
> Inserting ShowPress before Tap has always felt weird =/ (i.e., why
> can't Blink do it implicitly?).

I believe Tap is only fired when the finger lifts? We could hypothetically use
TapDown to generate ShowPress though. I'm not sure what the browser-side
contract is for the two.

[jdduke (slow), 2014-11-17 23:52:20]

https://codereview.chromium.org/724313003/diff/1/content/browser/android/cont...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/724313003/diff/1/content/browser/android/cont...
content/browser/android/content_view_core_impl.cc:1034:
WebInputEvent::GestureTapDown, time_ms, x, y);
On 2014/11/17 23:49:06, Zeeshan Qureshi wrote:
> > Inserting ShowPress before Tap has always felt weird =/ (i.e., why
> > can't Blink do it implicitly?).
> 
> I believe Tap is only fired when the finger lifts? We could hypothetically use
> TapDown to generate ShowPress though. I'm not sure what the browser-side
> contract is for the two.

Well, I think we still want the delay before activating ShowPress (to
accommodate scrolling). For gesture detection, we'll either get a showpress or a
scroll, never both, and if the press event doesn't fire before the tap event, we
insert the press event.

[Rick Byers, 2014-11-18 00:06:02]

On 2014/11/17 23:45:21, jdduke wrote:
>
https://codereview.chromium.org/724313003/diff/1/content/browser/android/cont...
> File content/browser/android/content_view_core_impl.cc (right):
> 
>
https://codereview.chromium.org/724313003/diff/1/content/browser/android/cont...
> content/browser/android/content_view_core_impl.cc:1034:
> WebInputEvent::GestureTapDown, time_ms, x, y);
> On 2014/11/17 22:55:24, Rick Byers wrote:
> > What about GestureShowPress?  Shouldn't Tap always be preceded by that too?
> 
> Hmm, you tell me :) I believe we insert ShowPress before Tap in the
> TouchDispositionGestureFilter, but I don't think we validate the ordering
> anywhere, and I'm not sure if we should (we can if it's worth it). I guess I
> don't understand the side effects of TapDown+ShowPress well enough to know
which
> is critical. Inserting ShowPress before Tap has always felt weird =/ (i.e.,
why
> can't Blink do it implicitly?).

In general we want chromium in charge of the ShowPress timeout (I think), which
is why blink doesn't synthesize them generally.  But we could make them optional
and have blink add them when they're missing (but it might be better to minimize
the number of places tracking the current gesture state).

TapDown doesn't trigger much in blink - just latching onto scrollbars I think
ShowPress triggers :active and :hover styles and link highlighting

So if we miss ShowPress we will miss feedback on tap.  It seems like we should
be either validating it, or ensuring blink adds it at Tap time when it's
missing.  I think I'd rather do the former, since the latter would make it easy
to mask bugs that would then manifest as unnecessary delay.  But this is all
fine for a future CL - feel free to land this one as is.

[jdduke (slow), 2014-11-18 14:54:55]

On 2014/11/18 00:06:02, Rick Byers wrote:
> On 2014/11/17 23:45:21, jdduke wrote:
> >
>
https://codereview.chromium.org/724313003/diff/1/content/browser/android/cont...
> > File content/browser/android/content_view_core_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/724313003/diff/1/content/browser/android/cont...
> > content/browser/android/content_view_core_impl.cc:1034:
> > WebInputEvent::GestureTapDown, time_ms, x, y);
> > On 2014/11/17 22:55:24, Rick Byers wrote:
> > > What about GestureShowPress?  Shouldn't Tap always be preceded by that
too?
> > 
> > Hmm, you tell me :) I believe we insert ShowPress before Tap in the
> > TouchDispositionGestureFilter, but I don't think we validate the ordering
> > anywhere, and I'm not sure if we should (we can if it's worth it). I guess I
> > don't understand the side effects of TapDown+ShowPress well enough to know
> which
> > is critical. Inserting ShowPress before Tap has always felt weird =/ (i.e.,
> why
> > can't Blink do it implicitly?).
> 
> In general we want chromium in charge of the ShowPress timeout (I think),
which
> is why blink doesn't synthesize them generally.  But we could make them
optional
> and have blink add them when they're missing (but it might be better to
minimize
> the number of places tracking the current gesture state).
> 
> TapDown doesn't trigger much in blink - just latching onto scrollbars I think
> ShowPress triggers :active and :hover styles and link highlighting
> 
> So if we miss ShowPress we will miss feedback on tap.  It seems like we should
> be either validating it, or ensuring blink adds it at Tap time when it's
> missing.  I think I'd rather do the former, since the latter would make it
easy
> to mask bugs that would then manifest as unnecessary delay.  But this is all
> fine for a future CL - feel free to land this one as is.

Sounds good, thanks.

[jdduke (slow), 2014-11-18 14:54:58]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-11-18 14:55:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/724313003/1

[commit-bot: I haz the power, 2014-11-18 15:57:13]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-11-18 15:58:51]

Patchset 1 (id:??) landed as
https://crrev.com/ed1dafa7db5fcd150a757d1237c83a81666ec47f
Cr-Commit-Position: refs/heads/master@{#304611}