978547 - [Clock] JavaScript functions should not rely on presence and position of localizable elements (AM, PM, :)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Clock, defect)

Description

[Francesco Lodolo [:flod]]

Code in the Clock apps makes a lot of assumptions on hard-coded strings and item order.

Bug 932270 made AM|PM localizable, which breaks basically everything in this app.

Utils.is12hFormat is fine, since it uses %p from dateTimeFormat_%X to determine if format is 12 or 24 hours.
https://github.com/mozilla-b2g/gaia/blob/master/apps/clock/js/utils.js#L221

But then Utils.parseTime breaks everything
https://github.com/mozilla-b2g/gaia/blob/master/apps/clock/js/utils.js#L250

First using ":" to split hours and minutes, then assuming %p is after %I:%M, finally using the position of "M" to determine if it's 12 or 24 hours.

Another problem is in Utils.format
https://github.com/mozilla-b2g/gaia/blob/master/apps/clock/js/utils.js#L379

Comment 1

[Francesco Lodolo [:flod]]

Note that similarly to bug 932356, this app should be able to display AM/PM before hours and minutes, according to dateTimeFormat_%X

Comment 2

[Francesco Lodolo [:flod]]

For reference.

AM/PM
http://www.unicode.org/cldr/charts/latest/by_type/date_&_time.fields.html#6b2fa3ccf14e33a8

Separator
http://www.unicode.org/cldr/charts/latest/by_type/date_&_time.gregorian.html#7637c409f3271b97

Instead of ":", we should probably read the character between %H/%I and %M, and fallback to ":" in the worst case (broken date format).

We should probably use shortTimeFormat instead of dateTimeFormat_%X, since we're talking about time.

Comment 3

[Fabien Cazenave [:kaze]]

Thanks for this sharp bug report: parsing the time formats is fine, parsing the formatted time strings is not.

(In reply to Francesco Lodolo [:flod] from comment #0)
> Bug 932270 made AM|PM localizable, which breaks basically everything in this
> app.

Just a word in case this comes to triage: bug 932270 will break this app if localizers choose to use a non-[AM|PM] string for their locale. So this is a corner case but yes, this should be fixed properly — e.g. to properly support Chinese.

> But then Utils.parseTime breaks everything
> https://github.com/mozilla-b2g/gaia/blob/master/apps/clock/js/utils.js#L250

True. IIRC we used to have the same issue with the homescreen clock for Brazilian (the pt-BR locale used 'h' as a separator instead of ':'). I thought this issue had been solved, but maybe the “fix” has been to use a colon separator for Brazilian…

Comment 4

[Francesco Lodolo [:flod]]

(In reply to Fabien Cazenave [:kaze] from comment #3)
> True. IIRC we used to have the same issue with the homescreen clock for
> Brazilian (the pt-BR locale used 'h' as a separator instead of ':'). I
> thought this issue had been solved, but maybe the “fix” has been to use a
> colon separator for Brazilian…

I didn't see this one (wasn't really following Gaia bugs at the time): bug 824706.

Portuguese definitely has peculiar time formats, which makes me think that Clock could be already failing there
https://hg.mozilla.org/gaia-l10n/pt-BR/file/default/shared/date/date.properties#l196

For reference, zh-TW
https://hg.mozilla.org/gaia-l10n/zh-TW/file/f0b97f49c661/shared/date/date.properties#l197

zh-CN is different, I wonder if that's just an error in the localization (i.e. not sure how much testing they're doing)
https://hg.mozilla.org/gaia-l10n/zh-CN/file/183a412f6d06/shared/date/date.properties#l197

Comment 5

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #4)
> Portuguese definitely has peculiar time formats, which makes me think that
> Clock could be already failing there

It currently works for pt-BR simply because it's ignoring the localized format and using %H:%M or %I:%M
https://github.com/mozilla-b2g/gaia/blob/master/apps/clock/js/utils.js#L229

So it will currently break only if AM/PM is changed to something else.

So these are the current problems:
* Utils.getLocaleTime should use the real localized format to display time
* Utils.parseTime should not use ":" or "AM"/"PM" to split values

Comment 6

[Marcus Cavanaugh [:mcav]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18342

This patch gets rid of all of the AM-PM-specific logic, deferring to l10n's `shortTimeFormat` in all cases. To address the needs of HTML display showing the `%p` in a smaller format, it modifies the formatString to directly add the HTML tag before formatting.

Comment 7

[Francesco Lodolo [:flod]]

Comment on attachment 8407129 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18342

Sorry but I'm very far from being a reviewer for code :-)

From a quick test, it doesn't seem to work correctly for zh-TW, where the period should be displayed before the time.

Comment 8

[Francesco Lodolo [:flod]]

Comment on attachment 8407129 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18342

After looking at the code I couldn't understand why it was failing, in fact it works fine. Not sure why my first test failed, probably the patch didn't apply at all.

Comment 9

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8407129 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18342

Ah, glad you figured it out, I was going to be stumped :-)

Comment 10

[Miller Medeiros [:millermedeiros]]

Comment on attachment 8407129 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18342

few nits on GH, but overall the patch looks good to me. reduced complexity and also removed some unused code in the process, which is always nice.

Comment 11

[Marcus Cavanaugh [:mcav]]

master: https://github.com/mozilla-b2g/gaia/commit/ad9a61c0043f344a6f1ccbb05588caf0cd478228