From 8fb0a31f5aa7bcf7379a04e4981728229fe342cd Mon Sep 17 00:00:00 2001 From: John Thomson Date: Tue, 25 Aug 2026 17:59:56 -0500 Subject: [PATCH] Show the sound's file name, not "A Recording", in the image audio submenu (BL-16669) When a sound is chosen to play when an overlay image or draggable is clicked, the submenu row that plays that sound showed the localized string "A Recording" instead of the chosen file's name, even though the parent menu row shows the name correctly. That row had also lost the check mark showing which sound is currently in effect. The row passed the file name as its englishLabel but also carried l10nId "ARecording", and the localization lookup wins over englishLabel, so the name was never displayed. "A Recording" is the right label for the text case (makeChooseAudioMenuItemForText), where a talking-book recording has no file name to show; the image path picked the id up during the port to the declarative canvas control registry. Before that port the row was built with l10nId null, the file name as its label, and a check mark. Both are restored here. Adds a unit test pinning the labels, plus a guard that a sound file name containing a '%' reaches the label undecoded -- the convention BL-16669 established for these names. The test was verified to fail when the l10nId is put back, so it catches the regression rather than passing vacuously. This required exporting makeChooseAudioMenuItemForImage, since there is no other way to assert these labels without a running Bloom and the canvas e2e suite cannot currently complete a run (BL-16752). Also, unrelated to the bug: repair openCanvasToolTab in the canvas Playwright helpers. It looked for an h3[data-toolid], but the React toolbox now renders tool headers as MUI accordion summaries with the data-toolid on a span, so every canvas e2e test was failing in the fixture before reaching its assertions. The h3 selectors are kept. No localizable strings were added, changed, or removed: the text path still uses ARecording, so its XLF entry stays live. Co-Authored-By: Claude Opus 5 (1M context) --- .../canvas-e2e-tests/helpers/canvasFrames.ts | 5 +- .../toolbox/canvas/canvasControlRegistry.ts | 10 +- .../toolbox/canvas/imageAudioMenuItem.test.ts | 139 ++++++++++++++++++ 3 files changed, 151 insertions(+), 3 deletions(-) create mode 100644 src/BloomBrowserUI/bookEdit/toolbox/canvas/imageAudioMenuItem.test.ts diff --git a/src/BloomBrowserUI/bookEdit/canvas-e2e-tests/helpers/canvasFrames.ts b/src/BloomBrowserUI/bookEdit/canvas-e2e-tests/helpers/canvasFrames.ts index ddc2f42d9e7a..2f5f24156278 100644 --- a/src/BloomBrowserUI/bookEdit/canvas-e2e-tests/helpers/canvasFrames.ts +++ b/src/BloomBrowserUI/bookEdit/canvas-e2e-tests/helpers/canvasFrames.ts @@ -71,9 +71,12 @@ export const openCanvasToolTab = async (toolboxFrame: Frame): Promise => { return; } + // The React toolbox renders each tool's header as a MUI accordion summary + // carrying a data-toolid span (section.id, "canvas" here). The h3 forms are + // the pre-React markup, kept so this still works against an older toolbox. const canvasToolHeader = toolboxFrame .locator( - 'h3[data-toolid="canvasTool"], h3[data-toolid="canvas"], h3[data-toolid*="canvas"], h3:has-text("Canvas")', + '.MuiAccordionSummary-root:has([data-toolid="canvas"]), h3[data-toolid="canvasTool"], h3[data-toolid="canvas"], h3[data-toolid*="canvas"], h3:has-text("Canvas")', ) .first(); diff --git a/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlRegistry.ts b/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlRegistry.ts index c3a7161aec96..36921c9a04e9 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlRegistry.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlRegistry.ts @@ -342,7 +342,10 @@ const makeChooseAudioMenuItemForText = ( }; }; -const makeChooseAudioMenuItemForImage = ( +// Exported for unit testing: the labels on these rows have regressed once (the +// play row showed the localized "A Recording" instead of the sound's file name), +// and this is the only way to assert them without a running Bloom. +export const makeChooseAudioMenuItemForImage = ( ctx: IControlContext, runtime: IControlRuntime, ): IControlMenuCommandRow => { @@ -374,9 +377,12 @@ const makeChooseAudioMenuItemForImage = ( }, { id: "playCurrentAudio", - l10nId: "ARecording", + // Deliberately no l10nId: this row's label is the chosen sound + // file's name, which must not be replaced by a localized string. englishLabel: imageSoundLabel, featureName: "canvas", + // Marks this as the sound currently in effect. + icon: React.createElement(CheckIcon, null), availability: { visible: (itemCtx) => itemCtx.hasCurrentImageSound, }, diff --git a/src/BloomBrowserUI/bookEdit/toolbox/canvas/imageAudioMenuItem.test.ts b/src/BloomBrowserUI/bookEdit/toolbox/canvas/imageAudioMenuItem.test.ts new file mode 100644 index 000000000000..23a7143cf5b9 --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/toolbox/canvas/imageAudioMenuItem.test.ts @@ -0,0 +1,139 @@ +import { describe, expect, test } from "vitest"; + +import { buildCanvasElementControlRegistryContext } from "./buildCanvasElementControlRegistryContext"; +import { makeChooseAudioMenuItemForImage } from "./canvasControlRegistry"; +import { IControlMenuCommandRow } from "./canvasControlTypes"; + +// Tests for the labels on the image "play when touched" menu and its submenu. +// +// The row that plays the currently-chosen sound must be labelled with that sound's +// file name, exactly as the parent row is. It regressed once: the row carried +// l10nId "ARecording" as well as the file name in englishLabel, and because the +// localization lookup wins over englishLabel, every image sound was displayed as +// the localized string "A Recording". "A Recording" is the right label for the +// *text* case, where a talking-book recording has no file name to show. +// +// So the assertion that matters below is about the ABSENCE of an l10nId on that +// row -- an l10nId there is silently authoritative over the label beside it. + +// Build a page with one image canvas element, optionally with a sound attached. +function makeImageCanvasElement(dataSound?: string): HTMLElement { + const page = document.createElement("div"); + page.className = "bloom-page"; + + const canvasElement = document.createElement("div"); + canvasElement.className = "bloom-canvas-element"; + if (dataSound) { + canvasElement.setAttribute("data-sound", dataSound); + } + + const container = document.createElement("div"); + container.className = "bloom-imageContainer"; + container.appendChild(document.createElement("img")); + canvasElement.appendChild(container); + + page.appendChild(canvasElement); + document.body.appendChild(page); + return canvasElement; +} + +// The audio menu item for an image element, built from a real registry context. +function makeAudioMenuItem(dataSound?: string): IControlMenuCommandRow { + const ctx = buildCanvasElementControlRegistryContext( + makeImageCanvasElement(dataSound), + ); + return makeChooseAudioMenuItemForImage(ctx, { closeMenu: () => {} }); +} + +function getSubMenuItem( + parent: IControlMenuCommandRow, + id: string, +): IControlMenuCommandRow { + const found = parent.subMenuItems?.find((item) => item.id === id); + if (!found) { + throw new Error( + `No submenu item with id "${id}". Found: ${parent.subMenuItems + ?.map((item) => item.id) + .join(", ")}`, + ); + } + return found as IControlMenuCommandRow; +} + +describe("image audio menu labels", () => { + test("setup: an element with a sound really is seen as having one", () => { + // If the context ever stops reading data-sound, the assertions below would be + // exercising the no-sound path and would pass for the wrong reason. + const withSound = buildCanvasElementControlRegistryContext( + makeImageCanvasElement("bird.mp3"), + ); + expect(withSound.hasCurrentImageSound).toBe(true); + expect(withSound.currentImageSoundLabel).toBe("bird"); + + const withoutSound = buildCanvasElementControlRegistryContext( + makeImageCanvasElement(), + ); + expect(withoutSound.hasCurrentImageSound).toBe(false); + }); + + test("the play row shows the sound's file name, not a localized string", () => { + const playRow = getSubMenuItem( + makeAudioMenuItem("bird.mp3"), + "playCurrentAudio", + ); + + // The regression: an l10nId here overrides englishLabel, so the file name + // would never be displayed. + expect(playRow.l10nId).toBeUndefined(); + expect(playRow.englishLabel).toBe("bird"); + }); + + test("the play row is marked as the sound currently in effect", () => { + const playRow = getSubMenuItem( + makeAudioMenuItem("bird.mp3"), + "playCurrentAudio", + ); + + expect(playRow.icon).toBeTruthy(); + }); + + test("the play row only appears when a sound is attached", () => { + const visible = getSubMenuItem( + makeAudioMenuItem("bird.mp3"), + "playCurrentAudio", + ).availability?.visible; + if (typeof visible !== "function") { + throw new Error("Expected the play row's visibility to be a rule."); + } + + const withSound = buildCanvasElementControlRegistryContext( + makeImageCanvasElement("bird.mp3"), + ); + const withoutSound = buildCanvasElementControlRegistryContext( + makeImageCanvasElement(), + ); + expect(visible(withSound)).toBe(true); + expect(visible(withoutSound)).toBe(false); + }); + + test("the parent row shows the file name too, and 'None' when there is no sound", () => { + const withSound = makeAudioMenuItem("bird.mp3"); + expect(withSound.l10nId).toBeUndefined(); + expect(withSound.englishLabel).toBe("bird"); + + const withoutSound = makeAudioMenuItem(); + expect(withoutSound.l10nId).toBe("EditTab.Toolbox.DragActivity.None"); + expect(withoutSound.englishLabel).toBe("None"); + }); + + test("a sound whose name contains a '%' is shown as-is", () => { + // Sound file names reach this menu unencoded on purpose (BL-16669); a name + // that looks like a URL escape must not be decoded on its way to the label. + const playRow = getSubMenuItem( + makeAudioMenuItem("clap%41.mp3"), + "playCurrentAudio", + ); + + expect(playRow.englishLabel).toBe("clap%41"); + }); +});