Skip to content
This repository has been archived by the owner on Mar 19, 2018. It is now read-only.

Rather than overload the 3rd argument of addEventListener could we overload the 2nd? #11

Closed
RByers opened this issue Jul 3, 2015 · 34 comments
Labels

Comments

@RByers
Copy link
Member

RByers commented Jul 3, 2015

The 2nd argument to addEventListener is an EventListener callback interface. We could possibly add additional attribute members to this interface. Eg.

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

Event registration code would then look like:

element.addEventListener(type, {handleEvent: function(e) {
    ...
  }, mayCancel: false});

This would separate capture from mayCancel but it makes some sense since mayCancel is describing a property of the EventListener. This would have the benefit of being backwards compatible (although we'd still want to encourage use of a polyfill to prevent accidentally depending on cancelable events). This would also avoid the need to change any of the language around the capture variable and what constitutes a redundant (and so ignored) event handler registration. However this seems like it might be an abuse of callback interfaces, is there precedent?

@RByers RByers added the spec label Jul 3, 2015
@RByers
Copy link
Member Author

RByers commented Jul 3, 2015

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?

@AshleyScirra
Copy link

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..

@annevk
Copy link
Collaborator

annevk commented Jul 12, 2015

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

@annevk annevk closed this as completed Jul 12, 2015
@RByers
Copy link
Member Author

RByers commented Jul 12, 2015

SGTM, thanks

@RByers
Copy link
Member Author

RByers commented Mar 17, 2016

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

@RByers RByers reopened this Mar 17, 2016
@jacobrossi
Copy link

+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.

@smaug----
Copy link
Collaborator

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)

@RByers
Copy link
Member Author

RByers commented Mar 17, 2016

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.

@RByers
Copy link
Member Author

RByers commented Mar 17, 2016

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).

@paulirish
Copy link
Contributor

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.

@smaug----
Copy link
Collaborator

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.

@annevk
Copy link
Collaborator

annevk commented Mar 18, 2016

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.

@foolip
Copy link
Member

foolip commented Mar 18, 2016

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.

@AshleyScirra
Copy link

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.

@annevk
Copy link
Collaborator

annevk commented Mar 18, 2016

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

@RByers
Copy link
Member Author

RByers commented Mar 18, 2016

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.

@annevk
Copy link
Collaborator

annevk commented Mar 18, 2016

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.

@smaug----
Copy link
Collaborator

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.

@RByers
Copy link
Member Author

RByers commented Mar 18, 2016

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).

@smaug----
Copy link
Collaborator

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.

@annevk
Copy link
Collaborator

annevk commented Mar 18, 2016

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.

@RByers
Copy link
Member Author

RByers commented Mar 18, 2016

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.

@paolocaminiti
Copy link

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...

@foolip
Copy link
Member

foolip commented Mar 21, 2016

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.

@annevk
Copy link
Collaborator

annevk commented Mar 21, 2016

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

@foolip
Copy link
Member

foolip commented Mar 21, 2016

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?

@annevk
Copy link
Collaborator

annevk commented Mar 21, 2016

Filed w3c/webrtc-pc#559.

@annevk
Copy link
Collaborator

annevk commented Mar 21, 2016

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

@smaug----
Copy link
Collaborator

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".

@annevk
Copy link
Collaborator

annevk commented Mar 21, 2016

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.

@smaug----
Copy link
Collaborator

"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.)

@RByers
Copy link
Member Author

RByers commented Mar 21, 2016

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.

@jacobrossi
Copy link

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.

@RByers
Copy link
Member Author

RByers commented Apr 5, 2016

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.

@RByers RByers closed this as completed Apr 5, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

8 participants