1570559 - Pinch zooming in on a picture from Amazon triggers the context menu

Status: VERIFIED FIXED

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

Description

[Emily Kager [:ekager]]

Copied from: https://github.com/mozilla-mobile/fenix/issues/3431

Able to reproduce in Fenix and Fennec. Not able to reproduce in Chrome.

Steps to reproduce

1. Navigate to : Amazon.de link
2. Tap on the product picture
3. Pinch zoom in

Expected behavior

Zoom in without triggering the image context menu

Actual behavior

Zooms in triggering the context menu

Device information

• Android device: Google Pixel 3 (Android 9), Samsung Galaxy S6 (Android 6.0.1), Xiaomi MiPad 3 (Android 7.0)

• Fenix version: 1.0.1924(Build#11640609) 13/06/19

Note

Video

Comment 1

[Chris Peterson [:cpeterson]]

Sending to the APZ component. snorp says APZ should suppress context menu event

Or does GV need to disable the long press handler? Or is this touch event webcompat bug in the content?

Comment 2

[Botond Ballo [:botond]]

I think what's happening here is that the content is handling the pinch rather than APZ, and preventDefault()ing most but not all of the touch events. The remaining touch events look to the APZ gesture detector like a long-tap, and trigger a context menu event.

Comment 3

[Botond Ballo [:botond]]

More specifically, what's happening is:

• The page has a root scroll frame R, and also a subframe S. Each has its own gesture detector.
• We get a touch-start E1 with one touch point, and it starts an input block A. The target for A is S. We forward E1 to content to see if it will prevent-default it.
• We get a touch-start E2 with two touch points, and it starts an input block B. The target for B is R (since multi-touch events are always routed to the root scroll frame). We forward E2 to content to see if it will prevent-default it.
• Content does not prevent-default E1 (it only wants to provide custom behaviour for two-finger gestures). E1 is sent to S. S's gesture detector starts a long-tap timeout task.
• Content does prevent-default E2. E2 is not sent to R. Instead, a touch-cancel is sent to R. The touch-cancel clears R's gesture detector's state, but leaves S's gesture detector's state alone.
• Meanwhile, we're receiving two-finger touch-move events. They all have R as their target, they're all prevent-defaulted by content, and they all cause additional touch-cancels to be sent to R. Nothing further is sent to S or its gesture detector, though.
• Eventually, S's gesture detector reaches its long-tap timeout, and fires the long-tap at S, triggering a context menu event.

Comment 4

[Botond Ballo [:botond]]

Anyways, for triage purposes, this is definitely an APZ bug. I'll have to think about how to fix it.

Comment 5

[Botond Ballo [:botond]]

Question: how do we avoid this in cases where content is not prevent-defaulting the events?

Comment 6

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

I think that in the not-prevent-default case, we switch the active APZC here which clears the gesture detector state here. If the new block is prevent defaulted, we go through this branch which clears the state on the new APZC but not on the old one. We probably want to call mLastActiveApzc->ResetTouchInputState(); somewhere on that branch too. Not sure if it would need additional guards like in UpdateActiveApzc.

(This is mostly a guess from reading the code on searchfox, will need verification)

Comment 7

[Botond Ballo [:botond]]

Kats, it seems you are correct and your suggestion fixes the problem. Thanks :)

Comment 8

[Botond Ballo [:botond]]

Attachment: Bug 1570559 - When the target APZC changes mid-gesture, always clear the old APZC's gesture state. r=tnikkel

Previously we were only doing this is content wasn't prevent-defaulting the
events targeting the new APZC.

Comment 9

[Botond Ballo [:botond]]

Posted the fix for now but I'd like to write a test as well.

Comment 10

[Botond Ballo [:botond]]

Attachment: Bug 1570559 - Add a helper function APZCTreeManagerTester::UpdateHitTestingTree(). r=tnikkel

Depends on D44712

Comment 11

[Botond Ballo [:botond]]

Attachment: Bug 1570559 - Move helper functions that are only used in TestTreeManager.cpp into that file. r=tnikkel

Depends on D45082

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1570559 - Add a gtest. r=tnikkel

Depends on D45083

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bea7e40d295e
When the target APZC changes mid-gesture, always clear the old APZC's gesture state. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/61cedc44f752
Add a helper function APZCTreeManagerTester::UpdateHitTestingTree(). r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/95f44aea61eb
Move helper functions that are only used in TestTreeManager.cpp into that file. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/7b6c7a490d9f
Add a gtest. r=tnikkel

Comment 14

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/bea7e40d295e
https://hg.mozilla.org/mozilla-central/rev/61cedc44f752
https://hg.mozilla.org/mozilla-central/rev/95f44aea61eb
https://hg.mozilla.org/mozilla-central/rev/7b6c7a490d9f

Comment 15

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

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 16

[Julien Cristau [:jcristau]]

Does fenix need this in beta70? Can you request uplift if so?

Comment 17

[Emily Kager [:ekager]]

I don't think we need uplift. Getting it with 71 should be fine! Thanks everyone! :)

Comment 18

[Sorina Florean [:sflorean]]

Tested on Fennec Nightly esr68 and the issue is not fixed. I think you should request an uplift in order that this fix gets to Fennec users since all Fennec builds are esr based.
For Fenix, the issue is fixed and verified on Nightly.

ni also for Stefan for his input regarding Fennec.

Comment 19

[Botond Ballo [:botond]]

Comment on attachment 9090489 [details]
Bug 1570559 - When the target APZC changes mid-gesture, always clear the old APZC's gesture state. r=tnikkel

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Bug affecting Fennec, which only built from the esr68 branch, on a popular website (Amazon)
• User impact if declined: When trying to zoom in on an Amazon product picture, a context menu spuriously appears.
• Fix Landed on Version: 71
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): A small change, localized to APZ code, and has been on central for over a month, has been verified in Fenix.
• String or UUID changes made by this patch: none

Comment 20

[Botond Ballo [:botond]]

I'm fine with uplifting this to esr68, it's a small, low-risk patch. Approval request above ^

Note, only the first patch needs to be uplifted. The remaining patches are test infrastructure and the test itself.

Comment 21

[Chris Peterson [:cpeterson]]

Adding [fennec68.3] whiteboard tag because this uplifted fix would ship in Fennec ESR 68.3 (December 3).

Comment 22

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9090489 [details]
Bug 1570559 - When the target APZC changes mid-gesture, always clear the old APZC's gesture state. r=tnikkel

Was on 71 for a month with no reported issue Uplift approved for esr68, thanks.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/b0b995198592

Comment 24

[Eliza Balazs]

Hi!
I tested this on RC 68.2.0, Beta 68.3b1, Nightly 68.3a1 (2019-11-02) with Samsung Galaxy Note 8 (Android 9), Sony Xperia Z5 Premium (Android 7.1.1) and the issue is still reproducible.
Screenshot: https://drive.google.com/open?id=16JtzZiSsgMQXMLfK4Q5Z3hxuTGJFgz9L
Video: https://drive.google.com/file/d/1macAMzA0NbrsptkeawtIpY3qpk3bnVHZ/view?usp=sharing

Comment 25

[Chris Peterson [:cpeterson]]

I tested this on RC 68.2.0, Beta 68.3b1, Nightly 68.3a1 (2019-11-02) with Samsung Galaxy Note 8 (Android 9), Sony Xperia Z5 Premium (Android 7.1.1) and the issue is still reproducible.

In that case, I am resetting status flag firefox-esr68=affected.

Comment 26

[Diana Rus]

Hi, the issue is reproducible on Firefox Beta 68.3b3 with OnePlus 6T(Android 9), Samsung Galaxy S9(Android 8.0.0) and Sony Xperia Z5 (Android 7.0).

Comment 27

[Andreea Pavel [:apavel]]

Pascal can you remove the approval-mozilla-esr68+ so this doesn't show up in the esr68 uplift query?

Comment 28

[Botond Ballo [:botond]]

(In reply to Eliza Balazs from comment #24)

I tested this on RC 68.2.0, Beta 68.3b1, Nightly 68.3a1 (2019-11-02) with Samsung Galaxy Note 8 (Android 9), Sony Xperia Z5 Premium (Android 7.1.1) and the issue is still reproducible.

I tried with the same nightly (2019-11-02) on a Moto G5 (Android 8.1). I found the issue reproduces at a low frequency, perhaps 1 out of 10 pinch gestures. I haven't been able to spot a pattern of what specifically triggers it.

So, there may indeed be a remaining issue there, but the behaviour should have improved significantly compared to before the fix.

Comment 29

[Laurentiu Apahidean]

I have retested the issue on 68.3.0 RC using a Samsung Galaxy S9 (Android). Compared to 68.2.1 the behavior has improved significantly I can zoom in and out without triggering the image context menu most of the time. I can still reproduce the initial issue but a lot rarer this may be because my pinch in/out gesture is interpreted as a long tap.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Let's move that to a new bug then to avoid complicating tracking.

Comment 31

[Diana Rus]

Hi, a new bug was created, Bug1604741, in order to easily monitor the tracking.

Comment 32

[Sorina Florean [:sflorean]]

Based on comments 29 and 31 I will change the status for this bug to VERIFIED.
Note that the issue on Fenix was verified as fixed.