575351 - Long-press on Android system keys (Menu, Search) does not work in Fennec

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Matt Brubeck (:mbrubeck)]

Attachment: partial fix

Android has the following system-wide shortcuts when you press and hold the system keys:

* Menu: Open onscreen keyboard.
* Home: View recent applications.
* Search: Search by voice.

The Home shortcut already works in Fennec.  This patch fixes the Search shortcut.

The Menu shortcut still does not work for some reason.  Menu long-press works if I return false from onKeyDown, but then our regular Menu key handler stops working, even if I remove the mSpecialKeyTracking check from nsWindow.cpp.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

Updated patch which also removes the mSpecialKeyTracking code on the Gecko side.  Long press on Menu key works now, but the onKeyUp is not suppressed as it is for the long press on the Search key.  This is because of some difference in the Android framework.  Still working on this.

Comment 2

[Matt Brubeck (:mbrubeck)]

The Search key was fixed by bug 582269.  Long-press on the Menu key is still broken.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Comment 6

[Michael Wu [:mwu]]

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

Matt, can you take a look/test this?

This will likely need additional review by smaug or roc since you're adding widget atoms.

Comment 7

[Matt Brubeck (:mbrubeck)]

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

The standard behavior for the menu button is to show the keyboard on long press.  This doesn't work in Fennec right now, but I think we should fix that rather than user it for a different purpose.

Also, this patch breaks Menu-key shortcuts on hardware keyboards, because the KEYCODE_MENU keydown event is not sent to Gecko.

Sending a different command for long-touch on Back is a good idea.  We should keep that part.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #7)
> Comment on attachment 487320 [details] [diff] [review]
> Patch
> 
> The standard behavior for the menu button is to show the keyboard on long
> press.  This doesn't work in Fennec right now, but I think we should fix that
> rather than user it for a different purpose.

I will fix that

> Also, this patch breaks Menu-key shortcuts on hardware keyboards, because the
> KEYCODE_MENU keydown event is not sent to Gecko.

Yeah this is something I was saying into #c0 of bug 608594, I don't have a device with an hardware keyboard to test so I've tried to bypass that by using the FLAG_VIRTUAL_HARD_KEY in the LongPress method but it looks like this is not doing what I want :s

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

I still can't test the hardware button cases since I don't have a device for that.
Matt, I have move the VIRTUAL_HARDWARE check into the KeyDown event listener to allow the KeyDown to be propagated, could you confirm me it works? (it was my question on IRC :))

Comment 10

[Matt Brubeck (:mbrubeck)]

Comment on attachment 487964 [details] [diff] [review]
Patch v0.2

>+++ a/embedding/android/GeckoApp.java
>+            case KeyEvent.KEYCODE_MENU:
>+              if (event.getRepeatCount() == 0 && 
>+                     (event.getFlags() & KeyEvent.FLAG_VIRTUAL_HARD_KEY) != 0) {
>+                  event.startTracking();
>+                  return true;
>+              }
>+              return false;

This still breaks hardware menu-key shortcuts, since we "return false" and don't reach sendEventToGecko.

Also, I get the ContextMenu event even for the non-virtual menu key on my hardware keyboard.  It seems that FLAG_LONG_PRESS is set even if you haven't called startTracking().

Finally, we should *always* call startTracking() and return "true" on the first keydown, otherwise the isTracking check will fail in onKeyUp and we will break normal keypresses for that case.

This seems to work:

            case KeyEvent.KEYCODE_MENU:
                if (event.getRepeatCount() == 0)
                    event.startTracking();

                // Ignore long press on HW menu key, since we use it for shortcuts.
                if ((event.getFlags() & KeyEvent.FLAG_VIRTUAL_HARD_KEY) == 0 &&
                    (event.getFlags() & KeyEvent.FLAG_LONG_PRESS) != 0)
                {
                    return true;
                }
                break;

>     public boolean onKeyUp(int keyCode, KeyEvent event) {
>         switch(keyCode) {
>             case KeyEvent.KEYCODE_BACK:
>+            case KeyEvent.KEYCODE_MENU:
>                 if (!event.isTracking() || event.isCanceled())
>                     return false;
>                 break;
>             default:
>                 break;
>         }
>         GeckoAppShell.sendEventToGecko(new GeckoEvent(event));
>         return true;
>     }

This has some bad effects too.  The C++ widget code never receives a keyup event after a long press, so it thinks that the Menu key is still down.  So if you do (1) long press menu; (2) release menu; (3) press "q"; then this will be treated as a shortcut (ctrl+q).  I'm not sure the best way around this.

>+            case KeyEvent.KEYCODE_MENU:
>+                GeckoAppShell.forceIME();

forceIME() does not actually do anything in my testing.  Is it broken, or is there something else we need to call?

>+++ a/widget/src/xpwidgets/nsWidgetAtomList.h
> WIDGET_ATOM(disabled, "disabled")
>+WIDGET_ATOM(Escape, "Escape") // AppCommand to return to the initial state
> WIDGET_ATOM(_false, "false")

I had to add ContextMenu here again, for this patch to compile.  Or did you mean to remove the ContextMenu code from nsWindow.cpp too?  Do we want to call forceIME *and* send a ContextMenu command?

>+++ a/widget/src/android/nsWindow.cpp
>+                if (gMenu) {
>+                    gMenuConsumed = PR_FALSE;
>+                }

If we use isTracking() to suppress keyup events in Java, then we can remove the gMenuConsumed variable in C++.

Since there are a lot of hardware-keyboard-only issues here, maybe I should try updating this patch?

Comment 11

[Matt Brubeck (:mbrubeck)]

Attachment: patch v0.3

This is an updated version of Vivien's patch that fixes the problems noted above.  I've tested it on the T-Mobile G2 (HTC Desire Z).  Single-taps, shortcuts, and long-press on the virtual hard key all work as expected.

I didn't have to use FLAG_VIRTUAL_HARD_KEY, because when the hardware keyboard is available the virtual keyboard is automatically suppressed.  (This is good, because we want this to work on phones with "real" system buttons, not just touchscreen ones.)

I had to use InputMethodManager.SHOW_FORCED to force the virtual keyboard to show on the G2.  However, long-press on Menu in other apps also does not show the keyboard on this phone, so this might be expected.  Maybe we should stick with SHOW_IMPLICIT for consistency, and accept that it is disabled on some phones.

Comment 12

[Michael Wu [:mwu]]

Comment on attachment 488089 [details] [diff] [review]
patch v0.3

Please use 4 space indent in these java files. Space after switch.

For the forceIME stuff, I suspect you don't need the timer there since it's not being triggered by gecko. Gecko needs that since it can end up sending unnecessary IME enable/disables but since this is being handled entirely within the java wrapper, we shouldn't need that. Since that's not needed, you can just inline the forceIME stuff.

I can't review new atoms in widgets. You probably need someone like roc or smaug for that. I'd also consider doing something like alt-escape or something to indicate a long back so we can test it on desktop, but I really don't know what the right answer is here.

Comment 13

[Matt Brubeck (:mbrubeck)]

Attachment: patch v4

> Please use 4 space indent in these java files. Space after switch.

Done.

> For the forceIME stuff, I suspect you don't need the timer there since it's not
> being triggered by gecko. Gecko needs that since it can end up sending.

Done.  Still works fine without the timer.

> I can't review new atoms in widgets. You probably need someone like roc or
> smaug for that. I'd also consider doing something like alt-escape or something
> to indicate a long back so we can test it on desktop, but I really don't know
> what the right answer is here.

Adding smaug to review the new atom.  I have no strong opinion on what it should be named, especially since we haven't decided exactly what the UI action for "long press on back button" should be.  (In the stock browser, it opens the history list.)

Synthesizing an alt-escape keyup/keydown pair sounds fine too, if you think that's better.  For testing of "command" events on desktop, we usually just add an alternate key combo that triggers the same command.

Comment 14

[Michael Wu [:mwu]]

Comment on attachment 489318 [details] [diff] [review]
patch v4

I recommend merging the forceIME code into the calling function. Looks fine otherwise. Just need a better idea of what to do with long press back now.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 489318 [details] [diff] [review]
patch v4

So what kind of button is AndroidKeyEvent::KEYCODE_BACK?
Escape sounds a bit strange, since it sounds like Esc key.
Is AndroidKeyEvent::KEYCODE_BACK something similar to Symbian's C (clear)?
Or is it more context dependent?

Would Reset or Clear work as the atom name?

Comment 16

[Matt Brubeck (:mbrubeck)]

(In reply to comment #15)
> So what kind of button is AndroidKeyEvent::KEYCODE_BACK?
> Escape sounds a bit strange, since it sounds like Esc key.
> Is AndroidKeyEvent::KEYCODE_BACK something similar to Symbian's C (clear)?
> Or is it more context dependent?

I'm not that familiar with Symbian, but I think it is similar.  "Back" is one of the four standard Android system buttons.  It returns the user to the previous page or activity.  In the browser it acts like the "back" navigation button where appropriate, but it is also used to dismiss dialogs and to return to the previous application or screen.

> Would Reset or Clear work as the atom name?

Clear sounds like a good name to me.

Comment 17

[Matt Brubeck (:mbrubeck)]

Attachment: patch v5

This just renames the new atom from "Escape" to "Clear".

Comment 18

[Matt Brubeck (:mbrubeck)]

Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/255ec74f08b4