Closed Bug 1354445 Opened 6 years ago Closed 6 years ago

Migrate Intl.DateTimeFormat to mozIntl.DateTimeFormat in chrome

Categories

(Core :: Internationalization, defect)

Product:
Core
Component:
Internationalization
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file)

Bug 1354445 - Migrate Intl.DateTimeFormat to mozIntl.DateTimeFormat in chrome.
59 bytes, text/x-review-board-request
mak
: review+
Details 
At the moment this affects:
 - browser/base/content/content.js
 - browser/components/feeds/FeedWriter.js
 - browser/components/places/content/places.js
 - browser/components/places/content/treeView.js
 - toolkit/components/passwordmgr/LoginManagerContent.jsm
 - toolkit/components/passwordmgr/LoginManagerContextMenu.jsm
 - toolkit/components/passwordmgr/content/passwordManager.js
 - toolkit/content/widgets/datepicker.js
 - toolkit/content/widgets/datetimebox.xml
 - toolkit/content/widgets/datetimepicker.xml
 - toolkit/crashreporter/content/crashes.js

In most cases, it would be good to unify the options into `dateStyle`/`timeStyle` to let mozIntl.DateTimeFormat use OS regional prefs when formatting.
This one seems fairly easy to fix. Let's see if any tests depend on the old behavior.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
NI'ing :jessica to put this one on her your radar.

There are three ways to get the order of components and time format for datetimepicker - 

 * Intl.DateTimeFormat - it'll use CLDR data and you can use formatToParts and resolvedOptions to get details for a given locale
 * mozIntl.getLocaleInfo - it'll use CLDR data
 * mozIntl.createDateTimeFormat - API is the same as ECMA402 one, but it'll also consult OS regional preferences

I believe it would be worth switching to the last API, so that the picker respects the order of components user selected in the OS and possibly time format (h12/h24).

Probably not the highest priority, but just wanted to flag you so that you know that we're migrating to this new API :)
Flags: needinfo?(jjong)
Comment on attachment 8856202 [details]
Bug 1354445 - Migrate Intl.DateTimeFormat to mozIntl.DateTimeFormat in chrome.

https://reviewboard.mozilla.org/r/128152/#review130654

::: browser/base/content/content.js:50
(Diff revision 1)
>    return new tmp.PageMenuChild();
>  });
>  XPCOMUtils.defineLazyModuleGetter(this, "Feeds",
>    "resource:///modules/Feeds.jsm");
>  
> +const mozIntl = Cc["@mozilla.org/mozintl;1"].getService(Ci.mozIMozIntl);

May I suggest adding this to Services.jsm as Services.intl?

::: browser/components/places/content/treeView.js:514
(Diff revision 1)
>  
>    __dateFormatter: null,
>    get _dateFormatter() {
>      if (!this.__dateFormatter) {
> -      const dtOptions = { year: "numeric", month: "numeric", day: "numeric",
> -                          hour: "numeric", minute: "numeric" };
> +      const dtOption = {
> +        dateStyle: "long",

how can numeric, long, numeric be long before... if here numeric, numberic, numeric is long again?
> May I suggest adding this to Services.jsm as Services.intl?

Good call, sure!

> how can numeric, long, numeric be long before... if here numeric, numberic, numeric is long again?

Good call again! I believe the closest will be "short" :)

I'll fix it with a bunch of tests in the next revision.
Blocks: 1343768
Comment on attachment 8856202 [details]
Bug 1354445 - Migrate Intl.DateTimeFormat to mozIntl.DateTimeFormat in chrome.

https://reviewboard.mozilla.org/r/128152/#review130824

::: browser/components/places/content/places.js:11
(Diff revision 2)
>  /* import-globals-from editBookmarkOverlay.js */
>  // Via downloadsViewOverlay.xul -> allDownloadsViewOverlay.xul
>  /* import-globals-from ../../../../toolkit/content/contentAreaUtils.js */
>  
> +const mozIntl = Components.classes["@mozilla.org/mozintl;1"]
> +  .getService(Components.interfaces.mozIMozIntl);

I'd be fine importing Services.jsm into places.js

::: browser/components/places/content/treeView.js:12
(Diff revision 2)
>  const PTV_interfaces = [Ci.nsITreeView,
>                          Ci.nsINavHistoryResultObserver,
>                          Ci.nsINavHistoryResultTreeViewer,
>                          Ci.nsISupportsWeakReference];
>  
> +const mozIntl = Cc["@mozilla.org/mozintl;1"].getService(Ci.mozIMozIntl);

as well as here.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #3)
> NI'ing :jessica to put this one on her your radar.
> 
> There are three ways to get the order of components and time format for
> datetimepicker - 
> 
>  * Intl.DateTimeFormat - it'll use CLDR data and you can use formatToParts
> and resolvedOptions to get details for a given locale
>  * mozIntl.getLocaleInfo - it'll use CLDR data
>  * mozIntl.createDateTimeFormat - API is the same as ECMA402 one, but it'll
> also consult OS regional preferences
> 
> I believe it would be worth switching to the last API, so that the picker
> respects the order of components user selected in the OS and possibly time
> format (h12/h24).
> 
> Probably not the highest priority, but just wanted to flag you so that you
> know that we're migrating to this new API :)

Thanks. I'll keep in mind about this. Is there any documentation on how to use `mozIntl.createDateTimeFormat`?
:jessica, not really unfortunately :(

You can see how we use it here - we just instead of listing all options manually (day: "long", month: "numeric" etc.) we use "dateStyle"/"timeStyle" with full | long | Medium | short and this allows us to look into OS preferences and adjust the result parts by it.

That means that taking "timeStyle: 'long'" will give you time with hour12/24 resolved not only to the locale, but also taking into account use preferences.
Same for date - if someone will manually set "yyyy !! MM && dd" pattern or something weird like that, then formatToParts will take this into account and give you:

[
  {type: "year", value: "2017"},
  {type: "literal", value: " !! "},
  {type: "month", value: "January"},
  {type: "literal", value: " && "},
  {type: "day", value: "23rd"}
]

which datetimepicker can then use both to display the pattern in the input box and for picker sorting.
Flags: needinfo?(jjong)
Comment on attachment 8856202 [details]
Bug 1354445 - Migrate Intl.DateTimeFormat to mozIntl.DateTimeFormat in chrome.

redirecting to :mak to offload :jfkthame :)
Attachment #8856202 - Flags: review?(jfkthame) → review?(mak77)
Comment on attachment 8856202 [details]
Bug 1354445 - Migrate Intl.DateTimeFormat to mozIntl.DateTimeFormat in chrome.

https://reviewboard.mozilla.org/r/128152/#review131362

I have a couple questions, I was trying Components.classes["@mozilla.org/mozintl;1"].getService(Components.interfaces.mozIMozIntl) in the browser console with some of the options, and it doesn't seem to work (or maybe I'm doing it wrong).

I'm in an en-US build, my system locale and time prefs are Italian. win10 x64. 10th of April Nightly.
mozIntl.createDateTimeFormat(undefined, { dateStyle: "medium" }).format(new Date()) => "4/11/2017".
mozIntl.createDateTimeFormat(undefined, { dateStyle: "short" }).format(new Date()) => "4/11/2017".
I was expecting "11/4/2017"

Also why is short == medium? Looking at Windows prefs, it indeed looks like it only defines short and extended, so maybe that's expected?
Though both don't respect system settings, the result is the same as it was with non-moz Intl.

as well as, long gives me "Tuesday, April 11, 2017", while system settings are "dddd d MMMM yyyy"

Finally, it still gives 12hours format, while system settings are 24hours.

::: browser/base/content/content.js:361
(Diff revision 7)
>          // If the difference is more than a day.
>          if (Math.abs(difference) > 60 * 60 * 24) {
> -          let formatter = new Intl.DateTimeFormat();
> +          let formatter = Services.intl.createDateTimeFormat(undefined, {
> +            dateStyle: "long",
> +            timeStyle: "short",
> +          });

Both instances in content were not passing any argument to DateTimeFormat, that on my system is equal to short/short.
Is there a specific reason to change to long/short here?
I found this screenshot in bug 1267229 that introduced this code: https://bug1267229.bmoattachments.org/attachment.cgi?id=8812748
where the date appears as short.

I don't have a particular preference, maybe you have localization reasons for this change? Otherwise I'd probably leave it as it was (short)

This question is valid for all the instances converted from Intl.DateTimeFormat() (no arguments)

::: browser/base/content/content.js:393
(Diff revision 7)
>            let systemDate = new Date();
>  
>            if (buildDate > systemDate) {
> -            let formatter = new Intl.DateTimeFormat();
> +            let formatter = Services.intl.createDateTimeFormat(undefined, {
> +              dateStyle: "long",
> +              timeStyle: "medium",

ditto (why here time becomes medium?)

::: browser/components/feeds/FeedWriter.js:195
(Diff revision 7)
>    get _dateFormatter() {
>      if (!this.__dateFormatter) {
> -      const dtOptions = { year: "numeric", month: "long", day: "numeric",
> -                          hour: "numeric", minute: "numeric" };
> -      this.__dateFormatter = new Intl.DateTimeFormat(undefined, dtOptions);
> +      const dtOptions = {
> +        timeStyle: "short",
> +        dateStyle: "long"
> +      }

missing semicolon, eslint may not be happy

::: browser/components/places/content/treeView.js:515
(Diff revision 7)
>    get _dateFormatter() {
>      if (!this.__dateFormatter) {
> -      const dtOptions = { year: "numeric", month: "numeric", day: "numeric",
> -                          hour: "numeric", minute: "numeric" };
> -      this.__dateFormatter = new Intl.DateTimeFormat(undefined, dtOptions);
> +      const dtOptions = {
> +        dateStyle: "short",
> +        timeStyle: "short"
> +      }

missing semicolon

::: browser/components/places/tests/chrome/test_treeview_date.xul:130
(Diff revision 7)
>                  let timeObj = new Date(node.time / 1000);
>                  // Default is short date format.
> -                let dtOptions = { year: 'numeric', month: 'numeric', day: 'numeric',
> -                                  hour: 'numeric', minute: 'numeric' };
> +                let dtOptions = {
> +                  dateStyle: "short",
> +                  timeStyle: "short"
> +                }

missing semicolon

::: toolkit/components/passwordmgr/test/unit/test_user_autocomplete_result.js:4
(Diff revision 7)
>  XPCOMUtils.defineLazyModuleGetter(this, "LoginHelper",
>                                    "resource://gre/modules/LoginHelper.jsm");
>  Cu.import("resource://gre/modules/LoginManagerContent.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");

This is not needed, head.js is already importing it.

::: toolkit/crashreporter/content/crashes.js:81
(Diff revision 7)
>    }
>  
>    var dateFormatter;
>    var timeFormatter;
>    try {
> -    dateFormatter = new Intl.DateTimeFormat(undefined, { year: "2-digit",
> +    dateFormatter = Services.intl.createDateTimeFormat(undefined, { dateStyle: "short" });

it's not exactly the same as 2 digit, but imo it makes more sense as 4 digits, as you did. I had a look at about:crashes and it's unreadable as 2 digits.

::: toolkit/modules/Services.jsm:78
(Diff revision 7)
>    ["downloads", "@mozilla.org/download-manager;1", "nsIDownloadManager"],
>    ["droppedLinkHandler", "@mozilla.org/content/dropped-link-handler;1", "nsIDroppedLinkHandler"],
>    ["els", "@mozilla.org/eventlistenerservice;1", "nsIEventListenerService"],
>    ["eTLD", "@mozilla.org/network/effective-tld-service;1", "nsIEffectiveTLDService"],
>    ["io", "@mozilla.org/network/io-service;1", "nsIIOService2"],
> +  ["intl", "@mozilla.org/mozintl;1", "mozIMozIntl"],

please add a test here:
http://searchfox.org/mozilla-central/source/toolkit/modules/tests/xpcshell/test_Services.js
Attachment #8856202 - Flags: review?(mak77)
> I'm in an en-US build, my system locale and time prefs are Italian. win10 x64. 10th of April Nightly.

The HiG of both MacOS and Windows indicate that you're setting the regional settings for the given locale, not for any locale.

So when you have Italian Windows, you're really adapting your hour12/hour24 or date format to Italian, not to any possible locale.

The reason for this is that many date patterns are not very self-descriptive and only make sense in the context of a given locale. 

For example: 

Imagine "dd MMM y". This is a pattern for medium style date in Italian[0]. If you have Italian Windows, any date-medium will be formatted using this pattern and then localized to give you sth like "24 marzo 2017".

But in the case described, we'll localize the date to Firefox UI locale which is en-US. If we take the pattern from Windows, we'll end up with "24 March 2017". This is a combo of Italian pattern and English translation.

In some cases this may get weird, like in case of "de" date-short which is "dd.MM.yy"[1] so if you combine German pattern with English translation you'll end up with "24.Mar.2017" which is a format that makes little sense to a German or an English speaker.

In order to prevent that from happening, we imposed a limit of how close the OS and Firefox locale have to be in order for us to use the regional settings across them.
The current limit is that the language component have to match, so we'll end up with "en-AU" matching "en-US" or "fr-CA" matching "fr-CH" but not "it" vs "en-US".

I'm willing to discuss that limit in a separate bug, as we could potentially for example keep it as-is for dates, but loosen it for time to let hour12/24 be better represented (although that opens another pandora box when OS locale is RTL and Firefox locale is LTR).

> Is there a specific reason to change to long/short here?

Great catch! You're right, it should be short (and no timeStyle)!

Fixing the rest of comments in the patch.
[0] http://www.unicode.org/cldr/charts/29/summary/it.html#1824
[1] http://www.unicode.org/cldr/charts/29/summary/de.html#2175
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #18)
> But in the case described, we'll localize the date to Firefox UI locale
> which is en-US. If we take the pattern from Windows, we'll end up with "24
> March 2017". This is a combo of Italian pattern and English translation.

This makes sense for alphanumeric dates, but those are readable even in English, since there's no risk of confusing the month and the day. I'm fine with that.
It makes less sense for purely numeric results ("short" in our case) since 04/11/2017 and 11/04/2017 are both valid dates. I blame US for this ;)

> I'm willing to discuss that limit in a separate bug, as we could potentially
> for example keep it as-is for dates, but loosen it for time to let hour12/24
> be better represented (although that opens another pandora box when OS
> locale is RTL and Firefox locale is LTR).

It would be great and greatly appreciated to be able to retain OS settings for "short" dates and times, where the result is purely numeric.

Regardless, this is an improvement globally and I'll r+ it as such. I agree that there is the need to evaluate a strategy to enlarge the limit, since this will fix bug 1343768 and similar only for a small subset of matching locales, not for everyone.
> It makes less sense for purely numeric results ("short" in our case) since 04/11/2017 and 11/04/2017 are both valid dates. I blame US for this ;)

Unfortunately, that's a bit more complicated.

There are multiple numeric systems in the world, and in a scenario such as an Arabic Windows vs en-US Firefox, which also happen to mix directionality with Arabic being RTL and en-US being LTR.

Trying to mix and match across them may give you an RTL pattern (Arabic Windows) localized using western-arabic numerals (0-9) in en-US Firefox and arabic separators which I bet would be very confusing (imagine "٣١‏/١٢‏/١٩٦٩" but with values replaced with 2017, 12 and 23).
Comment on attachment 8856202 [details]
Bug 1354445 - Migrate Intl.DateTimeFormat to mozIntl.DateTimeFormat in chrome.

https://reviewboard.mozilla.org/r/128152/#review131468

Changes look good, there's an orange XPCShell failure in the passwordmanager that looks suspicious though, probably due to this:
http://searchfox.org/mozilla-central/rev/624d25b2980cf5f83452b040e6d664460f9b7ec5/toolkit/components/passwordmgr/test/unit/test_context_menu.js#105

r=me with a "green" Try

::: browser/base/content/content.js:359
(Diff revision 9)
>          }
>  
>          // If the difference is more than a day.
>          if (Math.abs(difference) > 60 * 60 * 24) {
> -          let formatter = new Intl.DateTimeFormat();
> +          let formatter = Services.intl.createDateTimeFormat(undefined, {
> +            dateStyle: "short",

trailing comma

::: browser/base/content/content.js:391
(Diff revision 9)
>            let buildDate = new Date(year, month, day);
>            let systemDate = new Date();
>  
>            if (buildDate > systemDate) {
> -            let formatter = new Intl.DateTimeFormat();
> +            let formatter = Services.intl.createDateTimeFormat(undefined, {
> +              dateStyle: "short",

trailing comma

::: browser/base/content/test/general/browser_aboutCertError.js:155
(Diff revision 9)
>        };
>      });
>    }
>  
> -  let formatter = new Intl.DateTimeFormat();
> +  let formatter = Services.intl.createDateTimeFormat(undefined, {
> +    dateStyle: "short",

trailing comma
Attachment #8856202 - Flags: review?(mak77) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/79673e659b31
Migrate Intl.DateTimeFormat to mozIntl.DateTimeFormat in chrome. r=mak
https://hg.mozilla.org/mozilla-central/rev/79673e659b31
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Have you forgotten cookies and page info? Both those panels format dates. In a recent Nightly I see for bookmarks:
14/09/2016, 16:23 (using en-GB in the en-US app), but cookies show July 17, 2017, 11:07:13 AM and page info shows:
July 22, 2017, 10:29:40 PM
Flags: needinfo?(gandalf)
Blocks: 1383463
OK, filed bug 1383463 for cookies and page info.
Flags: needinfo?(gandalf)
You need to log in before you can comment on or make changes to this bug.