diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts index 3afef01cc193..977d0dbe2275 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts @@ -4126,6 +4126,16 @@ export default class AudioRecording implements IAudioRecorder { ); this.updateButtonStateHelper(expectedVerb, response); + + // The helper just recomputed this.haveAudio from the server, for whatever element is + // current now. The Advanced section's Recording Mode radio buttons are driven by values + // derived from it, so they have to be recomputed here too. Several paths that change which + // box is current (clicking in another text box, Next/Back, typing new text) refresh the + // buttons through here and nothing else, which left the radio buttons describing the + // *previous* box. That is BL-16632: after recording By Whole Text Box and then clicking a + // box with no recording, "By Sentence" stayed disabled, saying to use Clear first, even + // though Clear itself was (correctly) disabled because that box has no audio. + this.updateAudioStateInDisplay(); } private updateButtonStateHelper( @@ -4484,6 +4494,13 @@ export default class AudioRecording implements IAudioRecorder { // 1- When calling changeStateAndSetExpected() with expectedVerb = "" // 2- While doing auto segmenting // 3- An unrecoverable error has occurred + // Every path that gets here has nothing recordable selected: the page has no editable + // text, the click landed on something that can't be recorded, or we could not work out + // what is current. So the current selection has no audio, by definition. Saying so keeps + // the Advanced section honest -- otherwise it goes on reporting the audio of the box we + // were on before, which is the BL-16632 mismatch ("By Sentence" disabled telling you to + // press Clear, while Clear is disabled because there is nothing to clear). + this.haveAudio = false; this.setStatus("record", Status.Disabled); this.setStatus("play", Status.Disabled); this.setStatus("split", Status.Disabled); @@ -4491,6 +4508,12 @@ export default class AudioRecording implements IAudioRecorder { this.setStatus("prev", Status.Disabled); this.setStatus("clear", Status.Disabled); this.setStatus("listen", Status.Disabled); + + // The Advanced section's controls (the Recording Mode radio buttons, Insert Segment + // Marker) are driven by derived values that have to be refreshed here too. Otherwise + // deleting all the text on a page disables every button but leaves those controls enabled, + // still describing the text that used to be there (BL-16632). + this.updateAudioStateInDisplay(); } private showBusy(): void { @@ -4738,6 +4761,30 @@ export default class AudioRecording implements IAudioRecorder { await this.setShowPlaybackOrderMode(isOn); } + // Recompute just the parts of the display that depend on the current audio state: whether the + // current selection has a recording, whether that recording is a whole-text-box one, and + // whether the page has anything recordable. This is what the button-state refresh invalidates + // when it recomputes this.haveAudio, so that refresh calls this rather than the whole of + // updateDisplay (BL-16632). Deliberately narrow in two ways: + // - It does not publish the *mode* fields (recordingMode, inShowPlaybackOrderMode, + // showingImageDescriptions). Those belong to the code that changes those modes, and + // republishing them from a button refresh fights it -- e.g. it would undo the uiState reset + // removePlaybackOrderUi deliberately makes on page change while leaving its own field set. + // - It never *changes* anything, so it can't disturb a caller further up the stack: no + // highlight is chosen (which would recurse back here via getCurrentTextBox), and + // this.recordingMode is left alone -- updateDisplay's subscription downgrade could + // otherwise switch the mode out from under button statuses just computed for the old one. + private updateAudioStateInDisplay(): void { + const hasRecordableDivs = + this.getRecordableDivs(true, false).length > 0; + const currentPlaybackMode = this.getCurrentPlaybackMode(false); + this.uiState.haveACurrentTextboxModeRecording = + this.haveAudio && currentPlaybackMode === RecordingMode.TextBox; + this.uiState.hasAudio = this.haveAudio; + this.uiState.hasRecordableDivs = hasRecordableDivs; + this.notifyStateChanged(); + } + private updateDisplay(maySetHighlight = true): void { // It's a bit expensive to do the test for text present, but without it, // Import Recording will be improperly enabled on an empty page. diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts index 7640ad46c9ee..fa6bfb431893 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecordingSpec.ts @@ -1763,6 +1763,183 @@ describe("audio recording tests", () => { }); }); + // BL-16632: The Recording Mode radio buttons are driven by uiState.hasAudio and + // uiState.haveACurrentTextboxModeRecording, which are derived from this.haveAudio. The + // button-state refresh recomputes this.haveAudio from the server whenever the current + // element changes, so anything that refreshes the buttons must also refresh those derived + // flags. Otherwise "By Sentence" keeps saying 'first use the "Clear" button to remove your + // recording' about a box that has no recording (and whose Clear button is disabled). + describe("- recording mode enablement when the current box changes (BL-16632)", () => { + // Only box1 has a recording. + function setupApiResponsesWithRecordingForBox1() { + vi.spyOn(axios, "get").mockImplementation((url: string) => { + if (url.includes(kAnyRecordingApiUrl)) { + return Promise.resolve({ data: url.includes("box1") }); + } else { + return Promise.reject("Fake 404 error 16632."); + } + }); + } + + // A page where box1 has been recorded By Whole Text Box and box2 has text the user + // just typed but has never recorded. Both are marked up for TextBox recording, which + // is what the tool does to a newly typed box while that mode is selected. + function setupPageWithRecordedBox1AndUnrecordedBox2() { + const box1 = + '

This box has been recorded.

'; + const box2 = + '

This box has never been recorded.

'; + SetupIFrameFromHtml( + `
${box1}
${box2}
`, + ); + } + + it("stops claiming a whole-text-box recording after the highlight moves to a box with no recording", async () => { + setupApiResponsesWithRecordingForBox1(); + setupPageWithRecordedBox1AndUnrecordedBox2(); + + const recording = new AudioRecording(); + (recording as unknown as { isShowing: boolean }).isShowing = true; + recording.recordingMode = RecordingMode.TextBox; + + // Arrive on the page with box1 (which has a whole-text-box recording) current. + await recording.handleNewPageReady(); + + // Sanity check the starting state: with box1 current we really do have a + // whole-text-box recording, so "By Sentence" is correctly disabled here. + expect( + recording.uiState.buttons.clear, + "Setup problem: Clear should be enabled for the recorded box", + ).toBe(Status.Enabled); + expect(recording.uiState.hasAudio).toBe(true); + expect(recording.uiState.haveACurrentTextboxModeRecording).toBe( + true, + ); + + // The user clicks in box2, which has text but no recording. + const moveRecordingHighlightToElement = ( + recording as unknown as { + moveRecordingHighlightToElement( + target: HTMLElement, + ): Promise; + } + ).moveRecordingHighlightToElement.bind(recording); + await moveRecordingHighlightToElement( + getFrameElementById("page", "box2")!, + ); + + // Sanity check that the engine did notice box2 has no recording. + expect( + recording.uiState.buttons.clear, + "Setup problem: Clear should be disabled for the unrecorded box", + ).toBe(Status.Disabled); + // ...so the radio buttons must agree: no audio, no whole-text-box recording, + // hence "By Sentence" is available again. + expect(recording.uiState.hasAudio).toBe(false); + expect(recording.uiState.haveACurrentTextboxModeRecording).toBe( + false, + ); + }); + + // The other half of BL-16632: add a page and type into its empty text box. Nothing + // recomputed hasRecordableDivs after the page was found to be empty, so both Recording + // Mode radio buttons stayed disabled even though the box now had text to record. + it("enables the recording modes once text is typed into a page that started out empty", async () => { + setupDefaultApiResponses(); + SetupIFrameFromHtml( + '

', + ); + + const recording = new AudioRecording(); + (recording as unknown as { isShowing: boolean }).isShowing = true; + recording.recordingMode = RecordingMode.TextBox; + + // A newly added page: it has a text box, but no text yet, so there is nothing to record. + await recording.handleNewPageReady(); + expect(recording.uiState.hasRecordableDivs).toBe(false); + + // The user types. The markup update that follows ends by refreshing the button state. + const box1 = getFrameElementById("page", "box1")!; + box1.innerHTML = "

Text the user just typed.

"; + box1.classList.add("audio-sentence"); + ( + recording as unknown as { highlightedElement: HTMLElement } + ).highlightedElement = box1; + + await recording.changeStateAndSetExpectedAsync("record"); + + expect(recording.uiState.hasRecordableDivs).toBe(true); + }); + + // The mirror image: when the last of the text goes away, the tool disables every button + // (the empty expected verb), and the Advanced section's controls have to follow. They used + // to stay enabled, still describing the text that had just been deleted. + it("disables the recording modes once the last of the text is deleted", async () => { + setupDefaultApiResponses(); + SetupIFrameFromHtml( + '

Text that is about to be deleted.

', + ); + + const recording = new AudioRecording(); + (recording as unknown as { isShowing: boolean }).isShowing = true; + recording.recordingMode = RecordingMode.TextBox; + + await recording.handleNewPageReady(); + expect( + recording.uiState.hasRecordableDivs, + "Setup problem: the box starts out with text to record", + ).toBe(true); + + // The user deletes all of it. The markup update that follows finds nothing recordable + // left on the page, which disables the buttons with the empty expected verb. + getFrameElementById("page", "box1")!.innerHTML = "

"; + + await recording.changeStateAndSetExpectedAsync(""); + + expect(recording.uiState.hasRecordableDivs).toBe(false); + }); + + // Clicking something that can't be recorded (a picture, say) deselects everything and + // disables all the buttons. The Advanced section has to stop claiming the recording of the + // box we were on, or we are back to "By Sentence" disabled telling you to press a Clear + // button that is itself disabled. + it("stops claiming a recording when the click lands on something that cannot be recorded", async () => { + setupApiResponsesWithRecordingForBox1(); + const box1 = + '

This box has been recorded.

'; + SetupIFrameFromHtml( + `
${box1}
`, + ); + + const recording = new AudioRecording(); + (recording as unknown as { isShowing: boolean }).isShowing = true; + recording.recordingMode = RecordingMode.TextBox; + await recording.handleNewPageReady(); + expect( + recording.uiState.hasAudio, + "Setup problem: box1's recording should have been found", + ).toBe(true); + + const moveRecordingHighlightToElement = ( + recording as unknown as { + moveRecordingHighlightToElement( + target: HTMLElement, + ): Promise; + } + ).moveRecordingHighlightToElement.bind(recording); + await moveRecordingHighlightToElement( + getFrameElementById("page", "justAPicture")!, + ); + + // Sanity check: nothing is selected, so every button is disabled. + expect(recording.uiState.buttons.clear).toBe(Status.Disabled); + expect(recording.uiState.hasAudio).toBe(false); + expect(recording.uiState.haveACurrentTextboxModeRecording).toBe( + false, + ); + }); + }); + describe("- initializeAudioRecordingMode()", () => { it("initializeAudioRecordingMode gets mode from current div if available (synchronous) (Text Box)", () => { SetupIFrameFromHtml(