Rather than overload the 3rd argument of addEventListener could we overload the 2nd? about eventlisteneroptions HOT 34 CLOSED

We couldn't do the above as it would be a breaking change due to the definition of single operation callback interfaces, plus we'd need some way to indicate the default value for myCancel. Maybe:

callback interface EventListenerWithOptions extends EventListener {
  readonly attribute boolean mayCancel;
};
partial interface EventTarget {
  void addEventListener(DOMString type, EventListenerWithOptions callback, optional boolean capture = false);
  void removeEventListener(DOMString type, EventListenerWithOptions callback, optional boolean capture = false);
};

But this still doesn't scale nicely to support other optional options. Perhaps we could use a dictionary with a handleEvent callback function member?

from eventlisteneroptions.

Why not go further and make all parameters passed through a dictionary?

target.addEventListener({
    type: "scroll",
    handler: function (e) { ... },
    useCapture: false,
    mayCancel: true
});

This seems tantamount to a full API redesign though, and probably would deserve a new function name too..

from eventlisteneroptions.

Going to close this. Callbacks should be simple functions, the interface callback thing is mostly about compatibility.

from eventlisteneroptions.

SGTM, thanks

from eventlisteneroptions.

Give the ongoing debate here let's hash this out a little further. Would it really be so bad to have this?

callback interface EventListener {
  void handleEvent(Event event);
  readonly attribute boolean capture;
  readonly attribute boolean passive;
};

I know callback interfaces have fallen out of favor, but my reading of the WebIDL spec around this suggests that there's nothing really wrong with the above pattern. Maybe it's a reasonable pragmatic trade off to resolve the dispute in #12 in a way everyone can live with? We, of course, can still aim to replace addEventListener in the future to get rid of this legacy long-term. But is there some reason why this design would be really bad? It's certainly not too ugly from the web dev standpoint. Eg:

element.addEventListener(type, {passive:true, handleEvent: function(e) {
    ...
  }});

Seems nicer to me than:

var supportsPassive = false;
document.createElement("div").addEventListener("test", function(){}, { get passive() { supportsPassive = true }});
element.addEventListener(type, function(e) {
  ...
}, supportsPassive ? { passive: true } : false); 

@annevk @domenic @smaug---- @tabatkins @paulirish @jacobrossi @esprehn @grorg @foolip @phistuck @dtapuska

from eventlisteneroptions.

+1

handleEvent will be part of the event listener paradigm until we replace aEL (note a similar behavior exists for handleChange in MediaQueryListener). So it's seems sensible to me to resolve #12 this way until that time.

from eventlisteneroptions.

So {handleEvent: function(){}} would behave as now, but there could be possibly also passive and capture boolean properties in object passed in?
Or was readonly attribute boolean cancel; on purpose? (I think you mean capture)

from eventlisteneroptions.

So {handleEvent: function(){}} would behave as now, but there could be possibly also passive and capture boolean properties in object passed in?

Right

Or was readonly attribute boolean cancel; on purpose? (I think you mean capture)

Ooops, thanks - fixed.

from eventlisteneroptions.

And to be concrete, I'm also proposing we'd change the existing boolean capture arg to optional and specify that when it's specified it takes precedence over the capture property here.

Alternately I'd be OK omitting a duplicate capture entirely if people would prefer that (though I know Tab and Anne among others would like some path for improving the syntax for capture).

from eventlisteneroptions.

IMO the ergonomics of using the EventListener callback interface for these options is a huge win. It makes for far more predictable behavior for older browsers. And yeah, as the examples in Rick's comment above shows, the code is far more readable.

from eventlisteneroptions.

The current boolean capture (3rd param) is already optional.

I'd prefer to not add capture to this 2nd param approach. There isn't really any need for that, and it just adds compat risk, similar to the 3rd param approach.

Without capture in 2nd param, I'm ok with this approach. Spec'ing it shouldn't be too hard.
The passive property would be read as if the passed object was a dictionary (so read only once when the addEventListener is called), but otherwise 2nd param would work as it works now.

from eventlisteneroptions.

I don't see how this could work. IDL has very precise semantics for dictionaries and very precise semantics for callback interfaces (which are deprecated). I don't see how this works. Dismissing how this is bound as @paulirish did also seems misguided. That would be an actual backwards incompatible change.

from eventlisteneroptions.

To spell it out, the proposed IDL syntax above wouldn't work. Unless WebIDL is changed, the argument would have to be object and everything would have to be done with prose.

from eventlisteneroptions.

Is the one-argument case off the cards?

target.addEventListener({
    type: "scroll",
    handler: e => ...,
    capture: false,
    passive: true
});

As a web developer, if not this approach IMO overloading the 3rd parameter makes most sense. The 2nd param overload looks like a hack: you're starting to spread the event handler options across formal parameters and the options object.

from eventlisteneroptions.

@AshleyScirra yes, that's inconsistent with new Event().

from eventlisteneroptions.

I don't see how this could work. IDL has very precise semantics for dictionaries and very precise semantics for callback interfaces (which are deprecated).

Forget the garbage I proposed in #12 about the 2nd arg as a dictionary. Here I'm talking only about extending the callback interface (so no change in this from today, etc.).

To spell it out, the proposed IDL syntax above wouldn't work. Unless WebIDL is changed, the argument would have to be object and everything would have to be done with prose.

I'm probably missing something, but reading the WebIDL definition of callback interface and associated ES binding, I only see one small problem. We'd have to change is the definition of single operation callback interface so that my proposed extension to EventListener would still be considered "single operation" (and so not a breaking change). Eg. perhaps it's fine to just ignore the attributes, or maybe an extended attribute is needed on the handleEvent operation to mark it as the "single operation".

Perhaps there's also a 2nd issue with correctly supporting the case where passive isn't provided, but from my reading of the spec I think we'd just get the value undefined and could handle that identically to false via prose in the DOM spec.

Is there something else I'm missing for why this wouldn't work by extending callback interfaces? If there's not something obvious, then we can ask heycam for his input.

from eventlisteneroptions.

There's nothing there that actually gives you access to the values of arbitrary objects members, or mandates they are accessed in a specific order, as is important for dictionaries. There's only two callback interfaces in the platform or so, and neither needs this functionality.

from eventlisteneroptions.

It would be easy to say in prose that when an object is passed to addEventListener, (1) it is converted to some dictionary and .passive is read from it (defaulting to false)
(2) And then the object would be converted to EventListener.

from eventlisteneroptions.

There's nothing there that actually gives you access to the values of arbitrary objects members

Not "arbitrary" but specific attributes appear to be supported. Eg. there's this describing how arbitrary objects (including their attributes) are bound to callback interfaces:

Any object that is not a native RegExp object is considered to implement the interface.
...
The value of a user object’s attribute is retrieved using the following algorithm:
...

And there's even this example (albeit with a "suggestion" that you prefer using a dictionary because there's no operations here):

callback interface Options {
  attribute DOMString? option1;
  attribute DOMString? option2;
  attribute long? option3;
};

@heycam is there anything wrong with having a callback interface like this?

callback interface EventListener {
  void handleEvent(Event event);
  readonly attribute boolean passive;
};

I know it's a little weird, but (if we could make it a non-breaking change by tweaking the "single-operation callback interface" definition) it would have forwards compat properties that some people REALLY like (#12).

from eventlisteneroptions.

I would do the conversion in 2 steps. There would be dictionary EventListenerOpts { boolean passive = false; } and the existing EventListener. Then change the callback to object? and in prose say that first callback is converted to EventListenerOpts, similar to https://html.spec.whatwg.org/multipage/scripting.html#coerce-context-arguments-for-2d and passive value is read from there. (if .passive getter throws here, we'd pass the exception to caller and abort the algorithm). Then object is converted to EventListener and that part would work as now.

from eventlisteneroptions.

I thought about this a bit and I have these concerns:

• This approach is actually backwards incompatible. It's totally legit to pass in your own object as second argument. We are now reducing the number of members you can use there safely. That'll be a continuous hazard going forward as we add more. Worse, we might break actual content, which I think is far worse than the theoretical risk of some site not working until the user updates their browser.
• This approach is inconsistent with virtually every other event listener on the platform in that this will not point to the target, ever. That seems like a huge pain when writing reusable code.
• This approach makes us store an extra object per listener. That seems suboptimal.

I don't think it's worth pursuing this anymore.

from eventlisteneroptions.

This approach is actually backwards incompatible.

If we restrict this decision to just what to do about passive, this is something we could test concretely. I suspect the risk of existing callback interfaces having a member named passive is pretty low, but we could find out for sure.

But the argument about it not being a good pattern to build on in the future (the way EventListenerOptions could be) is a good one.

This approach is inconsistent with virtually every other event listener on the platform in that this will not point to the target, ever. That seems like a huge pain when writing reusable code.

Good point. So in practice people would probably want to write something ugly like this instead:

document.addEventListener(type, {passive: true,
  handleEvent: function(e) {
    (function(e) {
      .... 
    }).call(e.currentTarget, e);
  }
});

And I'm sure developers would often forget about the difference in this and so would have subtle bugs by forgetting to accommodate it when upgrading existing code to add passive.

This approach makes us store an extra object per listener.

Good point. It's common to have thousands of listeners in a frame, so the memory costs are potentially non-trivial. If this were the only potential deal breaker here I could do some work to quantify it a little better.

from eventlisteneroptions.

I've being using the handleEvent syntax quite a bit for animations (to keep state in the listener this and ease of removal), never had a "passive" property in the 2nd argument, but it seems to me that adding an arbitrary field to the object is not much forward looking: when usage spread people will have all sort of custom properties names and adding new features will be harder than is this time. It should then be instead an "handleEventOptions" object property able containing all possible future options. Or you may overload "handleEvent" to accept an object containing the handler and all the options as properties.

That said with everybody using event delegation won't the optimization be impossible as the not passive form of the listener would anyway be registered somewhere up in the DOM? Wouldn't instead be possible to have the passive behaviour be a property of the listener's target element, set via js or css like "-touch-action" is? Or have a completely new set of events like "touchmovePassive" carrying the behaviour?

If you are to change addEventListener itself, than to me a new 4th boolen seems the least weired way to go, when not present it will just be unoptimized, feature detection on something basic as addEventListener looks as frustrating as detecting attachEvent...

from eventlisteneroptions.

Thanks @RByers for pointing out that I was wrong about the WebIDL spec, it does indeed say things about callback interface attributes. Still, the current set of callback interfaces that may end up as widely implemented parts of the web platform are EventListener, NodeFilter, RTCIdentityProvider and XPathNSResolver, and none of these have attributes, yet.

No doubt this could be made to work, but a few things come to mind as not great:

1. Anyone who currently uses plain functions would have to keep track of the extra object if they later need to call removeEventListener. Some will likely try to use a new object literal instead.
2. It wouldn't be a proper dictionary, so if we ever come up with an API for easier feature detection of dictionary members, it wouldn't work here.

When it comes to backwards compat, to be fair the current spec also has risk, in that any code that already passes an object as the 3rd argument will see a change in behavior, from capturing to non-capturing. That's why the risk was measured and EventListenerOptions shipped without passive in M49.

from eventlisteneroptions.

Oh god, why was RTCIdentityProvider introduced? Bah, we really should have named this legacy callback interface.

from eventlisteneroptions.

I was also a bid sad to find it in Gecko's source code. File an issue at https://github.com/w3c/webrtc-pc/issues to see if it's too late?

from eventlisteneroptions.

Filed w3c/webrtc-pc#559.

from eventlisteneroptions.

So with @foolip's two additional points we now have five solid arguments against this approach. Time to close this issue?

from eventlisteneroptions.

Well, 3rd param approach has clear arguments against it too.
So we're still in state "should we change handling of 2nd or 3rd parameter or add 4th".

from eventlisteneroptions.

Well, no, we'd only need to consider 3 or 4 arguments, since 2 clearly doesn't work. I think we've made it sufficiently clear why 4 arguments is not a good idea either, but we can revisit that again I suppose, in a separate thread.

from eventlisteneroptions.

"clearly doesn't work"? I must be missing such argument. It is perhaps suboptimal approach, sure, but so are others too.

(Currently I'm back in favoring 4th param approach. I was hoping some agreement could be achieved with 2nd param approach and I was ok with that, but looks like I was too optimistic.)

from eventlisteneroptions.

Between the this binding issue, and extra difficulty of removal, the 4th arg option seems strictly superior in terms of developer ergonomics than this 2nd arg option . So unless anyone has any other arguments (@jacobrossi?) I agree we should (again) close this and focus any further debate on just the 4th arg option in #12.

from eventlisteneroptions.

I'm ok with either approach. I prefer the 2nd arg approach as I think the mandated bool in the 3rd arg is more annoying to a dev than learning a different this (it works no differently than handleEvent already does and as @paulirish says, many are already rebinding this anyway) or removing the listener (you can either pass in the same object you gave aEL or just clear handleEvent). But I won't object to the 4th arg approach if there's a consensus that is better.

from eventlisteneroptions.

Based on all the discussion/concerns, it seems the 4th arg option generally better than any sort of 2nd arg option so I'm going to close this issue and continue the discussion #12.

from eventlisteneroptions.