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

VERIFIED FIXED in 2.2 S5 (6feb)

Status

defect
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: onelson, Assigned: birtles)

Tracking

({regression})

unspecified
2.2 S5 (6feb)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.1 unaffected, b2g-v2.2 verified, b2g-master verified)

Details

(Whiteboard: [2.2-Daily-Testing][systemsfe], )

Attachments

(3 attachments, 4 obsolete attachments)

Reporter

Description

5 years ago
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
Reporter

Comment 1

5 years ago
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/
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
Keywords: regression
Whiteboard: [2.2-Daily-Testing]
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.
Component: Gaia::E-Mail → Gaia::System
Whiteboard: [2.2-Daily-Testing] → [2.2-Daily-Testing][systemsfe]
Bad user experience (status bar is difficult to distinguish) and is a regression.

Requesting a window.
blocking-b2g: --- → 2.2?
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
QA Contact: pcheng
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.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Brian, could you take a look at this please? It looks like the patches for bug 927349 caused this issue.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker) → needinfo?(bbirtles)
blocking-b2g: 2.2? → 2.2+
Assignee

Comment 6

5 years ago
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.
Assignee

Comment 7

5 years ago
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.
Assignee

Comment 8

5 years ago
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.
Flags: needinfo?(bbirtles)
Assignee

Comment 9

5 years ago
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?
Assignee

Comment 10

5 years ago
Posted patch Possible fix (obsolete) — Splinter Review
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.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Assignee

Comment 11

5 years ago
Posted file Github pull request (obsolete) —
I have no idea what I am doing here.
Attachment #8545062 - Flags: review?(21)
Assignee

Comment 12

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

Comment 13

5 years ago
Never mind, looks like that happens automatically in Gaia-land.
Assignee

Comment 14

5 years ago
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
Attachment #8545062 - Flags: review?(21)
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.
Assignee

Comment 16

5 years ago
(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.
Assignee

Comment 17

5 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #8545036 - Attachment is obsolete: true
Attachment #8545062 - Attachment is obsolete: true
Assignee

Comment 18

5 years ago
Posted file Possible fix v2 (obsolete) —
Assignee

Comment 19

5 years ago
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
Attachment #8547874 - Flags: review?(21)
Assignee

Updated

5 years ago
Depends on: 927349
Could you guys verify if [1], which landed recently, helped to this bug?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1115829
Flags: needinfo?(bbirtles)
Assignee

Comment 21

5 years ago
(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.
Flags: needinfo?(bbirtles)
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.
Duplicate of this bug: 1120864
Duplicate of this bug: 1121346
Assignee

Comment 25

5 years ago
KTucker, Chris, any suggestions on who I can get to review this? Vivien doesn't seem to be around.
Flags: needinfo?(ktucker)
Flags: needinfo?(chrislord.net)
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.
Flags: needinfo?(chrislord.net)
Attachment #8547874 - Flags: review?(alive)
Attachment #8547874 - Flags: review+
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?
Attachment #8547874 - Flags: review?(alive) → review?(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.
Attachment #8547874 - Flags: review?(kgrandon) → feedback+
Flags: needinfo?(ktucker)
Assignee

Updated

5 years ago
Attachment #8547874 - Flags: review?(21)
Assignee

Comment 29

5 years ago
(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).
(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 :)
Assignee

Comment 31

4 years ago
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.
Assignee

Comment 32

4 years ago
Posted file PR for landing
Carrying forward r+ from comment 26 (and comment 30).
Attachment #8546425 - Attachment is obsolete: true
Attachment #8547874 - Attachment is obsolete: true
Attachment #8555657 - Flags: review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/57b455f62bdac4b614b9135123dc7ef896df4833
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S5 (6feb)
Assignee

Updated

4 years ago
Blocks: 1127155
Assignee

Comment 34

4 years ago
(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.
Please request Gaia v2.2 approval on this when you get a chance.
Flags: needinfo?(bbirtles)
Assignee

Comment 36

4 years ago
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
Flags: needinfo?(bbirtles)
Attachment #8555657 - Flags: approval-gaia-v2.2?
Attachment #8555657 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
Assignee

Comment 37

4 years ago
Posted file 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.
Assignee

Comment 38

4 years ago
Adding checkin-needed for uplift to 2.2
Keywords: checkin-needed
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 :)
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.