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 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.

(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.

Reverted the lines with changed encoding.

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).

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 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.

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.

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

(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.

(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!

(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 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.

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.

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

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.

(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.

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.

(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.

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

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

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)

(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?

Files bug 1380751 for that.

> 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.

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