Color android status bar with tabs tray grey on Android L

VERIFIED FIXED in Firefox 38

Status

()

Firefox for Android
General
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: aaronmt, Assigned: mcomella)

Tracking

(Blocks: 1 bug, {regression, reproducible})

38 Branch
Firefox 39
ARM
Android
regression, reproducible
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox37 unaffected, firefox38+ verified, firefox39 verified, fennec38+)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8569901 [details]
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)
(Reporter)

Updated

3 years ago
Flags: needinfo?(michael.l.comella)
I think this is a regression from bug 1097337 (inherit from Material theme), uncovered by backing out bug 1056002 (tinting the status bar).
Assignee: nobody → michael.l.comella
Flags: needinfo?(michael.l.comella)
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
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
Flags: needinfo?(aaron.train)
tracking-fennec: ? → 39+
(Reporter)

Comment 4

3 years ago
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?
Flags: needinfo?(aaron.train)
(Reporter)

Comment 5

3 years ago
Got my S5, I see black. Looks like an Android 5.0 specific bug to me.
Upgraded N4 to 5.0.1 and I can repro the issue.
Created attachment 8570222 [details]
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
Attachment #8570222 - Flags: review?(liuche)
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.
Created attachment 8570225 [details]
Post patch screenshot

Status bar tint cool with you, Anthony?
Attachment #8570225 - Flags: feedback?(alam)
(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?).
Duplicate of this bug: 1100439
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...
Attachment #8570225 - Flags: feedback?(alam) → feedback+
(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.
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.")
status-firefox38: --- → affected
Version: Trunk → Firefox 38
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.
(Reporter)

Updated

3 years ago
Flags: needinfo?(michael.l.comella)
Created attachment 8571486 [details]
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.
Flags: needinfo?(michael.l.comella)
renom to track 38 (see comment 14).
tracking-fennec: 39+ → ?
Duplicate of this bug: 1137992
Blocks: 1030897
Summary: Regression: android status-bar theme colour is not adhered to platform → Color android status bar with tabs tray grey on Android L
Duplicate of this bug: 1139237
Comment on attachment 8570222 [details]
MozReview Request: bz://1137240/mcomella

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

Ship It!
Attachment #8570222 - Flags: review?(liuche) → review+
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
Attachment #8570222 - Flags: approval-mozilla-aurora?
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.
status-firefox37: --- → unaffected
tracking-firefox38: --- → ?
tracking-firefox39: --- → ?
Flags: needinfo?(dhenein)
tracking-fennec: ? → 38+
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.
Flags: needinfo?(dhenein)
https://hg.mozilla.org/mozilla-central/rev/97cf1b0eb9da
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
OK, thanks Darrin!  Tracking this for 38.
tracking-firefox38: ? → +
tracking-firefox39: ? → ---
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.
Attachment #8570222 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
v. Firefox for Android 39 and 38
Status: RESOLVED → VERIFIED
status-firefox38: fixed → verified
status-firefox39: fixed → verified
Comment on attachment 8570222 [details]
MozReview Request: bz://1137240/mcomella
Attachment #8570222 - Attachment is obsolete: true
Attachment #8619602 - Flags: review+
Created attachment 8619602 [details]
MozReview Request: Bug 1137240 - Specify primaryColorDark in v21 themes.xml to set status bar color. r=liuche
You need to log in before you can comment on or make changes to this bug.