1074991 - [Lockscreen] Icons are not the right color after opening a 'dark icons' app

Status: RESOLVED DUPLICATE of bug 1071706

(Firefox OS Graveyard :: Gaia::System::Lockscreen, defect)

Description

[Alberto Pastor [:albertopq]]

STR:

1.- With a lockscreen enabled phone, open Settings
2.- Press the power button (screen goes to sleep)
3.- Press again (screen wakes up)

Exepcted:

Statusbar icons are white

Actual

Statusbar icons are grey

Comment 1

[Alberto Pastor [:albertopq]]

Seems a regression from Bug 1057198.

Greg, any idea?

Comment 2

[Greg Weng [:snowmantw][:gweng][:λ]]

No. Need sometime to pin down the root case. Thanks for reporting, I would check it this later (it's too late in my timezone).

Comment 3

[Michael Henretty [:mikehenrty][:mhenretty]]

[Blocking Requested - why for this release]:

Same as bug 1071706. Broken feature.

Comment 4

[Gregor Wagner [:gwagner]]

If this is caused by bug 1057198 its not a 2.1 issue. Lets file a new bug if this still reproduces on 2.1

Comment 5

[Alberto Pastor [:albertopq]]

I cannot repro in 2.1. Adding qawanted to make sure which versions are affected

Comment 7

[Joshua Mitchell (Inactive)]

Is this a dupe of bug 1071706?

Comment 8

[Cody Roesch [:croesch]]

Tested with Shallow Flash

This bug repro's on Flame KK builds: Flame 2.2 KK

Actual Results: Icons in the Status bar are Grey when locking the screen when SETTINGS is open.

Repro Rate: 2/2

Environmental Variables:
Device: Flame Master KK
BuildID: 20141001060621
Gaia: a23d2c490b39c4699c9375e25c4acdf396a2fa85
Gecko: 835ef55e175e
Version: 35.0a1 (Master) 
Firmware Version: L1TC10011800
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.1 KK

Actual Result: Icons in the Status bar remain white after visiting SETTINGS then locking the device.

Repro Rate: 0/5

Environmental Variables:
Device: Flame 2.1 KK
BuildID: 20141001060122
Gaia: b327c640fea887770d011a127e349838b3b44724
Gecko: 7359d0d0222d
Version: 34.0a2
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
-----------------------------------------------------------------
Environmental Variables:
Device: Flame 2.0
BuildID: 20141001060124
Gaia: 8079cba2133e6f5443dba24dad077f7f91e6c978
Gecko: 66c1ea78b6c1
Version: 32.0 (2.0) 
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 9

[Joshua Mitchell (Inactive)]

I'm nomming this based on a very similar issue (bug 1071706) being nommed as well.

Comment 10

[Greg Weng [:snowmantw][:gweng][:λ]]

OK, I'm now digging this bug. It's the regression of Bug 1057198. However, it's not a 2.1 patch, so we may have to different bugs with two different root causes.

Comment 11

[Greg Weng [:snowmantw][:gweng][:λ]]

The possible root cause is the patch for Bug 1071706. This patch make setAppearance work incorrectly if System.locked, because:

If the previous appearance is 'light' because of the Settings or some other apps opened, it would NOT change to 'maximized' when we switch from 'Settings' to 'LockScreen' (because now the 'isLocked' is true, and we would leave the function without do the class toggling), thus the color is NOT correct (still keep as 'light', rather what we expected, the 'maximized').

The correct solution should be: The class would change from 'light' to 'maximized' when System is locked && we want to setAppearance while it is locking. From the debugging log I notice that the 'setAppearance' would be invoked while we press the power button, so I think to remove the conditional statement is OK. However, I don't know why we wrote a patch like this and why it looks working well, until this regression happen.

Comment 12

[Greg Weng [:snowmantw][:gweng][:λ]]

I try to remove the conditional statements and it resolves the issue now. And since I don't know what side-effects may happen although I manually tested several apps, I now NI the author, and maybe the reviewer.

Comment 13

[Greg Weng [:snowmantw][:gweng][:λ]]

I list the broken flow here to clarify the patch (according to my debugging result):

1. LockScreen - statusbar now is 'maximized' - isLocked is 'true'
2. Unlock, to Homescreen - statusbar now is 'maximized' - isLocked is 'false'
3. Launch Settings app - statusbar now is 'light' - isLocked is 'false'
4. Press the power button - statusbar now is 'light' - isLocked is 'true'

Now, the setAppearance get invoked. But since the 'isLocked' is 'true', it would not toggle the class of statusbar. So:

5. Screen get blacked - statusbar now is 'light' - isLocked is 'true'
6. Press the power button again, and now LockScreen shows. Thus, the final result:
7. LockScreen shows - statusbar now is 'light' - isLocked is 'true'

You can see at the last step the regression happens: if you manually change the status bar from 'light' to 'maximized' at this moment, the regression disappears. It shows we need toggle the class when we LockScreen the screen, which now is blocked via the conditional statement.

Comment 14

[Greg Weng [:snowmantw][:gweng][:λ]]

Sorry, typo: clarify the patch -> clarify the path.

Comment 15

[Greg Weng [:snowmantw][:gweng][:λ]]

I test to checkout master to Bug 1071706, and revert. Thus I can watch what problem the bug want to resolved. And I've found that:

- Card View STR (broken case) -
Settings app - 'light'
Card view - 'light'
Power button pressed - 'light'
LockScreen - 'light'

So status bar keep as 'light' from Settings app to LockScreen, which makes it broken.

- NO card view (no dark icon, expected case)-
Settings app - 'light'
Power button pressed - NO CLASS
LockScreen - NO CLASS, but it looks good as we want

So although the class on status bar is not 'maximized', but without a class it looks as good as we expected.

Comment 16

[Greg Weng [:snowmantw][:gweng][:λ]]

And this is the result with the patch of Bug 1071706:

- Card view STR -
Settings app - 'light'
Card view - 'light'
Power button pressed - ''
LockScreen - ''

- NO card view -
Settings app - 'light'
Power button pressed - NO CLASS
LockScreen - NO CLASS

So the major difference is the class would be removed while we pressed the power button.

Comment 17

[Greg Weng [:snowmantw][:gweng][:λ]]

Result:

1. With Bug 1071706 it fix the dark icon issue
2. Bug 1057198 make it broken again
3. I still have no clue why Bug 1057198 make it broken again, since it doesn't touch statusbar according to the code. But I guess, that Bug 1057198 change some timing, and make the bug appears again (or stabilized it).
4. From the path recorded in Comment 13, the patch of Bug 1057198 looks lead to the broken case (unless the path is actually not correct), and this is confirmed after I removed the conditional statement manually.
5. I don't know what side-effects would happen if I remove the statement directly. But this is the solution now I have.
6. According to Comment 15, I think for Bug 1071706 + Bug 1057198, the proper solution is not to block setAppearance while it get locked, but to check why from the button pressed state to the LockScreen locked state, there is no class change from 'light' to 'maximized' or NO CLASS.

Comment 18

[Alberto Pastor [:albertopq]]

The main problem is that the moment in which System.locked becomes 'true', is different after that commit (is already locked when we receive the lockscreen-appopenned event, when it wasn't before). Moreover, is even different in 2.1. In 2.1, when receiving the lockscreen-appopenned event, System.locked is false, and remains false until the screenchange event (in master, before Bug 1057198, System.locked was false on the lockscreen-appopened event, but true in all the following events).

We cannot remove that condition, because there are several events received when the phone is locked, and we don't want to update the icons on those events.

Is there any way of reliably listening when the phone is locked? We could, at that point, set the icons appearance to the lockscreen and not allow any other update until the system is unlocked again.

Comment 19

[Greg Weng [:snowmantw][:gweng][:λ]]

One possible reasonable approach is we setAppearance for LockScreen every time it get locked. After all, we set an different appearance for Homescreen and treat it as a special appearance that hardcoded in the setAppearance (set 'maximized'), there is no reason (or I didn't realize it) LockScreen should not follow the similar path. And, LockScreen now is one of the specialized AppWindow (use the same base class), which means when we should consider it as an app (which would be true in the near future), with it's own statusbar just like other apps.

And about the locking and other events' racing issue: I think to rely on the timing the event and the attribute changed is not so robust solution. Of course there may be some designation flaw let the locking timing different. I would try to take a look of the relation of statusbar's event handling and the locking state, to see if there is something we need to guarantee, and maybe add some thing we adopted in other components like queue or event a lightweight state machine (when you need to do lots of conditional switching and rely on different states/events/timing, it becomes the reasonable solution). But if this is a v2.1 blocker I don't think this could be in schedule. So I would try to find another short term solution if the first one (set appearance for LockScreen, as above) is not reasonable.

The different locking timing is because that now the state is totally delegate to AppWindow#isActive, which may differs from the previously manually-managed one. From LockScreen side this make sense because we are an AppWindow now, so we should follow what states that AppWindow would set, rather keep another parallel but duplicated states. So the change IMO it's reasonable.

Comment 20

[Alberto Pastor [:albertopq]]

Thanks a lot for your help, Greg!

Being resolved here => Bug 1071706

Comment 21

[Joshua Mitchell (Inactive)]

bug is resolved: dupe  - removing keywords