misc(scripts): improve collision check in collect-strings (#12697)

Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>

lighthouse-core/audits/accessibility/aria-progressbar-name.js

@@ -19,7 +19,7 @@ const UIStrings = {
   /** Title of an accessibility audit that evaluates if progressbar HTML elements do not have accessible names. This title is descriptive of the failing state and is shown to users when there is a failure that needs to be addressed. */
   failureTitle: 'ARIA `progressbar` elements do not have accessible names.',
   /** Description of a Lighthouse audit that tells the user *why* they should try to pass. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */
-  description: 'When an element doesn\'t have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. [Learn more](https://web.dev/aria-name/).',
+  description: 'When a `progressbar` element doesn\'t have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. [Learn more](https://web.dev/aria-name/).',
 };
 
 const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);

lighthouse-core/scripts/i18n/collect-strings.js

@@ -11,7 +11,7 @@
 const fs = require('fs');
 const glob = require('glob');
 const path = require('path');
-const assert = require('assert').strict;
+const expect = require('expect');
 const tsc = require('typescript');
 const MessageParser = require('intl-messageformat-parser').default;
 const Util = require('../../../report/renderer/util.js');
@@ -517,15 +517,6 @@ function parseUIStrings(sourceStr, liveUIStrings) {
   return parsedMessages;
 }
 
-/** @type {Map<string, string>} */
-const seenStrings = new Map();
-
-/** @type {number} */
-let collisions = 0;
-
-/** @type {Array<string>} */
-const collisionStrings = [];
-
 /**
  * Collects all LHL messsages defined in UIString from Javascript files in dir,
  * and converts them into CTC.
@@ -584,28 +575,6 @@ function collectAllStringsInDir(dir) {
 
       const messageKey = `${relativeToRootPath} | ${key}`;
       strings[messageKey] = ctc;
-
-      // check for duplicates, if duplicate, add @description as @meaning to both
-      if (seenStrings.has(ctc.message)) {
-        ctc.meaning = ctc.description;
-        const seenId = seenStrings.get(ctc.message);
-        // TODO: `strings[seenId]` check shouldn't be necessary here ...
-        // see https://github.com/GoogleChrome/lighthouse/pull/12441/files#r630521367
-        if (seenId && strings[seenId]) {
-          if (!strings[seenId].meaning) {
-            strings[seenId].meaning = strings[seenId].description;
-            collisions++;
-          }
-
-          if (ctc.meaning === strings[seenId].meaning) {
-            throw new Error(`'${messageKey}' is an exact duplicate of '${seenId}' when placeholders are removed. Each strings' \`message\` or \`description\` must be different for the translation pipeline`);
-          }
-
-          collisionStrings.push(ctc.message);
-          collisions++;
-        }
-      }
-      seenStrings.set(ctc.message, messageKey);
     }
   }
 
@@ -628,6 +597,98 @@ function writeStringsToCtcFiles(locale, strings) {
   fs.writeFileSync(fullPath, JSON.stringify(output, null, 2) + '\n');
 }
 
+/**
+ * This function does three things:
+ *
+ *    - Add `meaning` property to ctc messages that have the same message so TC can disambiguate (otherwise it fails to import).
+ *    - Throw if the `meaning` of any collisions *also* collides (can't disambiguate messages).
+ *    - Throw if the known collisions has changed at all.
+ *
+ * @param {Record<string, CtcMessage>} strings
+ */
+function resolveMessageCollisions(strings) {
+  /** @type {Map<string, Array<[string, CtcMessage]>>} */
+  const stringsByMessage = new Map();
+
+  // Group all the strings by their message.
+  for (const entry of Object.entries(strings)) {
+    const collisions = stringsByMessage.get(entry[1].message) || [];
+    collisions.push(entry);
+    stringsByMessage.set(entry[1].message, collisions);
+  }
+
+  /** @type {Array<[string, CtcMessage]>} */
+  const allCollisions = [];
+  for (const group of stringsByMessage.values()) {
+    // If this message didn't collide with anything else, skip it.
+    if (group.length <= 1) continue;
+    allCollisions.push(...group);
+
+    // We have a message collision, time to check collisions on the `meaning` property.
+    /** @type {Map<string|undefined, Array<[string, CtcMessage]>>} */
+    const stringsByMeaning = new Map();
+    for (const [key, ctc] of group) {
+      ctc.meaning = ctc.description;
+
+      const collisions = stringsByMeaning.get(ctc.meaning) || [];
+      collisions.push([key, ctc]);
+      stringsByMeaning.set(ctc.meaning, collisions);
+    }
+
+    for (const meaningGroup of stringsByMeaning.values()) {
+      if (meaningGroup.length <= 1) continue;
+
+      const debugMeaningList = meaningGroup.map(entry => [entry[0], entry[1].meaning].join('\n'));
+      const debugCollisionsMessage = `${meaningGroup[0][1].message}\n\n${debugMeaningList.join('\n\n')}`;
+      throw new Error(`Each strings' \`message\` or \`description\` must be different for the translation pipeline. The following keys did not have unique \`meaning\` values:\n\n${debugCollisionsMessage}`);
+    }
+  }
+
+  // We survived fatal collisions, now check that the known collisions match our known list.
+  const collidingMessages = allCollisions.map(collision => collision[1].message).sort();
+
+  try {
+    expect(collidingMessages).toEqual([
+      '$MARKDOWN_SNIPPET_0$ elements do not have $MARKDOWN_SNIPPET_1$ text',
+      '$MARKDOWN_SNIPPET_0$ elements do not have $MARKDOWN_SNIPPET_1$ text',
+      '$MARKDOWN_SNIPPET_0$ elements have $MARKDOWN_SNIPPET_1$ text',
+      '$MARKDOWN_SNIPPET_0$ elements have $MARKDOWN_SNIPPET_1$ text',
+      'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.',
+      'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.',
+      'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.',
+      'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.',
+      'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names',
+      'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names',
+      'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names',
+      'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names',
+      'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.',
+      'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.',
+      'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.',
+      'Consider using a $LINK_START_0$plugin$LINK_END_0$ or service that will automatically convert your uploaded images to the optimal formats.',
+      'Consider using a $LINK_START_0$plugin$LINK_END_0$ or service that will automatically convert your uploaded images to the optimal formats.',
+      'Document has a valid $MARKDOWN_SNIPPET_0$',
+      'Document has a valid $MARKDOWN_SNIPPET_0$',
+      'Failing Elements',
+      'Failing Elements',
+      'Name',
+      'Name',
+      'Potential Savings',
+      'Potential Savings',
+      'URL',
+      'URL',
+      'When an element doesn\'t have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.',
+      'When an element doesn\'t have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.',
+      'When an element doesn\'t have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.',
+      'When an element doesn\'t have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.',
+    ]);
+  } catch (err) {
+    console.log('The number of duplicate strings have changed, update this assertion if that is expected, or reword strings');
+    console.log('copy/paste this to pass check:');
+    console.log(collidingMessages);
+    throw new Error(err.message);
+  }
+}
+
 // Test if called from the CLI or as a module.
 if (require.main === module) {
   /** @type {Record<string, CtcMessage>} */
@@ -639,10 +700,7 @@ if (require.main === module) {
     Object.assign(strings, moreStrings);
   }
 
-  if (collisions > 0) {
-    console.log(`MEANING COLLISION: ${collisions} string(s) have the same content.`);
-    assert.equal(collisions, 28, `The number of duplicate strings have changed, update this assertion if that is expected, or reword strings. Collisions: ${collisionStrings.join('\n')}`);
-  }
+  resolveMessageCollisions(strings);
 
   writeStringsToCtcFiles('en-US', strings);
   console.log('Written to disk!', 'en-US.ctc.json');

lighthouse-core/test/results/sample_v2.json

@@ -2649,7 +2649,7 @@
     "aria-progressbar-name": {
       "id": "aria-progressbar-name",
       "title": "ARIA `progressbar` elements have accessible names",
-      "description": "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. [Learn more](https://web.dev/aria-name/).",
+      "description": "When a `progressbar` element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. [Learn more](https://web.dev/aria-name/).",
       "score": null,
       "scoreDisplayMode": "notApplicable"
     },