940997 - The new quick share row in the menu looks visually unbalanced

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

From IRC:

<lucasr> ibarlow, first a more high-level impression: the composition looks a bit unbalanced (visually)
<lucasr> ibarlow, there's too much weight at the top of the menu now
<lucasr> and the two first rows don't have the same height which is kinda strange
<lucasr> and the separators are different too
<lucasr> the visual flow looks a bit inconsistent
<lucasr> maybe it wouldn't feel this way if the share buttons were, say, in the middle of the menu or something
<lucasr> but because they come just after the top buttons, your brain kinda expects them to feel like the same type of thing
<lucasr> what about the separators?
<ibarlow> either the two rows need to be separated, or more visually distinct from each other
<lucasr> different bg color?
<mfinkle> maybe a thicker separator line for the top row of buttons?
<ibarlow> yeah that's kind of what i was thinking. different background colour, possibly a thicker separator between the rows

Comment 1

[Ian Barlow (:ibarlow)]

Lucas and I landed on this as the new menu design that cleans up a lot of visual clutter. http://cl.ly/image/3z3Y1V241J3H

Changes in these designs:
* Remove arrow pointer from the top of the menu
* Use different background colour for top row
* Add a back arrow into menu
* Place Bookmark and Share on the same row
* Add a "more" arrow next to the generic share icon
* Replace airplane with generic share icon
* Populate quick share items to the left of the share icon instead of the right 
* Remove all other menu list icons (new tab, find in page etc)

Note that tablets will behave a little differently here

* Small tablets will simply hide the row with [back/fwd/refresh], since those controls are in the title bar
* For large tablets, we'll need to decide whether we want to move the quick share items out into the title bar to sit with [bookmark], or if we want to leave them in the menu.

Comment 2

[Lucas Rocha (:lucasr)]

Wes, could you have a look at this one? You're probably more familiar with this code than me.

Comment 3

[Lucas Rocha (:lucasr)]

We decided to revert FF28 to use the single quickshare UI (see bug 946233). Re-nominating.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch 1/12 - Basic color/divider changes

Uploading WIP patches.

These are just basic coloring/divider changes.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch 2/12 - Remove the old bookmark menu item

This removes the old bookmark item

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch 3/12 - Allow handling action item clicks

This lets action item's assign listeners that can cancel the normal handling of clicks.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch 4/12

This adds "back" to the menu.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch 5/12 - Always create menu items

This may need to be reworked. It forces addActionButton to return the new view, and forces it to always create/return one. It also modifies the order views are added.

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch 6/12 - Allow adding custom actions

This allows you to add your own, non-intent actions to the provider. These are treated a bit different than normal items. i.e. They're always shown and don't reorder themselves. I think that matches what we want with "Bookmark" (i.e. "Bookmark" should never be booted from the menu if you don't use it?) but wouldn't necessarily match what we'd want with ALL of these types of items.

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch 7/12 - Add a "bookmark" action to the row.

This adds the bookmark action

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch 7/12 - Add a "bookmark" action to the row.

Updated

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Patch 8/12 - Remove the arrow from the menu

This just removes the arrow from this menu.

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Patch 9/12 - Add a "more" arrow to the icon

This adds the "more" arrow to the action menu button, even when its in icon mode.

Comment 14

[Wesley Johnston (:wesj)]

Attachment: Patch 10/12

Make the number of action items shown before we flip the share icon to "small" mode configurable. I'm probably trying to hard to support the old configuration here, but I wanted these changes to be easy to turn on/off while we work on 'em.

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Patch 11/12 - Don't show icons in menuitems

This disables icons in menus. It uses a static const to determine what we do. I don't love that. Looking for smarter ways...

Comment 16

[Wesley Johnston (:wesj)]

Whoops. There was no patch 12 :)

Comment 18

[Wesley Johnston (:wesj)]

This kills icons in the long share menu. We should probably not do that (its hard to identify apps without icons).

Comment 19

[Aaron Train [:aaronmt]]

(In reply to Ian Barlow (:ibarlow) from comment #1)
> Changes in these designs:
> * Replace airplane with generic share icon

This bug also covering the one used in the reader-mode toolbar?

Comment 20

[Sriram Ramasubramanian [:sriram]]

Someone had already changed the share icon. I don't know who/how.

Comment 21

[Sriram Ramasubramanian [:sriram]]

Architectural design & concerns:
--------------------------------

As per the new mockup,
1. The first row has the browser controls - back/forward/refresh.
2. The second row has 'save' related controls.
   -- Bookmark as an icon.
   -- Share (and frequently used apps) as icons, provided by an ActionProvider.

In the earlier design, the first row icons were treated as "action-items". The "Share" was a special kind of row, as it was backed by an ActionProvider. The first row was as "header" to the ListView that shows the menu. Note: The "Share" was a row in the ListView.

What this means:
1. The header is a View, that has ImageViews added to it. They are not backed by an adapter. They live in the app's lifecycle always.
2. The ListView is inflated, only when the menu is about to be shown. This is backed by an adapter, and the UI is inflated only when an user presses the "menu" button.
   -- The "Share" is a row in the ListView. This means, the menu will ask the provider to give the "latest" View, based on the Intent and the apps, right before the ListView is about to be shown. In other words, this row is inflated only once -- right before UI is shown -- every time the ListView is shown.

What changes now:
1. The first row has a blue background to group few controls.
2. The second row has the usual color.
   -- Bookmark and Share occupies the same row.

Proposal:
1. As earlier, the first row can still represent the "action-items". They will now represent only the "action-items" that have a value of "always" for "showAsAction". This way, either the url-bar or the ICS-action-mode or this first row can be an ActionItemBarPresenter. Everything works as before without any problem.
2. The second row can represent "action-items" that have a value of "ifRoom" for "showAsAction".
   -- Strictly speaking, "ifRoom" doesn't mean this. However, we made a decision to use only android's menu attributes in menu.xml. This helps in parsing the XML files. So, re-using "ifRoom" seems a better option.
   -- Every menu will have a "secondary ActionItemBarPresenter" that shows the "ifRoom" action-items. These entries will never show up in the primary presenter -- url-bar, first row in main menu, ICS-action-mode.
   -- Since they are still action-items, more than one MenuItem can share a single row.
   -- ActionItemBarPresenter is still a(nother) header in the ListView.

This approaches works and produces an output like above.

Problems:
1. Since the Presenter is a "header" for the ListView, the views are held in the app's lifecycle always. They are not backed by the ListView's adapter.
2. Bookmark and Share are "action-items" and not rows in the ListView.
3. Whenever there is a change in the webpage shown, like location change, we call invalidateOptionsMenu() to trigger update of menu items. This is what ensures the proper state of action-items -- like reflecting if a page is bookmarked or not.
4. This invalidateOptionsMenu() will now cause inflate the "Share" menu again and again, as this is an "action-item".
   -- This doesn't seem good for performance. There could be as much as 8-10 invalidateOptionsMenu() for a simple page load.
   -- Entries in the ListView are not affected as they are not inflated-and-refreshed until shown.
5. Making "bookmark" another entry in ActionProvider, and making Share just another row in ListView doesn't feel right.
   -- Quick share is available only 14+
   -- Messing up with the history.xml (of the ActionProvider) by injecting our own entries doesn't feel safe and good.

To summarize:
1. Having "bookmark" and "share" in the same row makes it use "action-items" concept.
2. This causes these views to be in memory always.
3. "share" and the app-icons are re-inflated every time something changes in the webpage. Typically 8-10 for a page load.

Options:
1. They both can be in two different rows, so they are part of ListViews as before.
   -- In the future, if "bookmark", "add to reading list", "add to home screen" are grouped into a single row, then that could be backed by an ActionProvider. Share will be backed by an ActionProvider. And each of these entries "backed by an ActionProvider" will take one row each in the ListView. And they won't be inflated-or-refreshed unless they are shown.

Comment 22

[Sriram Ramasubramanian [:sriram]]

Attachment: Part (1/2): Change first row

This patch changes the look and feel of first row in the custom menu. Additionally a "back" menu is added. Hence, in this patch, the bookmark is downgraded to be just another row in the main menu. (this will be reverted in next patch -- but for completion sake, this is so, in this patch).

Comment 23

[Sriram Ramasubramanian [:sriram]]

Attachment: Part (2/2): Support "ifRoom"

As per IRC discussions, the second row has entries from menu items that describes themselves as "ifRoom" action-items. This patch works as expected.

Comment 24

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

Comment on attachment 8349694 [details] [diff] [review]
Part (2/2): Support "ifRoom"

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

>+        if (historySize > dataModel.getActivityCount()) {
>+            return view;
>+        }

Add a comment about  why this code is returning early without adding an action button

Someday we need to document how the GeckoMenu system works, why it exists and how the main menu uses "action bar".

Comment 25

[Sriram Ramasubramanian [:sriram]]

Attachment: Part (3/4): Remove icons

This removes icons from the main menu. These are shown only for the GeckoActionProvider's sub menu (basically the apps).

The actual PNG resources are left behind -- as discussed.

Comment 26

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/integration/fx-team/rev/0e84f8ba1333
https://hg.mozilla.org/integration/fx-team/rev/63cac76f5f7f
https://hg.mozilla.org/integration/fx-team/rev/90699ba761fb

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0e84f8ba1333
https://hg.mozilla.org/mozilla-central/rev/63cac76f5f7f
https://hg.mozilla.org/mozilla-central/rev/90699ba761fb

Comment 28

[Sriram Ramasubramanian [:sriram]]

Attachment: Part (4/4): Magic!

There's so much magic in here!

As per Ian's mockup,
1. Center the "Share" icon, place the arrow at 5dp to its right.
2. If the width is too small, and the arrow is getting pushed to its edges, then center the "share + arrow" as a whole.

And that required a lot of math. Had to use a paint for disabled option, as we are using the bitmap directly.

The arrow is off-centered a bit. That's a resource problem and will be fixed by replacing images.

Comment 29

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

(In reply to Sriram Ramasubramanian [:sriram] from comment #28)

> The arrow is off-centered a bit. That's a resource problem and will be fixed
> by replacing images.

Have you asked for new images?

Also, a different question: Is it possible to display the share image, ">", in a disabled state? When using Guest Browsing, only the bookmark star is shown on that row of the menu, since sharing is not allowed in as a Guest. It makes the menu look a bit weird. If we could show the disabled share image, it might be more balanced.

Maybe Ian has other ideas for that situation.

Comment 30

[Ian Barlow (:ibarlow)]

Here are some new arrow images, Sriram http://cl.ly/0s0P2I2t012c Just bear in mind that this will also affect the "Tools" arrow, you'll need to move that down a bit because it's already centered with the current assets we use.

(In reply to Mark Finkle (:mfinkle) from comment #29)
> Also, a different question: Is it possible to display the share image, ">",
> in a disabled state? When using Guest Browsing, only the bookmark star is
> shown on that row of the menu, since sharing is not allowed in as a Guest.
> It makes the menu look a bit weird. If we could show the disabled share
> image, it might be more balanced.
> 
> Maybe Ian has other ideas for that situation.

Thats a good point. I would suggest either showing disabled Share, *or* hiding the bookmarks option from Guest Browsing as well (do guests really need to be able to bookmark?).

Comment 31

[Sriram Ramasubramanian [:sriram]]

(In reply to Ian Barlow (:ibarlow) from comment #30)
> Here are some new arrow images, Sriram http://cl.ly/0s0P2I2t012c Just bear
> in mind that this will also affect the "Tools" arrow, you'll need to move
> that down a bit because it's already centered with the current assets we use.
> 
> (In reply to Mark Finkle (:mfinkle) from comment #29)
> > Also, a different question: Is it possible to display the share image, ">",
> > in a disabled state? When using Guest Browsing, only the bookmark star is
> > shown on that row of the menu, since sharing is not allowed in as a Guest.
> > It makes the menu look a bit weird. If we could show the disabled share
> > image, it might be more balanced.
> > 
> > Maybe Ian has other ideas for that situation.
> 
> Thats a good point. I would suggest either showing disabled Share, *or*
> hiding the bookmarks option from Guest Browsing as well (do guests really
> need to be able to bookmark?).

Disabling share is a better option. I was discussing the possibility of having each row show either text or icon. But that's going to bloat the hierarchy a lot. I guess, it's safe to just disable the share.

Comment 32

[Sriram Ramasubramanian [:sriram]]

Attachment: Part (5/5): Replace arrows

There was an issue with translation. Since we should not move the background, but only the image, "onDraw" should be overriden. That's fixed in this.

I can fold this with the other one and push it.

Comment 33

[Sriram Ramasubramanian [:sriram]]

Folded and pushed:
https://hg.mozilla.org/integration/fx-team/rev/b1550c3edd5d

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 35

[Ben Bucksch (:BenB)]

Attachment: UXproposal.png

Copy of http://cl.ly/image/3z3Y1V241J3H/o for archival purposes, because external services can go away.

Comment 36

[Ioana Chiorean]

Nightly 29.0a1 2014-01-14 
I can see the changes (mentioned in the UX proposals) in the latest build. 

Is this going to be uplifted to Aurora too?

Comment 37

[Lucas Rocha (:lucasr)]

(In reply to Ioana Chiorean from comment #36)
> Nightly 29.0a1 2014-01-14 
> I can see the changes (mentioned in the UX proposals) in the latest build. 
> 
> Is this going to be uplifted to Aurora too?

Nope, we've reverted the feature entirely from 28 (see bug 946233)

Comment 38

[Ioana Chiorean]

Thanks Lucas, saw the bug only after I've post the comment. 

Setting bug as verified fixed.