New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 771330 link

Starred by 18 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 30
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 2
Type: Bug



Sign in to add a comment

If pinch-zoom is disallowed, it should remain so even if an ongoing panning "changes" to pinching

Project Member Reported by xidac...@chromium.org, Oct 3 2017

Issue description

Chrome Version: ToT
OS: Android

What steps will reproduce the problem?
(1) Go to http://output.jsbin.com/koxiyip/quiet
(2) Using one finger to pan down on the green box
(3) Then using the second finger to pinch zoom

What is the expected result?
pinch zoom should not be allowed

What happens instead?
pinch zoom is allowed

 
Cc: dtapu...@chromium.org mustaq@chromium.org
Apparently chrome matches edge's behavior, not sure what would be the next step.
Cc: rbyers@chromium.org
Components: Blink>Input
Labels: Hotlist-Input-Dev OS-Android OS-Chrome OS-Fuchsia OS-iOS OS-Linux OS-Mac OS-Windows
Summary: If pinch-zoom is disallowed, it should remain so even if an ongoing panning "changes" to pinching (was: pinch-zoom is mistakenly allowed when it is restricted to pan)
This repros in stable as well.  This affects:
[A] the gestures that switches from single-to-multi fingers as reported above, and
[B] the gestures that switches from multi-to-single fingers (with "touch-action: pinch-zoom" in the above repro).

We seem to have introduced this bug a while back.  Here is a quick recap: we used to have a touch-action filtering logic that re-evaluated an on-going browser action when the current gesture switches between single- & multi-fingers.  We changed this to completely isolate a two-finger gesture from a one-finger gesture because we needed to support an important Maps panning use-case: two-finger gestures handled by page, one-finger gestures handled by browser ( Issue 632525 , also see the repro http://mustaqahmed.github.io/web/image-panning.html).

At that time, we also discussed Case B above and decided to allow the switch because:
- Edge have had this behavior for a while,
- a two finger pinching "includes" panning---the outcome of a single-finger panning, and
- a "touch-action: pan-left" allows (single-finger) rightward panning after an initial leftward panning because "being sloppy" after gesture changes is kind of allowed from the spec wording that effective touch-actions must be determined at the beginning of a gesture.
(Dave correct me if I missed anything here.)

---

As for this bug (aka, Case A above), it seems to me like a different problem than Case B because of an apparent asymmetry between 1-finger panning and 2-finger pinching:
- "a 2-finger pinch can effectively pan" but
- "a 1-finger pan can never zoom".

Therefore, even though we agreed to fix Case B, we still need to fix this bug.

Note that this doesn't really do against the spec wording because once a browser action has started, the browser can "execute" the action in any reasonable way.

Comment 3 by mcnee@chromium.org, Nov 17 2017

Blocking: 786419
Labels: -OS-iOS

Comment 5 by mustaq@chromium.org, Feb 14 2018

Cc: vamshi.k...@techmahindra.com
 Issue 808957  has been merged into this issue.

Comment 6 by mcnee@chromium.org, Mar 12 2018

Cc: wjmaclean@chromium.org kenrb@chromium.org mcnee@chromium.org fsam...@chromium.org tedc...@chromium.org pnangunoori@chromium.org
 Issue 820391  has been merged into this issue.
Cc: m...@seth-holladay.com sandeepkumars@chromium.org dmazz...@chromium.org
 Issue 821861  has been merged into this issue.
Cc: nzolghadr@chromium.org xidac...@chromium.org
 Issue 851621  has been merged into this issue.
 Issue 863904  has been merged into this issue.
Any updates on this? We ran into this issue recently, we have implemented a workaround where we preventDefault() on touchstart events with more than one touch, but that requires having a root-level non-passive listener so it has performance impacts.

This is particularly bad because if a user accidentally zooms in this way, there is no easy way to unzoom the page - zoom-out pinches don't work (unless you manage to trigger this bug again for the zoom out), the zoom in dots-menu is still 100%, and even refreshing the page will not restore you to a normal zoom level where you can use the app.

I recorded a video of this behaviour in Google maps, and how I can easily get into a state where I can't see any of the Google Maps UI, and there is no way to zoom out: https://photos.app.goo.gl/esqz6DWstmZxL6UH6

Thanks
Blocking: -786419
Cc: bokan@chromium.org
Labels: -Pri-3 Pri-2
Re#10, No updates unfortunately.

However, I've come across where this issue lies; the TouchActionFilter does all gesture filtering on the GestureScrollBegin. It filters pinches by looking at GSBs with 2 touch points. This will fail if the scroll starts with one finger (as well as failing to filter double-tap-drag zoom, see https://crrev.com/c/1279236 for some more context).

Given the high number of stars we should prioritize this. xidachen@, do you have any immediate plans to work on this? 
Status: Started (was: Assigned)
xidachen@ and I had a discussion today, looks like we need to end the current gesture stream early when a gesture changes from 1-finger to 2-fingers, without possibly forwarding corresponding touch/pointer events to the page (because the page already received a touch/pointercancel for the current gesture).
The problem is more complicated. I thought about fixing this by re-evaluating whether a gesture is allowed or not at GesturePinchBegin, but this is basically going back to the state before this CL:
https://codereview.chromium.org/2726623002

Note that the above CL fixes an important use case, so we really don't want go back to the state before that. We need to find a better way to solve this.
I'm evaluating filtering on GesturePinchBegin in https://chromium-review.googlesource.com/c/chromium/src/+/1279236 but only for double-tap-drag. I think we shouldn't be keeping the same "should_filter" state for both pinches and scrolls. We should track them independently. 
We used to have independent "should_filter" for pinch vs scroll, see the CL in #c14---the real challenge with that idea is handling shaky fingers: a two finger scrolling easily fires a GPB through  even when user just meant to scroll.

Consider the following scenario which we fixed through the above CL:  suppose we have a div with "pan-y" which shouldn't allow zoom.  Assume we started dragging with simultaneous-2-fingers along the y-axis, we will see an event sequence like this:
  GSB, GSUs... , GPB (finger shakes here), GPUs..., GPE+GSE.
                 | <---- forbidden pinch gesture ----> |

If we drop [GPB...GPE], the div stops responding after the shaky fingers until the end of the gesture sequence (this was the Maps bug we fixed).  Since the end of the "pinch component" within the whole sequence seems impossible to detect just from the physical interaction, I think a Boolean (filter-only) outcome can't fix the problem.

I suggest this: instead of dropping the "forbidden pinch gesture" in this case, we can perhaps _change_ those events to GPUs.
Doesn't the sequence look like this? :

GSB, GSUs..., GPB, GPU, GSU, GPU, GSU..., GPE, GSE

GPU doesn't have any location/movement information in it so I'm assuming we send GSUs to keep the pinch centered at the users fingers (I just confirmed this is so). In this case, what I'm proposing is filtering out all the GP_ events but keeping the "centering" GSUs. Is there a reason that wouldn't work?
Hmm, I seem to recall that dropping the GP* events was causing the unresponsiveness in Maps panning ( Issue 632525 ).  May be I am wrong, may be dropping the interleaved GPUs (those interleaved between the GPUs) was the actual problem.

Project Member

Comment 19 by bugdroid1@chromium.org, Oct 30

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/06a04426d9ab89de735fcb04a0e5c7957d179e29

commit 06a04426d9ab89de735fcb04a0e5c7957d179e29
Author: Xida Chen <xidachen@chromium.org>
Date: Tue Oct 30 05:35:30 2018

Re-evaluate pinch events at GesturePinchBegin

Right now pinch events are evaluated at GestureScrollBegin, if a gesture
is allowed at GSB, then the entire gesture is allowed. This cause a
problem in the following scenario. We have a div with pan-y, user starts
with scrolling along the y-direction, and then the second finger touches
the screen and generates a pinch zoom gesture. In this case, the pinch
zoom gesture is allowed because the user starts with scrolling along the
y-axis which is allowed.

This CL fixes the problem by re-evaluating the gesture event at
GesturePinchBegin. There are few cases to consider
1. The above described case. We will allow scrolling on the y-axis but
disallow the pinch zoom when the second finger is touched.
2. It should work with certain embedded map case such as the demo here:
http://mustaqahmed.github.io/web/image-panning.html
If a user starts with two finger panning the embedded map, then the
gesture will be evaluated at GestureScrollBegin and will be disallowed,
which means the page will handle the pointer events and manipulate the
map.
3. It should work with double-tap-drag-zoom case. In this case, there
is no GSB, and when GesturePinchBegin arrives, we evaluate that.

Bug:  771330 
Change-Id: Ibeaeace11bc017384317f61aabc0f23008ad2b5f
Reviewed-on: https://chromium-review.googlesource.com/c/1297070
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Mustaq Ahmed <mustaq@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603795}
[modify] https://crrev.com/06a04426d9ab89de735fcb04a0e5c7957d179e29/content/browser/renderer_host/input/touch_action_browsertest.cc
[modify] https://crrev.com/06a04426d9ab89de735fcb04a0e5c7957d179e29/content/browser/renderer_host/input/touch_action_filter.cc
[modify] https://crrev.com/06a04426d9ab89de735fcb04a0e5c7957d179e29/content/browser/renderer_host/input/touch_action_filter.h
[modify] https://crrev.com/06a04426d9ab89de735fcb04a0e5c7957d179e29/content/browser/renderer_host/input/touch_action_filter_unittest.cc

Status: Fixed (was: Started)
Cc: chelamcherla@chromium.org
 Issue 899560  has been merged into this issue.
 Issue 649034  has been merged into this issue.

Sign in to add a comment