Closed Bug 1391770 Opened 7 years ago Closed 7 years ago

Doing a one-touch-pinch gesture with second tap far away from the first works when it shouldn't

Categories

(Core :: Panning and Zooming, defect, P2)

56 Branch
All
Android
defect

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox55 --- disabled
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(1 file)

Do a one-touch-pinch gesture (touchdown, touchup, touchdown, touchmove) but make the second touchdown far away from the first. It's easier if you use two fingers far away on the screen. This shouldn't work, but it does.

What's worse is that if you do a double-tap gesture in this fashion (one tap far away from the second) we actually quickly enter one-touch-pinch, change the zoom level, and then exit one-touch-pinch, making it look like the zoom level just "snapped" to some arbitrary value. (The value actually depends on the distance between your two fingers on the y-axis).
Note that prior to bug 1111333 landing, this scenario of doing a double-tap with two fingers far away would go through [1] and the MoveDistanceIsLarge() check would cause the gesture to abort. However, since bug 1111333 landed, the double-tap goes through [2] which enters the one-touch pinch state. This is important because in correcting this, we need to make sure that we don't accidentally end up detecting double-tap gestures.

[1] http://searchfox.org/mozilla-central/rev/123ab8fe26a3ef2438b8f918dfe548187edb885b/gfx/layers/apz/src/GestureEventListener.cpp#258
[2] http://searchfox.org/mozilla-central/rev/e8c36327cd8c9432c69e5e1383156a74330f11f2/gfx/layers/apz/src/GestureEventListener.cpp#283
Comment on attachment 8898970 [details]
Bug 1391770 - Don't allow a faraway second tap to start a one-touch-pinch gesture.

https://reviewboard.mozilla.org/r/170316/#review175568

::: gfx/layers/apz/src/GestureEventListener.h:150
(Diff revision 1)
>  
>    void TriggerSingleTapConfirmedEvent();
>  
> +  bool MoveDistanceExceeds(ScreenCoord aThreshold);
>    bool MoveDistanceIsLarge();
> +  bool SecondTapIsFar();

These methods can be |const|.

::: gfx/layers/apz/src/GestureEventListener.cpp:189
(Diff revision 1)
> +      CancelLongTapTimeoutTask();
> +      CancelMaxTapTimeoutTask();
> +      mSingleTapSent = Nothing();
> +      SetState(GESTURE_NONE);
> +    } else {
> +      // Otherwise, reset the touch start position so that the one-touch-pinch

At this point, we could still get either a double-tap or a one-touch-pinch, right? If so, could we clarify the comment, along the lines of, "... so that, if this turns into a one-touch-pinch gesture, it uses the ...".
Attachment #8898970 - Flags: review?(botond) → review+
Thanks, updated patch with both changes.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b920455971b0
Don't allow a faraway second tap to start a one-touch-pinch gesture. r=botond
https://hg.mozilla.org/mozilla-central/rev/b920455971b0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8898970 [details]
Bug 1391770 - Don't allow a faraway second tap to start a one-touch-pinch gesture.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1111333
[User impact if declined]: performing a "double-tap" type of gesture with the two taps far apart can result in a weird zoom
[Is this code covered by automated tests?]: partially
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not very
[Why is the change risky/not risky?]: fairly small code change, code is well-understood
[String changes made/needed]: none
Attachment #8898970 - Flags: approval-mozilla-beta?
Hi Ioana, could you help find someone to verify if this issue was fixed as expected on the latest Nightly build? Thanks!
Flags: qe-verify+
Flags: needinfo?(ioana.chiorean)
Comment on attachment 8898970 [details]
Bug 1391770 - Don't allow a faraway second tap to start a one-touch-pinch gesture.

Looks ok to uplift to beta 7, low risk change to pinch/zoom.  
We can verify this in beta once it lands.
Attachment #8898970 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Pixel XL - Android 8.0 
Using Nightly 59.0a1, Beta 58.0b3 and Release 57.0 

I followed the bug for several weeks in order to verify it on different screens and hardware and i am not able to reproduce it anymore.
Status: RESOLVED → VERIFIED
Flags: needinfo?(ioana.chiorean)
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.