BL-15300 Switch TBT highlighting to pseudoelement

[nabalone]

---

---

This change is 

[hatton]

Should we add a comment here explaining why we decided to have a different mechanism here versus what we do in the player? Reading over the issue, I gather that it's because there are situations where having the highlight in the DOM while editing can lead to messed up HTML that hard codes the highlighting spans or something like that?

[JohnThomson]

Looks like we have a pretty good explanation of why we use the highlight API here, but may be helpful to explain why we don't use it in bloom-player.

[nabalone]

Done.

[JohnThomson]

Overall, I like the approach. It's great that we can do highlighting without modifying the document (much).
Could we go a step further? Seems like code somewhere is deciding which element should get the classes that the new highlight manager is looking for, like audio-current and classes that say whether to show the highlights. If we communicated directly from that code to the highlight manager, could we manage audio highlights without modifying the DOM at all?
Not sure whether that is worth it for 6.4.

@JohnThomson reviewed 1 file and all commit messages, and made 8 comments.
Reviewable status: 1 of 12 files reviewed, 7 unresolved discussions (waiting on hatton and nabalone).

---

src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less line 38 at r8 (raw file):

// 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 {

I suggest we do this by changing the basePage.less rules to apply inside body:not(.bloom-audio-customHighlights). That way,

• the CSS is all in one place; we don't have to know that highlighting is affected by rules in two places
• we don't have to keep the new rules in sync and make sure they have higher specificity
• if we decide we want bloom-player to work this way, the new version just adds this class to the body and implements the new approach.

---

src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less line 100 at r8 (raw file):

    span {
        position: unset; // BL-11633, works around Chromium bug
    }

Did you test that the highlighting doesn't have the same Chromium bug as normal background color, hiding the cursor in position:relative elements?

---

src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts line 7 at r8 (raw file):

const kPostAudioSplitClass = "bloom-postAudioSplit";
const kTextBoxRecordingMode = "textbox";
const kCustomHighlightSupportClass = "bloom-audio-customHighlights";

Not keen on using 'custom' to name this. To me, that suggests that this is about something like customizing the colors used for highlighting. But really, this is about how highlighting is implemented: by CSS or using the ::highlight API. How about bloom-highlightAudioUsingRegistry? (Needs a change in CSS too, if you agree.)

---

src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts line 294 at r8 (raw file):

    }

    private updateCurrentHighlightColors(styleSource: Element): void {

I'm not getting why we need this. The CSS already uses --bloom-audio-current-highlight-background for the appropriate registry entry, and presumably something in the highlight-color dialog code arranges for there to be a rule that sets that. Why doesn't it just work?

---

src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts line 372 at r8 (raw file):

                .getAttribute("data-audiorecordingmode")
                ?.toLowerCase() === kTextBoxRecordingMode
        );

It feels vaguely wrong to me that we have code somewhere that is deciding whether we should show the highlights. It tries to control that by setting classes on things. The normal expectation would be that we are setting classes so that CSS rules can govern whether the highlight shows. But we don't have such rules; we have more code that looks for the classes and uses the information to decide whether to put stuff in the highlight registry.
Seems like we should either (a) have the code here always create the highlight registry entries, but use the classes in the ::highlight rule to suppress any visible sign of them; or (b) change the code that decides whether to show the highlights so that it communicates directly with the highlightManager to tell it whether to create the highlight registry entries. I lean toward (b).

---

src/content/bookLayout/basePage-sharedRules.less line 33 at r8 (raw file):

    background-color: #febf00;
    color: black;
}

Why not just set defaults for the variables, and have these rules (and the ::highlight ones) say to use the variables?

[JohnThomson]

Looks like we have a pretty good explanation of why we use the highlight API here, but may be helpful to explain why we don't use it in bloom-player.

[JohnThomson]

@JohnThomson reviewed 11 files and made 1 comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on hatton and nabalone).

---

src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts line 32 at r8 (raw file):

};

const installCustomHighlightPolyfill = (targetWindow: Window) => {

Do we need this? You might try telling CoPilot that this code only needs to run on Webview2 (and I suppose whatever the test harness uses) and see what polyfills etc it can drop.

[nabalone]

We discussed in meeting. See comments in card.

@nabalone made 6 comments.
Reviewable status: 9 of 12 files reviewed, 8 unresolved discussions (waiting on hatton and JohnThomson).

---

src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less line 38 at r8 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I suggest we do this by changing the basePage.less rules to apply inside body:not(.bloom-audio-customHighlights). That way,

• the CSS is all in one place; we don't have to know that highlighting is affected by rules in two places
• we don't have to keep the new rules in sync and make sure they have higher specificity
• if we decide we want bloom-player to work this way, the new version just adds this class to the body and implements the new approach.

Noting that this should become irrelevant after the changes we discussed

---

src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.less line 100 at r8 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Did you test that the highlighting doesn't have the same Chromium bug as normal background color, hiding the cursor in position:relative elements?

Yes, and I flagged it in testing notes also

---

src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts line 32 at r8 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Do we need this? You might try telling CoPilot that this code only needs to run on Webview2 (and I suppose whatever the test harness uses) and see what polyfills etc it can drop.

copilot says "The reason the polyfill exists is that ...Vitest is configured to use jsdom, and this spec even has other jsdom-specific polyfills around the iframe setup"

---

src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioTextHighlightManager.ts line 7 at r8 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Not keen on using 'custom' to name this. To me, that suggests that this is about something like customizing the colors used for highlighting. But really, this is about how highlighting is implemented: by CSS or using the ::highlight API. How about bloom-highlightAudioUsingRegistry? (Needs a change in CSS too, if you agree.)

I changed "custom highlight" to "pseudo highlight" so at least its not confusing, but we should revisit all these names after the changes we discussed

[nabalone]

Done.