1172621 - Switch lockscreen from l10n_date to Intl

Status: RESOLVED FIXED

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

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: pull request

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Hi Gabriele. In bug 1060333 you changed the behavior of the lockscreen to not display the am/pm portion in 12h scenario.

In this bug I'm migrating the lockscreen to use Intl API which has hour12 setting. The current patch takes hour12 from navigator.mozHour12 and displays it.
Do we really not want to have am/pm on the lockscreen? I thought it was supposed to be there, just smaller.

If that's the case, I can just set hour12: false and remove the eventlistener on mozhour12change.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ah, no, I cannot regardless of if we want the am/pm portion. I still need to react to mozHour12 since it controls if time is shown in 12/24 format and I need to remove the am/pm portion.

I updated the patch to incorporate callscreen. I didn't update tests yet.

One thing I'd like to consider is actually not removing am/pm from the lockscreen's clock. I think it's useful for users who use 12h clock format and I'm not sure what are we achieving by removing it. But that may be a question to someone from UX.

Comment 4

[Gabriele Svelto [:gsvelto] (PTO)]

I've been wondering why the original code was doing that; since I wasn't involved in the callscreen/lockscreen at the time I assumed it must have been an UX decision. It turns out that it wasn't as the UI mock in bug 887802 shows the 'PM' string besides the clock (see  attachment 803600 [details] and attachment 803629 [details]). The string does look smaller than the rest of the clock so maybe it was hidden as a hack because they hadn't made it the right size yet.

I'd be in favor of showing it again but you'll need UX feedback about it and probably you'll also need to put some code in place to make it use a smaller font.

NI'ing Steve Chung and Greg Weng too since they worked on this code back in the day and might know more about it.

Comment 5

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

We don't show PM/AM on LockScreen according to UX' opinon and spec of 2.1 (or 2.2). However, you know how chaotic our spec management is, so if you want the new spec we need to dig in it to find that. The spec you found is an very old one and it's for 1.2. So, if Zibi only concerns about whether to show or not to show the PM/AM, the answer is we don't show that now.

Comment 6

[Steve Chung [:steveck]]

Sorry that it's kind of out of ballpark to me that I only knew very little about the old pre 2.0 callscreen and the AM/PM was still there at that time. Maybe you can ask Rob(:robmac) if you have any thought about the lockscreen design.

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

Thanks for your feedback guys, so short answer is: we don't want to show the AM/PM part for now. We can always file a bug to reconsider that after this one lands.

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8616901 [details] [review]
pull request

This patch keeps the "no am/pm" logic, while removing LazyL10n, and l10n_date.js uses migrating them to Intl for the Lockscreen clock/date.

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

This is the second patch after the statusbar time icon.

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8616901 [details] [review]
pull request

The callscreen bits LGTM but you'll need :gweng's review for the lockscreen ones.

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8616901 [details] [review]
pull request

Oh, I thought :drs is for lockscreen :) Reassigning r? to gweng.

Comment 12

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8616901 [details] [review]
pull request

Greg, could you take a look at the Lockscreen parts of this?

Comment 13

[Doug Sherk (:drs) (inactive)]

Whoops, mid-air collision there. I'm a Dialer peer, so Gabriele's review was enough.

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Doug, Gabriele, also. I don't know how often the callscreen's "showClock" function is called, but if it's called often we can move the Intl.DateTimeFormat out of that function the same way as we do this in system's lockscreen and then set an eventlistener on languageschange/mozhourchange to regenerate it.

It only makes sense if showClock is performance sensitive. Let me know if you think it makes sense.

Comment 15

[Gabriele Svelto [:gsvelto] (PTO)]

It's probably one of the least performance-intensive function in the whole of gaia, we call it only once when showing the callscreen's lockscreen mock so don't worry :-)

Comment 16

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

Comment on attachment 8616901 [details] [review]
pull request

Thanks for patching. I have some concerns and commented them on the PR page.

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8616901 [details] [review]
pull request

Gerg, I responded to your concerns and also fixed the last Gij test so it should be green now.

Comment 18

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

Comment on attachment 8616901 [details] [review]
pull request

Okay, with Zibi's explanation on the PR page, it looks good to me. Thanks.

Comment 19

[Duane Dixon [:ddixon]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #8)
> Comment on attachment 8616901 [details] [review]
> pull request
> 
> This patch keeps the "no am/pm" logic, while removing LazyL10n, and
> l10n_date.js uses migrating them to Intl for the Lockscreen clock/date.

Hi Zibi, 

I have tested your patch: "1172621-move-lockscreen-to-intl" with the latest Flame engineering build. 

Environmental Variables:
Device: Flame 2.5 (Shallow Flash)
Build ID: 20150724130844
Gaia: 199bf066a34a8ad5f4b84e1cf3f6083a2f44c9f5
Gecko: a1829935d87e
Version: 42.0a1 (2.5)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42.0

Actual Results: 
If the user manually changes the Region to anything other than "America", the device will display AM or PM at the lockscreen.

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Duane Dixon [:ddixon] from comment #19)
> Actual Results: 
> If the user manually changes the Region to anything other than "America",
> the device will display AM or PM at the lockscreen.

That's unfortunately once again bug 1172609.

What happens is:

We don't have a clean way to display 12-hour clock without am/pm portion using Intl (we also didn't have a clean way to do this before. We'll try to add it to Intl API v2).

So what we do is we generate the am/pm portion for a given locale and then use string.replace to remove it from the time string.

Now, since we have the timezone bug, it may happen that we generate the amPm portion for the *old timezone* (for example "AM") and then try to replace it in the time string with the *new timezone* (for example "7:43 PM"). The result is that the removal piece doesn't work.

It will get fixed when bug 1172609 lands.

Comment 21

[Zibi Braniecki [:zbraniecki][:gandalf]]

(also, restarting B2G fixes it of course)

Comment 22

[Zibi Braniecki [:zbraniecki][:gandalf]]

Commit: https://github.com/mozilla-b2g/gaia/commit/899e795e94aa71964443e08d6861964a5d2b75e3