Fixed two-finger pan filtering with 'touch-action: pan-x pan-y'.

Fixed two-finger pan filtering with 'touch-action: pan-x pan-y'.

For touch-action filtering, we treat a two-finger pan like a single-finger
pan. As a result, a 'touch-action: pan-x pan-y' region allows two-finger
panning but disallows pinch-zooming. This looks problematic:
- It is not easy for users to distinguish between two-finger panning vs
  pinching.
- Edge's touch-action filtering treats a two-finger pan like a pinch-zoom
  for a while.
- Popular embedded maps need to distinguish between two-finger vs
  one-finger gestures through touch-action, where a two-finger gestures
  is used to manipulate the map while a one-finger gesture is handled by
  the browser. Here is a mock:
    http://mustaqahmed.github.io/web/image-panning.html

This CL filters a two-finger pan like a pinch-zoom to solve this problem.

In addition to solving the bug, this CL also simplified the touch-action
filtering code around panning/zooming.

BUG=632525
Review-Url: https://codereview.chromium.org/2726623002
Cr-Commit-Position: refs/heads/master@{#456722}
Committed: https://chromium.googlesource.com/chromium/src/+/16ce460b6793b9fff0c1d4cc72deab60b921a159

[mustaq, 2017-02-28 18:21:11]

mustaq@chromium.org changed reviewers:
+ dtapuska@chromium.org, rbyers@chromium.org

[mustaq, 2017-02-28 18:21:12]

ptal

[dtapuska, 2017-02-28 18:29:35]

https://codereview.chromium.org/2726623002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/touch_action_filter.h (right):

https://codereview.chromium.org/2726623002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/touch_action_filter.h:30: bool
FilterGestureEvent(blink::WebGestureEvent& gesture_event);
If this is an output parameter (which it is because it gets adjusted) it needs
to be passed by ptr not reference. (see
https://google.github.io/styleguide/cppguide.html#Reference_Arguments)

[mustaq, 2017-02-28 19:27:01]

https://codereview.chromium.org/2726623002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/touch_action_filter.h (right):

https://codereview.chromium.org/2726623002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/touch_action_filter.h:30: bool
FilterGestureEvent(blink::WebGestureEvent& gesture_event);
On 2017/02/28 18:29:35, dtapuska wrote:
> If this is an output parameter (which it is because it gets adjusted) it needs
> to be passed by ptr not reference. (see
> https://google.github.io/styleguide/cppguide.html#Reference_Arguments)
> 

Done. Sorry got misguided by Blink style guide:
https://www.chromium.org/blink/coding-style#TOC-Pointers-and-References

[dtapuska, 2017-03-13 19:17:35]

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

https://codereview.chromium.org/2726623002/diff/20001/content/browser/rendere...
content/browser/renderer_host/input/touch_action_filter.cc:55: if
(IsYAxisActionDisallowed(allowed_touch_action_)) {
I think this might be wrong now. Try two finger scrolling on a
touch-action:pinch-zoom div. I think it should scroll in the y direction but
this code will clobber the y axis and only allow scrolling in the x axis.

https://codereview.chromium.org/2726623002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/input/touch_action_filter.h (right):

https://codereview.chromium.org/2726623002/diff/20001/content/browser/rendere...
content/browser/renderer_host/input/touch_action_filter.h:45: bool
ShouldSuppressManipulation(const blink::WebGestureEvent&);
Why the one space indent?

[mustaq, 2017-03-13 20:36:40]

The CQ bit was checked by mustaq@chromium.org to run a CQ dry run

[mustaq, 2017-03-13 20:37:01]

ptal.

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

https://codereview.chromium.org/2726623002/diff/20001/content/browser/rendere...
content/browser/renderer_host/input/touch_action_filter.cc:55: if
(IsYAxisActionDisallowed(allowed_touch_action_)) {
On 2017/03/13 19:17:35, dtapuska wrote:
> I think this might be wrong now. Try two finger scrolling on a
> touch-action:pinch-zoom div. I think it should scroll in the y direction but
> this code will clobber the y axis and only allow scrolling in the x axis.

That's a good point but the clobbering takes place only when a "pan-*" is also
specified, as in "touch-action: pan-y pinch-zoom".

Fortunately, the behavior here matches Edge exactly: in Edge (and in chromium
with this CL), a "touch-action: pan-y pinch-zoom" allows vertical two-finger
scrolling but a "touch-action: pan-x pinch-zoom" doesn't. I added a TODO to go
ahead.

https://codereview.chromium.org/2726623002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/input/touch_action_filter.h (right):

https://codereview.chromium.org/2726623002/diff/20001/content/browser/rendere...
content/browser/renderer_host/input/touch_action_filter.h:45: bool
ShouldSuppressManipulation(const blink::WebGestureEvent&);
On 2017/03/13 19:17:35, dtapuska wrote:
> Why the one space indent?

Looks like a "git cl format" bug. Fixing the indentation for public/private
fixed the output. Sorry for missing this.

[commit-bot: I haz the power, 2017-03-13 20:37:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dtapuska, 2017-03-13 20:39:39]

On 2017/03/13 20:37:01, mustaq wrote:
> ptal.
> 
>
https://codereview.chromium.org/2726623002/diff/20001/content/browser/rendere...
> File content/browser/renderer_host/input/touch_action_filter.cc (right):
> 
>
https://codereview.chromium.org/2726623002/diff/20001/content/browser/rendere...
> content/browser/renderer_host/input/touch_action_filter.cc:55: if
> (IsYAxisActionDisallowed(allowed_touch_action_)) {
> On 2017/03/13 19:17:35, dtapuska wrote:
> > I think this might be wrong now. Try two finger scrolling on a
> > touch-action:pinch-zoom div. I think it should scroll in the y direction but
> > this code will clobber the y axis and only allow scrolling in the x axis.
> 
> That's a good point but the clobbering takes place only when a "pan-*" is also
> specified, as in "touch-action: pan-y pinch-zoom".
> 
> Fortunately, the behavior here matches Edge exactly: in Edge (and in chromium
> with this CL), a "touch-action: pan-y pinch-zoom" allows vertical two-finger
> scrolling but a "touch-action: pan-x pinch-zoom" doesn't. I added a TODO to go
> ahead.
> 
>
https://codereview.chromium.org/2726623002/diff/20001/content/browser/rendere...
> File content/browser/renderer_host/input/touch_action_filter.h (right):
> 
>
https://codereview.chromium.org/2726623002/diff/20001/content/browser/rendere...
> content/browser/renderer_host/input/touch_action_filter.h:45: bool
> ShouldSuppressManipulation(const blink::WebGestureEvent&);
> On 2017/03/13 19:17:35, dtapuska wrote:
> > Why the one space indent?
> 
> Looks like a "git cl format" bug. Fixing the indentation for public/private
> fixed the output. Sorry for missing this.

lgtm

[commit-bot: I haz the power, 2017-03-13 22:00:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 22:00:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mustaq, 2017-03-14 16:16:11]

The CQ bit was checked by mustaq@chromium.org

[commit-bot: I haz the power, 2017-03-14 16:16:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 16:22:40]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489508171741550,
"parent_rev": "d9499133c2f640b6e852197e6b11126ff45637ad", "commit_rev":
"16ce460b6793b9fff0c1d4cc72deab60b921a159"}

[commit-bot: I haz the power, 2017-03-14 16:23:28]

Description was changed from

==========
Fixed two-finger pan filtering with 'touch-action: pan-x pan-y'.

For touch-action filtering, we treat a two-finger pan like a single-finger
pan. As a result, a 'touch-action: pan-x pan-y' region allows two-finger
panning but disallows pinch-zooming. This looks problematic:
- It is not easy for users to distinguish between two-finger panning vs
  pinching.
- Edge's touch-action filtering treats a two-finger pan like a pinch-zoom
  for a while.
- Popular embedded maps need to distinguish between two-finger vs
  one-finger gestures through touch-action, where a two-finger gestures
  is used to manipulate the map while a one-finger gesture is handled by
  the browser. Here is a mock:
    http://mustaqahmed.github.io/web/image-panning.html

This CL filters a two-finger pan like a pinch-zoom to solve this problem.

In addition to solving the bug, this CL also simplified the touch-action
filtering code around panning/zooming.

BUG=632525
==========

to

==========
Fixed two-finger pan filtering with 'touch-action: pan-x pan-y'.

For touch-action filtering, we treat a two-finger pan like a single-finger
pan. As a result, a 'touch-action: pan-x pan-y' region allows two-finger
panning but disallows pinch-zooming. This looks problematic:
- It is not easy for users to distinguish between two-finger panning vs
  pinching.
- Edge's touch-action filtering treats a two-finger pan like a pinch-zoom
  for a while.
- Popular embedded maps need to distinguish between two-finger vs
  one-finger gestures through touch-action, where a two-finger gestures
  is used to manipulate the map while a one-finger gesture is handled by
  the browser. Here is a mock:
    http://mustaqahmed.github.io/web/image-panning.html

This CL filters a two-finger pan like a pinch-zoom to solve this problem.

In addition to solving the bug, this CL also simplified the touch-action
filtering code around panning/zooming.

BUG=632525

Review-Url: https://codereview.chromium.org/2726623002
Cr-Commit-Position: refs/heads/master@{#456722}
Committed:
https://chromium.googlesource.com/chromium/src/+/16ce460b6793b9fff0c1d4cc72de...
==========

[commit-bot: I haz the power, 2017-03-14 16:23:29]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/16ce460b6793b9fff0c1d4cc72de...

[qyearsley, 2017-03-14 18:39:23]

On 2017/03/14 at 16:23:29, commit-bot wrote:
> Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/16ce460b6793b9fff0c1d4cc72de...

Good news! This causes a test to start passing:

https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=ex...

I'll make a rebaseline CL real quick to update the expectations.