Closed Bug 1505435 Opened 11 months ago Closed 11 months ago

Date-picker in en-US locale is suddenly doing dd/mm/yyyy instead of mm/dd/yyyy

Categories

(MailNews Core :: XUL Replacements, defect)

defect
Not set

Tracking

(thunderbird_esr6064+ fixed, thunderbird64 fixed, thunderbird65 fixed)

RESOLVED FIXED
Thunderbird 65.0
Tracking Status
thunderbird_esr60 64+ fixed
thunderbird64 --- fixed
thunderbird65 --- fixed

People

(Reporter: jik, Assigned: jik)

Details

Attachments

(2 files, 4 obsolete files)

I'm not sure what happened, but the datepicker XUL widget, at least in Thunderbird 60.2.1 though I don't know exactly when it started, is suddenly using DD/MM/YYYY order instead of MM/DD/YYYY, even in the en-US locale which uses MM/DD/YYYY by default.
Hmm.

This is not happening when I use the Thunderbird 60.2.1 release downloaded from bugzilla.mozilla.org.

It is happening when I use the Thunderbird release in Ubuntu.

Will need to investigate further.
OK, so the problem here appears to be that `Intl.DateTimeFormat().resolvedOptions().locale` isn't picking the right locale.

In particular, I have both en-GB and en-US locales installed, and Thunderbird is picking en-GB, even though `locale` indicates that my locale setting at the OS level is en-US, not en-GB.

I have LANG=en_US.UTF-8 set in my environment. I tried LANG=en_US, and it doesn't work either. I tried explicitly setting all of the LC_* environment variables and exporting them before running thunderbird, and that didn't help either.

I'm not certain, but I think that this is actually https://bugzilla.mozilla.org/show_bug.cgi?id=1355307?
I have tried all of the preference settings I could think of to try, including digging through LocaleService.cpp and DateTimeFormat.cpp, in an effort to attempt to influence Thunderbird's choice of locale. Everything I've done has been unsuccessful. I have been unable to identify any way to tell Thunderbird to use en-US as my locale instead of en-GB.

Either the code is not working properly, or whatever it is trying to do is so convoluted and poorly documented that there's no way to understand what "properly" means.

#Frustrated
You may want to read through bug 1426907 (then you'll be really frustrated).
(In reply to Jorg K (GMT+1) from comment #4)
> You may want to read through bug 1426907 (then you'll be really frustrated).

Ah. It seems like perhaps this bug is a duplicate of that one? Do you agree?
Since I'm not on Linux, I tried to avoid frustration by not reading that bug. So you can be the judge of whether it's a duplicate or not.

For the record, I'm most likely the person who has suffered most through all the date/time formatting changes, starting at bug 1325745 on Christmas Eve of 2016. If it weren't past midnight, I could easily find a dozen bugs in C-C and M-C where we, in fact, mostly myself, followed or participated in all those changes.

As far as I'm concerned the matter is mostly closed, with only Linux remaining an issue on some distros. I tried to keep away from those details.
If I understand https://bugzilla.mozilla.org/show_bug.cgi?id=1426907#c73 correctly, then I think the attached patch is how to fix this.
Comment on attachment 9023832 [details] [diff] [review]
Patch to use mozIntl instead of Intl in datetimepicker

Review of attachment 9023832 [details] [diff] [review]:
-----------------------------------------------------------------

As I said, we suffered through at least a dozen datetime formatting bugs and here is now more :-(

Somehow we ported this to C-C and never made the switch to mozIntl, sigh. And since we don't use this in TB, we never noticed.

::: common/bindings/datetimepicker.xml
@@ +488,4 @@
>  
>              var numberOrder = /^(\D*)\s*(\d+)(\D*)(\d+)(\D*)(\d+)\s*(\D*)$/;
>  
> +            var mozIntl = Components.classes["@mozilla.org/mozintl;1"].getService(Components.interfaces.mozIMozIntl);

Please write this as
var mozIntl = Cc["@mozilla.org/mozintl;1"].getService(Ci.mozIMozIntl);
like here:
https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/toolkit/content/widgets/datetimepopup.xml#31

And twice more below. That also works for mozilla60.

@@ +1013,5 @@
> +            var mozIntl = Components.classes["@mozilla.org/mozintl;1"].getService(Components.interfaces.mozIMozIntl);
> +            var dtf = new mozIntl.DateTimeFormat();
> +            var locale = dtf.resolvedOptions().locale + "-u-ca-gregory";
> +            var dtfMonth = new mozIntl.DateTimeFormat(locale, {month: "long", timeZone: "UTC"});
> +            var dtfWeekday = new mozIntl.DateTimeFormat(locale, {weekday: "narrow"});

Also working in TB 60, see examples here:
https://dxr.mozilla.org/mozilla-esr60/search?q=mozIntl.DateTimeFormat&redirect=false

(I was thinking of the 59 change in bug 1428341.)
Attachment #9023832 - Flags: review+
(In reply to Jorg K (GMT+1) from comment #8)
> As I said, we suffered through at least a dozen datetime formatting bugs and
> here is now more :-(
... one more.
Attachment #9023832 - Attachment is obsolete: true
Attached patch datetimepicker.patch - updated (obsolete) — Splinter Review
I'm looking at this again.

Should toLocaleDateString() and toLocaleTimeString() also be replaced in the picker (three call sites) using mozIntl.DateTimeFormat()?

So for example instead of
  var dt = new Date(2002, 9, 4).toLocaleDateString(locale);
it would be
  var dtf = new mozIntl.DateTimeFormat();
  var formatter = dtf.DateTimeFormat(locale, {...});
  var dt = formatter.format(Date(2002, 9, 4));

or words to that extent. See https://searchfox.org/comm-central/search?q=DateTimeFormat&case=false&regexp=false&path= for examples.
Sorry, wires crossed, we both submitted the same patch. Anyway, see my concern in comment #11.
I don't know. I don't pretend to understand all of this. All I know is that it works as expected with the patch I already submitted. I would worry about the unexpected side-effects of additional changes to fix things that do not currently appear to be broken.
The thing is this: toLocaleDateString() is basically Intl.DateTimeFormat(), so we've mostly eliminated its use from the codebase which we switched to mozIntl:
https://searchfox.org/comm-central/search?q=toLocaleDateString&case=false&regexp=false&path=

The reason it that mozIntl follows, at least on Windows, the tweaks that you can do to the date formatting. toLocaleDateString() doesn't follow that. So I'd have to experiment with this a bit.

The same goes for toLocaleTimeString().

A good example to look at is this: https://hg.mozilla.org/comm-central/rev/5298286c9521
I'm not denying any of that.

What I'm saying is that I find all of this stuff to be arcane, incredibly poorly documented, and seemingly in flux, and therefore I am extremely reluctant to make any changes that I do not know for certain are necessary to fix something.

If you feel like you are comfortable with making additional changes, feel free.

My goal in providing a patch was not to sign up for carrying the patch through to landing. I am, frankly, all through playing that game with Thunderbird, having been frustrated by it too many times.

I submitted the patch to point out exactly where the problem is and thereby point others who understand all this stuff far better than I at what they need to do to fix the problem.

Land my patch, land my patch with additional changes, land nothing and let it remain broken, it's up to you. I've done what I intended to do here. *shrug*
Have you checked |new Service.intl.*| instead of |Intl.*| - that is the recommended way if you want OS settings being considered. See also [1] around line 66. Another good source of information about all that stuff is [2].

[1] https://searchfox.org/comm-central/source/mozilla/intl/docs/dataintl.rst
[2] https://firefox-source-docs.mozilla.org/intl/index.html
The comment I linked to above claims that mozIntl is the "recommended way" for privileged code to do this, so that's what I used in my patch.

Now you're telling me that something different is the "recommended way."

I frankly don't know whom to listen to.
I'm looking into it and I'll submit a patch within a few hours. MMD and Zibi are suggesting the same thing, to use mozIntl, which is what |new Service.intl.*| returns.

The problem is the |locale + "-u-ca-gregory-nu-latn"|. mozIntl will spit out dates an Arabic/Korean/Thai etc. if you don't use it.
Attached patch datetimepicker.patch (v2) (obsolete) — Splinter Review
This works for me. Zibi, how can we improve these few lines and avoid getting a new object twice?

+  var mozIntl = Cc["@mozilla.org/mozintl;1"].getService(Ci.mozIMozIntl);
+  var dtf = new mozIntl.DateTimeFormat();
+  var locale = dtf.resolvedOptions().locale + "-u-ca-gregory-nu-latn";
+  dtf = new mozIntl.DateTimeFormat(locale, { timeStyle: "long" });
+  var pmTime = dtf.format(new Date(2000, 0, 1, 16, 7, 9));

Does something like
dtf = new Service.intl.DateTimeFormat("u-ca-gregory-nu-latn", { timeStyle: "long" }); work?
So the "default" locale but with some modifiers?
Attachment #9023943 - Attachment is obsolete: true
Attachment #9023944 - Attachment is obsolete: true
Attachment #9024064 - Flags: feedback?(gandalf)
> This works for me. Zibi, how can we improve these few lines and avoid getting a new object twice?

Yes, we can.

Currently, until bug 1433303 lands, the best way would be to do:

```
let regionalPreferencesLocale = Services.locale.regionalPrefsLocales[0] + "-u-ca-gregory-nu-latn";
let dtf = new mozIntl.DateTimeFormat(locale, { timeStyle: "long" });
let pmTime = dtf.format(new Date(2000, 0, 1, 16, 7, 9));
```

once that lands, we'll want to switch to:

```
let loc = new Intl.Locale(Services.locale.regionalPrefsLocales[0], {
  calendar: "gregory",
  numberingSystem: "latn"
});
```
OK, this should do.
Attachment #9024064 - Attachment is obsolete: true
Attachment #9024064 - Flags: feedback?(gandalf)
Attachment #9024073 - Flags: review+
Attachment #9024064 - Attachment description: datetimepicker.patch → datetimepicker.patch (v2)
Assignee: nobody → jik
Status: NEW → ASSIGNED
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/38005533aabc
Switch date-picker from Intl.DateTimeFormat to mozIMozIntl. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 65.0
Comment on attachment 9024073 [details] [diff] [review]
datetimepicker.patch (v3)

Note that for TB 60 this
  var locale = Services.locale.regionalPrefsLocales[0] + "-u-ca-gregory-nu-latn";
needs to be changed to
  var locale = Services.locale.getRegionalPrefsLocales()[0] + "-u-ca-gregory-nu-latn";
Attachment #9024073 - Flags: approval-comm-beta+
Attachment #9024075 - Flags: approval-comm-esr60+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6c2e0403281d
Follow-up: Import Services.jsm. rs=bustage-fix DONTBUILD
Yes, the behavior is as expected in the try build.
You need to log in before you can comment on or make changes to this bug.