Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing event listeners through an identifier #208

Closed
annevk opened this issue Apr 8, 2016 · 68 comments
Closed

Removing event listeners through an identifier #208

annevk opened this issue Apr 8, 2016 · 68 comments
Assignees
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: events

Comments

@annevk
Copy link
Member

annevk commented Apr 8, 2016

Reportedly when writing components it would be convenient to be able to remove a whole swatch of event listeners through a common identifier, rather than having to enumerate all the specific types and callbacks.

I have discussed this a long time ago with @wycats I believe and more recently with @RByers. Now that addEventListener() supports a dictionary we could make this a reality. E.g.,

obj.addEventListener("click", ..., { class: "clicker" })
obj.addEventListener("focus", ..., { class: "clicker" })
obj.removeEventListener({class: "clicker" })

An alternative to overloading obj.removeEventListener() would be adding something like obj.removeEventListenerClass("clicker"), but that seems less extensible.

@dtapuska
Copy link

dtapuska commented Apr 8, 2016

Couldn't addEventListener just return a handle object? That then you would call removeEventListener on in the future?

@annevk
Copy link
Member Author

annevk commented Apr 8, 2016

@dtapuska it's been a while, but I think the concern with that is having to manage the objects, similar to how the current setup requires holding references to the callback. Having a group feature (suggestion from Twitter as a better name for class) makes this kind of management much simpler. A single widget/component could register dozens of event listeners, all using the widget's name. And then when the widget is removed it simply removes all the event listeners with it, without having to keep track of objects and such.

@justsml
Copy link

justsml commented Apr 8, 2016

I just think it's a little funny we (almost) have WebRTC, Asm.js, bg workers, sockets, etc... Yet DOM events slipped by, left to leak all over the place for years.

@jsen-
Copy link

jsen- commented Apr 8, 2016

please don't introduce more magic strings into the API.
I believe it would be much better to use Symbols or objects stored internally in a WeakMap than strings.

@domenic
Copy link
Member

domenic commented Apr 8, 2016

Allowing any value as the key (including a string) seems fine. You just later test with ===. Then people who want to use symbols or objects or whatever can do that, but most of us can just use strings like we're used to doing with jQuery.

@stuartpb
Copy link

stuartpb commented Apr 8, 2016

Now that addEventListener() supports a dictionary

What's this now? Are we talking about the options object being added for passive event listeners?

@RByers
Copy link
Contributor

RByers commented Apr 9, 2016

What's this now? Are we talking about the options object being added for passive event listeners?

Yes, this.

@annevk
Copy link
Member Author

annevk commented Apr 9, 2016

@jsen- could you explain how the string would be magic? It's developer-provided.

@jsen-
Copy link

jsen- commented Apr 9, 2016

@annevk you're right, there's nothing magic about them, I just don't like using strings as identifiers.
I'd prefer something like obj.addEventListener(Obj.events.CLICK, ..., { class: Obj.eventClasses.CLICKER }), but that's for a different typed/untyped discussion.
I'm completely fine with @domenic's explanation

@adamdbradley
Copy link

Could addEventListener work similar to how setTimeout and clearTimeout works? For example, with setTimeout, setInterval, requestAnimationFrame, it's easier for apps to keep track of an id rather than a function which needs be called later on. For example:

var id = obj.addEventListener('click', ...);
document.clearEventListener(id);

Using an id rather than holding onto references to the handler could also reduce the potential for memory leaks.

What would be great is if the returned id could be global and not specific to the object the handler was added to. I used document.clearEventListener just for an example. Additionally, whatever removes the listener could accept one id, or an array of ids.

And to be clear, I also think @annevk's proposal is great, but why not both?

@RByers
Copy link
Contributor

RByers commented Apr 12, 2016

@adamdbradley @dtapuska asked that above and @annevk replied that he thought the group feature was strictly superior to returning a single handle. Any feedback on the differences between those two approaches to the problem?

I agree with Anne that using some group identifier (possibly an object, not just a string) is probably easier to manager than having to track an id. If you're going to track an ID anyway, you could just be tracking the callback in the same way. In fact, addEventListener could just return an object that contains the type, handler and capture values to be passed to removeEventListener later...

@adamdbradley
Copy link

@RByers Being able to group many event listeners through one common identifier sounds great, and I see the value of it because you're not tracking anything at all.

using some group identifier (possibly an object, not just a string) is probably easier

So to remove a group, would it have to strictly be the same object that was used in the addEventListener, or is it any object but with the same string value for class?

@annevk
Copy link
Member Author

annevk commented Apr 13, 2016

@adamdbradley the idea would be obj.addEventListener(type, callback, { group: anything }) and something like obj.removeEventListener({ group: anything }) to get rid of it. anything can be any ECMAScript value, including objects. We'd simply use === on the value passed to see whether it's the same.

@annevk
Copy link
Member Author

annevk commented Apr 15, 2016

A suggestion from @adamdbradley that I did not explicitly address was making the identifier global. I would prefer not doing that as that means we need to globally keep track of these listeners, whereas currently they're always neatly scoped to an object.

The only remaining question here in my mind is whether we should overload removeEventListener() as I described above or add a new method, deleteEventListener() (JavaScript's Set uses add/delete). The latter would make feature testing a little easier and align DOM a bit more with JavaScript's naming scheme.

@markcellus
Copy link

@annevk I like the idea of conforming to the the "add/delete" convention, however if we're overloading addEventListener, wouldn't it be good to do the same to the removal of it too?

@annevk
Copy link
Member Author

annevk commented May 9, 2016

@mkay581 the main difference is that with removeEventListener() the three-argument signature no longer makes sense with this feature. Since the event type and the callback are already taken care of by the group identifier. So the question then is whether you go for the awkward argument-meaning depends on the number of arguments, or a new method that does not have that problem.

@markcellus
Copy link

markcellus commented May 9, 2016

@annevk Yeah, I vote for the latter.

@caub
Copy link

caub commented Aug 11, 2016

Sorry to disagree, I find it superfluous, and not great to put such a 'tag'/group key in listeners options

if the removeEventListener repetitions annoy you, you could do for ex:

var events=[ ['click', clickFn], ['focus', focusFn] ];
events.forEach( e=>obj.addEventListener(...e) )
events.forEach( e=>obj.removeEventListener(...e) )

@annevk annevk added the needs implementer interest Moving the issue forward requires implementers to express interest label Mar 16, 2017
@domenic domenic self-assigned this Jun 23, 2017
domenic added a commit that referenced this issue Jun 23, 2017
domenic added a commit that referenced this issue Jun 23, 2017
@smaug----
Copy link
Collaborator

So is there an API proposal here? Do we know it is such which script libraries would use, or would they still just have some their own API on top of that?
Before adding random new features to add/removeEventListener, I think we should think a bit more what all features we expect would be commonly used and would improve the platform.
Just adding more and more features makes the platform more complicated, so I'd prefer we had better reasoning for new stuff.
Also, making APis more complicated makes implementations slower - perhaps not much per new feature, but over time when more features are added...

@cdumez
Copy link

cdumez commented Jun 25, 2017

@rniwa thoughts?

I am personally unconvinced at this point. Seems like something the JS libraries could easily implement so I am not sure its is worth complicating the engine. That said, I don't think this would be a lot of work to implement.

@domenic
Copy link
Member

domenic commented Jun 25, 2017

Hi all. A bit surprised at some of these questions, @smaug----.

So is there an API proposal here?

Yes. It's the one discussed at length in this thread, and concretized by PR #469. I noted you commented over there, so I'm a bit confused why you state here you're unable to find the API proposal.

Do we know it is such which script libraries would use, or would they still just have some their own API on top of that?

Yes; jQuery is a prime candidate, since that is where this feature originates. As are all the frameworks built on top of it. In general this feature is highly desired by frameworks since they like to be able to manage their event listeners en-masse. Right now they are having to keep a side-table of function references so they can remove them later. This is especially the case for custom elements, which are like little self-contained frameworks themselves.

See also #412; although this proposal doesn't subsume that one, it provides a less-dangerous subset that addresses many of the same use cases (e.g. allowing coordination via disparate parts of the code/different libraries as long as they agree on the same group string).

Before adding random new features to add/removeEventListener, I think we should think a bit more what all features we expect would be commonly used and would improve the platform.

Indeed. This is part of that effort, going back years; there's actually been a lot of thought about this, which you can see from the various discussions here and, back in the day, on www-dom. (It's not "random".) Removing event listeneres by group is a top candidate, as is event delegation (#215), based on patterns that a majority of popular libraries end up inventing independently. The once option is another example, which we've successfully added, to great applause from web developers.

Also, making APis more complicated makes implementations slower - perhaps not much per new feature, but over time when more features are added...

I certainly understand that everything has a cost. I'm a bit surprised you think I need to be reminded of that. Furthermore, as I explain a bit more below, I think this will overall increase the speed of web pages.

@cdumez

Seems like something the JS libraries could easily implement so I am not sure its is worth complicating the engine. That said, I don't think this would be a lot of work to implement.

Yes, this is definitely not a new primitive. This is part of a different effort, to make the platform more ergonomic based on the feedback we've gotten over the years; we've provided the primitives, seen what people have built, and now it's time to bring that knowledge back into the platform.

It's more along the lines of the existing once option, or the append()/prepend()/remove() operations. These all make the "raw DOM" API more usable, so people stop having to download large libraries to wrap it. (In our experience, these extra libraries cause much more slowness than the cost of a bit of extra processing in built-in APIs.)

The other point is that this particular proposal fits well in the effort/reward tradeoff, as you point out. Personally I think this and #467 are a slam dunk in that regard. #215 is less clear; it'll likely be more effort to implement, but also higher reward. We'll be able to evaluate the actual tradeoff there better once we have a spec, and possibly a prototype in Blink.

@smaug----
Copy link
Collaborator

Yes. It's the one discussed at length in this thread, and concretized by PR #469. I noted you commented over there, so I'm a bit confused why you state here you're unable to find the API proposal.

PRs should be for actual changes for the spec once the proposal is ready. I wasn't going to do any review there. It makes tracking all the discussion hard if some discussion happens in issue and some in pr. PR should be just once the proposal is concrete and just minor editorial reviews are needed.

Yes; jQuery is a prime candidate, since that is where this feature originates. As are all the frameworks built on top of it. In general this feature is highly desired by frameworks since they like to be able to manage their event listeners en-masse.

What about other script libraries than jQuery? React.js happens to be hip and cool currently, would it use this stuff? What about other libraries?

I certainly understand that everything has a cost. I'm a bit surprised you think I need to be reminded of that.

We all need to be reminded about that all the time. Or at least I need to remind myself.

(Sorry if I'm a bit reluctant right now. I just happened to review implementation for .labels attribute recently and realized how bad API even such a small feature can be - if implemented correctly, it slows down pretty much all DOM mutations)

@LeaVerou
Copy link

Is capture part of the key? If so, the browser already has to normalize, to prevent these from being added twice:

el.addEventListener("type", cb, true);
el.addEventListener("type", cb, { capture: true });

@annevk
Copy link
Member Author

annevk commented Jun 28, 2017

I think it is, yes. I just meant that we might not want to extend the key further to keep things reasonably straightforward.

domenic added a commit that referenced this issue Jun 28, 2017
@domenic
Copy link
Member

domenic commented Jun 28, 2017

I've updated #469 to allow only strings and symbols, and to allow a sequence of groups (although I kept the name group; instead of groups; opinions welcome?). I am OK with the added complexity of the sequence if we think it's OK to not include it in the key. (So, you can add the same listener twice if you use different groups each time.)

Next up is updating the tests. Still hoping for some implementer interest beyond Chrome; thoughts @rniwa or @smaug----? :)

@Krinkle
Copy link
Member

Krinkle commented Jun 28, 2017

@annevk Hm.. once and passive are not included in the key right now, either. So doing the same for group would be consistent in that regard.

On the other hand, both of these options are relatively new, and both are the only two options in existence right now that don't modify behaviour of the handler itself (capture and type are unquestionably a different handler, to the point they could make sense as separate methods, like addClickHandler or addCaptureHandler).

I wonder if in the previous specification of once and/or passive there was any consideration with regards to the uniqueness/de-duplication key? If it wasn't explicitly discussed, I don't think we can consider absence of these two options in the key as predicate to keep doing the same going forward. It may've been just left untouched unintentionally, on the assumption that re-use of the same function object is rare anyway. If we have that conversation now, it could very well lead to a change in the spec to consider these two options as part of the key as well.

I would expect that to be a low risk change, in terms of web-compat.

It's only now with grouping that we observe potential user confusion and use cases for one way or the other. Although I see now what @LeaVerou meant. If we take away support for 1-to-many association (multiple groups), this problem/question won't exist. Users would naturally hit the silent ignore of addEventListener when adding a second version, as they do now.

el.addEventListener("type", cb, { once: true });
el.addEventListener("type", cb); // ignored

@annevk
Copy link
Member Author

annevk commented Jun 29, 2017

I can't find a discussion right now though I do remember we had some. The main question is whether there's a use for reusing a callback on the same event target object outside capture/non-capture. I don't think there's a big use for that and therefore I'd prefer keeping the key simple.

@koba04
Copy link

koba04 commented Jul 4, 2017

I like this proposal.

I also don't think it's a good idea to allow removing multiple groups using removeEventListener, because the semantics as you've described them are so different from those of addEventListener. addEventListener(type, cb, { groups: ["foo", "bar"] }) adds something with group "foo" and group "bar". Wheras removeEventListener({ groups: ["foo", "bar"] }) would, if I understand correctly, remove events with either "foo" or "bar". So I think if we allow passing multiple groups to removeEventListener, it should use the same semantics as addEventListener, and remove events with both "foo" and "bar". But maybe that's too confusing, so we should only support passing a single group value.

I think it is confusing that there is a difference between addEventListener and removeEventListener of a behavior of passing an iterable value to group.
addEventListener treats it as an iterable, but removeEventListener treats as a stringified value in spite of the same interface.

https://github.com/w3c/web-platform-tests/pull/6331/files#diff-5c1acdfc59932a9e59f60a2c246f1a80R157

I think it would be nice if removeEventListener treats it as an iterable or throws an error when a value that is not string or symbol.

@rniwa
Copy link
Collaborator

rniwa commented Jul 4, 2017

It appears to me that supporting multiple groups to be associated with seems like an unnecessary complexity. It also feels like something that belong in a framework / library. Why don't we just forego that aspect. If it turns out be useful, we can always add it later.

@mgol
Copy link

mgol commented Jul 4, 2017

If it's decided it's better to add it later, it'd be good if there was no stringification performed on the group value as then adding an optional array value may not be Web compatible. It should just throw if array is provided as @koba04 suggested.

@domenic
Copy link
Member

domenic commented Jul 4, 2017

I'm not really comfortable departing from every other web API that accepts a string by throwing if your string comes via conversion from an iterable (or other value).

@rniwa I thought it added too much complexity too, but after writing the spec I think it ended up being not so bad. Take a look at the PR (you can use the preview link to see a rendered version) and let me know whether you still believe it should be removed?

@rniwa
Copy link
Collaborator

rniwa commented Jul 4, 2017

I don't think it matters where the spec'ing it simple or that you can implement it easily. It's more of a matter of the API complexity. Although I can see the argument for supporting multiple values given we do support DOMTokenList for class names on an element. The use case here is very similar to that.

@LeaVerou
Copy link

LeaVerou commented Jul 5, 2017

It's more of a matter of the API complexity.

If the complexity doesn't get in the way of simple usage, I'm not sure what's the problem exactly. Users don't have to learn about sequences in group if they don't need them, and if they do need them, it's better if they exist.

@rniwa
Copy link
Collaborator

rniwa commented Jul 5, 2017

Keeping core DOM API simple is quite important. Every new complexity we add impose a more cognitive stress on someone trying to learn how to code in the Web.

Are you sure having the sequence support would be useful in 10 years? How about in 20 years? Any new API surface we're adding today should be looked at in that kind of timespan, not just next 2-3 years.

@LeaVerou
Copy link

LeaVerou commented Jul 5, 2017

Every new complexity we add impose a more cognitive stress on someone trying to learn how to code in the Web.

Again, if they don't need it, it doesn’t get in their way. They can just use strings and symbols until the end of time.

Understanding someone else’s code could be an issue, but for a common pattern like this, it's better to standardize one natural way of doing it than to have a number of different helpers and patterns that novices have to process. It's so common in a method signature to accept either a singular item or an array/iterable for multiple that many users might even guess it (and be surprised if it doesn’t work).

Anyhow, if we really want to find out whether that is useful and how much, we should probably ask the jQuery maintainers instead of just guessing based on our experience (or lack thereof). jQuery has allowed multiple namespaces for its events for years, so I suspect they may have some idea of usage stats. @addyosmani, any thoughts?

(notice how the use case for multiple namespaces in their docs is the same I mentioned earlier in this thread: one for plugin name, one for purpose)

Are you sure having the sequence support would be useful in 10 years? How about in 20 years? Any new API surface we're adding today should be looked at in that kind of timespan, not just next 2-3 years.

ZOMG really, that’s how standards work?!? I would never have guessed!!!11 What do I know about standards after all? 🙄

I don't see why if multiple groups are useful today they wouldn't be in 10 or 20 years. It's not a transitional feature of any sort. The same reasons are still going to exist. Do you have any reasons to think that is not the case? Given that you don't seem to understand why multiple groups are needed today, it seems odd that you would see those (nonexistent from your perspective) reasons going away in 10 or 20 years.

@domenic Just wondering, would the group info be available on the event object? Should it? Maybe something to think about.

@annevk
Copy link
Member Author

annevk commented Jul 5, 2017

How would you make it available on the event? It's tied to the event listener, no?

@domenic
Copy link
Member

domenic commented Jul 5, 2017

Please remember to be kind and curteous in discussions. There's no need to be sarcastic or rude. https://whatwg.org/code-of-conduct

@Krinkle
Copy link
Member

Krinkle commented Jul 5, 2017

@annevk Yeah, it's tied to the event listener and cannot (should not) be exposed to the event object. However, I do see @LeaVerou's perspective and had to think about this a few times.

The event object is constructed, like any other object, at the time the event "happens". E.g. you can manually do new Event('click') and call el.fireEvent; or the browser does it internally when processing user input). This must happen separately from event listeners, because 1) the code creating the event object can't know the handlers, and 2) multiple handlers may exist for the same event – and the same event object is passed to each of the handlers. Think about the event object for DOMContentLoaded, or window.onload as extreme example. The parameter tends to be unused there, but it illustrates the issue.

@dmethvin
Copy link

From a jQuery-experience point of view this seems like a good feature for what I've seen to be the most common use of namespaces, which is removing a group of event listeners for a plugin. Although there's a lot of painful logic to deal with multiple namespaces, I doubt it's used that often. We don't have any way to collect stats on that so I could be wrong.

The PR says that the group can be either a Symbol, string, or array of those. That should map fine to the multiple-namespace-string approach used by jQuery. A jQuery namespace of .a.b.c would equate to addEventListener as group: [ "a", "b", "c" ] .

However, note that jQuery does more with namespaces than just adding or removing listeners. It exposes event.namespace as a string; the example above would get ".a.b.c" for a value. That means the event listener can change its behavior based on namespace. Programmatic event triggering allow namespaces so .trigger("click.a") calls a jQuery listener attached with "click.a.b.c" but .trigger("click.c.d") would not.

I doubt jQuery could use this new event interface unless we deprecated and removed the existing namespace features, they are used in the wild. However, I don't think jQuery's ability to adopt the interface in its own codebase should be the litmus test for new features in the standard. It would be good to check with other frameworks and libraries to find out whether they could take advantage of this feature. They may have similar reasons why it doesn't quite match their needs.

The handleEvent pattern is the solution to most problems developers have, they just don't know/use this pattern because jQuery core at John time probably didn't use or know about such pattern so it didn't gain much attention.

I'm pretty sure most of the jQuery team was aware of this pattern. However, addEventListener() wasn't supported by IE 6/7/8 so it was even in play until those browsers died off. At that point, jQuery had a whole set of infrastructure in place to deal with various browser inadequacies and external code now depends on that behavior. (One example, browser devtools grope into jQuery event internals so they can see the user's actual event listener instead of jQuery's event multiplexer; we can't unexpose them now.) Rewriting a fundamental part of jQuery without breaking the ecosystem is nearly impossible.

@freshp86
Copy link

freshp86 commented Feb 7, 2018

This thread is fairly long and has not been updated since July. Can someone summarize the status of this proposal, to facilitate any further steps?

Per my understanding (although admittedly gave up on reading everything):
The discussion seems to have started with a simple idea of returning a handle/ID from addEventListener() and using that with removeEventListener() or a new clearEventListener(). Concerns about the implementation complexity were raised, and also questions on whether IDs should be globally unique or only per event target. Then the desire to add/remove listeners in groups was introduced.

As a side note (not sure if this was mentioned already), I think both the existing removeEventListener() as well as a potential new clearEventListener()/removeEventListenerById() should return a boolean revealing whether such a listener was indeed found and removed. This would help people catch many "silently did not remove any listener" bugs, and it is similar to many existing delete APIs like Map#delete().

Thank you.

@annevk
Copy link
Member Author

annevk commented Feb 16, 2018

There's a proposal in #469, but there's no real implementer interest to pursue this so this is mainly blocked on getting that.

@annevk
Copy link
Member Author

annevk commented Mar 22, 2018

Having read through this thread again it seems there's some interest in this proposal from implementers, but one concern that was raised is that multiple groups might be too much for v1.

Then @dmethvin brought up an interesting point which is that this does not give us feature parity with jQuery as they also expose the group on the event object during any listener callbacks. Additionally, Dave asked about what other libraries are doing here. Exposing the group on the event object seems strictly additive and something we could consider doing later, but figuring out what other event libraries do seems worthwhile to do before we continue. If anyone wants to take that on it'd be appreciated.

@annevk
Copy link
Member Author

annevk commented Mar 28, 2018

(Exposing the group on the event object is basically a subset of #501.)

@benjamingr
Copy link
Member

Related issue: #911

And Domenic's prior work: #469

I'll see if I can try working on the spec bits - I'm probably not skilled enough to make this change yet but it's probably good practice for me even if it doesn't work out.

@annevk
Copy link
Member Author

annevk commented Dec 8, 2020

I suggest we close this now that you can use abort signals as grouping mechanism thanks to @benjamingr's work. Thoughts?

@benjamingr
Copy link
Member

That sounds reasonable to me

@annevk annevk closed this as completed Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: events
Development

Successfully merging a pull request may close this issue.