fix: improve audio highlighting of titles (BL-16378)#427
Conversation
We no longer highlight the full width of the text box, assuming Adjust Timings has been used. And lines should not interfere with each other in other places where line spacing is tight.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shared/narration.ts`:
- Around line 910-919: The playing/error listeners attached to the persistent
media player (`#bloom-audio-player`) close over highlightElement (a node later
detached by removeAudioCurrent), leaking detached overlay clones for the
session; update the mediaPlayer.addEventListener calls (the "playing" and
"error" listeners in the block referencing highlightElement and
kSuppressHighlightClass/ui-audioCurrent) to register with the { once: true }
option so the handlers auto-remove after firing and do not retain the detached
subtree.
- Around line 805-813: The overlay clone created in
createAudioBackgroundHighlightElement duplicates id attributes (and descendants'
ids) which allows getCurrentNarrationPage().querySelector(`#${currentAudioId}`)
to match the clone instead of the real element; before inserting
overlayParagraph, strip the id attribute from overlayParagraph and recursively
from all descendant elements (the clone's subtree) so the overlay cannot be
matched by id-based queries and subElementsWithTimings will reference the real
DOM; this change ensures playCurrentInternal, subElementsWithTimings,
setHighlightTo, and removeAudioCurrent operate on the original elements, not the
detached clone.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8ccbfd50-005f-4bbb-9bef-a0e584c7cd2b
📒 Files selected for processing (2)
src/bloom-player-content.lesssrc/shared/narration.ts
| highlightElement.classList.add(kSuppressHighlightClass); | ||
| // When it starts playing, we know we really have such an audio file, so we can stop | ||
| // suppressing the highlight. | ||
| mediaPlayer.addEventListener("playing", () => { | ||
| newElement.classList.remove(kSuppressHighlightClass); | ||
| highlightElement.classList.remove(kSuppressHighlightClass); | ||
| }); | ||
| mediaPlayer.addEventListener("error", () => { | ||
| newElement.classList.remove("ui-audioCurrent"); | ||
| newElement.classList.remove(kSuppressHighlightClass); | ||
| highlightElement.classList.remove("ui-audioCurrent"); | ||
| highlightElement.classList.remove(kSuppressHighlightClass); | ||
| }); |
There was a problem hiding this comment.
playing/error listeners now leak detached overlay clones.
These listeners are registered on the persistent #bloom-audio-player (reused across pages/segments for the whole session) and are never removed. They now close over highlightElement, which is a child of the overlay clone that removeAudioCurrent later detaches — so each highlighted segment retains an entire detached <p> subtree for the life of the session. Adding { once: true } removes the common accumulation; the listener still lingers only if neither event fires before the element is superseded.
🧹 Proposed fix
- mediaPlayer.addEventListener("playing", () => {
- highlightElement.classList.remove(kSuppressHighlightClass);
- });
- mediaPlayer.addEventListener("error", () => {
- highlightElement.classList.remove("ui-audioCurrent");
- highlightElement.classList.remove(kSuppressHighlightClass);
- });
+ mediaPlayer.addEventListener(
+ "playing",
+ () => {
+ highlightElement.classList.remove(kSuppressHighlightClass);
+ },
+ { once: true },
+ );
+ mediaPlayer.addEventListener(
+ "error",
+ () => {
+ highlightElement.classList.remove("ui-audioCurrent");
+ highlightElement.classList.remove(kSuppressHighlightClass);
+ },
+ { once: true },
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| highlightElement.classList.add(kSuppressHighlightClass); | |
| // When it starts playing, we know we really have such an audio file, so we can stop | |
| // suppressing the highlight. | |
| mediaPlayer.addEventListener("playing", () => { | |
| newElement.classList.remove(kSuppressHighlightClass); | |
| highlightElement.classList.remove(kSuppressHighlightClass); | |
| }); | |
| mediaPlayer.addEventListener("error", () => { | |
| newElement.classList.remove("ui-audioCurrent"); | |
| newElement.classList.remove(kSuppressHighlightClass); | |
| highlightElement.classList.remove("ui-audioCurrent"); | |
| highlightElement.classList.remove(kSuppressHighlightClass); | |
| }); | |
| highlightElement.classList.add(kSuppressHighlightClass); | |
| // When it starts playing, we know we really have such an audio file, so we can stop | |
| // suppressing the highlight. | |
| mediaPlayer.addEventListener( | |
| "playing", | |
| () => { | |
| highlightElement.classList.remove(kSuppressHighlightClass); | |
| }, | |
| { once: true }, | |
| ); | |
| mediaPlayer.addEventListener( | |
| "error", | |
| () => { | |
| highlightElement.classList.remove("ui-audioCurrent"); | |
| highlightElement.classList.remove(kSuppressHighlightClass); | |
| }, | |
| { once: true }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/shared/narration.ts` around lines 910 - 919, The playing/error listeners
attached to the persistent media player (`#bloom-audio-player`) close over
highlightElement (a node later detached by removeAudioCurrent), leaking detached
overlay clones for the session; update the mediaPlayer.addEventListener calls
(the "playing" and "error" listeners in the block referencing highlightElement
and kSuppressHighlightClass/ui-audioCurrent) to register with the { once: true }
option so the handlers auto-remove after firing and do not retain the detached
subtree.
There was a problem hiding this comment.
I think we dealt with this, avoiding adding event listeners to cloned elements.
There was a problem hiding this comment.
@JohnThomson — Thanks for the clarification! Just to make sure we're on the same page: my concern wasn't about listeners being attached to cloned elements, but rather about the playing/error listeners on the persistent mediaPlayer closing over highlightElement (which can be a child of the overlay clone). That closure keeps a reference to the detached clone subtree alive on the mediaPlayer object for the duration of the session.
If the overlay path is never reached when disableHighlightIfNoAudio is true (e.g., the element is always returned as-is in that code path), then the concern is already moot. If that's what you've arranged, happy to close this out. Otherwise the { once: true } option on those two listeners would still be a cheap way to ensure the closure is released after the first fire.
There was a problem hiding this comment.
I believe the current changes ensure that the listeners only close over non-clone elements.
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson made 2 comments and resolved 1 discussion.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion.
| highlightElement.classList.add(kSuppressHighlightClass); | ||
| // When it starts playing, we know we really have such an audio file, so we can stop | ||
| // suppressing the highlight. | ||
| mediaPlayer.addEventListener("playing", () => { | ||
| newElement.classList.remove(kSuppressHighlightClass); | ||
| highlightElement.classList.remove(kSuppressHighlightClass); | ||
| }); | ||
| mediaPlayer.addEventListener("error", () => { | ||
| newElement.classList.remove("ui-audioCurrent"); | ||
| newElement.classList.remove(kSuppressHighlightClass); | ||
| highlightElement.classList.remove("ui-audioCurrent"); | ||
| highlightElement.classList.remove(kSuppressHighlightClass); | ||
| }); |
There was a problem hiding this comment.
I think we dealt with this, avoiding adding event listeners to cloned elements.
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson made 2 comments.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions.
a discussion (no related file):
Reviewer please consider carefully whether this is a good idea. It might be too much risk and complexity to add for a small visual improvement. OTOH, a real user was quite bothered by the whole-text-box title highlighting.
| highlightElement.classList.add(kSuppressHighlightClass); | ||
| // When it starts playing, we know we really have such an audio file, so we can stop | ||
| // suppressing the highlight. | ||
| mediaPlayer.addEventListener("playing", () => { | ||
| newElement.classList.remove(kSuppressHighlightClass); | ||
| highlightElement.classList.remove(kSuppressHighlightClass); | ||
| }); | ||
| mediaPlayer.addEventListener("error", () => { | ||
| newElement.classList.remove("ui-audioCurrent"); | ||
| newElement.classList.remove(kSuppressHighlightClass); | ||
| highlightElement.classList.remove("ui-audioCurrent"); | ||
| highlightElement.classList.remove(kSuppressHighlightClass); | ||
| }); |
There was a problem hiding this comment.
I believe the current changes ensure that the listeners only close over non-clone elements.
1 participant
We no longer highlight the full width of the text box, assuming Adjust Timings has been used. And lines should not interfere with each other in other places where line spacing is tight.
This change is
Summary by CodeRabbit