Add a way to remove a single event listener (#20983) #25535

ypujante · 2025-10-09T17:06:07Z

I know it has been quite a while since I opened #20983 but I finally got the time to work on it. I followed the discussion we had at the time and did the following:

added emscripten_remove_callback
- iterate over all handlers and only remove the one matching the arguments provided
changed all event setters to pass userData and eventTypeId which are required by this API
added a test that shows that it works

This first pass is to make sure that this would be an ok change to Emscripten and that the changes work (no failures introduced by the changes). I can obviously change whatever you want or drop it entirely if this is not desired. And can work on documentation/examples if that gets approval.

I believe it is a good addition because it allows to remove a previously set listener, which is not possible at the moment, as the APIs provided will remove all of the ones set (see discussion in the #20983 ticket).

- added emscripten_remove_callback - iterate over all handlers and only remove the one matching the arguments provided - changed all event setters to pass userData and eventTypeId which are required by this API

ypujante · 2025-10-10T14:29:13Z

@sbc100 The only failing test is the one about code size, which I can address if you are satisfied with the direction of this PR.

In the grand scheme of things it is a pretty low risk PR since the only changes to existing code (17 functions in total) is passing an additional 2 fields in the EventHandler map that are not used prior to this PR. Of course it makes the resulting code bigger.

I added a test which does a basic test of the functionality by counting the number of event handlers registered. That being said, during development of such test, I was running it manually in the browser and making sure that the result of removing a handler was actually having an action on the underlying JavaScript event handler being present or not.

Let me know your thoughts.

Thanks

sbc100

The approach seem pretty reasonable.

Do you really need to be able to register and un-register the same function with different user data?

src/lib/libhtml5.js

sbc100 · 2025-10-10T22:06:55Z

@juj what do you think about adding this new API?

juj · 2025-10-23T13:22:19Z

src/lib/libhtml5.js


+  emscripten_remove_callback__proxy: 'sync',
+  emscripten_remove_callback__deps: ['$JSEvents', '$findEventTarget'],
+  emscripten_remove_callback: (target, userData, eventTypeId, callback) => {


Hmm, should we key the userData pointer into the removal? I.e. it would be better for this API to be

emscripten_remove_callback: (target, eventTypeId, callback)

The rationale is that the userData pointer is generally just data to the function call, and not identifying. I.e. it is more of the 'value' part in a key->value association.

It might be some function local value in the caller (e.g. a boolean for some behavior), and might result in user having to store the userData field in some global to be able to unregister. So requiring users to know to match the userData value could be a bit tedious.

(If users did specifically want to differentiate unregistrations based on what was seemingly a different userData value, they could do that by separating to a different function with a trampoline)

Also, this function would be good to follow the same _on_thread model, so that it can be called from pthreads symmetric to how registering on pthreads works.

The semantics of function emscripten_remove_callback are to remove all copies of callback registrations of the given function. I wonder if the function naming should somehow reflect that?

I think this makes sense to me, but in that case I think it would also be good it if we make it impossible to register the same callback function for the same event with different userData.

emscripten_set_click_callback(window, data1, 0, mycallback); // success emscripten_set_click_callback(window, data2, 0, mycallback); // error `mycallback` is already registered as a click callback

If we didn't error in the second case, and allowed the same callback to be registred twice, there would be no way to unregister just one of them.

if we want to error when registering the same callback with a different user data, then it means we need to store the user data (which we don't prior to this changes)

it also means that code that is currently working could fail after this change

I think what @juj suggested was to indeed un-register all callbacks that were registered with different data but with the same target/event_id/callback combination. That would have eliminated having to store the user data.

It sounds like you don't like this because then "there would be no way to unregister just one of them".

I am definitely not in favor of erroring when registering 2 identical callbacks with different user data, because from a developer point of view, this is absolutely not an error and I can imagine scenarios where that could be useful.

So I guess the solution is to leave the API and changes as implemented in this PR. That way we can un-register a single callback (which IS the point of this PR). So I am personally in favor of this.

I will now work on updating the documentation and fixing the "Code Size" failure.

I am definitely not in favor of erroring when registering 2 identical callbacks with different user data, because from a developer point of view, this is absolutely not an error and I can imagine scenarios where that could be useful.

Can you give an example of when this might be useful?

This a theoretical example, because I am not implementing it this way at the moment, but imagine for GLFW, you have 2 windows (represented in C by GLFWwindow pointers and canvases in HTML) and you need to set mouse move callbacks to track the mouse movements.

In order to detect moving the mouse outside the canvas while the button is clicked (something my library supports where others don't) you cannot set the callback on each canvas, it has to be set on the window... so the callbacks would be:

emscripten_set_mousemove_callback(WINDOW, glfwWindow1, true, handle_move_callback); emscripten_set_mousemove_callback(WINDOW, glfwWindow2, true, handle_move_callback);

Again I am not doing it this way, but that could be a way to implement it...

Fair enough. If we do want to officially support that then we probably also want to officially support unregistering them individually (sadly)

juj · 2025-10-23T13:29:14Z

@juj what do you think about adding this new API?

Looks good to me. I would consider removing the userData part from the unregistration, and also this would be good to support operation from pthreads to be symmetric/complete.

Apart from that, looks like a great addition.

ypujante · 2025-10-23T14:00:35Z

@juj Thank you for the feedback

Looks good to me. I would consider removing the userData part from the unregistration,

Personally I do not have a strong opinion either way. What matters is that if my library (ex: glfw) sets a listener, I am able to remove it without removing other listeners set by the client code on the same target. The eventTypeId/callback combination should be enough to achieve this result (although it might make the code a bit more tedious when the caller wants to include userData as a differentiator). It will also simplify a bit the code and make it smaller since we don't need to store userData anymore. So if @sbc100 is fine with it I will remove it.

and also this would be good to support operation from pthreads to be symmetric/complete.

At first I tried to implement support for pthread but it doesn't seem to make sense from my understanding of the code. pthread is only used to specify how to invoke the callback once it has been registered. Registering the callback itself (which is done by calling JSEvents.registerOrRemoveHandler(eventHandler);) has nothing to do with pthread. So I don't see why we need pthread to remove/unregister the callback. Am I missing something?

The semantics of function emscripten_remove_callback are to remove all copies of callback registrations of the given function. I wonder if the function naming should somehow reflect that?

I suppose we could call it emscripten_remove_callbacks instead? Either way, I will also add this function to the documentation once approved to move forward to describe what it does.

From a schedule point of view, I am traveling so I won't be able to work on it for a couple weeks. I don't think there is urgency anyway ;)

ypujante · 2025-11-07T15:14:00Z

I am done with traveling so I can work on this again. And I would like to wrap this up if at all possible.

@sbc100 do you have any opinion on @juj feedback? Do you have an opinion on removing the userData parameter?

@juj Can you provide feedback on my comment about pthread?

Thanks

This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (1) test expectation files were updated by running the tests with `--rebaseline`: ``` codesize/test_codesize_hello_dylink_all.json: 819522 => 819904 [+382 bytes / +0.05%] Average change: +0.05% (+0.05% - +0.05%) ```

ypujante · 2025-11-09T19:19:49Z

I think I am done on my side:

Based on the discussion, I left the code as-is with the userData parameter included. If we exclude it, then we need to disallow registering multiple callbacks with the same userData. It is still doable if you prefer to go down this route, but it means that it will break code relying on the fact that it is allowed now.

I have changed the code
Updated the documentation
Updated the Changelog
Fixed the size (after many attempts...)
All tests are green

Let me know if there is anything else or if this can be merged as-is.

sbc100

Given hat we decided to have N different ways to register each callback it seems like we should probably just a N different un-registering functions too.

e.g. emscripten_set_mousedown_callback should have a corresponding emscripten_remove_mousedown_callback or emscripten_unset_mousdown_callback.

Alternatively we are doing to add a generic form such as emscripten_remove_callback the obvious question is why can't we have a generic emscripten_add_callback?

I guess one issue is that each callback has its own signature, which means the generic version requires the use of void* for the callback argument? So one advantage of using N different function is that we get better type safety.

I guess my preference would probably be to stick with current convention and have N different functions?

If we do go with emscripten_remove_callback maybe we can some up with less generic name .. there nothing in the name that suggests html5 or events.. so in isolation I think maybe it could be better named.

My apologies for opening up an log of last minute questions..

ypujante · 2025-11-10T15:35:32Z

@sbc100 No worries about the back and forth. I suppose that is the point of a PR.

So the "N" you are talking about is 35 in html5.h (and some additional in WebGL).

Honestly, I don't think it is very reasonable to add this kind of bloat to Emscripten to add this lacking feature (or fix a bug depending on how you look at it, since after all there is a way to remove callbacks right now, just that removing them, will remove more than you want, see #20983 for details).

In addition, I feel like the design was way too granular. I understand the need/want to have a type safe API with the proper function pointers, but there could have been 1 "set_callback" API per type (ex: 1 for all mouse events, instead of 9 like there is...). And yes you could totally have a single generic set_callback api, you would just loose the type safety...

For these reasons, I feel like adding an additional 35+ calls just for this is somewhat not the right approach.

Having a single generic call to remove any callback is by far the least intrusive. Yes you loose on type safety, but in this instance it is really unnecessary to have it since the function pointer is never used as a type pointer to invoke, but as a comparison argument.

I agree with you that taken out of context the name emscripten_remove_callback is not a good name. I wished I had realized this sooner. I noticed that there is already a function called emscripten_html5_remove_all_event_listeners so maybe we could name this one emscripten_html5_remove_event_listener instead?

Finally I am wondering if the API should return an error in case the event listener being asked to be removed doesn't exist (right now it doesn't in this instance). That way it could somehow cover cases where the wrong callback/function pointer is being used accidentally (a poor type safety check in some way).

In any case, these are my opinion and observations. You have the final call, and I will be fine with any way you want to go. Even if you want to go down the 35+ way, it will be far more work, but I am fine doing it. There is also the other alternative to simply close this PR and forget about it. There is currently no way to remove a specific callback (which is what is being addressed by this PR), but beside me, it doesn't seem that it has bothered anybody else. So I suppose we could just not do it.

sbc100 · 2025-11-10T15:53:04Z

Agree that adding 35 new API calls also seems like a bad idea :-/

Maybe we should at at least add a new generic API for this? i.e. if we also add a single API for registering callbacks then we could at least say that the old API is deprecated? On the other hand we do have a general policy of avoiding adding API that do too much multi-plexing since they prevent DCE. Perhaps that is good argument for allowing an asymmetry in this case.

ypujante · 2025-11-10T17:37:35Z

I have no opinion on whether to add a generic API or not but I have no issue implementing it. From your last message it looks like the 35+ option is out of the debate.

So what do you want me to do:

Rename emscripten_remove_callback into emscripten_html5_remove_event_listener (or some other name / please specify)
Implement 1) and add a generic API (like emscripten_html5_set_event_listener)
Close this PR
Something else? Please specify

Please let me know which option you want me to do.

sbc100 · 2025-11-10T17:43:19Z

I'm afraid I don't have an answer for you now. I think some more thought/discussion needs to go into this. Lets involve @juj and @kripken and see what they think.

Am I right in thinking that you can use this downstream today (as an out-of-tree JS library function) until we resolve it here?

ypujante · 2025-11-10T17:52:58Z

Am I right in thinking that you can use this downstream today (as an out-of-tree JS library function) until we resolve it here?

Not really no, because it requires changing the code of 17+ functions to pass the information that is currently not stored and needed by emscripten_html5_remove_event_listener. So I cannot just add emscripten_html5_remove_event_listener as an out-of-tree JS library function.

But waiting and not having an answer right now until we get it right is fine. I am not blocked. This has been like this for my library since I released it: if a user register/unregister event(s) using any of these functions, it will break the functioning of the library.

Also note that adding this function will still not fix the issue that my library can call the new emscripten_html5_remove_event_listener (and do the right thing) while the user code calls emscripten_set_resize_callback(EMSCRIPTEN_EVENT_TARGET_WINDOW, window, false, nullptr); to remove their listeners that would impact the ones set by my library because they would be removed as well (unless we deprecate and later forbid passing nullptr in these functions...)

In any case, I am good in waiting.

juj · 2025-11-13T17:11:17Z

site/source/docs/api_reference/html5.h.rst

+Remove callback function
+------------------------
+
+In order to remove a callback, previously set via a ``emscripten_set_some_callback`` call, there is a dedicated and generic function for this purpose:


Maybe slightly simpler phrasing:

In order to unregister a single event handler callback, call the following function:

site/source/docs/api_reference/html5.h.rst

juj · 2025-11-13T17:15:03Z

Maybe we should at at least add a new generic API for this? i.e. if we also add a single API for registering callbacks then we could at least say that the old API is deprecated?

I think it would be good to avoid trying to eat too big pie here at once. @ypujante's use case is looking to be able to remove an individual callback, so it would be good to not snowball this into needing to redesign the whole API at once.

The name emscripten_html5_remove_event_listener() sounds good to me.

and also this would be good to support operation from pthreads to be symmetric/complete.

At first I tried to implement support for pthread but it doesn't seem to make sense from my understanding of the code. pthread is only used to specify how to invoke the callback once it has been registered. Registering the callback itself (which is done by calling JSEvents.registerOrRemoveHandler(eventHandler);) has nothing to do with pthread. So I don't see why we need pthread to remove/unregister the callback. Am I missing something?

Thanks for the note. I realize now that there is already a emscripten_remove_callback__proxy: 'sync' directive on the removal function handler, and that is indeed enough to make unregistering work from a pthread. Doesn't look like anything else is needed.

You had a good point at https://github.com/emscripten-core/emscripten/pull/25535/files#r2505004777 about why to keep the userData as part of the key to removing a callback. So it's a good idea to keep that.

So I would recommend:

rename the function name to emscripten_html5_remove_event_listener(),
keep userData as part of the key. Add a comment to the function docs to highlight this fact

That would work for me to land.

If we wanted to have a modern revision of the HTML5 event handler APIs that would have fewer registration functions, that can be a completely separate story from this one.

ypujante · 2025-11-13T18:06:47Z

Thank you @juj for the review and comments. I am personally in full agreement with the approach. @sbc100 what do you think?

The only think I wanted to call out is the fact that this PR does not address the fact that a user can still call (for example) emscripten_set_resize_callback with nullptr for the function pointer to remove it. But as we know (that is the point of this PR), that it is wrong because it would remove all callbacks using the same target. I believe the solution is to not allow this anymore and instead use the new emscripten_html5_remove_event_listener function.

Obviously we could start by adding a deprecation warning when this scenario is detected. I could add this deprecation warning to this PR or open another one once this one is merged. Any preference?

sbc100 · 2025-11-13T18:09:35Z

Thank you @juj for the review and comments. I am personally in full agreement with the approach. @sbc100 what do you think?

Fair enough.

The only think I wanted to call out is the fact that this PR does not address the fact that a user can still call (for example) emscripten_set_resize_callback with nullptr for the function pointer to remove it. But as we know (that is the point of this PR), that it is wrong because it would remove all callbacks using the same target. I believe the solution is to not allow this anymore and instead use the new emscripten_html5_remove_event_listener function.

Obviously we could start by adding a deprecation warning when this scenario is detected. I could add this deprecation warning to this PR or open another one once this one is merged. Any preference?

Lets consider that (breaking) change separately, as a potentially followup.

# Conflicts: # test/codesize/test_codesize_hello_dylink_all.json

This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (30) test expectation files were updated by running the tests with `--rebaseline`: ``` codesize/test_codesize_cxx_ctors1.json: 149126 => 149173 [+47 bytes / +0.03%] codesize/test_codesize_cxx_ctors2.json: 148530 => 148577 [+47 bytes / +0.03%] codesize/test_codesize_cxx_except.json: 194589 => 194636 [+47 bytes / +0.02%] codesize/test_codesize_cxx_except_wasm.json: 164106 => 164153 [+47 bytes / +0.03%] codesize/test_codesize_cxx_except_wasm_legacy.json: 161764 => 161811 [+47 bytes / +0.03%] codesize/test_codesize_cxx_lto.json: 125452 => 125448 [-4 bytes / -0.00%] codesize/test_codesize_cxx_mangle.json: 258665 => 258718 [+53 bytes / +0.02%] codesize/test_codesize_cxx_noexcept.json: 151543 => 151590 [+47 bytes / +0.03%] codesize/test_codesize_cxx_wasmfs.json: 176720 => 176912 [+192 bytes / +0.11%] codesize/test_codesize_files_wasmfs.json: 55679 => 55818 [+139 bytes / +0.25%] codesize/test_codesize_hello_O0.json: 39336 => 39311 [-25 bytes / -0.06%] codesize/test_codesize_hello_dylink.json: 44313 => 44309 [-4 bytes / -0.01%] codesize/test_codesize_hello_dylink_all.json: 819266 => 819781 [+515 bytes / +0.06%] codesize/test_codesize_mem_O3.json: 9612 => 9610 [-2 bytes / -0.02%] codesize/test_codesize_mem_O3_grow.json: 9898 => 9896 [-2 bytes / -0.02%] codesize/test_codesize_mem_O3_grow_standalone.json: 9651 => 9649 [-2 bytes / -0.02%] codesize/test_codesize_mem_O3_standalone.json: 9509 => 9507 [-2 bytes / -0.02%] codesize/test_codesize_mem_O3_standalone_lib.json: 8804 => 8802 [-2 bytes / -0.02%] codesize/test_codesize_mem_O3_standalone_narg.json: 8837 => 8835 [-2 bytes / -0.02%] codesize/test_codesize_mem_O3_standalone_narg_flto.json: 7649 => 7647 [-2 bytes / -0.03%] codesize/test_codesize_minimal_pthreads.json: 27214 => 27208 [-6 bytes / -0.02%] codesize/test_codesize_minimal_pthreads_memgrowth.json: 27642 => 27636 [-6 bytes / -0.02%] codesize/test_minimal_runtime_code_size_hello_webgl2_wasm.json: 13199 => 13204 [+5 bytes / +0.04%] codesize/test_minimal_runtime_code_size_hello_webgl2_wasm2js.json: 18543 => 18554 [+11 bytes / +0.06%] codesize/test_minimal_runtime_code_size_hello_webgl2_wasm_singlefile.json: 15135 => 15147 [+12 bytes / +0.08%] codesize/test_minimal_runtime_code_size_hello_webgl_wasm.json: 12737 => 12742 [+5 bytes / +0.04%] codesize/test_minimal_runtime_code_size_hello_webgl_wasm2js.json: 18070 => 18081 [+11 bytes / +0.06%] codesize/test_minimal_runtime_code_size_random_printf_wasm.json: 10982 => 10993 [+11 bytes / +0.10%] codesize/test_minimal_runtime_code_size_random_printf_wasm2js.json: 17225 => 17229 [+4 bytes / +0.02%] codesize/test_unoptimized_code_size.json: 180978 => 180954 [-24 bytes / -0.01%] Average change: +0.02% (-0.06% - +0.25%) ```

# Conflicts: # test/codesize/test_codesize_hello_dylink.json # test/codesize/test_codesize_hello_dylink_all.json

ypujante · 2025-11-14T15:12:59Z

@sbc100 I have implemented the changes discussed.

I have reverted the code size changes because it was polluting this PR with 30+ file changes and the codesize checks were still failing (a bit of a mystery to me). I can see that the failing codesize checks states that it is a warning that can be ignored, so I have chosen to ignore it...

There is only one test failing during package installation as I reported earlier and it is clearly not related to these changes.

The relevant test is passing:

# from test-browser-firefox
  test_html5_remove_event_listener (test_browser.browser) ... ok (0.000s)

Let me know if there is anything else you want me to do for this PR

ypujante added 2 commits October 9, 2025 09:59

First pass at implementing emscripten-core#20983

adeb50b

- added emscripten_remove_callback - iterate over all handlers and only remove the one matching the arguments provided - changed all event setters to pass userData and eventTypeId which are required by this API

Added missing signature

4260b0b

ypujante changed the title ~~First pass at implementing #20983 (Add a way to remove a single event listener)~~ Add a way to remove a single event listener (#20983) Oct 9, 2025

ypujante added 2 commits October 10, 2025 05:29

fixes test failure

6925d65

Merge remote-tracking branch 'origin/main' into issue-20983

d489b58

sbc100 reviewed Oct 10, 2025

View reviewed changes

src/lib/libhtml5.js Show resolved Hide resolved

Merge branch 'main' into issue-20983

9fc8bd1

juj reviewed Oct 23, 2025

View reviewed changes

Merge branch 'main' into issue-20983

55d37cf

ypujante and others added 5 commits November 8, 2025 11:03

Merge branch 'emscripten-core:main' into issue-20983

b7b7240

Added emscripten_remove_callback to doc

6583129

Updated Changelog

cdf9b94

Merge remote-tracking branch 'origin/main' into issue-20983

8578fb2

ypujante force-pushed the issue-20983 branch from edbd750 to 581444b Compare November 9, 2025 17:51

sbc100 reviewed Nov 9, 2025

View reviewed changes

juj reviewed Nov 13, 2025

View reviewed changes

site/source/docs/api_reference/html5.h.rst Show resolved Hide resolved

ypujante added 6 commits November 13, 2025 10:27

Code review

42410ab

Merge remote-tracking branch 'origin/main' into issue-20983

5aea670

# Conflicts: # test/codesize/test_codesize_hello_dylink_all.json

fixed order

f7bd5d5

Merge remote-tracking branch 'origin/main' into issue-20983

b89401e

# Conflicts: # test/codesize/test_codesize_hello_dylink.json # test/codesize/test_codesize_hello_dylink_all.json

aligning codesize with "main"

1757110

ypujante mentioned this pull request Nov 14, 2025

Fix browser test runner on macOS Safari. #25799

Merged

Add a way to remove a single event listener (#20983) #25535

Are you sure you want to change the base?

Add a way to remove a single event listener (#20983) #25535

Conversation

ypujante commented Oct 9, 2025

Uh oh!

ypujante commented Oct 10, 2025

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbc100 commented Oct 10, 2025

Uh oh!

juj Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

sbc100 Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

ypujante Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

sbc100 Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

ypujante Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

sbc100 Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

juj commented Oct 23, 2025

Uh oh!

ypujante commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ypujante commented Nov 7, 2025

Uh oh!

ypujante commented Nov 9, 2025

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

ypujante commented Nov 10, 2025

Uh oh!

sbc100 commented Nov 10, 2025

Uh oh!

ypujante commented Nov 10, 2025

Uh oh!

sbc100 commented Nov 10, 2025

Uh oh!

ypujante commented Nov 10, 2025

Uh oh!

juj Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

juj commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ypujante commented Nov 13, 2025

Uh oh!

sbc100 commented Nov 13, 2025

Uh oh!

ypujante commented Nov 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ypujante commented Oct 23, 2025 •

edited

Loading

juj commented Nov 13, 2025 •

edited

Loading