1062326 - [FTE] Status bar should be dark on dark FTE screens

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::First Time Experience, defect)

Description

[Eric Pang [:epang] UX]

The FTE contains some dark screens where we should update the status bar.

This occurs when searching for wifi and in the tour.

The status bar should use the specs for media screens
https://mozilla.box.com/s/6a3zw86uyu27slbk4yc8

Comment 1

[Michael Henretty [:mikehenrty][:mhenretty]]

[Blocking Requested - why for this release]:

FTU is the first thing a user sees, and the current experience is pretty jarring when we get a dark screen in the FTU. I believe the level of effort to reward for this bug is high.

Comment 2

[Candice Serran (:cserran)]

not blocking release but priority bug for backlog

Comment 3

[Manuel Casas Barrado [:mancas]]

It seems that when the status bar theme is changed twice, icons don't change accordingly. Could it be a bug?

Comment 4

[Manuel Casas Barrado [:mancas]]

Attachment: Status bar color changed

The color is changed in dark screens, however icons doesn't change, as I said in the previous comment, and after talking with etienne it could be a bug.

Comment 5

[Guillaume Marty [:gmarty]]

I applied your patch and can replicate the bug. It appears that when the colour is changed, the CSS class 'light' is removed from the corresponding appWindow element (child of #window in the system app).

That said, I'm not familiar enough with the theme-color thing to tell what's wrong.

Comment 6

[Manuel Casas Barrado [:mancas]]

Take a look at my comments on github. Thanks!

Comment 7

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8485643 [details] [review]
Status bar color changed

Sorry I'm a bit overloaded with other stuf, so I'm forwarding this to another FTU peer.

Fernando, sorry for adding you more workload, could you take a look to this, or find someone to do it?

Thanks

Comment 8

[Fernando Campo (:fcampo)]

Comment on attachment 8485643 [details] [review]
Status bar color changed

Tested on flame, works nice, the FTU code also good, but few concerns:
- needs testing (I understand that this is a WIP)
- hardcoding the colours doesn't seem like a good idea if those are gonna be used from other apps, I'd go for shared file, or CSS global variables.
- need to check with UX if the delay that can be seen on the colour change is acceptable for situations like the overlay showing (the statusbar change is a little slower than the overlay animation, so it might seem like flashing colours)
- changes on system app gonna need system peer review.

Apart from those things, it looks really nice, thanks Manuel!

Comment 9

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8485643 [details] [review]
Status bar color changed

I've taken care of Fernando's comments. Please take a look at the system app code.

Thanks

Comment 10

[Etienne Segonzac (:etienne)]

Comment on attachment 8485643 [details] [review]
Status bar color changed

Wow, can you explain the patch a bit more?

I don't think we want to maintain a color JSON file like this, the whole point of the getComputedStyle [1] is to handle properly cases like |chartreuse| and all.

[1] https://github.com/mozilla-b2g/gaia/blob/f5e41cebfcafa475ef62aebc7a72448af021cc11/apps/system/js/app_chrome.js#L474

Comment 11

[Manuel Casas Barrado [:mancas]]

You're right but when we call setThemeColor twice consecutively, getComputedStyle retrieve the previous background color which consequence is that the icons don't change is color accordingly with statusbar color.

For this reason I've made the JSON list. Imagine we have an element with backgroundColor='red' so if we ask it for his background value we will obtain 'red' instead of a hex color. If we use the JSON to convert string colors to hex color the problem is solved. And then we can calculate the brightness

I don't really like this solution. If you know other way to avoid the wrong retrieving of colors using getComputedStyle, please, let me know

Comment 12

[Etienne Segonzac (:etienne)]

(In reply to Manuel Casas Barrado [:mancas] from comment #11)
> You're right but when we call setThemeColor twice consecutively,
> getComputedStyle retrieve the previous background color which consequence is
> that the icons don't change is color accordingly with statusbar color.
> 

Thanks for identifying the race and the issue, much clearer for me now.
I'm pretty sure this part of the bug is actually bug 1055746, and good news: it's fixed :)

So we'll just need the FTU part here.

Comment 13

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8485643 [details] [review]
Status bar color changed

Hey Fernando!

As Etienne said, it seems the race condition is now fixed. However I still having the same problem, icons on FTU are white instead of gray.
Take a look at the PR and please, answer my question in github.

Thanks!

Comment 14

[Fernando Campo (:fcampo)]

Comment on attachment 8485643 [details] [review]
Status bar color changed

after reading about CSS Globals and checking how other apps deal with status bar color change (they basically don't), I changed my opinion about storing the colors externally, I think we can hardcode them in the app and come back to it later if needed, so sorry for wasting your time.

The code looks nice, and tested on device works perfectly (apart from the delay already commented, but I don't think is an issue).

In the future we might want to add the color change code to the overlay class (after all, it is the overlay the one changing the base color of the app), but if we decide so, will be in a different bug.

Many thanks for taking care of this Manuel, and your patience.

Comment 15

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8485643 [details] [review]
Status bar color changed

Let's get a quick UI review before landing.

Comment 16

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Fernando Campo (:fcampo) from comment #14)
> Comment on attachment 8485643 [details] [review]
> Status bar color changed
> 
> after reading about CSS Globals and checking how other apps deal with status
> bar color change (they basically don't), I changed my opinion about storing
> the colors externally, I think we can hardcode them in the app and come back
> to it later if needed, so sorry for wasting your time.

Just FYI, email is the only other app that dynamically changes statusbar color at this time. So we are making up best practices as we go along.

Comment 17

[Eric Pang [:epang] UX]

Hey Manuel, thanks for working on this, it's looking great!  Looking at it though the change to the dark status bar for the 'scanning for networks' looks good since it's so fast.  I think we should either leave as a light status bar or if possible update the screen to be light instead of dark.  Jacqueline, is it possible to update the screen to be light since it's part of the fte?  Or would this be breaking the pattern?

Comment 18

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Eric Pang [:epang] from comment #17)
> Hey Manuel, thanks for working on this, it's looking great!  Looking at it
> though the change to the dark status bar for the 'scanning for networks'
> looks good since it's so fast.  I think we should either leave as a light
> status bar or if possible update the screen to be light instead of dark. 
> Jacqueline, is it possible to update the screen to be light since it's part
> of the fte?  Or would this be breaking the pattern?

See also: https://bugzilla.mozilla.org/show_bug.cgi?id=1069306

Comment 19

[Eric Pang [:epang] UX]

(In reply to Michael Henretty [:mhenretty] from comment #18)
> (In reply to Eric Pang [:epang] from comment #17)
> > Hey Manuel, thanks for working on this, it's looking great!  Looking at it
> > though the change to the dark status bar for the 'scanning for networks'
> > looks good since it's so fast.  I think we should either leave as a light
> > status bar or if possible update the screen to be light instead of dark. 
> > Jacqueline, is it possible to update the screen to be light since it's part
> > of the fte?  Or would this be breaking the pattern?
> 
> See also: https://bugzilla.mozilla.org/show_bug.cgi?id=1069306

I added a comment to the other bug, but to sum up I spoke with Jacqueline and we think the screen should be restyled to be light.

Comment 20

[Eric Pang [:epang] UX]

Comment on attachment 8485643 [details] [review]
Status bar color changed

I'm giving this patch an R+ since I don't want to hold up anything landing :)

But the status bar colour change on the 'scanning' screen should be removed and stay light. Everything else look great.

Once the scanning screen is restyled light in bug 1069306 this should be good!

Thanks Manual for your work on this!

Comment 21

[Manuel Casas Barrado [:mancas]]

Thanks for the R+ Eric. I'll take care of bug1069306

Comment 22

[Carsten Book [:Tomcat]]

master: https://github.com/mozilla-b2g/gaia/commit/c79d5498e18afa0a1697e230f77bf28b5ddbe2f4

Comment 23

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8485643 [details] [review]
Status bar color changed

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
bug 1041625

[User impact] if declined:
FTU app UX has some very strange looking screens where the page background is dark, but the statusbar is light. It looks like a bug.

[Testing completed]:
Tested manually by UX and approved. Patch also adds unit tests for theme color changes.

[Risk to taking this patch] (and alternatives if risky):
This patch is low risk since it only deals with changing a meta-tag in the FTU app. Risk would be to have inconsistent statusbar color with FTU background, but that is the problem we already have.

[String changes made]: none.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/9f0eba608d371ec95c930846ecd45c5dd76997b7