i18n: fix custom formatted ICU within plurals (#9460)

GoogleChrome · Aug 16, 2019 · 2e053e1 · 2e053e1
1 parent f276e9b
commit 2e053e1
Show file tree

Hide file tree

Showing 6 changed files with 100 additions and 14 deletions.
diff --git a/lighthouse-core/audits/resource-summary.js b/lighthouse-core/audits/resource-summary.js
@@ -16,8 +16,9 @@ const UIStrings = {
   description: 'To set budgets for the quantity and size of page resources,' +
     ' add a budget.json file. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/budgets).',
   /** [ICU Syntax] Label for an audit identifying the number of requests and kilobytes used to load the page. */
-  displayValue: `{requestCount, plural, =1 {1 request} other {# requests}}` +
-    ` • {byteCount, number, bytes} KB`,
+  displayValue: `{requestCount, plural, ` +
+    `=1 {1 request • {byteCount, number, bytes} KB} ` +
+    `other {# requests • {byteCount, number, bytes} KB}}`,
 };
 
 const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);

diff --git a/lighthouse-core/lib/i18n/i18n.js b/lighthouse-core/lib/i18n/i18n.js
@@ -132,8 +132,60 @@ function lookupLocale(locale) {
 function _preprocessMessageValues(icuMessage, values = {}) {
   const clonedValues = JSON.parse(JSON.stringify(values));
   const parsed = MessageParser.parse(icuMessage);
+
+  const elements = _collectAllCustomElementsFromICU(parsed.elements);
+
+  return _processParsedElements(Array.from(elements.values()), clonedValues);
+}
+
+/**
+ * Function to retrieve all 'argumentElement's from an ICU message. An argumentElement
+ * is an ICU element with an argument in it, like '{varName}' or '{varName, number, bytes}'. This
+ * differs from 'messageElement's which are just arbitrary text in a message.
+ *
+ * Notes:
+ *  This function will recursively inspect plural elements for nested argumentElements.
+ *
+ *  We need to find all the elements from the plural format sections, but
+ *  they need to be deduplicated. I.e. "=1{hello {icu}} =other{hello {icu}}"
+ *  the variable "icu" would appear twice if it wasn't de duplicated. And they cannot
+ *  be stored in a set because they are not equal since their locations are different,
+ *  thus they are stored via a Map keyed on the "id" which is the ICU varName.
+ *
+ * @param {Array<import('intl-messageformat-parser').Element>} icuElements
+ * @param {Map<string, import('intl-messageformat-parser').Element>} seenElementsById
+ */
+function _collectAllCustomElementsFromICU(icuElements, seenElementsById = new Map()) {
+  for (const el of icuElements) {
+    // We are only interested in elements that need ICU formatting (argumentElements)
+    if (el.type !== 'argumentElement') continue;
+    // @ts-ignore - el.id is always defined when el.format is defined
+    seenElementsById.set(el.id, el);
+
+    // Plurals need to be inspected recursively
+    if (!el.format || el.format.type !== 'pluralFormat') continue;
+    // Look at all options of the plural (=1{} =other{}...)
+    for (const option of el.format.options) {
+      // Run collections on each option's elements
+      _collectAllCustomElementsFromICU(option.value.elements,
+        seenElementsById);
+    }
+  }
+
+  return seenElementsById;
+}
+
+/**
+ * This function takes a list of ICU argumentElements and a map of values and
+ * will apply Lighthouse custom formatting to the values based on the argumentElement
+ * format style.
+ *
+ * @param {Array<import('intl-messageformat-parser').Element>} argumentElements
+ * @param {Record<string, string | number>} [values]
+ */
+function _processParsedElements(argumentElements, values = {}) {
   // Throw an error if a message's value isn't provided
-  parsed.elements
+  argumentElements
     .filter(el => el.type === 'argumentElement')
     .forEach(el => {
       if (el.id && (el.id in values) === false) {
@@ -142,24 +194,24 @@ function _preprocessMessageValues(icuMessage, values = {}) {
     });
 
   // Round all milliseconds to the nearest 10
-  parsed.elements
+  argumentElements
     .filter(el => el.format && el.format.style === 'milliseconds')
     // @ts-ignore - el.id is always defined when el.format is defined
-    .forEach(el => (clonedValues[el.id] = Math.round(clonedValues[el.id] / 10) * 10));
+    .forEach(el => (values[el.id] = Math.round(values[el.id] / 10) * 10));
 
   // Convert all seconds to the correct unit
-  parsed.elements
+  argumentElements
     .filter(el => el.format && el.format.style === 'seconds' && el.id === 'timeInMs')
     // @ts-ignore - el.id is always defined when el.format is defined
-    .forEach(el => (clonedValues[el.id] = Math.round(clonedValues[el.id] / 100) / 10));
+    .forEach(el => (values[el.id] = Math.round(values[el.id] / 100) / 10));
 
   // Replace all the bytes with KB
-  parsed.elements
+  argumentElements
     .filter(el => el.format && el.format.style === 'bytes')
     // @ts-ignore - el.id is always defined when el.format is defined
-    .forEach(el => (clonedValues[el.id] = clonedValues[el.id] / 1024));
+    .forEach(el => (values[el.id] = values[el.id] / 1024));
 
-  return clonedValues;
+  return values;
 }
 
 /**

diff --git a/lighthouse-core/lib/i18n/locales/en-US.json b/lighthouse-core/lib/i18n/locales/en-US.json
@@ -879,7 +879,7 @@
     "message": "To set budgets for the quantity and size of page resources, add a budget.json file. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/budgets)."
   },
   "lighthouse-core/audits/resource-summary.js | displayValue": {
-    "message": "{requestCount, plural, =1 {1 request} other {# requests}} • {byteCount, number, bytes} KB"
+    "message": "{requestCount, plural, =1 {1 request • {byteCount, number, bytes} KB} other {# requests • {byteCount, number, bytes} KB}}"
   },
   "lighthouse-core/audits/resource-summary.js | title": {
     "message": "Keep request counts low and transfer sizes small"

diff --git a/lighthouse-core/lib/i18n/locales/en-XL.json b/lighthouse-core/lib/i18n/locales/en-XL.json
@@ -879,7 +879,7 @@
     "message": "T̂ó ŝét̂ b́ûd́ĝét̂ś f̂ór̂ t́ĥé q̂úâńt̂ít̂ý âńd̂ śîźê óf̂ ṕâǵê ŕêśôúr̂ćêś, âd́d̂ á b̂úd̂ǵêt́.ĵśôń f̂íl̂é. [L̂éâŕn̂ ḿôŕê](https://developers.google.com/web/tools/lighthouse/audits/budgets)."
   },
   "lighthouse-core/audits/resource-summary.js | displayValue": {
-    "message": "{requestCount, plural, =1 {1 r̂éq̂úêśt̂} other {# ŕêq́ûéŝt́ŝ}} • {byteCount, number, bytes} ḰB̂"
+    "message": "{requestCount, plural, =1 {1 r̂éq̂úêśt̂ • {byteCount, number, bytes} ḰB̂} other {# ŕêq́ûéŝt́ŝ • {byteCount, number, bytes} ḰB̂}}"
   },
   "lighthouse-core/audits/resource-summary.js | title": {
     "message": "K̂éêṕ r̂éq̂úêśt̂ ćôún̂t́ŝ ĺôẃ âńd̂ t́r̂án̂śf̂ér̂ śîźêś ŝḿâĺl̂"

diff --git a/lighthouse-core/test/lib/i18n/i18n-test.js b/lighthouse-core/test/lib/i18n/i18n-test.js
@@ -103,6 +103,17 @@ describe('i18n', () => {
       helloTimeInMsWorld: 'Hello {timeInMs, number, seconds} World',
       helloPercentWorld: 'Hello {in, number, extendedPercent} World',
       helloWorldMultiReplace: '{hello} {world}',
+      helloPlural: '{itemCount, plural, =1{1 hello} other{hellos}}',
+      helloPluralNestedICU: '{itemCount, plural, ' +
+        '=1{1 hello {in, number, bytes}} ' +
+        'other{hellos {in, number, bytes}}}',
+      helloPluralNestedPluralAndICU: '{itemCount, plural, ' +
+        '=1{{innerItemCount, plural, ' +
+          '=1{1 hello 1 goodbye {in, number, bytes}} ' +
+          'other{1 hello, goodbyes {in, number, bytes}}}} ' +
+        'other{{innerItemCount, plural, ' +
+          '=1{hellos 1 goodbye {in, number, bytes}} ' +
+          'other{hellos, goodbyes {in, number, bytes}}}}}',
     };
     const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);
 
@@ -146,5 +157,27 @@ describe('i18n', () => {
         {hello: 'hello'}), 'en-US'))
       .toThrow(`ICU Message contains a value reference ("world") that wasn't provided`);
     });
+
+    it('formats a message with plurals', () => {
+      const helloStr = str_(UIStrings.helloPlural, {itemCount: 3});
+      expect(helloStr).toBeDisplayString('hellos');
+    });
+
+    it('throws an error when a plural control value is missing', () => {
+      expect(_ => i18n.getFormatted(str_(UIStrings.helloPlural), 'en-US'))
+      .toThrow(`ICU Message contains a value reference ("itemCount") that wasn't provided`);
+    });
+
+    it('formats a message with plurals and nested custom ICU', () => {
+      const helloStr = str_(UIStrings.helloPluralNestedICU, {itemCount: 3, in: 1875});
+      expect(helloStr).toBeDisplayString('hellos 2');
+    });
+
+    it('formats a message with plurals and nested custom ICU and nested plural', () => {
+      const helloStr = str_(UIStrings.helloPluralNestedPluralAndICU, {itemCount: 3,
+        innerItemCount: 1,
+        in: 1875});
+      expect(helloStr).toBeDisplayString('hellos 1 goodbye 2');
+    });
   });
 });
diff --git a/types/intl-messageformat-parser/index.d.ts b/types/intl-messageformat-parser/index.d.ts
@@ -1,9 +1,9 @@
 declare module 'intl-messageformat-parser' {
-  interface Element {
+  export interface Element {
     type: 'messageTextElement'|'argumentElement';
     id?: string
     value?: string
-    format?: null | {type: string; style?: string};
+    format?: null | {type: string; style?: string; options?: any};
   }
   function parse(message: string): {elements: Element[]};
   export {parse};