search

WICG / EventListenerOptions

An extension to the DOM event pattern to allow authors to disable support for preventDefault
Other
1.17k stars 140 forks source link

Safe to change behavior of addEventListener(type, callback, object)? #21

Closed foolip closed 8 years ago

foolip commented 8 years ago

If the third argument is of type (boolean or EventListenerOptions), it seems like this would have to change the behavior of existing calls to addEventListener(type, callback, {}) or any kind of object as the third argument.

This would of course be accidental, but given how many calls to addEventListener are out there it doesn't seem necessarily safe to change. If overloading the third argument still seems like the way to go, the size of the problem could be measured in Blink.

dtapuska commented 8 years ago

It should be pointed out Gecko has a fourth argument for trusted events.

smaug---- commented 8 years ago

It is 4th argument http://mxr.mozilla.org/mozilla-central/source/dom/webidl/EventTarget.webidl?rev=2db3491d59e4&mark=23-23

RByers commented 8 years ago

I'm not convinced we can really measure the size of the risk here. Sure we can count how many calls there are like addEventListener(type, callback, {}) but what fraction of those really intended to get capture=true instead of capture=false? I think there's a decent chance that by switching our behavior from capture-phase to non-capture-phase we'll actually fix more bugs that we'll create :-).

I don't know any better option than just to try. Of course we could try making this particular change now on it's own and see if it has any impact, ahead of doing all the work for the new API. Not sure it's worth it though.

Is this really any different from, say, adding a new argument to an existing API? People might be accidentally passing all sorts of things as an extra argument already and now we'll start interpreting it somehow rather than ignoring it. Do we really normally worry about that sort of thing pro-actively?

foolip commented 8 years ago

I think the more likely scenario is addEventListener(type, callback, capture) where capture was initialized by some expression that's truthy, but also happens to be an object.

Given that addEventListener() must be called on a large majority of pages, it doesn't seem too far fetched that this could have happened by accident non-trivial percentage of pages, or worse, in some framework.

Adding a new argument is almost entirely risk free and we don't worry about those cases, but this is changing the interpretation of an existing argument.

If the risk does not seems plausible then just trying it is fine by me, but measuring it now could save a few months if it turns out to be a problem.

annevk commented 8 years ago

We could just default to false unless any other dictionary members are given. Seems the safest. Unless of course someone is passing in objects now that have a capture member...

foolip commented 8 years ago

To preserve the behavior of existing code that calls addEventListener(type, callback, {}) (without realizing it), capture would have to default to true, which is a terrible default.

RByers commented 8 years ago

I suspect the risk here is pretty low. I suggest that we try to ship the API we like best in blink, and watch for any compat issues. If we find non-trivial issues then we can hold off on shipping and revisit the design.

RByers commented 8 years ago

Chromium is tracking shipping this here, hopefully this will happen soon.

RByers commented 8 years ago

Update: we see about 0.03% of page loads in Chrome stable passing an object to addEventListener's 3rd argument. This is higher than we expected but still relatively low (around the level we'd consider removing a feature). The behavior change for this has been in Chrome beta for ~4 weeks without a single report of an issue, and so is about to hit Chrome stable. I'll follow-up once we have a couple weeks of experience with this change in Chrome stable.

foolip commented 8 years ago

@RByers s/stable/beta/ for the first occurrence?

RByers commented 8 years ago

@RByers s/stable/beta/ for the first occurrence?

Nope, we've had the use counter since Chrome 48 (stable for ~6 weeks) but the behavior change (shipping EventListenerOptions) was in Chrome 49 (soon to be stable, in beta for past 4 weeks).

foolip commented 8 years ago

Oh, right :)

foolip commented 8 years ago

https://www.chromestatus.com/metrics/feature/timeline/popularity/967 is live again and it looks like it's actually around 0.01%.

And with M49 having been stable for almost two weeks, I think this issue can be closed and answered in the affirmative: it is safe to change this behavior.

RByers commented 8 years ago

Hah, nice timing - I had just decided this morning that it was time to close this today. I won't be surprised if we discover one or two breaks somewhere, but it's clearly not a substantial compat problem.

smaug---- commented 8 years ago

I hope blink folks are ok to change the behavior of 3rd param, if that is decided in that other bug.

RByers commented 8 years ago

@smaug---- You're just saying you hope #12 is resolved? Or something else?

smaug---- commented 8 years ago

Yeah, referring to that bug. It may or may change the 3rd param back to boolean. (If I was doing this all in Gecko, I definitely wouldn't ship 3rd param change when there are that kind of open issues.)

foolip commented 8 years ago

The point of shipping that was to find out if it would be safe or not as fast as possible, to revert and plot a new course if not.

RByers commented 8 years ago

I now have a new tool for answering compat questions like this (cluster runs on top websites). I checked the Alexa Top 10k for what sites were hitting our UseCounter for this during page load and found 8:

All this is to confirm what we already knew (since this has been shipping in Chrome for awhile) - this change is quite web compatible.

dtapuska commented 8 years ago

totalcar has an addEventListener in swiper3.1.2.min.js that they seem to be passing a function in as the 3rd argument.

mop.com is just too slow to load for me to debug.

On Wed, Apr 6, 2016 at 10:25 PM, Rick Byers notifications@github.com wrote:

I now have a new tool for answering compat questions like this (cluster runs on top websites). I checked the Alexa Top 10k for what sites were hitting our UseCounter for this during page load and found 8:

  • 4 of them are due to some "YuMe HTML5 SDK" where "this" is being passed for the capture argument of a "click" listener. It's clearly just a bug, I don't see any obvious reason why they'd really want capture (triggering ad clicks I think), so probably fine to change but could be a break. [1 http://www.mediotiempo.com 2 http://www.timesofisrael.com 3 http://www.record.com.mx 4 http://www.haaretz.com]
  • 1 was due to a script "swfobject.js" that was passing "null". This is totally fine (null was falsy before and still is). [1 http://www.movembed.com]
  • 1 didn't seem to be triggering at in manual testing (maybe non-deterministic, or the site just changed, dunno) [1 http://www.haaretz.co.il]
  • 2 were still triggering the use counter but I had some problem finding the code responsible (I'm overriding EventTarget.prototype.addEventListener to catch it in the debugger). Not going to bother digging further ATM [1 http://www.mop.com 2 http://www.totalcar.hu]

All this is to confirm what we already knew (since this has been shipping in Chrome for awhile) - this change is reasonably web compatible.

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/WICG/EventListenerOptions/issues/21#issuecomment-206661449

RByers commented 8 years ago

Yeah I see the totalcar code now - thanks. Looks like someone wrote = when they probably meant == (want capture when two variables refer to the same function? Kinda weird but whatever). This is for the "click" listener on the buttons on either sides of the image carousels near the bottom of the page. It seems to work fine, so there's no obvious reason why not getting capture makes a difference in this page (but it's of course possible there's a subtle bug).