1463139 - One-touch-pinch to zoom gesture (aka double-tap-drag) begins after a delay but zoom calculation does not take that delay into account, leading to a visual snap when zoom suddenly jumps from 1.00 to 1.05 (for example)

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Adam D. Walling]

Steps to reproduce:
Double-tap-drag zoom in and out.

Expected result
After a short delay, the touch gesture is interpreted as being a one-touch-pinch and zooming begins, starting at no zoom (1.00) and smoothly increasing (or decreasing) in proportion to the drag distance.

Actual result
Since there is a brief delay before the gesture begins, there is also a brief distance dragged before the gesture begins as well. When the zooming starts, it does so as if the zoom was running since the very first tap. This causes the perception of lost frames or an animation jerk since the zoom starts immediately with a non-zero distance yielding (eg) 1.05 zoom instead of 1.00.

The calculation of the distance dragged for the zoom should use the delta of the current touch position and the position at which the gesture began (after the delay) rather than the position that was initially tapped.

This behavior better emulates the default gesture behavior on Android and, for example, within Chrome.

Environment:
Originally observed on Firefox 60 on Android (Galaxy Note 5) and reproduced on myriad devices.

Comment 1

[Adam D. Walling]

Attachment: calculate Y-span for one-touch-pinch gesture from point that gesture began

Comment 2

[Adam D. Walling]

Attached patch appears to work great on the devices I've tested with, though those are narrow (Galaxy Note 5, Galaxy S9)

I can adb shell screenrecord later if necessary to show the difference between the two (firefox beta vs build with this patch for example)

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8979292 [details] [diff] [review]
calculate Y-span for one-touch-pinch gesture from point that gesture began

Review of attachment 8979292 [details] [diff] [review]:
-----------------------------------------------------------------

I can't say I've noticed this behaviour, but the patch looks reasonable enough. One small naming nit below. With that addressed I'm happy to land the patch.

::: gfx/layers/apz/src/GestureEventListener.h
@@ +215,5 @@
> +   * the threshold for the gesture, there is already a span that the zoom
> +   * is calculated from, instead of starting at 1.0 when the threshold gets
> +   * passed.
> +   */
> +  ParentLayerPoint mTouchOneTouchPinchPosition;

I'd prefer to call this "mOneTouchPinchStartPosition" rather than having "touch" in the name twice.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Hi Adam, are you planning on updating the patch to address the review comment above?

Comment 5

[Adam D. Walling]

I had to recreate my build environment but I'll submit the new patch before too long.

Comment 6

[Adam D. Walling]

Attachment: renamed mTouchOneTouchPinchPosition to mOneTouc hPinchStartPosition

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8984846 [details] [diff] [review]
renamed mTouchOneTouchPinchPosition to mOneTouc hPinchStartPosition

Looks good, thanks!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90d7fb47fbe9
calculate Y-span for one-touch-pinch gesture from point that gesture began (which occurs after a brief delay) instead of first touch point. r=kats

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/90d7fb47fbe9