1118361 - [Email] Status Bar displays white icons on white background after in Settings of App

Status: VERIFIED FIXED

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

Description

[Oliver Nelson [:oliverthor]]

Attachment: logcat_20150106_1008.txt

Description:
When a user navigates through the Settings of the Email app, they will observe that the status bar icons are white on a white background and are difficult to see. The pages this can be observed :
* Directly following signup of a new email account
* Settings after tapping gear icon in top left corner of Inbox/Drawer view
   
Repro Steps:
1) Update a Flame device to BuildID: 20150106010234
2) Open the 'Email' App.
3) Manually setup an email account.
4) After confirming details, observe status bar.
5) Navigate to Inbox/Drawer View.
6) Navigate to Settings via Gear icon in top left after Shelves icon.
7) Observe status bar in Settings.
  
Actual:
Status Bar Icons display white on a white background and are difficult to see.
  
Expected: 
Status Bar Icons color contrast with the background of the status bar so that they are easily visible.
  
Environmental Variables:
Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20150106010234
Gaia: b77e0d56d197e0ee02d801a25c784130d888c9db
Gecko: 2a193b7f395c
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 37.0a1 (2.2 Master)
Firmware: V18D
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
  
  
Repro frequency: 5/5
See attached: 
video- http://youtu.be/_9COAUOug2E
logcat

Comment 1

[Oliver Nelson [:oliverthor]]

Issue DOES NOT REPRO on flame 2.1 devices:
Results: Status Bar Icons color contrast with the background of the status bar so that they are easily visible.

Environmental Variables:
-------------------------
[unable to pull full values at this time, will update later]
Device: Flame 2.1 KK (319mb)(Kitkat Base)(Full Flash)
BuildID: 20150106001308
Firmware: v18D
Pulled from pvt: nightly/mozilla-b2g34_v2_1-flame-kk/latest/

Comment 2

[James Burke [:jrburke]]

Switching over to systemsfe: email just indicates the background theme-color for statusbar, and does not control the contrast or adjust of color of the icons.

Comment 3

[Peter Bylenga [:PBylenga]]

Bad user experience (status bar is difficult to distinguish) and is a regression.

Requesting a window.

Comment 4

[Pi Wei Cheng [:piwei] (inactive)]

mozilla-inbound regression window:

Last Working Environmental Variables:
Device: Flame
BuildID: 20150104211243
Gaia: c2bf20d23851d5fda9f8f0ef0267db5f49152376
Gecko: b9f40d0310d5
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

First Broken Environmental Variables:
Device: Flame
BuildID: 20150105033543
Gaia: 4ceeff19086b2a2955f044ad923dcfa63a293de3
Gecko: b9ba4d717f8a
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

First Broken Gaia & Last Working Gecko - issue does NOT repro
Gaia: 4ceeff19086b2a2955f044ad923dcfa63a293de3
Gecko: b9f40d0310d5

First Broken Gecko & Last Working Gaia - issue DOES repro
Gaia: c2bf20d23851d5fda9f8f0ef0267db5f49152376
Gecko: b9ba4d717f8a

Gecko pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b9f40d0310d5&tochange=b9ba4d717f8a


Possibly caused by patches for Bug 927349.

Comment 5

[KTucker [:KTucker][Inactive 3/4/2016]]

Brian, could you take a look at this please? It looks like the patches for bug 927349 caused this issue.

Comment 6

[Brian Birtles (:birtles)]

I'm having trouble reproducing this. With trunk Gaia and trunk Gecko, after tapping the "E-mail" app icon I just get a white screen with the rocket bar along the top.

Comment 7

[Brian Birtles (:birtles)]

After restarting the device the mail app works again and without following the STR, simply by loading the Mail app I can see the white text on the status bar during the New Account page is hard to read.

Comment 8

[Brian Birtles (:birtles)]

Ok, I have theory about what is going on.

When we switch to the Mail app or change screens we end up calling AppChrome.setThemeColor

https://github.com/mozilla-b2g/gaia/blob/b77e0d56d197e0ee02d801a25c784130d888c9db/apps/system/js/app_chrome.js#L504

That sets the background-color on the AppChrome div element which triggers a transition over 0.5s to the specific color.

Then we run a rAF loop to check what the brightness of the background is on each frame of the transition. When the brightness goes above 200 we update the AppWindow div, adding the 'light' class.

However, within that loop we check if the background-color is the same between subsequent frames. We use that to check if the transition has finished or is a null-op and then we stop running the rAF loop.

With bug 927349 we delay the start of an animation until it has painted its first frame. That may mean that the computed background-color before the transition starts and the computed background-color compare equal. That, in turn, will mean that our rAF loop will think that the transition has finished and will abort. As a result, the 'light' class never gets applied.

Comment 9

[Brian Birtles (:birtles)]

The idea of comparing the computed style to tell if an animation has finished is brittle. Two subsequent frames of an animation could produce the same computed style depending on the timing function being used. Likewise, given a high frame rate, and a small delta between the transition end points, any rounding applied to the computed background-color could also mean that subsequent frames compare equal even while the animation is in play.

The long term solution will be to use the Web Animations API which gives reliable information about the state of animations running on an element. However, it's not pref'ed on yet for release builds so it doesn't help here.

Listening for transitionend events may work but transitionend events aren't dispatched if that DOM element is dropped. If we never delete the AppChrome DOM element (or, if when we do we no longer need the transitionend event) then that may be a more robust solution here?

Comment 10

[Brian Birtles (:birtles)]

Attachment: Possible fix

This fixes the bug for me. It's going to take me a while to work out how to convert this into a gaia pull request.

Comment 11

[Brian Birtles (:birtles)]

Attachment: Github pull request

I have no idea what I am doing here.

Comment 12

[Brian Birtles (:birtles)]

Also, excuse this Gaia novice, but how do I run tests on this changeset? Can I even push this to tryserver?

Comment 13

[Brian Birtles (:birtles)]

Never mind, looks like that happens automatically in Gaia-land.

Comment 14

[Brian Birtles (:birtles)]

Comment on attachment 8545062 [details] [review]
Github pull request

Cancelling review, clearly that patch is broken based on what I'm seeing on try:
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=19afa39b35fb

Comment 15

[Chris Lord [:cwiiis]]

Comment on attachment 8545036 [details] [diff] [review]
Possible fix

Review of attachment 8545036 [details] [diff] [review]:
-----------------------------------------------------------------

Fly-by comments, do with these as you wish :) (I'm not a good reviewer for app_chrome.js, but I've worked with the code a bit)

::: apps/system/js/app_chrome.js
@@ +522,5 @@
> +      self.element.removeEventListener('transitionend', endBackgroundFade);
> +      clearTimeout(safetyTimeout);
> +    };
> +    this.element.addEventListener('transitionend', endBackgroundFade);
> +    safetyTimeout = setTimeout(endBackgroundFade, 1000);

I'm not a fan of these safety timeouts - we should either just use the timeout or juse use transitionend - if transitionend isn't firing, we should know why and fix/work around that.

If there's a common case where transitionend is circumvented in this code, we should just stick with the timeout (and change the timeout value to match the length of the animation, which I assume doesn't take a whole second?)

@@ +543,5 @@
>          Math.sqrt((r*r) * 0.241 + (g*g) * 0.691 + (b*b) * 0.068);
>  
>        self.app.element.classList.toggle('light', brightness > 200);
> +      // XXX Do we really need to publish this on every frame of an animation
> +      // or just when we actually toggle the 'light' class?

I can't possibly imagine there's a good reason to publish this on every frame and it sounds like a performance nightmare - we should only publish this when it changes. If that breaks something, let's fix that something.

Comment 16

[Brian Birtles (:birtles)]

(In reply to Chris Lord [:cwiiis] from comment #15)
> Comment on attachment 8545036 [details] [diff] [review]
> Possible fix
> 
> Review of attachment 8545036 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Fly-by comments, do with these as you wish :) (I'm not a good reviewer for
> app_chrome.js, but I've worked with the code a bit)
> 
> ::: apps/system/js/app_chrome.js
> @@ +522,5 @@
> > +      self.element.removeEventListener('transitionend', endBackgroundFade);
> > +      clearTimeout(safetyTimeout);
> > +    };
> > +    this.element.addEventListener('transitionend', endBackgroundFade);
> > +    safetyTimeout = setTimeout(endBackgroundFade, 1000);
> 
> I'm not a fan of these safety timeouts - we should either just use the
> timeout or juse use transitionend - if transitionend isn't firing, we should
> know why and fix/work around that.

I was concerned about hitting this:

 http://mxr.mozilla.org/mozilla-central/source/layout/style/nsTransitionManager.cpp?rev=b2c5a67125d6#448

That is, if something else triggered a transition to something non-interpolable we wouldn't get the transitionend event.

However, for background-color I don't think there's anything that triggers that code path. At least not from my testing here:

 http://jsfiddle.net/y24rupf7/

So I've removed the safety timeout in favour of relying on transitionend events.

> @@ +543,5 @@
> >          Math.sqrt((r*r) * 0.241 + (g*g) * 0.691 + (b*b) * 0.068);
> >  
> >        self.app.element.classList.toggle('light', brightness > 200);
> > +      // XXX Do we really need to publish this on every frame of an animation
> > +      // or just when we actually toggle the 'light' class?
> 
> I can't possibly imagine there's a good reason to publish this on every
> frame and it sounds like a performance nightmare - we should only publish
> this when it changes. If that breaks something, let's fix that something.

Great. I've fixed this code too at the same time and updated the corresponding tests. This should really go in a separate patch but I'm such a git novice I just lumped them altogether.

Review request forthcoming once I see how try likes it.

Comment 17

[Brian Birtles (:birtles)]

Attachment: Patch v2

It took me a while, but I think I finally worked out why try was failing.

Try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=69a7acecbc80

The long weekend starts here in about 10 minutes so if this is urgent let me know and I'll try to set up the PR over the weekend.

Comment 18

[Brian Birtles (:birtles)]

Attachment: Possible fix v2

Comment 19

[Brian Birtles (:birtles)]

Comment on attachment 8547874 [details] [review]
Possible fix v2

Try run looks good so requesting review:
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=9fdb3dbfc537

Comment 20

[Alberto Pastor [:albertopq]]

Could you guys verify if [1], which landed recently, helped to this bug?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1115829

Comment 21

[Brian Birtles (:birtles)]

(In reply to Alberto Pastor [:albertopq] from comment #20)
> Could you guys verify if [1], which landed recently, helped to this bug?
> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1115829

That doesn't appear to be related. It has a completely different root cause.

Comment 22

[Alberto Pastor [:albertopq]]

Well, the fact that we were using a mozElement as statusbar and after bug 1115829 we don't anymore, I think it could affect. I might be wrong.

Comment 25

[Brian Birtles (:birtles)]

KTucker, Chris, any suggestions on who I can get to review this? Vivien doesn't seem to be around.

Comment 26

[Chris Lord [:cwiiis]]

Comment on attachment 8547874 [details] [review]
Possible fix v2

I'm giving this a + because it's better than what's there at the moment, but I have a comment on github that I think would improve things more still.

If this needs to land soon, I think you can take my review, but I'd feel better waiting for alive's review too. (and I'd feel even better if you addressed the comment :))

Not clearing ktucker's n? in case he has a different opinion.

Comment 27

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8547874 [details] [review]
Possible fix v2

I wrote AppChrome but the rAF stuff was added by Vivien and reviewed by Kevin :)

+1 curious about why rAF is necessary. Isn't there a way not to use it?

Comment 28

[Kevin Grandon :kgrandon]

Comment on attachment 8547874 [details] [review]
Possible fix v2

I'm fine going with Chris's review for now. I am concerned about dropping the safety timeout. This has caused us a lot of problems in the past, and until we get web animations, I'd recommend that we leave it in there. It looks like if we miss the transitionend event, we might continuously get rAF() calls which would not be good. I'd recommend landing with the timeout, with the assumption that we will make this go away by swapping to a single setTimeout, or using web animations.

To make this easy, we already have a global "eventSafety" library loaded in the system app. You can leverage this like: https://github.com/mozilla-b2g/gaia/blob/37e6bd961aef9f9c958e4d19ff25ad62a775eb8d/apps/system/js/rocketbar.js#L121

The idea was that we use this everywhere for now, and once we have web animations, we'll grep and remove all instances of it.

Comment 29

[Brian Birtles (:birtles)]

(In reply to Kevin Grandon :kgrandon [INACTIVE - heads down on Gij Issue] from comment #28)
> I'm fine going with Chris's review for now. I am concerned about dropping
> the safety timeout. This has caused us a lot of problems in the past, and
> until we get web animations, I'd recommend that we leave it in there. It
> looks like if we miss the transitionend event, we might continuously get
> rAF() calls which would not be good. I'd recommend landing with the timeout,
> with the assumption that we will make this go away by swapping to a single
> setTimeout, or using web animations.
> 
> To make this easy, we already have a global "eventSafety" library loaded in
> the system app. You can leverage this like:
> https://github.com/mozilla-b2g/gaia/blob/
> 37e6bd961aef9f9c958e4d19ff25ad62a775eb8d/apps/system/js/rocketbar.js#L121

I've added this to the PR now.

(In reply to Chris Lord [:cwiiis] from comment #26)
> Comment on attachment 8547874 [details] [review]
> Possible fix v2
> 
> I'm giving this a + because it's better than what's there at the moment, but
> I have a comment on github that I think would improve things more still.

I agree we should just detect when the bright/dark transition should occur and use setTimeout but I think that should be a follow-up. I'd rather keep this bug just to fixing the regression in the most straightforward way possible and then optimize later (especially because I can't really put more time into this right now).

Comment 30

[Chris Lord [:cwiiis]]

(In reply to Brian Birtles (:birtles) from comment #29)
> I agree we should just detect when the bright/dark transition should occur
> and use setTimeout but I think that should be a follow-up. I'd rather keep
> this bug just to fixing the regression in the most straightforward way
> possible and then optimize later (especially because I can't really put more
> time into this right now).

Fair, and my r+ stands - if you could file the bug, cc me on it and add 'polish' to the keywords, that'd be really great :)

Comment 31

[Brian Birtles (:birtles)]

I hadn't marked this for landing because I was seeing all sorts of failures on try.

However, I noticed that what appears to be a push from master to try is giving the same failures:

  https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=83b9898b24bb

I'm going to squash up all these commits and then mark this for checkin.

Comment 32

[Brian Birtles (:birtles)]

Attachment: PR for landing

Carrying forward r+ from comment 26 (and comment 30).

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/57b455f62bdac4b614b9135123dc7ef896df4833

Comment 34

[Brian Birtles (:birtles)]

(In reply to Chris Lord [:cwiiis] from comment #30)
> (In reply to Brian Birtles (:birtles) from comment #29)
> > I agree we should just detect when the bright/dark transition should occur
> > and use setTimeout but I think that should be a follow-up. I'd rather keep
> > this bug just to fixing the regression in the most straightforward way
> > possible and then optimize later (especially because I can't really put more
> > time into this right now).
> 
> Fair, and my r+ stands - if you could file the bug, cc me on it and add
> 'polish' to the keywords, that'd be really great :)

Filed bug 1127155 for this.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

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

Comment 36

[Brian Birtles (:birtles)]

Comment on attachment 8555657 [details] [review]
PR for landing

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 927349
[User impact] if declined: Very difficult to read status bar text in some situations
[Testing completed]: Patch includes tests, has been on trunk for about 1 day
[Risk to taking this patch] (and alternatives if risky): No known risks or side-effects at this time.
[String changes made]: None

Comment 37

[Brian Birtles (:birtles)]

Attachment: Gaia 2.2 PR

Sorry, I'm not too familiar with Gaia landing procedures so I'm not sure if a second PR is needed for landing on 2.2.

Comment 38

[Brian Birtles (:birtles)]

Adding checkin-needed for uplift to 2.2

Comment 39

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/c100488f9bc358e944fca6ebfe5fd30fcb905e14

You don't need to set checkin-needed for uplifts. In fact, it's preferred you not so as to not clutter up the different bug queries :)

Comment 40

[Pi Wei Cheng [:piwei] (inactive)]

This issue is verified fixed on Flame 2.2 and on Flame 3.0.

Observed behavior: Status bar now displays gray colored icons on white colored background, correctly contrasted and easy to read, at step 4 and step 7 of original STR.

Device: Flame 2.2
BuildID: 20150203002504
Gaia: cd62ff9fe199fb43920ba27bd5fdbc5c311016fc
Gecko: 11d93135c678
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Device: Flame 3.0 Master
BuildID: 20150203055641
Gaia: ae5a1580da948c3b9f93528146b007fc4f6a712b
Gecko: ae5d04409cd9
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0 Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0