Sometimes the tray is left in "dark" mode on the lockscreen

VERIFIED FIXED in Firefox OS v2.1

Status

VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: khuey, Assigned: apastor)

Tracking

({regression})

unspecified
2.1 S6 (10oct)
ARM
Gonk (Firefox OS)
regression
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

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

Details

(Whiteboard: [systemsfe])

Attachments

(3 attachments, 1 obsolete attachment)

Created attachment 8493865 [details]
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.
(Assignee)

Comment 9

4 years ago
Yeah, I think Kevin is right. I'll take a look
Flags: needinfo?(apastor)
(Assignee)

Updated

4 years ago
Assignee: nobody → apastor
(Assignee)

Comment 10

4 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
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?]
status-b2g-v2.0: --- → unaffected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → affected
Flags: needinfo?(jmitchell)
Keywords: qawanted → regression
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?
(Assignee)

Comment 15

4 years ago
Created attachment 8495196 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24426
Attachment #8495196 - Flags: review?(kgrandon)
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+
(Assignee)

Comment 18

4 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

4 years ago
master: https://github.com/mozilla-b2g/gaia/commit/5df75bd3d85f4a7bb512cb0c602c646bacc14d57
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-b2g-v2.2: affected → fixed
Resolution: --- → FIXED
(Assignee)

Comment 20

4 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?
Attachment #8495196 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1+
v2.1: https://github.com/mozilla-b2g/gaia/commit/3136db0264b8e619297fbd5e7327a808dcab8b04
status-b2g-v2.1: affected → fixed
Target Milestone: --- → 2.1 S5 (26sep)
Duplicate of this bug: 1075248
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)
(Assignee)

Comment 25

4 years ago
Created attachment 8499640 [details] [review]
Link to Pull Request: https://github.com/mozilla-b2g/gaia/pull/24753
Attachment #8495196 - Attachment is obsolete: true
Attachment #8499640 - Flags: review?(mhenretty)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1075248
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1074991
Target Milestone: 2.1 S5 (26sep) → 2.1 S6 (10oct)
(Assignee)

Updated

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

Comment 29

4 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)
Clearing ni since we discussed it on IRC :)
Flags: needinfo?(etienne)
(Assignee)

Comment 31

4 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 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.
(Assignee)

Updated

4 years ago
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
status-b2g-v2.1: fixed → affected
(Assignee)

Comment 36

4 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

4 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+.
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)
(Assignee)

Comment 40

4 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)

Comment 42

4 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
status-b2g-v2.1: fixed → verified
status-b2g-v2.2: fixed → verified
Flags: needinfo?(ktucker)

Comment 43

4 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
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
(Assignee)

Updated

4 years ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.