Implement support for touch-action: pan-x/pan-y in chromium

Implement support for touch-action: pan-x/pan-y in chromium

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

[Rick Byers, 2013-12-19 03:31:34]

Jared,
Please take a look.  I still need to upload a separate CL to populate the scroll
begin hints on Android, but since this is all unit tests it can land in advance
of that.

Thanks,
   Rick

[jdduke (slow), 2013-12-19 05:06:04]

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

https://codereview.chromium.org/93733005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/touch_action_filter.cc:28:
DCHECK(!drop_scroll_gesture_events_);

We'll have to make a couple (minor) adjustments for this to work on Android. 
Currently we set a |is_scrolling| bit that shortcuts the round-trip to the TEQ
from the ContentViewGestureHandler after we've sent a GestureScrollBegin (so
events are fed directly to the GR rather than being ack'ed synchronously by the
TEQ).  However, I don't think we lose anything (and we gain a lot, like
eliminating gotchas like this) by removing that bit and relying solely on the
TEQ for dispatch; it's really just a remnant of the TouchCancel optimization
code that we moved to the TEQ.

I can fix that separately (and also populate the hints) if you want?

https://codereview.chromium.org/93733005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/touch_action_filter.cc:47: if
(gesture_event->sourceDevice != WebGestureEvent::Touchscreen)

Do we ever get GSB/GSU events from a touchpad? Or do those always come down as
MouseWheels?

https://codereview.chromium.org/93733005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/touch_action_filter.cc:55: // FALL THROUGH

Nit: Maybe instead of a fall through we could have a common "return
FilterGestureEnd()" or similar method? Not a huge deal.

https://codereview.chromium.org/93733005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/touch_action_filter.cc:79:
allowed_touch_action_ = Intersect(allowed_touch_action_, touch_action);

Is there a stronger spec for this? To me it seems like the initial pointer
touch-action should be the rule for the gesture, but maybe there are scenarios
for which that's insufficient?

https://codereview.chromium.org/93733005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/touch_action_filter.cc:84:
DCHECK(gesture_event.type == WebInputEvent::GestureScrollBegin);

Nit: DCHECK_EQ

[Rick Byers, 2013-12-19 05:35:46]

Thanks!

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

https://codereview.chromium.org/93733005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/touch_action_filter.cc:28:
DCHECK(!drop_scroll_gesture_events_);
On 2013/12/19 05:06:05, jdduke wrote:
> 
> We'll have to make a couple (minor) adjustments for this to work on Android. 
> Currently we set a |is_scrolling| bit that shortcuts the round-trip to the TEQ
> from the ContentViewGestureHandler after we've sent a GestureScrollBegin (so
> events are fed directly to the GR rather than being ack'ed synchronously by
the
> TEQ).  However, I don't think we lose anything (and we gain a lot, like
> eliminating gotchas like this) by removing that bit and relying solely on the
> TEQ for dispatch; it's really just a remnant of the TouchCancel optimization
> code that we moved to the TEQ.

Cool, thanks for the details.

> I can fix that separately (and also populate the hints) if you want?

Thanks!  I'm happy to do it too - it's actually easier to test on Android than
ChromeOS when I'm travelling anyway.  I don't think there's any ordering
requirements between these two CLs (right)?  Anyway up to you...

https://codereview.chromium.org/93733005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/touch_action_filter.cc:47: if
(gesture_event->sourceDevice != WebGestureEvent::Touchscreen)
On 2013/12/19 05:06:05, jdduke wrote:
> 
> Do we ever get GSB/GSU events from a touchpad? Or do those always come down as
> MouseWheels?

No, they're always mousewheels.  I did a quick scan of the code earlier today to
confirm that Touchpad is never set for anything other than GestureFlingStart.  I
considered putting this if condition at the top but since we never read it
elsewhere for non-fling scenarios I figured I'd probably just get failures from
tests that are being lazy and failing to initialize it (0 is Touchpad
unfortunately).

https://codereview.chromium.org/93733005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/touch_action_filter.cc:55: // FALL THROUGH
On 2013/12/19 05:06:05, jdduke wrote:
> 
> Nit: Maybe instead of a fall through we could have a common "return
> FilterGestureEnd()" or similar method? Not a huge deal.

Sure, done.  I agree this case statement is starting to get too complex (and
it's going to get worse when I add pinch support).

https://codereview.chromium.org/93733005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/touch_action_filter.cc:79:
allowed_touch_action_ = Intersect(allowed_touch_action_, touch_action);
On 2013/12/19 05:06:05, jdduke wrote:
> 
> Is there a stronger spec for this? To me it seems like the initial pointer
> touch-action should be the rule for the gesture, but maybe there are scenarios
> for which that's insufficient?

I couldn't get the Microsoft guys to agree to spec something here because it's
too related to the behavior of 'pinch' which is full of IP concerns (and so out
of scope for the pointer events WG).  I played a bit with IE in a number of edge
cases and saw some surprising behavior that seemed to be at least partially due
to bugs.

I figured we should just pick something that seemed reasonable and play with it
for a bit (in particular test content designed for IE).  Almost all multi-finger
scenarios here are edge-casey (the common case will probably be when the
touch-action matches for all fingers).

Using just the action for the first finger is possibly reasonable too.  One
thing I don't like about it is that the behavior may seem non-deterministic to a
user when they but both fingers down "at the same time".  Also if you think of a
game where you drag one finger around a canvas (eg. angry birds) it would kind
of suck if briefly touching a second finger on the space around the game canvas
could trigger pinching.  I felt you'd best avoid accidental pinches by allowing
it only when both fingers are on areas that allow it.  What do you think?  Can
you think of scenarios where the user experience would be better if we took just
the action for the first finger?

https://codereview.chromium.org/93733005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/touch_action_filter.cc:84:
DCHECK(gesture_event.type == WebInputEvent::GestureScrollBegin);
On 2013/12/19 05:06:05, jdduke wrote:
> 
> Nit: DCHECK_EQ

Done.

[jdduke (slow), 2013-12-19 16:17:16]

content/browser/renderer_host/input lgtm.

https://codereview.chromium.org/93733005/diff/20001/content/browser/renderer_...
File content/browser/renderer_host/input/touch_action_filter.cc (right):

https://codereview.chromium.org/93733005/diff/20001/content/browser/renderer_...
content/browser/renderer_host/input/touch_action_filter.cc:30: return
drop_scroll_gesture_events_;

If a GSB is dropped, the pinch sequence wrapped by the GSB/GSE will fail when it
reaches the impl thread.  As long as we're aware of this  moving forward...

https://codereview.chromium.org/93733005/diff/20001/content/common/input/touc...
File content/common/input/touch_action.h (right):

https://codereview.chromium.org/93733005/diff/20001/content/common/input/touc...
content/common/input/touch_action.h:15: TOUCH_ACTION_AUTO = 0,

In the future, having TOUCH_ACTION_AUTO be the union of the flags, with
TOUCH_ACTION_NONE == 0 might be useful, but there are few enough combinations
here it's not a big deal.

[Rick Byers, 2013-12-19 16:35:09]

Thanks!

https://codereview.chromium.org/93733005/diff/20001/content/browser/renderer_...
File content/browser/renderer_host/input/touch_action_filter.cc (right):

https://codereview.chromium.org/93733005/diff/20001/content/browser/renderer_...
content/browser/renderer_host/input/touch_action_filter.cc:30: return
drop_scroll_gesture_events_;
On 2013/12/19 16:17:16, jdduke wrote:
> 
> If a GSB is dropped, the pinch sequence wrapped by the GSB/GSE will fail when
it
> reaches the impl thread.  As long as we're aware of this  moving forward...

Yes, thanks.  As discussed I'll be dropping pinch events whenever scroll events
get dropped, but I'd rather do that in the upcoming pinch-centric CL (see TODO
above).  At the moment nothing outside tests can actually hit this codepath
because the CSSTouchAction RuntimeEnabledFeature in blink is set to 'Test'. 
I'll do some testing on device (including pinch scenarios) before flipping it to
'Experimental' (which will enable it behind a runtime flag).

https://codereview.chromium.org/93733005/diff/20001/content/common/input/touc...
File content/common/input/touch_action.h (right):

https://codereview.chromium.org/93733005/diff/20001/content/common/input/touc...
content/common/input/touch_action.h:15: TOUCH_ACTION_AUTO = 0,
On 2013/12/19 16:17:16, jdduke wrote:
> 
> In the future, having TOUCH_ACTION_AUTO be the union of the flags, with
> TOUCH_ACTION_NONE == 0 might be useful, but there are few enough combinations
> here it's not a big deal.

We can't actually - the semantics are different.  Auto permits pinching for
example where TOUCH_ACTION_PAN_X | TOUCH_ACTION_PAN_Y doesn't.  So there's
always going to be some 'flag' that can't be combined with any others.

We debated a little on the blink side (and changed it once) about whether AUTO
or NONE should have the value 0 - the benefit to AUTO being 0 is that AUTO is
the default (same as unspecified) and so will be the 99% common case on the web
for awhile.  But as long as we have this impure-flags semantics, then it's never
going to be simple.  Also I now COMPILE_ASSERT that the 3 different definitions
of this enum match their values exactly.

See http://msdn.microsoft.com/en-us/library/Windows/Apps/hh767313.aspx for
details on the larger set of semantics IE has here.

[jdduke (slow), 2013-12-19 16:35:47]

On 2013/12/19 16:35:09, Rick Byers wrote:
> Thanks!
> 
>
https://codereview.chromium.org/93733005/diff/20001/content/browser/renderer_...
> File content/browser/renderer_host/input/touch_action_filter.cc (right):
> 
>
https://codereview.chromium.org/93733005/diff/20001/content/browser/renderer_...
> content/browser/renderer_host/input/touch_action_filter.cc:30: return
> drop_scroll_gesture_events_;
> On 2013/12/19 16:17:16, jdduke wrote:
> > 
> > If a GSB is dropped, the pinch sequence wrapped by the GSB/GSE will fail
when
> it
> > reaches the impl thread.  As long as we're aware of this  moving forward...
> 
> Yes, thanks.  As discussed I'll be dropping pinch events whenever scroll
events
> get dropped, but I'd rather do that in the upcoming pinch-centric CL (see TODO
> above).  At the moment nothing outside tests can actually hit this codepath
> because the CSSTouchAction RuntimeEnabledFeature in blink is set to 'Test'. 
> I'll do some testing on device (including pinch scenarios) before flipping it
to
> 'Experimental' (which will enable it behind a runtime flag).
> 
>
https://codereview.chromium.org/93733005/diff/20001/content/common/input/touc...
> File content/common/input/touch_action.h (right):
> 
>
https://codereview.chromium.org/93733005/diff/20001/content/common/input/touc...
> content/common/input/touch_action.h:15: TOUCH_ACTION_AUTO = 0,
> On 2013/12/19 16:17:16, jdduke wrote:
> > 
> > In the future, having TOUCH_ACTION_AUTO be the union of the flags, with
> > TOUCH_ACTION_NONE == 0 might be useful, but there are few enough
combinations
> > here it's not a big deal.
> 
> We can't actually - the semantics are different.  Auto permits pinching for
> example where TOUCH_ACTION_PAN_X | TOUCH_ACTION_PAN_Y doesn't.  So there's
> always going to be some 'flag' that can't be combined with any others.
> 
> We debated a little on the blink side (and changed it once) about whether AUTO
> or NONE should have the value 0 - the benefit to AUTO being 0 is that AUTO is
> the default (same as unspecified) and so will be the 99% common case on the
web
> for awhile.  But as long as we have this impure-flags semantics, then it's
never
> going to be simple.  Also I now COMPILE_ASSERT that the 3 different
definitions
> of this enum match their values exactly.
> 
> See http://msdn.microsoft.com/en-us/library/Windows/Apps/hh767313.aspx for
> details on the larger set of semantics IE has here.

Thanks for the info.  content/common/input also lgtm :)

[Rick Byers, 2013-12-19 16:39:06]

Thanks.

+jochen for OWNERS of content/renderer/render_widget.cc

[jochen (gone - plz use gerrit), 2013-12-20 07:53:55]

lgtm

[commit-bot: I haz the power, 2013-12-20 15:19:58]

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

[commit-bot: I haz the power, 2013-12-20 17:29:35]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-12-20 17:50:30]

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

[commit-bot: I haz the power, 2013-12-20 19:44:39]

Change committed as 242147