1350679 - Remove deprecated toLocaleFormat from calendar, use Intl.DateTimeFormat or mozIntl instead

Status: RESOLVED FIXED

(Calendar :: General, enhancement, P3)

Description

[[:MakeMyDay]]

Since bug 1332351 landed, apart from suite, calendar is the remaining consumer of the deprecated Date.toLocaleFormat function. This bug is to remove it from the calendar code:

https://searchfox.org/comm-central/search?q=toLocaleFormat&case=true&regexp=false&path=calendar

Comment 1

[Martin Schröder [:mschroeder]]

Attachment: Patch v1

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 8881078 [details] [diff] [review]
Patch v1

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

::: calendar/import-export/calOutlookCSVImportExport.js
@@ +50,5 @@
>      headAllDayEvent:  "Evenement, duurt hele dag",
>      headAlarm:        "Herinneringen aan/uit",
>      headAlarmDate:    "Herinneringsdatum",
>      headAlarmTime:    "Herinneringstijd",
> +    headCategories:   "Categorie�n",

Something wrong here. Looks like your patch is encoded as ANSI, but it should be UTF-8.

@@ +55,3 @@
>      headDescription:  "Beschrijving",
>      headLocation:     "Locatie",
> +    headPrivate:      "Priv�",

And here.

Comment 3

[Martin Schröder [:mschroeder]]

(In reply to Jorg K (GMT+2) from comment #2)
> Comment on attachment 8881078 [details] [diff] [review]
> Patch v1
[...]
> Something wrong here. Looks like your patch is encoded as ANSI, but it
> should be UTF-8.

My IDE was too smart and tried to correct some corrupt encoding, probably because these strings are special to MS Outlook. I will try to generate the patch using other tooling.

Comment 4

[Martin Schröder [:mschroeder]]

Attachment: Patch v2

Reverted the lines with changed encoding.

Comment 5

[[:MakeMyDay]]

Thanks for taking this. I would like to use mozIntl datetime formatting here as well, see my comment #31 in bug 1360915. What do you think?

If we do so, we would need to include also one or two further previous occurences, that have been adapted already recently in the minimonth accessibility bug (sorry, I don't recall the bug number offhand).

Comment 6

[Martin Schröder [:mschroeder]]

I do not think that for the use cases touched in this patch mozIntl is helpful. They are (1) specialized parsing and formatting in en-US always for Outlook import/export, (2) weekday formatting and (3) specialized code to setup minimonth widget correctly.

Comment 7

[[:MakeMyDay]]

Comment on attachment 8881123 [details] [diff] [review]
Patch v2

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

Ok, let's go with it for now. Thanks for the patch, r+ with the style nits fixed.

But I am still in favour to consolidate all datetime formatting accross the calendar code in a new jsm or whatever we decide to go for js modules to get rid of the xpcom interface (we shold do that wherever already possible) and allow to cover all of it by unit tests. That refactoring should also include the places and options you touched with this patch. But let's leave it for a separate bug.

::: calendar/base/src/calTimezoneService.js
@@ +570,5 @@
>  
>      function weekday(icsDate, timezone) {
>          let calDate = cal.createDateTime(icsDate);
>          calDate.timezone = timezone;
> +        return cal.dateTimeToJsDate(calDate).toLocaleString(undefined, { 'weekday': 'short' });

Please use double quotes instead of single qoutes here and at the other places in this patch for pattern like this.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: rm_tolocaleformat.diff (v3).

I took the liberty to change the quotes. At the same time I noticed that the (v2) had some funny things going on with accented characters, which I also corrected. qimportbz also failed on Windows.

More importantly, I think the object attributes shouldn't have quotes, so this was wrong, right?
return cal.dateTimeToJsDate(calDate).toLocaleString(undefined, { 'weekday': 'short' });
weekday should not have quotes.

Martin, can you please check the patch again. I also noticed
+          let pmProbeString = pmProbeTime.toLocaleTimeString();
without any argument. That's OK, I suppose.

Comment 9

[Jorg K (CEST = GMT+2)]

Oh, the (v2) patch was ANSI, but for Privé you need UTF-8.

Comment 10

[Martin Schröder [:mschroeder]]

(In reply to Jorg K (GMT+2) from comment #8)
> Created attachment 8884530 [details] [diff] [review]
> rm_tolocaleformat.diff (v3).
> 
> I took the liberty to change the quotes. At the same time I noticed that the
> (v2) had some funny things going on with accented characters, which I also
> corrected. qimportbz also failed on Windows.

Thanks for that. I do not know what happened there, but the encoding of some file is fishy and seems to be only okay on Windows and gets destroyed on macOS.

> More importantly, I think the object attributes shouldn't have quotes, so
> this was wrong, right?
> return cal.dateTimeToJsDate(calDate).toLocaleString(undefined, { 'weekday':
> 'short' });
> weekday should not have quotes

Ack.

> Martin, can you please check the patch again. I also noticed
> +          let pmProbeString = pmProbeTime.toLocaleTimeString();
> without any argument. That's OK, I suppose.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleTimeString shows arguments as optional.

Comment 11

[Martin Schröder [:mschroeder]]

(In reply to [:MakeMyDay] from comment #7)
> Comment on attachment 8881123 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8881123 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, let's go with it for now. Thanks for the patch, r+ with the style nits
> fixed.
> 
> But I am still in favour to consolidate all datetime formatting accross the
> calendar code in a new jsm or whatever we decide to go for js modules to get
> rid of the xpcom interface (we shold do that wherever already possible) and
> allow to cover all of it by unit tests. That refactoring should also include
> the places and options you touched with this patch. But let's leave it for a
> separate bug.

I did not introduce any XPCOM interface to the code but used the standard JavaScript Date object and its methods!

Comment 12

[[:MakeMyDay]]

(In reply to Martin Schröder [:martinschroeder] from comment #11)

> I did not introduce any XPCOM interface to the code but used the standard
> JavaScript Date object and its methods!

I know that you didn't. There have been two motivations for my first comment: making the way of formatting consistent across calendar (which even with this patch isn't the case, beacuse toLocaleString is using Intl instead of mozIntl iirc), but as mentioned above it is ok as is for now. The second was to get rid of the current datetimeformatter mid-term and convert it to js only. My last comment was intended to explain this, but obviously it failed.

Comment 13

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/8635ff1796b88bfdfaa9c72a576bd6778f239125

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 8884530 [details] [diff] [review]
rm_tolocaleformat.diff (v3).

We need to uplift this since TB 55 will be a messy mixture of date/time formatting otherwise.

Comment 15

[Jorg K (CEST = GMT+2)]

Sorry, this patch in datetimepickers.xml must use mozIntl otherwise the display is inconsistent, I currently get en-GB dates with en-US times. Grrr.

Patch coming.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1350679-time-picker.patch (v1).

Sorry, just when I thought we were done, another very visible problem popped up.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: 1350679.png - Screenshot

mozIntl works. As you can see in the error console, I have a Daily en-US version. However, the UI follows en-GB dates/times.

Without the additional patch, I get 5 PM in the date picker since that follows the app locale which we don't want.

Comment 18

[Martin Schröder [:mschroeder]]

(In reply to Jorg K (GMT+2) from comment #16)
> Created attachment 8884578 [details] [diff] [review]
> 1350679-time-picker.patch (v1).
> 
> Sorry, just when I thought we were done, another very visible problem popped
> up.

You should probably change every call to toLocaleString()/toLocaleTimeString (excluding calOutlookCSVImportExport.js) from my patch.

Comment 19

[Jorg K (CEST = GMT+2)]

I don't think so:
calTimezoneService.js: Formats a weekday.
calOutlookCSVImportExport.js you don't suggest to change, and me neither.
That leaves the picker.

Comment 20

[Martin Schröder [:mschroeder]]

(In reply to Jorg K (GMT+2) from comment #19)
> I don't think so:
> calTimezoneService.js: Formats a weekday.
> calOutlookCSVImportExport.js you don't suggest to change, and me neither.
> That leaves the picker.

As you like it, but for consistency I would recommend to also change the two occurrences you missed in the picker code: amProbeString/pmProbeString.

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 1350679-time-picker.patch (v2).

Oops, yes,I missed some. Thanks for letting me know.

Comment 22

[Jorg K (CEST = GMT+2)]

Could you please review this quickly since I need this for TB 55 which we will build soon.

Comment 23

[[:MakeMyDay]]

Comment on attachment 8884583 [details] [diff] [review]
1350679-time-picker.patch (v2).

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

Thanks, r+

As a side note: there exist further places where a date formatting still doesn't use mozIntl yet:

https://dxr.mozilla.org/comm-central/search?q=path%3Aminimonth.xml+tolocale&redirect=false

(the two last hits in minimonth.xml)

Comment 24

[Jorg K (CEST = GMT+2)]

(In reply to [:MakeMyDay] from comment #23)
> As a side note: there exist further places where a date formatting still
> doesn't use mozIntl yet:
> https://dxr.mozilla.org/comm-central/search?q=path%3Aminimonth.
> xml+tolocale&redirect=false
> (the two last hits in minimonth.xml)
:-((
Those hits both set the aria-label, right? So how do I see/test that?

Comment 25

[Jorg K (CEST = GMT+2)]

Files bug 1380751 for that.

Comment 26

[[:MakeMyDay]]

> Those hits both set the aria-label, right? So how do I see/test that?

If you don't have a screen reader at hand, dom inspector is probably the best choice, I assume.

Comment 27

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/3a4c51fcb4aa98885607679be7c4c7b5082641d4

Comment 28

[Jorg K (CEST = GMT+2)]

Beta (TB 55, Calendar 5.7):
https://hg.mozilla.org/releases/comm-beta/rev/4c00dd76c910fc88d060cbb460a9f5fb84916149
https://hg.mozilla.org/releases/comm-beta/rev/f594e262e735030a4734d703c93036e83aa699d7