739412 - [TABLET] Overflow menu

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

Attachment: Mockup

Let's make a custom overflow menu, similar to what we have on our current version of Firefox on tablets

Specs and assets (and some possible menu reordering) to follow

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

-ing to keep triage lists clean

Comment 2

[Ian Barlow (:ibarlow)]

Attachment: Mockup of menus across tablets and phones

Attached is a more up to date menu mockup. The tablet menu has been changed slightly from its original design, it is now a single column list. 

The original 3-icon-across design, while nice looking, doesn't work very well unless there are exactly 3 items in each row. Which is often not the case, particularly once we start allowing add-ons to have space in the menu. 

More detailed specs and assets will follow soon.

Comment 3

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP

This patch is almost done. In tablets, the action bar items are lost on rotation, that should be fixed.
And I started with android's MenuInflater as a base for GeckoMenuInflater. I need to revisit it and re-write it.
Also, cleanup other places.

Few decisions taken in code:
1. This is enabled only for 11+ devices. Other uses the default by calling super.xyz() methods. So gingerbread will have "5+more" menu. If needed, removing the check for >=11 will enable in all devices.

2. values/attrs.xml holds the styleable values for menu. Android doesn't expose them. They are internal and can be referenced only as "com.android.internal", which we don't have access to. Hence I've re-added them all there.

3. "showAsAction" is an enum in android specification. However, we either show an icon or not. Hence, I've made it a boolean in our case. Same in the case of "checkable"

4. GeckoMenuItem cannot extend a layout directly. As it has to implement MenuItem.
In MenuItem: public MenuItem setEnabled(boolean enabled);
In View: public void setEnabled(boolean enabled);
The return type changes, causing it to leave the layout implementation to be handled by something else.

5. Since we need a icon+text for normal menu and icon only for action-bar, there are 2 different implementations, implementing GeckoMenuItem.Layout.

6. In devices with ICS "and" h/w menu button, the panel shows up in a android panel called "FEATURE_OPTIONS_PANEL". We just need to inject the layout into it and it takes care of showing it appropriately (right near the menu button). In case of honeycomb devices and ICS devices with soft menu button, we need to show a popup. MenuPanel is for condition #1. MenuPopup encloses the MenuPanel for condition #2.

7. GeckoMenu should know where to add the action-bar items. Hence, if we support action-bar items (only in case of tablets), we register mBrowserToolbar with the menu, so that, when the item is specified as "showAsAction", it adds to the action-bar.

8. The "showAsAction" can change anytime in the code. Since we are implementing Menu, we need to use "add" to add the MenuItem. At this point, MenuItem doesn't know if its an action-bar item or not. Hence it directly adds to the menu. Now when it knows its an action-bar item, it changes its layout. It then informs the listener (this case the menu). The GeckoMenu removes it from its layout and its the new layout to the action-bar supplied by #7.

9. The way to position arrow in MenuPopup is super hacky. I'll file a followup. This works for now ;)

10. There are magic numbers floating around in GeckoMenuItemDefault and GeckoMenuItemActionBar. I'll move it to dimens.xml

11. Since the tail goes away, and the overflow-items are liberated from the evil-clutches of android, we can use repeating-bitmap in honeycomb (v. 11 and 12). Hence the action-bar background's hacky code is removed now. yaay!

Comment 4

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

Comment on attachment 627252 [details] [diff] [review]
WIP

In general, this looks OK to me. There is a lot going on and we'll do a more complete review when it's ready to land.

I am concerned about needed to subclass so many of the Android menu classes. I am worried that we'll need to keep updating those subclasses as Android changes its API. It might be something we just need to do.

On an OCD note, I am torn over the naming convention we started where we use GeckoXxx for so many things. Since everything we do is Gecko, it seems redundant sometimes. I'd be happy to move any from the GeckoXxx (and we have for some things). Maybe it is a good idea to use GeckoXxx when we are overriding a built-in class.

Comment 5

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch is complete. GeckoMenuInflater has the minimal code required to parse a menu file and inflate it. The names have been changed: anything that overrides / implements Android class is named as GeckoXxx, and the rest as just Yyy.

The MenuPopup that holds the MenuPanel in honeycomb/ics with no h/w button, is moved in BrowserToolbar. (In android, action-bar takes care of the menu. Hence in our case, BrowserToolbar holding it made sense). The code is cleaner by allowing BrowserToolbar to handle it.

values-xlarge/styles.xml and values-xlarge/themes.xml are no more needed, as we use our own menu button. Same holds for values-sw600dp/*.*

Comment 6

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

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

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>         public boolean onMenuItemClick(MenuItem item) {
>             Log.i(LOGTAG, "menu item clicked");
>             GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Menu:Clicked", Integer.toString(id)));
>+            ((Activity) GeckoApp.mAppContext).closeOptionsMenu();

This seems a little hacky to me. I know we need to close the menu after the click, but I am not happy it's in the click handler. I can let this go for now, but we should think about a better approach. Perhaps we could do something in the GeckoMenuItem or GeckoMenu clasees? File a bug?

>     public boolean onCreateOptionsMenu(Menu menu)

>+        // Inform the menu about the action-items bar. 
>+        if (menu instanceof GeckoMenu && isTablet())
>+            ((GeckoMenu) menu).setActionItemBarPresenter(mBrowserToolbar);

Not a fan of the isTablet() check here, but we can address it later I think

>+    public void openOptionsMenu() {
>+        if (mBrowserToolbar.hasSoftMenuButton()) {
>+            onPreparePanel(Window.FEATURE_OPTIONS_PANEL, mMenuPanel, sMenu);
>+            mBrowserToolbar.openOptionsMenu();
>+        } else {
>+            super.openOptionsMenu();
>+        }

I wonder if we could move the hasSoftMenuButton() check into the mBrowserToolbar.openOptionsMenu() call and have that method return | true | if it opened the menu, | false | if not. Like this:

          if (mBrowserToolbar.openOptionsMenu())
              onPreparePanel(Window.FEATURE_OPTIONS_PANEL, mMenuPanel, sMenu);
          else
              super.openOptionsMenu();

Followup bug is OK too.

>+    public void closeOptionsMenu() {
>+        if (mBrowserToolbar.hasSoftMenuButton())
>+            mBrowserToolbar.closeOptionsMenu();
>+        else
>+            super.closeOptionsMenu();
>+    }

I wonder if we could move the hasSoftMenuButton() check into the mBrowserToolbar.closeOptionsMenu() call and have that method return | true | if it closed the menu, | false | if not. Like this:

          if (!mBrowserToolbar.closeOptionsMenu())
              super.closeOptionsMenu();

Followup bug is OK too. Same as the one above.

>+    public boolean onCreatePanelMenu(int featureId, Menu menu) {

>+            if (mMenuPanel == null) {
>+                mMenuPanel = (MenuPanel) onCreatePanelView(featureId);
>+            }

{} not needed


r+ but it's a big patch and I could have missed something, so lets get it landed and tested.

Comment 8

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/202d63933120

Comment 9

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #6)
> Comment on attachment 628512 [details] [diff] [review]
> Patch

> 
> >+    public boolean onCreatePanelMenu(int featureId, Menu menu) {
> 
> >+            if (mMenuPanel == null) {
> >+                mMenuPanel = (MenuPanel) onCreatePanelView(featureId);
> >+            }
> 
> {} not needed
> 
> 
> r+ but it's a big patch and I could have missed something, so lets get it
> landed and tested.

I thought of removing this, but a comment in "else" is sticking there -- which requires a {}. Hence preserving these two for the neatness of code.

Comment 10

[Sriram Ramasubramanian [:sriram]]

Filed bug 760217 for moving opening/closing custom menu check to BrowserToolbar.

Comment 11

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch fixes a crash on Nexus S running ICS (with menu key).
Basically the older code didn't initialize custom menu, if there is no soft menu button. This fixes it.

Comment 12

[Matt Brubeck (:mbrubeck)]

Backed out along with bug 760196 because it may have caused startup crashes related to mysterious missing drawable resources:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa02228ce9ab

Comment 13

[Sriram Ramasubramanian [:sriram]]

Pushed again:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c92becb066e4
by merging both the patches.

The issue was that there aren't any pre-honeycomb resources. Hence it crashes on 2.2 devices (tbpl). The second patch I posted does the MenuPanel initialization only for post-honeycomb devices, hence we won't run into any crash.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c92becb066e4

Comment 15

[Andreea Pod]

Verified fixed on:
-build: Firefox for Android 22.0a1 (2013-02-20)
-device: Asus Transformers TF101
-OS: Android 4.0.3