610784 - Handle overflow (>6 items) in the app menu

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

When our Android-style app menu (bug 606565) contains more than six items, we should display the first five items plus a "More" button.  Pressing "More" should display the remaining items in a popup dialog.  The behavior should match the native Android menu behavior as much as possible.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Here's a crack at this.

I'm a bit leery of this since, these sorts of changes might not translate well to tablets. Maybe we need to take a different approach to this?

Comment 2

[Wesley Johnston (:wesj)]

Comment on attachment 521092 [details] [diff] [review]
Patch v1

Whoops. I don't think this will handle dynamically removed/hidden/shown entries very well. Removing review request.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Patch.

A basic tests patch coming.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Tests

Comment 5

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

Screenshots?

Comment 6

[Wesley Johnston (:wesj)]

Oh whoops. Forgot to mention I need an icon here. I'll post shots in a sec.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Menu

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Screenshot 2

Comment 9

[Matt Brubeck (:mbrubeck)]

The "more" popup in native Android apps shows only the items that *aren't* shown in the initial menu (i.e., it does not include the first 5 items).  I think we should emulate this in our menu.

The native Android "more" popup is also aligned to the bottom of the screen, but I don't feel too strongly about this - we could file a follow-up bug to polish the look and feel.

Comment 10

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

I'd like the secondary "menu" to look more Android-like and less like our <select> UI. We can land that as a followup, even as a different bug.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Sounds good to me. This only shows overflow items in the "More" menu. I also moved all the images into css here, mostly because I like keeping that stuff in CSS (should be nice if we ever have platform specific themes, or start seeing themes on AMO).

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Test

Updated to check that only overflow items appear in the overflow menu.

Comment 13

[Matt Brubeck (:mbrubeck)]

Comment on attachment 522938 [details] [diff] [review]
Patch v3

>+  showAsList: function showAsList() {
>+    // allow menu to hide to remove the more button before we show the menulist
>+    setTimeout((function() {
>+      this.menu.menupopup.children = this.overflowMenu;
>+      MenuListHelperUI.show(this.menu);
>+    }).bind(this), 0)
>+  },

Is the setTimeout still needed, now that we are not including the whole menu in the menupopup?

>+  menu : {
>+    id: "appmenu-menulist",
>+    dispatchEvent: function(aEvent) {
>+      let menuitem = AppMenu.overflowMenu[this.selectedIndex];
>+      if (menuitem)
>+        menuitem.click();
>+    },
>+    menupopup: {
>+      hasAttribute: function(aAttr) { return false; }
>+    },
>+    selectedIndex: -1
>+  },

I'm slightly worried that this is too clever (we'll need to update it whenever we change MenuListHelperUI to access different properties of the popup element).  On the other hard, it lets us reuse a bunch of code.  Maybe we should have a followup to add a more abstract API to MenuListHelperUI.  Requesting mfinkle's feedback on this part.

>+richlistitem.appmenu-addons-button > image,
>+richlistitem.appmenu-preferences-button > image,
>+richlistitem.appmenu-downloads-button > image,

You can remove the "> image" from these selectors, because list-style-image is inherited: https://developer.mozilla.org/en/writing_efficient_css#Rely_on_inheritance

I like the idea of showing images in the overflow menu, but our low-quality image scaling (bug 598736) makes them look a bit poor.  Native Android apps do not show images in the overflow menu.  I'd lean toward getting rid of them.

Comment 14

[Matt Brubeck (:mbrubeck)]

Comment on attachment 522939 [details] [diff] [review]
Test

>+function test() {
>+  waitForExplicitFinish();
>+  setTimeout(runNextTest, 200);
>+}

What's with the setTimeout?

Comment 15

[Wesley Johnston (:wesj)]

(In reply to comment #14)
> Comment on attachment 522939 [details] [diff] [review]
> Test
> 
> >+function test() {
> >+  waitForExplicitFinish();
> >+  setTimeout(runNextTest, 200);
> >+}
> 
> What's with the setTimeout?

Whoops. Copy and paste error. Not sure why browser_awesomescreen.js is using it, but I can pull it out.

Comment 16

[Matt Brubeck (:mbrubeck)]

Comment on attachment 522939 [details] [diff] [review]
Test

The tests look good to me.  Some more minor suggestions:

>+  run: function() {

Could you add a one-sentence comment describing the flow of this test?  (e.g. "The test will keep adding buttons and checking the result until there are a total of 7 buttons, then it will hide one item and check the result again.")

>+  moreShown: function(aEvent) {

This functions should also verify that the appmenu is hidden.

>+    is(listbox.childNodes.length, 2, "Menu popup only shows overflow children");

It would be good to go to 8 or 9 nodes, and verify that the overflow menu continues to grow correctly.

Comment 17

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

Status: I am looking at the patch. Feedback soon.

Comment 18

[Matt Brubeck (:mbrubeck)]

This is the icon used by stock Android for the "More" button:

http://android.git.kernel.org/?p=platform/frameworks/base.git;a=blob_plain;f=core/res/res/drawable-hdpi/ic_menu_more.png;hb=HEAD

I'm not sure if we can ship this ourselves (Android source code is under the Apache license), or if there's a good way to access it from our Java code.

Comment 19

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

Comment on attachment 522938 [details] [diff] [review]
Patch v3


>+  createMoreButton: function() {
>+    button.setAttribute("oncommand", "AppMenu.showAsList();");

button.addEventListener("oncommand" ... }


I'm not in love with the "show" attribute, in addition to the "count" attribute, but I can live with it for now.

r+ with the addEventListener change

Comment 20

[Wesley Johnston (:wesj)]

http://hg.mozilla.org/mozilla-central/rev/c93094805140
http://hg.mozilla.org/mozilla-central/rev/ecb9212d635f

Comment 21

[Anna (Waverley)]

Can you please provide some steps to reproduce on how to obtain more then 6 items in the application menu? On a clean build I can only see four: "Site Options", "Preferences", "Add-ons", "Downloads". I install "Quit Fennec for Mobile" and I can see 5... 

What else should I install so that to have more than six items?

Comment 22

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

You'll need to install some add-ons that add menu items to the Android menu:
* FullScreen
* Quit Firefox Mobile

Comment 23

[Anna (Waverley)]

VERIFIED FIXED on:

Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:2.2a1pre) Gecko/20110411 Firefox/4.2a1pre Fennec /4.1a1pre 

Device: Motorola Droid 2 (Android 2.2)