BL-15300 Switch TBT highlighting to pseudoelement#7754
BL-15300 Switch TBT highlighting to pseudoelement#7754
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const getDocumentWindow = (contextNode: Node): Window | undefined => { | ||
| return contextNode.ownerDocument?.defaultView ?? undefined; | ||
| }; | ||
|
|
||
| const 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; | ||
| }; | ||
|
|
||
| const getHighlightConstructor = ( | ||
| contextNode: Node, | ||
| ): HighlightConstructor | undefined => { | ||
| const docWindow = getDocumentWindow(contextNode) as | ||
| | (Window & { | ||
| Highlight?: HighlightConstructor; | ||
| }) | ||
| | undefined; | ||
| return docWindow?.Highlight; | ||
| }; |
There was a problem hiding this comment.
🟡 Module-level functions use arrow-function syntax instead of function declarations
The src/BloomBrowserUI/AGENTS.md rule states: "For functions, prefer typescript 'function' syntax over const foo = () => functions." Three new module-level functions in audioTextHighlightManager.ts (getDocumentWindow, getHighlightRegistry, getHighlightConstructor) are defined as const arrow-function assignments instead of using the function keyword. The existing codebase consistently follows the function declaration pattern for standalone utility functions.
Was this helpful? React with 👍 or 👎 to provide feedback.
| this.audioTextHighlightManager.clearManagedHighlights( | ||
| currentTextBox ?? undefined, | ||
| ); |
There was a problem hiding this comment.
🚩 clearAudioSplit silently skips highlight cleanup when currentTextBox is null
At audioRecording.ts:4411-4413, if getCurrentTextBox() returns null, clearManagedHighlights receives undefined and returns immediately at audioTextHighlightManager.ts:48-49 — leaving any existing ::highlight() entries in the registry. With the old CSS approach, removing the bloom-postAudioSplit class from the element was sufficient since the CSS selectors would stop matching. With the new approach, the registry entries persist independently. In practice this is mitigated because removeAudioCurrentFromPageDocBody also clears highlights and is called on most highlight transitions. But if a code path calls clearAudioSplit expecting to remove the visual split colors, and no highlight change follows, the colors could linger.
Was this helpful? React with 👍 or 👎 to provide feedback.
nabalone
force-pushed
the
BL-15300_switch_tbt_highlighting_to_pseudoelement
branch
from
3dd43e2 to
d20ed54
Compare
| expect(sentenceRule?.cssText).toContain( | ||
| "--bloom-audio-highlight-color: rgb(1, 2, 3)", | ||
| ); |
There was a problem hiding this comment.
🔴 Test uses wrong CSS variable name --bloom-audio-highlight-color instead of --bloom-audio-highlight-text-color
The test at line 348 asserts that sentenceRule?.cssText contains "--bloom-audio-highlight-color: rgb(1, 2, 3)", but the production code in StyleEditor.ts:688 sets the property as "--bloom-audio-highlight-text-color". The substring --bloom-audio-highlight-color does not appear in --bloom-audio-highlight-text-color (which has an extra text- segment), so the toContain assertion will fail.
| expect(sentenceRule?.cssText).toContain( | |
| "--bloom-audio-highlight-color: rgb(1, 2, 3)", | |
| ); | |
| expect(sentenceRule?.cssText).toContain( | |
| "--bloom-audio-highlight-text-color: rgb(1, 2, 3)", | |
| ); |
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| it("copies the current highlight colors from CSS variables", async () => { | ||
| SetupIFrameFromHtml( | ||
| '<div id="page1"><div class="bloom-editable" data-audiorecordingmode="Sentence"><p><span id="span1" class="audio-sentence ui-audioCurrent" style="--bloom-audio-highlight-background: rgb(1, 2, 3); --bloom-audio-highlight-color: rgb(4, 5, 6);">One.</span></p></div></div>', |
There was a problem hiding this comment.
🔴 Test inline style uses wrong CSS variable name --bloom-audio-highlight-color instead of --bloom-audio-highlight-text-color
The test HTML at line 2199 sets the inline style --bloom-audio-highlight-color: rgb(4, 5, 6) on the span, but audioTextHighlightManager.ts:10 reads via kCurrentHighlightColorSourceCssVar which is "--bloom-audio-highlight-text-color". Since the wrong variable name is set on the element, getComputedStyle in audioTextHighlightManager.ts:299-300 will not find it, and the assertion at line 2234 that expects --bloom-audio-current-highlight-color to be "rgb(4, 5, 6)" will fail.
| '<div id="page1"><div class="bloom-editable" data-audiorecordingmode="Sentence"><p><span id="span1" class="audio-sentence ui-audioCurrent" style="--bloom-audio-highlight-background: rgb(1, 2, 3); --bloom-audio-highlight-color: rgb(4, 5, 6);">One.</span></p></div></div>', | |
| '<div id="page1"><div class="bloom-editable" data-audiorecordingmode="Sentence"><p><span id="span1" class="audio-sentence ui-audioCurrent" style="--bloom-audio-highlight-background: rgb(1, 2, 3); --bloom-audio-highlight-text-color: rgb(4, 5, 6);">One.</span></p></div></div>', |
Was this helpful? React with 👍 or 👎 to provide feedback.
| updatedCssRules, | ||
| Does.Contain( | ||
| ".Bubble-style span.ui-audioCurrent { background-color: transparent; color: rgb(0, 255, 255); }" | ||
| ".Bubble-style span.ui-audioCurrent { --bloom-audio-highlight-background: transparent; --bloom-audio-highlight-color: rgb(0, 255, 255); background-color: transparent; color: rgb(0, 255, 255); }" |
There was a problem hiding this comment.
🔴 C# test assertions use wrong CSS variable name --bloom-audio-highlight-color instead of --bloom-audio-highlight-text-color
Multiple test assertions in AddMissingAudioHighlightRules_WorksForMissing use the CSS variable name --bloom-audio-highlight-color, but HtmlDom.cs:3853 defines highlightColorVariable = "--bloom-audio-highlight-text-color". The AddAudioHighlightCssVariables method inserts --bloom-audio-highlight-text-color into the CSS text, so Does.Contain("--bloom-audio-highlight-color") will not find a match (the substring highlight-color is not present in highlight-text-color). This affects assertions at lines 1270, 1282, and 1294.
Prompt for agents
In src/BloomTests/Book/HtmlDomTests.cs, replace all occurrences of the CSS variable name `--bloom-audio-highlight-color` with `--bloom-audio-highlight-text-color` in the test assertion strings. This affects:
Line 1270: Change `--bloom-audio-highlight-color:` to `--bloom-audio-highlight-text-color:` in the Bubble-style sentence rule assertion
Line 1282: Change `--bloom-audio-highlight-color:` to `--bloom-audio-highlight-text-color:` in the Bubble-style enableHighlight rule assertion
Line 1294: Change `--bloom-audio-highlight-color:` to `--bloom-audio-highlight-text-color:` in the standalone variable assertion
Line 1339: Change `--bloom-audio-highlight-color:` to `--bloom-audio-highlight-text-color:` in the WorksForNoneMissing Bubble-style sentence rule assertion
Line 1345: Change `--bloom-audio-highlight-color:` to `--bloom-audio-highlight-text-color:` in the WorksForNoneMissing Bubble-style enableHighlight rule assertion
Line 1351: Change `--bloom-audio-highlight-color:` to `--bloom-audio-highlight-text-color:` in the WorksForNoneMissing Bubble-style paragraph rule assertion
The production code in src/BloomExe/Book/HtmlDom.cs line 3853 uses `--bloom-audio-highlight-text-color` as the variable name.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
This change is