If pinch-zoom is disallowed, it should remain so even if an ongoing panning "changes" to pinching |
||||||||||
Issue descriptionChrome 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
,
Oct 4 2017
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.
,
Nov 17 2017
,
Dec 15 2017
,
Feb 14 2018
,
Mar 12 2018
Issue 820391 has been merged into this issue.
,
Mar 20 2018
Issue 821861 has been merged into this issue.
,
Jun 11 2018
,
Jul 19
Issue 863904 has been merged into this issue.
,
Sep 3
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
,
Sep 26
,
Oct 18
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?
,
Oct 18
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).
,
Oct 18
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.
,
Oct 18
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.
,
Oct 19
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.
,
Oct 19
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?
,
Oct 19
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.
,
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
,
Oct 30
,
Nov 1
,
Nov 20
Issue 649034 has been merged into this issue. |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by xidac...@chromium.org
, Oct 4 2017