1269774 - Fix main/overflow menu

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: Screen Shot 2016-05-02 at 1.29.04 PM.png

Bug 1265433 landed some changes to our menu but we need these polished before it hits general.

Priorities:
 - Fix icon artifacts ("fuzziness" around the blue star - I've noticed a bit of this elsewhere in our home panels too, has something changed about these icons recently?)
 - Fix padding to spec
 - Move menu position 
 - Remove dividers

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Anthony Lam (:antlam) from comment #0)
>  - Move menu position 

bug 1189272.

Comment 2

[Andrzej Hunt :ahunt]

Here are some patches that:
- Fix the artifacts, I somehow managed to break transparency which makes the icons ugly when moving the star into the top of the menu.
- Hopefully fix padding (I'm not sure I understand all the various layouts that are interacting here, and we can't influence the padding that the share icons will have).

I'm still looking into materialising the design (menu position / dividers), but haven't gotten very far there yet.

Comment 3

[Andrzej Hunt :ahunt]

Attachment: menu_adjustedpadding.png

Here's a screenshot of the fixed bookmark star, and adjusted padding.

Comment 4

[Andrzej Hunt :ahunt]

Attachment: menu_position_padding_dividers.png

Here's an updated screenshot that hopefully exhibits all the desired behaviours.

Comment 5

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1269774 - Fix bookmark star artifacts r=sebastian

These images were incorrectly saved, leading to the loss of the smooth
edges they should have. This commit replaces the edited bookmark stars
with higher quality versions.

Review commit: https://reviewboard.mozilla.org/r/50239/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50239/

Comment 6

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1269774 - Fix padding for context menu icons r=sebastian

The icons in the first row require more padding. In the second
row the share icons should have more padding, however all other
icons should remain the same size - it's simplest to adapt the padding
by using separate styles for each type of icon.

Review commit: https://reviewboard.mozilla.org/r/50241/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50241/

Comment 7

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1269774 - Remove dividers from browser menu r=sebastian

This removes dividers from the main menu, and also the tab-tray menu.

Review commit: https://reviewboard.mozilla.org/r/50243/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50243/

Comment 8

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1269774 - Show browser menu over the menu button r=sebastian

due to different top and right paddings. These are fixed in the
next commit.

Review commit: https://reviewboard.mozilla.org/r/50245/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50245/

Comment 9

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1269774 - Reduce browser menu top padding r=sebastian

We want to have equal paddings for the top and right hand side of the
menu, whereas we previously had 24dp padding for the top, and only 8dp
on the right.

removes empty vertical space from the top of the menu background images.

Review commit: https://reviewboard.mozilla.org/r/50247/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50247/

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8748361 [details]
MozReview Request: Bug 1269774 - Fix bookmark star artifacts r=sebastian

https://reviewboard.mozilla.org/r/50239/#review47201

Please remove them from v11 and move them to the base folders (Let's clean up the v11 mess whenever we touch resources).

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: menu_bottom.png

On my Nexus 6P the menu doesn't go all the way to the bottom anymore. In theory there should be enough space to show the full menu (or at least as much menu as before).

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8748362 [details]
MozReview Request: Bug 1269774 - Fix padding for context menu icons r=sebastian

https://reviewboard.mozilla.org/r/50241/#review47203

::: mobile/android/base/resources/values/styles.xml:88
(Diff revision 1)
>      <style name="Widget.GeckoMenuListView" parent="Widget.ListView">
>          <item name="android:listSelector">@drawable/menu_item_action_bar_bg</item>
>          <item name="android:divider">@color/toolbar_divider_grey</item>
>      </style>
>  
>      <style name="Widget.MenuItemActionBar">

This style also exists in values-large-v11. Does this need to be changed too?

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8748363 [details]
MozReview Request: Bug 1269774 - Remove dividers from browser menu r=sebastian

https://reviewboard.mozilla.org/r/50243/#review47217

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8748364 [details]
MozReview Request: Bug 1269774 - Show browser menu over the menu button r=sebastian

https://reviewboard.mozilla.org/r/50245/#review47219

::: mobile/android/base/java/org/mozilla/gecko/menu/MenuPopup.java:26
(Diff revision 1)
>  public class MenuPopup extends PopupWindow {
>      private final FrameLayout mPanel;
>  
> -    private final int mYOffset;
>      private final int mPopupWidth;
>      private final int mPopupMinHeight;

It looks like mPopupMinHeight is unused too. We could just remove it here.

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8748365 [details]
MozReview Request: Bug 1269774 - Reduce browser menu top padding r=sebastian

https://reviewboard.mozilla.org/r/50247/#review47221

Comment 16

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8748351 [details]
menu_position_padding_dividers.png

Awesome! Thanks for the feedback flag :)

Can we add the 2dp corner radius to this menu?

Comment 18

[Andrzej Hunt :ahunt]

Adding corner radius will be tricky:
- Currently we're using a 9-patch drawable for the background (which also defines the padding)
- We can replace that with an xml based drawable with the appropriate radius easily
- But: the action bar is also square: even if we make its top corners rounded, it will misbehave when you scroll (everything scrolls within the rounded rectangle).

This means we'd probably need a rounded rectangle mask (or some other way of clipping) for the actual menu content (in addition to the correct background), I'm still investigating if that's possible / how to do it.

Comment 19

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Andrzej Hunt :ahunt from comment #18)
> Adding corner radius will be tricky:

Let's file a follow up for this then.

Comment 20

[Andrzej Hunt :ahunt]

Comment on attachment 8748361 [details]
MozReview Request: Bug 1269774 - Fix bookmark star artifacts r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50239/diff/1-2/

Comment 21

[Andrzej Hunt :ahunt]

Comment on attachment 8748362 [details]
MozReview Request: Bug 1269774 - Fix padding for context menu icons r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50241/diff/1-2/

Comment 22

[Andrzej Hunt :ahunt]

Comment on attachment 8748363 [details]
MozReview Request: Bug 1269774 - Remove dividers from browser menu r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50243/diff/1-2/

Comment 23

[Andrzej Hunt :ahunt]

Comment on attachment 8748364 [details]
MozReview Request: Bug 1269774 - Show browser menu over the menu button r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50245/diff/1-2/

Comment 24

[Andrzej Hunt :ahunt]

Comment on attachment 8748365 [details]
MozReview Request: Bug 1269774 - Reduce browser menu top padding r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50247/diff/1-2/

Comment 25

[Andrzej Hunt :ahunt]

Comment on attachment 8748591 [details]
menu_bottom.png

It turns out we have to set the divider height to 0 in addition to setting now divider drawable. Setting the divider to @null results in us (or android?) not considering divider height when calculating the menu height, but we still insert space for the dividers when actually building the menu. On large screens that means that we give the menu approximately 11px less space than it needs. (The issue only becomes visible if we have enough space to show the entire menu. In most cases the menu has to scroll, and we successfully use the entire screen for this.)

Comment 26

[Andrzej Hunt :ahunt]

Comment on attachment 8748363 [details]
MozReview Request: Bug 1269774 - Remove dividers from browser menu r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50243/diff/2-3/

Comment 27

[Andrzej Hunt :ahunt]

Comment on attachment 8748364 [details]
MozReview Request: Bug 1269774 - Show browser menu over the menu button r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50245/diff/2-3/

Comment 28

[Andrzej Hunt :ahunt]

Comment on attachment 8748365 [details]
MozReview Request: Bug 1269774 - Reduce browser menu top padding r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50247/diff/2-3/

Comment 29

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

From IRC:

 - Tablets: we should make this change right now for mobile first
 - seemingly missing shadow from the screenshot via irc, is this the same one :ahunt?

Comment 31

[Andrzej Hunt :ahunt]

(In reply to Anthony Lam (:antlam) from comment #19)
> (In reply to Andrzej Hunt :ahunt from comment #18)
> > Adding corner radius will be tricky:
> 
> Let's file a follow up for this then.

I'm tracking that in Bug 1271428, I'll hopefully have some screens for that tomorrow. Since it involves adding another support library I'm not sure whether we can uplift the corners, but we definitely want to uplift the improvements in this bug.

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a01ca776cb4a
https://hg.mozilla.org/integration/fx-team/rev/ab7c4938c659
https://hg.mozilla.org/integration/fx-team/rev/a347b31b8a63
https://hg.mozilla.org/integration/fx-team/rev/b99a871e2c72
https://hg.mozilla.org/integration/fx-team/rev/4067fdc3fdc0

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a01ca776cb4a
https://hg.mozilla.org/mozilla-central/rev/ab7c4938c659
https://hg.mozilla.org/mozilla-central/rev/a347b31b8a63
https://hg.mozilla.org/mozilla-central/rev/b99a871e2c72
https://hg.mozilla.org/mozilla-central/rev/4067fdc3fdc0

Comment 34

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot from 2016-05-13 10:58:42.png

Tested using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 49.0a1 (2016-05-12)

Comment 35

[Andrzej Hunt :ahunt]

Attachment: aurora_combined_patches

I'd like to uplift this bug (which fixes menu appearance and padding), along with Bug 1265433 (predecessor bug which rearranged menu items, but looks odd without the padding fixes from this bug), and Bug 1272659 which reverts one of the padding changes made in this bug.

I've rolled all 3 into one patch containing all commits from these 3 bugs.

Approval Request Comment
[Feature/regressing bug #]: Bug 1234319
[User impact if declined]: Menu looks bad due to the removal of the previous reading list button. 47 has the old menu including the reading list button, 49 has a redesigned menu, for consistency we would like to uplift the new menu to 48 to avoid the intermediate state on 48.
[Describe test coverage new/current, TreeHerder]: Manual testing, the majority of these patches have been on nightly for 1+ weeks.
[Risks and why]: Low risk: padding changes, icons moved to different locations in the menu, adjusted icons.
[String/UUID change made/needed]: none.

Comment 36

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8755994 [details] [diff] [review]
aurora_combined_patches

Polish a change, taking it before the firs beta

Comment 37

[Carsten Book [:Tomcat]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #36)
> Comment on attachment 8755994 [details] [diff] [review]
> aurora_combined_patches
> 
> Polish a change, taking it before the firs beta

seems the rollup patch has problems since it does not apply cleanly to aurora:

renaming 1269774 to menu_aurora.patch
applying menu_aurora.patch
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh menu_aurora.patch

Comment 38

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #37)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #36)
> > Comment on attachment 8755994 [details] [diff] [review]
> > aurora_combined_patches
> > 
> > Polish a change, taking it before the firs beta
> 
> seems the rollup patch has problems since it does not apply cleanly to
> aurora:
> 
> renaming 1269774 to menu_aurora.patch
> applying menu_aurora.patch
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and qrefresh menu_aurora.patch

hm maybe its also because of the different bug numbers in this patch

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c713740d109c
https://hg.mozilla.org/releases/mozilla-aurora/rev/7f4050a2fb1e
https://hg.mozilla.org/releases/mozilla-aurora/rev/f940e3a54aca
https://hg.mozilla.org/releases/mozilla-aurora/rev/13f8321f46d0
https://hg.mozilla.org/releases/mozilla-aurora/rev/ddf15775455b

Comment 40

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot from 2016-06-02 11:36:18.png

Verified as fixed using:
Devices: Moto X (Android 4.4), Nexus 7 (Android 5.1.1)
Build: Firefox for Android 48.0a2 (2016-06-01)