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)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.1+, b2g-v2.0 unaffected, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S6 (10oct)
blocking-b2g 2.1+
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)

Attached image Bad
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.
I will look into this or find someone to do so.
Flags: needinfo?(kgrandon)
I haven't found a clear STR yet, but I'll keep an eye out for it.
Flags: needinfo?(kgrandon)
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.
Not sure. I definitely have picked up bug 1055746 by now though, so I'll let you know if I see this again.
Nope, just had it happen right now.
Alberto, you worked on bug 1055746, any idea what is going on here?
Blocks: 1041625
Flags: needinfo?(apastor)
Whiteboard: [systemsfe]
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.
Yeah, I think Kevin is right. I'll take a look
Flags: needinfo?(apastor)
Assignee: nobody → apastor
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
That reproduces it for me too.
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?]
Flags: needinfo?(jmitchell)
Keywords: qawantedregression
QA Contact: croesch
no nomming: lighting / graphic issue only
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
It's early in 2.1, I think we should consider this.
blocking-b2g: --- → 2.1?
Bad usability regression.
blocking-b2g: 2.1? → 2.1+
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+
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!
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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?
Attachment #8495196 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1+
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.
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: verifyme
Yeah, I still see this on master with the STR from comment 10 and the settings app.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Attachment #8495196 - Attachment is obsolete: true
Attachment #8499640 - Flags: review?(mhenretty)
Target Milestone: 2.1 S5 (26sep) → 2.1 S6 (10oct)
Attachment #8499640 - Flags: review?(mhenretty) → review?(etienne)
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)
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)
Clearing ni since we discussed it on IRC :)
Flags: needinfo?(etienne)
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 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 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+
Looks like all the comments are addressed. I'm going to merge this so it winds up in our afternoon build.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
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)
A patch might be needed for adapting the integration tests to 2.1, so I'll do it myself after approval-2.1+.
Attachment #8495196 - Flags: approval-gaia-v2.1+
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+
(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)
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)
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)
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
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: