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

RESOLVED FIXED in mozilla2.0b8

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mbrubeck, Assigned: mbrubeck)

Tracking

Trunk
mozilla2.0b8
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0+)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

8 years ago
Created attachment 454589 [details] [diff] [review]
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.
Attachment #454589 - Flags: review?(mwu)
tracking-fennec: --- → 2.0+
(Assignee)

Comment 1

8 years ago
Created attachment 455760 [details] [diff] [review]
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.
Attachment #454589 - Attachment is obsolete: true
Attachment #454589 - Flags: review?(mwu)
(Assignee)

Updated

8 years ago
Depends on: 582269
(Assignee)

Comment 2

8 years ago
The Search key was fixed by bug 582269.  Long-press on the Menu key is still broken.
(Assignee)

Updated

8 years ago
Duplicate of this bug: 592739
(Assignee)

Updated

8 years ago
Attachment #455760 - Attachment is obsolete: true
Duplicate of this bug: 608594

Comment 6

8 years ago
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.
Attachment #487320 - Flags: review?(mbrubeck)
(Assignee)

Comment 7

8 years ago
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.
Attachment #487320 - Flags: review?(mbrubeck) → review-

Updated

8 years ago
Attachment #487320 - Flags: review?(mwu)
(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
Created attachment 487964 [details] [diff] [review]
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 :))
Attachment #487320 - Attachment is obsolete: true
Attachment #487964 - Flags: review?(mbrubeck)
(Assignee)

Comment 10

8 years ago
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?
Attachment #487964 - Flags: review?(mbrubeck) → review-
(Assignee)

Comment 11

8 years ago
Created attachment 488089 [details] [diff] [review]
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.
Attachment #487964 - Attachment is obsolete: true
Attachment #488089 - Flags: review?(mwu)

Comment 12

8 years ago
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.
Attachment #488089 - Flags: review?(mwu)
(Assignee)

Comment 13

8 years ago
Created attachment 489318 [details] [diff] [review]
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.
Attachment #488089 - Attachment is obsolete: true
Attachment #489318 - Flags: review?(mwu)
Attachment #489318 - Flags: review?(Olli.Pettay)

Comment 14

8 years ago
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.
Attachment #489318 - Flags: review?(mwu) → review+
(Assignee)

Updated

8 years ago
Blocks: 610635
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?
(Assignee)

Comment 16

8 years ago
(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.
(Assignee)

Comment 17

8 years ago
Created attachment 490225 [details] [diff] [review]
patch v5

This just renames the new atom from "Escape" to "Clear".
Attachment #489318 - Attachment is obsolete: true
Attachment #490225 - Flags: review?(Olli.Pettay)
Attachment #489318 - Flags: review?(Olli.Pettay)

Updated

8 years ago
Attachment #490225 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 18

8 years ago
Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/255ec74f08b4
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.