Closed Bug 785994 Opened 7 years ago Closed 7 years ago

Group menu items into smaller Submenus

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 18
Tracking Status
firefox18 --- verified

People

(Reporter: sriram, Assigned: sriram)

References

Details

Attachments

(4 files, 2 obsolete files)

The menu list is keeping on growing day by day. It's better to group the items into submenu -- so that we have a folded menu approach that doesnt need scrolling.
Assignee: nobody → sriram
OS: Mac OS X → Android
Hardware: x86 → ARM
Attached patch WIP: Code complete (obsolete) — Splinter Review
The implementation is code complete.
Implementation notes:
* Android provides a class SubMenu and I've used it for compatibility purposes.
* MenuInflater is enhanced to handle submenus
* Submenu items doesn't need an "id" attribute. Their only job is to show a submenu -- which the code handles.
* Items grouped under the submenu share the same id as their counterparts in pre-honeycomb. Hence no change in code-path for onPrepareOptionsMenu() or onOptionItemsSelected().

However there are few things lacking in it:
1. Do we use "..." or ">" (image) to denote submenus?
   - If we use ">", then we need a image.
2. The submenu items are created by us, which gives us an opportunity to add icons to them. In that case, we need icons.
   - Additionally, submenu can have headers (like when showing folded menu). Do we want that?
3. I am not sure of the grouping yet. What menu items go under what sub menu items?
4. If we are going to have only scrolling list of items, I would suggest using an adapter instead of the current <ScrollView><LinearLayout/></ScrollView> approach.
Attachment #655699 - Flags: feedback?(mark.finkle)
(In reply to Sriram Ramasubramanian [:sriram] from comment #0)
> The menu list is keeping on growing day by day. It's better to group the
> items into submenu -- so that we have a folded menu approach that doesnt
> need scrolling.

How is the interaction with the folded items? Could you please post a screenshot or point to UI specs somewhere?
There are no UI specs as such. (Or I don't know where Ian has mockups).
The lists shows up exactly as how it is. A list item with submenu, shows its list.. just the same as original list.. in the same popup window.
Hitting back first clears out any submenu and goes to main menu. A back again will dismiss menu.
Comment on attachment 655699 [details] [diff] [review]
WIP: Code complete

Looking good to me
Attachment #655699 - Flags: feedback?(mark.finkle) → feedback+
Attached patch Patch (obsolete) — Splinter Review
This patch adds upon the WIP to add the support for tablets too.

Open ended questions:
1. What all are grouped together under what header (what menu item name) ?
2. Do we use a ">" or "..."?

Known bug hard to fix:
So, either we can have the touch outside to dismiss the menu -- or have back button to go to parent menu from a sub menu.
Problem is: PopupWindow cannot listen to key events. Hence there is no way to track onBackPressed(). So, the MenuPanel needs to gain focus to receive keyevents and hence dismiss popup on Back button. In this case, PopupWindow doesn't have the focus, hence cannot be dismissed on pressing outside (basically the UI in the BrowserApp can receive touch events -- and start processing it -- say like a click on webpage element). On the other hand, if PopupWindow has the focus, we can tap outside to dismiss it. But MenuPanel loses focus, so it cannot capture Back button to go one level up.

Probable fix:
Back button on tablet is at a diagonally opposite place to the menu. And it feels awkward to tap it to go one level back. Probably we can have a header that can go one level back.

This patch dismissing the popup, hence back doesnt go one level back.
Attachment #655699 - Attachment is obsolete: true
Attachment #659381 - Flags: review?(mark.finkle)
Oooooh.. we need animations too!
(In reply to Sriram Ramasubramanian [:sriram] from comment #5)

> Open ended questions:
> 1. What all are grouped together under what header (what menu item name) ?

I pitched this idea to Ian:
Page >
  Reading List
  Save as PDF
  Find in Page

Tools >
  Add-ons
  Downloads
  Apps


I'm not sure about moving "Bookmark" and  "Share" under "Page". It could work though.

It's arguable that we could add "Settings" under "Tools" but I am not dead set on doing it.

Ian - More thoughts?

> 2. Do we use a ">" or "..."?

I would like to avoid using "..." for submenus. What are the known problems with using ">"?
New mockup: http://cl.ly/image/0q2640360H2P

Yeah, I would like to keep Bookmark and Share out as their own things, since they will both launch submenus of their own. 

From what I understand, using a > to disclose a secondary menu is frowned upon on Android (too iOS-like). There actually doesn't seem to be a standard indicator for this. But seeing that all of our menu items (except for forward and refresh) consistently take you to either a secondary menu or a different place, they probably don't need any arrow or ellipsis-like indicator to differentiate themselves.
Sriram, please confirm that these are the assets you'll need to make these changes, and I'll start putting them together for you:

Sizes: MDPI-XHDPI

ICS / JB 
- Forward icon
- Refresh icon
- Bookmark icon

GB and earlier (for a 3x2 icon menu tray)
- Forward icon
- Refresh icon
- Bookmark icon
- Share icon
- Page icon
- More
(In reply to Ian Barlow (:ibarlow) from comment #8)
> New mockup: http://cl.ly/image/0q2640360H2P

The only pushback I have on this mockup is the "submenu" for Share. On Android, it's not really a menu. It's an OS UI that is used for sharing. It also has the "[ ] Use as default" functionality.

I'd rather keep the share list as the native OS UI.
Android's Gallery / Picture viewer actually has a menu like this
(In reply to Ian Barlow (:ibarlow) from comment #11)
> Android's Gallery / Picture viewer actually has a menu like this

Yes, it does. It has some most-recently-used features built-in. That is beyond the scope of this bug. File a new bug to add that kind of support.
(In reply to Ian Barlow (:ibarlow) from comment #8)
> New mockup: http://cl.ly/image/0q2640360H2P
> 
> Yeah, I would like to keep Bookmark and Share out as their own things, since
> they will both launch submenus of their own. 
> 
> From what I understand, using a > to disclose a secondary menu is frowned
> upon on Android (too iOS-like). There actually doesn't seem to be a standard
> indicator for this. But seeing that all of our menu items (except for
> forward and refresh) consistently take you to either a secondary menu or a
> different place, they probably don't need any arrow or ellipsis-like
> indicator to differentiate themselves.

From what I see, this is same as the PopupMenu that Android supports natively from 11+. We can use that (or in fact, it by itself can use this), than writing our custom menu. Some things that would change with that approach are:
1. It's hard to do the 3 icons on top. I haven't figured out how chrome does it.
2. The submenu items will show up as a "ContextMenu" like dialog box, instead of showing as a PopupMenu.

If we want our custom menu, with the submenu also showing up in the same PopupWindow, I am up for it too.

Also, there is a bit of usability issue. As a user, I would like the empty bookmark to fillup, when I tap on the star. I wouldn't be expecting a newer menu with "Reading List" in it. To me, "Reading List" is the same as Bookmark, even though the internal manipulation may be so. We might need to re-think about grouping them under a star.

Regarding the share provider: There *might* be two ways to achieve it. I guess we can get the list of apps that can listen for this particular "share" mime type, and we can list it ourselves. Or, we can change the way the "Share Dialog" is shown. <-- this needs more exploration.
(In reply to Sriram Ramasubramanian [:sriram] from comment #13)
> Also, there is a bit of usability issue. As a user, I would like the empty
> bookmark to fillup, when I tap on the star. I wouldn't be expecting a newer
> menu with "Reading List" in it. To me, "Reading List" is the same as
> Bookmark, even though the internal manipulation may be so. We might need to
> re-think about grouping them under a star.
With XUL Fennec, we would bookmark when you tapped the star (and the page wasn't starred), and then show a "submenu" with "Unbookmark", "Edit", and "Add to Homescreen". We could do something pretty similar here. If it is starred we could just show the submenu.
(In reply to Wesley Johnston (:wesj) from comment #14)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #13)
> > Also, there is a bit of usability issue. As a user, I would like the empty
> > bookmark to fillup, when I tap on the star. I wouldn't be expecting a newer
> > menu with "Reading List" in it. To me, "Reading List" is the same as
> > Bookmark, even though the internal manipulation may be so. We might need to
> > re-think about grouping them under a star.
> With XUL Fennec, we would bookmark when you tapped the star (and the page
> wasn't starred), and then show a "submenu" with "Unbookmark", "Edit", and
> "Add to Homescreen". We could do something pretty similar here. If it is
> starred we could just show the submenu.

It's the same behavior in desktop too. However, it was found out of research (done by an intern, Brian) that it's more clunkier and that's not what the users expect (a big list of things to do for adding a bookmark).
This is how the menu will look: http://cl.ly/image/2B313N3t3r3q
Alright, I think we hashed this out in IRC but just posting here as well for posterity. Updated menu:  http://cl.ly/image/0i0C0W3q0i0y (icons are WIP) Couple things there to note. 

1. You're probably right about the bookmark button. Lets pull any kind of reading list reference out of there for now, so no secondary bookmark menu. You can still add things to the reading list from Reader, which is enough until we make a good UX to add them from somewhere else too.

2. Icons. I am fine to add them in. I thought they were unnecessary on such a small list, but icons are a useful indicator, and would also lend themselves nicely to customization, when we enable dragging an icon up into the toolbar
This patch uses a list view with header for the custom menu.
Attachment #659381 - Attachment is obsolete: true
Attachment #659381 - Flags: review?(mark.finkle)
Attachment #661892 - Flags: review?(mark.finkle)
This patch adds support for submenu.
Attachment #661894 - Flags: review?(mark.finkle)
This patch removes reading list from the menu.
Attachment #662227 - Flags: review?(mark.finkle)
Comment on attachment 661892 [details] [diff] [review]
Patch (1/4): Use list

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

>     public void setValues(ParsedItem item, MenuItem menuItem) {
>         menuItem.setChecked(item.checked)
>                 .setVisible(item.visible)
>                 .setEnabled(item.enabled)
>                 .setCheckable(item.checkable)
>+                .setCheckable(item.checked)
>                 .setIcon(item.iconRes)
>                 .setShowAsAction(item.showAsAction ? 1 : 0);

This looks wrong. We already call setChecked using item.checked and setCheckable using item.checkable

>diff --git a/mobile/android/base/resources/values-xlarge-v11/styles.xml b/mobile/android/base/resources/values-xlarge-v11/styles.xml

>     <!-- Lists in AwesomeBar -->
>-    <style name="AwesomeBarList" parent="android:style/Widget.Holo.ListView">
>-        <item name="android:layout_width">fill_parent</item>
>-        <item name="android:layout_height">fill_parent</item>
>-        <item name="android:layout_weight">1</item>
>+    <style name="AwesomeBarList" parent="@style/GeckoList">
>         <item name="android:paddingLeft">52dp</item>
>         <item name="android:paddingRight">52dp</item>
>-        <item name="android:background">#EEF1F5</item>
>-        <item name="android:divider">#D1D5DA</item>
>-        <item name="android:dividerHeight">1dp</item>
>-        <item name="android:cacheColorHint">#EEF1F5</item>
>-        <item name="android:listSelector">@drawable/action_bar_button</item>
>     </style>

What are these changes for?

r+ with these fixed
Attachment #661892 - Flags: review?(mark.finkle) → review+
Attachment #661894 - Flags: review?(mark.finkle) → review+
Attachment #662227 - Flags: review?(mark.finkle) → review+
Depends on: 792253
Depends on: 792273
Whiteboard: [leave open]
This patch replaces the arrows and the background to fit properly with the color we use.
Attachment #662654 - Flags: review?(mark.finkle)
Attachment #662654 - Flags: review?(mark.finkle) → review+
Pushed the arrows to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5820bc7dfa3a
Whiteboard: [leave open]
Comment on attachment 661892 [details] [diff] [review]
Patch (1/4): Use list

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature
User impact if declined: A long list of menu items.
Testing completed (on m-c, etc.): Landed in m-c on 09/19
Risk to taking this patch (and alternatives if risky): None. This uses a better ListView.
String or UUID changes made by this patch: None.
Attachment #661892 - Flags: approval-mozilla-aurora?
Comment on attachment 661894 [details] [diff] [review]
Patch (2/4): Add submenu support

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature
User impact if declined: A long list of menu items.
Testing completed (on m-c, etc.): Landed in m-c on 09/19
Risk to taking this patch (and alternatives if risky): None. This add support for submenu.
String or UUID changes made by this patch: Added a new string "Tools".

+<!ENTITY tools "Tools">
Attachment #661894 - Flags: approval-mozilla-aurora?
Comment on attachment 662227 [details] [diff] [review]
Patch (3/4): Remove reading list

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature
User impact if declined: Reading list will be shown in the menu.
Testing completed (on m-c, etc.): Landed on m-c on 09/19
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.
Attachment #662227 - Flags: approval-mozilla-aurora?
Comment on attachment 662654 [details] [diff] [review]
Patch (4/4): Replace arrows

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature
User impact if declined: No pretty looking arrows.
Testing completed (on m-c, etc.): Landed on m-c on 09/19
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.
Attachment #662654 - Flags: approval-mozilla-aurora?
Blocks: 793197
Comment on attachment 661892 [details] [diff] [review]
Patch (1/4): Use list

[Triage Comment]
I don't see a product team request to uplift this functionality - this would be more for QA to test on a shorter timeline. Please re-nominate if the product team feels otherwise.
Attachment #661892 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #661894 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #662227 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #662654 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Can this please be uplifted for Fx 17? This is a big usability issue that I would like to see in the product asap.
Changes were applied on the latest Nightly build. Save as PDF, Add-ons, Downloads and Apps options were added to Tools submenu. Also Reading List option was removed. Closing bug as verified fixed on:

Firefox 18.0a1 (2012-09-24)
Device: Galaxy Note
OS: Android 4.0.4
Status: RESOLVED → VERIFIED
Depends on: 794088
Depends on: 793136
No longer depends on: 794088
Depends on: 794088
Depends on: 794581
Duplicate of this bug: 789764
Depends on: 799043
You need to log in before you can comment on or make changes to this bug.