1055746 - [Search/Rocketbar] Choice of icon color when status bar color is changed sometimes seems non-optimal, not consistent

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Search, defect)

Description

[James Burke [:jrburke]]

This is related to bug 1041625, and noticed after implementing theme-color switching in email tracked in bug 1041904:

Sometimes the icon color choice does not seem to be the best as far as contrast or readability, when some of the status bar color changes in email. Examples to try with latest master email app:

1) tap menu icon to open drawer, tap settings gear to go to white settings page. The icons in the status bar are white, where a darker color would read better.

2) when first launching email, icons are white on the orange background. After going to Mail Settings, as in the first point, going back to message list, the icons are now dark. I expected a consistent icon color choice based on the background color.

Comment 2

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

Attachment: [screenshot] wrong icon colors

Comment 3

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

Etienne, do you have any idea what is causing this inconsistency?

Comment 4

[Etienne Segonzac (:etienne)]

Redirecting since I was on PTO while this was implemented/reviewed :)

Comment 5

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

Alberto, can you help us figure out what is going on here?

Comment 6

[Alberto Pastor [:albertopq]]

Sure, I'll take a look

Comment 7

[Eric Pang [:epang] UX]

(In reply to Alberto Pastor [:albertopq] from comment #6)
> Sure, I'll take a look

Just a note, but the same thing seems to have in the FTE the first time the phone boots up.  (Light icons on a light status bar).  But if you manually launch the FTE from the settings later on the correct icons seem to be used.

Comment 8

[Alberto Pastor [:albertopq]]

After deep digging into this bug, I found out that the problem is on the setThemeColor method (app_chrome.js.)

Every time the color to be updated is set in rbg() format (hex format, and literal colors like 'black', work fine), the computed style after requestAnimationFrame is always rgb(0,0,0).

We can workaround it, by transforming rgb formatted colors to hex before setting the background or by delaying with a setTimeout getting the computedStyle, but I guess there is something plattform wise.

What should we do?

Comment 9

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

Hmmm, I'm not sure we can tranform rgb to hex because I think we still want to support transparencies. I'm actually not sure about that, but just thinking out loud. In any case, we have a little time here so let's see if we can fix the platform issue. If not, it's good you have some workarounds in mind.

Vivien, any ideas about comment 8 regarding rgb changes not showing up in computed style on the next animation frame?

Comment 10

[Alberto Pastor [:albertopq]]

After checking again, it seems that the problem is not related to hex or rgb. That was just an unfortunate coincidence.

The problem comes with the transition on the background color of the chrome. As we are requesting only the first frame, we'll get the previous computed background color and the brightness will be, in most of cases, wrong. We need to keep requesting frames until it ends the transition. I'll work on that tomorrow.

Comment 11

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

(In reply to Alberto Pastor [:albertopq] from comment #10)
> After checking again, it seems that the problem is not related to hex or
> rgb. That was just an unfortunate coincidence.
> 
> The problem comes with the transition on the background color of the chrome.
> As we are requesting only the first frame, we'll get the previous computed
> background color and the brightness will be, in most of cases, wrong. We
> need to keep requesting frames until it ends the transition. I'll work on
> that tomorrow.

Good catch! Could we listen for the 'transitionend' event on the statusbar instead of polling with requestAnimationFrame?

Comment 12

[Alberto Pastor [:albertopq]]

I don't think transitionend would work here for 2 reasons:

- There are cases in which the background does not actually transition, so we don't receive the event.
- In the cases we have the event, switching the icons color just at the end doesn't look smooth. I mean, we need to change the color specifically when the brightness is too high/low, and that might be in the middle of a transition.

If we could use the color passed by parameter to the setThemeColor(color) method, we would be able to decide the icons without polling, but my guess is that we are using filters, and then we need to get the computed style, so I don't see any other option.

I'll send a PR. Let me know if you find any other solution.
Thanks!

Comment 13

[Alberto Pastor [:albertopq]]

Attachment: Link to Github Pull Request: https://github.com/mozilla-b2g/gaia/pull/24135

Comment 14

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

Comment on attachment 8490661 [details] [review]
Link to Github Pull Request: https://github.com/mozilla-b2g/gaia/pull/24135

Seems reasonable enough. The only thing that worries me is I don't know how expensive the brightness calculation is, and whether it can cause jank. Usually when an app changes the theme color they are also transitioning elements on the page, and we don't want to add jank to those transitions. Forwarding the review to Vivien though, since I can't think of a better way to do this (other than use the color passed in to set theme color).

Comment 15

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

[Blocking Requested - why for this release]:

We need to block on this. Inconsistent UI with light on light fonts is confusing at best and unusable at worst.

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8490661 [details] [review]
Link to Github Pull Request: https://github.com/mozilla-b2g/gaia/pull/24135

I guess one possible way of implementing this would have been to use a 'test' dom element, that does not suffer from any animation/transition and use it as our way to retrieve the computed color.

That said the brightness calculation should be very straighforward, while the icons source swap that may result of a change may be more expensive. But I don't think the proposed solution makes the situation worse.

Comment 17

[Alberto Pastor [:albertopq]]

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

Comment 18

[Candice Serran (:cserran)]

blocking as it's very poor functionality. ux change. low risk patch

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Please request Gaia v2.1 approval on this when you get a chance.

Comment 20

[Alberto Pastor [:albertopq]]

Comment on attachment 8490661 [details] [review]
Link to Github Pull Request: https://github.com/mozilla-b2g/gaia/pull/24135

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): -
[User impact] if declined: The statusbar icons are hardly visible on some apps
[Testing completed]: Added unit tests for all the cases
[Risk to taking this patch] (and alternatives if risky): There is a small risk of a performance regression, but according to :vingtetun and the tests I've been doing, it doesn't seem to affect
[String changes made]: none

Comment 22

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/36a5a5cd4ea58c9f64778547572a910a2b91a042

Comment 23

[[:RJ]]

this issue no longer occurs in Flame 2.2 Master KK (319mb) (Full Flash) 2.1 KK (319mb) (Full Flash)


All of the areas mentioned, FTU and EMAIL, have a legible and easy to read color scheme. And both versions seem to have a consistent color flow as well.


Flame 2.2 Master KK (319mb) (Full Flash)

Device: Flame 2.2 Master
BuildID: 20141011040204
Gaia: 95f580a1522ffd0f09302372b78200dab9b6f322
Gecko: 3f6a51950eb5
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

2.1 KK (319mb) (Full Flash)

Environmental Variables:
Device: Flame 2.1 KK (319mb) (Full Flash)
Build ID: 20141010000201
Gaia: d71f8804d7229f4b354259d5d8543c25b4796064
Gecko: 7fa82c9acdf2
Version: 34.0a2 Flame 2.1 KK (319mb)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0