Closed Bug 1396211 Opened 2 years ago Closed 2 years ago

Clean up code for dates in aboutTelemetry.js

Categories

(Toolkit :: Telemetry, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: flod, Assigned: flod)

Details

Attachments

(2 files)

Right now datetime in pings is displayed as "mm/dd/yyyy hh:mm:ss", with some hard coded logic and concatenations in aboutTelemetry.js

http://searchfox.org/mozilla-central/rev/2aa0806c598ec433e431728f5ddd3a6847c1a417/toolkit/content/aboutTelemetry.js#169-210

There's also a lot of unused code around.

shortDateString is completely unused since you're using date.toLocaleDateString()
http://searchfox.org/mozilla-central/rev/2aa0806c598ec433e431728f5ddd3a6847c1a417/toolkit/content/aboutTelemetry.js#463
For user-facing datetime formatting, please, use mozIntl.createDateTimeFormat and use dateStyle and timeStyle.
That API can optionally use OS regional preferences the user set.
Comment on attachment 8903889 [details]
Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code

Zibi, does this make any sense in terms of JS and Intl?

I don't want to change the format they're using ("date time, pingname"), and couldn't find a better way to do it. Using toLocaleString would get me "date, time, ping".
Attachment #8903889 - Flags: feedback?(gandalf)
Comment on attachment 8903889 [details]
Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code

https://reviewboard.mozilla.org/r/175668/#review180740

::: toolkit/content/aboutTelemetry.js:429
(Diff revision 1)
> +    let timeFormat = {
> +      hour: '2-digit',
> +      minute: '2-digit',
> +      second: '2-digit',
> +      hour12: false
> +    };

Why do you force `hour12: false` here? That's not intl if hardcode something that should depend on locale :)

::: toolkit/content/aboutTelemetry.js:434
(Diff revision 1)
> +    };
>      for (let p of this._archivedPings) {
>        pingTypes.add(p.type);
> -      let date = new Date(p.timestampCreated);
> -      let datetext = date.toLocaleDateString() + " " + shortTimeString(date);
> +      let pingDate = new Date(p.timestampCreated);
> +      let datetext = (new Intl.DateTimeFormat([],dateFormat).format(pingDate)) +
> +                     " " + (new Intl.DateTimeFormat([],timeFormat).format(pingDate));

So, Intl.DateTimeFormat is the standardized part of JavaScript that allows us to generate internationalized date/time. So your use of that API is technically correct.

Now, at Mozilla, we have an extended version of that API that is chrome-only, and allows us to do more. It's called mozIntl.DateTimeFormat.

Whenever you work within Firefox chrome, I recommend using mozIntl.DateTimeFormat over Intl.DateTimeFormat.

The mozIntl version optionally looks up OS regional preferences following user adjustments.

Since Windows and Mac only allow the user to adjust their date/time formatting for styles short/medium/long/full - we have a new options `dateStyle` and `timeStyle`. If you use them, we'll look up into regional prefs.

There's a convenient accessor to mozIntl - `Services.intl`.

So, I'd suggest:
```
const dateText = Services.intl.createDateTimeFormat(undefined, {dateStyle: 'short'}).format(pingdate);
const timeText = Services.intl.createDateTimeFormat(undefined, {timeStyle: 'long'}).format(pingdate);
let text = `${dateText} ${timeText}, ${p.type}`.
```

You can toy with different values for dateStyle/timeStyle by opening browser console and calling the `Services.intl.createDateTimeFormat(undefined, {...}).format(Date.now())`

Now, the other problem with your approach is that you hardcode the space between date and time making it forcefully left-to-right. Also, not every language uses space as a separator between date and time.

So, you can imporve that partially by:
```
const dateText = Services.intl.createDateTimeFormat(undefined, {dateStyle: 'short', timeStyle: 'long'}).format(pingdate);
let text = `${dateText}, ${p.type}`.
```

Now, mozIntl will look up date/time separator for the given locale and use it. In result your date and time text is now properly internationalized.

But you still have the hardcoded LTR part about `, ${p.type}`.
I'm not sure what to do about it. My instinct would be to try to have two semantic elements - one for date, one for type, but it doesn't seem like it'll work for `<option>` so maybe it's ok to leave it for now?

::: toolkit/content/aboutTelemetry.js:444
(Diff revision 1)
>        option.appendChild(content);
>        option.setAttribute("value", p.id);
>        option.dataset.type = p.type;
>        option.dataset.date = datetext;
>  
> -      if (date.toDateString() == todayString) {
> +      if (pingDate.toDateString() == today.toDateString()) {

nit: forcing two dates to produce a string representation is very inefficient.

On top of that, you're possibly doing this twice here.

It would be better to get a representation of the date stripped of time:
```
const dateWithoutTime = new Date(pingDate).setHours(0, 0, 0, 0);
const today = new Date();
today.setHours(0, 0, 0, 0);
const yesterday = new Date(today);
yesterday.setDate(today.getDate() - 1);

if (dateWithoutTime === today) {
  ...
} else if (dateWithoutTime === yesterday) {
}
```
Comment on attachment 8903889 [details]
Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code

https://reviewboard.mozilla.org/r/175668/#review180754

Thanks for taking the time Zibi, that's a lot of useful information :)

I have the feeling I'm out of my depth here, but let's give it a shot.
Comment on attachment 8903889 [details]
Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code

https://reviewboard.mozilla.org/r/175668/#review180740

> Why do you force `hour12: false` here? That's not intl if hardcode something that should depend on locale :)

Because I'm trying to stay as close as possible to the current behavior, in case I'm not sure it would be my call to change the current time format. 

For example we could drop the hardcoded space and use:

Services.intl.createDateTimeFormat(undefined, {dateStyle: 'short', timeStyle: 'short'}).format(Date.now())

But that would result in "9/3/17, 9:08 AM, PINGNAME" for en-US (double comma).

We need to show seconds, which would turn into a "10:40:00 AM" for en-US, with the header showing something as long as "10/10/2017 10:00:00 AM", and that requires to reduce the font size even further (15px, maybe 14px).

> nit: forcing two dates to produce a string representation is very inefficient.
> 
> On top of that, you're possibly doing this twice here.
> 
> It would be better to get a representation of the date stripped of time:
> ```
> const dateWithoutTime = new Date(pingDate).setHours(0, 0, 0, 0);
> const today = new Date();
> today.setHours(0, 0, 0, 0);
> const yesterday = new Date(today);
> yesterday.setDate(today.getDate() - 1);
> 
> if (dateWithoutTime === today) {
>   ...
> } else if (dateWithoutTime === yesterday) {
> }
> ```

As the rest of the code, I tried to move away as little as possible from the original (e.g. toLocaleString), but I think it makes sense to do it, and clean up things. Thanks.
Comment on attachment 8903889 [details]
Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code

Resetting flag.

I kept the 24 hours format for now, in case it's up to a Telemetry peer to decide if the format could change to something else.

I also find JS behavior with dates extremely confusing (the code fragment you suggested wouldn't work).

const d = new Date();
date.setHours(0, 0, 0, 0);
print(d);

/*
Sun Sep 03 2017 00:00:00 GMT+0200 (CEST)
*/

but

const d = new Date().setHours(0, 0, 0, 0);
print(d);

/*
1504389600000
*/
Attachment #8903889 - Flags: feedback?(gandalf)
> the code fragment you suggested wouldn't work

can you please be more specific?

> but

That's an artifact of how the API work. There's new Date API for JS in works - https://github.com/tc39/proposal-temporal - hope it'll be more consistent.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #10)
> > the code fragment you suggested wouldn't work
> 
> can you please be more specific?

```
const dateWithoutTime = new Date(pingDate).setHours(0, 0, 0, 0);
const today = new Date();
today.setHours(0, 0, 0, 0);
const yesterday = new Date(today);
yesterday.setDate(today.getDate() - 1);

if (dateWithoutTime === today) {
  ...
} else if (dateWithoutTime === yesterday) {
}
```

dateWithoutTime is a timestamp (for the artifact above), today and yesterday are not

dateWithoutTime != today but dateWithoutTime === today.getTime()
Comment on attachment 8903889 [details]
Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code

https://reviewboard.mozilla.org/r/175668/#review180762

::: toolkit/content/aboutTelemetry.js:439
(Diff revision 3)
> +    };
> +
>      for (let p of this._archivedPings) {
>        pingTypes.add(p.type);
> -      let date = new Date(p.timestampCreated);
> -      let datetext = date.toLocaleDateString() + " " + shortTimeString(date);
> +      const pingDate = new Date(p.timestampCreated);
> +      const pingDateWithoutTime = new Date(pingDate).setHours(0, 0, 0, 0);

I don't think you need this variable, nor `timeValue` or `dateValue`. You can compare two date objects.

::: toolkit/content/aboutTelemetry.js:441
(Diff revision 3)
>      for (let p of this._archivedPings) {
>        pingTypes.add(p.type);
> -      let date = new Date(p.timestampCreated);
> -      let datetext = date.toLocaleDateString() + " " + shortTimeString(date);
> -      let text = datetext + ", " + p.type;
> +      const pingDate = new Date(p.timestampCreated);
> +      const pingDateWithoutTime = new Date(pingDate).setHours(0, 0, 0, 0);
> +      const dateText = Services.intl.createDateTimeFormat(undefined, dateFormat).format(pingDate);
> +      const timeText = Services.intl.createDateTimeFormat(undefined, timeFormat).format(pingDate);

You seem to decide to stick to manual list of tokens instead of `dateStyle` and `timeStyle` which I suggested in order to conform to the Firefox UI Intl practice of respecting user's Regional Preferneces in their OS.

What's the reason for that?

::: toolkit/content/aboutTelemetry.js:442
(Diff revision 3)
>        pingTypes.add(p.type);
> -      let date = new Date(p.timestampCreated);
> -      let datetext = date.toLocaleDateString() + " " + shortTimeString(date);
> -      let text = datetext + ", " + p.type;
> +      const pingDate = new Date(p.timestampCreated);
> +      const pingDateWithoutTime = new Date(pingDate).setHours(0, 0, 0, 0);
> +      const dateText = Services.intl.createDateTimeFormat(undefined, dateFormat).format(pingDate);
> +      const timeText = Services.intl.createDateTimeFormat(undefined, timeFormat).format(pingDate);
> +      let datetimeText = `${dateText} ${timeText}`;

So, you're still hardcoding LTR and space separator. Any reason for that?

I provided you a snippet that internationalized date and time together.
> dateWithoutTime is a timestamp (for the artifact above), today and yesterday are not

Oh, then I'd recommend:

```
const dateWithoutTime = new Date(pingDate);
dateWithoutTime.setHours(0, 0, 0, 0);
```
Comment on attachment 8903889 [details]
Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code

https://reviewboard.mozilla.org/r/175668/#review180756

::: toolkit/content/aboutTelemetry.js:441
(Diff revision 3)
>      for (let p of this._archivedPings) {
>        pingTypes.add(p.type);
> -      let date = new Date(p.timestampCreated);
> -      let datetext = date.toLocaleDateString() + " " + shortTimeString(date);
> -      let text = datetext + ", " + p.type;
> +      const pingDate = new Date(p.timestampCreated);
> +      const pingDateWithoutTime = new Date(pingDate).setHours(0, 0, 0, 0);
> +      const dateText = Services.intl.createDateTimeFormat(undefined, dateFormat).format(pingDate);
> +      const timeText = Services.intl.createDateTimeFormat(undefined, timeFormat).format(pingDate);

As explained, I don't think that's my call to change the date and time format for now, it should be up to a Telemetry peer. 

dateStyle: short has 2 numbers for the year, dateTime: long has AM/PM.

At this point, it's probably clearer to NI and ask instead of continuining the back and forth between me and you ;-)

::: toolkit/content/aboutTelemetry.js:442
(Diff revision 3)
>        pingTypes.add(p.type);
> -      let date = new Date(p.timestampCreated);
> -      let datetext = date.toLocaleDateString() + " " + shortTimeString(date);
> -      let text = datetext + ", " + p.type;
> +      const pingDate = new Date(p.timestampCreated);
> +      const pingDateWithoutTime = new Date(pingDate).setHours(0, 0, 0, 0);
> +      const dateText = Services.intl.createDateTimeFormat(undefined, dateFormat).format(pingDate);
> +      const timeText = Services.intl.createDateTimeFormat(undefined, timeFormat).format(pingDate);
> +      let datetimeText = `${dateText} ${timeText}`;

Explained in https://bugzilla.mozilla.org/show_bug.cgi?id=1396211#c6
Chris, would it be OK to change the date format in the ping name for en-US from 'dd/mm/yyyy hh:mm:ss' to 'dd/mm/yy h:mm:ss AM/PM'?

That would get rid of the hardcoded space, and make the date/time properly internationalized.
Flags: needinfo?(chutten)
Oh, sorry for my review questions. I somehow skipped your responses. My responses below:

> Because I'm trying to stay as close as possible to the current behavior, in case I'm not sure it would be my call to change the current time format. 

It seems to me that the author of the code just tried to get the date to display correctly, I doubt there's any deeper meaning behind enforcing hour12, but you can NI them here :)

> But that would result in "9/3/17, 9:08 AM, PINGNAME" for en-US (double comma).

Yeah, that's what you get for trying to use intl data with hardcoded semantics ;)

I guess the double comma should be ok for en-US.

> We need to show seconds, which would turn into a "10:40:00 AM" for en-US, with the header showing something as long as "10/10/2017 10:00:00 AM", and that requires to reduce the font size even further (15px, maybe 14px).

that is the format that users expect in the locale. The only thing I can say is that if you try to ignore that, you're running into the risk of some other language using it anyway.

If the UI is not ready for longer text, then it may not be ready for internationalized datetime formatting :(

My general recommendation would be to go for dateStyle/timeStyle, use a single mozIntl with both of them, be ok with double comma, and make space for longer date/time string if you need to handle seconds.

But that should be the call of the author of the code, so I'd suggest NI'ing them :)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #13)
> > dateWithoutTime is a timestamp (for the artifact above), today and yesterday are not
> 
> Oh, then I'd recommend:
> 
> ```
> const dateWithoutTime = new Date(pingDate);
> dateWithoutTime.setHours(0, 0, 0, 0);
> ```

I'm ready to throw a show at JavaScript…

Looks like JS is comparing objects when you try to compare two dates

let d1 = new Date('2017', '8', '3', '0', '0', '0', '0');
let d2 = new Date('2017', '8', '3', '0', '0', '0', '0');

print(d1 == d2); // false
print(d1 === d2); // false
print(d1.valueOf() === d2.valueOf()); //true
print(d1.getTime() === d2.getTime()); //true
> print(d1.getTime() === d2.getTime()); //true

Use that.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #18)
> > print(d1.getTime() === d2.getTime()); //true
> 
> Use that.

That's what the current code is doing, just storing it into a variable to avoid calling .getTime() multiple times. 

Wouldn't it be less efficient to use .getTime directly in the if conditions?
> That's what the current code is doing, just storing it into a variable to avoid calling .getTime() multiple times. 
> Wouldn't it be less efficient to use .getTime directly in the if conditions?

No, it's all good. I meant, use it instead of formatting it to a string to compare :)
Flags: needinfo?(chutten)
Moving NI in a comment where it's clearer

If we use properly internationalized dates:
* The date appearing in the selector and header would change from 'dd/mm/yyyy hh:mm:ss' to 'dd/mm/yy h:mm:ss AM/PM'.
* The text displayed in the Home panel will also change from 'dd/mm/yyyy hh:mm:ss, ping name' to 'dd/mm/yy, h:mm:ss AM/PM, ping name' (with a double comma).

Are these changes OK?

@zibi
Isn't something like this supposed to work (according to bug 1329904)?

Services.intl.createDateTimeFormat(undefined, {dateStyle: 'short', year: 'numeric'}).format(Date.now())

It still returns me 03/09/17, like "year" wasn't specified.
Flags: needinfo?(chutten)
Attached image test.png
Highlighted changes using en-US and Services.intl.createDateTimeFormat(undefined, {dateStyle: 'short', timeStyle: 'medium'}).format(pingDate) for the datetime.

The CSS changes (incremented margin top, reduced font size) would not really be necessary, but I think they help (current layout is crowded in that area).
> Isn't something like this supposed to work (according to bug 1329904)?
> Services.intl.createDateTimeFormat(undefined, {dateStyle: 'short', year: 'numeric'}).format(Date.now())
> It still returns me 03/09/17, like "year" wasn't specified.

It's not supposed to work. When you select "dateStyle" or "timeStyle" you're basically telling the API to go look into OS Regional Preferences for the pattern that matches date-style or time-style you selected.

We do this and use the result pattern which may not even have year (if the user customized it to not have the year token).

Thus when you select dateStyle/timeStyle we ignore the token options as per http://searchfox.org/mozilla-central/source/js/src/builtin/Intl.js#2508
(In reply to Francesco Lodolo [:flod] from comment #22)
> Moving NI in a comment where it's clearer
> 
> If we use properly internationalized dates:
> * The date appearing in the selector and header would change from
> 'dd/mm/yyyy hh:mm:ss' to 'dd/mm/yy h:mm:ss AM/PM'.
> * The text displayed in the Home panel will also change from 'dd/mm/yyyy
> hh:mm:ss, ping name' to 'dd/mm/yy, h:mm:ss AM/PM, ping name' (with a double
> comma).
> 
> Are these changes OK?

If it is most convenient, then yes. I have no problems with double-comma or using user-specified date formats if this is what is recommended by l10n.
Flags: needinfo?(chutten)
Comment on attachment 8903889 [details]
Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code

This should be in better shape for feedback now.
Attachment #8903889 - Flags: feedback?(gandalf)
Comment on attachment 8903889 [details]
Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code

looks good to me! Thanks Flod! :)
Attachment #8903889 - Flags: feedback?(gandalf) → feedback+
Assignee: nobody → francesco.lodolo
Comment on attachment 8903889 [details]
Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code

https://reviewboard.mozilla.org/r/175668/#review181452

Just a small nit from me.

::: toolkit/content/aboutTelemetry.css:97
(Diff revision 6)
>  
>  #controls {
>    display: flex;
> -  margin-top: 4px;
> +  margin-top: 6px;
>    justify-content: space-between;
> +  font-size: 16px;

16px is not in the Photon Design System typography recommendation: http://design.firefox.com/photon/visual/typography.html
Attachment #8903889 - Flags: review?(chutten) → review-
(In reply to Chris H-C :chutten from comment #30)
> 16px is not in the Photon Design System typography recommendation:
> http://design.firefox.com/photon/visual/typography.html

The current header is using 18px and weight normal(.heading). Would use 17px and weight 600 for both work?

Looks like font sizes are all over the place at the moment: 15.95px (1.45rem) in the sidebar, 15px in tables (the only one actually in the Photon guidelines).
(In reply to Francesco Lodolo [:flod] from comment #31)
> (In reply to Chris H-C :chutten from comment #30)
> > 16px is not in the Photon Design System typography recommendation:
> > http://design.firefox.com/photon/visual/typography.html
> 
> The current header is using 18px and weight normal(.heading). Would use 17px
> and weight 600 for both work?
> 
> Looks like font sizes are all over the place at the moment: 15.95px
> (1.45rem) in the sidebar, 15px in tables (the only one actually in the
> Photon guidelines).

There are some bugs around exact sizing at the moment (most notably bug 1392532 on Linux), so not all of this may be deliberate...

At any rate, I'm happy bringing things closer to the ideal. Even better is sourcing some classes directly from Common.css we could use instead. But 17px/600 ought to do the trick.
Comment on attachment 8903889 [details]
Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code

https://reviewboard.mozilla.org/r/175668/#review181462

::: toolkit/content/aboutTelemetry.css:97
(Diff revision 6)
>  
>  #controls {
>    display: flex;
> -  margin-top: 4px;
> +  margin-top: 6px;
>    justify-content: space-between;
> +  font-size: 16px;

Setting .heading to 17px/600, and removing the increased margin since the font is smaller
Comment on attachment 8903889 [details]
Bug 1396211 - Use mozIntl for dates in aboutTelemetry.js, clean up unused code

https://reviewboard.mozilla.org/r/175668/#review181464

LGTM
Attachment #8903889 - Flags: review?(chutten) → review+
Pushed by francesco.lodolo@mozillaitalia.org:
https://hg.mozilla.org/integration/autoland/rev/78cce071b69e
Use mozIntl for dates in aboutTelemetry.js, clean up unused code r=chutten
Backed out for eslint failure in toolkit/content/aboutTelemetry.js: Strings must use doublequote:

https://hg.mozilla.org/integration/autoland/rev/0f5857f79e6f69ed8f25f1f7cd27ef28d7a2e541

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=78cce071b69e26eb55df4da1f66409b3a2226d3b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=128640004&repo=autoland

TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/content/aboutTelemetry.js:425:22 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/content/aboutTelemetry.js:426:22 | Strings must use doublequote. (quotes)
Flags: needinfo?(francesco.lodolo)
Sorry about that, I never work with JS files and forgot about eslint :-\

Updating the patch after running tests locally

$ ./mach lint -l eslint toolkit/content/aboutTelemetry.js
✖ 0 problems (0 errors, 0 warnings)
Flags: needinfo?(francesco.lodolo)
Pushed by francesco.lodolo@mozillaitalia.org:
https://hg.mozilla.org/integration/autoland/rev/6e875574d0cb
Use mozIntl for dates in aboutTelemetry.js, clean up unused code r=chutten
https://hg.mozilla.org/mozilla-central/rev/6e875574d0cb
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.