Closed Bug 1403653 Opened 4 years ago Closed 4 years ago

White status bar may clash visually with lightweight themes

Categories

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

57 Branch
All
Android
defect

Tracking

(relnote-firefox 58+, fennec+, firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 wontfix, firefox58 verified)

VERIFIED FIXED
Firefox 58
Tracking Status
relnote-firefox --- 58+
fennec + ---
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- wontfix
firefox58 --- verified

People

(Reporter: JanH, Assigned: JanH)

References

Details

Attachments

(6 files)

With the unskinned URL bar the white status bar is still debatable, but at least you can argue that it's consistent with the rest of the URL bar, which is white as well.

However with a lightweight theme applied, this is no longer the case, and I feel that a dark grey (like in the tabs tray) default might fit most themes better than the white statusbar.

Experimentally, it might also be an idea to see whether using the accent colour provided by lightweight themes as the status bar colour might yield something aesthetically pleasing.
Sampling a few random themes, using the accent colour doesn't yield perfect results all the time, but in the right combination it can look quite nice (see attached screenshot) and would be an improvement I think.
Flags: needinfo?(chuang)
See Also: → 1405558
(In reply to Jan Henning [:JanH] from comment #1)
> Created attachment 8912886 [details]
> LWT + statusbar colours.png
> 
> Sampling a few random themes, using the accent colour doesn't yield perfect
> results all the time, but in the right combination it can look quite nice
> (see attached screenshot) and would be an improvement I think.

I found that some themes didn't provide accent color, which causes status bar cannot be tinted with correct color. Perhaps we can use 'Palette' library [1] to fetch vibrant color from the bitmap?

[1] https://developer.android.com/training/material/palette-colors.html
Given that we're still using the same version of the support library, we probably need to pay some attention to avoid a recurrence of bug 1318667...
Assignee: nobody → jh+bugzilla
Attached image themes.png
Verified on both the build provided. 
For the middle theme is there a way to check the pallet to be sure it is the right color - I see some drops of gray so i think it is ok -
https://addons.mozilla.org/en-US/android/addon/ocean-spray/
@Jan can you try to check the palette - and push the changes further? 
Looks fine from my side otherwise.
Flags: needinfo?(jh+bugzilla)
(In reply to Ioana Chiorean from comment #6)
> Created attachment 8920269 [details]
> themes.png
> 
> Verified on both the build provided. 
> For the middle theme is there a way to check the pallet to be sure it is the
> right color - I see some drops of gray so i think it is ok -
> https://addons.mozilla.org/en-US/android/addon/ocean-spray/

Right-click on the preview picture and Inspect Element. The theme data is stored as an attribute ("persona" respectively "data-browsertheme") on that <div>. For that particular theme, the accent colour has been explicitly set to #ffffff, so there's not much we can if we don't want to start second-guessing theme authors.
Flags: needinfo?(jh+bugzilla)
Comment on attachment 8920302 [details]
Bug 1403653 - Part 0 - Cleanup imports.

https://reviewboard.mozilla.org/r/189418/#review196470
Attachment #8920302 - Flags: review?(jh+bugzilla) → review+
Comment on attachment 8920303 [details]
Bug 1403653 - Part 1 - Refactor getDominantColor.

https://reviewboard.mozilla.org/r/189420/#review196822
Attachment #8920303 - Flags: review?(cnevinchen)
Comment on attachment 8920304 [details]
Bug 1403653 - Part 2 - Split out conversion from ColorRes to ColorInt into separate function.

https://reviewboard.mozilla.org/r/189422/#review196824
Attachment #8920304 - Flags: review+
Comment on attachment 8920305 [details]
Bug 1403653 - Part 3 - Use the Lightweight Theme accent colour for the status bar in normal mode.

https://reviewboard.mozilla.org/r/189424/#review196826
Attachment #8920305 - Flags: review+
Comment on attachment 8920303 [details]
Bug 1403653 - Part 1 - Refactor getDominantColor.

https://reviewboard.mozilla.org/r/189420/#review196828
Attachment #8920303 - Flags: review?(cnevinchen) → review+
tracking-fennec: ? → +
Priority: -- → P2
Pushed by nechen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/66232f26609e
Part 0 - Cleanup imports. r=JanH
https://hg.mozilla.org/integration/autoland/rev/e40240828163
Part 1 - Refactor getDominantColor. r=nechen
https://hg.mozilla.org/integration/autoland/rev/1e66240eb8b6
Part 2 - Split out conversion from ColorRes to ColorInt into separate function. r=nechen
https://hg.mozilla.org/integration/autoland/rev/f36de129726f
Part 3 - Use the Lightweight Theme accent colour for the status bar in normal mode. r=nechen
Backed out for Android bustage at mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/BitmapUtils.java:23: package android.support.v7.graphics does not exist:

https://hg.mozilla.org/integration/autoland/rev/c2be7c49dc3a242750815cec13792e6cc7d42a26
https://hg.mozilla.org/integration/autoland/rev/9f108013a77d8489ec9ab603fb315233bba75083
https://hg.mozilla.org/integration/autoland/rev/58bc1deb8591d7b2ed80f16cd38f23e882b44de3
https://hg.mozilla.org/integration/autoland/rev/b07f7c15092c99549cd07d5b22c735ea91ebc32d

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f36de129726f4952a47ef323a972247a1bdd78d0&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=138726395&repo=autoland

 /builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/BitmapUtils.java:23: error: package android.support.v7.graphics does not exist
/builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/BitmapUtils.java:152: error: cannot find symbol
/builds/worker/workspace/build/src/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/BitmapUtils.java:152: error: cannot find symbol
Flags: needinfo?(jh+bugzilla)
Forgot to update moz.build as well for the changed library usage.
Flags: needinfo?(jh+bugzilla)
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/1d10c858233d
Part 0 - Cleanup imports. r=JanH
https://hg.mozilla.org/integration/autoland/rev/6869117e9c70
Part 1 - Refactor getDominantColor. r=nechen
https://hg.mozilla.org/integration/autoland/rev/007b3dd7e32b
Part 2 - Split out conversion from ColorRes to ColorInt into separate function. r=nechen
https://hg.mozilla.org/integration/autoland/rev/13f88bb8b30e
Part 3 - Use the Lightweight Theme accent colour for the status bar in normal mode. r=nechen
Verified as fixed on Nightly 58.0a1.
Devices:
Galaxy S8 (Android 7.0)
LG G4 (Android 6.0)
Status: RESOLVED → VERIFIED
Just to confirm it here - this will be riding the train right? cc Wesly, Ritu, Nicole.
I see Sebastian Hengst [:aryx][:archaeopteryx] has set it to 58 but this addresses issues n Photon.
Flags: needinfo?(wehuang)
Flags: needinfo?(rkothari)
Flags: needinfo?(nyee)
Hi Ioana,

I think given that we are days away from 57 and the fact that this is P2 bug suggests that this is a better candidate to ride the 58 nightly train and is not critical enough for uplift to 57.
Flags: needinfo?(nyee)
I agree with the team's assessment of letting this ride the 58 train. If the change is big, now very low risk and doesn't fix a common/mainline scenario, we can let it be wontfix for 57.
Flags: needinfo?(rkothari)
let's keep it as P2 thus riding 58, not uplifting to 57. Thanks.

(In reply to Ioana Chiorean from comment #28)
> Just to confirm it here - this will be riding the train right? cc Wesly,
> Ritu, Nicole.
> I see Sebastian Hengst [:aryx][:archaeopteryx] has set it to 58 but this
> addresses issues n Photon.
Flags: needinfo?(wehuang)
Flags: needinfo?(chuang)
Release Note Request (optional, but appreciated)
[Why is this notable]: Seen some feedback on 57 specifically mentioning the white status bar.
[Affects Firefox for Android]: Only.
[Suggested wording]: Themes can now change the status bar colour as well.
[Links (documentation, blog post, etc)]: none
relnote-firefox: --- → ?
Gerry, another one for 58. 
Maybe something like "Added the ability to change the status bar color in themes"
Flags: needinfo?(gchang)
Liz, I've already added in the notes.
Flags: needinfo?(gchang)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.