834542 - Time isn't displayed in status bar anymore, when the phone is locked but not at the "at-rest" lock-screen. (e.g. in passcode-entry & emergency dialer)

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

STR:
 0. Have a passcode lock on your phone.
 1. Lock phone.
 2. Now, from lock screen, tap "unlock" button.
    ---> passcode-entry screen
 3. Tap emergency dialer.
   (now, pretend you're now making an emergency call)

ACTUAL RESULTS:
 * When you lock your phone, the time disappears from status bar.  (why?)
 * Not a huge deal when you're looking at the "at rest" lock screen, but after step 2 and after step 3 (e.g. when you're on the phone w/ emergency services), the current time isn't displayed anywhere on the phone.

EXPECTED RESULTS: Time should just stay displayed in status bar.


I'm pretty sure the time used to be displayed in the status bar all the time, but I noticed it was missing on the lock screen in the latest dogfood build.

Build ID: 20130121070201

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screenshot: passcode entry screen (w/ no clock visible in title bar or anywhere else)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: screenshot: emergency dialer screen (w/ no clock visible in title bar or anywhere else)

Comment 3

[Dietrich Ayala (:dietrich)]

not having clock on emergency screen is a bit concerning.

Comment 4

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Josh, you reported Bug 811240 whose patch causes this behavior, should the clock be displayed in the status bar on the "Enter security code" and emergency dialer screens?

Comment 5

[Daniel Holbert [:dholbert]]

I'd think it should -- the justification for the clock removal was that it was redundant, on the "at rest" lock screen, per bug 811240 comment 0.

I really think we should just back out Bug 811240. Its value is questionable, and unless we can somehow make it more targeted, it comes at a cost (hiding information that we previously displayed, in situations where it could be crucial, e.g. in emergency dialer).

Comment 6

[Josh Carpenter [:jcarpenter]]

Seems reasonable that we'd want to include the clock in emergency calls. Here's our thinking:

Strong preference:
- Leave clock off default Lock screen
- Leave clock off "Enter security code" screen
- Add clock to Emergency Call screens

Backup plan:
- If we cannot differentiate, let's look at backing out 811240. I'd also like to get TEF's input here, for certification reasons. I imagine there are regulations here.

Adding Beatriz on needsinfo.

Comment 7

[Alex Keybl [:akeybl]]

Mihai - please renominate with blocker reasoning. Note that you can ask for v1.1 approval for the time being without this bug blocking.

Comment 8

[Beatriz Rodríguez [:brg]]

Thanks for asking. There is no special requirement in this topic. We are not in hurry for this improvement in v1.0.1.

Comment 9

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

(In reply to Alex Keybl [:akeybl] from comment #7)
> Mihai - please renominate with blocker reasoning. Note that you can ask for
> v1.1 approval for the time being without this bug blocking.

Hi Alex, thanks for the comment, my reasoning for requesting blocking-b2g:leo was based on the details of comment 5 and comment 6: hiding information from the user in crucial situations is not desired -- when making an emergency call from the lock screen, knowing the time might be quite important.

If you think this is a good reason for blocking feel free to set the flag.

Comment 10

[Josh Carpenter [:jcarpenter]]

I don't think this blocks, given comment 8.

Comment 11

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Attachment: Pull Request #9218 - Display clock for the Emergency Call screen

Extended the StatusBar API with two new methods that enable hiding specific elements from the statusbar, and used the new methods to hide the clock in the statusbar for the 'main' and 'passcode' screens of the lockscreen and display it for the 'emergency-call' screen.

Removed the changes added by patch for Bug 811240 since the behavior is now treated in the LockScreen.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 738142 [details]
Pull Request #9218 - Display clock for the Emergency Call screen

Could you expose panel info from the 'lockpanelchange' event and have the StatusBar listens to it (and toggle the clock based on the info?)

I think that is the better approach, instead of exposing new APIs.

Comment 13

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #12)
> Comment on attachment 738142 [details]
> Pull Request #9218 - Display clock for the Emergency Call screen
> 
> Could you expose panel info from the 'lockpanelchange' event and have the
> StatusBar listens to it (and toggle the clock based on the info?)
> 
> I think that is the better approach, instead of exposing new APIs.

Thanks for the feedback, Tim! I updated the patch to add event listeners in StatusBar, however, I had to include both 'unlock' and 'lock' event listeners in order to correctly display the clock, as detailed in comment 6.

Have a look at the updated patch (PR #9218: attachment 738142 [details]) and let me know what you think.

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 738142 [details]
Pull Request #9218 - Display clock for the Emergency Call screen

Please address my comment on Github, you are free to land the fix after all comments is addressed, or if you are unsure, re-request for a review.

One thing I am fear of, after reading your code, would be some kind of loading sequence problem: what if the first 'lock' or 'unlock' event is already propagated *before* statusbar init()? Let's not too concern about that, as long as the system app do the expected thing (both when lock screen is enabled or disabled), we could land your fix first.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://github.com/mozilla-b2g/gaia/commit/564b1b13d0b1d19753450d5355c9124dd07b2e02

Comment 16

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Comment on attachment 738142 [details]
Pull Request #9218 - Display clock for the Emergency Call screen

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Lockscreen > StatusBar time
User impact if declined: Low (no time is displayed for emergency call when accessed from passcode unlock screen)
Testing completed: Yes
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: No

Comment 17

[Alex Keybl [:akeybl]]

(In reply to Mihai Cirlanaru [:mihai][:mcirlanaru] from comment #16)
> Risk to taking this patch (and alternatives if risky): Low

Can you address comment 14? We want to be sure this is very low risk since it's not a blocker.

Comment 18

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Hi Tim, to address your question from comment 14 for uplift risk assessment:

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #14)
> One thing I am fear of, after reading your code, would be some kind of
> loading sequence problem: what if the first 'lock' or 'unlock' event is
> already propagated *before* statusbar init()?

From what I remember (and double-checked now: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/index.html#L102) the statusbar is initialized before the lockscreen in the system app, so lockscreen events ('lock', 'unlock') will not propagate before statusbar.init().

Can you confirm that this will be a low risk uplift to Alex? Thanks!

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Mihai Cirlanaru [:mihai][:mcirlanaru] from comment #18)
> From what I remember (and double-checked now:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/index.html#L102)
> the statusbar is initialized before the lockscreen in the system app, so
> lockscreen events ('lock', 'unlock') will not propagate before
> statusbar.init().
> 
> Can you confirm that this will be a low risk uplift to Alex? Thanks!

Sigh. <script> order does not guarantee that. The Gaia init code path is pretty messed up and the timing of lockscreen events would actually depend on mozSettings API.

Statusbar.init() depend on l10n localized event.

That said, given the fact this work on master w/o a problem, we could safely assume there won't be difference on v1-train.

We really need to address this in the long run...

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 738142 [details]
Pull Request #9218 - Display clock for the Emergency Call screen

I don't have the permission on Bugzilla to set approval‑gaia‑v1+, but you get what I meant on the previous comment.

Comment 21

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #19)
> Sigh. <script> order does not guarantee that. The Gaia init code path is
> pretty messed up and the timing of lockscreen events would actually depend
> on mozSettings API.
> 

Ohhh, I see, I thought the order is sequential.

> Statusbar.init() depend on l10n localized event.
> 
> That said, given the fact this work on master w/o a problem, we could safely
> assume there won't be difference on v1-train.
> 
> We really need to address this in the long run...

Alex, since this patch worked well on master, as Tim describes, do you think it is a good candidate for uplifting?

Comment 22

[Alex Keybl [:akeybl]]

Comment on attachment 738142 [details]
Pull Request #9218 - Display clock for the Emergency Call screen

This will make our approval (as opposed to blocker-only) deadline. Approved.

Comment 23

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 564b1b13d0b1d19753450d5355c9124dd07b2e02
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 24

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Fixed the merge conflicts in a new pull request for v1-train: https://github.com/mozilla-b2g/gaia/pull/10283

Tim, can you check it and, if everything looks fine, merge it to v1-train? Thanks!

Comment 25

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1-train: https://github.com/mozilla-b2g/gaia/commit/52ec28e7e47adbca974324d37b15884ffe842be9