703279 - Panning is still enabled after the Context Menu is triggered if the touch was not released

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P4)

Description

[Cristian Nicolae (:xti)]

Mozilla/5.0 (Android;Linux armv7l;rv:11.0a1)Gecko/20111117
Firefox/11.0a1 Fennec/11.0a1
Devices: Samsung Galaxy Nexus S
OS: Android 2.3.4

Steps to reproduce:
1. Open Fennec App
2. Browse to www.yahoo.com
3. Long tap on any link, but don't release your finger on the screen
4. After the Context Menu is triggered, pan the page around without release your finger

Expected result:
Panning should be disabled after the Context Menu is triggered.

Actual result:
Panning is still active at step 4. It will be disabled only after the finger touch will be released

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

I remember this same bug in XUL Fennec

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch

This is the super simple way to fix this. Unfortunately, it doesn't take into account whether we actually show a context menu or not. The alternative is to do this in browser.js I think. We'll need some message from JS to turn off the PanZoomController (if it doesn't already exist)?

Comment 3

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

Comment on attachment 591221 [details] [diff] [review]
Patch

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

This seems like it would leave the page stuck in overscroll if you did a long press while the page was doing a bounce-back. Did you test that scenario?

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Better patch

This seems better to me. Basically it kills touching and snaps us back any time a dialog is shown. That means the context menu item will slip out from under your finger but not until we're already showing the menu. And in places where do don't show a menu, things should behave normally.

And it should save us from other weird cases where we might show a dialog but still enable panning (select elements maybe? although they don't show until you life your finger).

Comment 5

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

Comment on attachment 591243 [details] [diff] [review]
Better patch

as discussed on IRC cancel() needs to run on the UI thread

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch

Dispatching to what (I think?) is the right thread. Added a comment as well.

Comment 7

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

Comment on attachment 595218 [details] [diff] [review]
Patch

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

::: mobile/android/base/gfx/LayerController.java
@@ +475,5 @@
>          mView.requestRender();
>      }
> +
> +    public void cancelTouches() {
> +        mPanZoomController.cancel();

This call should be done inside a runnable dispatched to the UI thread.

::: mobile/android/base/ui/PanZoomController.java
@@ +356,5 @@
> +            public void run() {
> +                cancel();
> +            }
> +        });
> +        return false;

onTouchCancel is already called from the UI thread, so this runnable is not needed here.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch

We have more infrastructure to help with this now!

Comment 9

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

Comment on attachment 638004 [details] [diff] [review]
Patch

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

I don't know about this... injecting events into the TouchEventHandler like this is just asking for trouble. The number of calls to defaultPrevented has to match exactly or the balance will get out of whack. Also, doesn't the PromptService get called for other things too?

How about just setting | mState = PanZoomState.NOTHING; | in PanZoomController.onLongPress?

Comment 10

[Wesley Johnston (:wesj)]

Hmm. Probably good to be worried. Just doing something on long tap won't work either (i.e. not every long tap shows a context menu).

Comment 11

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

How about adding another function to PanZoomController called abortPanning()? All it needs to do is set mState to NOTHING and call bounce(), and then you can call that on the UI thread from PromptService.show().

Actually I'm kind of surprised that android continues to deliver touch events to the LayerView once the context menu is up. Maybe we should correct that by calling setFilterTouchesWhenObscured(false) on the LayerView.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Patch v22

I hate this bug.

setFilterTouchesWhenObscured(true) in LayerView.onCreate() doesn't seem to do anything.

Comment 13

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

Comment on attachment 643222 [details] [diff] [review]
Patch v22

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

LGTM assuming that runnable gets posted to the UI thread.

::: mobile/android/base/PromptService.java
@@ +200,5 @@
>          return promptServiceResult;
>      }
>  
>      public void show(String aTitle, String aText, PromptButton[] aButtons, PromptListItem[] aMenuList, boolean aMultipleSelection) {
> +        mController.post(new Runnable() {

In the versions of PromptService I have lying around there's no mController.

Comment 14

[Wesley Johnston (:wesj)]

Heh. fixed that and forgot to qref before uploading.

Comment 15

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b835a85752cb

and forgot to qref again before pushing: https://hg.mozilla.org/integration/mozilla-inbound/rev/6b1ef3183423

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b835a85752cb
https://hg.mozilla.org/mozilla-central/rev/6b1ef3183423

Comment 17

[Paul Feher]

Verified on:
Nightly 18.0a1 (2012-09-11)
Aurora 17.0a2 (2012-09-11)
Device: Samsung Galaxy Tab 2 7.0
OS: Android 4.0.4

The panning action is disabled after the context menu is triggered but the     pinch-zoom action is still enabled. For this issue a new bug was opened bug790245