Closed Bug 1269774 Opened 8 years ago Closed 8 years ago

Fix main/overflow menu

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox48 verified, firefox49 verified)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox48 --- verified
firefox49 --- verified

People

(Reporter: antlam, Assigned: ahunt)

References

Details

Attachments

(11 files, 1 obsolete file)

129.12 KB, image/png
Details
91.11 KB, image/png
antlam
: feedback-
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
101.41 KB, image/png
Details
47.99 KB, image/png
Details
58.50 KB, patch
Details | Diff | Splinter Review
94.45 KB, image/png
Details
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
Flags: needinfo?(ahunt)
(In reply to Anthony Lam (:antlam) from comment #0)
>  - Move menu position 

bug 1189272.
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.
Flags: needinfo?(ahunt)
Attached image menu_adjustedpadding.png (obsolete) —
Here's a screenshot of the fixed bookmark star, and adjusted padding.
Assignee: nobody → ahunt
Status: NEW → ASSIGNED
Here's an updated screenshot that hopefully exhibits all the desired behaviours.
Attachment #8748296 - Attachment is obsolete: true
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/
Attachment #8748361 - Flags: review?(s.kaspari)
Attachment #8748362 - Flags: review?(s.kaspari)
Attachment #8748363 - Flags: review?(s.kaspari)
Attachment #8748364 - Flags: review?(s.kaspari)
Attachment #8748365 - Flags: review?(s.kaspari)
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/
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/
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/
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 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).
Attachment #8748361 - Flags: review?(s.kaspari) → review+
Attached image 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).
Attachment #8748591 - Flags: feedback?(ahunt)
Attachment #8748362 - Flags: review?(s.kaspari) → review+
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 on attachment 8748363 [details]
MozReview Request: Bug 1269774 - Remove dividers from browser menu r=sebastian

https://reviewboard.mozilla.org/r/50243/#review47217
Attachment #8748363 - Flags: review?(s.kaspari) → review+
Attachment #8748364 - Flags: review?(s.kaspari) → review+
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 on attachment 8748365 [details]
MozReview Request: Bug 1269774 - Reduce browser menu top padding r=sebastian

https://reviewboard.mozilla.org/r/50247/#review47221
Attachment #8748365 - Flags: review?(s.kaspari) → review+
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?
Flags: needinfo?(ahunt)
Attachment #8748351 - Flags: feedback?(alam) → feedback-
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.
Flags: needinfo?(ahunt)
(In reply to Andrzej Hunt :ahunt from comment #18)
> Adding corner radius will be tricky:

Let's file a follow up for this then.
Flags: needinfo?(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/
Attachment #8748361 - Attachment description: MozReview Request: Bug 1269774 - Fix bookmark star artifacts r?sebastian → MozReview Request: Bug 1269774 - Fix bookmark star artifacts r=sebastian
Attachment #8748362 - Attachment description: MozReview Request: Bug 1269774 - Fix padding for context menu icons r?sebastian → MozReview Request: Bug 1269774 - Fix padding for context menu icons r=sebastian
Attachment #8748363 - Attachment description: MozReview Request: Bug 1269774 - Remove dividers from browser menu r?sebastian → MozReview Request: Bug 1269774 - Remove dividers from browser menu r=sebastian
Attachment #8748364 - Attachment description: MozReview Request: Bug 1269774 - Show browser menu over the menu button r?sebastian → MozReview Request: Bug 1269774 - Show browser menu over the menu button r=sebastian
Attachment #8748365 - Attachment description: MozReview Request: Bug 1269774 - Reduce browser menu top padding r?sebastian → MozReview Request: Bug 1269774 - Reduce browser menu top padding r=sebastian
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 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 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 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 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.)
Flags: needinfo?(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 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 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/
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?
Flags: needinfo?(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.
Flags: needinfo?(ahunt)
Tested using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 49.0a1 (2016-05-12)
No longer depends on: 1271428
Attachment #8748591 - Flags: feedback?(ahunt)
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.
Attachment #8755994 - Flags: approval-mozilla-aurora?
Comment on attachment 8755994 [details] [diff] [review]
aurora_combined_patches

Polish a change, taking it before the firs beta
Attachment #8755994 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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
Flags: needinfo?(ahunt)
(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
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)
Flags: needinfo?(ahunt)
See Also: → 1529557
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.