i18n: handle string placeholder collisions #14432
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ import {expect} from 'expect'; | |
| import tsc from 'typescript'; | ||
| import MessageParser from 'intl-messageformat-parser'; | ||
| import esMain from 'es-main'; | ||
| import isDeepEqual from 'lodash/isEqual.js'; | ||
|
|
||
| import {Util} from '../../util.cjs'; | ||
| import {collectAndBakeCtcStrings} from './bake-ctc-to-lhl.js'; | ||
|
|
@@ -613,43 +614,90 @@ function writeStringsToCtcFiles(locale, strings) { | |
| } | ||
|
|
||
| /** | ||
| * TODO: replace by Array.prototype.groupToMap when available. | ||
| * @template {unknown} T | ||
| * @template {string} U | ||
| * @param {Array<T>} elements | ||
| * @param {(element: T) => U} callback | ||
| * @return {Map<U, Array<T>>} | ||
| */ | ||
| function arrayGroupToMap(elements, callback) { | ||
|
There was a problem hiding this comment. Had to do this twice (nested) and was getting kind of gross. It's a nice API (for Node 20 or whatever) |
||
| /** @type {Map<U, Array<T>>} */ | ||
| const elementsByValue = new Map(); | ||
|
|
||
| for (const element of elements) { | ||
| const key = callback(element); | ||
| const group = elementsByValue.get(key) || []; | ||
| group.push(element); | ||
| elementsByValue.set(key, group); | ||
| } | ||
|
|
||
| return elementsByValue; | ||
| } | ||
|
|
||
| /** | ||
| * Returns whether all the placeholders in the given CtcMessages match. | ||
| * @param {Array<{ctc: CtcMessage}>} strings | ||
| * @return {boolean} | ||
| */ | ||
| function doPlaceholdersMatch(strings) { | ||
| // Technically placeholder `content` is not required to match by TC, but since | ||
| // `example` must match and any auto-generated `example` is copied from `content`, | ||
| // it would be confusing to let it differ when `example` is explicit. | ||
| return strings.every(val => isDeepEqual(val.ctc.placeholders, strings[0].ctc.placeholders)); | ||
| } | ||
|
|
||
| /** | ||
| * If two or more messages have the same `message`: | ||
| * - if `description`, and `placeholders` are also the same, TC treats them as a | ||
| * single message, the collision is allowed, and no change is made. | ||
| * - if `description` is unique for the collision members, the `description` is | ||
| * copied to `meaning` to differentiate them for TC. They must be added to the | ||
| * `collidingMessages` list for this case as a nudge to not be wasteful. | ||
| * - if `description` is the same but `placeholders` differ, an error is thrown. | ||
| * Either `placeholders` should be the same, or `message` or `description` | ||
| * should be changed to explain the need for different `placeholders`. | ||
| * | ||
| * Modifies `strings` in place to fix collisions. | ||
| * @see https://github.com/GoogleChrome/lighthouse/blob/main/core/lib/i18n/README.md#collisions | ||
| * @param {Record<string, CtcMessage>} strings | ||
| */ | ||
| function resolveMessageCollisions(strings) { | ||
| const entries = Object.entries(strings).map(([key, ctc]) => ({key, ctc})); | ||
|
|
||
| const stringsByMessage = arrayGroupToMap(entries, (entry) => entry.ctc.message); | ||
| for (const messageGroup of stringsByMessage.values()) { | ||
| // If this message didn't collide with anything else, skip it. | ||
| if (messageGroup.length <= 1) continue; | ||
|
|
||
| // For each group of matching message+description, placeholders must also match. | ||
| const stringsByDescription = arrayGroupToMap(messageGroup, (entry) => entry.ctc.description); | ||
| for (const descriptionGroup of stringsByDescription.values()) { | ||
| if (!doPlaceholdersMatch(descriptionGroup)) { | ||
| throw new Error('string collision: `message` and `description` are the same but differ in `placeholders`:\n' + | ||
| descriptionGroup.map(entry => `key: ${entry.key}\n`).join('') + | ||
| 'make `placeholders` match or update `message` or `description`s to explain why they don\'t match'); | ||
| } | ||
| } | ||
|
|
||
| // If the entire message group has the same description and placeholders, | ||
| // they can be translated as if a single message, no meaning needed. | ||
| if (stringsByDescription.size <= 1) continue; | ||
|
|
||
| // We have duplicate messages with different descriptions. Disambiguate using `meaning` for TC. | ||
| for (const {ctc} of messageGroup) { | ||
| ctc.meaning = ctc.description; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Check that fixed collisions match our known list. | ||
| * @param {Record<string, CtcMessage>} strings | ||
| */ | ||
| function checkKnownFixedCollisions(strings) { | ||
| const fixedCollisions = Object.values(strings).filter(string => string.meaning); | ||
| const collidingMessages = fixedCollisions.map(collision => collision.message).sort(); | ||
|
|
||
| try { | ||
| expect(collidingMessages).toEqual([ | ||
|
|
@@ -696,6 +744,7 @@ async function main() { | |
| } | ||
|
|
||
| resolveMessageCollisions(strings); | ||
| checkKnownFixedCollisions(strings); | ||
|
|
||
| writeStringsToCtcFiles('en-US', strings); | ||
| console.log('Written to disk!', 'en-US.ctc.json'); | ||
|
|
@@ -734,4 +783,5 @@ export { | |
| parseUIStrings, | ||
| createPsuedoLocaleStrings, | ||
| convertMessageToCtc, | ||
| resolveMessageCollisions, | ||
| }; | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we mark tooltip/meter with backticks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took that from the axe docs (e.g. "Aria tooltip elements must have discernible text..."), though to be fair, their docs language selector has only one other language and that particular string stays in english :)
I kind of justified it in my head as it's an aria role, but also a description of the type of element, so it makes some sense to include a localized name.
tooltip(andmeterforaria-meter-name) are backticked later in the string 🤷edit: I could make the second reference more explicitly the markup version,
Learn how to name `role="tooltip" ` elements