Skip to content

Fix mouse handler binding for talking book tool (BL-16366)#8000

Open
StephenMcConnel wants to merge 1 commit into
Version6.4from
BL-16366-TalkingBookToolNotWorking
Open

Fix mouse handler binding for talking book tool (BL-16366)#8000
StephenMcConnel wants to merge 1 commit into
Version6.4from
BL-16366-TalkingBookToolNotWorking

Conversation

@StephenMcConnel

@StephenMcConnel StephenMcConnel commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Also a bit of cleanup in that general area as well. I'm not sure these changes are needed in 6.5 since the bug was not manifesting there.


This change is Reviewable

Devin review

@greptile-apps

greptile-apps Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes the talking book toolbar becoming unresponsive after the toolbox re-hydrates its DOM by switching button handlers from direct-element binding to namespaced document-delegated events (.audioRecorder), which survive the innerHTML wipe that destroys the original button nodes. It also addresses previously flagged this-binding bugs by converting audioLevelListener and micErrorListener to arrow-function class fields, and adds a wiredPlayer tracker to re-attach non-bubbling media events when #player is replaced.

  • Delegated handlers are bound in bindDocumentDelegatedHandlers() (called from both initializeTalkingBookToolAsync and setupForRecordingAsync) and cleaned up in unbindDocumentDelegatedHandlers() (called from handleToolHiding), closing the lifecycle correctly.
  • WebSocketManager.removeListener gains a guard for an undefined context entry, preventing a crash on first use after closeSocket has deleted the callback map for that context.

Important Files Changed

Filename Overview
src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts Refactors button event handlers from direct DOM binding to document-delegated namespaced events so they survive toolbox re-hydration; converts audioLevelListener/micErrorListener to arrow-function class fields; adds wiredPlayer tracking to re-attach media events after #player replacement.
src/BloomBrowserUI/utils/WebSocketManager.ts Adds a null-guard in removeListener so it returns early when the clientContext callbacks array has already been deleted by closeSocket, preventing a crash on first addAudioLevelListener/addMicErrorListener call.

Reviews (5): Last reviewed commit: "Fix mouse handler binding for talking bo..." | Re-trigger Greptile

Comment thread src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts Outdated
Comment thread src/BloomBrowserUI/utils/WebSocketManager.ts Outdated
@StephenMcConnel StephenMcConnel force-pushed the BL-16366-TalkingBookToolNotWorking branch 2 times, most recently from dd9237c to 28135c3 Compare June 24, 2026 21:52
@StephenMcConnel StephenMcConnel self-assigned this Jun 24, 2026

@JohnThomson JohnThomson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@JohnThomson reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on StephenMcConnel).


src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts line 250 at r2 (raw file):

            .on("click" + ns, "#audio-next", () =>
                this.moveToNextAudioElement(),
            );

If I'm understanding right, since these are attached to the document, all clicks in any toolbox will activate all of them...then a filter will determine that the event didn't originate with the right element and ignore it. I don't suppose it matters very much, but I wonder if we should try to clean up when TB closes?
Of course then we have to reinstate them when it opens...and that's probably after the new buttons get created, so we could just attach them to the buttons, if we do it then (if I'm understanding the problem).
We could also just merge as-is. But it seemed worth asking.

@StephenMcConnel StephenMcConnel force-pushed the BL-16366-TalkingBookToolNotWorking branch from 28135c3 to 6504c1f Compare July 1, 2026 19:01

@StephenMcConnel StephenMcConnel left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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


src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts line 250 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

If I'm understanding right, since these are attached to the document, all clicks in any toolbox will activate all of them...then a filter will determine that the event didn't originate with the right element and ignore it. I don't suppose it matters very much, but I wonder if we should try to clean up when TB closes?
Of course then we have to reinstate them when it opens...and that's probably after the new buttons get created, so we could just attach them to the buttons, if we do it then (if I'm understanding the problem).
We could also just merge as-is. But it seemed worth asking.

Done.

Also a bit of cleanup in that general area as well.  I'm not sure these
changes are needed in 6.5 since the bug was not manifesting there.
@StephenMcConnel StephenMcConnel force-pushed the BL-16366-TalkingBookToolNotWorking branch from 6504c1f to 3dfffee Compare July 1, 2026 20:45

@JohnThomson JohnThomson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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


src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts line 529 at r3 (raw file):

    public async setupForRecordingAsync(): Promise<void> {
        this.isShowing = true;
        this.bindDocumentDelegatedHandlers();

If this happens every time the tool is opened, then do we still need to call it also in initializeTalkingBookToolAsync()? Given that it has the "off" calls it won't hurt much to do it twice, but better not?
Also, it feels like this method is now doing more than its name says (maybe already was). Time to rename it something like openTool()? There might also be a better way to word the comment about "when the tool is created", which suggests (contrary to the line before) that it only happens once.

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.

3 participants