Should the passive option be part of the event listener key

[RByers]

At the moment, the following code installs two listeners (handler will get invoked twice):

  target.addEventListener(type, handler);
  target.addEventListener(type, handler, {passive: true});

@annevk points out this may not make sense - it may be better to treat them as the same.

If we want to change this, we have to define what the second call does. Is it just ignored? Or maybe a listener can be "upgraded" from passive to blocking but not the other way around?

I was really just following the precedent set by capture here - no real argument one way or the other. Can anyone shed light into why capture behaves this way? That seems odd / useless also.

Is there a good reason we'd want capture to have different semantics than passive (and presumably new options)? Maybe capture can just be forever odd for legacy compat reasons?

This is an edge case I think is unlikely to affect any real code, so I think the compat risk of changing this is minimal.

[dtapuska]

Perhaps I am misunderstanding.... It seems to make sense for capture to me...
For capture you can be inside handler and query event.eventPhase; deciding what phase you are in. So if you want to fire a handler in both capturing and bubbling phases you could use the double register approach.

I think because we don't know what will be in the EventListenerOptions and what each UA supports defining individual items as not part of the matching algorithm seems odd to me. It would seem cleaner to me call it with the same args the behavior is this.

For example: Suppose Chrome doesn't support the sendUntrustedEvents option of the dictionary but FireFox does. And they define it in a fashion you are describing here. How does that work in UAs that don't support the field?

[RByers]

So if you want to fire a handler in both capturing and bubbling phases you could use the double register approach.

[Edit - I was smoking something when I wrote my response, sorry]
Yep makes sense.

[annevk]

I don't know why capture behaves the way it does. Maybe @smaug---- does?

[smaug----]

because it is the sane thing to do? event phase is a core piece of whole event dispatch and you may want to use the same listener for many things. Like add capturing and bubble listener to window object. capturing listener could set some state and the bubble listener would be sort of catch call, close to default handling. And depending on the state you may or may not want get called during capturing phase.

Gecko's allowUntrusted is inconsistent here. Has been since the beginning. addEventListener does care about the untrusted flag (you can add same listener twice even for same phase), but removeEventListener explicitly just removes the listener which has matching (type, listener, capture).

But this is tricky. One could easily reuse same listener (or {handleEvent: function() {}}) for many things, both passive and non-passive, and capturing and non-capturing, and add it and remove it using different options. That is not weird code at all, and it should just work.
If we add more stuff to the EventListenerOptions, how should this all work so that it is still easy to write js code which works in various browsers supporting possibly different set of options, not sure.

If we go with the approach where capture is special cased, removeEventListener wouldn't need to take EventListenerOptions, but just boolean. Or in practice it would perhaps just use the capture flag of the dictionary. But then, what should happen if one does
target.addEventListener(type, handler); target.addEventListener(type, handler, {passive: true});
Does the first addEventListener call win or the latter? I'd say in case of passive, the non-passive should have higher priority. But is that the case with other possible options we'll have?
How should once behave.

This is tricky. Need to think this a bit.

[annevk]

So once is only applicable to addEventListener() since you'll never need to use it with removeEventListener(). Therefore in my proposal it's not part of the key. So if you invoke addEventListener() twice with the same arguments except you flip once, the second call will be a no-op. I added once through a subdictionary of EventListenerOptions that addEventListener() takes, but I like your idea of not using EventListenerOptions for removeEventListener() at all. That would simplify things quite a bit.

I think as we add more features we want some kind of identifier approach. Either something similar to setTimeout() where an identifier is returned or as proposed in whatwg/dom#208 one where you explicitly pass an identifier. If we have that we could just require that to be used and decide per feature whether or not it contributes to the key when it comes to checking for duplicates while adding.

[smaug----]

I think as we add more features we want some kind of identifier approach
That is rather not compatible with current browsers though.

[RByers]

This is interesting, thanks guys. I'm kinda shocked we didn't think through the implications of this over the past year of debate on this API ;-)

I support saying that capture is the only special option which influences the key, and as a result changing removeEventListener to not take options at all.

We're actively getting sites to use EventListenerOptions now, so there's some risk we could start to have compat concerns if we don't act quickly. Eg. this demo I wrote recently relies on this property (without options support, it would try and fail to remove a capturing listener).

[smaug----]

We're actively getting sites to use EventListenerOptions now

And that has the risk to break sites on browsers which don't support EventListenerOptions :/

[smaug----]

I'm kinda shocked we didn't think through the implications of this over the past year of debate on this API ;-)

btw, except in some trivial cases, based on my experience it takes at least two separate implementations before some API becomes good enough. Hmm, perhaps we should try to not ship APIs unless there is at least some other implementation (which could be still on nightly/dev builds only).

[RByers]

And that has the risk to break sites on browsers which don't support EventListenerOptions :/

As does using every new API. Let's not rehash the issue #12 centi-thread here, shall we? Here's my first PR, note the feature detection.

[RByers]

Actually, if we're going to add AddEventListenerOptions anyway, then passive could just move there, and removeEventListener could still take EventListenerOptions or boolean. That would be compatible with what we have today and also allow symmetry like this (which could be surprising otherwise):

  var options = shouldCapture;
  if (supportsCaptureOption)
    options = {capture:shouldCapture};
  target.addEventListener(type, handler, options);
  ...
  target.removeEventListener(type, handler, options);

I think that's probably better than saying removeEventListener only supports boolean in the 3rd arg.

[annevk]

That is rather not compatible with current browsers though.

Yes, but it's something that over time will become very useful. Just like node.remove().

@RByers I guess that's fair. Sounds like a reasonable solution.

[domenic]

So I did some testing to see if jQuery could give us some guidance, especially with regard to the proposed once option: https://jsbin.com/riwuru/edit?html,console

It turns out they don't seem to have the notion of key at all; every call to their .on() or .one() registers a new listener, I guess. So, I'm not sure they can be much guidance.

[ojanvafai]

I'm a big fan of having addEventListener return an ID the way setTimeout does. I don't know if that's backwards compatible. Hopefully not much content depends on the return value of addEventListener being undefined.

I also agree that just adding the listener multiple times is the simplest thing to reason about from a developer and platform perspective. There aren't great use cases here, so doing the simplest thing makes the most sense to me.

FWIW, there is a use case for removeEventListener with once, e.g. you register a click handler but decide to remove it before the click has happened.

[annevk]

@ojanvafai in whatwg/dom#208 I argued why having a "group" feature is better than returning an identifier.

Having said that, it seems like there is agreement for passive to not be part of the key. I will make that change after landing once, since that puts all the infrastructure in place for it.

[ojanvafai]

I think the group feature is fine as well. I see the appeal of being able to remove a group of listeners at once, but I'm a bit skeptical that authors will actually use it that way. In either case, I don't feel strongly between the two options.

To be clear, I wasn't making an argument that passive be part of the key or not part of the key. It's weird to me that passive wouldn't be part of the key, but frankly, I'm not invested enough in this problem to have an informed opinion, so I defer to @RByers and @dtapuska here.

[RByers]

Having said that, it seems like there is agreement for passive to not be part of the key. I will make that change after landing once, since that puts all the infrastructure in place for it.

Thanks Anne. Filed a bug to update the blink implementation

But we still need to iron out the exact semantics (ideally prior to reviewing the PR over in WHATWG). I see two main options:

1. Completely ignore any subsequent addEventListener calls. I.e. for all options (unless described otherwise) the call is just a no-op. This has the advantage of being consistent among most new options without requiring special cases. But it has the disadvantage of possibly causing surprising behavior - eg. it's possible to add a non-passive listener which can't actually use preventDefault (because an identical passive listener was already added). But I think that's really an edge case that represents an app bug, at least in the case of passive. passive is a promise about the callback behavior, there's no good reason why the same callback could be considered to both be "passive" and "not-passive" (either the code may call preventDefault or it definitely won't).

2. For each option that doesn't impact the key, define what the union of the two options sets are. For passive I think we'd want "OR" (if either request was for a non-passive listener then you get non-passive behavior).

Personally I think I prefer option 1, but I haven't thought through the implications for the other potential options we're talking about. I guess we could go with 1 for passive and leave the door open to doing 2 for new options where it might make more sense (eg. a 'maxRate' option).

@jacobrossi any input?

[annevk]

Yeah, my thinking was 1. That keeps the logic very straightforward: If input.key not in eventListeners, then append input to eventListeners.

[smaug----]

if we don't do (1), then passive would be somehow part of the key.
and (2) may not quite work, assuming we'll end up having properties which are something one can "OR" easily.

[jacobrossi]

What about libraries that do event delegation? If you force only the first registration to win, then I can see libraries just ignoring whether their listeners want passive or not -- they just register their delegate as non-passive always. But for an option like (2), you would give the library a way to support exposing passive to their listeners and then the browser handles whether the event can be fired passively (using the OR union logic).

I suppose, event with (1), a library could remove its delegate listener and re-add it if it needs to change its passive status. But that might not be intuitive.

[annevk]

Event delegation works by registering a single listener that handles the events for many of its descendant targets. I don't quite understand the scenario you're sketching. Also, event delegation is something we should support natively, see whatwg/dom#215 for that.

[smaug----]

One, perhaps a bit ugly option, is to throw if one tries to add existing listener with same key but different EventListenerOptions.

[RByers]

In the event delegation scenario, any such library should really be delegating separately for the passive and non-passive clients (and so installing two real listeners with different handler functions). Eg. we wouldn't want one tiny non-passive touch listener on the page to cause an existing passive touch listener on the window to be treated as non-passive (that would be really bad for perf). I agree it's possible that event delegation code might screw this up, but I think there's a lot of ways they could screw up in any of the options, not clear to me that either approach is substantially less error-prone. Or maybe I don't understand completely?

One, perhaps a bit ugly option, is to throw if one tries to add existing listener with same key but different EventListenerOptions.

I'd be fine with that, it also gives us the flexibility to potentially relax in the future if we deem it valuable.

[domenic]

The problem with throwing is that we don't do that for capture, so we'd have to allow same-listener/different-options in the case where the options vary in their capture value. (Unless the proposal is to add another bit to the key, "boolean or object", then throw only for cases where "boolean or object" is "object" and any of the options differ.)

[smaug----]

well, .capture is part of the key. So we'd first look for (type, listener, capturingOrNot) listener, and then compare the options.
(Using 4th param approach would have made also this a bit nicer.)

[jacobrossi]

I think it's worth pondering how something like PEP would handle this. Requiring separate delegators just for this feature might more overhead than the library authors deem worth it. jquery-archive/PEP#278 (comment)

[annevk]

Let me try again, could you explain in more detail how this affects delegation?

[annevk]

@RByers when is the next version of Chrome coming out? I'd like to see this resolved and implemented as resolved by then.

[RByers]

I think it's worth pondering how something like PEP would handle this.

I think the PEP case is conceptually easy actually (although the implementation may be complicated). They ALWAYS install just passive touch listeners for the purpose of generating pointer events (since pointer events never block scrolling). The trickier part is in emulating touch-action. But really that's orthogonal (since some browsers support touch-action but not pointer events). Emulating touch-action would need to rely on it's own non-passive touch listener. Since pointer event listeners and any usage of touch-action could be on unrelated parts of the DOM, I think these really should be separate listeners anyway. An implementation of touch-action emulation may choose to simplify things by relying on a single document-wide listener, but that's going to always have to be non-passive (and so no great for performance).

@RByers when is the next version of Chrome coming out? I'd like to see this resolved and implemented as resolved by then.

Chrome 51 (the release with passive) is scheduled to hit stable around the end of May. I'm all for getting this change done now if there's rough consensus on the design and trying to include it in the M51 release. But worst case (eg. the change is too involved and/or late to merge to the M51 branch), I don't think we need to rush - I'm confident we can change this in M52 without much compat risk if necessary. @annevk do you disagree?

Regardless it sounds like there's rough consensus here that we should follow option 1) above. Unless anyone has a concrete reason why that would be worse than some other choice, I'll prepare a PR to the DOM spec for this next week (on vacation for most of the rest of this week).

[jacobrossi]

Thinking on this further, I don't think the delegation scenario presents much of an issue. Carry on. :-)

[annevk]

Sounds good. @RByers I don't disagree regarding the risk, I just didn't want to delay it more than one release after passive ships.

[RByers]

Yep, agreed @annevk. PR up for review - it was trivial thanks to your once work (although I was really tempted to try to find a better name for "flatten more options" ).

[annevk]

If you can think of one let me know.