From ee812b5df2a984b00c0ff398bdd66e54fea3925f Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Mon, 28 Oct 2024 14:34:12 +0100 Subject: [PATCH] [Editor] Utilize Fluent "better" when localizing the AltText Currently we manually localize and update the DOM-elements of the AltText-button, and it seems nicer to utilize Fluent "properly" for that task. This can be achieved by introducing an explicit `span`-element on the AltText-button (similar to e.g. the regular toolbar-buttons), and adding a few more l10n-strings, since that allows just setting the `data-l10n-id`-attribute on all the relevant DOM-elements. Finally, note how we no longer need to localize any strings eagerly when initializing the various editors. --- l10n/en-US/viewer.ftl | 11 +++- src/display/editor/alt_text.js | 85 ++++++++++++++------------ src/display/editor/editor.js | 21 ++----- test/integration/stamp_editor_spec.mjs | 11 ++++ 4 files changed, 70 insertions(+), 58 deletions(-) diff --git a/l10n/en-US/viewer.ftl b/l10n/en-US/viewer.ftl index 29848cd992fce..fbff0d86a9c36 100644 --- a/l10n/en-US/viewer.ftl +++ b/l10n/en-US/viewer.ftl @@ -360,9 +360,12 @@ pdfjs-ink-canvas = ## Alt-text dialog # Alternative text (alt text) helps when people can't see the image. +pdfjs-editor-alt-text-button = + .aria-label = Alt text pdfjs-editor-alt-text-button-label = Alt text -pdfjs-editor-alt-text-edit-button-label = Edit alt text +pdfjs-editor-alt-text-edit-button = + .aria-label = Edit alt text pdfjs-editor-alt-text-dialog-label = Choose an option pdfjs-editor-alt-text-dialog-description = Alt text (alternative text) helps when people can’t see the image or when it doesn’t load. pdfjs-editor-alt-text-add-description-label = Add a description @@ -457,12 +460,18 @@ pdfjs-editor-new-alt-text-ai-model-downloading-progress = Downloading alt text A .aria-valuetext = Downloading alt text AI model ({ $downloadedSize } of { $totalSize } MB) # This is a button that users can click to edit the alt text they have already added. +pdfjs-editor-new-alt-text-added-button = + .aria-label = Alt text added pdfjs-editor-new-alt-text-added-button-label = Alt text added # This is a button that users can click to open the alt text editor and add alt text when it is not present. +pdfjs-editor-new-alt-text-missing-button = + .aria-label = Missing alt text pdfjs-editor-new-alt-text-missing-button-label = Missing alt text # This is a button that opens up the alt text modal where users should review the alt text that was automatically generated. +pdfjs-editor-new-alt-text-to-review-button = + .aria-label = Review alt text pdfjs-editor-new-alt-text-to-review-button-label = Review alt text # "Created automatically" is a prefix that will be added to the beginning of any alt text that has been automatically generated. After the colon, the user will see/hear the actual alt text description. If the alt text has been edited by a human, this prefix will not appear. diff --git a/src/display/editor/alt_text.js b/src/display/editor/alt_text.js index cc7181d855a8c..6ccdea85f4b94 100644 --- a/src/display/editor/alt_text.js +++ b/src/display/editor/alt_text.js @@ -22,6 +22,8 @@ class AltText { #altTextButton = null; + #altTextButtonLabel = null; + #altTextTooltip = null; #altTextTooltipTimeout = null; @@ -40,38 +42,46 @@ class AltText { static #l10nNewButton = null; - static _l10nPromise = null; + static _l10n = null; constructor(editor) { this.#editor = editor; this.#useNewAltTextFlow = editor._uiManager.useNewAltTextFlow; AltText.#l10nNewButton ||= Object.freeze({ - added: "pdfjs-editor-new-alt-text-added-button-label", - missing: "pdfjs-editor-new-alt-text-missing-button-label", - review: "pdfjs-editor-new-alt-text-to-review-button-label", + added: "pdfjs-editor-new-alt-text-added-button", + "added-label": "pdfjs-editor-new-alt-text-added-button-label", + missing: "pdfjs-editor-new-alt-text-missing-button", + "missing-label": "pdfjs-editor-new-alt-text-missing-button-label", + review: "pdfjs-editor-new-alt-text-to-review-button", + "review-label": "pdfjs-editor-new-alt-text-to-review-button-label", }); } - static initialize(l10nPromise) { - AltText._l10nPromise ||= l10nPromise; + static initialize(l10n) { + AltText._l10n ??= l10n; } async render() { const altText = (this.#altTextButton = document.createElement("button")); altText.className = "altText"; - let msg; + altText.tabIndex = "0"; + + const label = (this.#altTextButtonLabel = document.createElement("span")); + altText.append(label); + if (this.#useNewAltTextFlow) { altText.classList.add("new"); - msg = await AltText._l10nPromise.get(AltText.#l10nNewButton.missing); - } else { - msg = await AltText._l10nPromise.get( - "pdfjs-editor-alt-text-button-label" + altText.setAttribute("data-l10n-id", AltText.#l10nNewButton.missing); + label.setAttribute( + "data-l10n-id", + AltText.#l10nNewButton["missing-label"] ); + } else { + altText.setAttribute("data-l10n-id", "pdfjs-editor-alt-text-button"); + label.setAttribute("data-l10n-id", "pdfjs-editor-alt-text-button-label"); } - altText.textContent = msg; - altText.setAttribute("aria-label", msg); - altText.tabIndex = "0"; + const signal = this.#editor._uiManager._signal; altText.addEventListener("contextmenu", noContextMenu, { signal }); altText.addEventListener("pointerdown", event => event.stopPropagation(), { @@ -144,9 +154,10 @@ class AltText { return; } this.#guessedText = guessedText; - this.#textWithDisclaimer = await AltText._l10nPromise.get( - "pdfjs-editor-new-alt-text-generated-alt-text-with-disclaimer" - )({ generatedAltText: guessedText }); + this.#textWithDisclaimer = await AltText._l10n.get( + "pdfjs-editor-new-alt-text-generated-alt-text-with-disclaimer", + { generatedAltText: guessedText } + ); this.#setState(); } @@ -229,6 +240,7 @@ class AltText { destroy() { this.#altTextButton?.remove(); this.#altTextButton = null; + this.#altTextButtonLabel = null; this.#altTextTooltip = null; this.#badge?.remove(); this.#badge = null; @@ -242,19 +254,12 @@ class AltText { if (this.#useNewAltTextFlow) { button.classList.toggle("done", !!this.#altText); - AltText._l10nPromise - .get(AltText.#l10nNewButton[this.#label]) - .then(msg => { - button.setAttribute("aria-label", msg); - // We can't just use button.textContent here, because it would remove - // the existing tooltip element. - for (const child of button.childNodes) { - if (child.nodeType === Node.TEXT_NODE) { - child.textContent = msg; - break; - } - } - }); + button.setAttribute("data-l10n-id", AltText.#l10nNewButton[this.#label]); + + this.#altTextButtonLabel?.setAttribute( + "data-l10n-id", + AltText.#l10nNewButton[`${this.#label}-label`] + ); if (!this.#altText) { this.#altTextTooltip?.remove(); return; @@ -266,11 +271,7 @@ class AltText { return; } button.classList.add("done"); - AltText._l10nPromise - .get("pdfjs-editor-alt-text-edit-button-label") - .then(msg => { - button.setAttribute("aria-label", msg); - }); + button.setAttribute("data-l10n-id", "pdfjs-editor-alt-text-edit-button"); } let tooltip = this.#altTextTooltip; @@ -315,11 +316,15 @@ class AltText { { signal } ); } - tooltip.innerText = this.#altTextDecorative - ? await AltText._l10nPromise.get( - "pdfjs-editor-alt-text-decorative-tooltip" - ) - : this.#altText; + if (this.#altTextDecorative) { + tooltip.setAttribute( + "data-l10n-id", + "pdfjs-editor-alt-text-decorative-tooltip" + ); + } else { + tooltip.removeAttribute("data-l10n-id"); + tooltip.textContent = this.#altText; + } if (!tooltip.parentNode) { button.append(tooltip); diff --git a/src/display/editor/editor.js b/src/display/editor/editor.js index d345c21a9befb..6320d7a592f80 100644 --- a/src/display/editor/editor.js +++ b/src/display/editor/editor.js @@ -88,7 +88,7 @@ class AnnotationEditor { _focusEventsAllowed = true; - static _l10nPromise = null; + static _l10n = null; static _l10nResizer = null; @@ -210,6 +210,8 @@ class AnnotationEditor { * @param {Object} l10n */ static initialize(l10n, _uiManager) { + AnnotationEditor._l10n ??= l10n; + AnnotationEditor._l10nResizer ||= Object.freeze({ topLeft: "pdfjs-editor-resizer-top-left", topMiddle: "pdfjs-editor-resizer-top-middle", @@ -221,21 +223,6 @@ class AnnotationEditor { middleLeft: "pdfjs-editor-resizer-middle-left", }); - AnnotationEditor._l10nPromise ||= new Map([ - ...[ - "pdfjs-editor-alt-text-button-label", - "pdfjs-editor-alt-text-edit-button-label", - "pdfjs-editor-alt-text-decorative-tooltip", - "pdfjs-editor-new-alt-text-added-button-label", - "pdfjs-editor-new-alt-text-missing-button-label", - "pdfjs-editor-new-alt-text-to-review-button-label", - ].map(str => [str, l10n.get(str)]), - ...[ - // Strings that need l10n-arguments. - "pdfjs-editor-new-alt-text-generated-alt-text-with-disclaimer", - ].map(str => [str, l10n.get.bind(l10n, str)]), - ]); - if (AnnotationEditor._borderLineWidth !== -1) { return; } @@ -1003,7 +990,7 @@ class AnnotationEditor { if (this.#altText) { return; } - AltText.initialize(AnnotationEditor._l10nPromise); + AltText.initialize(AnnotationEditor._l10n); this.#altText = new AltText(this); if (this.#accessibilityData) { this.#altText.data = this.#accessibilityData; diff --git a/test/integration/stamp_editor_spec.mjs b/test/integration/stamp_editor_spec.mjs index 1b4aecd1d722f..c8c4275e3c011 100644 --- a/test/integration/stamp_editor_spec.mjs +++ b/test/integration/stamp_editor_spec.mjs @@ -94,6 +94,14 @@ const copyImage = async (page, imagePath, number) => { await waitForImage(page, getEditorSelector(number)); }; +async function waitForTranslation(page) { + return page.evaluate(async () => { + await new Promise(resolve => { + window.requestAnimationFrame(resolve); + }); + }); +} + const switchToStamp = switchToEditor.bind(null, "Stamp"); describe("Stamp Editor", () => { @@ -987,6 +995,7 @@ describe("Stamp Editor", () => { const buttonSelector = `${editorSelector} button.altText.new`; await page.waitForSelector(buttonSelector, { visible: true }); + await waitForTranslation(page); // Check the text in the button. let text = await page.evaluate( sel => document.querySelector(sel).textContent, @@ -1036,6 +1045,7 @@ describe("Stamp Editor", () => { await waitForSelectedEditor(page, editorSelector); await page.waitForSelector(buttonSelector, { visible: true }); + await waitForTranslation(page); // Check the text in the button. text = await page.evaluate( sel => document.querySelector(sel).textContent, @@ -1078,6 +1088,7 @@ describe("Stamp Editor", () => { await page.click("#newAltTextSave"); await page.waitForSelector("#newAltTextDialog", { visible: false }); + await waitForTranslation(page); // Check the text in the button. text = await page.evaluate( sel => document.querySelector(sel).firstChild.textContent,