1685648 - pinch zoom on non-pinch zoomable page often leads to unintended fast fling

Status: RESOLVED FIXED

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

Description

[Timothy Nikkel (:tnikkel)]

Load a page such as https://skyewealth.ca/vitality/ where you can't pinch zoom on firefox for android. Try to pinch zoom with your fingers moving in a vertical fashion, then lift your fingers. Very often I get a pretty fast fling that scrolls the page quite far. Testing on chrome I get a very modest scroll that doesn't feel off.

Comment 1

[Botond Ballo [:botond]]

On non-zoomable pages, two-finger touch gestures are still processed, and the gestures are still modelled as pinch gesture events, but only the scrolling resulting from the movement of the pinch gesture's focus point is applied, not the zooming resulting from the change in the pinch gesture's span. We often call this "two-finger panning".

We implemented flinging at the end of a two-finger pan in bug 1180799.

We also implemented something called "pinch locking" in bug 1180865 (with improvements in bug 1451461), which tries to detect whether a two-finger touch gesture is intended to be a pinch or a two-finger pan and lock you into that.

It seems like it should be relatively straightforward to make sure pinch-locking kicks in on non-zoomable pages and prevents panning (and the subsequent fling) from happening for gestures which are predominantly pinches (i.e. large span change).

gmoore, jogandavison, would either of you be interested in looking into this?

Comment 2

[jlogandavison]

(In reply to Botond Ballo [:botond] from comment #1)

gmoore, jogandavison, would either of you be interested in looking into this?

So it's been a while since I've touched this code and I'll need to refamiliarise, but I'd certainly be happy to have a look at this :)

I think I'm witnessing the issue as described. Panning with two fingers does appear to trigger a fling in cases where a one finger pan does not. I'll start by going back over the APZ code before getting back to you with a possible cause

Comment 3

[Botond Ballo [:botond]]

Great, thanks!

Let me know if you have any questions, I'm happy to help.

Comment 4

[jlogandavison]

Attachment: patch.diff

Okay, so this behaviour occurs when OnScaleEnd initiates a pan gesture (lines 1718-1723).

To replicate, on a page where zooming is disabled:

• Place 2 fingers on the screen
• Perform a pinch gesture with your fingers travelling outwards vertically. One finger going up, the other going down.
• Lift one finger

The viewport will snap from the original focus point of the pinch to the position of the remaining touch point. If you lift your second finger fast enough, this fast pan translates into a fling.

I've provided a patch that avoids the fast pan by calling OnTouchStart with the location of the remaining touch point. Doing this doesn't trigger any test failures, but based on on-device testing, I'm not convinced that this isn't messing with intended the two finger fling behaviour.

---

I think part of the issue is that we're currently deciding between a pinch/pan gesture based on the mAllowZoom property. Meaning that, on un-zoomable pages, we're essentially misinterpreting (potentially valid) attempts to zoom the page by the user.

We have code to discriminate between pinching/panning in the pinch locking method. Would it be worth re-engineering slightly so that we initiate a pan + fling based on this instead? This could have the added benefit of enabling two-finger fling gestures on zoomable pages too, which I think would help with consistency.

Comment 5

[Botond Ballo [:botond]]

(In reply to jlogandavison from comment #4)

Okay, so this behaviour occurs when OnScaleEnd initiates a pan gesture (lines 1718-1723).

To replicate, on a page where zooming is disabled:

• Place 2 fingers on the screen
• Perform a pinch gesture with your fingers travelling outwards vertically. One finger going up, the other going down.
• Lift one finger

The viewport will snap from the original focus point of the pinch to the position of the remaining touch point. If you lift your second finger fast enough, this fast pan translates into a fling.

Thanks for investigating! This explanation makes sense.

I've provided a patch that avoids the fast pan by calling OnTouchStart with the location of the remaining touch point. Doing this doesn't trigger any test failures, but based on on-device testing, I'm not convinced that this isn't messing with intended the two finger fling behaviour.

Based on code reading, I believe this patch will have the effect of re-setting the velocity tracking at the point we transition from two fingers to one. This should indeed fix the reported issue, but I think it will regress another scenario:

• Perform a quick two-finger pan (with the two fingers moving in the same direction).
• Release the fingers one at a time but in quick succession.

Since we re-set the velocity tracker at the time the first finger is lifted, the resulting fling will not reflect the velocity built up during the two-finger
portion of the pan.

I think part of the issue is that we're currently deciding between a pinch/pan gesture based on the mAllowZoom property. Meaning that, on un-zoomable pages, we're essentially misinterpreting (potentially valid) attempts to zoom the page by the user.

We have code to discriminate between pinching/panning in the pinch locking method. Would it be worth re-engineering slightly so that we initiate a pan + fling based on this instead?

I assume you're talking specifically about this condition. (Since here, for example, we take both mAllowZoom and the pinch-locked state into account.)

I think it would make sense to take the first branch there if we're ending a true pinch (meaning, a pinch gesture for which mPinchLocked == false), even on a non-zoomable page. That way, if you were trying to perform a pinch, and then lifted one finger, you would only get a fling if you built up velocity from panning with the remaining finger.

This could have the added benefit of enabling two-finger fling gestures on zoomable pages too, which I think would help with consistency.

Doesn't that work already? I don't see what would prevent it based on a quick read, but I could be overlooking something.

Comment 6

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #5)

This could have the added benefit of enabling two-finger fling gestures on zoomable pages too, which I think would help with consistency.

Doesn't that work already? I don't see what would prevent it based on a quick read, but I could be overlooking something.

Ah, it probably doesn't work in the case where the fingers are lifted one by one (but in quick succession), because then this StartTouch() call resets the velocity tracker when the first finger is lifted.

I take it, then, that you're proposing changing the condition there not to mAllowZoom || !mPinchLocked, but just to !mPinchLocked.

I think that sounds reasonable! The current logic pre-dates pinch locking, which is why we didn't do it that way at the time.

Comment 8

[Botond Ballo [:botond]]

jlogandavison, hi! Wanted to check in to see if you're interested in continuing work on this bug :)

Comment 9

[Frank Griffith Jr]

Will include this in FFXP 1449 Pinch - Zoom Feature Repairs/Enhancements

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:botond, could you please find another way to get the information or close the bug as INCOMPLETE if it is not actionable?

For more information, please visit auto_nag documentation.

Comment 11

[Botond Ballo [:botond]]

This bug remains valid and available for interested contributors to work on.

It's also on our (APZ team) roadmap, so that if no one picks it up sooner, we'll fix it when we get to the mentioned "FFXP 1449 Pinch - Zoom Feature Repairs/Enhancements" epic (hopefully sometime in H2).

Comment 12

[Razvan Cojocaru]

FWIW, try as I might I could not reproduce this at all in the emulator. It does appear to still happen on a real Android phone with the latest Firefox though.

Comment 13

[Razvan Cojocaru]

Suggestions on the best way to write a gtest to simulate the problem (since I'm somewhat Android-challenged at the moment) are appreciated. Thanks!

EDIT: I'm guessing something along these lines?

Comment 14

[Botond Ballo [:botond]]

(In reply to Razvan Cojocaru from comment #13)

EDIT: I'm guessing something along these lines?

Yep, that's a good one to take inspiration from!

Since the steps to reproduce involve the pinch having a particular shape (mostly vertical movement of the touch points), instead of calling PinchWithTouchInput() you probably want to just send individual touch events (as in the implementation of PinchWithTouchInput()) directly from the test, with the coordinates chosen to produce this shape.

MakeApzcUnzoomable() is what you need to simulate the "page where you can't pinch zoom" part. (In terms of the code, it will ensure mZoomConstraints.mAllowZoom == false.)

To check whether the bug occurs, it should be sufficient to look at apzc->GetVelocityVector() after the fingers are lifted, and check that its magnitude is below some threshold.

Comment 15

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #14)

you probably want to just send individual touch events (as in the implementation of PinchWithTouchInput()) directly from the test

One subtlety here is that, based on jlogandavison's investigation in comment 4, reproducing the bug requires that one finger be lifted slightly earlier than the second. This is what happens most of the time during production use (i.e. even when you intend to remove both fingers simultaneously, in practice there is likely to be a small time delta and the touchscreen hardware often generates an event in that time interval).

The semantics of MULTITOUCH_END is that it should contain only the touch points that were lifted. So in this case, we want a sequence similar to:

MULTITOUCH_START (touch id 1, touch id 2)
MULTITOUCH_MOVE (touch id 1, touch id 2)
// additional moves
MULTITOUCH_END (touch id 2)  // finger 2 lifted
MULTITOUCH_END (touch id 1)  // finger 1 also lifted

Comment 16

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #5)

(In reply to jlogandavison from comment #4)

I've provided a patch that avoids the fast pan by calling OnTouchStart with the location of the remaining touch point. Doing this doesn't trigger any test failures, but based on on-device testing, I'm not convinced that this isn't messing with intended the two finger fling behaviour.

Based on code reading, I believe this patch will have the effect of re-setting the velocity tracking at the point we transition from two fingers to one. This should indeed fix the reported issue, but I think it will regress another scenario:

• Perform a quick two-finger pan (with the two fingers moving in the same direction).
• Release the fingers one at a time but in quick succession.

Since we re-set the velocity tracker at the time the first finger is lifted, the resulting fling will not reflect the velocity built up during the two-finger
portion of the pan.

It would be nice to also add a test for this scenario, to verify that it does not regress. (Here, we'd want to assert the velocity at the end is above a threshold.)

Comment 17

[Razvan Cojocaru]

(In reply to Botond Ballo [:botond] from comment #14)

(In reply to Razvan Cojocaru from comment #13)

EDIT: I'm guessing something along these lines?

Yep, that's a good one to take inspiration from!

Since the steps to reproduce involve the pinch having a particular shape (mostly vertical movement of the touch points), instead of calling PinchWithTouchInput() you probably want to just send individual touch events (as in the implementation of PinchWithTouchInput()) directly from the test, with the coordinates chosen to produce this shape.

I've looked into this, and I wonder if we wouldn't benefit more from just passing a bool vertical = false extra parameter to PinchWithTouchInput() and make it pinch vertically in that case. As for your other comment, I see that there's a PinchOptions aOptions = PinchOptions::LiftBothFingers parameter to PinchWithTouchInput() here, maybe we could add something like LiftBothFingersInSequence?

If you prefer that I just duplicate the relevant logic in the test and don't modify PinchWithTouchInput() I'll be happy to do that as well.

Comment 18

[Botond Ballo [:botond]]

(In reply to Razvan Cojocaru from comment #17)

I've looked into this, and I wonder if we wouldn't benefit more from just passing a bool vertical = false extra parameter to PinchWithTouchInput() and make it pinch vertically in that case. As for your other comment, I see that there's a PinchOptions aOptions = PinchOptions::LiftBothFingers parameter to PinchWithTouchInput() here, maybe we could add something like LiftBothFingersInSequence?

Yep, this approach sounds reasonable too.

Comment 19

[Razvan Cojocaru]

Attachment: Bug 1685648 - Prevent a fast fling when trying to pinch-zoom on a non-zoomable page. r?botond

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a4a0e5f0e1d9
Prevent a fast fling when trying to pinch-zoom on a non-zoomable page. r=botond

Comment 21

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/a4a0e5f0e1d9

Comment 22

[Botond Ballo [:botond]]

Thanks very much for fixing this, Razvan!

Comment 23

[Razvan Cojocaru]

(In reply to Botond Ballo [:botond] from comment #22)

Thanks very much for fixing this, Razvan!

Don't mention it! Thanks for the prompt and very helpful reviews!