editor-js / editor-js/document-model

BlocksUI doesn't remove its blocksHolder DOM event listeners (or the holder itself) on destroy()

Open
#170 0 comments 0 reactions 0 assignees View on GitHub
good first issue
Dominant language
TypeScript
Stars
12
Forks
3
PR merge metrics
No merged PRs in 30d

Description

`BlocksUI.#prepareBlocksHolder()` ([packages/ui/src/Blocks/Blocks.ts](https://github.com/editor-js/document-model/blob/main/packages/ui/src/Blocks/Blocks.ts)) registers three DOM listeners directly on `blocksHolder`:

- `beforeinput` (line 89)
- `keydown` (line 118)
- `copy` (line 140)

`BlocksUI.destroy()` only cleans up rendered block elements:

```ts
public destroy(): void {
this.#blocks.forEach(block => block.remove());
this.#blocks = [];
}
```

None of the three listeners above are ever removed, and `#blocksHolder` itself is never detached. The holder is handed off via `BlocksHolderRenderedUIEvent` to `EditorUI`, which appends it into `#editorWrapper` (`packages/ui/src/index.ts`) — but `EditorUI.destroy()` is currently a no-op (`// Cleanup if needed`), so nothing ever detaches it either.

## Impact

If a `blocksHolder` element is reused, or a new `BlocksUI`/`Core` instance is created against a holder from a previous instance, the old listeners stay attached and continue firing alongside the new instance's listeners. Concretely this means duplicate `BeforeInputUIEvent`/`CopyUIEvent` dispatches per user action, and stale `keydown` (undo/redo) handling from a destroyed instance.

## Where this came from

Flagged by review on #129, specifically for the new `copy` listener: https://github.com/editor-js/document-model/pull/129#discussion_r3616676234. Investigation showed the same gap already existed for `beforeinput`/`keydown` before this PR — this issue is to track fixing all three (and the missing holder detachment) rather than just the newest one.

## Suggested fix

- Store references to the three listener functions (or use an `AbortController`/`AbortSignal` passed to `addEventListener` for the whole group) so `destroy()` can call `removeEventListener` for each, matching the pattern already used in `ClipboardPlugin`/`ShortcutsPlugin` elsewhere in this codebase.
- Consider detaching `#blocksHolder` from the DOM in `destroy()`, and implementing `EditorUI.destroy()` (currently a no-op) to tear down `#editorWrapper`.
- Add test coverage — there's currently no `Blocks.spec.ts` at all, so `destroy()` behavior is completely unverified.

Contributor guide

Open the contributing guide

Research direction

Read packages/ui/src/Blocks/Blocks.ts and packages/ui/src/index.ts to trace blocksHolder creation, listener registration, handoff, and destruction. Compare teardown patterns in ClipboardPlugin and ShortcutsPlugin, then add a new Blocks.spec.ts covering listener cleanup and holder detachment; done means destroyed instances no longer dispatch duplicate or stale events.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
frontend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
58/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.