From a71b0285e71cff0866c1524dae629ec6e6a4003f Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Tue, 31 Mar 2026 10:02:22 -0400 Subject: [PATCH 1/9] BL-15300 switch edit tab highlighting to pseudo element --- .../bookEdit/StyleEditor/StyleEditor.ts | 2 +- .../bookEdit/StyleEditor/StyleEditorSpec.ts | 94 +++- .../toolbox/talkingBook/audioRecording.less | 60 +-- .../toolbox/talkingBook/audioRecording.ts | 43 +- .../toolbox/talkingBook/audioRecordingSpec.ts | 279 ++++++++++++ .../talkingBook/audioTextHighlightManager.ts | 407 ++++++++++++++++++ .../bookLayout/basePage-sharedRules.less | 10 +- 7 files changed, 842 insertions(+), 53 deletions(-) create mode 100644 src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts diff --git a/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts b/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts index 257e80cfef2c..2f6bc9af9969 100644 --- a/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts +++ b/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts @@ -695,7 +695,7 @@ export default class StyleEditor { this.sentenceHiliteRuleSelector, false, ); - const hiliteTextColor = sentenceRule?.style?.color; + const hiliteTextColor = sentenceRule?.style?.color || undefined; let hiliteBgColor = sentenceRule?.style?.backgroundColor; if (!hiliteBgColor) { hiliteBgColor = kBloomYellow; diff --git a/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditorSpec.ts b/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditorSpec.ts index cfb8b561bd83..266adcc860ca 100644 --- a/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditorSpec.ts +++ b/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditorSpec.ts @@ -51,14 +51,12 @@ function ChangeSizeAbsolute( editor.changeSizeInternal(newSize + "pt", shouldSetDefaultRule); } -function GetUserModifiedStyleSheet(): any { +function GetUserModifiedStyleSheet(): CSSStyleSheet | undefined { for (let i = 0; i < document.styleSheets.length; i++) { - if (document.styleSheets[i].title == "userModifiedStyles") + if (document.styleSheets[i].title === "userModifiedStyles") return document.styleSheets[i]; } - // this is not a valid constructor - //return new CSSStyleSheet(); - return {}; + return undefined; } function GetFooStyleRuleFontSize(): number { @@ -83,8 +81,8 @@ function ParseRuleForFontSize(ruleText: string): number { } function GetRuleForFooStyle(): CSSRule | null { - const x: CSSRuleList = (GetUserModifiedStyleSheet()) - .cssRules; + const x = GetUserModifiedStyleSheet()?.cssRules; + if (!x) return null; for (let i = 0; i < x.length; i++) { if (x[i].cssText.indexOf("foo-style") > -1) { @@ -95,8 +93,7 @@ function GetRuleForFooStyle(): CSSRule | null { } function GetRuleForNormalStyle(): CSSRule | null { - const x: CSSRuleList = (GetUserModifiedStyleSheet()) - .cssRules; + const x = GetUserModifiedStyleSheet()?.cssRules; if (!x) return null; for (let i = 0; i < x.length; i++) { @@ -108,8 +105,7 @@ function GetRuleForNormalStyle(): CSSRule | null { } function GetRuleForCoverTitleStyle(): CSSRule | null { - const x: CSSRuleList = (GetUserModifiedStyleSheet()) - .cssRules; + const x = GetUserModifiedStyleSheet()?.cssRules; if (!x) return null; for (let i = 0; i < x.length; i++) { if (x[i].cssText.indexOf("Title-On-Cover-style") > -1) { @@ -128,8 +124,9 @@ function GetCalculatedFontSize(target: string): number { } function GetRuleMatchingSelector(selector: string): CSSRule | null { - const x = (GetUserModifiedStyleSheet()).cssRules; - const count = 0; + const sheet = GetUserModifiedStyleSheet(); + const x = sheet?.cssRules; + if (!x) return null; for (let i = 0; i < x.length; i++) { if (x[i].cssText.indexOf(selector) > -1) { return x[i]; @@ -139,7 +136,9 @@ function GetRuleMatchingSelector(selector: string): CSSRule | null { } function HasRuleMatchingThisSelector(selector: string): boolean { - const x = (GetUserModifiedStyleSheet()).cssRules; + const sheet = GetUserModifiedStyleSheet(); + const x = sheet?.cssRules; + if (!x) return false; let count = 0; for (let i = 0; i < x.length; i++) { if (x[i].cssText.indexOf(selector) > -1) { @@ -150,8 +149,8 @@ function HasRuleMatchingThisSelector(selector: string): boolean { } function countFooStyleRules(): number { - const x: CSSRuleList = (GetUserModifiedStyleSheet()) - .cssRules; + const x = GetUserModifiedStyleSheet()?.cssRules; + if (!x) return 0; let count = 0; for (let i = 0; i < x.length; i++) { @@ -320,6 +319,69 @@ describe("StyleEditor", () => { if (rule != null) expect(ParseRuleForFontSize(rule.cssText)).toBe(20); }); + it("putAudioHiliteRulesInDom stores audio highlight legacy properties", () => { + const editor = new StyleEditor( + "file://" + "C:/dev/Bloom/src/BloomBrowserUI/bookEdit", + ); + + const sentenceSelector = "foo-style span.ui-audioCurrent"; + const paddedSentenceSelector = + "foo-style span.ui-audioCurrent > span.ui-enableHighlight"; + const paragraphSelector = "foo-style.ui-audioCurrent p"; + + // sanity check that the rules do not yet exist + expect(HasRuleMatchingThisSelector(sentenceSelector)).toBeFalsy(); + expect(HasRuleMatchingThisSelector(paddedSentenceSelector)).toBeFalsy(); + expect(HasRuleMatchingThisSelector(paragraphSelector)).toBeFalsy(); + + editor.putAudioHiliteRulesInDom( + "foo-style", + "rgb(1, 2, 3)", + "rgb(4, 5, 6)", + ); + + const sentenceRule = GetRuleMatchingSelector(sentenceSelector); + const paddedSentenceRule = GetRuleMatchingSelector( + paddedSentenceSelector, + ); + const paragraphRule = GetRuleMatchingSelector(paragraphSelector); + + expect(sentenceRule?.cssText).toContain( + "background-color: rgb(4, 5, 6)", + ); + expect(sentenceRule?.cssText).toContain("color: rgb(1, 2, 3)"); + expect(paddedSentenceRule?.cssText).toContain( + "background-color: rgb(4, 5, 6)", + ); + expect(paddedSentenceRule?.cssText).toContain("color: rgb(1, 2, 3)"); + expect(paragraphRule?.cssText).toContain( + "background-color: rgb(4, 5, 6)", + ); + expect(paragraphRule?.cssText).toContain("color: rgb(1, 2, 3)"); + }); + + it("getAudioHiliteProps reads colors from audio highlight legacy properties", () => { + const editor = new StyleEditor( + "file://" + "C:/dev/Bloom/src/BloomBrowserUI/bookEdit", + ); + + const origProps = editor.getAudioHiliteProps("foo-style"); + + expect(origProps?.hiliteTextColor).not.toBe("rgb(1, 2, 3)"); + expect(origProps?.hiliteBgColor).not.toBe("rgb(4, 5, 6)"); + + editor.putAudioHiliteRulesInDom( + "foo-style", + "rgb(1, 2, 3)", + "rgb(4, 5, 6)", + ); + + const props = editor.getAudioHiliteProps("foo-style"); + + expect(props.hiliteTextColor).toBe("rgb(1, 2, 3)"); + expect(props.hiliteBgColor).toBe("rgb(4, 5, 6)"); + }); + // Skipped because currently we're running in jsdom. Making use of the existing rule depends on // getComputedStyle, which jsdom does not support. ChatGpt thinks it also depends on actual // dom element sizes, which jsdom also does not support. Attempts to polyfill proved difficult. diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less index b87d9d5ec44e..cfdc292cae14 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less @@ -31,6 +31,19 @@ div.ui-audioCurrent:not(.ui-suppressHighlight):not(.ui-disableHighlight) p { // color: @textOnLightBackground; } +// In the Edit tab, we now use pseudo-elements to display the highlight instead of just background-color nd color, to avoid +// computed styles getting stuck in the book (BL-15300, see audioTextHighlightManager.ts). Here overwrite the highlighting +// colors from basePage.less which are otherwise used to display highlighting (e.g. in Bloom Player), so that only the +// pseudo-elements are displaying the highlights +body.bloom-audio-customHighlights { + span.ui-audioCurrent:not(.ui-suppressHighlight):not(.ui-disableHighlight), + div.ui-audioCurrent:not(.ui-suppressHighlight):not(.ui-disableHighlight) p, + .ui-audioCurrent .ui-enableHighlight { + background-color: transparent !important; + color: inherit !important; + } +} + .bloom-ui-current-audio-marker:before { background-image: url(currentTextIndicator.svg); background-repeat: no-repeat; @@ -76,33 +89,26 @@ div.ui-audioCurrent p { position: unset; // BL-11633, works around Chromium bug } -.ui-audioCurrent.bloom-postAudioSplit[data-audiorecordingmode="TextBox"]:not( - .ui-suppressHighlight - ):not(.ui-disableHighlight) { - // Special highlighting after the Split button completes to show it completed. - // Note: This highlighting is expected to persist across sessions, but to be hidden (displayed with the yellow color) while each segment is playing. - // This is accomplished because this rule temporarily drops out of effect when .ui-audioCurrent is moved to the span as that segment plays. - // (The rule requires a span BELOW the .ui-audioCurrent, so it drops out of effect the span IS the .ui-audioCurrent). - span:nth-child(3n + 1 of .bloom-highlightSegment) { - background-color: #bfedf3; - } - - span:nth-child(3n + 2 of .bloom-highlightSegment) { - background-color: #7fdae6; - } - - span:nth-child(3n + 3 of .bloom-highlightSegment) { - background-color: #29c2d6; - } - - span { - position: unset; // BL-11633, works around Chromium bug - } - - p { - // Override the normal yellow highlight so it doesn't clash with the ones we just added - background: none; - } +// Use pseudoelements to display TBT highlighting in the Edit tab. See audioTextHighlightManager.ts +::highlight(bloom-audio-current) { + // Read from CSS variables so the Format dialog's stored audio highlight colors + // control the normally-yellow recording highlight + background-color: var( + --bloom-audio-current-highlight-background, + @highlightColor + ); + color: var(--bloom-audio-current-highlight-color, @textOnLightBackground); +} +// Note: This highlighting is expected to persist across sessions, but to be hidden +// (displayed with the yellow color) while each segment is playing. +::highlight(bloom-audio-split-1) { + background-color: #bfedf3; +} +::highlight(bloom-audio-split-2) { + background-color: #7fdae6; +} +::highlight(bloom-audio-split-3) { + background-color: #29c2d6; } .ui-audioBody { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts index 3f13b9240437..5c5ef2b4f771 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts @@ -69,6 +69,7 @@ import { } from "../../../react_components/featureStatus"; import { animateStyleName } from "../../../utils/shared"; import jQuery from "jquery"; +import { AudioTextHighlightManager } from "./audioTextHighlightManager"; enum Status { Disabled, // Can't use button now (e.g., Play when there is no recording) @@ -207,6 +208,7 @@ export default class AudioRecording implements IAudioRecorder { private playbackOrderCache: IPlaybackOrderInfo[] = []; private disablingOverlay: HTMLDivElement; + private audioTextHighlightManager = new AudioTextHighlightManager(); constructor(maySetHighlight: boolean = true) { this.audioSplitButton = ( @@ -898,6 +900,10 @@ export default class AudioRecording implements IAudioRecorder { if (pageDocBody) { this.removeAudioCurrent(pageDocBody); } + + this.audioTextHighlightManager.clearAllManagedHighlights( + pageDocBody ?? undefined, + ); } private removeAudioCurrent(parentElement: Element) { @@ -1002,7 +1008,9 @@ export default class AudioRecording implements IAudioRecorder { } if (oldElement === newElement && !forceRedisplay) { - // No need to do much, and better not to so we can avoid any temporary flashes as the highlight is removed and re-applied + // The current element is unchanged, so avoid tearing down and rebuilding anything in the DOM. + // We still need to refresh the custom-highlight pseudoelement state so yellow/split highlights stay in sync without a flash. + this.refreshAudioTextHighlights(newElement); return; } @@ -1075,6 +1083,23 @@ export default class AudioRecording implements IAudioRecorder { ); } } + + this.refreshAudioTextHighlights(newElement); + } + + private refreshAudioTextHighlights(currentHighlight?: Element | null) { + const activeHighlight = currentHighlight ?? this.getCurrentHighlight(); + const currentTextBox = activeHighlight + ? ((this.getTextBoxOfElement( + activeHighlight, + ) as HTMLElement | null) ?? null) + : null; + // The manager keeps both the yellow current highlight and the blue post-split + // highlights in sync so callers do not need separate refresh paths. + this.audioTextHighlightManager.refreshHighlights( + activeHighlight, + currentTextBox, + ); } // Scrolls an element into view. @@ -1172,6 +1197,12 @@ export default class AudioRecording implements IAudioRecorder { } this.resetAudioIfPaused(); + // According to copilot: + // Clear any split-complete state before rebuilding the current highlight. + // Otherwise the custom-highlight logic will still treat the textbox as "post split" + // and suppress the yellow current highlight we want to show as Speak starts. + this.clearAudioSplit(); + // If we were paused highlighting one sentence but are recording in text box mode, // things could get confusing. At least make sure the selection reflects what we // actually want to record. @@ -1187,8 +1218,6 @@ export default class AudioRecording implements IAudioRecorder { const id = this.getCurrentAudioId(); - this.clearAudioSplit(); - return axios .post("/bloom/api/audio/startRecord?id=" + id) .then(() => { @@ -2069,7 +2098,7 @@ export default class AudioRecording implements IAudioRecorder { this.updateDisplay(); } private showPlaybackOrderUi(docBody: HTMLElement) { - this.removeAudioCurrent(docBody); + this.removeAudioCurrentFromPageDocBody(); this.playbackOrderCache = []; const translationGroups = this.getVisibleTranslationGroups(docBody); if (translationGroups.length < 1) { @@ -4412,6 +4441,7 @@ export default class AudioRecording implements IAudioRecorder { const currentTextBox = this.getCurrentTextBox(); if (currentTextBox) { currentTextBox.classList.add("bloom-postAudioSplit"); + this.refreshAudioTextHighlights(currentTextBox); } } @@ -4421,6 +4451,10 @@ export default class AudioRecording implements IAudioRecorder { currentTextBox.classList.remove("bloom-postAudioSplit"); currentTextBox.removeAttribute("data-audioRecordingEndTimes"); } + + this.audioTextHighlightManager.clearSplitHighlights( + currentTextBox ?? undefined, + ); } private getElementsToUpdateForCursor(): (Element | null)[] { @@ -4860,6 +4894,7 @@ export default class AudioRecording implements IAudioRecorder { } }); this.nodesToRestoreAfterPlayEnded.clear(); + this.refreshAudioTextHighlights(); } } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts index 6e7dbe83a6ac..2af5d599eef9 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts @@ -19,6 +19,78 @@ import { RecordingMode } from "./recordingMode"; import axios from "axios"; import $ from "jquery"; import { mockReplies } from "../../../utils/bloomApi"; +import { + currentHighlightName, + splitHighlightNames, +} from "./audioTextHighlightManager"; + +class FakeHighlight { + public ranges: Range[]; + + public constructor(...ranges: Range[]) { + this.ranges = ranges; + } +} + +type FakeHighlightRegistry = Map; +type TestCssWithHighlights = { + highlights?: FakeHighlightRegistry; +}; + +const installCustomHighlightPolyfill = (targetWindow: Window) => { + const targetWindowWithCss = targetWindow as Window & { + CSS?: TestCssWithHighlights; + }; + if (!targetWindowWithCss.CSS) { + targetWindowWithCss.CSS = {}; + } + + const cssWithHighlights = targetWindowWithCss.CSS; + cssWithHighlights.highlights = new Map(); + ( + targetWindow as Window & { + Highlight?: typeof FakeHighlight; + } + ).Highlight = FakeHighlight; +}; + +const getPageWindow = (): Window | undefined => { + const iframe = parent.window.document.getElementById( + "page", + ) as HTMLIFrameElement | null; + return iframe?.contentWindow ?? undefined; +}; + +const getCustomHighlightsRegistry = (): FakeHighlightRegistry => { + const targetWindow = getPageWindow() ?? globalThis.window; + const cssWithHighlights = ( + targetWindow as Window & { + CSS?: TestCssWithHighlights; + } + ).CSS; + if (!cssWithHighlights?.highlights) { + throw new Error( + "Expected CSS.highlights test polyfill to be installed", + ); + } + + return cssWithHighlights.highlights; +}; + +const getSplitHighlightTexts = (): string[][] => { + const registry = getCustomHighlightsRegistry(); + return splitHighlightNames.map((name) => { + const highlight = registry.get(name); + return highlight + ? highlight.ranges.map((range) => range.toString()) + : []; + }); +}; + +const getHighlightTexts = (highlightName: string): string[] => { + const highlight = getCustomHighlightsRegistry().get(highlightName); + return highlight ? highlight.ranges.map((range) => range.toString()) : []; +}; // Notes: // For any async tests: @@ -38,13 +110,21 @@ import { mockReplies } from "../../../utils/bloomApi"; describe("audio recording tests", () => { beforeAll(async () => { + installCustomHighlightPolyfill(globalThis.window); + await setupForAudioRecordingTests(); + + const pageWindow = getPageWindow(); + if (pageWindow) { + installCustomHighlightPolyfill(pageWindow); + } }); afterEach(() => { // Clean up any pending timers to prevent "parent is not defined" errors // when tests finish before timers fire theOneAudioRecorder?.clearTimeouts(); + getCustomHighlightsRegistry().clear(); }); // In an earlier version of our API, checkForAnyRecording was designed to fail (404) if there was no recording. @@ -2166,6 +2246,205 @@ describe("audio recording tests", () => { }); }); }); + + describe("- custom split highlights", () => { + it("registers the current sentence highlight with custom highlights", async () => { + SetupIFrameFromHtml( + '

One.

', + ); + + const recording = new AudioRecording(); + const setHighlightToAsync = ( + recording as unknown as { + setHighlightToAsync(args: { + newElement: Element; + shouldScrollToElement: boolean; + forceRedisplay?: boolean; + }): Promise; + } + ).setHighlightToAsync.bind(recording); + + await setHighlightToAsync({ + newElement: getFrameElementById("page", "span1")!, + shouldScrollToElement: false, + forceRedisplay: true, + }); + + expect(getHighlightTexts(currentHighlightName)).toEqual(["One."]); + }); + + it("clears custom highlights when entering show playback order mode", async () => { + SetupIFrameFromHtml( + '

One.

', + ); + + const recording = new AudioRecording(); + const setHighlightToAsync = ( + recording as unknown as { + setHighlightToAsync(args: { + newElement: Element; + shouldScrollToElement: boolean; + forceRedisplay?: boolean; + }): Promise; + } + ).setHighlightToAsync.bind(recording); + + await setHighlightToAsync({ + newElement: getFrameElementById("page", "span1")!, + shouldScrollToElement: false, + forceRedisplay: true, + }); + + expect(getHighlightTexts(currentHighlightName)).toEqual(["One."]); + + await recording.setShowPlaybackOrderMode(true); + + expect(getHighlightTexts(currentHighlightName)).toEqual([]); + expect(getSplitHighlightTexts()).toEqual([[], [], []]); + }); + + it("uses only ui-enableHighlight descendants for current highlight when its own background is disabled", async () => { + SetupIFrameFromHtml( + '

One   Two

', + ); + + const recording = new AudioRecording(); + const setHighlightToAsync = ( + recording as unknown as { + setHighlightToAsync(args: { + newElement: Element; + shouldScrollToElement: boolean; + forceRedisplay?: boolean; + }): Promise; + } + ).setHighlightToAsync.bind(recording); + + await setHighlightToAsync({ + newElement: getFrameElementById("page", "span1")!, + shouldScrollToElement: false, + forceRedisplay: true, + }); + + expect(getHighlightTexts(currentHighlightName)).toEqual([ + "One", + "Two", + ]); + }); + + it("copies the current highlight colors from computed highlight styles", async () => { + SetupIFrameFromHtml( + '

One.

', + ); + + const recording = new AudioRecording(); + const setHighlightToAsync = ( + recording as unknown as { + setHighlightToAsync(args: { + newElement: Element; + shouldScrollToElement: boolean; + forceRedisplay?: boolean; + }): Promise; + } + ).setHighlightToAsync.bind(recording); + + await setHighlightToAsync({ + newElement: getFrameElementById("page", "span1")!, + shouldScrollToElement: false, + forceRedisplay: true, + }); + + const pageDocument = ( + parent.window.document.getElementById( + "page", + ) as HTMLIFrameElement + ).contentDocument!; + const documentStyle = pageDocument.documentElement.style; + expect( + documentStyle.getPropertyValue( + "--bloom-audio-current-highlight-background", + ), + ).toBe("rgb(1, 2, 3)"); + expect( + documentStyle.getPropertyValue( + "--bloom-audio-current-highlight-color", + ), + ).toBe("rgb(4, 5, 6)"); + }); + + it("registers the split highlight color groups for the current text box", () => { + SetupIFrameFromHtml( + '

One.Two.Three.Four.

', + ); + + const recording = new AudioRecording(); + recording.markAudioSplit(); + + expect(getSplitHighlightTexts()).toEqual([ + ["One.", "Four."], + ["Two."], + ["Three."], + ]); + }); + + it("clears split highlights while playback moves to an individual segment", async () => { + SetupIFrameFromHtml( + '

One.Two.

', + ); + + const recording = new AudioRecording(); + recording.markAudioSplit(); + + const setHighlightToAsync = ( + recording as unknown as { + setHighlightToAsync(args: { + newElement: Element; + shouldScrollToElement: boolean; + }): Promise; + } + ).setHighlightToAsync.bind(recording); + + await setHighlightToAsync({ + newElement: getFrameElementById("page", "span1")!, + shouldScrollToElement: false, + }); + + expect(getSplitHighlightTexts()).toEqual([[], [], []]); + }); + + it("keeps the current highlight when Speak clears a previous split state", async () => { + SetupIFrameFromHtml( + '

One.Two.

', + ); + + vi.spyOn(axios, "post").mockResolvedValue({}); + + const recording = new AudioRecording(); + recording.recordingMode = RecordingMode.TextBox; + document.getElementById("audio-record")?.classList.add("expected"); + + await recording.startRecordCurrentAsync(); + + expect(getHighlightTexts(currentHighlightName).join("")).toContain( + "One.", + ); + expect(getSplitHighlightTexts()).toEqual([[], [], []]); + }); + + it("uses only ui-enableHighlight descendants when a segment disables its own background", () => { + SetupIFrameFromHtml( + '

One.Two   Three

', + ); + + const recording = new AudioRecording(); + recording.markAudioSplit(); + + expect(getSplitHighlightTexts()).toEqual([ + ["One."], + ["Two", "Three"], + [], + ]); + }); + }); }); function StripEmptyClasses(html) { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts new file mode 100644 index 000000000000..41866abf330c --- /dev/null +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts @@ -0,0 +1,407 @@ +const kSegmentClass = "bloom-highlightSegment"; +const kEnableHighlightClass = "ui-enableHighlight"; +const kSuppressHighlightClass = "ui-suppressHighlight"; +const kDisableHighlightClass = "ui-disableHighlight"; +const kPostAudioSplitClass = "bloom-postAudioSplit"; +const kTextBoxRecordingMode = "textbox"; +const kCustomHighlightSupportClass = "bloom-audio-customHighlights"; + +const kCurrentHighlightBackgroundCssVar = + "--bloom-audio-current-highlight-background"; +const kCurrentHighlightColorCssVar = "--bloom-audio-current-highlight-color"; + +// This manager translates Bloom's audio-highlight classes into the CSS Custom Highlight API. +// In rare cases, the browser can automatically move computed css into an inline style within a contenteditable, which +// we suspect is causing BL-15300 where TBT highlighting gets stuck in the book. This method of highlighting without +// modifying the dom should prevent that, and is also the direction we want to move in for highlighting. +// The DOM still decides which pieces of text are eligible and marks them with the appropriate classes, but in the Edit +// Tab the visible paint comes from ::highlight pseudo-elements instead of the element +// background colors, which we continue to use in Bloom Player etc. + +export const currentHighlightName = "bloom-audio-current"; + +export const splitHighlightNames = [ + "bloom-audio-split-1", + "bloom-audio-split-2", + "bloom-audio-split-3", +] as const; + +const allManagedHighlightNames = [currentHighlightName, ...splitHighlightNames]; + +type HighlightRegistry = Map; +type HighlightConstructor = new (...ranges: Range[]) => unknown; + +function getDocumentWindow(contextNode: Node): Window | undefined { + return contextNode.ownerDocument?.defaultView ?? undefined; +} + +function getDocumentElement(contextNode: Node): HTMLElement | undefined { + return contextNode.ownerDocument?.documentElement ?? undefined; +} + +function getDocumentBody(contextNode: Node): HTMLElement | undefined { + return contextNode.ownerDocument?.body ?? undefined; +} + +function getHighlightRegistry( + contextNode: Node, +): HighlightRegistry | undefined { + const docWindow = getDocumentWindow(contextNode) as + | (Window & typeof globalThis) + | undefined; + const cssWithHighlights = docWindow?.CSS as + | (typeof globalThis.CSS & { + highlights?: HighlightRegistry; + }) + | undefined; + return cssWithHighlights?.highlights; +} + +function getHighlightConstructor( + contextNode: Node, +): HighlightConstructor | undefined { + const docWindow = getDocumentWindow(contextNode) as + | (Window & { + Highlight?: HighlightConstructor; + }) + | undefined; + return docWindow?.Highlight; +} + +export class AudioTextHighlightManager { + public clearAllManagedHighlights(contextNode?: Node): void { + if (!contextNode) { + return; + } + + const registry = getHighlightRegistry(contextNode); + if (!registry) { + return; + } + + allManagedHighlightNames.forEach((name) => registry.delete(name)); + } + + public clearSplitHighlights(contextNode?: Node): void { + if (!contextNode) { + return; + } + + const registry = getHighlightRegistry(contextNode); + if (!registry) { + return; + } + + splitHighlightNames.forEach((name) => registry.delete(name)); + } + + public refreshHighlights( + currentHighlight: Element | null, + currentTextBox: HTMLElement | null, + ): void { + const contextNode = currentHighlight ?? currentTextBox; + if (!contextNode) { + return; + } + + if ( + !getHighlightRegistry(contextNode) || + !getHighlightConstructor(contextNode) + ) { + return; + } + + // Once custom highlights are active, suppress the old element background rules so we don't double-highlight + getDocumentBody(contextNode)?.classList.add( + kCustomHighlightSupportClass, + ); + + this.refreshCurrentHighlight(currentHighlight, currentTextBox); + this.refreshSplitHighlights(currentHighlight, currentTextBox); + } + + private refreshCurrentHighlight( + currentHighlight: Element | null, + currentTextBox: HTMLElement | null, + ): void { + const contextNode = currentHighlight ?? currentTextBox; + if (!contextNode) { + console.error( + "AudioTextHighlightManager.refreshCurrentHighlight() was called without a context node.", + ); + return; + } + + const registry = getHighlightRegistry(contextNode); + const Highlight = getHighlightConstructor(contextNode); + // copilot wants belt-and-suspenders, fine + if (!registry || !Highlight) { + console.error( + "AudioTextHighlightManager.refreshCurrentHighlight() lost custom highlight support after refreshHighlights() verified it.", + ); + return; + } + + // The split-complete blue state replaces the yellow current highlight while it is visible, + // so clear the yellow registry entry instead of letting the two overlap. + if (this.shouldShowSplitHighlights(currentHighlight, currentTextBox)) { + registry.delete(currentHighlightName); + return; + } + + const highlightInfo = this.getCurrentHighlightInfo( + currentHighlight, + currentTextBox, + ); + if (!highlightInfo || highlightInfo.ranges.length === 0) { + registry.delete(currentHighlightName); + return; + } + + // enhance: don't check for highlight color settings changes so often + this.updateCurrentHighlightColors(highlightInfo.styleSource); + registry.set( + currentHighlightName, + new Highlight(...highlightInfo.ranges), + ); + } + + private refreshSplitHighlights( + currentHighlight: Element | null, + currentTextBox: HTMLElement | null, + ): void { + const contextNode = currentHighlight ?? currentTextBox; + if (!contextNode) { + console.error( + "AudioTextHighlightManager.refreshSplitHighlights() was called without a context node.", + ); + return; + } + + if (!this.shouldShowSplitHighlights(currentHighlight, currentTextBox)) { + this.clearSplitHighlights(contextNode); + return; + } + + const registry = getHighlightRegistry(contextNode); + const Highlight = getHighlightConstructor(contextNode); + if (!registry || !Highlight) { + console.error( + "AudioTextHighlightManager.refreshSplitHighlights() lost custom highlight support after refreshHighlights() verified it.", + ); + return; + } + + // We cycle through 3 colors for split highlights + const rangesByName = new Map(); + splitHighlightNames.forEach((name) => rangesByName.set(name, [])); + + const segmentGroups = new Map(); + Array.from( + currentTextBox.querySelectorAll(`span.${kSegmentClass}`), + ).forEach((segment) => { + const parent = segment.parentElement; + if (!parent) { + // won't happen + return; + } + + const segments = segmentGroups.get(parent) ?? []; + segments.push(segment); + segmentGroups.set(parent, segments); + }); + + segmentGroups.forEach((segments) => { + segments.forEach((segment, index) => { + const highlightName = + splitHighlightNames[index % splitHighlightNames.length]; + const ranges = rangesByName.get(highlightName); + ranges?.push(...this.getRangesForSegment(segment)); + }); + }); + + splitHighlightNames.forEach((name) => { + const ranges = rangesByName.get(name) ?? []; + if (ranges.length > 0) { + registry.set(name, new Highlight(...ranges)); + } else { + registry.delete(name); + } + }); + } + + private getCurrentHighlightInfo( + currentHighlight: Element | null, + currentTextBox: HTMLElement | null, + ): + | { + ranges: Range[]; + styleSource: Element; + } + | undefined { + if (!currentHighlight) { + return undefined; + } + + if (currentHighlight.classList.contains(kSuppressHighlightClass)) { + return undefined; + } + + // copilot says: fixHighlighting() can carve the visible pieces into nested ui-enableHighlight + // spans so punctuation or outer whitespace stays unpainted. Prefer those exact + // spans whenever they exist so the pseudo-highlight matches the background-color highlight behavior + const enabledDescendants = Array.from( + currentHighlight.querySelectorAll(`span.${kEnableHighlightClass}`), + ); + const enabledRanges = enabledDescendants + .map((enabledSpan) => this.makeRange(enabledSpan)) + .filter((range): range is Range => !!range); + if (enabledRanges.length > 0) { + return { + ranges: enabledRanges, + styleSource: enabledDescendants[0], + }; + } + + if (currentHighlight.classList.contains(kDisableHighlightClass)) { + return undefined; + } + + if (currentHighlight === currentTextBox) { + const paragraphs = Array.from(currentTextBox.querySelectorAll("p")); + const paragraphRanges = paragraphs + .map((paragraph) => this.makeRange(paragraph)) + .filter((range): range is Range => !!range); + if (paragraphRanges.length > 0) { + return { + ranges: paragraphRanges, + styleSource: paragraphs[0], + }; + } + } + + const wholeElementRange = this.makeRange(currentHighlight); + if (!wholeElementRange) { + return undefined; + } + + return { + ranges: [wholeElementRange], + styleSource: currentHighlight, + }; + } + + private updateCurrentHighlightColors(styleSource: Element): void { + const documentElement = getDocumentElement(styleSource); + if (!documentElement) { + console.error( + "AudioTextHighlightManager.updateCurrentHighlightColors() could not find documentElement for the style source.", + ); + return; + } + + // The actual colors still come from CSS so user-modified highlight colors keep working. + // We copy them into document-level variables that the ::highlight rule can read. + const documentBody = getDocumentBody(styleSource); + const hadCustomHighlightOverride = documentBody?.classList.contains( + kCustomHighlightSupportClass, + ); + + // We have rules in audioRecording.less to override the normal highlighting background-color + // (make it transparent), so it isn't underneath + // the pseudo-element highlights which we use instead in the edit tab. Remove that rule, detect the otherwise + // present computed highlight color, and then put the rule back. + if (hadCustomHighlightOverride) { + documentBody?.classList.remove(kCustomHighlightSupportClass); + } + const computedStyle = + getDocumentWindow(styleSource)?.getComputedStyle(styleSource); + const backgroundColor = computedStyle?.backgroundColor; + const color = computedStyle?.color; + if (hadCustomHighlightOverride) { + documentBody?.classList.add(kCustomHighlightSupportClass); + } + + if (backgroundColor) { + documentElement.style.setProperty( + kCurrentHighlightBackgroundCssVar, + backgroundColor, + ); + } else { + documentElement.style.removeProperty( + kCurrentHighlightBackgroundCssVar, + ); + } + + if (color) { + documentElement.style.setProperty( + kCurrentHighlightColorCssVar, + color, + ); + } else { + documentElement.style.removeProperty(kCurrentHighlightColorCssVar); + } + } + + private shouldShowSplitHighlights( + currentHighlight: Element | null, + currentTextBox: HTMLElement | null, + ): currentTextBox is HTMLElement { + if (!currentHighlight || !currentTextBox) { + return false; + } + + if (currentHighlight !== currentTextBox) { + return false; + } + + if ( + currentTextBox.classList.contains(kSuppressHighlightClass) || + currentTextBox.classList.contains(kDisableHighlightClass) + ) { + return false; + } + + // Split highlights are only for textbox recordings after AudioRecording has + // split the textbox into segment spans and marked it as post-split. + return ( + currentTextBox.classList.contains(kPostAudioSplitClass) && + currentTextBox + .getAttribute("data-audiorecordingmode") + ?.toLowerCase() === kTextBoxRecordingMode + ); + } + + private getRangesForSegment(segment: Element): Range[] { + const enabledRanges = Array.from( + segment.querySelectorAll(`span.${kEnableHighlightClass}`), + ) + .map((enabledSpan) => this.makeRange(enabledSpan)) + .filter((range): range is Range => !!range); + + if (enabledRanges.length > 0) { + return enabledRanges; + } + + const wholeSegmentRange = this.makeRange(segment); + return wholeSegmentRange ? [wholeSegmentRange] : []; + } + + private makeRange(node: Node): Range | undefined { + if (node.textContent === null || node.textContent.length === 0) { + return undefined; + } + + const ownerDocument = node.ownerDocument; + if (!ownerDocument) { + console.error( + "AudioTextHighlightManager.makeRange() could not find ownerDocument for a highlighted node.", + ); + return undefined; + } + + const range = ownerDocument.createRange(); + range.selectNodeContents(node); + return range; + } +} diff --git a/src/content/bookLayout/basePage-sharedRules.less b/src/content/bookLayout/basePage-sharedRules.less index 5ee951ca6dc7..395f1862d61e 100644 --- a/src/content/bookLayout/basePage-sharedRules.less +++ b/src/content/bookLayout/basePage-sharedRules.less @@ -15,19 +15,19 @@ body { // Bloom applies in the talking-book tool. Accordingly, we tell the agent to // apply that class. However, the user may NOT have configured a custom highlighting. // In case they do not, this provides the default highlighting. In this location, -// it works for edit mode, bloomPubs, both kinds of ePUBs, and very likely anywhere -// else we'd want ui-audioCurrent to produce a highlight. +// it works for bloomPubs and both kinds of ePUBs, and very likely anywhere +// else we'd want ui-audioCurrent to produce a highlight. (As of March 2026, in the +// edit tab we are using highlighting pseudo-elements instead, BL-15300. Those read +// the computed highlight colors and copy them to document-level variables.) // Setting the foreground color is usually redundant, but digital comic book covers // use white text, and it's now possible for the user to choose a text color, so // we want to ensure a contrast with the background color (unless the user overrides, // in which case, adequate contrast is up to them). // (Color is @bloom-yellow.) -// (When editing the text, this background causes problems (due to a Chromium bug) -// if the elements have their default position: relative. This is overidden in -// audioRecording.less.) span.ui-audioCurrent, div.ui-audioCurrent p, .ui-audioCurrent .ui-enableHighlight { + // Default audio highlight colors background-color: #febf00; color: black; } From a946e440c7b2a9262215aec85bd84dbc88b38c28 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Wed, 8 Apr 2026 13:47:13 -0400 Subject: [PATCH 2/9] code review, renaming etc. --- .../toolbox/talkingBook/audioRecording.less | 2 +- .../toolbox/talkingBook/audioRecordingSpec.ts | 20 ++++++------- .../talkingBook/audioTextHighlightManager.ts | 29 ++++++++++--------- 3 files changed, 27 insertions(+), 24 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less index cfdc292cae14..b52a10fcce14 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less @@ -35,7 +35,7 @@ div.ui-audioCurrent:not(.ui-suppressHighlight):not(.ui-disableHighlight) p { // computed styles getting stuck in the book (BL-15300, see audioTextHighlightManager.ts). Here overwrite the highlighting // colors from basePage.less which are otherwise used to display highlighting (e.g. in Bloom Player), so that only the // pseudo-elements are displaying the highlights -body.bloom-audio-customHighlights { +body.bloom-audio-pseudoHighlights { span.ui-audioCurrent:not(.ui-suppressHighlight):not(.ui-disableHighlight), div.ui-audioCurrent:not(.ui-suppressHighlight):not(.ui-disableHighlight) p, .ui-audioCurrent .ui-enableHighlight { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts index 2af5d599eef9..88a672d79bcd 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts @@ -37,7 +37,7 @@ type TestCssWithHighlights = { highlights?: FakeHighlightRegistry; }; -const installCustomHighlightPolyfill = (targetWindow: Window) => { +const installPseudoHighlightPolyfill = (targetWindow: Window) => { const targetWindowWithCss = targetWindow as Window & { CSS?: TestCssWithHighlights; }; @@ -61,7 +61,7 @@ const getPageWindow = (): Window | undefined => { return iframe?.contentWindow ?? undefined; }; -const getCustomHighlightsRegistry = (): FakeHighlightRegistry => { +const getPseudoHighlightsRegistry = (): FakeHighlightRegistry => { const targetWindow = getPageWindow() ?? globalThis.window; const cssWithHighlights = ( targetWindow as Window & { @@ -78,7 +78,7 @@ const getCustomHighlightsRegistry = (): FakeHighlightRegistry => { }; const getSplitHighlightTexts = (): string[][] => { - const registry = getCustomHighlightsRegistry(); + const registry = getPseudoHighlightsRegistry(); return splitHighlightNames.map((name) => { const highlight = registry.get(name); return highlight @@ -88,7 +88,7 @@ const getSplitHighlightTexts = (): string[][] => { }; const getHighlightTexts = (highlightName: string): string[] => { - const highlight = getCustomHighlightsRegistry().get(highlightName); + const highlight = getPseudoHighlightsRegistry().get(highlightName); return highlight ? highlight.ranges.map((range) => range.toString()) : []; }; @@ -110,13 +110,13 @@ const getHighlightTexts = (highlightName: string): string[] => { describe("audio recording tests", () => { beforeAll(async () => { - installCustomHighlightPolyfill(globalThis.window); + installPseudoHighlightPolyfill(globalThis.window); await setupForAudioRecordingTests(); const pageWindow = getPageWindow(); if (pageWindow) { - installCustomHighlightPolyfill(pageWindow); + installPseudoHighlightPolyfill(pageWindow); } }); @@ -124,7 +124,7 @@ describe("audio recording tests", () => { // Clean up any pending timers to prevent "parent is not defined" errors // when tests finish before timers fire theOneAudioRecorder?.clearTimeouts(); - getCustomHighlightsRegistry().clear(); + getPseudoHighlightsRegistry().clear(); }); // In an earlier version of our API, checkForAnyRecording was designed to fail (404) if there was no recording. @@ -2247,8 +2247,8 @@ describe("audio recording tests", () => { }); }); - describe("- custom split highlights", () => { - it("registers the current sentence highlight with custom highlights", async () => { + describe("- pseudo-element split highlights", () => { + it("registers the current sentence highlight with pseudo-element highlights", async () => { SetupIFrameFromHtml( '

One.

', ); @@ -2273,7 +2273,7 @@ describe("audio recording tests", () => { expect(getHighlightTexts(currentHighlightName)).toEqual(["One."]); }); - it("clears custom highlights when entering show playback order mode", async () => { + it("clears pseudo-element highlights when entering show playback order mode", async () => { SetupIFrameFromHtml( '

One.

', ); diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts index 41866abf330c..1e8c3367d446 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts @@ -4,19 +4,22 @@ const kSuppressHighlightClass = "ui-suppressHighlight"; const kDisableHighlightClass = "ui-disableHighlight"; const kPostAudioSplitClass = "bloom-postAudioSplit"; const kTextBoxRecordingMode = "textbox"; -const kCustomHighlightSupportClass = "bloom-audio-customHighlights"; +const kPseudoHighlightSupportClass = "bloom-audio-pseudoHighlights"; const kCurrentHighlightBackgroundCssVar = "--bloom-audio-current-highlight-background"; const kCurrentHighlightColorCssVar = "--bloom-audio-current-highlight-color"; -// This manager translates Bloom's audio-highlight classes into the CSS Custom Highlight API. +// This manager translates Bloom's audio-highlight classes into the CSS highlight registry and +// ::highlight pseudo-elements. // In rare cases, the browser can automatically move computed css into an inline style within a contenteditable, which // we suspect is causing BL-15300 where TBT highlighting gets stuck in the book. This method of highlighting without // modifying the dom should prevent that, and is also the direction we want to move in for highlighting. // The DOM still decides which pieces of text are eligible and marks them with the appropriate classes, but in the Edit // Tab the visible paint comes from ::highlight pseudo-elements instead of the element -// background colors, which we continue to use in Bloom Player etc. +// background colors, which we continue to use in Bloom Player etc. - we will need to keep the original class and css +// rules for a while so that old versions of Bloom player display the highlights, but a next step would be to make +// newer versions of Bloom Player switch to using pseudo-elements to display highlights like we do here export const currentHighlightName = "bloom-audio-current"; @@ -111,9 +114,9 @@ export class AudioTextHighlightManager { return; } - // Once custom highlights are active, suppress the old element background rules so we don't double-highlight + // Once pseudo-element highlights are active, suppress the old element background rules so we don't double-highlight getDocumentBody(contextNode)?.classList.add( - kCustomHighlightSupportClass, + kPseudoHighlightSupportClass, ); this.refreshCurrentHighlight(currentHighlight, currentTextBox); @@ -137,7 +140,7 @@ export class AudioTextHighlightManager { // copilot wants belt-and-suspenders, fine if (!registry || !Highlight) { console.error( - "AudioTextHighlightManager.refreshCurrentHighlight() lost custom highlight support after refreshHighlights() verified it.", + "AudioTextHighlightManager.refreshCurrentHighlight() lost pseudo-highlight support after refreshHighlights() verified it.", ); return; } @@ -187,7 +190,7 @@ export class AudioTextHighlightManager { const Highlight = getHighlightConstructor(contextNode); if (!registry || !Highlight) { console.error( - "AudioTextHighlightManager.refreshSplitHighlights() lost custom highlight support after refreshHighlights() verified it.", + "AudioTextHighlightManager.refreshSplitHighlights() lost pseudo-highlight support after refreshHighlights() verified it.", ); return; } @@ -303,23 +306,23 @@ export class AudioTextHighlightManager { // The actual colors still come from CSS so user-modified highlight colors keep working. // We copy them into document-level variables that the ::highlight rule can read. const documentBody = getDocumentBody(styleSource); - const hadCustomHighlightOverride = documentBody?.classList.contains( - kCustomHighlightSupportClass, + const hadPseudoHighlightOverride = documentBody?.classList.contains( + kPseudoHighlightSupportClass, ); // We have rules in audioRecording.less to override the normal highlighting background-color // (make it transparent), so it isn't underneath // the pseudo-element highlights which we use instead in the edit tab. Remove that rule, detect the otherwise // present computed highlight color, and then put the rule back. - if (hadCustomHighlightOverride) { - documentBody?.classList.remove(kCustomHighlightSupportClass); + if (hadPseudoHighlightOverride) { + documentBody?.classList.remove(kPseudoHighlightSupportClass); } const computedStyle = getDocumentWindow(styleSource)?.getComputedStyle(styleSource); const backgroundColor = computedStyle?.backgroundColor; const color = computedStyle?.color; - if (hadCustomHighlightOverride) { - documentBody?.classList.add(kCustomHighlightSupportClass); + if (hadPseudoHighlightOverride) { + documentBody?.classList.add(kPseudoHighlightSupportClass); } if (backgroundColor) { From d3880763acd291804ff8f905feaa699f45a54a9f Mon Sep 17 00:00:00 2001 From: John Thomson Date: Fri, 26 Jun 2026 15:42:46 -0500 Subject: [PATCH 3/9] More work on highlight (BL-15300) Completely removed the need for Bloom editing to set a class on the highlighted element. --- src/BloomBrowserUI/bookEdit/css/editMode.less | 47 +++-- .../talkingBook/AdjustTimingsDialog.tsx | 41 ++-- .../toolbox/talkingBook/audioRecording.less | 69 +------ .../toolbox/talkingBook/audioRecording.ts | 195 +++++++++--------- .../talkingBook/audioTextHighlightManager.ts | 97 +++++---- src/BloomBrowserUI/bookEdit/workspaceRoot.ts | 1 + 6 files changed, 212 insertions(+), 238 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/css/editMode.less b/src/BloomBrowserUI/bookEdit/css/editMode.less index 0732fca8181a..1a5b45c6d8d6 100644 --- a/src/BloomBrowserUI/bookEdit/css/editMode.less +++ b/src/BloomBrowserUI/bookEdit/css/editMode.less @@ -1036,15 +1036,31 @@ even when the div is focused (as long as it is empty).*/ color: @bloom-buff; } +// BL-11633: Works around a Chromium bug where elements with position:relative and a +// background color cause the caret to disappear. We unset position on the element +// itself and on any inline text-formatting children, since the bug can trigger at +// any level of nesting. +.bl11633-unset-position() { + position: unset; + em, + i, + strong, + span[style], + u, + sup { + position: unset; + } +} + /*This block handles marking elements that violate decodable book and leveled reader constraints*/ span.sentence-too-long { background-color: @LeveledReaderViolationColor; - position: unset; // BL-11633, works around Chromium bug + .bl11633-unset-position(); } span.word-too-long { background-color: @bloom-lightblue; - position: unset; // BL-11633, works around Chromium bug + .bl11633-unset-position(); } .page-too-many-words-or-sentences .marginBox { @@ -1068,21 +1084,20 @@ span.sight-word { span.word-not-found { background-color: @DecodableReaderViolationColor; - position: unset; // BL-11633, works around Chromium bug + .bl11633-unset-position(); } div.bloom-editable { - // Chrome 105 and later (ie, WebView2) has the :has selector. - // These rules may not need the :has(span) selector, but I - // don't want to risk breaking something. This is enough to - // work around the Chromium bug reported in BL-11633. - span[style]:has(span), - em:has(span), - strong:has(span), - u:has(span), - sup:has(span) { - position: unset; // BL-11633, works around Chromium bug - } + .bl11633-unset-position(); +} + +// p elements can be the ::highlight range in text-box recording mode; they need position:unset +// for the same BL-11633 reason (caret hidden behind a background-colored ancestor). +// Sentence spans are the ::highlight range in sentence mode, so they need it too. +div.bloom-editable p, +div.bloom-editable p > .audio-sentence, +div.bloom-editable p > .bloom-highlightSegment { + .bl11633-unset-position(); } /* We are disabling the "Possible Word" feature at this time. @@ -1225,10 +1240,10 @@ body { // and a background color cause the caret not to show up. Somehow, this // triggers when a decodable reader error span is nested inside one of these // spans, even though when DR is active the rules that give these segments -// background color are not active. Nothing depends on thei being position:relative, +// background color are not active. Nothing depends on their being position:relative, // so it's easiest to just prevent it. See BL-11633 for reproduction steps in Bloom 5.5. .bloom-highlightSegment { - position: unset; + .bl11633-unset-position(); } // Don't show text over picture borders, resizing handles, or format buttons unless hovering over the image diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/AdjustTimingsDialog.tsx b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/AdjustTimingsDialog.tsx index b5a5d5794f39..31ee121ce124 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/AdjustTimingsDialog.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/AdjustTimingsDialog.tsx @@ -20,7 +20,6 @@ import { } from "../../../react_components/BloomDialog/commonDialogComponents"; import { useL10n } from "../../../react_components/l10nHooks"; import { getAsync, postBoolean, postJsonAsync } from "../../../utils/bloomApi"; -import { kAudioCurrent } from "./audioRecording"; import { AdjustTimingsControl, TimedTextSegment } from "./AdjustTimingsControl"; import { getUrlPrefixFromWindowHref, @@ -40,6 +39,7 @@ const timingsMenuId = "timingsMenuAnchor"; export const AdjustTimingsDialog: React.FunctionComponent<{ dialogEnvironment?: IBloomDialogEnvironmentParams; + currentTextBox: HTMLElement; split: (timingFilePath: string) => Promise; editTimingsFile: (timingsFilePath?: string) => Promise; applyTimingsFile: (timingsFilePath?: string) => Promise; @@ -154,7 +154,7 @@ export const AdjustTimingsDialog: React.FunctionComponent<{ // if we have one. This really wants to not happen again, since it would discard any changes // the user has made. setAudioRecordingEndTimes( - getCurrentTextBox()?.getAttribute("data-audiorecordingendtimes"), + props.currentTextBox?.getAttribute("data-audiorecordingendtimes"), ); // This is supposed to execute exactly once, when the dialog is first opened. // React insists it must have this dependency, even though I set up a useCallback @@ -167,7 +167,7 @@ export const AdjustTimingsDialog: React.FunctionComponent<{ // will be passed to the control to tell it to fine tune the segments based on the audio. // gets set back to false when the control sends us the adjusted times. setShouldAdjustSegments(true); - const bloomEditable = getCurrentTextBox(); + const bloomEditable = props.currentTextBox; const segmentElements = Array.from( bloomEditable.getElementsByClassName(kHighlightSegmentClass), @@ -180,9 +180,9 @@ export const AdjustTimingsDialog: React.FunctionComponent<{ React.useEffect(() => { if (!propsForBloomDialog.open) return; - const bloomEditable = getCurrentTextBox(); + const bloomEditable = props.currentTextBox; async function getTimingsFileData() { - setTimingsFilePath(await getTimingsFileName()); + setTimingsFilePath(await getTimingsFileName(props.currentTextBox)); } getTimingsFileData(); const ff = ( @@ -361,7 +361,11 @@ export const AdjustTimingsDialog: React.FunctionComponent<{ l10nId="EditTab.Toolbox.TalkingBookTool.EditTimingsFile" onClick={() => { closeMoreMenu(); - exportTimingsFile(timingsFilePath!, endTimes); + exportTimingsFile( + timingsFilePath!, + endTimes, + props.currentTextBox, + ); props.editTimingsFile(timingsFilePath); }} icon={} @@ -387,9 +391,9 @@ export const AdjustTimingsDialog: React.FunctionComponent<{ { - // Update the data-audiorecordingendtimes attribute in the getCurrentTextBox() div to match + // Update the data-audiorecordingendtimes attribute in the props.currentTextBox div to match // the current state of the adjustments. - const bloomEditable = getCurrentTextBox(); + const bloomEditable = props.currentTextBox; bloomEditable.setAttribute( "data-audiorecordingendtimes", endTimes.join(" "), @@ -412,6 +416,7 @@ let show: () => void = () => { }; export function showAdjustTimingsDialog( + currentTextBox: HTMLElement, split: (timingFilePath: string) => Promise, editTimingsFile: (timingsFilePath?: string) => Promise, applyTimingsFile: (timingsFilePath?: string) => Promise, @@ -420,6 +425,7 @@ export function showAdjustTimingsDialog( try { renderRootSync( { - const bloomEditable = getCurrentTextBox(); +async function getTimingsFileName( + currentTextBox: HTMLElement, +): Promise { + const bloomEditable = currentTextBox; const fileName = `audio/${bloomEditable.getAttribute("id")}_timings.txt`; // id should be a guid, so should not need encoding. const result = await postJsonAsync("fileIO/completeRelativePath?", { @@ -514,8 +512,9 @@ const computeSegments = ( const exportTimingsFile = async ( timingsFileName: string, endTimes: number[], + currentTextBox: HTMLElement, ) => { - const bloomEditable = getCurrentTextBox(); + const bloomEditable = currentTextBox; const segmentElements = Array.from( bloomEditable.getElementsByClassName(kHighlightSegmentClass), ) as HTMLElement[]; diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less index b52a10fcce14..8de554bfb943 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less @@ -10,40 +10,6 @@ @textOnLightBackground: black; @disablingOverlayZindex: 1001; // higher than #audio-devlist -// See BL-7442: When low line height causes lines to overlap, the highlighting overlaps and -// can over up the bottom of the characters in the line above. For Chrome/Safari we have -// a fix that can be found in bloom-player. But those don't work in Firefox yet (as of FF 68). -// So we deal with the most common case of overlap (the book title on the cover) by just -// treating the text as a rectangle. And it's fine that this will show this way in Chrome too: -.Title-On-Cover-style - span.ui-audioCurrent:not(.ui-suppressHighlight):not(.ui-disableHighlight) { - display: inline-block; - white-space: pre-wrap; -} - -span.ui-audioCurrent:not(.ui-suppressHighlight):not(.ui-disableHighlight), -div.ui-audioCurrent:not(.ui-suppressHighlight):not(.ui-disableHighlight) p { - // This behavior is now achieved by a default rule in content/basePage-shared.less, - // from whence it can work in various players and be overridden by our new audio-hightlighting - // control. The negation for suppress/disable is now handled by a separate rule below. - // background: @highlightColor; - // /* make highlighted text easier to read if it is normally a light color (like white)*/ - // color: @textOnLightBackground; -} - -// In the Edit tab, we now use pseudo-elements to display the highlight instead of just background-color nd color, to avoid -// computed styles getting stuck in the book (BL-15300, see audioTextHighlightManager.ts). Here overwrite the highlighting -// colors from basePage.less which are otherwise used to display highlighting (e.g. in Bloom Player), so that only the -// pseudo-elements are displaying the highlights -body.bloom-audio-pseudoHighlights { - span.ui-audioCurrent:not(.ui-suppressHighlight):not(.ui-disableHighlight), - div.ui-audioCurrent:not(.ui-suppressHighlight):not(.ui-disableHighlight) p, - .ui-audioCurrent .ui-enableHighlight { - background-color: transparent !important; - color: inherit !important; - } -} - .bloom-ui-current-audio-marker:before { background-image: url(currentTextIndicator.svg); background-repeat: no-repeat; @@ -57,38 +23,6 @@ body.bloom-audio-pseudoHighlights { content: " "; } -// These classes get applied to ui-audioCurrent elements when we don't currently want -// them highlighted. Because a lot of rules now set various foreground and background -// colors for .ui-audioCurrent, not only during editing when the suppress and disable -// classes are relevant, it's cleaner to do the suppression with separate rules -// rather than :not claues like the above. -// The goal is that rules that set background and foreground color for the current -// playback element shall be defeated when the disable/suppress classes are present. -// There could pathologically be a case where some other rule was defeated unintentionally, -// but we don't have any other rule-based text or background coloring that applies to -// audio elements, so I think the neatness of not having to complicate many rules -// with :not clauses that aren't relevant in most contexts is worth it. -.ui-audioCurrent.ui-suppressHighlight, -.ui-audioCurrent.ui-suppressHighlight p, -.ui-audioCurrent.ui-disableHighlight, -.ui-audioCurrent.ui-disableHighlight p { - background-color: unset !important; - color: unset !important; -} - -.ui-audioCurrent .ui-enableHighlight { - background-color: @highlightColor; - // If we can one day get rid of this, we can simplify code for positioning the microphone icon near the span. - position: unset; // BL-11633, works around Chromium bug -} - -span.ui-audioCurrent, -div.ui-audioCurrent p { - // The rule that sets the background color is in basepage.less. However, we only need to - // interfere with position: relative when editing, so I _think_ this rule belongs here. - position: unset; // BL-11633, works around Chromium bug -} - // Use pseudoelements to display TBT highlighting in the Edit tab. See audioTextHighlightManager.ts ::highlight(bloom-audio-current) { // Read from CSS variables so the Format dialog's stored audio highlight colors @@ -98,6 +32,9 @@ div.ui-audioCurrent p { @highlightColor ); color: var(--bloom-audio-current-highlight-color, @textOnLightBackground); + -webkit-box-decoration-break: clone; + box-decoration-break: clone; + mix-blend-mode: multiply; } // Note: This highlighting is expected to persist across sessions, but to be hidden // (displayed with the yellow color) while each segment is playing. diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts index 5c5ef2b4f771..4cc262f2c7df 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts @@ -114,8 +114,6 @@ const kDisableHighlightClass = "ui-disableHighlight"; const kSuppressHighlightClass = "ui-suppressHighlight"; const kAudioSentence = "audio-sentence"; // Even though these can now encompass more than strict sentences, we continue to use this class name for backwards compatability reasons const kAudioSentenceClassSelector = "." + kAudioSentence; -export const kAudioCurrent = "ui-audioCurrent"; -const kAudioCurrentClassSelector = "." + kAudioCurrent; const kBloomEditableTextBoxClass = "bloom-editable"; const kBloomEditableTextBoxSelector = "div.bloom-editable"; const kBloomTranslationGroupClass = "bloom-translationGroup"; @@ -184,6 +182,9 @@ export default class AudioRecording implements IAudioRecorder { private audioSplitButton: HTMLButtonElement; + // Tracks which element currently has the audio highlight (replaces DOM-based kAudioCurrent class lookup). + private highlightedElement: HTMLElement | null = null; + private showingImageDescriptions: boolean; public recordingMode: RecordingMode; private previousRecordMode: RecordingMode; @@ -261,6 +262,7 @@ export default class AudioRecording implements IAudioRecorder { const mediaPlayer = this.getMediaPlayer(); mediaPlayer.pause(); getWorkspaceBundleExports().showAdjustTimingsDialogFromWorkspaceRoot( + this.highlightedElement, this.split, this.editTimingsFileAsync, this.applyTimingsFileAsync, @@ -516,9 +518,6 @@ export default class AudioRecording implements IAudioRecorder { public removeRecordingSetup() { this.removeAudioCurrentFromPageDocBody(); const page = this.getPageDocBodyJQuery(); - page.find(kAudioCurrentClassSelector) - .removeClass(kAudioCurrent) - .removeClass(kSuppressHighlightClass); if (this.inShowPlaybackOrderMode) { // We are removing the UI because we're changing tools or pages, but we want to leave // the checkbox checked for the next time this tool is active, so it will turn on the @@ -826,10 +825,8 @@ export default class AudioRecording implements IAudioRecorder { // Enhance: Maybe this would be safer to advance/rewind to the next SPAN instead of next audio-sentence. const incrementAmount = isTraverseInReverseOn ? -1 : 1; - const current = (( - this.getPageDocBody() - )).getElementsByClassName(kAudioCurrent); - if (!current || current.length === 0) { + const currentItem = this.highlightedElement; + if (!currentItem) { return null; } @@ -838,8 +835,7 @@ export default class AudioRecording implements IAudioRecorder { if (audioElts.length === 0) { return null; } - const nextIndex = - audioElts.indexOf(current.item(0)) + incrementAmount; + const nextIndex = audioElts.indexOf(currentItem) + incrementAmount; if (nextIndex < 0 || nextIndex >= audioElts.length) { return null; } @@ -907,18 +903,12 @@ export default class AudioRecording implements IAudioRecorder { } private removeAudioCurrent(parentElement: Element) { - // Note that HTMLCollectionOf's length can change if you change the number of elements matching the selector. - const audioCurrentCollection: HTMLCollectionOf = - parentElement.getElementsByClassName(kAudioCurrent); - - // Convert to an array whose length won't be changed - const audioCurrentArray: Element[] = Array.from(audioCurrentCollection); - - for (let i = 0; i < audioCurrentArray.length; i++) { - audioCurrentArray[i].classList.remove( - kAudioCurrent, - kSuppressHighlightClass, - ); + if ( + this.highlightedElement && + parentElement.contains(this.highlightedElement) + ) { + this.highlightedElement.classList.remove(kSuppressHighlightClass); + this.highlightedElement = null; } const iconHolders = Array.from( @@ -1021,7 +1011,7 @@ export default class AudioRecording implements IAudioRecorder { // It's good for this to happen before awaiting the subsequent async behavior, // especially if the caller doesn't await this function. // This allows us to generally represent the correct current element immediately. - newElement.classList.add(kAudioCurrent); + this.highlightedElement = newElement as HTMLElement; if (!this.listening) { // We don't need to mess with the canvas element focus while listening, and especially, // if we're doing a motion preview we don't want to see the edit controls there. @@ -1038,7 +1028,7 @@ export default class AudioRecording implements IAudioRecorder { if (visible && !inAnimation && !bloomPageHidden) { // Show a record icon // This is a workaround for a Chromium bug; see BL-11633. We'd like our style rules - // to just put the icon on the element that has kAudioCurrent. But that element + // to just put the icon on the currently highlighted element. But that element // has a background color, so (due to the bug) it cannot have position:relative, // or we lose the cursor. So insert an empty element (which by default will have // position: relative) to hold the icon. @@ -1197,12 +1187,6 @@ export default class AudioRecording implements IAudioRecorder { } this.resetAudioIfPaused(); - // According to copilot: - // Clear any split-complete state before rebuilding the current highlight. - // Otherwise the custom-highlight logic will still treat the textbox as "post split" - // and suppress the yellow current highlight we want to show as Speak starts. - this.clearAudioSplit(); - // If we were paused highlighting one sentence but are recording in text box mode, // things could get confusing. At least make sure the selection reflects what we // actually want to record. @@ -1218,6 +1202,8 @@ export default class AudioRecording implements IAudioRecorder { const id = this.getCurrentAudioId(); + this.clearAudioSplit(); + return axios .post("/bloom/api/audio/startRecord?id=" + id) .then(() => { @@ -1251,13 +1237,7 @@ export default class AudioRecording implements IAudioRecorder { private getCurrentAudioId(): string | undefined { let id: string | undefined = undefined; - const pageDocBody = this.getPageDocBody(); - const audioCurrentElements = - pageDocBody!.getElementsByClassName(kAudioCurrent); - let currentElement: Element | null = null; - if (audioCurrentElements.length > 0) { - currentElement = audioCurrentElements.item(0); - } + const currentElement = this.highlightedElement; if (currentElement) { if (currentElement.hasAttribute("id")) { id = currentElement.getAttribute("id")!; @@ -2098,7 +2078,7 @@ export default class AudioRecording implements IAudioRecorder { this.updateDisplay(); } private showPlaybackOrderUi(docBody: HTMLElement) { - this.removeAudioCurrentFromPageDocBody(); + this.removeAudioCurrent(docBody); this.playbackOrderCache = []; const translationGroups = this.getVisibleTranslationGroups(docBody); if (translationGroups.length < 1) { @@ -2418,20 +2398,13 @@ export default class AudioRecording implements IAudioRecorder { // Returns the element (could be either div, span, etc.) which is currently highlighted. public getCurrentHighlight(): HTMLElement | null { - let page = this.getPageDocBodyJQuery(); - // ENHANCE: I don't think this really needs to be here? - if (page.length <= 0) { + if (!this.getPageDocBodyJQuery().length) { // The first one is probably the right one when this case is triggered, but even if not, it's better than nothing. this.setCurrentAudioElementToDefaultAsync(); - page = this.getPageDocBodyJQuery(); } - const current = page.find(kAudioCurrentClassSelector); - if (current && current.length > 0) { - return current.get(0); - } - return null; + return this.highlightedElement; } // Returns the text of the currently highlighted element @@ -2496,14 +2469,13 @@ export default class AudioRecording implements IAudioRecorder { return null; } - let audioCurrentElements = ( - Array.from( - pageBody.getElementsByClassName(kAudioCurrent), - ) as HTMLElement[] - ).filter((x) => this.isVisible(x)); + let audioCurrentElements = + this.highlightedElement && this.isVisible(this.highlightedElement) + ? [this.highlightedElement] + : []; if (audioCurrentElements.length === 0 && maySetHighlight) { - // Oops, ui-audioCurrent not set on anything. Just going to have to stick it onto the first element. + // Oops, highlightedElement not set or not visible. Just going to have to stick it onto the first element. // ENHANCE: Theoretically, we should await this. (Or at least, the end of the function should await this promise // That means all the callers should be async'ify'd, which is like... everything. :( @@ -2515,9 +2487,9 @@ export default class AudioRecording implements IAudioRecorder { // 1) This original version (that includes the asynchronous fallback) // 2) Also a synchronous (but no fallback) version of this function called getCurrentTextBoxSync() this.setCurrentAudioElementToDefaultAsync(); - audioCurrentElements = Array.from( - pageBody.getElementsByClassName(kAudioCurrent), - ) as HTMLElement[]; + audioCurrentElements = this.highlightedElement + ? [this.highlightedElement] + : []; if (audioCurrentElements.length <= 0) { return null; @@ -2532,19 +2504,7 @@ export default class AudioRecording implements IAudioRecorder { } public getAudioCurrentElement(): HTMLElement | null { - const pageBody = this.getPageDocBody(); - if (!pageBody) { - return null; - } - - const audioCurrentElements = - pageBody.getElementsByClassName(kAudioCurrent); - - if (audioCurrentElements.length === 0) { - return null; - } - - return audioCurrentElements.item(0) as HTMLElement; + return this.highlightedElement; } // Gets the current text box. If none exists, immediately returns null. @@ -2552,26 +2512,21 @@ export default class AudioRecording implements IAudioRecorder { // TODO: Refactor the old getCurrentTextBox to something like: getCurrentTextBoxWithFallbackAsync // After that, you can rename this function to getCurrentTextBox - const pageBody = this.getPageDocBody(); if ( - !pageBody || + !this.getPageDocBody() || // Tests may not have a value for 'showPlaybackInput'. this.inShowPlaybackOrderMode ) { return null; } - const audioCurrentElements = - pageBody.getElementsByClassName(kAudioCurrent); - - if (audioCurrentElements.length === 0) { - // Oops, ui-audioCurrent not set on anything. Just give up. + const currentItem = this.highlightedElement; + if (!currentItem) { + // Oops, highlightedElement not set. Just give up. return null; } - const currentTextBox = this.getTextBoxOfElement( - audioCurrentElements.item(0), - ); + const currentTextBox = this.getTextBoxOfElement(currentItem); console.assert(!!currentTextBox, "CurrentTextBox should not be null"); return currentTextBox; } @@ -2632,6 +2587,12 @@ export default class AudioRecording implements IAudioRecorder { this.initializeAudioRecordingMode(); const docBody = this.getPageDocBody(); + // Defensive cleanup: strip any ui-audioCurrent class left by older Bloom versions that used DOM marking. + // Modern code tracks the highlight via this.highlightedElement instead. + Array.from( + docBody?.getElementsByClassName("ui-audioCurrent") ?? [], + ).forEach((el) => el.classList.remove("ui-audioCurrent")); + // This check needs to be before the check for recordable divs below (which may return immediately), because sometimes // we may have empty textboxes that should nevertheless show the playback order UI. if (this.inShowPlaybackOrderMode) { @@ -2905,6 +2866,23 @@ export default class AudioRecording implements IAudioRecorder { await this.tryGetUpdateMarkupForTextBoxActionAsync(currentTextBox); return async () => { + // Save the index (position) of the highlighted sentence among its siblings + // before markup changes the DOM. IDs are regenerated when text changes, so + // we use ordinal position instead — it survives ordinary edits. + let previousHighlightIndex = -1; + if ( + this.highlightedElement && + this.highlightedElement.isConnected && + currentTextBox + ) { + const sentences = Array.from( + currentTextBox.getElementsByClassName(kAudioSentence), + ); + previousHighlightIndex = sentences.indexOf( + this.highlightedElement as Element, + ); + } + updateTheElement(); // Adjust the current highlight appropriately // Regardless of whether it's present, we always need to set the current audio element @@ -2917,7 +2895,42 @@ export default class AudioRecording implements IAudioRecorder { // the async actions complete. await this.resetCurrentAudioElementAsync(currentTextBox); + // cleanUpNbsps() in toolbox.ts runs synchronously while we are suspended at the + // first await above. It unconditionally sets editableDiv.innerHTML, detaching + // the span that resetCurrentAudioElementAsync just registered as highlightedElement. + // IDs are preserved through that replacement, so we can recover the live DOM node. + if ( + this.highlightedElement && + !this.highlightedElement.isConnected && + this.highlightedElement.id + ) { + const pageBody = this.getPageDocBody(); + const freshHighlight = pageBody + ? (pageBody.querySelector( + `#${this.highlightedElement.id}`, + ) as HTMLElement | null) + : null; + if (freshHighlight) { + this.highlightedElement = freshHighlight; + } + } + + // resetCurrentAudioElementAsync always resets to the first sentence, but the + // user was editing a specific sentence. Restore by ordinal index; IDs are + // regenerated when text changes so they cannot be used to find the same sentence. + if (previousHighlightIndex >= 0 && currentTextBox) { + const sentences = + currentTextBox.getElementsByClassName(kAudioSentence); + const targetSentence = sentences.item( + previousHighlightIndex, + ) as HTMLElement | null; + if (targetSentence) { + this.highlightedElement = targetSentence; + } + } + await this.changeStateAndSetExpectedAsync("record"); + this.refreshAudioTextHighlights(this.highlightedElement); }; } @@ -3089,30 +3102,23 @@ export default class AudioRecording implements IAudioRecorder { } console.assert(this.recordingMode == RecordingMode.TextBox); - const pageDocBody = this.getPageDocBody(); - if (!pageDocBody) { + if (!this.getPageDocBody()) { return; } - const audioCurrentList = - pageDocBody.getElementsByClassName(kAudioCurrent); - if (isEarlyAbortEnabled && audioCurrentList.length >= 1) { + if (isEarlyAbortEnabled && this.highlightedElement !== null) { // audioCurrent highlight is already working, so don't bother trying to fix anything up. // I think this probably can also help if you rapidly check and uncheck the checkbox, then click Next. // We wouldn't want multiple things highlighted, or end up pointing to the wrong thing, etc. return; } - let audioCurrent: Element | null = null; - if (audioCurrentList.length >= 1) { - audioCurrent = audioCurrentList.item(0); - } const changeTo = this.getTextBoxOfElement(element); if (changeTo) { return this.setSoundAndHighlightAsync({ newElement: changeTo, // Don't automatically scroll because tool is possibly being initialized (we only want it to scroll on explicit user interaction like Next/Prev) shouldScrollToElement: false, - oldElement: audioCurrent, + oldElement: this.highlightedElement, }); } } @@ -3126,11 +3132,7 @@ export default class AudioRecording implements IAudioRecorder { return; } - const audioCurrentList = this.getPageDocBodyJQuery().find( - kAudioCurrentClassSelector, - ); - - if (isEarlyAbortEnabled && audioCurrentList.length >= 1) { + if (isEarlyAbortEnabled && this.highlightedElement !== null) { // audioCurrent highlight is already working, so don't bother trying to fix anything up. // I think this probably can also help if you rapidly check and uncheck the checkbox, then click Next. // We wouldn't want multiple things highlighted, or end up pointing to the wrong thing, etc. @@ -3762,8 +3764,7 @@ export default class AudioRecording implements IAudioRecorder { // Finding no audioCurrent is only unexpected if there are non-zero number of audio elements if ( - this.getPageDocBodyJQuery().find(kAudioCurrentClassSelector) - .length === 0 && + this.highlightedElement === null && this.containsAnyAudioElements() ) { // We have reached an unexpected state :( diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts index 1e8c3367d446..aea529e00e2f 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts @@ -98,6 +98,11 @@ export class AudioTextHighlightManager { splitHighlightNames.forEach((name) => registry.delete(name)); } + // currentHighlight is the element currently selected for recording etc. + // It might be a span (sentence mode) or text box (text box mode). + // currentTextBox is either the same as currentHighlight (text box mode) + // or its TextBox ancestor (sentence mode). + // Adjust pseudo-element hightlights to what they should be for this state of things. public refreshHighlights( currentHighlight: Element | null, currentTextBox: HTMLElement | null, @@ -294,6 +299,8 @@ export class AudioTextHighlightManager { }; } + // Set the CSS variables that control the ::highlight colors to match the user's chosen + // highlight color for the current text style, falling back to the default yellow. private updateCurrentHighlightColors(styleSource: Element): void { const documentElement = getDocumentElement(styleSource); if (!documentElement) { @@ -303,47 +310,61 @@ export class AudioTextHighlightManager { return; } - // The actual colors still come from CSS so user-modified highlight colors keep working. - // We copy them into document-level variables that the ::highlight rule can read. - const documentBody = getDocumentBody(styleSource); - const hadPseudoHighlightOverride = documentBody?.classList.contains( - kPseudoHighlightSupportClass, + const bloomEditable = styleSource.closest(".bloom-editable"); + const styleName = bloomEditable + ? Array.from(bloomEditable.classList).find((c) => + c.endsWith("-style"), + ) + : undefined; + + const userColors = styleName + ? this.getHighlightColorsFromUserStyles( + styleSource.ownerDocument, + styleName, + ) + : undefined; + + documentElement.style.setProperty( + kCurrentHighlightBackgroundCssVar, + userColors?.backgroundColor ?? "#febf00", ); + documentElement.style.setProperty( + kCurrentHighlightColorCssVar, + userColors?.color ?? "black", + ); + } - // We have rules in audioRecording.less to override the normal highlighting background-color - // (make it transparent), so it isn't underneath - // the pseudo-element highlights which we use instead in the edit tab. Remove that rule, detect the otherwise - // present computed highlight color, and then put the rule back. - if (hadPseudoHighlightOverride) { - documentBody?.classList.remove(kPseudoHighlightSupportClass); - } - const computedStyle = - getDocumentWindow(styleSource)?.getComputedStyle(styleSource); - const backgroundColor = computedStyle?.backgroundColor; - const color = computedStyle?.color; - if (hadPseudoHighlightOverride) { - documentBody?.classList.add(kPseudoHighlightSupportClass); - } - - if (backgroundColor) { - documentElement.style.setProperty( - kCurrentHighlightBackgroundCssVar, - backgroundColor, - ); - } else { - documentElement.style.removeProperty( - kCurrentHighlightBackgroundCssVar, - ); - } - - if (color) { - documentElement.style.setProperty( - kCurrentHighlightColorCssVar, - color, - ); - } else { - documentElement.style.removeProperty(kCurrentHighlightColorCssVar); + // Look in the book's userModifiedStyles sheet for an audio highlight rule for the + // given style name, and return its background-color and color if found. + private getHighlightColorsFromUserStyles( + doc: Document, + styleName: string, + ): { backgroundColor: string; color: string } | undefined { + const userStyles = Array.from(doc.styleSheets).find( + (s) => + (s.ownerNode as Element)?.getAttribute("title") === + "userModifiedStyles", + ); + if (!userStyles) return undefined; + + try { + for (const cssRule of Array.from(userStyles.cssRules)) { + const rule = cssRule as CSSStyleRule; + if ( + rule.selectorText?.includes(styleName) && + rule.selectorText?.includes("ui-audioCurrent") && + rule.style.backgroundColor + ) { + return { + backgroundColor: rule.style.backgroundColor, + color: rule.style.color || "black", + }; + } + } + } catch { + // Stylesheets can throw on cross-origin access; shouldn't happen here } + return undefined; } private shouldShowSplitHighlights( diff --git a/src/BloomBrowserUI/bookEdit/workspaceRoot.ts b/src/BloomBrowserUI/bookEdit/workspaceRoot.ts index ae0dcb2c9eb2..aac0c73d26b5 100644 --- a/src/BloomBrowserUI/bookEdit/workspaceRoot.ts +++ b/src/BloomBrowserUI/bookEdit/workspaceRoot.ts @@ -34,6 +34,7 @@ export interface IWorkspaceExports { showCopyrightAndLicenseDialog(imageUrl?: string): void; showEditViewTopicChooserDialog(): void; showAdjustTimingsDialogFromWorkspaceRoot( + currentTextBox: HTMLElement | null, // The split and applyTimingsFile calls both return a list of new timings, // such as we might find in data-audioRecordingEndTimes split: (timingFilePath: string) => Promise, From 63c7dc71e7508d4ba45de2bfe683642f39de3e56 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Mon, 29 Jun 2026 14:10:24 -0500 Subject: [PATCH 4/9] JohnT improvements - All managed by the :highlight code. We no longer add and remove classes to manage highlighting - The microphone icon is also managed without modifying the page content as it moves around - new CSS for highlight causes less conflict with previous lines when tightly spaced --- .../toolbox/talkingBook/audioRecording.less | 43 ++-- .../toolbox/talkingBook/audioRecording.ts | 192 ++++++++++++------ .../talkingBook/audioTextHighlightManager.ts | 22 +- 3 files changed, 171 insertions(+), 86 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less index 8de554bfb943..13167d749776 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less @@ -10,31 +10,42 @@ @textOnLightBackground: black; @disablingOverlayZindex: 1001; // higher than #audio-devlist -.bloom-ui-current-audio-marker:before { - background-image: url(currentTextIndicator.svg); - background-repeat: no-repeat; - background-size: 10px 13px; - background-position: 2px 5px; - left: -15px; - top: 0; // should have no effect, but prevents a FF bug causing BL-6796 +.bloom-ui-current-audio-marker { + // Use the same highlight background color as the current sentence. + background-color: var( + --bloom-audio-current-highlight-background, + @highlightColor + ); + // Mask to the microphone shape so the background-color shows through it. + -webkit-mask-image: url(currentTextIndicator.svg); + -webkit-mask-size: 10px 13px; + -webkit-mask-position: 2px 5px; + -webkit-mask-repeat: no-repeat; + mask-image: url(currentTextIndicator.svg); + mask-size: 10px 13px; + mask-position: 2px 5px; + mask-repeat: no-repeat; width: 15px; height: 19px; - position: absolute; - content: " "; + // Must be above the split-pane-component and other content in the page frame. + z-index: 1; } // Use pseudoelements to display TBT highlighting in the Edit tab. See audioTextHighlightManager.ts ::highlight(bloom-audio-current) { - // Read from CSS variables so the Format dialog's stored audio highlight colors - // control the normally-yellow recording highlight - background-color: var( + // A thick underline offset upwards (the skip-ink setting allows it to be drawn right against the text) + // does not extend upwards as far as background-color highlighting, which greatly reduces the + // extent to which it covers previous lines of text when tightly spaced. + // Color is read from a CSS variable so the Format dialog's stored audio highlight colors apply. + text-decoration-line: underline; + text-decoration-style: solid; + text-decoration-color: var( --bloom-audio-current-highlight-background, @highlightColor ); - color: var(--bloom-audio-current-highlight-color, @textOnLightBackground); - -webkit-box-decoration-break: clone; - box-decoration-break: clone; - mix-blend-mode: multiply; + text-decoration-thickness: 1.1em; + text-underline-offset: -0.8em; + text-decoration-skip-ink: none; } // Note: This highlighting is expected to persist across sessions, but to be hidden // (displayed with the yellow color) while each segment is playing. diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts index 4cc262f2c7df..af2eb625c2d5 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts @@ -108,10 +108,6 @@ const kEnableHighlightClass = "ui-enableHighlight"; // For example, some elements have highlighting prevented at this level // because its content has been broken into child elements, only some of which show the highlight const kDisableHighlightClass = "ui-disableHighlight"; -// Indicates that highlighting is briefly/temporarily suppressed, -// but may become highlighted later. -// For example, audio highlighting is suppressed until the related audio starts playing (to avoid flashes) -const kSuppressHighlightClass = "ui-suppressHighlight"; const kAudioSentence = "audio-sentence"; // Even though these can now encompass more than strict sentences, we continue to use this class name for backwards compatability reasons const kAudioSentenceClassSelector = "." + kAudioSentence; const kBloomEditableTextBoxClass = "bloom-editable"; @@ -907,18 +903,10 @@ export default class AudioRecording implements IAudioRecorder { this.highlightedElement && parentElement.contains(this.highlightedElement) ) { - this.highlightedElement.classList.remove(kSuppressHighlightClass); this.highlightedElement = null; } - const iconHolders = Array.from( - parentElement.getElementsByClassName( - "bloom-ui-current-audio-marker", - ), - ); - for (let i = 0; i < iconHolders.length; i++) { - iconHolders[i].remove(); - } + this.updateIconMarker(null); } // I'm not sure why activeToolId falsy should count as "true" but that's how some old code @@ -1019,65 +1007,33 @@ export default class AudioRecording implements IAudioRecorder { newElement as HTMLElement, ); } - // during animation we don't want to add this, even in stuff that's not visible. - // It can get left behind and get wrapped in an extra paragraph (BL-15293) - let bloomPageHidden = false; - const page = newElement.closest(".bloom-page"); - if (page && window.getComputedStyle(page).visibility === "hidden") - bloomPageHidden = true; - if (visible && !inAnimation && !bloomPageHidden) { - // Show a record icon - // This is a workaround for a Chromium bug; see BL-11633. We'd like our style rules - // to just put the icon on the currently highlighted element. But that element - // has a background color, so (due to the bug) it cannot have position:relative, - // or we lose the cursor. So insert an empty element (which by default will have - // position: relative) to hold the icon. - const iconHolder = - newElement.ownerDocument.createElement("span"); - iconHolder.classList.add("bloom-ui-current-audio-marker"); - iconHolder.classList.add("bloom-ui"); // makes sure it never becomes part of saved document. - // If we're recording by text-box, we want the icon to be at the beginning of the text box, - // but we also want it inside the text-box div. Otherwise, the appearance system introduced - // by Bloom 6.0 will cause a gap to appear between the invisible icon and the text, shifting - // the text down while it is being recorded. See BL-13128. - // (The icon doesn't actually display for whole text box recording or for the first sentence - // of sentence-by-sentence recording, but that's a separate issue that makes the text shift - // even more mysterious.) - if (newElement.tagName === "DIV") { - newElement.insertBefore(iconHolder, newElement.firstChild); - } else { - newElement.parentElement?.insertBefore( - iconHolder, - newElement, - ); - } - } } + let suppressHighlight = false; if (suppressHighlightIfNoAudio && visible) { - // prevents highlight showing at once - // FYI: Because of how JS works, no rendering should happen between setting audioCurrent above and setting ui-suppressHighlight here. - newElement.classList.add(kSuppressHighlightClass); try { const response: AxiosResponse = await axios.get( "/bloom/api/audio/checkForSegment?id=" + newElement.id, ); - - if (response.data === "exists") { - newElement.classList.remove(kSuppressHighlightClass); - } + suppressHighlight = response.data !== "exists"; } catch (error) { - //server couldn't find it, so just leave it unhighlighted + //server couldn't find it, so leave it unhighlighted toastr.error( - "Error checking on audio file " + error.statusText, + "Error checking on audio file " + + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (error as any)?.response?.statusText, ); + suppressHighlight = true; } } - this.refreshAudioTextHighlights(newElement); + this.refreshAudioTextHighlights(newElement, suppressHighlight); } - private refreshAudioTextHighlights(currentHighlight?: Element | null) { + private refreshAudioTextHighlights( + currentHighlight?: Element | null, + suppressCurrentHighlight?: boolean, + ) { const activeHighlight = currentHighlight ?? this.getCurrentHighlight(); const currentTextBox = activeHighlight ? ((this.getTextBoxOfElement( @@ -1089,7 +1045,129 @@ export default class AudioRecording implements IAudioRecorder { this.audioTextHighlightManager.refreshHighlights( activeHighlight, currentTextBox, + suppressCurrentHighlight, + ); + this.updateIconMarker( + suppressCurrentHighlight + ? null + : (activeHighlight as HTMLElement | null), + ); + } + + // The ID used for the single persistent microphone icon marker element. + private readonly kIconMarkerId = "bloom-ui-current-audio-icon"; + + // Update the position of the absolutely-placed microphone icon marker, or hide it. + // The marker lives inside #page-scaling-container (a sibling of .bloom-page) so it + // is inside the page zoom transform but outside the page content, where it cannot be + // accidentally saved or disturb CKEditor. getBoundingClientRects() gives positions + // in viewport (post-zoom) coordinates; we divide by the container's CSS scale to + // convert to the container's local coordinate space for position: absolute. + private updateIconMarker(element: HTMLElement | null): void { + const pageDocBody = this.getPageDocBody(); + if (!pageDocBody) return; + + const hideExisting = () => { + const existing = pageDocBody.ownerDocument.getElementById( + this.kIconMarkerId, + ) as HTMLElement | null; + if (existing) existing.style.display = "none"; + }; + + if (!element || this.inShowPlaybackOrderMode) { + hideExisting(); + return; + } + + // Don't show during motion-tool animation or on hidden pages. + if (element.closest("." + animateStyleName)) { + hideExisting(); + return; + } + const bloomPage = element.closest(".bloom-page") as HTMLElement | null; + const docView = element.ownerDocument.defaultView; + if ( + bloomPage && + docView?.getComputedStyle(bloomPage).visibility === "hidden" + ) { + hideExisting(); + return; + } + + // Don't show for hidden language blocks or other invisible elements. + if (!this.isVisible(element)) { + hideExisting(); + return; + } + + const container = pageDocBody.querySelector( + "#page-scaling-container", + ) as HTMLElement | null; + if (!container) return; + + // getClientRects() returns one rect per line box; the first is the first line. + const rects = element.getClientRects(); + if (rects.length === 0) { + hideExisting(); + return; + } + + const firstLineRect = rects[0]; + const containerRect = container.getBoundingClientRect(); + + // #page-scaling-container uses transform: scale(zoom) so viewport distances + // must be divided by the zoom factor to get local coordinates. + const transformStr = + docView?.getComputedStyle(container).transform ?? "none"; + const zoom = + transformStr !== "none" ? new DOMMatrix(transformStr).a : 1; + + // getComputedStyle().fontSize is in CSS pixels (pre-zoom), same coordinate space + // as the top/left values we set, so we can add the em-based offset directly. + const fontSizePx = parseFloat( + docView?.getComputedStyle(element).fontSize ?? "16", ); + + // All conditions met — get or create the icon only now that we'll show it. + const icon = this.getOrCreateIconMarker(pageDocBody); + if (!icon) return; + + icon.style.display = ""; + // Shift down ~0.2em so the icon tracks the text rather than the top of the line box. + icon.style.top = `${(firstLineRect.top - containerRect.top) / zoom + fontSizePx * 0.2}px`; + // Place the icon 15px to the left of the sentence start, matching the + // background-position offset in audioRecording.less. + icon.style.left = `${(firstLineRect.left - containerRect.left) / zoom - 15}px`; + } + + // Find the icon marker element, or create and insert it inside #page-scaling-container. + private getOrCreateIconMarker( + pageDocBody: HTMLElement, + ): HTMLElement | null { + const container = pageDocBody.querySelector( + "#page-scaling-container", + ) as HTMLElement | null; + if (!container) return null; + + const existing = pageDocBody.ownerDocument.getElementById( + this.kIconMarkerId, + ) as HTMLElement | null; + if (existing) return existing; + + const icon = pageDocBody.ownerDocument.createElement("span"); + icon.id = this.kIconMarkerId; + icon.className = "bloom-ui-current-audio-marker bloom-ui"; + icon.style.position = "absolute"; + icon.style.pointerEvents = "none"; + // Ensure #page-scaling-container is a positioning context for our absolute child. + const containerPosition = + container.ownerDocument.defaultView?.getComputedStyle(container) + .position ?? "static"; + if (containerPosition === "static") { + container.style.position = "relative"; + } + container.appendChild(icon); + return icon; } // Scrolls an element into view. diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts index aea529e00e2f..5138cf2f55f0 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts @@ -1,6 +1,5 @@ const kSegmentClass = "bloom-highlightSegment"; const kEnableHighlightClass = "ui-enableHighlight"; -const kSuppressHighlightClass = "ui-suppressHighlight"; const kDisableHighlightClass = "ui-disableHighlight"; const kPostAudioSplitClass = "bloom-postAudioSplit"; const kTextBoxRecordingMode = "textbox"; @@ -106,16 +105,15 @@ export class AudioTextHighlightManager { public refreshHighlights( currentHighlight: Element | null, currentTextBox: HTMLElement | null, + suppressCurrentHighlight?: boolean, ): void { const contextNode = currentHighlight ?? currentTextBox; if (!contextNode) { return; } - if ( - !getHighlightRegistry(contextNode) || - !getHighlightConstructor(contextNode) - ) { + const registry = getHighlightRegistry(contextNode); + if (!registry || !getHighlightConstructor(contextNode)) { return; } @@ -124,6 +122,11 @@ export class AudioTextHighlightManager { kPseudoHighlightSupportClass, ); + if (suppressCurrentHighlight) { + allManagedHighlightNames.forEach((name) => registry.delete(name)); + return; + } + this.refreshCurrentHighlight(currentHighlight, currentTextBox); this.refreshSplitHighlights(currentHighlight, currentTextBox); } @@ -251,10 +254,6 @@ export class AudioTextHighlightManager { return undefined; } - if (currentHighlight.classList.contains(kSuppressHighlightClass)) { - return undefined; - } - // copilot says: fixHighlighting() can carve the visible pieces into nested ui-enableHighlight // spans so punctuation or outer whitespace stays unpainted. Prefer those exact // spans whenever they exist so the pseudo-highlight matches the background-color highlight behavior @@ -379,10 +378,7 @@ export class AudioTextHighlightManager { return false; } - if ( - currentTextBox.classList.contains(kSuppressHighlightClass) || - currentTextBox.classList.contains(kDisableHighlightClass) - ) { + if (currentTextBox.classList.contains(kDisableHighlightClass)) { return false; } From e11f4ab36060a8f8fb3b74e406218663cba9c993 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Mon, 29 Jun 2026 16:23:23 -0500 Subject: [PATCH 5/9] post-review cleanup on BL-15300 --- .../toolbox/talkingBook/audioRecording.less | 21 +++- .../toolbox/talkingBook/audioRecording.ts | 7 +- .../talkingBook/audioTextHighlightManager.ts | 113 +++++------------- src/BloomBrowserUI/bookEdit/workspaceRoot.ts | 2 +- 4 files changed, 52 insertions(+), 91 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less index 13167d749776..f129b95be465 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less @@ -50,13 +50,28 @@ // Note: This highlighting is expected to persist across sessions, but to be hidden // (displayed with the yellow color) while each segment is playing. ::highlight(bloom-audio-split-1) { - background-color: #bfedf3; + text-decoration-line: underline; + text-decoration-style: solid; + text-decoration-color: #bfedf3; + text-decoration-thickness: 1.1em; + text-underline-offset: -0.8em; + text-decoration-skip-ink: none; } ::highlight(bloom-audio-split-2) { - background-color: #7fdae6; + text-decoration-line: underline; + text-decoration-style: solid; + text-decoration-color: #7fdae6; + text-decoration-thickness: 1.1em; + text-underline-offset: -0.8em; + text-decoration-skip-ink: none; } ::highlight(bloom-audio-split-3) { - background-color: #29c2d6; + text-decoration-line: underline; + text-decoration-style: solid; + text-decoration-color: #29c2d6; + text-decoration-thickness: 1.1em; + text-underline-offset: -0.8em; + text-decoration-skip-ink: none; } .ui-audioBody { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts index af2eb625c2d5..bcaa262aa1cb 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts @@ -257,6 +257,7 @@ export default class AudioRecording implements IAudioRecorder { .click(async (e) => { const mediaPlayer = this.getMediaPlayer(); mediaPlayer.pause(); + if (!this.highlightedElement) return; getWorkspaceBundleExports().showAdjustTimingsDialogFromWorkspaceRoot( this.highlightedElement, this.split, @@ -2146,7 +2147,7 @@ export default class AudioRecording implements IAudioRecorder { // If the highlight was on something not currently visible, move the selection const current = this.getCurrentHighlight(); if (page && current && !this.isVisible(current)) { - this.removeAudioCurrent(page); + this.removeAudioCurrentFromPageDocBody(); await this.setCurrentAudioElementToDefaultAsync(); } // Whether or not we had to move the selection, some button states may need to change. @@ -2156,7 +2157,7 @@ export default class AudioRecording implements IAudioRecorder { this.updateDisplay(); } private showPlaybackOrderUi(docBody: HTMLElement) { - this.removeAudioCurrent(docBody); + this.removeAudioCurrentFromPageDocBody(); this.playbackOrderCache = []; const translationGroups = this.getVisibleTranslationGroups(docBody); if (translationGroups.length < 1) { @@ -2748,7 +2749,7 @@ export default class AudioRecording implements IAudioRecorder { const oldHighlight = this.getCurrentHighlight(); if (!boxToSelect) { this.resetAudioIfPaused(); - this.removeAudioCurrent(this.getPageDocBody()!); + this.removeAudioCurrentFromPageDocBody(); await this.changeStateAndSetExpectedAsync(""); this.updateDisplay(false); return false; diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts index 5138cf2f55f0..bc7b4bcc0de4 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts @@ -3,7 +3,6 @@ const kEnableHighlightClass = "ui-enableHighlight"; const kDisableHighlightClass = "ui-disableHighlight"; const kPostAudioSplitClass = "bloom-postAudioSplit"; const kTextBoxRecordingMode = "textbox"; -const kPseudoHighlightSupportClass = "bloom-audio-pseudoHighlights"; const kCurrentHighlightBackgroundCssVar = "--bloom-audio-current-highlight-background"; @@ -41,10 +40,6 @@ function getDocumentElement(contextNode: Node): HTMLElement | undefined { return contextNode.ownerDocument?.documentElement ?? undefined; } -function getDocumentBody(contextNode: Node): HTMLElement | undefined { - return contextNode.ownerDocument?.body ?? undefined; -} - function getHighlightRegistry( contextNode: Node, ): HighlightRegistry | undefined { @@ -101,7 +96,7 @@ export class AudioTextHighlightManager { // It might be a span (sentence mode) or text box (text box mode). // currentTextBox is either the same as currentHighlight (text box mode) // or its TextBox ancestor (sentence mode). - // Adjust pseudo-element hightlights to what they should be for this state of things. + // Adjust pseudo-element highlights to what they should be for this state of things. public refreshHighlights( currentHighlight: Element | null, currentTextBox: HTMLElement | null, @@ -113,53 +108,38 @@ export class AudioTextHighlightManager { } const registry = getHighlightRegistry(contextNode); - if (!registry || !getHighlightConstructor(contextNode)) { + const Highlight = getHighlightConstructor(contextNode); + if (!registry || !Highlight) { return; } - // Once pseudo-element highlights are active, suppress the old element background rules so we don't double-highlight - getDocumentBody(contextNode)?.classList.add( - kPseudoHighlightSupportClass, - ); - if (suppressCurrentHighlight) { allManagedHighlightNames.forEach((name) => registry.delete(name)); return; } - this.refreshCurrentHighlight(currentHighlight, currentTextBox); - this.refreshSplitHighlights(currentHighlight, currentTextBox); + // Split highlights (blue segments after a textbox recording) and the current highlight + // (yellow sentence) are mutually exclusive: split state replaces the yellow highlight. + if (this.shouldShowSplitHighlights(currentHighlight, currentTextBox)) { + registry.delete(currentHighlightName); + this.refreshSplitHighlights(currentTextBox, registry, Highlight); + } else { + splitHighlightNames.forEach((name) => registry.delete(name)); + this.refreshCurrentHighlight( + currentHighlight, + currentTextBox, + registry, + Highlight, + ); + } } private refreshCurrentHighlight( currentHighlight: Element | null, currentTextBox: HTMLElement | null, + registry: HighlightRegistry, + Highlight: HighlightConstructor, ): void { - const contextNode = currentHighlight ?? currentTextBox; - if (!contextNode) { - console.error( - "AudioTextHighlightManager.refreshCurrentHighlight() was called without a context node.", - ); - return; - } - - const registry = getHighlightRegistry(contextNode); - const Highlight = getHighlightConstructor(contextNode); - // copilot wants belt-and-suspenders, fine - if (!registry || !Highlight) { - console.error( - "AudioTextHighlightManager.refreshCurrentHighlight() lost pseudo-highlight support after refreshHighlights() verified it.", - ); - return; - } - - // The split-complete blue state replaces the yellow current highlight while it is visible, - // so clear the yellow registry entry instead of letting the two overlap. - if (this.shouldShowSplitHighlights(currentHighlight, currentTextBox)) { - registry.delete(currentHighlightName); - return; - } - const highlightInfo = this.getCurrentHighlightInfo( currentHighlight, currentTextBox, @@ -178,57 +158,22 @@ export class AudioTextHighlightManager { } private refreshSplitHighlights( - currentHighlight: Element | null, - currentTextBox: HTMLElement | null, + currentTextBox: HTMLElement, + registry: HighlightRegistry, + Highlight: HighlightConstructor, ): void { - const contextNode = currentHighlight ?? currentTextBox; - if (!contextNode) { - console.error( - "AudioTextHighlightManager.refreshSplitHighlights() was called without a context node.", - ); - return; - } - - if (!this.shouldShowSplitHighlights(currentHighlight, currentTextBox)) { - this.clearSplitHighlights(contextNode); - return; - } - - const registry = getHighlightRegistry(contextNode); - const Highlight = getHighlightConstructor(contextNode); - if (!registry || !Highlight) { - console.error( - "AudioTextHighlightManager.refreshSplitHighlights() lost pseudo-highlight support after refreshHighlights() verified it.", - ); - return; - } - - // We cycle through 3 colors for split highlights + // Cycle through 3 colors using a page-relative index so adjacent paragraphs + // never share the same color at their boundary. const rangesByName = new Map(); splitHighlightNames.forEach((name) => rangesByName.set(name, [])); - const segmentGroups = new Map(); Array.from( currentTextBox.querySelectorAll(`span.${kSegmentClass}`), - ).forEach((segment) => { - const parent = segment.parentElement; - if (!parent) { - // won't happen - return; - } - - const segments = segmentGroups.get(parent) ?? []; - segments.push(segment); - segmentGroups.set(parent, segments); - }); - - segmentGroups.forEach((segments) => { - segments.forEach((segment, index) => { - const highlightName = - splitHighlightNames[index % splitHighlightNames.length]; - const ranges = rangesByName.get(highlightName); - ranges?.push(...this.getRangesForSegment(segment)); - }); + ).forEach((segment, index) => { + const highlightName = + splitHighlightNames[index % splitHighlightNames.length]; + const ranges = rangesByName.get(highlightName); + ranges?.push(...this.getRangesForSegment(segment)); }); splitHighlightNames.forEach((name) => { diff --git a/src/BloomBrowserUI/bookEdit/workspaceRoot.ts b/src/BloomBrowserUI/bookEdit/workspaceRoot.ts index aac0c73d26b5..febdbb3065cc 100644 --- a/src/BloomBrowserUI/bookEdit/workspaceRoot.ts +++ b/src/BloomBrowserUI/bookEdit/workspaceRoot.ts @@ -34,7 +34,7 @@ export interface IWorkspaceExports { showCopyrightAndLicenseDialog(imageUrl?: string): void; showEditViewTopicChooserDialog(): void; showAdjustTimingsDialogFromWorkspaceRoot( - currentTextBox: HTMLElement | null, + currentTextBox: HTMLElement, // The split and applyTimingsFile calls both return a list of new timings, // such as we might find in data-audioRecordingEndTimes split: (timingFilePath: string) => Promise, From cdbd06bdf02ea99e0c887f5a7f9a15843f149a24 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Tue, 30 Jun 2026 11:46:50 -0500 Subject: [PATCH 6/9] Get tests passing --- .../bookEdit/StyleEditor/StyleEditor.ts | 6 +- .../toolbox/talkingBook/audioRecording.ts | 7 +- .../toolbox/talkingBook/audioRecordingSpec.ts | 218 +++++++++++------- .../talkingBook/audioTextHighlightManager.ts | 2 + .../toolbox/talkingBook/talkingBookSpec.ts | 32 ++- src/BloomBrowserUI/vite.config.mts | 17 +- src/BloomBrowserUI/vitest.setup.ts | 20 ++ 7 files changed, 199 insertions(+), 103 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts b/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts index 2f6bc9af9969..0a838ec2d57b 100644 --- a/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts +++ b/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts @@ -695,8 +695,10 @@ export default class StyleEditor { this.sentenceHiliteRuleSelector, false, ); - const hiliteTextColor = sentenceRule?.style?.color || undefined; - let hiliteBgColor = sentenceRule?.style?.backgroundColor; + const hiliteTextColor = + sentenceRule?.style?.getPropertyValue("color") || undefined; + let hiliteBgColor = + sentenceRule?.style?.getPropertyValue("background-color"); if (!hiliteBgColor) { hiliteBgColor = kBloomYellow; } diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts index bcaa262aa1cb..87bf26108284 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts @@ -1266,6 +1266,11 @@ export default class AudioRecording implements IAudioRecorder { } this.resetAudioIfPaused(); + // Clear any split-complete state before rebuilding the current highlight. + // Otherwise the custom-highlight logic will still treat the textbox as "post split" + // and suppress the yellow current highlight we want to show as Speak starts. + this.clearAudioSplit(); + // If we were paused highlighting one sentence but are recording in text box mode, // things could get confusing. At least make sure the selection reflects what we // actually want to record. @@ -1281,8 +1286,6 @@ export default class AudioRecording implements IAudioRecorder { const id = this.getCurrentAudioId(); - this.clearAudioSplit(); - return axios .post("/bloom/api/audio/startRecord?id=" + id) .then(() => { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts index 88a672d79bcd..bf43adb0d06d 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts @@ -1,4 +1,4 @@ -import { +import { describe, it, expect, @@ -92,6 +92,26 @@ const getHighlightTexts = (highlightName: string): string[] => { return highlight ? highlight.ranges.map((range) => range.toString()) : []; }; +/** + * In BL-15300, the "current" audio element is tracked by AudioRecording.highlightedElement. + * Tests mark the intended pre-selected element with data-test-preselect="true" in their + * HTML fixture (never via the production ui-audioCurrent class). This helper reads that + * attribute and transfers the element to the recording's highlightedElement field before + * calling methods under test. + */ +const setHighlightedElementFromDom = (recording: AudioRecording) => { + const pageFrame = parent.window.document.getElementById( + "page", + ) as HTMLIFrameElement | null; + const doc = pageFrame?.contentDocument ?? document; + const elem = doc.querySelector( + "[data-test-preselect]", + ) as HTMLElement | null; + ( + recording as unknown as { highlightedElement: HTMLElement | null } + ).highlightedElement = elem; +}; + // Notes: // For any async tests: // I recommend using async/await syntax, without the done() callback. @@ -148,16 +168,16 @@ describe("audio recording tests", () => { // Returns the HTML for a single text box for a variety of recording modes function getTextBoxHtmlSimple1(scenario: AudioMode) { if (scenario === AudioMode.PureSentence) { - return `

Sentence 1.1. Sentence 1.2

`; + return `

Sentence 1.1. Sentence 1.2

`; } else if (scenario === AudioMode.PreTextBox) { - return `

Sentence 1.1. Sentence 1.2

`; + return `

Sentence 1.1. Sentence 1.2

`; } else if (scenario === AudioMode.PureTextBox) { - return `

Sentence 1.1. Sentence 1.2

`; + return `

Sentence 1.1. Sentence 1.2

`; } else if (scenario === AudioMode.HardSplitTextBox) { - // FYI: Yes, it is confirmed that in hardSplit, ui-audioCurrent goes on the div, not the span. - return `

Sentence 1.1. Sentence 1.2

`; + // FYI: Yes, it is confirmed that in hardSplit, the pre-selected element is the div, not the span. + return `

Sentence 1.1. Sentence 1.2

`; } else if (scenario === AudioMode.SoftSplitTextBox) { - return `

Sentence 1.1. Sentence 1.2

`; + return `

Sentence 1.1. Sentence 1.2

`; } else { throw new Error("Unknown scenario: " + AudioMode[scenario]); } @@ -166,9 +186,10 @@ describe("audio recording tests", () => { describe("- Next()", () => { it("Record=Sentence, last sentence returns disabled for Next button", () => { SetupIFrameFromHtml( - "

Sentence 1.

", + "

Sentence 1.

", ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.Sentence; const observed = recording.getNextAudioElement(); @@ -178,9 +199,10 @@ describe("audio recording tests", () => { it("Record=TextBox/Play=Sentence, last sentence returns disabled for Next button", () => { SetupIFrameFromHtml( - "

Sentence 1.

", + "

Sentence 1.

", ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.TextBox; const observed = recording.getNextAudioElement(); @@ -190,9 +212,10 @@ describe("audio recording tests", () => { it("Record=TextBox/Play=TextBox, last sentence returns disabled for Next button", () => { SetupIFrameFromHtml( - "
p>Sentence 1.

", + "
p>Sentence 1.

", ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.TextBox; const observed = recording.getNextAudioElement(); @@ -202,7 +225,7 @@ describe("audio recording tests", () => { it("SS -> SS, returns next box's first sentence", () => { const box1Html = - "

Sentence 1.

"; + "

Sentence 1.

"; const box2Html = "

Sentence 2.Sentence 3.

"; SetupIFrameFromHtml( @@ -210,6 +233,7 @@ describe("audio recording tests", () => { ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.Sentence; const observed = recording.getNextAudioElement(); @@ -219,7 +243,7 @@ describe("audio recording tests", () => { it("TS -> TT, returns next box", () => { const box1Html = - "

Sentence 1.

"; + "

Sentence 1.

"; const box2Html = "

Sentence 2.Sentence 3.

"; SetupIFrameFromHtml( @@ -227,6 +251,7 @@ describe("audio recording tests", () => { ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.TextBox; const observed = recording.getNextAudioElement(); @@ -236,9 +261,10 @@ describe("audio recording tests", () => { it("TT -> TT, returns next box", () => { SetupIFrameFromHtml( - "
p>Sentence 1.

p>Sentence 2.

", + "
p>Sentence 1.

p>Sentence 2.

", ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.TextBox; const observed = recording.getNextAudioElement(); @@ -247,10 +273,14 @@ describe("audio recording tests", () => { }); it("Next() skips over empty box, TT -> TT -> TT", () => { - const boxTemplate = (index: number, extraClasses: string) => { - return `
p>Sentence ${index}.

`; + const boxTemplate = ( + index: number, + extraClasses: string, + preselect = false, + ) => { + return `
p>Sentence ${index}.

`; }; - const box1Html = boxTemplate(1, " ui-audioCurrent"); + const box1Html = boxTemplate(1, "", true); const box2Html = '

'; const box3Html = boxTemplate(3, ""); @@ -260,6 +290,7 @@ describe("audio recording tests", () => { ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.TextBox; const observed = recording.getNextAudioElement(); @@ -272,9 +303,10 @@ describe("audio recording tests", () => { describe("- Prev()", () => { it("Record=Sentence, first sentence returns disabled for Back button", () => { SetupIFrameFromHtml( - "

Sentence 1.

", + "

Sentence 1.

", ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.Sentence; const observed = recording.getPreviousAudioElement(); @@ -284,9 +316,10 @@ describe("audio recording tests", () => { it("Record=TextBox/Play=Sentence, first sentence returns disabled for Back button", () => { SetupIFrameFromHtml( - "

Sentence 1.

", + "

Sentence 1.

", ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.TextBox; const observed = recording.getPreviousAudioElement(); @@ -296,9 +329,10 @@ describe("audio recording tests", () => { it("Record=TextBox/Play=TextBox, first sentence returns disabled for Back button", () => { SetupIFrameFromHtml( - "
p>Sentence 1.

", + "
p>Sentence 1.

", ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.TextBox; const observed = recording.getPreviousAudioElement(); @@ -310,12 +344,13 @@ describe("audio recording tests", () => { const box1Html = "

Sentence 1.Sentence 2.

"; const box2Html = - "

Sentence 3.

"; + "

Sentence 3.

"; SetupIFrameFromHtml( `
${box1Html}${box2Html}
`, ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.Sentence; const observed = recording.getPreviousAudioElement(); @@ -327,12 +362,13 @@ describe("audio recording tests", () => { const box1Html = "

Sentence 1.Sentence 2.

"; const box2Html = - "

Sentence 3.

"; + "

Sentence 3.

"; SetupIFrameFromHtml( `
${box1Html}${box2Html}
`, ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.TextBox; const observed = recording.getPreviousAudioElement(); @@ -342,9 +378,10 @@ describe("audio recording tests", () => { it("TT <- TT, returns previous box", () => { SetupIFrameFromHtml( - "
p>Sentence 1.

p>Sentence 2.

", + "
p>Sentence 1.

p>Sentence 2.

", ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.TextBox; const observed = recording.getPreviousAudioElement(); @@ -353,19 +390,24 @@ describe("audio recording tests", () => { }); it("Prev() skips over empty box, TT <- TT <- TT", () => { - const boxTemplate = (index: number, extraClasses: string) => { - return `
p>Sentence ${index}.

`; + const boxTemplate = ( + index: number, + extraClasses: string, + preselect = false, + ) => { + return `
p>Sentence ${index}.

`; }; const box1Html = boxTemplate(1, ""); const box2Html = '

'; - const box3Html = boxTemplate(3, " ui-audioCurrent"); + const box3Html = boxTemplate(3, "", true); SetupIFrameFromHtml( `
${box1Html}${box2Html}${box3Html}
`, ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.TextBox; const observed = recording.getPreviousAudioElement(); @@ -378,7 +420,7 @@ describe("audio recording tests", () => { describe("- PlayingMultipleAudio()", () => { it("returns true while in listen to whole page with multiple text boxes", async () => { SetupIFrameFromHtml( - "
p>Sentence 1.

p>Sentence 2.

", + "
p>Sentence 1.

p>Sentence 2.

", ); const recording = new AudioRecording(); await recording.listenAsync(); @@ -396,7 +438,7 @@ describe("audio recording tests", () => { it("returns false while preloading", () => { SetupIFrameFromHtml( - "
p>Sentence 1.

p>Sentence 2.

", + "
p>Sentence 1.

p>Sentence 2.

", ); const recording = new AudioRecording(); @@ -941,7 +983,7 @@ describe("audio recording tests", () => { const textBoxDivHtml = '
'; const paragraphsMarkedBySentenceHtml = - '

Sentence 1. Sentence 2. Sentence 3.

Paragraph 2.

'; + '

Sentence 1. Sentence 2. Sentence 3.

Paragraph 2.

'; const formatButtonHtml = '
'; const originalHtml = @@ -1022,11 +1064,7 @@ describe("audio recording tests", () => { expect( StripAllGuidIds(StripEmptyClasses(parent.html())), "Swap back to original", - ).toBe( - StripAllGuidIds( - StripEmptyClasses(StripAudioCurrent(originalHtml)), - ), - ); + ).toBe(StripAllGuidIds(StripEmptyClasses(originalHtml))); }); it("converts by-text-box into by-sentence (bloom-editable includes format button)", () => { @@ -1129,17 +1167,13 @@ describe("audio recording tests", () => { expect( StripAllGuidIds(StripEmptyClasses(parentDiv.html())), "Swap back to original", - ).toBe( - StripAllGuidIds( - StripEmptyClasses(StripAudioCurrent(originalHtml)), - ), - ); + ).toBe(StripAllGuidIds(StripEmptyClasses(originalHtml))); }); it("loads by-text-box without changing anything", () => { // This tests real input from Bloom that has been marked up in by-text-box mode (e.g., clicking the checkbox from not-by-sentence into by-sentence) const textBoxDivHtml = - '
'; + '
'; const formatButtonHtml = '
'; const originalHtml = @@ -1332,7 +1366,7 @@ describe("audio recording tests", () => { describe("startRecordCurrentAsync", () => { const setupRecordButtonHtml = () => { SetupIFrameFromHtml( - `

Sentence 1.1.

`, + `

Sentence 1.1.

`, ); }; @@ -1398,8 +1432,8 @@ describe("audio recording tests", () => { setupRecordButtonHtml(); let resolveStartRecord: () => void; - const startRecordPromise = new Promise((resolve) => { - resolveStartRecord = resolve; + const startRecordPromise = new Promise((resolve) => { + resolveStartRecord = () => resolve(); }); vi.spyOn(axios, "post").mockImplementation((url: string) => { if (url.startsWith("/bloom/api/audio/startRecord")) { @@ -1455,6 +1489,7 @@ describe("audio recording tests", () => { const runClearRecordingAsync = async (scenario) => { const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); if (scenario === AudioMode.PureSentence) { recording.recordingMode = RecordingMode.Sentence; } else { @@ -1598,17 +1633,18 @@ describe("audio recording tests", () => { // Verification const firstDiv = getFrameElementById("page", "div1")!; - expect(firstDiv).toHaveClass("ui-audioCurrent"); + expect(recording.getAudioCurrentElement()).toBe(firstDiv); }); }); describe("- initializeAudioRecordingMode()", () => { it("initializeAudioRecordingMode gets mode from current div if available (synchronous) (Text Box)", () => { SetupIFrameFromHtml( - "
Sentence 1. Sentence 2.
Paragraph 2.
", + "
Sentence 1. Sentence 2.
Paragraph 2.
", ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.Sentence; // Just to make sure that the code under test can read the current div at all. @@ -1625,10 +1661,11 @@ describe("audio recording tests", () => { it("initializeAudioRecordingMode gets mode from current div if available (synchronous) (Sentence)", () => { SetupIFrameFromHtml( - "
Paragraph 1.
Paragraph 2.
", + "
Paragraph 1.
Paragraph 2.
", ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.Sentence; // Just to make sure that the code under test can read the current div at all. @@ -1645,10 +1682,11 @@ describe("audio recording tests", () => { it("initializeAudioRecordingMode gets mode from other divs on page as fallback (synchronous) (TextBox)", () => { SetupIFrameFromHtml( - "
Paragraph 1
Paragraph 2.
", + "
Paragraph 1
Paragraph 2.
", ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.Sentence; // Just to make sure that the code under test can read the current div at all. @@ -1667,10 +1705,11 @@ describe("audio recording tests", () => { // The 2nd div doesn't really look well-formed because we're trying to get the test to exercise some fallback cases // The first div doesn't look well-formed either but I want the test to exercise that it is getting it from the data-audiorecordingmode attribute not from any of the div's innerHTML markup. SetupIFrameFromHtml( - "
Paragraph 1
Paragraph 2.
", + "
Paragraph 1
Paragraph 2.
", ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.TextBox; // Just to make sure that the code under test can read the current div at all. @@ -1687,10 +1726,11 @@ describe("audio recording tests", () => { it("initializeAudioRecordingMode identifies 4.3 audio-sentences (synchronous)", () => { SetupIFrameFromHtml( - "
Sentence 1. Sentence 2.
Paragraph 2.
", + "
Sentence 1. Sentence 2.
Paragraph 2.
", ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.TextBox; // Just to make sure that the code under test can read the current div at all. @@ -1819,9 +1859,12 @@ describe("audio recording tests", () => { it("getCurrentText works", () => { setupDefaultApiResponses(); - SetupIFrameFromHtml("
Hello world
"); + SetupIFrameFromHtml( + "
Hello world
", + ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); expect(recording.getCurrentHighlight()).toBeTruthy(); const returnedText = recording.getCurrentText(); @@ -1831,10 +1874,11 @@ describe("audio recording tests", () => { it("getAutoSegmentLanguageCode works", () => { setupDefaultApiResponses(); SetupIFrameFromHtml( - "
Hello world
", + "
Hello world
", ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); const returnedText = recording.getAutoSegmentLanguageCode(); expect(returnedText).toBe("es"); @@ -1843,10 +1887,11 @@ describe("audio recording tests", () => { it("extractFragmentsForAudioSegmentation works", () => { setupDefaultApiResponses(); SetupIFrameFromHtml( - "
Sentence 1. Sentence 2.
", + "
Sentence 1. Sentence 2.
", ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); const returnedFragmentIds: AudioTextFragment[] = recording.extractFragmentsAndSetSpanIdsForAudioSegmentation(); @@ -1862,10 +1907,11 @@ describe("audio recording tests", () => { it("extractFragmentsForAudioSegmentation handles duplicate sentences separately", () => { setupDefaultApiResponses(); SetupIFrameFromHtml( - "
What color is the sky? Blue. What color is the ocean? Blue. Hello. Hello.

", + "
What color is the sky? Blue. What color is the ocean? Blue. Hello. Hello.

", ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.TextBox; const returnedFragmentIds: AudioTextFragment[] = recording.extractFragmentsAndSetSpanIdsForAudioSegmentation(); @@ -1963,10 +2009,11 @@ describe("audio recording tests", () => { it("importRecording() encodes special characters", async () => { // Setup const div1 = - '

One. Two. Three.

'; + '

One. Two. Three.

'; SetupIFrameFromHtml(div1); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.recordingMode = RecordingMode.TextBox; // Should be the old state, toggleRecordingMode() will flip the state const baseName = "A`B~C!D@E#F$G%H^I&J(K)L-M_N=O+P[Q{R]S}T;U'V,W.X"; @@ -2077,7 +2124,7 @@ describe("audio recording tests", () => { scenarios.forEach((scenario) => { it(`[${scenario}] doesn't change anything for two or fewer whitespace in split text box w/single highlight segment`, () => { const originalHtml = - '

One Two  Three

'; + '

One Two  Three

'; SetupIFrameFromHtml(originalHtml); const box1 = getFrameElementById("page", "box1")!; @@ -2092,7 +2139,7 @@ describe("audio recording tests", () => { it(`[${scenario}] disables highlight on 3 or more whitespace in split text box w/single highlight segment`, () => { SetupIFrameFromHtml( - '

One Two  Three   Four    End

', + '

One Two  Three   Four    End

', ); const box1 = getFrameElementById("page", "box1")!; @@ -2115,7 +2162,7 @@ describe("audio recording tests", () => { it(`[${scenario}] disables highlight on 3 or more whitespace in split text box w/multiple highlight segments.`, () => { const html = - '

One Two  End1.Three   End2.Four    Five     End3.

'; + '

One Two  End1.Three   End2.Four    Five     End3.

'; SetupIFrameFromHtml(html); const box1 = getFrameElementById("page", "box1")!; @@ -2136,7 +2183,7 @@ describe("audio recording tests", () => { it(`[${scenario}] doesn't do anything on unsplit text box.`, () => { const originalHtml = - '

One Two  End1. Three   End2. Four    Five     End3.

'; + '

One Two  End1. Three   End2. Four    Five     End3.

'; SetupIFrameFromHtml(`
${originalHtml}
`); const box1 = getFrameElementById("page", "box1")!; @@ -2151,7 +2198,7 @@ describe("audio recording tests", () => { it(`[${scenario}] disables highlight on 3 or more whitespace in record-by-sentence box`, () => { const originalHtml = - '

One Two  End1.Three   End2.Four    Five     End3.

'; + '

One Two  End1.Three   End2.Four    Five     End3.

'; SetupIFrameFromHtml( `
${originalHtml}
`, ); @@ -2165,7 +2212,7 @@ describe("audio recording tests", () => { // Verification expect(box1.innerHTML).toBe( "

" + - 'One Two  End1.' + + 'One Two  End1.' + 'Three   End2.' + 'Four    Five     End3.' + "

", @@ -2174,7 +2221,7 @@ describe("audio recording tests", () => { it(`[${scenario}] disables highlight on emphasized text`, () => { const originalHtml = - '

Three   End2.

'; + '

Three   End2.

'; SetupIFrameFromHtml(`
${originalHtml}
`); const box1 = getFrameElementById("page", "box1")!; @@ -2186,14 +2233,14 @@ describe("audio recording tests", () => { // Verification expect(box1.innerHTML).toBe( "

" + - 'Three   End2.' + + 'Three   End2.' + "

", ); }); it(`[${scenario}] disables highlight for ​ (\u200B)`, () => { const originalHtml = - '

Three\u200B \u200BEnd2.

'; + '

Three\u200B \u200BEnd2.

'; SetupIFrameFromHtml( `
${originalHtml}
`, ); @@ -2207,7 +2254,7 @@ describe("audio recording tests", () => { // Verification expect(box1.innerHTML).toBe( "

" + - 'Three\u200B \u200BEnd2.' + + 'Three\u200B \u200BEnd2.' + "

", ); }); @@ -2231,7 +2278,7 @@ describe("audio recording tests", () => { it(`[${scenario}] reverts fixHighlighting() in split text box.`, () => { const originalHtml = - '

One Two  End1.Three   End2.Four    Five     End3.

'; + '

One Two  End1.Three   End2.Four    Five     End3.

'; SetupIFrameFromHtml(`
${originalHtml}
`); const box1 = getFrameElementById("page", "box1")!; @@ -2250,10 +2297,11 @@ describe("audio recording tests", () => { describe("- pseudo-element split highlights", () => { it("registers the current sentence highlight with pseudo-element highlights", async () => { SetupIFrameFromHtml( - '

One.

', + '

One.

', ); const recording = new AudioRecording(); + (recording as unknown as { isShowing: boolean }).isShowing = true; const setHighlightToAsync = ( recording as unknown as { setHighlightToAsync(args: { @@ -2275,10 +2323,11 @@ describe("audio recording tests", () => { it("clears pseudo-element highlights when entering show playback order mode", async () => { SetupIFrameFromHtml( - '

One.

', + '

One.

', ); const recording = new AudioRecording(); + (recording as unknown as { isShowing: boolean }).isShowing = true; const setHighlightToAsync = ( recording as unknown as { setHighlightToAsync(args: { @@ -2305,10 +2354,11 @@ describe("audio recording tests", () => { it("uses only ui-enableHighlight descendants for current highlight when its own background is disabled", async () => { SetupIFrameFromHtml( - '

One   Two

', + '

One   Two

', ); const recording = new AudioRecording(); + (recording as unknown as { isShowing: boolean }).isShowing = true; const setHighlightToAsync = ( recording as unknown as { setHighlightToAsync(args: { @@ -2331,12 +2381,17 @@ describe("audio recording tests", () => { ]); }); - it("copies the current highlight colors from computed highlight styles", async () => { + it("sets default highlight colors when no user styles are configured", async () => { + // Tests that setHighlightToAsync writes the CSS custom properties to the document + // element. When no userModifiedStyles sheet is present the defaults (#febf00, black) + // are used. Reading custom colors from the userModifiedStyles stylesheet requires + // real CSS rule parsing that jsdom's iframe documents don't support. SetupIFrameFromHtml( - '

One.

', + '

One.

', ); const recording = new AudioRecording(); + (recording as unknown as { isShowing: boolean }).isShowing = true; const setHighlightToAsync = ( recording as unknown as { setHighlightToAsync(args: { @@ -2363,20 +2418,21 @@ describe("audio recording tests", () => { documentStyle.getPropertyValue( "--bloom-audio-current-highlight-background", ), - ).toBe("rgb(1, 2, 3)"); + ).toBe("#febf00"); expect( documentStyle.getPropertyValue( "--bloom-audio-current-highlight-color", ), - ).toBe("rgb(4, 5, 6)"); + ).toBe("black"); }); it("registers the split highlight color groups for the current text box", () => { SetupIFrameFromHtml( - '

One.Two.Three.Four.

', + '

One.Two.Three.Four.

', ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.markAudioSplit(); expect(getSplitHighlightTexts()).toEqual([ @@ -2388,10 +2444,11 @@ describe("audio recording tests", () => { it("clears split highlights while playback moves to an individual segment", async () => { SetupIFrameFromHtml( - '

One.Two.

', + '

One.Two.

', ); const recording = new AudioRecording(); + (recording as unknown as { isShowing: boolean }).isShowing = true; recording.markAudioSplit(); const setHighlightToAsync = ( @@ -2413,12 +2470,14 @@ describe("audio recording tests", () => { it("keeps the current highlight when Speak clears a previous split state", async () => { SetupIFrameFromHtml( - '

One.Two.

', + '

One.Two.

', ); vi.spyOn(axios, "post").mockResolvedValue({}); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); + (recording as unknown as { isShowing: boolean }).isShowing = true; recording.recordingMode = RecordingMode.TextBox; document.getElementById("audio-record")?.classList.add("expected"); @@ -2432,10 +2491,11 @@ describe("audio recording tests", () => { it("uses only ui-enableHighlight descendants when a segment disables its own background", () => { SetupIFrameFromHtml( - '

One.Two   Three

', + '

One.Two   Three

', ); const recording = new AudioRecording(); + setHighlightedElementFromDom(recording); recording.markAudioSplit(); expect(getSplitHighlightTexts()).toEqual([ @@ -2468,12 +2528,6 @@ export function StripAllGuidIds(html) { .replace(/ id=""/g, ""); } -function StripAudioCurrent(html) { - return html - .replace(/ ui-audioCurrent/g, "") - .replace(/ class="ui-audioCurrent"/g, ""); -} - export function StripRecordingMd5(html: string): string { return html.replace(/ recordingmd5="[0-9A-Za-z]*"/g, ""); } @@ -2521,7 +2575,7 @@ async function SetupIFrameAsync(id = "page"): Promise { } // bodyContentHtml should not contain HTML or Body tags. It should be the innerHtml of the body -// It might look something like this:
Hello world
+// It might look something like this:
Hello world
export function SetupIFrameFromHtml(bodyContentHtml: string, id = "page") { const iframe = parent.window.document.getElementById(id); diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts index bc7b4bcc0de4..22cd2bfaf2bc 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts @@ -66,6 +66,7 @@ function getHighlightConstructor( } export class AudioTextHighlightManager { + // Remove all current and split highlights from the registry for the document containing contextNode. public clearAllManagedHighlights(contextNode?: Node): void { if (!contextNode) { return; @@ -79,6 +80,7 @@ export class AudioTextHighlightManager { allManagedHighlightNames.forEach((name) => registry.delete(name)); } + // Remove only the split (blue segment) highlights from the registry, leaving the current highlight intact. public clearSplitHighlights(contextNode?: Node): void { if (!contextNode) { return; diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookSpec.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookSpec.ts index bb8e8c0e7311..4625f83aba36 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookSpec.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookSpec.ts @@ -390,16 +390,24 @@ describe("talking book tests", () => { } } + // The new highlight code tracks highlighting via highlightedElement instead of DOM + // class manipulation. setCurrentAudioElementToDefaultAsync() strips ui-audioCurrent + // from all DOM elements as defensive cleanup, so the class will never appear in + // outerHTML after showTool() — strip it from both sides before comparing. + function stripAudioCurrent(html: string): string { + return html.replace(/\s*\bui-audioCurrent\b/g, ""); + } + function verifyHtmlPreserved(_scenario: AudioMode) { const currentHtml1 = getFrameElementById("page", "div1") ?.outerHTML as string; - expect(StripRecordingMd5(currentHtml1)).toBe( - StripRecordingMd5(originalDiv1Html), + expect(StripRecordingMd5(stripAudioCurrent(currentHtml1))).toBe( + StripRecordingMd5(stripAudioCurrent(originalDiv1Html)), ); const currentHtml2 = getFrameElementById("page", "div2") ?.outerHTML as string; - expect(StripRecordingMd5(currentHtml2)).toBe( - StripRecordingMd5(originalDiv2Html), + expect(StripRecordingMd5(stripAudioCurrent(currentHtml2))).toBe( + StripRecordingMd5(stripAudioCurrent(originalDiv2Html)), ); } @@ -429,18 +437,18 @@ describe("talking book tests", () => { return; } - const page1 = getFrameElementById("page", "page1"); - const numCurrents = - page1?.querySelectorAll(".ui-audioCurrent").length; + // The new code tracks the current element via highlightedElement rather than + // the ui-audioCurrent DOM class. Use getCurrentHighlight() instead. + const currentHighlight = theOneAudioRecorder.getCurrentHighlight(); expect( - numCurrents, - "Only 1 item is allowed to be the current: " + page1?.innerHTML, - ).toBe(1); + currentHighlight, + "getCurrentHighlight() should return a non-null element after showTool", + ).not.toBeNull(); switch (scenario) { case AudioMode.PureSentence: { const firstSpan = div.querySelector("span.audio-sentence"); - expect(firstSpan).toHaveClass("ui-audioCurrent"); + expect(currentHighlight).toBe(firstSpan); break; } @@ -448,7 +456,7 @@ describe("talking book tests", () => { case AudioMode.PureTextBox: case AudioMode.HardSplitTextBox: case AudioMode.SoftSplitTextBox: { - expect(div).toHaveClass("ui-audioCurrent"); + expect(currentHighlight).toBe(div); break; } diff --git a/src/BloomBrowserUI/vite.config.mts b/src/BloomBrowserUI/vite.config.mts index 1c2e7344c3c6..87e1329f821d 100644 --- a/src/BloomBrowserUI/vite.config.mts +++ b/src/BloomBrowserUI/vite.config.mts @@ -813,11 +813,18 @@ export default defineConfig(async ({ command }) => { globals: false, // Don't inject global test functions (use imports instead) testTimeout: 30000, // 30 second timeout for async operations teardownTimeout: 10000, // 10s max for after-test cleanup; prevents hung workers from blocking the pool - // Cap parallel workers. On Windows, running too many jsdom workers simultaneously - // may exhaust system resources (TCP connections to localhost, libuv thread pool), causing - // workers to stall indefinitely. This may slightly slow things down on some computers - // but reduces the chance of the tests hanging altogether. - maxWorkers: "70%", + // Use worker threads instead of child-process forks. Vitest 4.0 changed the + // default pool to 'forks', but on Windows the process-creation overhead causes + // workers to time out before they start ("Timeout starting forks runner"). + // The threads pool is lighter-weight and avoids that issue. + pool: "threads", + // Limit concurrent workers. The heavy transform cost of some test files + // (notably PrepareAppStepper.spec.tsx with its deep MUI import chain) saturates + // the CPU while workers are starting, causing other workers to miss the + // hardcoded 5-second vitest startup timeout. With pool: "threads" the + // per-worker startup overhead is negligible, so we can safely use more workers. + // 4 workers roughly halves the wall-clock collect time compared to 2. + maxWorkers: 4, minWorkers: 2, sourcemap: true, // Enable source maps for debugging test code deps: { diff --git a/src/BloomBrowserUI/vitest.setup.ts b/src/BloomBrowserUI/vitest.setup.ts index 93f62fbfe9de..b089cd9542ba 100644 --- a/src/BloomBrowserUI/vitest.setup.ts +++ b/src/BloomBrowserUI/vitest.setup.ts @@ -1,6 +1,11 @@ import "@testing-library/jest-dom/vitest"; import { vi } from "vitest"; +// Tell React 18 that this environment supports act(), suppressing the +// "not configured to support act(...)" warning in every React component test. +(globalThis as unknown as Record).IS_REACT_ACT_ENVIRONMENT = + true; + // Some legacy specs use the Jasmine/Jest-style fail() helper, which vitest does // not provide. Define it globally (throwing an Error) so those assertions both // work at runtime and type-check. Test-only: this file is loaded only by vitest. @@ -44,6 +49,21 @@ console.error = (...args: unknown[]) => { return; } + // React 18 emits two families of act() warnings in jsdom test environments: + // • "not configured to support act()" – the environment flag is absent + // • "not wrapped in act(...)" – an async state update escaped an act() boundary + // Both arise from React 18's concurrent scheduler and from third-party libraries + // (e.g. MUI Accordion/Tooltip transitions) whose timer-based state updates fire + // outside of synchronous act() blocks. All tests still pass; the warnings are + // noise. @testing-library/react suppresses the same patterns for the same reason. + const isReactActWarning = + serialized.includes("not wrapped in act(...)") || + serialized.includes("not configured to support act(...)"); + + if (isReactActWarning) { + return; + } + originalConsoleError(...args); }; From d0e6c107ce46bed6334fc72be587ef7e72336185 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Wed, 1 Jul 2026 10:02:24 -0500 Subject: [PATCH 7/9] More post-review fixes --- .../bookEdit/toolbox/talkingBook/audioRecording.less | 1 + .../bookEdit/toolbox/talkingBook/audioRecordingSpec.ts | 2 +- .../bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts | 3 ++- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less index f129b95be465..1ebbd9f7d567 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less @@ -46,6 +46,7 @@ text-decoration-thickness: 1.1em; text-underline-offset: -0.8em; text-decoration-skip-ink: none; + color: var(--bloom-audio-current-highlight-color, black); } // Note: This highlighting is expected to persist across sessions, but to be hidden // (displayed with the yellow color) while each segment is playing. diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts index bf43adb0d06d..b8c2ae34bcae 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts @@ -95,7 +95,7 @@ const getHighlightTexts = (highlightName: string): string[] => { /** * In BL-15300, the "current" audio element is tracked by AudioRecording.highlightedElement. * Tests mark the intended pre-selected element with data-test-preselect="true" in their - * HTML fixture (never via the production ui-audioCurrent class). This helper reads that + * HTML fixture (never via the obsolete ui-audioCurrent class). This helper reads that * attribute and transfers the element to the recording's highlightedElement field before * calling methods under test. */ diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts index 22cd2bfaf2bc..244c339d3898 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts @@ -297,7 +297,8 @@ export class AudioTextHighlightManager { for (const cssRule of Array.from(userStyles.cssRules)) { const rule = cssRule as CSSStyleRule; if ( - rule.selectorText?.includes(styleName) && + (rule.selectorText?.includes("." + styleName + " ") || + rule.selectorText?.includes("." + styleName + ".")) && rule.selectorText?.includes("ui-audioCurrent") && rule.style.backgroundColor ) { From 089630167a7bd836d3d7603f5df5a724b81b073c Mon Sep 17 00:00:00 2001 From: John Thomson Date: Fri, 17 Jul 2026 09:44:52 -0500 Subject: [PATCH 8/9] Delegate audio-highlight color lookup to StyleEditor (BL-15300) AudioTextHighlightManager.updateCurrentHighlightColors now reads the user's audio-highlight colors via StyleEditor.getAudioHiliteProps instead of its own parallel selectorText scan, so the read and StyleEditor.putAudioHiliteRulesInDom can no longer drift apart (addresses Devin's duplicate-lookup flag). Removed the now-duplicated getHighlightColorsFromUserStyles. getAudioHiliteProps takes an optional documentToUse so the manager can look in the page frame's document. GetRuleForStyle no longer creates a userModifiedStyles sheet as a side effect when only reading (create === false); a read that finds no sheet now returns null directly, which also avoids adding an empty sheet to the page during highlighting. Also gitignore per-developer .claude/settings(.local).json; the shared .claude/CLAUDE.md stays tracked. Co-Authored-By: Claude Opus 4.8 (1M context) --- .gitignore | 4 ++ .../bookEdit/StyleEditor/StyleEditor.ts | 14 ++++- .../talkingBook/audioTextHighlightManager.ts | 60 +++++++------------ 3 files changed, 37 insertions(+), 41 deletions(-) diff --git a/.gitignore b/.gitignore index 83fd7f282c74..075093c8045e 100644 --- a/.gitignore +++ b/.gitignore @@ -204,3 +204,7 @@ critiqueAI.json pr-comments*.md *.stackdump + +# Per-developer Claude Code settings (the shared .claude/CLAUDE.md stays tracked). +.claude/settings.json +.claude/settings.local.json diff --git a/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts b/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts index 0a838ec2d57b..90fb61900414 100644 --- a/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts +++ b/src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts @@ -578,8 +578,12 @@ export default class StyleEditor { create: boolean, documentToUse: Document = document, ): CSSStyleRule | null { - const styleSheet = - this.GetOrCreateUserModifiedStyleSheet(documentToUse); + // When we are only reading (create === false), do not create a userModifiedStyles + // sheet as a side effect: a caller looking up a rule that isn't there should not + // mutate the document. Only create the sheet when we actually intend to add a rule. + const styleSheet = create + ? this.GetOrCreateUserModifiedStyleSheet(documentToUse) + : this.FindExistingUserModifiedStyleSheet(documentToUse); if (styleSheet == null) { return null; } @@ -685,7 +689,10 @@ export default class StyleEditor { } } - public getAudioHiliteProps(styleName: string): { + public getAudioHiliteProps( + styleName: string, + documentToUse: Document = document, + ): { hiliteTextColor: string | undefined; hiliteBgColor: string; } { @@ -694,6 +701,7 @@ export default class StyleEditor { // The two should have the same content, so for reading, we only need one. this.sentenceHiliteRuleSelector, false, + documentToUse, ); const hiliteTextColor = sentenceRule?.style?.getPropertyValue("color") || undefined; diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts index 244c339d3898..d27b6245642e 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts @@ -1,3 +1,5 @@ +import StyleEditor from "../../StyleEditor/StyleEditor"; + const kSegmentClass = "bloom-highlightSegment"; const kEnableHighlightClass = "ui-enableHighlight"; const kDisableHighlightClass = "ui-disableHighlight"; @@ -65,6 +67,18 @@ function getHighlightConstructor( return docWindow?.Highlight; } +// A StyleEditor instance used only to read the user's audio-highlight colors from a book's +// userModifiedStyles sheet. We share StyleEditor's rule lookup rather than duplicating the +// selector-matching logic, so the read and the write (StyleEditor.putAudioHiliteRulesInDom) +// can never drift apart. supportFilesRoot is irrelevant for this read-only use. +let styleEditorForColorLookup: StyleEditor | undefined; +function getStyleEditorForColorLookup(): StyleEditor { + if (!styleEditorForColorLookup) { + styleEditorForColorLookup = new StyleEditor("/bloom/bookEdit"); + } + return styleEditorForColorLookup; +} + export class AudioTextHighlightManager { // Remove all current and split highlights from the registry for the document containing contextNode. public clearAllManagedHighlights(contextNode?: Node): void { @@ -263,57 +277,27 @@ export class AudioTextHighlightManager { ) : undefined; + // Read the user's chosen highlight colors for this style from the same + // userModifiedStyles rules that StyleEditor.putAudioHiliteRulesInDom writes, by + // delegating to StyleEditor's own lookup (looking in the page's document, not the + // toolbox's). When there is no such style, fall back to the default yellow/black. const userColors = styleName - ? this.getHighlightColorsFromUserStyles( - styleSource.ownerDocument, + ? getStyleEditorForColorLookup().getAudioHiliteProps( styleName, + styleSource.ownerDocument, ) : undefined; documentElement.style.setProperty( kCurrentHighlightBackgroundCssVar, - userColors?.backgroundColor ?? "#febf00", + userColors?.hiliteBgColor ?? "#febf00", ); documentElement.style.setProperty( kCurrentHighlightColorCssVar, - userColors?.color ?? "black", + userColors?.hiliteTextColor ?? "black", ); } - // Look in the book's userModifiedStyles sheet for an audio highlight rule for the - // given style name, and return its background-color and color if found. - private getHighlightColorsFromUserStyles( - doc: Document, - styleName: string, - ): { backgroundColor: string; color: string } | undefined { - const userStyles = Array.from(doc.styleSheets).find( - (s) => - (s.ownerNode as Element)?.getAttribute("title") === - "userModifiedStyles", - ); - if (!userStyles) return undefined; - - try { - for (const cssRule of Array.from(userStyles.cssRules)) { - const rule = cssRule as CSSStyleRule; - if ( - (rule.selectorText?.includes("." + styleName + " ") || - rule.selectorText?.includes("." + styleName + ".")) && - rule.selectorText?.includes("ui-audioCurrent") && - rule.style.backgroundColor - ) { - return { - backgroundColor: rule.style.backgroundColor, - color: rule.style.color || "black", - }; - } - } - } catch { - // Stylesheets can throw on cross-origin access; shouldn't happen here - } - return undefined; - } - private shouldShowSplitHighlights( currentHighlight: Element | null, currentTextBox: HTMLElement | null, From 4242d98ea802bb6b0f38c30b1cfd35fe3d0a72e6 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Fri, 17 Jul 2026 11:30:02 -0500 Subject: [PATCH 9/9] Scope BL-11633 position:unset to inline children on block containers (BL-15300) The .bl11633-unset-position() mixin set position:unset on the element it was applied to. That is right for inline background-colored spans (leveled/decodable-reader violation spans, and the sentence span that is the ::highlight range), but wrong for block containers: div.bloom-editable and p carry the absolutely-positioned language-code tags and paragraph/ line-break markers, which rely on the container staying position:relative. Applying the mixin to those blocks made the markers re-anchor and jump to the wrong corner (Devin's "language tags / paragraph markers jump" finding). Split out .bl11633-unset-position-on-children() (inline children only) and use it for div.bloom-editable and p. .bl11633-unset-position() now composes that plus the self-unset and is used only on inline elements. In the edit tab the current highlight is a ::highlight pseudo-element drawn with text-decoration (not a background color), so the block no longer needs a self-unset for the caret bug. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/BloomBrowserUI/bookEdit/css/editMode.less | 37 +++++++++++++------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/css/editMode.less b/src/BloomBrowserUI/bookEdit/css/editMode.less index 1a5b45c6d8d6..b7a956134b73 100644 --- a/src/BloomBrowserUI/bookEdit/css/editMode.less +++ b/src/BloomBrowserUI/bookEdit/css/editMode.less @@ -1036,12 +1036,14 @@ even when the div is focused (as long as it is empty).*/ color: @bloom-buff; } -// BL-11633: Works around a Chromium bug where elements with position:relative and a -// background color cause the caret to disappear. We unset position on the element -// itself and on any inline text-formatting children, since the bug can trigger at -// any level of nesting. -.bl11633-unset-position() { - position: unset; +// BL-11633: Works around a Chromium bug where an element with position:relative and a +// background color makes the caret disappear. The bug can trigger on inline text-formatting +// elements at any level of nesting, so this unsets position on those children wherever they +// appear. Use this (rather than .bl11633-unset-position()) on BLOCK containers such as +// div.bloom-editable and p: those must keep their own position:relative, because the little +// language-code tags and paragraph/line-break markers are positioned relative to them and +// would otherwise jump to the wrong corner. +.bl11633-unset-position-on-children() { em, i, strong, @@ -1051,6 +1053,13 @@ even when the div is focused (as long as it is empty).*/ position: unset; } } +// For an INLINE element that is itself background-colored (e.g. a decodable/leveled-reader +// violation span, or the sentence span that is the ::highlight range), also unset position +// on the element itself. Do not use this on block containers - see the note above. +.bl11633-unset-position() { + position: unset; + .bl11633-unset-position-on-children(); +} /*This block handles marking elements that violate decodable book and leveled reader constraints*/ span.sentence-too-long { @@ -1087,14 +1096,18 @@ span.word-not-found { .bl11633-unset-position(); } -div.bloom-editable { - .bl11633-unset-position(); +// div.bloom-editable and p are block-level and carry the language-code tags and paragraph/ +// line-break markers, which are positioned relative to them, so they must keep their own +// position:relative; only their inline children need position:unset. (In the edit tab the +// current highlight is a ::highlight pseudo-element drawn with text-decoration, not a +// background color, so the block itself does not trigger the BL-11633 caret bug.) +div.bloom-editable, +div.bloom-editable p { + .bl11633-unset-position-on-children(); } -// p elements can be the ::highlight range in text-box recording mode; they need position:unset -// for the same BL-11633 reason (caret hidden behind a background-colored ancestor). -// Sentence spans are the ::highlight range in sentence mode, so they need it too. -div.bloom-editable p, +// Sentence spans / highlight segments are inline and are themselves the ::highlight range in +// sentence mode, so they (and their inline children) need position:unset on the element itself. div.bloom-editable p > .audio-sentence, div.bloom-editable p > .bloom-highlightSegment { .bl11633-unset-position();