Upgrade app menu to CardView (add rounded corners to the main menu on Android 5.0+)

VERIFIED FIXED in Firefox 49

Status

()

VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: ahunt, Assigned: ahunt)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(6 attachments)

(Assignee)

Description

3 years ago
We'd like to have rounded corners in the main menu, see Bug 1269774.

The simplest way to do this is to use CardView (which is available as a v7 support library).

For performance reasons CardView doesn't support internal clipping on API < 21. Therefore we need to either implement our own clipping, or just disable rounded corners on those devices. (The menu "action bar", i.e. top bar, has its own background: this either overlaps the corners, or we have ugly white edges at the top/sides if we use the default internal padding that CardView provides.) The current preference seems to be to avoid clipping since we don't know what the performance impact might be.
(Assignee)

Comment 1

3 years ago
TODO:
- investigate additional space used by adding CardView (note: we remove the 9-patch images we previously used as the menu background in this bug, which saves us some space too).
- maybe: investigate performance of internal clipping, not a huge priority?
Assignee: nobody → ahunt
Status: NEW → ASSIGNED
Blocks: 1269774
(In reply to Andrzej Hunt :ahunt from comment #0)
> (The menu "action bar", i.e. top bar, has
> its own background: this either overlaps the corners, or we have ugly white
> edges at the top/sides if we use the default internal padding that CardView
> provides.) The current preference seems to be to avoid clipping since we
> don't know what the performance impact might be.

And can't we "just" replace the "menu action bar" background with a background that has rounded corners too?
(In reply to Sebastian Kaspari (:sebastian) from comment #2)
> And can't we "just" replace the "menu action bar" background with a
> background that has rounded corners too?

Oh, this of course does not work if the menu needs to be scrolled.
(Assignee)

Comment 4

3 years ago
(In reply to Sebastian Kaspari (:sebastian) from comment #3)
> (In reply to Sebastian Kaspari (:sebastian) from comment #2)
> > And can't we "just" replace the "menu action bar" background with a
> > background that has rounded corners too?
> 
> Oh, this of course does not work if the menu needs to be scrolled.

Yup ; ).

Another option I though of was making the action-bar non scrolling, i.e.:

<action-bar/>
<scrollable-list-view>
  <An item/>
  <An other item/>
</scrollable-list-view>

I doubt this would be very nice to use, but it's technically doable...
(Assignee)

Comment 5

3 years ago
If my measurements are to be trusted, then we're only using 32kb more with cardview included:

ls -sk objdir-frontend/dist/fennec-49.0a1.en-US.android-arm.apk

Before:
37628 objdir-frontend/dist/fennec-49.0a1.en-US.android-arm.apk
After:
37660 objdir-frontend/dist/fennec-49.0a1.en-US.android-arm.apk
(Assignee)

Comment 6

3 years ago
Created attachment 8751077 [details]
MozReview Request: Bug 1271428 - Pre: add cardview support library r?sebastian

Review commit: https://reviewboard.mozilla.org/r/51793/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51793/
Attachment #8751077 - Flags: review?(s.kaspari)
Attachment #8751078 - Flags: review?(s.kaspari)
(Assignee)

Comment 7

3 years ago
Created attachment 8751078 [details]
MozReview Request: Bug 1271428 - Use CardView for main menu r?sebastian

Review commit: https://reviewboard.mozilla.org/r/51795/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51795/
(Assignee)

Comment 8

3 years ago
Created attachment 8751079 [details]
MozReview Request: DO NOT USE: Bug 1271428 - Implement ClippedCardView

Brings rounded corner goodness to API < 21.

We don't want to use this for performance reasons, this patch is
posted primarily for reference.

Review commit: https://reviewboard.mozilla.org/r/51797/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51797/
(Assignee)

Updated

3 years ago
Summary: Upgrade app menu to CardView → Upgrade app menu to CardView (add rounded corners to the main menu on Android 5.0+)
(In reply to Andrzej Hunt :ahunt from comment #5)
> If my measurements are to be trusted, then we're only using 32kb more with
> cardview included:
> 
> ls -sk objdir-frontend/dist/fennec-49.0a1.en-US.android-arm.apk
> 
> Before:
> 37628 objdir-frontend/dist/fennec-49.0a1.en-US.android-arm.apk
> After:
> 37660 objdir-frontend/dist/fennec-49.0a1.en-US.android-arm.apk

Did you already use the library in those builds? Otherwise proguard might already have removed most of it.
Comment on attachment 8751077 [details]
MozReview Request: Bug 1271428 - Pre: add cardview support library r?sebastian

https://reviewboard.mozilla.org/r/51793/#review48771

LGTM. Let's see what the installer size tracking says.
Attachment #8751077 - Flags: review?(s.kaspari) → review+
Comment on attachment 8751078 [details]
MozReview Request: Bug 1271428 - Use CardView for main menu r?sebastian

https://reviewboard.mozilla.org/r/51795/#review48769

::: mobile/android/base/java/org/mozilla/gecko/menu/MenuPopup.java:53
(Diff revision 1)
> +        // by default there's a 2px white edge along the top and sides (i.e. an inset corresponding
> +        // to the corner radius), if we disable the inset then the corners overlap.
> +        // It's possible to implement custom clipping, however given that the support library
> +        // chose not to support this for performance reasons, we too have chosen to just disable
> +        // corners on < 21, see Bug 1271428.
> +        if (Build.VERSION.SDK_INT < 21) {

Why didn't you use our AppConstants.Versions class and preLollipop?

::: mobile/android/base/java/org/mozilla/gecko/menu/MenuPopup.java:54
(Diff revision 1)
> +        // to the corner radius), if we disable the inset then the corners overlap.
> +        // It's possible to implement custom clipping, however given that the support library
> +        // chose not to support this for performance reasons, we too have chosen to just disable
> +        // corners on < 21, see Bug 1271428.
> +        if (Build.VERSION.SDK_INT < 21) {
> +            mPanel.setRadius(0);

How does this look like? In general for very "visual" bugs I can only recommend uploading some screenshots and flagging antlam for feedback to avoid many patch cycles. :)
Attachment #8751078 - Flags: review?(s.kaspari) → review+
(Assignee)

Comment 12

3 years ago
Created attachment 8751383 [details]
cardview_android6_phone.png

Here's a screenshot of the upgraded menu, as it looks on Android 5+ devices (this screenshot is from an Android 6.0 emulator).
Flags: needinfo?(alam)
(Assignee)

Comment 13

3 years ago
Created attachment 8751384 [details]
cardview_android4_phone.png

And this is how the menu looks on Android 4 devices (where the rounded corners are disabled).

For some reason disabling the rounded corners makes the shadow less visible, which I'll need to investigate.
(Assignee)

Comment 14

3 years ago
(In reply to Andrzej Hunt :ahunt from comment #13)
> Created attachment 8751384 [details]
> cardview_android4_phone.png
> 
> 
> For some reason disabling the rounded corners makes the shadow less visible,
> which I'll need to investigate.

Actually this seems to be unrelated to the corners (although enabling corners changes how the shadow is perceived). The pre-21 implementation of CardView seems to just have less shadow, I'll have to investigate whether we can:
(A) tune that?
(B) (Maybe it's fixed in a newer version of the support library?)
(C) use the old graphics on pre-21 (this would waste more apk space if we have to keep the old 9-patches)
(In reply to Andrzej Hunt :ahunt from comment #14)
> (In reply to Andrzej Hunt :ahunt from comment #13)
> > Created attachment 8751384 [details]
> > cardview_android4_phone.png
> > 
> > 
> > For some reason disabling the rounded corners makes the shadow less visible,
> > which I'll need to investigate.
> 
> Actually this seems to be unrelated to the corners (although enabling
> corners changes how the shadow is perceived). The pre-21 implementation of
> CardView seems to just have less shadow, I'll have to investigate whether we
> can:
> (A) tune that?
> (B) (Maybe it's fixed in a newer version of the support library?)
> (C) use the old graphics on pre-21 (this would waste more apk space if we
> have to keep the old 9-patches)

(D) Leave it as-is?

It doesn't look too bad? On Lollipop+ the system should render the shadow based on the elevation property. And the pre-Lollipop version probably just uses some image assets.
(In reply to Sebastian Kaspari (:sebastian) from comment #15)
> (In reply to Andrzej Hunt :ahunt from comment #14)
> > (In reply to Andrzej Hunt :ahunt from comment #13)
> > > Created attachment 8751384 [details]
> > > cardview_android4_phone.png
> > > 
> > > 
> > > For some reason disabling the rounded corners makes the shadow less visible,
> > > which I'll need to investigate.
> > 
> > Actually this seems to be unrelated to the corners (although enabling
> > corners changes how the shadow is perceived). The pre-21 implementation of
> > CardView seems to just have less shadow, I'll have to investigate whether we
> > can:
> > (A) tune that?
> > (B) (Maybe it's fixed in a newer version of the support library?)
> > (C) use the old graphics on pre-21 (this would waste more apk space if we
> > have to keep the old 9-patches)
> 
> (D) Leave it as-is?
> 
> It doesn't look too bad? On Lollipop+ the system should render the shadow
> based on the elevation property. And the pre-Lollipop version probably just
> uses some image assets.

that's what I was thinking too :)
Blocks: 1157964
No longer blocks: 1269774
Flags: needinfo?(alam)

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/99de71314883
https://hg.mozilla.org/mozilla-central/rev/c0e570859257
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Created attachment 8755414 [details]
Screenshot_20160523-154459.png

Tested using:
Device: Motorola Nexus 6 (Android 6.0)
Build: Firefox for Android 49.0a1 (2016-05-18)
Status: RESOLVED → VERIFIED

Updated

2 years ago
Duplicate of this bug: 1203730
You need to log in before you can comment on or make changes to this bug.