Closed
Bug 1215068
Opened 9 years ago
Closed 9 years ago
Fix the dayperiod patterns in mozIntl
Categories
(Firefox OS Graveyard :: Gaia::L10n, defect, P2)
Tracking
(blocking-b2g:2.5+)
RESOLVED
FIXED
blocking-b2g | 2.5+ |
People
(Reporter: zbraniecki, Assigned: zbraniecki)
References
Details
(Whiteboard: [2.5-rtl-test-run])
Attachments
(1 file, 2 obsolete files)
Unfortunately, dayperiod patterns in mozIntl are not working well for languages like arabic, which don't use western-arabic numbers.
The problem is that date.toLocaleFormat('%p') does not at all cover ar locale. It just always shows 'AM', 'PM'.
We need to move away from toLocaleFormat('%p').
Unfortunately, working that around is also not simple - different languages use different patterns when displaying 'just hour', or 24h/12h clocks which makes it hard to get 'hour dayperiod', and 'hour' strings and remove the latter from the former.
Assignee | ||
Comment 1•9 years ago
|
||
Ted: how hard would it be to add `dayperiod` true|false|undefined to Intl DateTimeFormat options and show/hide this token?
It seems that all other methods are failing, workaround may be very costly, and the current solution using non-standard toLocaleFormat, doesn't work for all locales :(
Flags: needinfo?(tclancy)
Assignee | ||
Comment 2•9 years ago
|
||
It seems surprisingly simple according to http://unicode.org/reports/tr35/tr35-dates.html#Date_Field_Symbol_Table
Is it that simple?
Attachment #8674213 -
Flags: feedback?(tclancy)
Comment 3•9 years ago
|
||
Gregor, this is blocking an RTL bug for 2.5, can we find another owner?
Flags: needinfo?(anygregor)
Comment 4•9 years ago
|
||
Naveed, can you help out here?
Flags: needinfo?(anygregor) → needinfo?(nihsanullah)
Assignee | ||
Comment 5•9 years ago
|
||
This is a fairly complex issue and I'd like to present some background and my perspective on how to fix it.
Historically, we have always had this bug in one form or another. The reason for that is because we never had a good way to format time in a proper localized way. We were mitigating the problem by using Gecko's propertietary `Date.prototype.toLocaleFormat` method combined with some strings that we explicitly asked our localizers to provide.
That worked in some cases, not in others and each piece of code that needed to format time string was coming up with their own creative way to make it look good in some scenarios.
In particular, we have this problem wherever Gecko default language does not match Gaia language because that's how the old, proprietary and hopefully-soon-to-be-deprecated toLocaleFormat works.
In 2.5 we got a new shiny Intl API which provides a very robust and powerful capabilities for date, time and number formatting across locales.
I spent a better part of this cycle moving all of Gaia code to it. We now use Intl API for basically all date/time formatting and for a good chunk of number formattings.
There is unfortunately one limitation of that API so far - and it is that the output strings are opaque and are not supposed to be mingled with.
To add to that, the original idea behind the API was that developers should not control some aspects of time string, like the dayperiod token. This goes against the UX guidelines for Firefox OS which require us to display 12h clock without dayperiod token on lockscreen and in statusbar (and format it in a few other places).
In our mozIntl polyfill I explored two ways of addressing this:
- add `dayperiod` token to Intl options allowing developers to control wherever this token is displayed
- provide additional API that allows us to get information on token positions within datetime string
The former API is simpler, but is also more limited. The latter solves all our problems.
I proposed this change to ECMA 402 standard group and the chairman accepted it. I have a spec, polyfill and Gecko implementations for the `formatWithPosition` function that returns the formatted string and meta information on token positions.
This is also not a novel feature - the ICU library that Chrome, Android, Java, IBM and others use, exposes exactly the same function - so we're in a good position to leverage from it and there's a solid chance that the feature will be added.
Example of how it'll work:
var f = Intl.DateTimeFormat();
var res = f.formatWithPosition(now);
// returns:
// {
// value: "12:32 am",
// position: {
// hour: {beginIndex: 0, endIndex: 2},
// minute: {beginIndex: 3, endIndex: 5},
// dayperiod: {beginIndex: 7, endIndex: 9}
// }
// }
This will, of course allow us to remove the dayperiod token from the string or format it the way we need.
The Gecko patch for it is in bug 1216150 and I discussed it with Waldo who will be my reviewer.
Waldo's only concern is that we want to maximize the chance that the API will not change in the process of ECMA 402 standardization and we will not have to heavily modify the code in half a year.
My current perspective is:
1) This bug should be solved by patch from bug 1216150
2) This bug has to be solved in 2.5 and should be a blocker (together with bug 1216150 which I nominated)
3) We have ECMA 402 meeting in two weeks and I will do my best to get a cross-vendor consensus on this feature by that time
4) Once I have it, I want the patch from bug 1216150 to land immediately and be backported to 2.5 branch
5) With that, I will be able to adjust mozIntl to use it and it will, once and for all, solve all our problems with date/time formattings.
I am absolutely certain that without this, we will see many more edge cases where our current patchwork of Intl API, toLocaleFormat, platform locales and gaia localizations produces wrong outputs and the output is very visible because it is on the lockscreen and statusbar.
I also believe that reverting our code to 2.2 behavior is not a solution because we will still have edge cases where non-latin locales will see this bug, it's just that it's going to be exposed less often so will probably get less backing from the release drivers.
The best way to help me get this feature landed and the whole class of bugs related to date/time formatting fixed is to go to bug 1216150 and help me iron out the patch there. My C++ is very rudimentary and I'd like to make sure that once ECMA agrees on the details of `formatWithPosition` we can land it instantly.
That means that I need the patch to be ready, with tests, and possibly reviewed.
Depends on: 1216150
Assignee | ||
Updated•9 years ago
|
Attachment #8674213 -
Attachment is obsolete: true
Attachment #8674213 -
Flags: feedback?(tclancy)
Comment 6•9 years ago
|
||
Assignee | ||
Comment 7•9 years ago
|
||
Comment on attachment 8680145 [details]
[gaia] zbraniecki:1215068-use-formatWithPosition-in-mozintl > mozilla-b2g:master
To get us ready for fixing it the clean way, I'd like to get f+ from my future reviewer.
This patch will not work just yet as it required the patch from bug 1216150, but once we land that, this is what should fix dayperiod formatting in Gaia.
Attachment #8680145 -
Flags: feedback?(stas)
Assignee | ||
Updated•9 years ago
|
blocking-b2g: --- → 2.5?
Comment 8•9 years ago
|
||
[Tracking Requested - why for this release]:
Other dependent bugs; bug 1211388 and bug 1208808 (https://bugzilla.mozilla.org/show_bug.cgi?id=1208808#c20) are being tracked for post 44.
Please land this on master. Removing nomination for 2.5
blocking-b2g: 2.5? → ---
tracking-b2g:
--- → backlog
Comment 9•9 years ago
|
||
Comment on attachment 8680145 [details]
[gaia] zbraniecki:1215068-use-formatWithPosition-in-mozintl > mozilla-b2g:master
lgtm, thanks. I do prefer your suggestion to concatenate instead of replace tokens, but that's a minor comment.
Attachment #8680145 -
Flags: feedback?(stas) → feedback+
Comment 10•9 years ago
|
||
(In reply to Mahendra Potharaju [:mahe] from comment #8)
> [Tracking Requested - why for this release]:
>
> Other dependent bugs; bug 1211388 and bug 1208808
> (https://bugzilla.mozilla.org/show_bug.cgi?id=1208808#c20) are being tracked
> for post 44.
> Please land this on master. Removing nomination for 2.5
Hi Mahe,
bug 1211388 and bug 1208808 are both follow up bugs for RTL in 2.5.
We still need all of them include this one to be fix in 2.5 branch.
I moved it out from backlog. Thanks
tracking-b2g:
backlog → ---
Flags: needinfo?(mpotharaju)
Priority: -- → P2
Summary: Fix the dayperiod patterns in mozIntl → [RTL] Fix the dayperiod patterns in mozIntl
Whiteboard: [2.5-rtl-test-run]
Assignee | ||
Comment 11•9 years ago
|
||
The current bottle neck for this is the decision in bug 1216150 and the fix in bug 1208808.
Bug 1208808 regressed and the owner - Waldo - is currently overwhelmed with other tasks. If you can find someone else to help with the regression, that would be awesome.
Bug 1216150 is a bit more complex. We are working toward a preliminary code that is going to be in next year's ECMA revision. I have a patch in this bug that requires a little bit more feedback and review from Waldo, but that's not very challenging. What is is that the whole patch is introducing code that we are planning to land in ECMA 402 rev. 3 due to June 2016.
The big question is if we (mostly - Waldo) feels confident enough to let us land this before the spec is finalized.
We have ECMA 402 meeting next week and I'm going to present the feature there. I have support form the spec's editor who wants it in rev.3 and I think we have a good consensus for the API design.
Which means that I hope that we will be able to land the patch in bug 1216150 within the next week or two. If the regression is fixed and the patch lands this bug will be fixed once and for all.
Assignee | ||
Comment 12•9 years ago
|
||
Also, just to make it clear - I strongly believe that this bug should block 2.5. It manifests itself in many places, not only RTL (also German for example) and it makes multiple pieces of our system look broken - Lockscreen, Calendar, Status bar.
Comment 14•9 years ago
|
||
Can we get a status update on this, as it blocks some RTL 2.5 bugs we were hoping to fix and uplift.
Flags: needinfo?(gandalf)
Comment 15•9 years ago
|
||
I know Waldo is working on the fix for bug 1208808. Will follow up with him if he has an ETA
Flags: needinfo?(mpotharaju) → needinfo?(jwalden+bmo)
Comment 16•9 years ago
|
||
ETA is when the patch in bug 1220693 gets uplifted enough places, which I think depends on its being validated on trunk (it only landed today).
Flags: needinfo?(jwalden+bmo)
Comment 17•9 years ago
|
||
I'm confused by what is blocking this now. Currently this bug is marked as blocked by bug 1216150, but in comments 15 and 16 we mention bug 1208808 and bug 1220693. Can someone fix the dependency tree here so we can see what it will take to get this onto 2.5?
Assignee | ||
Comment 18•9 years ago
|
||
The dependency tree is correct. This depends on bug 1216150. I provided a patch and we have to make a decision on wherever we will decide to solve it via implementing next-year ECMA 402 API extension.
The decision is up to :Waldo, and I can only provide him as much information as I can, but it's not an easy decision to make.
If he will decide against resolving this by implementing preliminary support for the feature, we will have to find a backup strategy. Quite honestly, I'm out of ideas how to solve it differently in a way that will work for all locales, but I will focus on that in case of :waldo's veto.
Flags: needinfo?(gandalf)
Updated•9 years ago
|
Assignee: nobody → gandalf
Comment 19•9 years ago
|
||
I chatted on irc with gandalf to understand the situation better. I think we have a solution to move forward. For now our priority is to make that available to gaia apps, so only exposing formatToParts to certified apps would be enough for us.
Once standardization is stabilized, we can move forward on m-c as usual to expose the new api more broadly.
Would that work for everyone?
Comment 20•9 years ago
|
||
Works for me, lets be mindful that there are also blockers downstream of this so late landing will have a cascade effect. Heads-up for :kaze who is on the hook for at least some of them.
Flags: needinfo?(kaze)
Assignee | ||
Comment 21•9 years ago
|
||
The only cascade I know of is that we will have to land bug 1216150 and then a patch in this bug to switch mozIntl to use formatToParts (when available).
Comment 22•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8680145 -
Attachment is obsolete: true
Assignee | ||
Comment 23•9 years ago
|
||
This pull request is minimal. It requires fix from bug 1216150.
We will want to fully switch to formatToParts in 2.6 cycle, but my priority now is to get us in line to fix the dayperiod related bugs in 2.5 for which we'll need to land bug 1216150 and this patch.
I tested this patch on the device against all blocker bugs (calendar, lockscreen and status bar) and it fixes them.
Flags: needinfo?(ted.clancy)
Summary: [RTL] Fix the dayperiod patterns in mozIntl → Fix the dayperiod patterns in mozIntl
Assignee | ||
Updated•9 years ago
|
Attachment #8693820 -
Flags: review?(stas)
Comment 24•9 years ago
|
||
Comment on attachment 8693820 [details]
[gaia] zbraniecki:1215068-use-formatToParts-in-mozintl > mozilla-b2g:master
lgtm, sorry for the long wait. Are you also planning to rewrite the token formatting using formatToParts? Is there already a bug to track that?
Attachment #8693820 -
Flags: review?(stas) → review+
Assignee | ||
Comment 25•9 years ago
|
||
Fabrice, I have the patch ready and it looks good except that it fails sharedtest GU (GU19) - my first guess is that for some reason sharedtest app is not certified. Can we make it certified?
Flags: needinfo?(nihsanullah)
Flags: needinfo?(kaze)
Flags: needinfo?(fabrice)
Assignee | ||
Comment 26•9 years ago
|
||
It works on the device for all apps (I can WebIDE into the app and use `formatToParts`), but when I launch ./bin/gaia-test against the FirefoxNightly, the `formatToParts` is not available in ./apps/sharedtest.
Assignee | ||
Comment 27•9 years ago
|
||
Ok, `formatToParts` is not available in any Gu test when launched against FirefoxNightly. It seems like Gu tests are not launched in certified environment :/
I'm going to shim it in tests...
Assignee | ||
Comment 28•9 years ago
|
||
Landed with a shim for the tests and a note: https://github.com/mozilla-b2g/gaia/commit/879770a42cd5e4cedc6dcb5f3bc420605364c7f5
I'm leaving NI on Fabrice in case he'd prefer me to somehow get Gu work with certified only API.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 29•9 years ago
|
||
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #25)
> Fabrice, I have the patch ready and it looks good except that it fails
> sharedtest GU (GU19) - my first guess is that for some reason sharedtest app
> is not certified. Can we make it certified?
Sure.
Flags: needinfo?(fabrice)
You need to log in
before you can comment on or make changes to this bug.
Description
•