1056002 - Experiment tinting Android's statusbar with our tabs tray background color

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

Likely a nice touch. Might look a bit weird without any clear separation between statusbar and the tabs tray header though.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Screenshot with tinted status bar

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Screenshot with tinted status bar (tabs panel open)

Comment 3

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

I'm still trying to decide if I like these or not, but they definitely feel like a nice touch on first glance!

Holding onto that feedback button for now..

Comment 4

[Paul [pwd]]

I'm a huge fan. I say we land the patch and get feedback, if it's negative, we can always pull it before the train reaches release.

Comment 5

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

Comment on attachment 8476573 [details]
Screenshot with tinted status bar

I like this - I'm ok with putting it in and seeing how it feels IRL :P

Comment 6

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

Actually, another plus for this fading bar is that we can then pad our tabs right up against the status bar and save us some vertical space (more applicable for Tablet's UI). 

Since, one of the reasons for not doing that right now is that it looks awkward since our background matte is a grey and the statusbar is a black, we needed some room for the top of the tabs to breath. This would probably help that quite a bit... Thoughts?

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Anthony Lam (:antlam) from comment #6)
> Actually, another plus for this fading bar is that we can then pad our tabs
> right up against the status bar and save us some vertical space (more
> applicable for Tablet's UI). 
> 
> Since, one of the reasons for not doing that right now is that it looks
> awkward since our background matte is a grey and the statusbar is a black,
> we needed some room for the top of the tabs to breath. This would probably
> help that quite a bit... Thoughts?

Good point but keep in mind that status bar tinting is only available in KitKat and later Android releases. We'll have to add top padding in the tab strip on pre-KitKat.

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: Merge picasso and nineoldandroids jars into one (r=nalexander)

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: Tint the status bar on KitKat+ (r=mfinkle)

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8495321 [details] [diff] [review]
Merge picasso and nineoldandroids jars into one (r=nalexander)

I think it's fine for search activity to link with Picasso too (even though it's not really using it) as we're not planning to ship it in a separate APK anyway.

Comment 11

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

Comment on attachment 8495322 [details] [diff] [review]
Tint the status bar on KitKat+ (r=mfinkle)

I assume that Proguard would remove the unused Java code during it's optimization run, so we don't need build flags in the moz.build file.

Comment 12

[Lucas Rocha (:lucasr)]

Forgot the say it: the reason I'm merging these libraries into a single jar is that I'll be integrating some smaller libraries soon and we don't really need to have separate jars for each 3rd-party library. We can split them up if we end up needing more granularity in the future.

Comment 13

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #11)
> Comment on attachment 8495322 [details] [diff] [review]
> Tint the status bar on KitKat+ (r=mfinkle)
> 
> I assume that Proguard would remove the unused Java code during it's
> optimization run, so we don't need build flags in the moz.build file.

I'm assuming proguard is able to remove unreachable code like the stuff in setupSystemUITinting(). rnewman?

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8495321 [details] [diff] [review]
Merge picasso and nineoldandroids jars into one (r=nalexander)

Review of attachment 8495321 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.  For bonus points, include a comment at the declaration of gecko-thirdparty explaining that this is a good place to put small independent libraries.

Comment 15

[Richard Newman [:rnewman]]

(In reply to Lucas Rocha (:lucasr) from comment #13)
 
> I'm assuming proguard is able to remove unreachable code like the stuff in
> setupSystemUITinting(). rnewman?

javac itself will remove the unreachable code if Versions.feature19Plus is known to be false during the build.

ProGuard should do it if javac didn't know for sure.

ProGuard might or might not eliminate the unused class (it should, but it's public…); this is something I'd verify experimentally rather than trying pure reason :D

If you want to test this, make a backup of your .mozconfig, change the object directory in there, and add

ac_add_options --with-android-min-sdk=9
ac_add_options --with-android-max-sdk=9

You can then do a build and take a look at the output in the new object directory, see which classes make it through, and use javap to see what bytecode javac produces.

You can also look at $objdir/mobile/android/base/jars-proguarded to see the post-ProGuard output.

  jar tf $objdir/mobile/android/base/jars-proguarded/gecko-browser.jar | fgrep MyClass

will tell you if ProGuard decided to eliminate the redundant class.

Comment 16

[Lucas Rocha (:lucasr)]

Tried using --with-android-min-sdk=9 & --with-android-max-sdk=9 and found out that the new tablet UI is breaking the build with these flags :-/ We'll have to add alias/stubs for a bunch of resources. Filed bug 1073474 to track this.

I'll double-check that the tint manager is not present once I fix bug 1073474. I'll just land it now as it's just a single class anyway.

Comment 17

[Lucas Rocha (:lucasr)]

(In reply to Nick Alexander :nalexander from comment #14)
> Comment on attachment 8495321 [details] [diff] [review]
> Merge picasso and nineoldandroids jars into one (r=nalexander)
> 
> Review of attachment 8495321 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm.  For bonus points, include a comment at the declaration of
> gecko-thirdparty explaining that this is a good place to put small
> independent libraries.

Done.

Comment 18

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/3fb6805983a0
https://hg.mozilla.org/integration/fx-team/rev/c56275d516ec

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3fb6805983a0
https://hg.mozilla.org/mozilla-central/rev/c56275d516ec

Comment 20

[Sören Hentzschel]

Attachment: android.png

It does not work as expected with the HTC One, see the screenshot.

Comment 21

[Aaron Train [:aaronmt]]

(In reply to Sören Hentzschel from comment #20)
> Created attachment 8496429 [details]
> android.png
> 
> It does not work as expected with the HTC One, see the screenshot.

I filed a bug, bug 1073845

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

This has caused quite a few regressions (see the depedent bugs) and, in particular, makes our fullscreen experience pretty unpolished. I propose we back it out and come up with a more refined solution. We are currently using the SystemBarTintManager [1] which does not gracefully handle toggling fullscreen modes (causing many graphical glitches) so we'd probably have to roll our own solution (that can better integrate with our fullscreen mode). In the mean time, it's built into the system on Lollipop so we can get easy wins there.

What do you think, Mark?

We currently ship the tint in 35 and 36, but can backout starting with 37. This also has the benefit of simplifying the uplifts I'd have to make to fix some of the other fullscreen bugs tracking 37 that I'm assigned to.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/thirdparty/com/readystatesoftware/systembartint/SystemBarTintManager.java

Comment 23

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

(In reply to Michael Comella (:mcomella) from comment #22)

> What do you think, Mark?

I agree. A backout seems like the best option for now.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: bz://1056002/mcomella

/r/4279 - Bug 1056002 - Backout changeset c56275d516ec. r=mfinkle

Pull down this commit:

hg pull review -r 5448050404c59576081a8e16ebbe3725fe41836c

Comment 25

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

Comment on attachment 8568867 [details]
MozReview Request: bz://1056002/mcomella

https://reviewboard.mozilla.org/r/4277/#review3443

Ship It!

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/ce460734fd09

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8568867 [details]
MozReview Request: bz://1056002/mcomella

Backout of this bug: note the fix is already in release (35 & 36) so we'll be taking away the toolbar tint from users.

Approval Request Comment
[Feature/regressing bug #]: This one.

[User impact if declined]:
  All of the unclosed dependent bugs remain broken - these are largely fullscreen mode polish issues. Toolbar tint will remain.

[Describe test coverage new/current, TreeHerder]:
  None

[Risks and why]:
  Low - backing out a feature that interfaces through an instance that we no longer access. The risk is slightly higher because I had to back out both bug 1074924 and bug 1076692, but I did it by hand in order to make the compile work because I didn't realize the deps.
 
[String/UUID change made/needed]: None

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(backout is comment 26)

Comment 29

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

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

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Actually, this is a lot of extra work for little gain (i.e. this only works in KitKat) - I want to call WONTFIX.

Anthony, thoughts?

Comment 31

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

Hm, this actually helped our toolbar feel less cramped in many ways and was a nice touch I felt. That being said, I'm ok with this proposal for it's impact on full screen mode and complexity.

But, for the record can I get the effects in a screenshot here? Also, what (solid color im guessing) are you proposing we switch this to?

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: No tint (system status bar color)

(In reply to Anthony Lam (:antlam) from comment #31)
> But, for the record can I get the effects in a screenshot here? Also, what
> (solid color im guessing) are you proposing we switch this to?

Here's the breakdown:
  * Pre KitKat - no control over the status bar color
  * KitKat - gradient status bar color possible, with some effort
  * Lollipop - status bar color is simple, gradient status bar color possible with same efforts. Note that a gradient status bar would break Android convention

This may change in the future if Google backports some of their Lollipop APIs to earlier versions, where a solid status bar color is possible (but perhaps unlikely).

The reason I don't think it's worth the effort is that I imagine many (most?) KitKat devices will get the bump to Lollipop.

In Lollipop, we're adding the tint in bug 1137240 [2], which is tabs tray grey.

Note that I also found a blog post [1] that makes doing the gradient color easy on KitKat only, but for an ugly perf hit.

[1]: https://gauntface.com/blog/2014/01/10/translucent-theme-in-android/
[2]: https://bug1137240.bugzilla.mozilla.org/attachment.cgi?id=8570225

Comment 33

[Richard Newman [:rnewman]]

Fixing status: the merge was a backout, not a resolution.

FWIW, the status bar turning grey or red in some apps is one of the things I hate about Lollipop. The color never matches (deliberately!), and notification bar icons typically don't work well with every color.

I don't like the idea of spending time trying to get this working on KK and earlier, especially when previous attempts are known to cause regressions.

Comment 34

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

I'm ok with just using a solid "Text and Tabs tray grey" as a color for the status bar.

Comment 35

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Anthony Lam (:antlam) from comment #34)
> I'm ok with just using a solid "Text and Tabs tray grey" as a color for the
> status bar.

For now, that's only on L and up.

Comment 36

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8568867 [details]
MozReview Request: bz://1056002/mcomella

Approving backout - this is nice and early in Beta so hopefully we will catch anything missed in the backout.

Comment 37

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Note that bug 1137240 should be uplifted along with this (it doesn't cause crashes so it doesn't have to be simultaneous).

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Glad to hear that since bug 1137240 hasn't even landed or had approval requests yet.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/415a2cc6f3f5

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e8a752491ccc

Comment 41

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #38)
> Glad to hear that since bug 1137240 hasn't even landed or had approval
> requests yet.

It's not a tight dependency so I'm not too concerned - bug 1137240 exists because this bug was backed out but that being said, getting this backout landed is more important than getting them landed together.

Comment 43

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1056002 - Backout changeset c56275d516ec. r=mfinkle