1403653 - White status bar may clash visually with lightweight themes

Status: VERIFIED FIXED

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

Description

[Jan Henning [:JanH]]

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.

Comment 1

[Jan Henning [:JanH]]

Attachment: 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.

Comment 2

[Jan Henning [:JanH]]

Test build available here:
https://queue.taskcluster.net/v1/task/X-I3EkuET5ih2hD2zUoqNA/runs/0/artifacts/public/build/target.apk (https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d52cc25d26ac2bdaa355a1b2c0c5ddb545eae1d&selectedJob=133663003)

Comment 3

[Jing-wei Wu [:jwu]]

(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

Comment 4

[Jan Henning [:JanH]]

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...

Comment 5

[Jan Henning [:JanH]]

Updated Try build that falls back to calculating the dominant colour ourselves if the theme doesn't provide an accent colour:
https://queue.taskcluster.net/v1/task/ct8zPC-xTsaDyteLgkz7Hg/runs/0/artifacts/public/build/target.apk
(https://treeherder.mozilla.org/#/jobs?repo=try&revision=19a98d0b5550f49d7873f795a88b235b157abd64&selectedJob=137027070)

Comment 6

[Ioana Chiorean]

Attachment: 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/

Comment 7

[Ioana Chiorean]

@Jan can you try to check the palette - and push the changes further? 
Looks fine from my side otherwise.

Comment 8

[Jan Henning [:JanH]]

(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.

Comment 9

[Jan Henning [:JanH]]

Attachment: Bug 1403653 - Part 0 - Cleanup imports.

Review commit: https://reviewboard.mozilla.org/r/189418/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/189418/

Comment 10

[Jan Henning [:JanH]]

Attachment: Bug 1403653 - Part 1 - Refactor getDominantColor.

We want to use the Palette library for getting a fallback accent colour for lightweight themes, however because of bug 1318667, we might have to continue using our own implementation of getDominantColor on x86 devices.

Therefore we move this into BitmapUtils, so we can have a central location from which to switch between our own and the Palette library implementation.

Review commit: https://reviewboard.mozilla.org/r/189420/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/189420/

Comment 11

[Jan Henning [:JanH]]

Attachment: Bug 1403653 - Part 2 - Split out conversion from ColorRes to ColorInt into separate function.

Review commit: https://reviewboard.mozilla.org/r/189422/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/189422/

Comment 12

[Jan Henning [:JanH]]

Attachment: Bug 1403653 - Part 3 - Use the Lightweight Theme accent colour for the status bar in normal mode.

If the theme doesn't provide an accent colour, we try determining something appropriate ourselves.

Review commit: https://reviewboard.mozilla.org/r/189424/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/189424/

Comment 13

[Jan Henning [:JanH]]

Comment on attachment 8920302 [details]
Bug 1403653 - Part 0 - Cleanup imports.

https://reviewboard.mozilla.org/r/189418/#review196470

Comment 14

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8920303 [details]
Bug 1403653 - Part 1 - Refactor getDominantColor.

https://reviewboard.mozilla.org/r/189420/#review196822

Comment 15

[Nevin Chen(Not active on Bugzilla)]

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

Comment 16

[Nevin Chen(Not active on Bugzilla)]

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

Comment 17

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8920303 [details]
Bug 1403653 - Part 1 - Refactor getDominantColor.

https://reviewboard.mozilla.org/r/189420/#review196828

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 20

[Jan Henning [:JanH]]

Forgot to update moz.build as well for the changed library usage.

Comment 21

[Jan Henning [:JanH]]

Comment on attachment 8920302 [details]
Bug 1403653 - Part 0 - Cleanup imports.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189418/diff/1-2/

Comment 22

[Jan Henning [:JanH]]

Comment on attachment 8920303 [details]
Bug 1403653 - Part 1 - Refactor getDominantColor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189420/diff/1-2/

Comment 23

[Jan Henning [:JanH]]

Comment on attachment 8920304 [details]
Bug 1403653 - Part 2 - Split out conversion from ColorRes to ColorInt into separate function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189422/diff/1-2/

Comment 24

[Jan Henning [:JanH]]

Comment on attachment 8920305 [details]
Bug 1403653 - Part 3 - Use the Lightweight Theme accent colour for the status bar in normal mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189424/diff/1-2/

Comment 25

[Pulsebot]

Pushed by [e-mail]:
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

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1d10c858233d
https://hg.mozilla.org/mozilla-central/rev/6869117e9c70
https://hg.mozilla.org/mozilla-central/rev/007b3dd7e32b
https://hg.mozilla.org/mozilla-central/rev/13f88bb8b30e

Comment 27

[Oana Horvath [:ohorvath]]

Verified as fixed on Nightly 58.0a1.
Devices:
Galaxy S8 (Android 7.0)
LG G4 (Android 6.0)

Comment 28

[Ioana Chiorean]

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.

Comment 29

[Nicole Yee (:nicoleyee)]

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.

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 31

[Wesly Huang (EPM)]

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.

Comment 32

[Jan Henning [:JanH]]

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

Comment 33

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

Gerry, another one for 58. 
Maybe something like "Added the ability to change the status bar color in themes"

Comment 34

[Gerry Chang [:gchang]]

Liz, I've already added in the notes.

Comment 35

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fennec 58 release notes.