Closed Bug 932356 Opened 11 years ago Closed 10 years ago

[B2G][Time Picker] Reel order should be localizable

Categories

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

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:backlog, b2g-v1.4 fixed)

RESOLVED FIXED
1.4 S3 (14mar)
tracking-b2g backlog
Tracking Status
b2g-v1.4 --- fixed

People

(Reporter: petercpg, Assigned: flod)

References

Details

(Keywords: l12y, Whiteboard: LocRun1.3, LocRun1.2)

Attachments

(5 files)

In some languages, the term of AM/PM are put before the time (for example in Chinese, we use "上午 12:34") while in English it is put after "12:34 AM". Putting such unlocalized time string in a localized context might look weird in my opinion.

We should have l12y for the reel orders in the time picker.


Device: Geeksphone Keon
Build: 20131025225454
Gaia: 2ad615234dfde72b69bd4ac397e6dd898076bcab
Platform Version: 26.0a2
Still affects 1.3.


Device: Geeksphone Keon
Build: 20140205010019
Gaia: 3405205
Platform Version: 28.0
Blocks: 968062
Whiteboard: LocRun1.2 → LocRun1.3
(In reply to Francesco Lodolo [:flod] from comment #2)
> That should be shortTimeFormat
> http://hg.mozilla.org/releases/gaia-l10n/v1_3/zh-TW/file/07ce0fb4134c/shared/
> date/date.properties#l201
> 
> Isn't it working for you?

The format works, but I don't think time picker follows format (including reel order). (%p should return AM or PM localized)
I'm surprised that iOS made this correctly, let me upload screenshot.
Not sure this should be considered a blocker. Nominating in case
blocking-b2g: --- → 1.3T?
(In reply to delphine from comment #7)
> Not sure this should be considered a blocker. Nominating in case

I think you meant to set 1.3? here - 1.3T? is used for tarako
blocking-b2g: 1.3T? → 1.3?
NI on product to confirm if simplified Chinese and traditional Chinese(zh-TW) are a requirement for 1.3  phone devices ? Per my conversation with delphine these were intended for flatfish delivery and if that's the case we should not be blocking this bug .
Flags: needinfo?(ffos-product)
Axel,

traditional Chinese in 1.3. I thought it was a must have for the phone.
Flags: needinfo?(l10n)
This isn't necessarily a blocking-locale => blocking-bug decision. I think the fall-out for l10n in 1.3 across all locales for this issue isn't in favor of taking this bug in 1.3.
Flags: needinfo?(l10n)
Moving to backlog, we'll move it out of 1.3.
blocking-b2g: 1.3? → backlog
This needs to be part of a larger RTL (and general language support) initiative.  We've put it in backlog for now with the expectation that it will be pulled into such an initiative.
Flags: needinfo?(ffos-product)
The reel order could be defined by the `dateTimeFormat_%X' and `shortTimeFormat' l10n entities, see shared/locales/date. It would be rather easy to parse these strings and find the position of `%p' (= am/pm) relative to %I:%M (= hh:mm) and place the reels accordingly.

Taking, but as I probably won’t work on it before the 1.5 branch anybody is welcome to steal this bug from me.
Assignee: nobody → kaze
I recall that we struggled with that other use-case that deduces information from the string formatter when localizers used variants of the dateformat that weren't expected.

I wonder if that's worth it, of if we just create a flag.

http://mxr.mozilla.org/l10n-gaia/search?string=dateTimeFormat_%25X&case=on&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=l10n-gaia looks rather sane these days, but I doubt we're out of the weeds as soon as we get broader participation in l10n again.
We're definitely relying on "%p" to determine if time is in 12 hours format
https://github.com/mozilla-b2g/gaia/blob/master/apps/clock/js/utils.js#L221
Sorry, I realized only now this bug is in Gaia::System and not Gaia::Clock. Filed bug 978547 for clock, since it's in much worse shape.
These are the problems I found so far in the time picker:
* AM and PM are hard-coded
* separator (":") is hard-coded and positioned between first and second column
* period (AM/PM) column has a different background

I have a local patch that seems to works (doing more tests), not sure how to deal with .html files in /shared (i.e. if I'm supposed to replicate the change in that too).
Attached image Patch tested on Keon
Attached file Pull request on Github
Waiting to see how Travis will decide to die this time.
Comment on attachment 8384271 [details] [review]
Pull request on Github

Green travis for reference
https://travis-ci.org/mozilla-b2g/gaia/builds/19919058
Attachment #8384271 - Flags: review?(kaze)
Looking at tests, there's currently not much in value_selector_test.js (show, hide). 
I'm trying to add a timepicker in there but utterly failing.
Whiteboard: LocRun1.3 → LocRun1.3, LocRun1.2
(In reply to Francesco Lodolo [:flod] from comment #23)
> I'm trying to add a timepicker in there but utterly failing.

Note: I finally managed to add tests, not having experience in that field they might be just using the wrong approach.
Comment on attachment 8384271 [details] [review]
Pull request on Github

The approach is OK (= parsing the time format string instead of the formatted string) but I left a few remarks on your pull-request.

I’m on PTO for one week, so I can’t review this patch before the 1.4 branching; if you want this to land in the 1.4 branch, please ask Alive to review your patch.
Attachment #8384271 - Flags: review?(kaze) → review-
PS: a nice bonus would be to support RTL; this should just be a matter of setting left/right offsets in the CSS file.

If you don’t want to deal with RTL in this patch, please open a follow-up and CC Ahmed Nefzaoui on it: he’s been doing a lot of RTL patches lately (see bug 906270). :-)
Thanks Kaze. I'll take a look at your comments on Github (have a nice week off!).

I don't think it's fundamental to have this on 1.4, it was more important to correctly display the time on lockscreen or status bar, and that's done.

I'll CC Ahmed to this bug, so he's aware of this coming change and maybe we can try to address some things already.
Thanks Francesco. :-)
Comment on attachment 8384271 [details] [review]
Pull request on Github

Your PR update looks great and the l10n tests make a lot of sense.
Travis is green.
Attachment #8384271 - Flags: review- → review+
So much for PTO :-)

I'll set checkin-needed then, so we'll have a consistent time selector now that localizers can translate am/pm.

I'm going to file a follow-up bug for RTL support in the next days, and I think I found another issue but need to study it a bit more.
Keywords: checkin-needed
Master: 6afe56aaa28fe31807993e804c69d6e36ea25e6e
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S3 (14mar)
Assignee: kaze → francesco.lodolo
Blocks: 981598
Blocks: 981604
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: