Closed Bug 1570559 Opened 2 years ago Closed 2 years ago

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

Categories

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

69 Branch
Unspecified
Android
defect

Tracking

()

VERIFIED FIXED
mozilla71
Tracking Status
firefox-esr68 71+ verified
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- verified

People

(Reporter: ekager, Assigned: botond)

References

Details

(Whiteboard: [fennec68.3])

Attachments

(4 files)

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

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?

Component: General → Panning and Zooming
OS: All → Android
Product: GeckoView → Core

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.

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.

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

Priority: -- → P2

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

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)

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

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

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

Assignee: nobody → botond

Depends on D45083

Pushed by bballo@mozilla.com:
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

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

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

Flags: needinfo?(ekager)
Flags: needinfo?(botond)

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

Flags: needinfo?(ekager)

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.

Flags: needinfo?(botond)
Flags: needinfo?(sarentz)

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
Flags: needinfo?(botond)
Attachment #9090489 - Flags: approval-mozilla-esr68?

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.

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

Whiteboard: [fennec68.3]

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.

Attachment #9090489 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

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

Flags: needinfo?(botond)

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.

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).

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

Flags: needinfo?(pascalc)
Flags: needinfo?(pascalc)
Attachment #9090489 - Flags: approval-mozilla-esr68+

(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.

Flags: needinfo?(botond)

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.

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

Flags: needinfo?(sarentz)
See Also: → 1604741

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

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.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.