1137240 - Color android status bar with tabs tray grey on Android L

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: screenshot.png

See screenshot.

Thinking this is a regression from backing out the semi-transparent patch.

In the screenshot, we should have a black bar on Android 5.0. With it now, the browser looks out of place on the platform.

--
Nexus 6 (Android 5.0.1)

Comment 1

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

I think this is a regression from bug 1097337 (inherit from Material theme), uncovered by backing out bug 1056002 (tinting the status bar).

Comment 2

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

I can't repro on my N9.

I imagine this is related to android:colorPrimaryDark [1] not being set (and perhaps being different across Nexus devices? ew).

[1]: https://developer.android.com/training/material/theme.html#StatusBar

Comment 3

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

But then again, I don't see `WindowManager.LayoutParams.FLAG_DRAWS_SYSTEM_BAR_BACKGROUNDS` and `WindowManager.LayoutParams.FLAG_TRANSLUCENT_STATUS`, or their XML equivalents, defined anywhere (which allow us to affect the system bar colors).

Aaron, can you repro on any other devices? e.g. N5

Comment 4

[Aaron Train [:aaronmt]]

Yeah reproducible on my Nexus 5 as well, alongside my other Android 5.0 devices.

Don't have any 4.x on me, what does it look like?

Comment 5

[Aaron Train [:aaronmt]]

Got my S5, I see black. Looks like an Android 5.0 specific bug to me.

Comment 6

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

Upgraded N4 to 5.0.1 and I can repro the issue.

Comment 7

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

Attachment: MozReview Request: bz://1137240/mcomella

/r/4405 - Bug 1137240 - Specify primaryColorDark in v21 themes.xml to set status bar color. r=liuche

Pull down this commit:

hg pull review -r d9029fa465ad1de6c80b89916fbdc399dbec522b

Comment 8

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

Solution is to apply proper tint color on Lollipop. It still doesn't work on my N9 but I'm going to guess it's an Android 5 bug. It has the dark system status bar so it doesn't look terrible.

Comment 9

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

Attachment: Post patch screenshot

Status bar tint cool with you, Anthony?

Comment 10

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

(In reply to Michael Comella (:mcomella) from comment #8)
> Solution is to apply proper tint color on Lollipop.

I'm assuming the reason is because the default system status bar color is that grey color (perhaps because we inherit from Material light?).

Comment 12

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

Comment on attachment 8570225 [details]
Post patch screenshot

Looks good to me! Though, this isn't a tint so much as a solid color right? or are my eyes deceiving me...

Comment 13

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

(In reply to Anthony Lam (:antlam) from comment #12)
> Looks good to me! Though, this isn't a tint so much as a solid color right?

Yes - solid colors are Android L convention. See bug 1056002 comment 32 for more details.

Comment 14

[Matt Brubeck (:mbrubeck)]

This regression is also in Aurora 38.  Should this be nominated for tracking-fennec:38+ and Aurora uplift?

(In reply to Michael Comella (:mcomella) from comment #9)
> Status bar tint cool with you, Anthony?

I think we should also consider going back to black for the status bar, since the status bar is still visible even when the url bar disappears, and any color besides black looks a bit weird in that case.  (It feels less "full screen.")

Comment 15

[Matt Brubeck (:mbrubeck)]

For comparison: Chrome always uses black for the status bar.  Apps like YouTube and Play Books change the status bar to black when you enter their "content" views.

Comment 16

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

Attachment: Post patch screenshot, url bar hidden

(In reply to Matt Brubeck (:mbrubeck) from comment #14)
> I think we should also consider going back to black for the status bar,
> since the status bar is still visible even when the url bar disappears, and
> any color besides black looks a bit weird in that case.  (It feels less
> "full screen.")

See this screenshot for comparison - I think our tabs tray color is dark enough that the contrast to page content looks good though I'll leave the final decision to Anthony. 

(In reply to Matt Brubeck (:mbrubeck) from comment #15)
> For comparison: Chrome always uses black for the status bar.  Apps like
> YouTube and Play Books change the status bar to black when you enter their
> "content" views.

Good sleuthing - we can also try doing something to this effect.

Comment 17

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

renom to track 38 (see comment 14).

Comment 20

[Chenxia Liu [:liuche]]

Comment on attachment 8570222 [details]
MozReview Request: bz://1137240/mcomella

https://reviewboard.mozilla.org/r/4403/#review3803

Ship It!

Comment 21

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

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

Comment 22

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

Comment on attachment 8570222 [details]
MozReview Request: bz://1137240/mcomella

Approval Request Comment
[Feature/regressing bug #]: bug 1097337
[User impact if declined]:
  Users on android L will have an inconsistent grey status bar - this patch makes it fit the system.

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

[Risks and why]:
  Extremely low - we just add a "colorPrimaryDark" attribute which is only used by the system to set the status bar color. Since we don't reference the attribute, nothing else should change.

[String/UUID change made/needed]: None

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Darrin, How critical from a ux perspective is this, for uplift to aurora ?

[Tracking Requested - why for this release]:  This was nominated for tracking and uplift on bug 1139237 so I'm moving the nomination here.

Comment 24

[Darrin Henein [:darrin]]

From my perspective, its something I'd certainly like to see uplifted. Its a pretty major regression in our UI, and will be clearly noticeable.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/97cf1b0eb9da

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, thanks Darrin!  Tracking this for 38.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8570222 [details]
MozReview Request: bz://1137240/mcomella

Approving this for uplift to 38 since it's a noticeable UI regression and looks low-risk.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5a1e981853b0

Comment 29

[Kevin Brosnan [Ex-Mozilla]]

v. Firefox for Android 39 and 38

Comment 31

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

Attachment: MozReview Request: Bug 1137240 - Specify primaryColorDark in v21 themes.xml to set status bar color. r=liuche