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

RESOLVED WONTFIX

Status

()

defect
RESOLVED WONTFIX
5 years ago
3 years ago

People

(Reporter: lucasr, Assigned: mcomella)

Tracking

(Blocks 1 bug)

unspecified
Firefox 39
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox37+ wontfix, firefox38+ wontfix, firefox39 wontfix)

Details

Attachments

(7 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Likely a nice touch. Might look a bit weird without any clear separation between statusbar and the tabs tray header though.
(Reporter)

Comment 1

5 years ago
Attachment #8476573 - Flags: feedback?(alam)
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

5 years ago
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 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
Attachment #8476573 - Flags: feedback?(alam) → feedback+
Attachment #8476575 - Flags: feedback?(alam) → feedback+
(Reporter)

Updated

5 years ago
Blocks: new-toolbar-v2
No longer blocks: new-toolbar-v1
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?
Flags: needinfo?(lucasr.at.mozilla)
(Reporter)

Comment 7

5 years ago
(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.
Flags: needinfo?(lucasr.at.mozilla)
(Reporter)

Updated

5 years ago
Attachment #8495322 - Flags: review?(mark.finkle)
(Reporter)

Comment 10

5 years ago
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.
Attachment #8495321 - Flags: review?(nalexander)
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.
Attachment #8495322 - Flags: review?(mark.finkle) → review+
(Reporter)

Comment 12

5 years ago
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.
(Reporter)

Comment 13

5 years ago
(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?
Flags: needinfo?(rnewman)
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.
Attachment #8495321 - Flags: review?(nalexander) → review+
(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.
Flags: needinfo?(rnewman)
(Reporter)

Comment 16

5 years ago
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.
(Reporter)

Comment 17

5 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/3fb6805983a0
https://hg.mozilla.org/mozilla-central/rev/c56275d516ec
Assignee: nobody → lucasr.at.mozilla
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Posted image android.png
It does not work as expected with the HTC One, see the screenshot.
Depends on: 1073845
(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
Depends on: 1074536
Depends on: 1074924
Depends on: 1076692
Depends on: 1084079
Depends on: 1100439
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
Flags: needinfo?(mark.finkle)
(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.
Flags: needinfo?(mark.finkle)
/r/4279 - Bug 1056002 - Backout changeset c56275d516ec. r=mfinkle

Pull down this commit:

hg pull review -r 5448050404c59576081a8e16ebbe3725fe41836c
Attachment #8568867 - Flags: review?(mark.finkle)
Comment on attachment 8568867 [details]
MozReview Request: bz://1056002/mcomella

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

Ship It!
Attachment #8568867 - Flags: review?(mark.finkle) → review+
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
Attachment #8568867 - Flags: approval-mozilla-beta?
Attachment #8568867 - Flags: approval-mozilla-aurora?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: lucasr.at.mozilla → nobody
https://hg.mozilla.org/mozilla-central/rev/ce460734fd09
Assignee: nobody → michael.l.comella
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 35 → Firefox 39
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?
Flags: needinfo?(alam)
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?
Flags: needinfo?(alam) → needinfo?(michael.l.comella)
(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
Flags: needinfo?(michael.l.comella)
Attachment #8570525 - Flags: feedback?(alam)
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I'm ok with just using a solid "Text and Tabs tray grey" as a color for the status bar.
Attachment #8570525 - Flags: feedback?(alam)
(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.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → WONTFIX
See Also: → 1137240
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.
Attachment #8568867 - Flags: approval-mozilla-beta?
Attachment #8568867 - Flags: approval-mozilla-beta+
Attachment #8568867 - Flags: approval-mozilla-aurora?
Attachment #8568867 - Flags: approval-mozilla-aurora+
Note that bug 1137240 should be uplifted along with this (it doesn't cause crashes so it doesn't have to be simultaneous).
Glad to hear that since bug 1137240 hasn't even landed or had approval requests yet.
(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.
Attachment #8568867 - Attachment is obsolete: true
Attachment #8618281 - Flags: review+
You need to log in before you can comment on or make changes to this bug.