Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

i18n: localize units in report formatter #13830

Merged
merged 14 commits into from
Apr 19, 2022
2 changes: 1 addition & 1 deletion flow-report/src/summary/category.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ const SummaryTooltip: FunctionComponent<{
{
!displayAsFraction && category.score !== null && <>
<span> · </span>
<span>{i18n.formatNumber(category.score * 100)}</span>
<span>{i18n.formatInteger(category.score * 100)}</span>
</>
}
</div>
Expand Down
20 changes: 13 additions & 7 deletions lighthouse-core/util-commonjs.js
Original file line number Diff line number Diff line change
Expand Up @@ -433,10 +433,13 @@ class Util {
break;
case 'devtools': {
const {cpuSlowdownMultiplier, requestLatencyMs} = throttling;
cpuThrottling = `${Util.i18n.formatNumber(cpuSlowdownMultiplier)}x slowdown (DevTools)`;
networkThrottling = `${Util.i18n.formatNumber(requestLatencyMs)}${NBSP}ms HTTP RTT, ` +
`${Util.i18n.formatNumber(throttling.downloadThroughputKbps)}${NBSP}Kbps down, ` +
`${Util.i18n.formatNumber(throttling.uploadThroughputKbps)}${NBSP}Kbps up (DevTools)`;
// TODO: better api in i18n formatter such that this isn't needed.
const cpuGranularity = Number.isInteger(cpuSlowdownMultiplier) ? 1 : 0.1;
// eslint-disable-next-line max-len
cpuThrottling = `${Util.i18n.formatNumber(cpuSlowdownMultiplier, cpuGranularity)}x slowdown (DevTools)`;
networkThrottling = `${Util.i18n.formatMilliseconds(requestLatencyMs, 1)} HTTP RTT, ` +
`${Util.i18n.formatKbps(throttling.downloadThroughputKbps)} down, ` +
`${Util.i18n.formatKbps(throttling.uploadThroughputKbps)} up (DevTools)`;

const isSlow4G = () => {
return requestLatencyMs === 150 * 3.75 &&
Expand All @@ -448,9 +451,12 @@ class Util {
}
case 'simulate': {
const {cpuSlowdownMultiplier, rttMs, throughputKbps} = throttling;
cpuThrottling = `${Util.i18n.formatNumber(cpuSlowdownMultiplier)}x slowdown (Simulated)`;
networkThrottling = `${Util.i18n.formatNumber(rttMs)}${NBSP}ms TCP RTT, ` +
`${Util.i18n.formatNumber(throughputKbps)}${NBSP}Kbps throughput (Simulated)`;
// TODO: better api in i18n formatter such that this isn't needed.
const cpuGranularity = Number.isInteger(cpuSlowdownMultiplier) ? 1 : 0.1;
// eslint-disable-next-line max-len
cpuThrottling = `${Util.i18n.formatNumber(cpuSlowdownMultiplier, cpuGranularity)}x slowdown (Simulated)`;
networkThrottling = `${Util.i18n.formatMilliseconds(rttMs)} TCP RTT, ` +
`${Util.i18n.formatKbps(throughputKbps)} throughput (Simulated)`;

const isSlow4G = () => {
return rttMs === 150 && throughputKbps === 1.6 * 1024;
Expand Down
139 changes: 91 additions & 48 deletions report/renderer/i18n.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,25 +21,59 @@ export class I18n {
// When testing, use a locale with more exciting numeric formatting.
if (locale === 'en-XA') locale = 'de';

this._numberDateLocale = locale;
this._numberFormatter = new Intl.NumberFormat(locale);
this._percentFormatter = new Intl.NumberFormat(locale, {style: 'percent'});
this._locale = locale;
this._strings = strings;
}

get strings() {
return this._strings;
}

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

This comment was marked as resolved.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const log10 = -Math.log10(granularity);
if (!Number.isFinite(log10) || (granularity > 1 && !Number.isInteger(log10))) {
console.warn(`granularity of ${granularity} is invalid, defaulting to value of 1`);
granularity = 1;
}

if (granularity < 1) {
opts.minimumFractionDigits = opts.maximumFractionDigits = Math.ceil(log10);
}

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (Object.is(number, -0)) number = 0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IEEE-754 requires that they compare as equal 🤓


return new Intl.NumberFormat(this._locale, opts).format(number).replace(' ', NBSP2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Copy link
Collaborator Author

@connorjclark connorjclark Apr 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

hmm....

}

/**
* 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) {
Copy link
Collaborator Author

@connorjclark connorjclark Apr 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead I made a new function formatInteger

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const coarseValue = Math.round(number / granularity) * granularity;
return this._numberFormatter.format(coarseValue);
return this._formatNumberWithGranularity(number, granularity);
}

/**
* Format integer.
* Just like {@link formatNumber} but uses a granularity of 1, rounding to the nearest
* whole number.
* @param {number} number
* @return {string}
*/
formatInteger(number) {
return this._formatNumberWithGranularity(number, 1);
}

/**
Expand All @@ -48,7 +82,7 @@ export class I18n {
* @return {string}
*/
formatPercent(number) {
return this._percentFormatter.format(number);
return new Intl.NumberFormat(this._locale, {style: 'percent'}).format(number);
}

/**
Expand All @@ -57,9 +91,7 @@ export class I18n {
* @return {string}
*/
formatBytesToKiB(size, granularity = 0.1) {
const formatter = this._byteFormatterForGranularity(granularity);
const kbs = formatter.format(Math.round(size / 1024 / granularity) * granularity);
return `${kbs}${NBSP2}KiB`;
return this._formatNumberWithGranularity(size / KiB, granularity) + `${NBSP2}KiB`;
}

/**
Expand All @@ -68,9 +100,7 @@ export class I18n {
* @return {string}
*/
formatBytesToMiB(size, granularity = 0.1) {
const formatter = this._byteFormatterForGranularity(granularity);
const kbs = formatter.format(Math.round(size / (1024 ** 2) / granularity) * granularity);
return `${kbs}${NBSP2}MiB`;
return this._formatNumberWithGranularity(size / MiB, granularity) + `${NBSP2}MiB`;
}

/**
Expand All @@ -79,9 +109,11 @@ export class I18n {
* @return {string}
*/
formatBytes(size, granularity = 1) {
const formatter = this._byteFormatterForGranularity(granularity);
const kbs = formatter.format(Math.round(size / granularity) * granularity);
return `${kbs}${NBSP2}bytes`;
return this._formatNumberWithGranularity(size, granularity, {
style: 'unit',
unit: 'byte',
unitDisplay: 'long',
});
}

/**
Expand All @@ -92,25 +124,23 @@ export class I18n {
formatBytesWithBestUnit(size, granularity = 0.1) {
if (size >= MiB) return this.formatBytesToMiB(size, granularity);
if (size >= KiB) return this.formatBytesToKiB(size, granularity);
return this.formatNumber(size, granularity) + '\xa0B';
return this._formatNumberWithGranularity(size, granularity, {
style: 'unit',
unit: 'byte',
unitDisplay: 'narrow',
});
}

/**
* Format bytes with a constant number of fractional digits, i.e. for a granularity of 0.1, 10 becomes '10.0'
* @param {number} granularity Controls how coarse the displayed value is
* @return {Intl.NumberFormat}
* @param {number} size
* @param {number=} granularity Controls how coarse the displayed value is, defaults to 1
* @return {string}
*/
_byteFormatterForGranularity(granularity) {
// assume any granularity above 1 will not contain fractional parts, i.e. will never be 1.5
let numberOfFractionDigits = 0;
if (granularity < 1) {
numberOfFractionDigits = -Math.floor(Math.log10(granularity));
}

return new Intl.NumberFormat(this._numberDateLocale, {
...this._numberFormatter.resolvedOptions(),
maximumFractionDigits: numberOfFractionDigits,
minimumFractionDigits: numberOfFractionDigits,
formatKbps(size, granularity = 1) {
return this._formatNumberWithGranularity(size, granularity, {
style: 'unit',
unit: 'kilobit-per-second',
unitDisplay: 'short',
});
}

Expand All @@ -120,10 +150,11 @@ export class I18n {
* @return {string}
*/
formatMilliseconds(ms, granularity = 10) {
const coarseTime = Math.round(ms / granularity) * granularity;
return coarseTime === 0
? `${this._numberFormatter.format(0)}${NBSP2}ms`
: `${this._numberFormatter.format(coarseTime)}${NBSP2}ms`;
return this._formatNumberWithGranularity(ms, granularity, {
style: 'unit',
unit: 'millisecond',
unitDisplay: 'short',
});
}

/**
Expand All @@ -132,8 +163,11 @@ export class I18n {
* @return {string}
*/
formatSeconds(ms, granularity = 0.1) {
const coarseTime = Math.round(ms / 1000 / granularity) * granularity;
return `${this._numberFormatter.format(coarseTime)}${NBSP2}s`;
return this._formatNumberWithGranularity(ms / 1000, granularity, {
style: 'unit',
unit: 'second',
unitDisplay: 'short',
});
}

/**
Expand All @@ -153,10 +187,10 @@ export class I18n {
// and https://github.com/GoogleChrome/lighthouse/pull/9822
let formatter;
try {
formatter = new Intl.DateTimeFormat(this._numberDateLocale, options);
formatter = new Intl.DateTimeFormat(this._locale, options);
} catch (err) {
options.timeZone = 'UTC';
formatter = new Intl.DateTimeFormat(this._numberDateLocale, options);
formatter = new Intl.DateTimeFormat(this._locale, options);
}

return formatter.format(new Date(date));
Expand All @@ -168,6 +202,10 @@ export class I18n {
* @return {string}
*/
formatDuration(timeInMilliseconds) {
Copy link
Collaborator Author

@connorjclark connorjclark Apr 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just realized the comment for this function was wrong

1d 2h 13m 52s

we had NBSP all up in there...

// There is a proposal for a Intl.DurationFormat.
// https://github.com/tc39/proposal-intl-duration-format
// Until then, we do things a bit more manually.

let timeInSeconds = timeInMilliseconds / 1000;
if (Math.round(timeInSeconds) === 0) {
return 'None';
Expand All @@ -176,19 +214,24 @@ export class I18n {
/** @type {Array<string>} */
const parts = [];
/** @type {Record<string, number>} */
const unitLabels = {
d: 60 * 60 * 24,
h: 60 * 60,
m: 60,
s: 1,
const unitToSecondsPer = {
day: 60 * 60 * 24,
hour: 60 * 60,
minute: 60,
second: 1,
};

Object.keys(unitLabels).forEach(label => {
const unit = unitLabels[label];
const numberOfUnits = Math.floor(timeInSeconds / unit);
Object.keys(unitToSecondsPer).forEach(unit => {
const secondsPerUnit = unitToSecondsPer[unit];
const numberOfUnits = Math.floor(timeInSeconds / secondsPerUnit);
if (numberOfUnits > 0) {
timeInSeconds -= numberOfUnits * unit;
parts.push(`${numberOfUnits}\xa0${label}`);
timeInSeconds -= numberOfUnits * secondsPerUnit;
const part = this._formatNumberWithGranularity(numberOfUnits, 1, {
style: 'unit',
unit,
unitDisplay: 'narrow',
});
parts.push(part);
}
});

Expand Down
20 changes: 13 additions & 7 deletions report/renderer/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -429,10 +429,13 @@ class Util {
break;
case 'devtools': {
const {cpuSlowdownMultiplier, requestLatencyMs} = throttling;
cpuThrottling = `${Util.i18n.formatNumber(cpuSlowdownMultiplier)}x slowdown (DevTools)`;
networkThrottling = `${Util.i18n.formatNumber(requestLatencyMs)}${NBSP}ms HTTP RTT, ` +
`${Util.i18n.formatNumber(throttling.downloadThroughputKbps)}${NBSP}Kbps down, ` +
`${Util.i18n.formatNumber(throttling.uploadThroughputKbps)}${NBSP}Kbps up (DevTools)`;
// TODO: better api in i18n formatter such that this isn't needed.
const cpuGranularity = Number.isInteger(cpuSlowdownMultiplier) ? 1 : 0.1;
// eslint-disable-next-line max-len
cpuThrottling = `${Util.i18n.formatNumber(cpuSlowdownMultiplier, cpuGranularity)}x slowdown (DevTools)`;
networkThrottling = `${Util.i18n.formatMilliseconds(requestLatencyMs, 1)} HTTP RTT, ` +
`${Util.i18n.formatKbps(throttling.downloadThroughputKbps)} down, ` +
`${Util.i18n.formatKbps(throttling.uploadThroughputKbps)} up (DevTools)`;

const isSlow4G = () => {
return requestLatencyMs === 150 * 3.75 &&
Expand All @@ -444,9 +447,12 @@ class Util {
}
case 'simulate': {
const {cpuSlowdownMultiplier, rttMs, throughputKbps} = throttling;
cpuThrottling = `${Util.i18n.formatNumber(cpuSlowdownMultiplier)}x slowdown (Simulated)`;
networkThrottling = `${Util.i18n.formatNumber(rttMs)}${NBSP}ms TCP RTT, ` +
`${Util.i18n.formatNumber(throughputKbps)}${NBSP}Kbps throughput (Simulated)`;
// TODO: better api in i18n formatter such that this isn't needed.
const cpuGranularity = Number.isInteger(cpuSlowdownMultiplier) ? 1 : 0.1;
// eslint-disable-next-line max-len
cpuThrottling = `${Util.i18n.formatNumber(cpuSlowdownMultiplier, cpuGranularity)}x slowdown (Simulated)`;
networkThrottling = `${Util.i18n.formatMilliseconds(rttMs)} TCP RTT, ` +
`${Util.i18n.formatKbps(throughputKbps)} throughput (Simulated)`;

const isSlow4G = () => {
return rttMs === 150 && throughputKbps === 1.6 * 1024;
Expand Down
33 changes: 26 additions & 7 deletions report/test/renderer/i18n-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,12 @@ const NBSP = '\xa0';
describe('util helpers', () => {
it('formats a number', () => {
const i18n = new I18n('en', {...Util.UIStrings});
assert.strictEqual(i18n.formatNumber(10), '10');
assert.strictEqual(i18n.formatNumber(100.01), '100');
assert.strictEqual(i18n.formatNumber(10), '10.0');
assert.strictEqual(i18n.formatNumber(100.01), '100.0');
assert.strictEqual(i18n.formatNumber(13000.456), '13,000.5');
assert.strictEqual(i18n.formatInteger(10), '10');
assert.strictEqual(i18n.formatInteger(100.01), '100');
assert.strictEqual(i18n.formatInteger(13000.6), '13,001');
});

it('formats a date', () => {
Expand Down Expand Up @@ -100,9 +103,25 @@ describe('util helpers', () => {

it('formats a duration', () => {
const i18n = new I18n('en', {...Util.UIStrings});
assert.equal(i18n.formatDuration(60 * 1000), `1${NBSP}m`);
assert.equal(i18n.formatDuration(60 * 60 * 1000 + 5000), `1${NBSP}h 5${NBSP}s`);
assert.equal(i18n.formatDuration(28 * 60 * 60 * 1000 + 5000), `1${NBSP}d 4${NBSP}h 5${NBSP}s`);
assert.equal(i18n.formatDuration(60 * 1000), '1m');
assert.equal(i18n.formatDuration(60 * 60 * 1000 + 5000), '1h 5s');
assert.equal(i18n.formatDuration(28 * 60 * 60 * 1000 + 5000), '1d 4h 5s');
});

it('formats a duration based on locale', () => {
let i18n = new I18n('de', {...Util.UIStrings});
assert.equal(i18n.formatDuration(60 * 1000), `1${NBSP}Min.`);
assert.equal(i18n.formatDuration(60 * 60 * 1000 + 5000), `1${NBSP}Std. 5${NBSP}Sek.`);
assert.equal(
i18n.formatDuration(28 * 60 * 60 * 1000 + 5000), `1${NBSP}T 4${NBSP}Std. 5${NBSP}Sek.`);

// Yes, this is actually backwards (s h d).
i18n = new I18n('ar', {...Util.UIStrings});
/* eslint-disable no-irregular-whitespace */
assert.equal(i18n.formatDuration(60 * 1000), `١${NBSP}د`);
assert.equal(i18n.formatDuration(60 * 60 * 1000 + 5000), `١${NBSP}س ٥${NBSP}ث`);
assert.equal(i18n.formatDuration(28 * 60 * 60 * 1000 + 5000), `١ ي ٤ س ٥ ث`);
/* eslint-enable no-irregular-whitespace */
});

it('formats numbers based on locale', () => {
Expand All @@ -113,7 +132,7 @@ describe('util helpers', () => {
assert.strictEqual(i18n.formatNumber(number), '12.346,9');
assert.strictEqual(i18n.formatBytesToKiB(number), `12,1${NBSP}KiB`);
assert.strictEqual(i18n.formatMilliseconds(number), `12.350${NBSP}ms`);
assert.strictEqual(i18n.formatSeconds(number), `12,3${NBSP}s`);
assert.strictEqual(i18n.formatSeconds(number), `12,3${NBSP}Sek.`);
});

it('uses decimal comma with en-XA test locale', () => {
Expand All @@ -124,7 +143,7 @@ describe('util helpers', () => {
assert.strictEqual(i18n.formatNumber(number), '12.346,9');
assert.strictEqual(i18n.formatBytesToKiB(number), `12,1${NBSP}KiB`);
assert.strictEqual(i18n.formatMilliseconds(number), `12.350${NBSP}ms`);
assert.strictEqual(i18n.formatSeconds(number), `12,3${NBSP}s`);
assert.strictEqual(i18n.formatSeconds(number), `12,3${NBSP}Sek.`);
});

it('should not crash on unknown locales', () => {
Expand Down
4 changes: 2 additions & 2 deletions report/test/renderer/util-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ describe('util helpers', () => {
});

// eslint-disable-next-line max-len
assert.equal(descriptions.networkThrottling, '565\xa0ms HTTP RTT, 1,400\xa0Kbps down, 600\xa0Kbps up (DevTools)');
assert.equal(descriptions.networkThrottling, '565\xa0ms HTTP RTT, 1,400\xa0kb/s down, 600\xa0kb/s up (DevTools)');
assert.equal(descriptions.cpuThrottling, '4.5x slowdown (DevTools)');
});

Expand All @@ -71,7 +71,7 @@ describe('util helpers', () => {
});

// eslint-disable-next-line max-len
assert.equal(descriptions.networkThrottling, '150\xa0ms TCP RTT, 1,600\xa0Kbps throughput (Simulated)');
assert.equal(descriptions.networkThrottling, '150\xa0ms TCP RTT, 1,600\xa0kb/s throughput (Simulated)');
assert.equal(descriptions.cpuThrottling, '2x slowdown (Simulated)');
});

Expand Down
Loading