Closed Bug 610784 Opened 9 years ago Closed 9 years ago

Handle overflow (>6 items) in the app menu

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set

Tracking

(fennec2.0next+)

VERIFIED FIXED
Firefox 5
Tracking Status
fennec 2.0next+ ---

People

(Reporter: mbrubeck, Assigned: wesj)

References

Details

(Whiteboard: [has patch][strings])

Attachments

(4 files, 3 obsolete files)

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.
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0-
tracking-fennec: 2.0- → 2.0next+
Attached patch Patch v1 (obsolete) — Splinter Review
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?
Assignee: mbrubeck → wjohnston
Attachment #521092 - Flags: review?(mbrubeck)
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.
Attachment #521092 - Flags: review?(mbrubeck)
Attached patch Patch v2 (obsolete) — Splinter Review
Patch.

A basic tests patch coming.
Attachment #521092 - Attachment is obsolete: true
Attachment #522842 - Flags: review?(mbrubeck)
Attached patch Tests (obsolete) — Splinter Review
Attachment #522843 - Flags: review?(mbrubeck)
Oh whoops. Forgot to mention I need an icon here. I'll post shots in a sec.
Attached image Menu
Attached image Screenshot 2
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.
Whiteboard: [has patch][strings]
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.
Attached patch Patch v3Splinter Review
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).
Attachment #522842 - Attachment is obsolete: true
Attachment #522938 - Flags: review?(mbrubeck)
Attachment #522842 - Flags: review?(mbrubeck)
Attached patch TestSplinter Review
Updated to check that only overflow items appear in the overflow menu.
Attachment #522843 - Attachment is obsolete: true
Attachment #522939 - Flags: review?(mbrubeck)
Attachment #522843 - Flags: review?(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.
Attachment #522938 - Flags: review?(mbrubeck)
Attachment #522938 - Flags: review+
Attachment #522938 - Flags: feedback?(mark.finkle)
Comment on attachment 522939 [details] [diff] [review]
Test

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

What's with the setTimeout?
(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 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.
Attachment #522939 - Flags: review?(mbrubeck) → review+
Blocks: 612193
Status: I am looking at the patch. Feedback soon.
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 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
Attachment #522938 - Flags: feedback?(mark.finkle) → feedback+
http://hg.mozilla.org/mozilla-central/rev/c93094805140
http://hg.mozilla.org/mozilla-central/rev/ecb9212d635f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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?
You'll need to install some add-ons that add menu items to the Android menu:
* FullScreen
* Quit Firefox Mobile
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)
Status: RESOLVED → VERIFIED
Target Milestone: --- → Firefox 5
You need to log in before you can comment on or make changes to this bug.