i18n: localize units in report formatter #13830

connorjclark · 2022-04-06T23:04:47Z

All number time formatters now use Intl.NumberFormatter with the unit field
So does formatBytes, but the larger byte formatters could not use Intl units because binary units are currently not supported
formatDuration is now localized
formatNumber now does not drop significant digits (ex: formatNumber(10, 0.1) => 10.0, but before it was 10. This matches the behavior of how granularity was implemented for _byteFormatterForGranularity, and better conveys the precision/rounding to the user (should they even notice)
moved all the granularity stuff into a shared function

Even with this change, you'll notice that many time units are still not localized:

Only the opportunity audits display values use our report/renderer/i18n.js formatter

lighthouse/report/renderer/performance-category-renderer.js

Line 79 in 0b88142

    
           displayEl.textContent = Util.i18n.formatSeconds(details.overallSavingsMs, 0.01);

The other audit's display value field is used directly, and is hardcoding the unit:

lighthouse/lighthouse-core/audits/bootup-time.js

Line 212 in 0b88142

str_(i18n.UIStrings.seconds, {timeInMs: totalBootupTime}) : '',

lighthouse/lighthouse-core/lib/i18n/i18n.js

Line 25 in 0b88142

seconds: '{timeInMs, number, seconds}\xa0s',

We have the numeric value and the unit in the audit results, so we should format the value in the report directly. We can also look into using our formatter directly in the audit result so that displayValue in JSON is still localized and with the right unit. I suspect it may require a refactor since this formatter module is in the report, so this will be a follow up PR.

connorjclark · 2022-04-06T23:10:42Z

One option for formatting KiB/MiB (the binary prefixes) would be to use kilobyte as the unit but for english manually format as KiB/MiB. That way, we stick with our intention to use the less ambiguous unit in english (#10870) but for other languages at least show their equivalent of KB/MB (and for all we know, the same confusion does not exist in their locale)

connorjclark · 2022-04-06T23:18:17Z

report/renderer/i18n.js

+    number = Math.round(number / granularity) * granularity;
+
+    // Avoid displaying a negative value that rounds to zero as "0".
+    if (Object.is(number, -0)) number = 0;


I could have done if (number === 0) number = 0 but it was just too much.

What if JS added ==== so -0 ==== 0 would be false 🤪

IEEE-754 requires that they compare as equal 🤓

connorjclark · 2022-04-06T23:32:05Z

report/renderer/i18n.js

+
+    return new Intl.NumberFormat(this._locale, opts).format(number).replace(' ', NBSP2);
+  }
+
  /**
   * Format number.
   * @param {number} number
   * @param {number=} granularity Number of decimal places to include. Defaults to 0.1.
   * @return {string}
   */
  formatNumber(number, granularity = 0.1) {


perhaps this default should be 1? it would match the previous behavior, such that integers are formatted without a fractional component

instead I made a new function formatInteger

yeah, it's reasonable to show the extra fractional digit when granularity is 0.1 (which is what #11489 did for bytes), and maybe we don't really want 0.1 as the default based on how we usually don't do anything about it, but that's annoying to audit and formatInteger is a great solution :)

connorjclark · 2022-04-07T00:29:19Z

We have the numeric value and the unit in the audit results, so we should format the value in the report directly. We can also look into using our formatter directly in the audit result so that displayValue in JSON is still localized and with the right unit. I suspect it may require a refactor since this formatter module is in the report, so this will be a follow up PR.

Perhaps a better solution could be to upgrade our icu package and use the new skeleton format in our icu message

see:

https://messageformat.github.io/messageformat/guide/#number

https://github.com/unicode-org/icu/blob/main/docs/userguide/format_parse/numbers/skeletons.md

lighthouse-core/audits/bootup-time.js

adamraine · 2022-04-07T19:35:03Z

report/renderer/i18n.js

+    number = Math.round(number / granularity) * granularity;
+
+    // Avoid displaying a negative value that rounds to zero as "0".
+    if (Object.is(number, -0)) number = 0;


What if JS added ==== so -0 ==== 0 would be false 🤪

adamraine · 2022-04-07T19:50:14Z

treemap/app/src/util.js

@@ -147,8 +147,8 @@ class TreemapUtil {
   * @param {string} unit
   */
  static format(value, unit) {


Do we use this function anywhere?

lol i'll delete it

brendankenny · 2022-04-07T20:07:13Z

lighthouse-core/util-commonjs.js

-          `${Util.i18n.formatNumber(throttling.downloadThroughputKbps)}${NBSP}Kbps down, ` +
-          `${Util.i18n.formatNumber(throttling.uploadThroughputKbps)}${NBSP}Kbps up (DevTools)`;
+        networkThrottling = `${Util.i18n.formatMilliseconds(requestLatencyMs, 1)} HTTP RTT, ` +
+          `${Util.i18n.formatInteger(throttling.downloadThroughputKbps)}${NBSP}Kbps down, ` +


can do unit: 'kilobit-per-second' for these since it's Kbps (not...Kibps?). It'll be kb/s instead of Kbps though

sounds better to me!

connorjclark · 2022-04-07T23:07:05Z

report/renderer/i18n.js

+   * @return {string}
+   */
+  _formatNumberWithGranularity(number, granularity, opts = {}) {
+    opts = {...opts};


yeah i had my cs prof yelling in my head "don't mutate input parameters!" so i copied it

brendankenny · 2022-04-07T20:15:24Z

report/renderer/i18n.js

+  _formatNumberWithGranularity(number, granularity, opts = {}) {
+    opts = {...opts};
+    const log10 = -Math.log10(granularity);
+    if (!Number.isFinite(log10) || (granularity > 1 && Math.floor(log10) !== log10)) {


I think if (!Number.isFinite(log10) || !Number.isInteger(log10)) { is sufficient if we just want powers of 10

We have a test that uses 0.5 as a value here:

lighthouse/report/test/renderer/i18n-test.js

Line 63 in b4a8e23

granularity = 0.5;

and given we pass granularity through a details object throwing here may even be a breaking change... but good thing we are in pre-10.0 mode :)

hmmm throwing in report code is probably bad anyhow. wish we had a DCHECK (cc @exterkamp lol)

We have a test that uses 0.5 as a value here:

ha, well that looks like something that was supported so it was nice to add a test, rather that something we've ever used or wanted to use, but if we want to keep it, if (!Number.isFinite(log10) || (granularity > 1 && !Number.isInteger(log10))) is slightly easier to understand

for some reason i had no idea isInteger existed

brendankenny · 2022-04-07T20:18:07Z

report/renderer/i18n.js

+
+    number = Math.round(number / granularity) * granularity;
+
+    // Avoid displaying a negative value that rounds to zero as "0".


signDisplay: 'negative' will finally let this be handled how signDisplay should have defaulted in the first place :) Firefox only for now though.

brendankenny · 2022-04-07T20:19:55Z

report/renderer/i18n.js

+    number = Math.round(number / granularity) * granularity;
+
+    // Avoid displaying a negative value that rounds to zero as "0".
+    if (Object.is(number, -0)) number = 0;


IEEE-754 requires that they compare as equal 🤓

brendankenny · 2022-04-07T20:22:11Z

report/renderer/i18n.js

+    // Avoid displaying a negative value that rounds to zero as "0".
+    if (Object.is(number, -0)) number = 0;
+
+    return new Intl.NumberFormat(this._locale, opts).format(number).replace(' ', NBSP2);


It seems like it should be fine to do the replace thing, though it feels dirty and maybe there's somehow an issue with rtl or something? 🤷

Do we still need it? I assume we do, but it would be cool if our various layout initiatives over the years made it unnecessary to keep them from breaking across lines

Interestingly, for new Intl.NumberFormat('fr', {style: 'unit', unit: 'second', unitDisplay: 'long'}).format(5) ('5 secondes'), the second character is NBSP2, but that doesn't appear true for most (all?) other languages.

new Intl.NumberFormat('fr', {style: 'unit', unit: 'second', unitDisplay: 'short'}) has \u202f (narrow no-break space) as its second character.

Also, languages disagree on spacing of units :/
new Intl.NumberFormat('en-US', {style: 'unit', unit: 'byte', unitDisplay: 'narrow'}).format(5) -> '5B'
new Intl.NumberFormat('en-US', {style: 'unit', unit: 'byte', unitDisplay: 'narrow'}).format(5) -> '5 B'

omg this is infuriatingly inconsistent. no way this is due to locale differences, these control characters are formatting and what locale would expect numbers to be disassociated from their unit by newlines?? this has to be a mistake in the localization library...

your fr example is odd because the CLDR data has 32 space for long but narrow non-break space for short, based on Intl output I'd expect the former to show NBSP2. (narrow has no whitespace)

One can sort of see reason for there being a non-breaking space between the number and the short string but not for the longer one, as without that number the shorter strings being further from their number gives them far less context.

(your last example is the same two lines of code btw. i'd expect narrow to never include any whitespace. I see de does though... I think for some languages CLDR doesn't bother defining narrows strings so it defaults to short)

One can sort of see reason for there being a non-breaking space between the number and the short string but not for the longer one, as without that number the shorter strings being further from their number gives them far less context.

totally, but why isn't it the same across languages?? Or at least, why wouldn't english (or german or spanish or...) do it as well?

(your last example is the same two lines of code btw. i'd expect narrow to never include any whitespace)

oops, I meant de for the second one:

new Intl.NumberFormat('en-US', {style: 'unit', unit: 'byte', unitDisplay: 'narrow'}).format(5) -> '5B'
new Intl.NumberFormat('de', {style: 'unit', unit: 'byte', unitDisplay: 'narrow'}).format(5) -> '5 B'

Do we still need it? I assume we do, but it would be cool if our various layout initiatives over the years made it unnecessary to keep them from breaking across lines

hmm....

brendankenny · 2022-04-07T20:25:25Z

report/renderer/i18n.js

+
+    return new Intl.NumberFormat(this._locale, opts).format(number).replace(' ', NBSP2);
+  }
+
  /**
   * Format number.
   * @param {number} number
   * @param {number=} granularity Number of decimal places to include. Defaults to 0.1.
   * @return {string}
   */
  formatNumber(number, granularity = 0.1) {


yeah, it's reasonable to show the extra fractional digit when granularity is 0.1 (which is what #11489 did for bytes), and maybe we don't really want 0.1 as the default based on how we usually don't do anything about it, but that's annoying to audit and formatInteger is a great solution :)

brendankenny · 2022-04-07T20:46:40Z

report/renderer/i18n.js

+        unit,
+        unitDisplay: 'narrow',
+      });
+      const label = unitFormatter.formatToParts(0)


this seems like it could be problematic to use 0 here for languages with different (abbreviated) unit names for plurals? Might not be a problem for any of our languages, but this also seems to be a very roundabout way vs concatenating calls to _formatNumberWithGranularity. What's the motivation? May be worth a comment

good point, better to call that function directly

brendankenny · 2022-04-07T20:47:10Z

report/renderer/util.js

@@ -431,9 +431,9 @@ class Util {
      case 'devtools': {
        const {cpuSlowdownMultiplier, requestLatencyMs} = throttling;
        cpuThrottling = `${Util.i18n.formatNumber(cpuSlowdownMultiplier)}x slowdown (DevTools)`;


probably formatInteger on the cpu throttling as well?

While we don't ever assign this value to non-integer numbers, others might via the CLI.

fair point. 2.0x looks kind of dumb, though :)

hold up why don't we just do ${cpuSlowdownMultiplier}x

this entire string gets a F for i18n atm anyway so we still need to do this correctly with a ICU message

oh right, arabic

brendankenny · 2022-04-07T20:56:55Z

We have the numeric value and the unit in the audit results, so we should format the value in the report directly. We can also look into using our formatter directly in the audit result so that displayValue in JSON is still localized and with the right unit. I suspect it may require a refactor since this formatter module is in the report, so this will be a follow up PR.

this may be worth a more in-depth conversation with our TC contact, because this is running automated Intl.NumberFormat translations up against human-translator-provided translations and wanting them to be consistent. It's possible they've judged '{timeInMs, number, milliseconds}\xa0ms' staying as '{PH} ms' as the better translation for their locale, even if Intl.NumberFormat might give something else.

Maybe it's better, but OTOH maybe the report formatter should be using i18n.UIStrings.ms instead of Intl.NumberFormat?

connorjclark · 2022-04-07T21:07:37Z

Since writing that "next PR steps" comments my opinion has changed to basically this:

maybe the report formatter should be using i18n.UIStrings.ms instead of Intl.NumberFormat?

TBH, I have more trust in these CLDR strings being translated correctly (and by default, always consistent!) than our human translators process, so I'd like to see full reliance on the ICU pipeline for things like unit localization when/if possible, and let the human translators focus on the really hard work of translating the rest of the string.

For that ms string specifically we'd be sending strings with no translation work, which may be kinda funny to them:

ms: '{timeInMs, number, ::unit/.......}'

(tbh I haven't grokked how to use this yet, but the docs suggest we can specify unit and width so should be good)

this may be worth a more in-depth conversation with our TC contact

Just for the consistency angle or also for "can we use a different translation process for units"?

connorjclark · 2022-04-07T21:29:20Z

just looking at a few locales on how our translators might differ in unit localization compared to CLDR

de

our string:

"{timeInMs, number, milliseconds} ms"

Intl.NumberFormat('de', {style: 'unit', unit: 'millisecond', unitDisplay: 'short'}).format(1);
> 1 ms
Intl.NumberFormat('de', {style: 'unit', unit: 'millisecond', unitDisplay: 'long'}).format(1);
> 1 Millisekunden

all good... CLDR specifies the short string in de to inherit from the english string

hu
(Hungarian)

our string:

"{timeInMs, number, milliseconds} ms"

Intl.NumberFormat('hu', {style: 'unit', unit: 'millisecond', unitDisplay: 'short'}).format(1);
> 1 ms
Intl.NumberFormat('hu', {style: 'unit', unit: 'millisecond', unitDisplay: 'long'}).format(1);
> 1 ezredmásodperc

Also good

ko

our string:

"{timeInMs, number, milliseconds} 밀리초"

Intl.NumberFormat('ko', {style: 'unit', unit: 'millisecond', unitDisplay: 'short'}).format(12);
> 12ms
Intl.NumberFormat('ko', {style: 'unit', unit: 'millisecond', unitDisplay: 'long'}).format(12);
> 12밀리초

according to CLDR the preferred value here would have been ms (if we really wished for a short display here)

brendankenny · 2022-04-07T21:30:42Z

Just for the consistency angle or also for "can we use a different translation process for units"?

basically what's the best practice here, are the automatic translations good enough, etc. Full consistency would also include things like 'Third-party code blocked the main thread for {timeInMs, number, milliseconds} ms' dropping the ms and letting the number formatter add it. Does interpolating the already unitized number in there work?

connorjclark · 2022-04-07T21:35:41Z

hmm I'm not sure "interpolating the already unitized number in there" is an accurate representation of what the ICU message formatter does. we do some slight massaging of the values we pass to the formatter via _preformatValues but the formatter is in charge of stringing (heh) all these parts together, and it's gonna do that in a locale-aware manner, including its addition of the unit (should we begin to use this feature)

connorjclark · 2022-04-07T21:47:23Z

One issue with our human translator process is we don't specify in our translators comments anything about "how much space should this unit take up / what unit display width is preferred", relying on the assumption that if we state ms we want a short abbreviated term, when possible. This ambiguity may result in different translators giving us different terms. We could fix all our translator strings to include this, but that doesn't leave out the possibility of some being overlooked (by us or them)

brendankenny · 2022-04-07T21:52:43Z

according to CLDR the preferred value here would have been ms (if we really wished for a short display here)

is that better? I don't know :) That's why I was suggesting we reach out.

One issue with our human translator process is we don't specify in our translators comments anything about "how much space should this unit take up / what unit display width is preferred", relying on the assumption that if we state ms we want a short abbreviated term, when possible. This ambiguity may result in different translators giving us different terms. We could fix all our translator strings to include this, but that doesn't leave out the possibility of some being overlooked (by us or them)

this is definitely our fault, this is an explicit request for writing translation descriptions when the length is important. We do have it in a few places (e.g. "Ideally fits within a ~40 character limit"), but we could probably have some automated annotation for checking short strings across locales automatically if we really wanted to keep that in check.

brendankenny · 2022-04-07T21:53:02Z

is this conversation off topic and really about the next PR?

connorjclark · 2022-04-07T21:59:10Z

yeah I think this PR can be merged (after I address feedback) as it's a net-positive (localizing text that wasn't before) but the issue on consistency is worth discussing somewhere

Co-authored-by: Adam Raine <6752989+adamraine@users.noreply.github.com>

connorjclark · 2022-04-07T23:37:48Z

report/renderer/i18n.js

@@ -169,6 +189,10 @@ export class I18n {
   * @return {string}
   */
  formatDuration(timeInMilliseconds) {


just realized the comment for this function was wrong

1d 2h 13m 52s

we had NBSP all up in there...

adamraine

CI failure looks like a simple fix, so LGTM

connorjclark added 2 commits April 6, 2022 15:15

first pass, localize units

504e62a

localize formatDuration

0b88142

connorjclark requested a review from a team as a code owner April 6, 2022 23:04

connorjclark requested review from adamraine and removed request for a team April 6, 2022 23:04

devtools-bot assigned adamraine Apr 6, 2022

devtools-bot added the waiting4reviewer label Apr 6, 2022

connorjclark commented Apr 6, 2022

View reviewed changes

hmm

7dd7d01

vercel bot deployed to Preview April 6, 2022 23:32 View deployment

connorjclark commented Apr 6, 2022

View reviewed changes

update

08bc98b

vercel bot deployed to Preview April 7, 2022 00:15 View deployment

adamraine reviewed Apr 7, 2022

View reviewed changes

brendankenny reviewed Apr 7, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

Update lighthouse-core/audits/bootup-time.js

b4a8e23

Co-authored-by: Adam Raine <6752989+adamraine@users.noreply.github.com>

vercel bot deployed to Preview April 7, 2022 23:06 View deployment

connorjclark commented Apr 7, 2022

View reviewed changes

connorjclark added 2 commits April 7, 2022 16:45

pr

61c23f5

kb/s

6232d93

vercel bot deployed to Preview April 7, 2022 23:51 View deployment

connorjclark mentioned this pull request Apr 8, 2022

i18n: upgrade to latest icu formatter #13834

Merged

update

9e82306

vercel bot deployed to Preview April 8, 2022 19:17 View deployment

fix

325d84b

vercel bot deployed to Preview April 8, 2022 19:19 View deployment

adamraine approved these changes Apr 8, 2022

View reviewed changes

fix

0d3cbc6

vercel bot deployed to Preview April 8, 2022 19:35 View deployment

tweak

350a33a

vercel bot deployed to Preview April 8, 2022 19:52 View deployment

connorjclark added 2 commits April 8, 2022 12:52

fix

a9c7388

fix

7db81d2

vercel bot deployed to Preview April 8, 2022 19:54 View deployment

Merge remote-tracking branch 'origin/master' into i18n-formatter-units

fc20aad

vercel bot deployed to Preview April 19, 2022 18:24 View deployment

connorjclark merged commit 5846954 into master Apr 19, 2022

connorjclark deleted the i18n-formatter-units branch April 19, 2022 20:58

connorjclark mentioned this pull request May 23, 2022

i18n: use locale for all numeric formatting in the report #10786

Open

brendankenny mentioned this pull request Aug 29, 2022

report: line breaks in the middle of the 'sec' unit in opportunities #14326

Closed


		number = Math.round(number / granularity) * granularity;

		// Avoid displaying a negative value that rounds to zero as "0".

i18n: localize units in report formatter #13830

i18n: localize units in report formatter #13830

Conversation

connorjclark commented Apr 6, 2022 • edited Loading

connorjclark commented Apr 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Apr 7, 2022 • edited Loading

connorjclark commented Apr 7, 2022 • edited Loading

connorjclark commented Apr 7, 2022 • edited Loading

brendankenny commented Apr 7, 2022

This comment was marked as outdated.

connorjclark commented Apr 7, 2022 • edited Loading

This comment was marked as outdated.

connorjclark commented Apr 7, 2022 • edited Loading

brendankenny commented Apr 7, 2022

brendankenny commented Apr 7, 2022

connorjclark commented Apr 7, 2022 • edited Loading

connorjclark Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

adamraine left a comment

Choose a reason for hiding this comment

connorjclark commented Apr 6, 2022 •

edited

Loading

connorjclark Apr 6, 2022 •

edited

Loading

connorjclark commented Apr 7, 2022 •

edited

Loading

connorjclark Apr 7, 2022 •

edited

Loading

brendankenny commented Apr 7, 2022 •

edited

Loading

connorjclark commented Apr 7, 2022 •

edited

Loading

connorjclark commented Apr 7, 2022 •

edited

Loading

connorjclark commented Apr 7, 2022 •

edited

Loading

connorjclark commented Apr 7, 2022 •

edited

Loading

connorjclark commented Apr 7, 2022 •

edited

Loading

connorjclark Apr 7, 2022 •

edited

Loading