Improve highlighting in edit not to modify dom (BL-15300) by JohnThomson · Pull Request #8016 · BloomBooks/BloomDesktop

JohnThomson · 2026-06-29T19:34:41Z

By JohnT and Noel (and Claude and probably CoPilot)

This change is

Completely removed the need for Bloom editing to set a class on the highlighted element.

- 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

greptile-apps · 2026-06-29T19:41:35Z

Greptile Summary

This PR replaces DOM-class-based audio highlighting (ui-audioCurrent) with the CSS Custom Highlight API (::highlight()) in the edit tab, fixing BL-15300 where Chromium would migrate computed styles into inline styles inside contenteditable elements, causing highlights to persist incorrectly in saved books.

AudioTextHighlightManager is introduced to own all CSS highlight-registry operations; AudioRecording replaces the DOM class with an in-memory highlightedElement field and a new updateIconMarker method that positions the microphone icon inside #page-scaling-container (outside page content) using absolute coordinates computed from getClientRects().
StyleEditor.GetOrCreateAndGetRuleForSelector is fixed to use FindExistingUserModifiedStyleSheet (not GetOrCreate…) when create === false, preventing a side-effect stylesheet creation on every color-lookup call.
AdjustTimingsDialog stops reaching into the DOM for the current text box; it now receives currentTextBox as a prop at open time, and its associated getCurrentTextBox() helper is deleted.

Important Files Changed

Filename	Overview
src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts	New class managing CSS Custom Highlight API registrations for current and split highlights; core of the BL-15300 fix.
src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts	Replaces DOM class (ui-audioCurrent) tracking with in-memory highlightedElement field; moves icon marker outside page content into #page-scaling-container.
src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts	Fixes GetOrCreateAndGetRuleForSelector to use FindExistingUserModifiedStyleSheet when create=false, preventing unintended stylesheet creation as a side-effect of a read.
src/BloomBrowserUI/bookEdit/toolbox/talkingBook/AdjustTimingsDialog.tsx	Removes DOM lookup of current text box via kAudioCurrent; now receives currentTextBox as a prop passed at dialog open time.
src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less	Replaces background-color CSS rules on ui-audioCurrent with ::highlight() pseudo-element rules using thick underline offsets; icon now uses CSS mask instead of background-image.
src/BloomBrowserUI/bookEdit/css/editMode.less	Extracts BL-11633 position:unset workaround into reusable LESS mixins; broadens the fix to cover div.bloom-editable p children.
src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts	Adds CSS Highlight API polyfill infrastructure for tests; replaces ui-audioCurrent in fixtures with data-test-preselect; adds new tests for highlight manager behavior.
src/BloomBrowserUI/bookEdit/toolbox/talkingBook/talkingBookSpec.ts	Updates assertions to use getCurrentHighlight() instead of checking ui-audioCurrent class; strips residual ui-audioCurrent from both sides of HTML comparisons.
src/BloomBrowserUI/bookEdit/workspaceRoot.ts	Adds currentTextBox parameter to showAdjustTimingsDialogFromWorkspaceRoot interface to match the AdjustTimingsDialog signature change.
src/content/bookLayout/basePage-sharedRules.less	Comment-only update noting that ui-audioCurrent background-color rules now serve only Bloom Player / ePUB, not the edit tab.
src/BloomBrowserUI/vite.config.mts	Switches Vitest pool from forks to threads (fixes Windows startup timeouts with Vitest 4.0) and pins maxWorkers to 4.
src/BloomBrowserUI/vitest.setup.ts	Sets IS_REACT_ACT_ENVIRONMENT and suppresses known-noisy React 18 act() warnings from MUI transitions.
src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditorSpec.ts	Minor test updates accompanying the StyleEditor API change (documentToUse parameter added to getAudioHiliteProps).
.gitignore	Adds per-developer Claude Code settings files to .gitignore while keeping shared CLAUDE.md tracked.

_{Reviews (7): Last reviewed commit: "Scope BL-11633 position:unset to inline ..." | Re-trigger Greptile}

hatton · 2026-07-01T23:14:08Z

@JohnThomson Devin has two unresolved flags. Can you look at those and resolve them?

JohnThomson

@JohnThomson made 1 comment and resolved 1 discussion.
Reviewable status: 0 of 13 files reviewed, all discussions resolved (waiting on JohnThomson).

StephenMcConnel

I saw one or two possible trivial tweaks, but nothing worth mentioning.

My only question is whether or not you've looked at and resolved the Devin tags that JH mentioned at the top of the review.

@StephenMcConnel reviewed 13 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on JohnThomson).

JohnThomson · 2026-07-17T14:04:27Z

🤖 Claude (Opus 4.8): Consulted Devin on 2026-07-17 09:03 CDT up to commit d0e6c10.

Devin's analysis for this commit is complete. Current findings: 0 unresolved Bugs (Devin had already marked 4 bugs Resolved) and 3 Investigate flags; all Informational items were skipped as low-signal.

The 3 Investigate flags were mirrored as inline review threads:

::highlight CSS rules must reside in the page iframe's stylesheet — assessed as not an issue and resolved (audioRecording.css is injected into the page frame via editablePage.ts, the same document as the CSS.highlights registry). See the thread on audioRecording.less.
Stale highlightedElement reference after DOM mutations — assessed as not an issue and resolved (the code guards with .isConnected, id re-query, and ordinal-index recovery). See the thread on audioRecording.ts:182.
Duplicate highlight-color lookup between AudioTextHighlightManager and StyleEditor — left open for the developer: it is a design/DRY judgment call (whether the manager should delegate to StyleEditor.getAudioHiliteProps), not a correctness bug. See the thread on audioTextHighlightManager.ts:285-315.

CI, Greptile, and Reviewable are all green/clean for this commit.

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) <noreply@anthropic.com>

JohnThomson

@JohnThomson+AGNT resolved 3 discussions.
Reviewable status: 11 of 14 files reviewed, all discussions resolved (waiting on JohnThomson and StephenMcConnel).

JohnThomson · 2026-07-17T15:04:23Z

🤖 Claude (Opus 4.8): Consulted Devin again on 2026-07-17 (up to commit 0896301).

Re-review of the new commit is complete. Devin now marks the duplicate highlight-color lookup flag Resolved (fixed by the delegation refactor), and logged Informational notes acknowledging the change (read-only StyleEditor cache, GetRuleForStyle no longer creating the sheet on read).

The re-review surfaced two new findings, both mirrored as inline threads and left open for the developer (each has my assessment as a reply):

Bug — Language tags / paragraph markers may attach to the wrong container because the BL-11633 .bl11633-unset-position() mixin sets position: unset on div.bloom-editable and paragraphs, not just inline children. Plausible; needs visual verification in the edit tab; risky to auto-fix (BL-11633 workaround). See the thread on editMode.less:1090.
Investigate — refreshHighlights(suppressCurrentHighlight=true) deletes the persistent blue split highlights along with the yellow current one, so landing on a no-audio sentence may wipe a text box's splits. UX call. See the thread on audioTextHighlightManager.ts:132-135.

The previously-mirrored ::highlight-in-page-frame and stale-reference flags reappeared with the same titles on the re-review; both were already assessed as non-issues and resolved, so they were not re-posted. All previously-mirrored threads are resolved on GitHub and in Reviewable.

…(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) <noreply@anthropic.com>

JohnThomson · 2026-07-17T16:38:15Z

🤖 Claude (Opus 4.8): Consulted Devin again on 2026-07-17 (up to commit 4242d98).

Re-review of the mixin fix is complete and clean. Devin now marks the "language tags / paragraph markers jump" Bug Resolved (fixed by scoping .bl11633-unset-position() to inline children on block containers). 0 unresolved bugs, no new findings.

The only still-open flag is "Suppressing current highlight also clears persistent split highlights" (audioTextHighlightManager.ts:132-135) — already mirrored and left open awaiting the developer's UX decision. The ::highlight-in-page-frame and stale-reference flags reappeared with the same titles and are already resolved, so they were not re-posted. All other mirrored threads (duplicate-lookup, both non-issues, and this Bug) are resolved on GitHub and in Reviewable.

JohnThomson

@JohnThomson+AGNT resolved 1 discussion.
Reviewable status: 10 of 14 files reviewed, 1 unresolved discussion (waiting on JohnThomson and StephenMcConnel).

JohnThomson

@JohnThomson+AGNT resolved 1 discussion.
Reviewable status: 10 of 14 files reviewed, all discussions resolved (waiting on JohnThomson and StephenMcConnel).

JohnThomson

@JohnThomson made 1 comment.
Reviewable status: 10 of 14 files reviewed, 1 unresolved discussion (waiting on JohnThomson and StephenMcConnel).

a discussion (no related file):
Last attempt there were still a couple of Devin flags...two of which proved to be real bugs. Now fixed. Clean preflight report at https://claude.ai/code/artifact/d50d66af-8a59-498b-806d-57122bd218d9

nabalone and others added 4 commits June 29, 2026 14:23

BL-15300 switch edit tab highlighting to pseudo element

a71b028

code review, renaming etc.

a946e44

More work on highlight (BL-15300)

d388076

Completely removed the need for Bloom editing to set a class on the highlighted element.

github-actions Bot assigned JohnThomson Jun 29, 2026

greptile-apps Bot reviewed Jun 29, 2026

View reviewed changes

Comment thread src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts Outdated

JohnThomson added 2 commits June 29, 2026 16:23

post-review cleanup on BL-15300

e11f4ab

Get tests passing

cdbd06b

JohnThomson force-pushed the improveHighlighting branch from f8f5436 to cdbd06b Compare June 30, 2026 22:38

greptile-apps Bot reviewed Jun 30, 2026

View reviewed changes

Comment thread src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts Outdated

More post-review fixes

d0e6c10

JohnThomson force-pushed the improveHighlighting branch from 8bbc444 to d0e6c10 Compare July 2, 2026 17:06

JohnThomson commented Jul 2, 2026

View reviewed changes

Comment thread src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts Outdated

StephenMcConnel reviewed Jul 16, 2026

View reviewed changes