Closed Bug 559915 Opened 10 years ago Closed 10 years ago

Fennec should handle Menu and Search buttons on Android

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: alexp, Assigned: alexp)

References

Details

Attachments

(4 files, 3 obsolete files)

Android phones have a special Menu button, which opens a local menu in each application. Fennec should have the same - it's a good opportunity to make frequently used actions easier to access.
Suggestions what to put into such a menu are welcome.
Search button has to be handled as well.
In Android browser it brings up the Search/URL bar. It makes sense for Fennec to activate the awesomebar with autocomplete popup as well.
Assignee: nobody → alexp
Status: NEW → ASSIGNED
Summary: Fennec should handle Menu button on Android → Fennec should handle Menu and Search buttons on Android
For easier implementation Android KEYCODE_MENU and KEYCODE_SEARCH key codes have to be mapped to some DOM_VK_* codes defined in Firefox, which then will be handled in JS.
MENU obviously could be mapped to VK_CONTEXT_MENU, but there is nothing like this for the SEARCH. How about VK_HELP? Suggestions are welcome.
I would really prefer that we don't randomly overload these -- what happens if you're on a device that actually has a distinct help key and a distinct search key?  I still think we should just add new VK_* entries for the hardware buttons here.  MENU/CONTEXT_MENU is probably ok, and BACK mapping to VK_BACK is also probably fine (though the DOM equivalent here is DOM_VK_BACK_SPACE, which isn't), but the android HOME button should not map to VK_HOME -- they have very different meanings.  And the search button is its own thing.

I would check with jst to see if there's any problem with adding new DOM_VK_* bits to nsIDOMKeyEvent.idl/xblprototypehandler.  We also have NS_VK_* in nsGUIEvent.h, which currently just map to the DOM_VK_* bits, but I think the right thing to do is to create new DOM_VK_* symbols.
(In reply to comment #4)
> MENU/CONTEXT_MENU is probably ok, and BACK mapping to VK_BACK is also
> probably fine (though the DOM equivalent here is DOM_VK_BACK_SPACE, which
> isn't)

BACK key is currently mapped to VK_ESCAPE, and it totally makes sense. The dialogs even already support it.

> I would check with jst to see if there's any problem with adding new DOM_VK_*
> bits to nsIDOMKeyEvent.idl/xblprototypehandler.  We also have NS_VK_* in
> nsGUIEvent.h, which currently just map to the DOM_VK_* bits, but I think the
> right thing to do is to create new DOM_VK_* symbols.

I agree that defining a new DOM_VK_* symbol would be better. We just have to come to an agreement with everyone, as it is a common code, and I don't think we can just blindly add something, which is related to Android only.

DOM_VK_SEARCH is actually not too bad, it's a common name, which could easily apply to other platforms. By the way, Windows SDK already has VK_BROWSER_SEARCH (0xAA) defined. We could define a corresponding DOM_VK_SEARCH symbol for this code, at least it will be compatible with Windows, and we could map KEYCODE_SEARCH Android code to this.
(In reply to comment #5)
> (In reply to comment #4)
> > MENU/CONTEXT_MENU is probably ok, and BACK mapping to VK_BACK is also
> > probably fine (though the DOM equivalent here is DOM_VK_BACK_SPACE, which
> > isn't)

I don't think we want to map MENU to CONTEXT_MENU. I think CONTEXT_MENU causes the "contextmenu" event, which displays the context menu, not the site menu.
Another possibility to SEARCH/BACK/etc is to use command events.
That is what we do on Windows; if the keyboard has special
search "command", we dispatch AppCommand event which has "search" as the command.
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#3266
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1113
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1072

AppCommand isn't standardized anywhere, but FF UI makes sure that AppCommand events don't propagate to content, so web pages don't see those events.
Note, DOM 3 Events draft (which we should start implement quite soon, I hope)
does have separate key values for search/back/forward/menu/etc.
(In reply to comment #7)
> Another possibility to SEARCH/BACK/etc is to use command events.
> That is what we do on Windows; if the keyboard has special
> search "command", we dispatch AppCommand event which has "search" as the
> command.

Yes, Mark already suggested this before, and I had a look.
This approach is good from a point of view that it does not affect other platforms. But it requires some extra coding to implement this a bit more complex way, and more processing for each key in runtime.
Dispatching AppCommand event from widget code isn't hard, and I doubt
the processing of that event takes really that much time.
So I'd try that, at least for now.
Another possible (and actually the most obvious) way to handle special buttons is to map them to Fxx keys.

I know that nobody really likes this approach, as those keys don't have meaningful names, so the code using these keys is less readable, and every time requires a reference to the mapping list/table.

But unlike the named keys, with which the originally assumed action could clash with the one we want to assign, the Fxx keys are actually designed to be used for the extra functions, which don't have a special standard key, and don't really have a default assumption.
The Fxx key codes are already used on some devices for the special buttons, for example softkeys on WinMo are mapped to VK_F1 and VK_F2, Maemo generates some Fxx codes (F7/F8 for volume +/- or zoom in/out, F4 - menu button on N810, F5 - Home, F6 - Full screen).

So maybe it's not that bad, and in case of Android, such mapping is very easy to implement in our code.
Doesn't mean that those mappings are good, and again, those break down badly if someone were to ever attach any kind of hardware keyboard to the device (think tablets, for example).  Adding new keycodes isn't difficult, though I like the AppCommand approach more.
(In reply to comment #12)
> Doesn't mean that those mappings are good, and again, those break down badly if
> someone were to ever attach any kind of hardware keyboard to the device (think
> tablets, for example).  Adding new keycodes isn't difficult, though I like the
> AppCommand approach more.

Yeah, AppCommand was my first thought too. I think we should add "BACK" to AppCommand as well.
BACK maps to VK_ESCAPE really well. It's actually exactly the same meaning. As ESCAPE it is even already supported by the dialogs automatically, so I'd prefer to leave it.

As for the AppCommand, what I don't really like about this approach is that it looks like kind of an artificial superstructure above the native mechanism, making some of the originally equal buttons "less equal" than the others.
Though on the other hand such special implementation is local to a platform, and does not affect the common code. I like this.

So, I guess, for now we'll go with the AppCommand.
But I believe, for the future we should consider some easy common mechanism to define platform-specific key codes, which does not require major changes in the common components that could affect other platforms.
(In reply to comment #14)
> But I believe, for the future we should consider some easy common mechanism to
> define platform-specific key codes, which does not require major changes in the
> common components that could affect other platforms.

Yes. In the hopefully not-too-far-in-the-future we should implement DOM 3 Events KeyEvents. Hopefully that would be enough for all our special keys.
Attached patch Handle special keys in BrowserUI (obsolete) — Splinter Review
Use capitalized widget atom names to keep them consistent with other AppCommand names.
Attachment #441310 - Attachment is obsolete: true
Attachment #441311 - Attachment is obsolete: true
Attachment #441604 - Flags: review?(mark.finkle)
Attachment #441604 - Flags: review?(mark.finkle) → review+
Comment on attachment 441604 [details] [diff] [review]
Handle special keys in BrowserUI v2

browser.xul has a <key id="key_menu" ... /> that we should probably remove too. Right now, it's hooked to F4 which is an application menu button on the N810, but the N900 has no such button.
Don't we support N810?
(In reply to comment #22)
> Don't we support N810?

We do for Fennec 1.1, but this is not going into Fennec 1.1

I'm also worried that F4 could be hooked to something else on the N900 and beyond.
Removed F4 from browser.xul
Attachment #441604 - Attachment is obsolete: true
Attachment #441721 - Flags: review+
I thought I'd said this in here already, but looks like no.  My suggestions for the menu and search buttons are:

menu button - open the site menu;  if the titlebar is offscreen, float it.

search button - focus the awesomebar
(In reply to comment #25)
> menu button - open the site menu;  if the titlebar is offscreen, float it.
> search button - focus the awesomebar

That's exactly how it works now with this patch.
Comment on attachment 441603 [details] [diff] [review]
Convert special keys to the AppCommand events v2

>+    case AndroidKeyEvent::KEYCODE_MENU:
>+    case AndroidKeyEvent::KEYCODE_SEARCH:
>+    case AndroidKeyEvent::KEYCODE_VOLUME_UP:
>+    case AndroidKeyEvent::KEYCODE_VOLUME_DOWN:
>+        if (isDown) {
>+            HandleSpecialKey(ae);
>+            return;
>+        }
>+        break;
I think we'd actually prefer to send the event on up. See the "Key events executed on key-up" section of http://developer.android.com/sdk/android-2.0.html for more details. I'll make this change in the commit.

>diff --git a/widget/src/xpwidgets/nsWidgetAtomList.h b/widget/src/xpwidgets/nsWidgetAtomList.h
>+WIDGET_ATOM(Menu, "Menu") // AppCommand to open a menu
> WIDGET_ATOM(menu, "menu") // Represents an XP menu
Hmm I think we can just overload the menu atom that's already here, with a comment change. Not sure.
(In reply to comment #27)
> I think we'd actually prefer to send the event on up. See the "Key events
> executed on key-up" section of
> http://developer.android.com/sdk/android-2.0.html for more details. I'll make
> this change in the commit.

I agree with that. Thanks for the note.

> >+WIDGET_ATOM(Menu, "Menu") // AppCommand to open a menu
> > WIDGET_ATOM(menu, "menu") // Represents an XP menu
> Hmm I think we can just overload the menu atom that's already here, with a
> comment change. Not sure.

This one is questionable. I wanted to reuse the existing "menu", but discussed this with Olli and he recommended to add a new capitalized atom to make it consistent with other app commands, which are also capitalized (for example "Search", which I reused here).
Duplicate of this bug: 564887
Flags: in-litmus?
Fennec now handle Menu and Search buttons on Android. 

Retested bug with:
Build id : Mozilla/5.0 (Android;Linux armv7l; rv:9.0a1)Gecko/20110927
Firefox/9.0a1 Fennec /9.0a1
Device: Motorola DROID 2
OS: Android 2.3

Device Menu button displays a menu with the following options:
Site options
Preferences
Add-ons
Share Page
Find in Page
More

Device search button opens Awesome screen or search engines list if Awesome screen is already displayed.

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