emscripten-core / emscripten-core/emscripten

`JSEvents.registerOrRemoveHandler`: should the add path dedupe like the remove path does?

Open
#26,851 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
27.6k
Forks
3.6k
Avg merge
1d 1h
Merged PRs (30d)
105

Description

`JSEvents.registerOrRemoveHandler` in `src/lib/libhtml5.js` deduplicates by `(target, eventTypeString)` on the **remove** path but not on the **add** path:

```js
registerOrRemoveHandler(eventHandler) {
...
if (eventHandler.callbackfunc) {
eventHandler.target.addEventListener(...); // always adds
JSEvents.eventHandlers.push(eventHandler); // never dedupes
} else {
for (var i = 0; i < JSEvents.eventHandlers.length; ++i) {
if (JSEvents.eventHandlers[i].target == eventHandler.target
&& JSEvents.eventHandlers[i].eventTypeString == eventHandler.eventTypeString) {
JSEvents._removeHandler(i--); // dedupe-and-replace
}
}
}
...
}
```

So calling e.g. `emscripten_set_keydown_callback(target, ud1, ..., cbA)` followed by `emscripten_set_keydown_callback(target, ud2, ..., cbB)` results in *both* listeners staying attached to the DOM target. Every browser `keydown` then fires `cbA` and `cbB` in order. The user has to know to clear the slot first by passing `NULL` as the callback.

Is this asymmetry intentional?

The current behavior is surprising: a name like `set_*_callback` reads as a setter (replace), not as a "push another listener". And there's no clean way for a caller to register a single handler idempotently without an unrelated `(target, NULL, 0, NULL)` clear first.

---

I just hit this issue in a project using SDL3 as the backend. The PR I sent to SDL3 explains everything more in detail: https://github.com/libsdl-org/SDL/pull/15511

Possible options:

1. **Status quo**: keep the current behavior; maybe document it more prominently.

2. **Dedupe on the add path**: make `registerOrRemoveHandler` remove any existing `(target, eventTypeString)` entry before pushing the new one. Behavioral change for any code that relied on stacking, but I'd guess that code is rare/accidental.

3. **New explicit API**: keep `emscripten_set_*_callback` as add-only, introduce `emscripten_replace_*_callback` (or a flag) for the dedupe-then-add semantics.

Thoughts?

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.