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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Andrzej Hunt :ahunt]

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.

Comment 1

[Andrzej Hunt :ahunt]

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?

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

(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?

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

(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.

Comment 4

[Andrzej Hunt :ahunt]

(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...

Comment 5

[Andrzej Hunt :ahunt]

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

Comment 6

[Andrzej Hunt :ahunt]

Attachment: 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/

Comment 7

[Andrzej Hunt :ahunt]

Attachment: 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/

Comment 8

[Andrzej Hunt :ahunt]

Attachment: 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/

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

(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 10

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

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. :)

Comment 12

[Andrzej Hunt :ahunt]

Attachment: 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).

Comment 13

[Andrzej Hunt :ahunt]

Attachment: 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.

Comment 14

[Andrzej Hunt :ahunt]

(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)

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

(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.

Comment 16

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

(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 :)

Comment 17

[Andrzej Hunt :ahunt]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3dc0aada8b4

Comment 18

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/99de713148834eceeb0e37315ecd43a8a41f3bd3
Bug 1271428 - Pre: add cardview support library r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/c0e57085925708e340e625f37cc4b013c942f055
Bug 1271428 - Use CardView for main menu r=sebastian

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/99de71314883
https://hg.mozilla.org/mozilla-central/rev/c0e570859257

Comment 20

[Alexandru Simonca [:asimonca], Release Desktop QA]

Attachment: Screenshot_20160523-154459.png

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