Skip to content

BL-15300 Switch TBT highlighting to pseudoelement#7754

Open
nabalone wants to merge 1 commit intomasterfrom
BL-15300_switch_tbt_highlighting_to_pseudoelement
Open

BL-15300 Switch TBT highlighting to pseudoelement#7754
nabalone wants to merge 1 commit intomasterfrom
BL-15300_switch_tbt_highlighting_to_pseudoelement

Conversation

@nabalone
Copy link
Contributor

@nabalone nabalone commented Mar 18, 2026


Open with Devin

This change is Reviewable

Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

View 2 additional findings in Devin Review.

Open in Devin Review

Comment on lines +17 to +44
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;
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Suggested change
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;
};
function getDocumentWindow(contextNode: Node): Window | undefined {
return contextNode.ownerDocument?.defaultView ?? 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;
}
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +4411 to +4413
this.audioTextHighlightManager.clearManagedHighlights(
currentTextBox ?? undefined,
);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant