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

[foolip]

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]

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

[smaug----]

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

[RByers]

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]

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]

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]

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]

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]

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

[RByers]

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]

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

[RByers]

@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]

Oh, right :)

[foolip]

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]

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

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

[RByers]

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

[smaug----]

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]

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]

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" (a "YuMeHTMLSDK" object) 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 2 3 4]
• 1 was due to a script "swfobject.js" that was passing "null". This is totally fine (null was falsy before and still is). [1]
• 1 didn't seem to be triggering at in manual testing (maybe non-deterministic, or the site just changed, dunno) [1]
• 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 2]

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]

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
#21 (comment)

[RByers]

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