Make touch-action apply to double-tap zoom

Make touch-action apply to double-tap zoom

Filter out GestureDoubleTap events on elements with touch-actions
other than auto. When GestureDoubleTap is being filtered out,
we also eliminate the tap delay.

BUG=337534
TEST=TouchActionFilterTest.DoubleTap,
  TouchActionFilterTest.DoubleTapWithTouchActionAuto

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

[tdresser, 2014-02-18 15:37:26]

rbyers@, can you do a preliminary review?

[Rick Byers, 2014-02-18 16:14:43]

Thanks.  A couple minor issues.

https://codereview.chromium.org/170603002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_action_filter.cc (right):

https://codereview.chromium.org/170603002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_action_filter.cc:93: if
(!drop_next_tap_event_)
I'd find this clearer if you invert the condition (and so avoid the nested
break). 

Similarly the nested break above seems slightly more confusing than if you just
had the logic inside of the if condition (although I don't have a strong feeling
about that one).

https://codereview.chromium.org/170603002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_action_filter.cc:95:
drop_next_tap_event_ = false;
You're going to want to reset this to false in other cases as well
(TapUnconfirmed isn't guaranteed to be followed by 'Tap', otherwise it wouldn't
really be "unconfirmed" <grin>).  Although in most cases I guess you'll probably
just get another TapUnconfirmed so it won't matter (but, for example, if the
viewport meta tag changes during the life of a page, you could cause a tap to be
lost).

Perhaps you should reset it whenever you see a GestureTapDown, or otherwise
detect the start of a new gesture sequence?  Or you could be more disciplined
about it and call it "drop current tap event", clear it on TapCancel, and ASSERT
you never see it still true on TapDown.

Note that resetting it in GestureDoubleTap is insufficient because of
double-tap-zoom (where a TapUnconfirmed is followed by a Pinch).

https://codereview.chromium.org/170603002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_action_filter.cc:113:
allowed_touch_action_ = TOUCH_ACTION_AUTO;
Wow, I just realized this is broken - it should be resetting
allowed_touch_action at the end of ALL gestures, not just scroll gestures. 
Otherwise a tap on a touch_action_none area followed by a scroll on a
touch_action_auto area would cause the scroll to be rejected.  I'll file a
separate bug for this.

https://codereview.chromium.org/170603002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_action_filter_unittest.cc
(right):

https://codereview.chromium.org/170603002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_action_filter_unittest.cc:449: //
Double tap is disabled with touch action none.
s/none/any value other than auto/
I don't think we need to test the other cases (since it's the same code path),
but I want to avoid confusion that 'none' is special here.  It's really the
absence of 'touch-action: double-tap-zoom', but since we don't (yet) have that
in blink (or the W3C standard), the only way to specify it is via 'auto'.

https://codereview.chromium.org/170603002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_action_filter_unittest.cc:454:
EXPECT_TRUE(filter.FilterGestureEvent(&tap));
Should verify that a following Tap event (without being preceeded by
TapUnconfirmed) still gets through.

https://codereview.chromium.org/170603002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_action_filter_unittest.cc:469: //
Double tap is disabled with touch action none.
comment doesn't describe this case

[tdresser, 2014-02-18 20:12:14]

https://codereview.chromium.org/170603002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_action_filter.cc (right):

https://codereview.chromium.org/170603002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_action_filter.cc:93: if
(!drop_next_tap_event_)
On 2014/02/18 16:14:43, Rick Byers wrote:
> I'd find this clearer if you invert the condition (and so avoid the nested
> break). 
> 
> Similarly the nested break above seems slightly more confusing than if you
just
> had the logic inside of the if condition (although I don't have a strong
feeling
> about that one).

Done.

https://codereview.chromium.org/170603002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_action_filter.cc:95:
drop_next_tap_event_ = false;
On 2014/02/18 16:14:43, Rick Byers wrote:
> You're going to want to reset this to false in other cases as well
> (TapUnconfirmed isn't guaranteed to be followed by 'Tap', otherwise it
wouldn't
> really be "unconfirmed" <grin>).  Although in most cases I guess you'll
probably
> just get another TapUnconfirmed so it won't matter (but, for example, if the
> viewport meta tag changes during the life of a page, you could cause a tap to
be
> lost).
> 
> Perhaps you should reset it whenever you see a GestureTapDown, or otherwise
> detect the start of a new gesture sequence?  Or you could be more disciplined
> about it and call it "drop current tap event", clear it on TapCancel, and
ASSERT
> you never see it still true on TapDown.
> 
> Note that resetting it in GestureDoubleTap is insufficient because of
> double-tap-zoom (where a TapUnconfirmed is followed by a Pinch).

I'm clearing it on TapCancel, and asserting in TapDown.
Done.

https://codereview.chromium.org/170603002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/input/touch_action_filter_unittest.cc
(right):

https://codereview.chromium.org/170603002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_action_filter_unittest.cc:449: //
Double tap is disabled with touch action none.
On 2014/02/18 16:14:43, Rick Byers wrote:
> s/none/any value other than auto/
> I don't think we need to test the other cases (since it's the same code path),
> but I want to avoid confusion that 'none' is special here.  It's really the
> absence of 'touch-action: double-tap-zoom', but since we don't (yet) have that
> in blink (or the W3C standard), the only way to specify it is via 'auto'.

Done.

https://codereview.chromium.org/170603002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_action_filter_unittest.cc:454:
EXPECT_TRUE(filter.FilterGestureEvent(&tap));
On 2014/02/18 16:14:43, Rick Byers wrote:
> Should verify that a following Tap event (without being preceeded by
> TapUnconfirmed) still gets through.

Done.

https://codereview.chromium.org/170603002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/input/touch_action_filter_unittest.cc:469: //
Double tap is disabled with touch action none.
On 2014/02/18 16:14:43, Rick Byers wrote:
> comment doesn't describe this case

Whoops, thanks. Done.

[Rick Byers, 2014-02-18 20:40:16]

lgtm with nits

https://codereview.chromium.org/170603002/diff/50022/content/browser/renderer...
File content/browser/renderer_host/input/touch_action_filter.cc (right):

https://codereview.chromium.org/170603002/diff/50022/content/browser/renderer...
content/browser/renderer_host/input/touch_action_filter.cc:99: case
WebInputEvent::GestureTapCancel:
maybe combine this case with the above into a single case since they logically
represent the same scenario?

https://codereview.chromium.org/170603002/diff/50022/content/browser/renderer...
File content/browser/renderer_host/input/touch_action_filter_unittest.cc
(right):

https://codereview.chromium.org/170603002/diff/50022/content/browser/renderer...
content/browser/renderer_host/input/touch_action_filter_unittest.cc:479:
TEST(TouchActionFilterTest, DoubleTapWithTouchActionAuto) {
nit: move this test case up above the other two since it represents the default
mode.  Also remove the OnSetTouchAction call - Auto is the default, so in
practice we never explicitly set Auto (and we have a test above that partly
verifies explicitly setting Auto is a no-op).

[tdresser, 2014-02-18 21:27:14]

jdduke, PTAL.

https://codereview.chromium.org/170603002/diff/50022/content/browser/renderer...
File content/browser/renderer_host/input/touch_action_filter.cc (right):

https://codereview.chromium.org/170603002/diff/50022/content/browser/renderer...
content/browser/renderer_host/input/touch_action_filter.cc:99: case
WebInputEvent::GestureTapCancel:
On 2014/02/18 20:40:16, Rick Byers wrote:
> maybe combine this case with the above into a single case since they logically
> represent the same scenario?

Done.

https://codereview.chromium.org/170603002/diff/50022/content/browser/renderer...
File content/browser/renderer_host/input/touch_action_filter_unittest.cc
(right):

https://codereview.chromium.org/170603002/diff/50022/content/browser/renderer...
content/browser/renderer_host/input/touch_action_filter_unittest.cc:479:
TEST(TouchActionFilterTest, DoubleTapWithTouchActionAuto) {
On 2014/02/18 20:40:16, Rick Byers wrote:
> nit: move this test case up above the other two since it represents the
default
> mode.  Also remove the OnSetTouchAction call - Auto is the default, so in
> practice we never explicitly set Auto (and we have a test above that partly
> verifies explicitly setting Auto is a no-op).

Done.

[jdduke (slow), 2014-02-18 21:51:55]

https://codereview.chromium.org/170603002/diff/230001/content/browser/rendere...
File content/browser/renderer_host/input/touch_action_filter_unittest.cc
(right):

https://codereview.chromium.org/170603002/diff/230001/content/browser/rendere...
content/browser/renderer_host/input/touch_action_filter_unittest.cc:459:
WebGestureEvent unconfirmed_tap = SyntheticWebGestureEventBuilder::Build(
Hmm, these sequences seem a little off. We should either get:

GestureTapDown -> GestureTapUnconfirmed -> GestureTap, 

or

GestureTapDown -> GestureTapUnconfirmed -> GestureTapCancel -> GestureTapDown ->
GestureDoubleTap

The key being that GestureTap follows GestureTapUnconfirmed iff the stream does
not turn into a double tap sequence (either double tap or double tap drag).

[Rick Byers, 2014-02-18 22:09:08]

On 2014/02/18 21:51:55, jdduke wrote:
>
https://codereview.chromium.org/170603002/diff/230001/content/browser/rendere...
> File content/browser/renderer_host/input/touch_action_filter_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/170603002/diff/230001/content/browser/rendere...
> content/browser/renderer_host/input/touch_action_filter_unittest.cc:459:
> WebGestureEvent unconfirmed_tap = SyntheticWebGestureEventBuilder::Build(
> Hmm, these sequences seem a little off. We should either get:
> 
> GestureTapDown -> GestureTapUnconfirmed -> GestureTap, 
> 
> or
> 
> GestureTapDown -> GestureTapUnconfirmed -> GestureTapCancel -> GestureTapDown
->
> GestureDoubleTap
> 
> The key being that GestureTap follows GestureTapUnconfirmed iff the stream
does
> not turn into a double tap sequence (either double tap or double tap drag).

Yeah, there's an interesting meta-issue here about how much we want to bake into
the implementation and tests about the legal event sequences.  I don't think we
need to go as far as making sure we test ONLY complete real-world sequences
(I.e. adding lots of GestureTapDown/GestureTapCancel pairs that have no impact
on what's being tested), but I also think we should avoid testing completely
unrealistic sequences.  This probably relates to the gesture sequence validation
stuff Tim has been thinking about - we should really define precisely what a
valid sequence is, then ensure our real behavior is a subset of that, and our
tests conform to it (but not necessarily to the subset that we happen to
implement).

In this case I was initially confused by this and then figured it was fine to
test a bare tap event, followed by a new gesture sequence.  Perhaps we just need
Tim to add the 'StartNewTouchSequence' calls to this test
(https://codereview.chromium.org/171283002/) to make it clear.

[tdresser, 2014-02-18 22:15:55]

The new set of tests is clearer and also reflects a realistic gesture stream.

https://codereview.chromium.org/170603002/diff/230001/content/browser/rendere...
File content/browser/renderer_host/input/touch_action_filter_unittest.cc
(right):

https://codereview.chromium.org/170603002/diff/230001/content/browser/rendere...
content/browser/renderer_host/input/touch_action_filter_unittest.cc:459:
WebGestureEvent unconfirmed_tap = SyntheticWebGestureEventBuilder::Build(
On 2014/02/18 21:51:55, jdduke wrote:
> Hmm, these sequences seem a little off. We should either get:
> 
> GestureTapDown -> GestureTapUnconfirmed -> GestureTap, 
> 
> or
> 
> GestureTapDown -> GestureTapUnconfirmed -> GestureTapCancel -> GestureTapDown
->
> GestureDoubleTap
> 
> The key being that GestureTap follows GestureTapUnconfirmed iff the stream
does
> not turn into a double tap sequence (either double tap or double tap drag). 

Done.

[jdduke (slow), 2014-02-18 22:43:03]

lgtm per offline discussion with tdresser@ that we should convert the dropped
DoubleTap to TapCancel.

[tdresser, 2014-02-19 14:24:57]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-02-19 14:25:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdresser@chromium.org/170603002/330001

[commit-bot: I haz the power, 2014-02-19 15:03:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdresser@chromium.org/170603002/330001

[commit-bot: I haz the power, 2014-02-19 15:31:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdresser@chromium.org/170603002/330001

[commit-bot: I haz the power, 2014-02-19 20:48:28]

Change committed as 252073