Closed
Bug 1071706
Opened 10 years ago
Closed 10 years ago
Sometimes the tray is left in "dark" mode on the lockscreen
Categories
(Firefox OS Graveyard :: Gaia::System::Lockscreen, defect)
Tracking
(blocking-b2g:2.1+, b2g-v2.0 unaffected, b2g-v2.1 verified, b2g-v2.2 verified)
Tracking | Status | |
---|---|---|
b2g-v2.0 | --- | unaffected |
b2g-v2.1 | --- | verified |
b2g-v2.2 | --- | verified |
People
(Reporter: khuey, Assigned: apastor)
References
Details
(Keywords: regression, Whiteboard: [systemsfe])
Attachments
(3 files, 1 obsolete file)
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.
Reporter | ||
Comment 1•10 years ago
|
||
Comment 3•10 years ago
|
||
I haven't found a clear STR yet, but I'll keep an eye out for it.
Flags: needinfo?(kgrandon)
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.
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.
Reporter | ||
Comment 6•10 years ago
|
||
Nope, just had it happen right now.
Comment 7•10 years ago
|
||
Alberto, you worked on bug 1055746, any idea what is going on here?
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.
Assignee | ||
Comment 9•10 years ago
|
||
Yeah, I think Kevin is right. I'll take a look
Flags: needinfo?(apastor)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → apastor
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.
Keywords: qawanted
Reporter | ||
Comment 11•10 years ago
|
||
That reproduces it for me too.
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
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.1:
--- → affected
status-b2g-v2.2:
--- → affected
Flags: needinfo?(jmitchell)
Keywords: qawanted → regression
QA Contact: croesch
Comment 13•10 years ago
|
||
no nomming: lighting / graphic issue only
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
Reporter | ||
Comment 14•10 years ago
|
||
It's early in 2.1, I think we should consider this.
blocking-b2g: --- → 2.1?
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8495196 -
Flags: review?(kgrandon)
Comment 17•10 years ago
|
||
Comment on attachment 8495196 [details] [review]
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!
Attachment #8495196 -
Flags: review?(kgrandon) → review+
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!
Assignee | ||
Comment 19•10 years ago
|
||
Assignee | ||
Comment 20•10 years ago
|
||
Comment on attachment 8495196 [details] [review]
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]: -
Attachment #8495196 -
Flags: approval-gaia-v2.1?
Updated•10 years ago
|
Attachment #8495196 -
Flags: approval-gaia-v2.1? → approval-gaia-v2.1+
Comment 21•10 years ago
|
||
Target Milestone: --- → 2.1 S5 (26sep)
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.
Reporter | ||
Comment 24•10 years ago
|
||
Yeah, I still see this on master with the STR from comment 10 and the settings app.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8495196 -
Attachment is obsolete: true
Attachment #8499640 -
Flags: review?(mhenretty)
Updated•10 years ago
|
Target Milestone: 2.1 S5 (26sep) → 2.1 S6 (10oct)
Assignee | ||
Updated•10 years ago
|
Attachment #8499640 -
Flags: review?(mhenretty) → review?(etienne)
Comment 28•10 years ago
|
||
Comment on attachment 8499640 [details] [review]
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.
Attachment #8499640 -
Flags: review?(etienne)
Flags: needinfo?(gweng)
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!
Flags: needinfo?(etienne)
Assignee | ||
Comment 31•10 years ago
|
||
Comment on attachment 8499640 [details] [review]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/24753
Clearing gweng's ni, as I'm not using /lib/lockscreen anymore.
Attachment #8499640 -
Flags: review?(etienne)
Flags: needinfo?(gweng)
Comment 32•10 years ago
|
||
Comment on attachment 8499640 [details] [review]
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.
Attachment #8499640 -
Flags: review?(gaye)
Attachment #8499640 -
Flags: review?(etienne)
Attachment #8499640 -
Flags: review+
Comment 33•10 years ago
|
||
Comment on attachment 8499640 [details] [review]
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.
Attachment #8499640 -
Flags: review?(gaye) → review+
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.
Assignee | ||
Comment 35•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 36•10 years ago
|
||
Comment on attachment 8499640 [details] [review]
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]: -
Attachment #8499640 -
Flags: approval-gaia-v2.1?(fabrice)
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+.
Updated•10 years ago
|
Attachment #8495196 -
Flags: approval-gaia-v2.1+
Comment 38•10 years ago
|
||
Comment on attachment 8499640 [details] [review]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/24753
QA, please help virify this once it lands..
Attachment #8499640 -
Flags: approval-gaia-v2.1?(fabrice) → approval-gaia-v2.1+
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+.
Flags: needinfo?(apastor)
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!
Flags: needinfo?(apastor)
Assignee | ||
Comment 41•10 years ago
|
||
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
Status: RESOLVED → VERIFIED
Flags: needinfo?(ktucker)
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.
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Keywords: verifyme
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Assignee | ||
Updated•10 years ago
|
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•