Kyle Huey (Exited; not receiving bugmail, old account, do not use) Reporter
	Description • 10 years ago

I've seen this happen a few times now.  Don't have STR, but it seems to involve letting the screen time out when we're on an app that makes the system tray dark.

	Kevin Grandon :kgrandon
	Comment 2 • 10 years ago

I will look into this or find someone to do so.

	Kevin Grandon :kgrandon
	Comment 3 • 10 years ago

I haven't found a clear STR yet, but I'll keep an eye out for it.

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 4 • 10 years ago

What build were you using? I'm pretty sure this is a dupe of bug 1055746, but that was fixed last week and uplifted to 2.1 yesterday.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Reporter
	Comment 5 • 10 years ago

Not sure.  I definitely have picked up bug 1055746 by now though, so I'll let you know if I see this again.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Reporter
	Comment 6 • 10 years ago

Nope, just had it happen right now.

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 7 • 10 years ago

Alberto, you worked on bug 1055746, any idea what is going on here?

	Kevin Grandon :kgrandon
	Comment 8 • 10 years ago

I wonder if this is a race due to the fact that we set the "light" class inside of a requestAnimationFrame, and statusbar calls setAppearance when appopened is received. Seems like it could be a bit racey.

	Alberto Pastor [:albertopq] Assignee
	Comment 9 • 10 years ago

Yeah, I think Kevin is right. I'll take a look

	Alberto Pastor [:albertopq] Assignee
	Comment 10 • 10 years ago

I think I found stable STR:

1.- With screen lock enabled, open an app with dark icons (Browser, for instance)
2.- Long press the home button in order to access the card view
3.- Press the power button to get the screen to sleep
4.- Press again to wake the screen up

Expected:

The icons are light

Actual:

The icons are dark

Wondering which versions are affected. Adding qawanted.

I'll keep working tomorrow on it.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Reporter
	Comment 11 • 10 years ago

That reproduces it for me too.

	Cody Roesch [:croesch]
	Comment 12 • 10 years ago

This bug repro's on Flame KK builds: Flame 2.2 KK, Flame 2.1 KK, OpenC 2.2

Actual Results: Dark Icons appearing on the Lockscreen when certain apps are in cardview when phone is locked.

Repro Rate: 10/10

Environmental Variables:
Device: Flame Master KK
BuildID: 20140923233152
Gaia: ff6dbb006e4e60edba697639aa2a2a199b07069b
Gecko: 1e2993c99323
Version: 35.0a1 (Master) 
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
-----------------------------------------------------------------
Environmental Variables:
Device: Flame 2.1 KK
BuildID: 20140924061258
Gaia: 020e6283a033e8fbcf65e7ed81c5b75ba0095f22
Gecko: d6b762814638
Version: 34.0a2
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
-----------------------------------------------------------------
Environmental Variables:
Device: Open_C Master
BuildID: 20140923233152
Gaia: ff6dbb006e4e60edba697639aa2a2a199b07069b
Gecko: 1e2993c99323
Version: 35.0a1 (Master) 
Firmware Version: P821A10V1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

-----------------------------------------------------------------
-----------------------------------------------------------------

This bug does NOT repro on Flame kk build: Flame 2.0 KK, Flame 2.0 KK Base

Actual Result: No Dark icons seen on the lockscreen.

Repro Rate: 0/10

Environmental Variables:
Device: Flame 2.0 KK
BuildID: 20140923195101
Gaia: 263e3b201dca967ec5346e35901aa981ca47dce7
Gecko: 35d791e16d31
Version: 32.0 (2.0) 
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
-----------------------------------------------------------------
Environmental Variables:
Device: Flame 2.0 KK Base
BuildID: 20140904160718
Gaia: 506da297098326c671523707caae6eaba7e718da
Gecko: 
Version: 32.0 (2.0)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

	Joshua Mitchell (Inactive)
	Comment 13 • 10 years ago

no nomming: lighting / graphic issue only

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Reporter
	Comment 14 • 10 years ago

It's early in 2.1, I think we should consider this.

	Gregor Wagner [:gwagner]
	Comment 16 • 10 years ago

Bad usability regression.

	Kevin Grandon :kgrandon
	Comment 17 • 10 years ago

Comment on attachment 8495196 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24426

Seems like a bit of a band-aid, but I can't really think of a better fix right now that's not risky. Thanks for the patch!

	Alberto Pastor [:albertopq] Assignee
	Comment 18 • 10 years ago

Yeah, I kind of agree, the thing is that I checked all the events we receive when pressing the power button and wake it up again with the card view open, and there are several ones: 'lockscreen-appopened', 'lockscreen-appclosing', 'cardviewclosed', 'stackchanged', 'appopened'... Is not that we have a race between 2 events, but every time we need to setAppearence and the phone is locked, we should dimiss those. That's the reason I fixed in that way. We can revisit this if we find a better way. Thanks for the review!

	Alberto Pastor [:albertopq] Assignee
	Comment 19 • 10 years ago

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

	Alberto Pastor [:albertopq] Assignee
	Comment 20 • 10 years ago

Comment on attachment 8495196 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24426

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Bug 1041625
[User impact] if declined: Statusbar icons are hardly visible in some specific cases (see more on the STR).
[Testing completed]: Added unit tests
[Risk to taking this patch] (and alternatives if risky): Only adding a condition for returning in a method if the phone is locked. I would say the risk is low, and in the very worse case, the consequences of a regression would be the same than the bug itself (bad calculation of the icons brightness).
[String changes made]: -

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 10 years ago

v2.1: https://github.com/mozilla-b2g/gaia/commit/3136db0264b8e619297fbd5e7327a808dcab8b04

	Yeojin Chung [:YeojinC]
	Comment 23 • 10 years ago

I was able to reproduce this issue on today's Flame 2.2 and 2.1 (Nightly):

Flame 2.2 KitKat Base (319mb)(Full Flash)
  
Environmental Variables:
Device: Flame 2.2 Master
BuildID: 20141002093155
Gaia: 191d805f4911628d37a8a90a1e23a6013995138f
Gecko: 5d6ec4dddf14
Gonk: 2c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
  
Flame 2.1 KitKat Base (319mb)(Full Flash)

Environmental Variables:
Device: Flame 2.1
BuildID: 20141002000202
Gaia: 94dcc25f2e34a4900ea58310c26be52bcb089161
Gecko: baaa0c3ab8fd
Gonk: 2c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Dark icons appear on the lockscreen when certain apps are in card view.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Reporter
	Comment 24 • 10 years ago

Yeah, I still see this on master with the STR from comment 10 and the settings app.

	Etienne Segonzac (:etienne)
	Comment 28 • 10 years ago

Comment on attachment 8499640 [details]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/24753

I'm not a big fan of the approach :/
It's adding state (confusing state since it's different from the "is locked" notion already present in the file).

I'd rather have a condition plainly stating that, when we're locked, only `setAppearance` calls for `LockScreenWindow` should pass.

Testing notes:
* I think we should assert that the icons are actually in "light" mode once the settings app is launched
* And that, with the settings app displayed, we're back to light mode after locking/unlocking the phone
* In unit-test, prefer dispatching fake, but real-world, CustomEvent than setting internal state (or better, get rid of the internal state ;))


Also double-checking with Greg tat it's safe to use `apps/system/test/lib/lockscreen.js` for marionette scenarios actually involving the lockscreen.

	Alberto Pastor [:albertopq] Assignee
	Comment 29 • 10 years ago

Thanks Etienne.

I do understand your concerns, but take a look to https://bugzilla.mozilla.org/show_bug.cgi?id=1074991#c18.
The main problem is that the fix you are proposing (that was the first approach I followed) doensn't fix the issue in 2.1. We receive events after the lockscreen has been shown, but System.locked is still false.
Locally tracking the lockscreen was the only idea I had for fixing this without a refactor of the event sequence and Sytem.locked combinations.

Happy to follow other paths if we can find one.

Thanks!

	Etienne Segonzac (:etienne)
	Comment 30 • 10 years ago

Clearing ni since we discussed it on IRC :)

	Alberto Pastor [:albertopq] Assignee
	Comment 31 • 10 years ago

Comment on attachment 8499640 [details]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/24753

Clearing gweng's ni, as I'm not using /lib/lockscreen anymore.

	Etienne Segonzac (:etienne)
	Comment 32 • 10 years ago

Comment on attachment 8499640 [details]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/24753

r=me with the tests comments addressed. let's talk through them if they're unclear.

Happy with the marionette test coverage :)

Also flagging Gareth for the js-marionette/atom part.

	Gareth Aye [:gaye] (back from PTO)
	Comment 33 • 10 years ago

Comment on attachment 8499640 [details]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/24753

Looks good! We also have https://github.com/mozilla-b2g/marionette-helper for miscellaneous mostly gaia-specific helper functions, but totally up to you.

	Michael Henretty [:mikehenrty][:mhenretty]
	Comment 34 • 10 years ago

Looks like all the comments are addressed. I'm going to merge this so it winds up in our afternoon build.

	Alberto Pastor [:albertopq] Assignee
	Comment 35 • 10 years ago

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

	Alberto Pastor [:albertopq] Assignee
	Comment 36 • 10 years ago

Comment on attachment 8499640 [details]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/24753

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): -
[User impact] if declined: The icons are not in the right color when going to the lockscreen
[Testing completed]: Added unit tests and integration tests
[Risk to taking this patch] (and alternatives if risky): This fix is the lowest risk possible for this bug. A change in the sequence of events received by the statusbar could break it, but integration tests are strongly covering it, and we would notice it before it lands.
[String changes made]: -

	Alberto Pastor [:albertopq] Assignee
	Comment 37 • 10 years ago

A patch might be needed for adapting the integration tests to 2.1, so I'll do it myself after approval-2.1+.

	bhavana bajaj [:bajaj]
	Comment 38 • 10 years ago

Comment on attachment 8499640 [details]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/24753

QA, please help virify this once it lands..

	Ryan VanderMeulen [:RyanVM]
	Comment 39 • 10 years ago

(In reply to Alberto Pastor [:albertopq] from comment #37)
> A patch might be needed for adapting the integration tests to 2.1, so I'll
> do it myself after approval-2.1+.

	Alberto Pastor [:albertopq] Assignee
	Comment 40 • 10 years ago

Hi! As we made some changes on the tests that might affect others, I did a pull request just in case:

https://github.com/mozilla-b2g/gaia/pull/25021

I'll merge it as soon as everything goes green. Thanks!

	Alberto Pastor [:albertopq] Assignee
	Comment 41 • 10 years ago

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

	[:RJ]
	Comment 42 • 10 years ago

Issue is verified fixed on Flame 2.2 Master KK (319mb) (Full Flash), Flame 2.0 KK (319mb) (Full Flash)

After putting phone into sleep mode when browser app is in card view and reawakening the phone no longer left in the dark mode and is displaying properly.

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

Device: Flame 2.2 Master KK (319mb) (Full Flash)
BuildID: 20141012040203
Gaia: 717ad4e8b7fc10ab8248500d00ba5ba0977fa8ab
Gecko: 44168a7af20d
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

Flame 2.1 KK (319mb) (Full Flash)

Device: Flame 2.1 KK (319mb) (Full Flash)
BuildID: 20141012001201
Gaia: d18e130216cd3960cd327179364d9f71e42debda
Gecko: 610ee0e6a776
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

	[:RJ]
	Comment 43 • 10 years ago

on previous comment 2.0 was stated in variables but it was meant to say 2.1 sorry if there is any confusion.