Skip to content
This repository has been archived by the owner. It is now read-only.

Should the API be forwards compatible? #12

Closed
RByers opened this Issue Jul 3, 2015 · 144 comments

Comments

Projects
None yet
@RByers
Copy link
Member

RByers commented Jul 3, 2015

Is the requirement to use feature detection or a polyfill OK?

Olli Pettay suggests:

...but unfortunately that is not backwards compatible. If we could find a nice backwards compatible syntax.
addEventListener("wheel", listener, true, { preventDefault: "never" }); would behave fine, but is a bit long. Would it be good enough?)

To me the benefit of this doesn't seem worth the cost in verbosity, but I could live with it if there was consensus otherwise.

Another option is discussed in #11

@RByers RByers added the spec label Jul 3, 2015

@RByers RByers changed the title Should we aim to be backwards compatible? Should the API be backwards compatible? Jul 6, 2015

@RByers

This comment has been minimized.

Copy link
Member Author

RByers commented Jul 6, 2015

I'd argue that as long as we're changing cancelable (#2) then backwards compatibility should not be a goal. Developers need to use a polyfill for proper test coverage.

@RByers

This comment has been minimized.

Copy link
Member Author

RByers commented Sep 22, 2015

Even though we're not directly changing cancelable (#2), we concluded that we would still change behavior so this is more than a hint and so full backwards compatibility isn't possible (developers must use a polyfill / feature detection).

@RByers RByers closed this Sep 22, 2015

@RByers RByers changed the title Should the API be backwards compatible? Should the API be forwards compatible? Feb 19, 2016

@RByers

This comment has been minimized.

Copy link
Member Author

RByers commented Feb 19, 2016

There's renewed debate on this point, let's continue the discussion here?

@RByers RByers reopened this Feb 19, 2016

@RByers

This comment has been minimized.

Copy link
Member Author

RByers commented Feb 19, 2016

Perhaps we should brainstorm a bit about alternatives which are more "forwards compatible"? We can't achieve perfect forwards compatibility while preserving the preventDefault impact we want. But we could design an API where the options are just ignored by older browsers.

Options I see that are largely "forwards compatible":

  1. Just make passive the 4th boolean option: addEventListener(type, handler, useCapture, usePassive). Not compatible with Gecko's non-standard extension, but perhaps that could be broken?
  2. Move the options dictionary to the 4th argument withpassive the only option for now: addEventListener(type, handler, useCapture, options). Still potentially a compat issue for Gecko?
  3. Tweak the method name and always encourage use of a trivial polyfill for forwards compat:
    addListener(type, handler, options). Isn't really any more "forwards compatible".
  4. Define a dummy 4th arg and add a 5th? addEventListener(type, handler, useCapture, mustBeFalse, options)

@smaug---- @jacobrossi @domenic @annevk @dtapuska @foolip thoughts?

@phistuck

This comment has been minimized.

Copy link

phistuck commented Feb 19, 2016

Mm... 5. Add a single argument overload that only accepts a dictionary. addEventListener({type: "touchstart", handler: function () {}, capture: false /* or just omit it */, passive: true})
Should be familiar from jQuery.

@RByers

This comment has been minimized.

Copy link
Member Author

RByers commented Feb 19, 2016

But that's not forwards compatible either - will either do nothing or fail with an exception on older browsers. But, like 3, maybe if the polyfill is a trivial one-liner that's still ok?

@phistuck

This comment has been minimized.

Copy link

phistuck commented Feb 19, 2016

Perhaps this is not forward compatible because it requires feature detection (an exception is a very easy feature detection, though), but I think this (having a single dictionary parameter) is the approach the web platform is taking recently. Since (at first at least) developers will be using this for passive: true and not for regular event listeners, this does not seem like a bad idea to me.

@domenic

This comment has been minimized.

Copy link
Collaborator

domenic commented Feb 19, 2016

@RByers I really think the current approach is the best. But if I had to pick one of the alternate approaches, I guess 1 or 2 is not the worst.

I agree that @phistuck's 5 does not solve anything compared to the current approach; it requires just as much workaround/polyfilling/etc. and does not have the apparently-desired property of people being able to write one line of code that is automatically passive/non-capturing in new engines and non-capturing in old engines.

@RByers

This comment has been minimized.

Copy link
Member Author

RByers commented Feb 19, 2016

Thanks @domenic, the current approach seems better than any of these to me too, but I don't know how to evaluate that beyond just a judgement call / gut impression of the compat risks. Any suggestion for how we could bring more data to the debate? Eg. are there other examples where we've ignored forwards compat beyond the common case of just adding new APIs? Every time we add a new argument to a method, has it been designed such that ignoring the argument is unlikely to be breaking?

@tabatkins may also have input.

@phistuck

This comment has been minimized.

Copy link

phistuck commented Feb 19, 2016

And another one (which I actually think I initially preferred, within the intent thread) is the same as my 5. - but with a different name (addListener, or on).

@RByers

This comment has been minimized.

Copy link
Member Author

RByers commented Feb 19, 2016

And another one (which I actually think I initially preferred, in the intent thread) is the same as my 5. - but with a different name (addListener, or on).

That's my 3, right?

@phistuck

This comment has been minimized.

Copy link

phistuck commented Feb 19, 2016

Almost, yes - 3 + 5 -
addListener({type: "touchstart", handler: function () {}, capture: false /* or just omit it */, passive: true})
Or -
on({type: "touchstart", handler: function () {}, capture: false /* or just omit it */, passive: true})

@annevk

This comment has been minimized.

Copy link
Collaborator

annevk commented Feb 20, 2016

I don't see a problem with the current approach. This is no different from any other API change. Browsers should just implement the dictionary syntax.

@jacobrossi

This comment has been minimized.

Copy link

jacobrossi commented Feb 22, 2016

@annevk not all browsers are committed to implementing this and even if all browsers implemented this tomorrow we can't add it to existing extended support browsers (IE11, Firefox ESR, etc.). In the blink-dev thread I outlined how this will likely cause unnecessary compatibility issues for these browsers. We could easily avoid this with an approach like 1 or 2 above.

@annevk

This comment has been minimized.

Copy link
Collaborator

annevk commented Feb 22, 2016

1/2 are both rather terrible. I really don't see how this case is different from any other API extensions.

@jacobrossi

This comment has been minimized.

Copy link

jacobrossi commented Feb 22, 2016

Could you elaborate on why you think they're terrible? It's not clear to me why these are bad options.

What we're talking about here isn't really a typical API extension, it's an API overload. The overloading is what introduces unnecessary compatibility pain.

@annevk

This comment has been minimized.

Copy link
Collaborator

annevk commented Feb 22, 2016

Because boolean arguments are bad (they're fine in a dictionary because the name will always be visible from the caller). Making a boolean argument required for every new feature we're going to add is even worse.

@jacobrossi

This comment has been minimized.

Copy link

jacobrossi commented Feb 22, 2016

I agree that if we were designing this from scratch in 2016 we would only use a dictionary. Unfortunately, browsers have all been shipping this API for years with the boolean argument, which means there are compat issues we have to address.

@annevk

This comment has been minimized.

Copy link
Collaborator

annevk commented Feb 22, 2016

We don't address forward compatibility problems. We never have.

@annevk

This comment has been minimized.

Copy link
Collaborator

annevk commented Feb 22, 2016

And even if we were to start now, preserving that boolean argument is not going to fly.

@foolip

This comment has been minimized.

Copy link
Collaborator

foolip commented Feb 22, 2016

The very first wave of users will have to use feature detection since browsers this will have a minority usage share. Is the concern that adoption in the browsers web developers use will be so quick that they will stop using feature detection before that's actually safe to do in the general population? That sounds like the same situation as with every new feature, or is there a difference?

@smaug----

This comment has been minimized.

Copy link
Collaborator

smaug---- commented Feb 22, 2016

I would probably go with (2). It happens to also clarify what each of the params are for.
1st param to addEventListener would be 'type', telling what event is being listened for
2nd param would be 'listener' - 'what is listening the event'
3rd param would be 'capture' - 'where in the propagation path the event is being listened for'
4th param then (EventHandlingOption or boolean)[1] telling 'how the event will be handled'

dictionary EventHandlingOption
{
boolean passive;
};

[1] boolean just to be compatible with Gecko

@annevk

This comment has been minimized.

Copy link
Collaborator

annevk commented Feb 22, 2016

@smaug---- if at some point we decide that you want listener to be invoked for anywhere in the event path, what do you do then? Locking the third parameter to "capture" precludes those kind of extensions. Doesn't seem like a good idea.

@annevk

This comment has been minimized.

Copy link
Collaborator

annevk commented Apr 5, 2016

Sure, I'm not sure if that brings us far into the weeds, but okay: target.addEventListener(type, callback, { ignoreBubbles: true }). And then we'd adjust event dispatch https://dom.spec.whatwg.org/#dispatching-events to check event's bubbles attribute at a later stage, while invoking the listeners, and ignore it completely if the listener has an "ignore bubbles flag" set. This is an oft-requested feature from the developer community, who would very much like all events to bubble, so they can use event delegation tactics on all of them equally.

@foolip

This comment has been minimized.

Copy link
Collaborator

foolip commented Apr 5, 2016

Thanks, I just wanted to see how it would fit into the addEventListener mold. Is there any feature request you're aware of where a new method would make sense?

@RByers

This comment has been minimized.

Copy link
Member Author

RByers commented Apr 5, 2016

I think the 4th arg is a good compromise,

@jacobrossi FWIW I think that's a mis-interpretation of this entire debate. It's always been 3rd arg vs. 4th arg - there is sadly apparently no compromise in the middle that satisfies both the developer ergonomics and forwards compat concerns.

I think the only "compromise" would be to agree that there are a number of ergonomics issues with addEventListener and so it's important for us to invest in designing a replacement API for the long-term. Absent that we need to choose one side or the other.

@annevk

This comment has been minimized.

Copy link
Collaborator

annevk commented Apr 5, 2016

@foolip I think the main reason for a new method would be to shorten the amount of characters. E.g., try to go from addEventListener to on, although that of course caries a number of risks on its own. The only other potential reason would be TC39 designing its own event API, but although there have been rumors, that seems to be relatively dormant now. Given that the first does not seem attractive from a compatibility standpoint (and just renaming doesn't seem sufficient justification for a new method), and the second is unlikely to happen, I think our best path forward is evolving addEventListener() further based on the design @RByers put in the DOM Standard.

@RByers

This comment has been minimized.

Copy link
Member Author

RByers commented Apr 7, 2016

Ok, not hearing any response from @jacobrossi or @smaug---- it sounds like nobody thinks we should be investing long-term in a replacement for addEventListener, so we need to assume that we're designing for the next 10+ years. Given the other use cases for EventListenerOptions, making sure developers can use them easily is definitely worth some effort / risk.

Obviously we're still divided on how to weigh ergonomics vs. forward compat. To summarize:

  • @jacobrossi and @smaug---- strongly prefer the 4th arg approach (forward compat)
  • @annevk and @foolip strongly prefer prefer the 3rd arg approach (ergonomics)
  • several others (@GROG, @tabatkins, @paulirish, @domenic, etc.) prefer 3rd arg but say they could live with 4th arg if necessary.
  • Others we've reached out to (including others from Mozilla) say they don't have a strong preference.

Chrome 51 is branching tomorrow. We were prepared to switch to the 4th arg approach this week if this discussion had gone differently, but we're now out of time unless we want to delay by another release (which, given our urgency, we'd need a strong justification for). Therefore we believe the time has come to ship 3rd arg and move on despite the disagreement. I'm sorry we couldn't come to a resolution where everyone was happy - I really do highly value all of your input and collaboration.

So that this long debate is not without some value, I'll commit to circling back here once usage has gotten above ~5% of page views in Chrome to see what we have learned about forwards compat risk. In particular, what fraction of the top sites are using EventListenerOptions with and without the correct feature detection (for my <1% prediction).

@RByers RByers closed this Apr 7, 2016

@annevk

This comment has been minimized.

Copy link
Collaborator

annevk commented Apr 8, 2016

We should probably just add another simple feature to EventListenerOptions so user agents not interested in passive can implement that instead and reduce their risk of eventual forward compat issues. I think once: true might be a good fit for that. Removes the event listener once it has been invoked.

@foolip

This comment has been minimized.

Copy link
Collaborator

foolip commented Apr 8, 2016

That would be a great feature to have, yeah.

@annevk

This comment has been minimized.

Copy link
Collaborator

annevk commented Apr 8, 2016

Since there might still be some lurking controversy I created a PR so folks can object: whatwg/dom#207.

@RByers

This comment has been minimized.

Copy link
Member Author

RByers commented Apr 8, 2016

I like the idea of the feature! But note we already have the "capture"
option if some UA wants ELO but not passive (like Chrome 49&50).

@annevk

This comment has been minimized.

Copy link
Collaborator

annevk commented Apr 8, 2016

I figured that might not be enough incentive to invest resources, but totally true.

@jacobrossi

This comment has been minimized.

Copy link

jacobrossi commented Apr 8, 2016

Ok, not hearing any response from @jacobrossi or @smaug----

I was travelling...

Chrome 51 is branching tomorrow. We were prepared to switch to the 4th arg approach this week if this discussion had gone differently, but we're now out of time

I want to go on record saying I think this is reckless. We just uncovered another issue with how ELO should behave and there isn't a second experimental implementation of this feature. The performance issues of Touch Events (and wheel) have existed for over 5 years -- it can't suddenly be this critical that days matter.

I figured that might not be enough incentive to invest resources, but totally true.

I don't think any browser has expressed an issue investing resources on this feature. At least for us, code on the web using ELO will be a major incentive for us for interop. Additionally, we want to have the passive listeners feature for performance. The issue is extended support browsers that we can't update no matter how much incentive the API presents for new browser versions.

@RByers

This comment has been minimized.

Copy link
Member Author

RByers commented Apr 8, 2016

Chrome 51 is branching tomorrow. We were prepared to switch to the 4th arg approach this week if this discussion had gone differently, but we're now out of time

I want to go on record saying I think this is reckless. We just uncovered another issue with how ELO should behave and there isn't a second experimental implementation of this feature. The performance issues of Touch Events (and wheel) have existed for over 5 years -- it can't suddenly be this critical that days matter.

Noted.

Days have added up to well over a year debating this API, and 7 weeks actively debating this point. There is rough consensus in the community that shipping this is a good thing to do, and we're well past the point of diminishing returns on further debate. Of course if something new and major comes up before Chrome 51 reaches stable (~May 31) then we can still pull it back. But the cost of slipping another release is substantial, so we'd need a correspondingly substantial potential benefit to do so.

@phistuck

This comment has been minimized.

Copy link

phistuck commented Apr 8, 2016

The issue is extended support browsers that we can't update no matter how much incentive the API presents for new browser versions.

Well... Internet Explorer 11 gets non security fixes on a regular basis, so for that matter - you can update, but you prefer not to update it with this kind of fixes (understandable, of course).

@jacobrossi

This comment has been minimized.

Copy link

jacobrossi commented Apr 8, 2016

you prefer not to update it with this kind of fixes (understandable, of course).

Not really a preference, but a customer promise to the users of these browsers. It defeats the purpose of their existence if you add features like this.

Of course if something new and major comes up before Chrome 51 reaches stable (~May 31) then we can still pull it back.

It would seem to me that #27 is new and major and should have been sorted out before ELO was ever shipped in a browser. It's exactly the kind of thing where a 2nd experimental implementation uncovers an assumption that the 1st implementation didn't consider into the spec. That's why we generally try to do it that way. :-(

EDIT: which is what puts us in this situation which is harmful for the web:

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.

@phistuck

This comment has been minimized.

Copy link

phistuck commented Apr 8, 2016

Not really a preference, but a customer promise to the users of these browsers. It defeats the purpose of their existence if you add features like this.

The feature to which I was referring here is not to treat an object as true (that would fix the forward compatibility issue). Supporting {capture: true} (or anything else) is a big feature that I would not expect you to backport, of course.

Just a thought.

@annevk

This comment has been minimized.

Copy link
Collaborator

annevk commented Apr 8, 2016

@jacobrossi I think you're overstating things. #27 is pretty minor given the conclusions in the end.

@smaug----

This comment has been minimized.

Copy link
Collaborator

smaug---- commented Apr 8, 2016

How is @jacobrossi overstating. We're effectively about to make changes to some very core DOM APIs, and realize even before shipping that some of the changes may not be stable yet. And still plan to ship. Doesn't sound too good.

@annevk

This comment has been minimized.

Copy link
Collaborator

annevk commented Apr 8, 2016

Because the unstable bit is pretty close to dead code in practice.

@RByers

This comment has been minimized.

Copy link
Member Author

RByers commented Apr 8, 2016

If you guys think a second implementation is critical to getting this right, and this API is critical to get right, why hasn't anyone else written a line of code for this yet despite the last 2+ years of Google pushing on this problem? Sorry, but I really don't have any sympathy for "we need two implementations before any one ships" when we've been the only ones investing engineering resources for years.

If you recall from our discussions at the time, I implemented the scroll-blocks-on proposed solution 18 months ago, and have since ripped the whole thing out in favor of this better design based on the API feedback we got. So this is not a case of recklessly shipping the first solution that came to us (despite substantial pressure to just ship scroll-blocks-on).

@phistuck

This comment has been minimized.

Copy link

phistuck commented Apr 8, 2016

If you guys think a second implementation is critical to getting this right, and this API is critical to get right, why hasn't anyone else written a line of code for this yet despite the last 2+ years of Google pushing on this problem?

If I recall correctly, the problem does not exist in pointer events (for touch) and pointer events are being pushed, which is an incentive not to tackle the TouchEvents problem. But I may be misremembering and this whole comment is moot.

@RByers

This comment has been minimized.

Copy link
Member Author

RByers commented Apr 8, 2016

If I recall correctly, the problem does not exist in pointer events (for touch) and pointer events are being pushed, which is an incentive not to tackle the TouchEvents problem. But I may be misremembering and this whole comment is moot.

Yes, that is correct (for the touch case, wheel is still an issue). I don't personally believe that we'll be able to get all the important touch event code on the web replaced with pointer events.

@FremyCompany

This comment has been minimized.

Copy link

FremyCompany commented Apr 8, 2016

Just for the record, I think the code needed to feature-detect this -- while clever and interesting -- is long, ugly, and very very very very unlikely to be understood my most web developers. There is absolutely no doubt this is going to cause issues in the future to older browsers, and as noted debugging those issues is going to be very very difficult, so most developers will likely give up on web compat at this point.

This change is adverse to the web platform. Some people have older devices which they cannot update to a more modern browser, and this change is going to break them in ways that are not necessary. Saving 4 chars when calling a function is clearly not worth it.

Both the 4th and 2nd argument approach are way better and I wonder why it hasn't been chosen as the path forwards already. By the way, shipping this feature is against the Blink policies which state that:

If a change has high interoperability risk but is expected to significantly move the web forward, Blink will sometimes welcome it after careful, publicly-explained consideration. In such cases, the implementer is expected to: ◦Take on an active commitment to shepherd the feature through the standards process, accepting the burden of possible API changes.

If people from both Mozilla and Microsoft came up with concerns about the feature and asked for updating the API, I am pretty confident the right move was to change the API.

@tabatkins

This comment has been minimized.

Copy link
Collaborator

tabatkins commented Apr 8, 2016

We've been over the 2-arg approach in #11. It's dead.

As far as we can tell, 3-arg is web-compatible. We'll revert if actual usage proves us wrong.

The "testing is too hard" argument is actually a fully-generic argument against ever adding any behavior-changing options to a dict argument - the same testing will be required before the author can be sure it's safe to use the new option. We can't accept this argument, as it would rule out vast swathes of API design space we've all intentionally angled ourselves toward. If we want to fix this, let's design something that'll work in a reliable way (like how @supports works without needing explicit support).

"Saving 4 chars is not worth it" is also a fully generic argument against ever fixing APIs with terrible ergonomics. It wins a lot, and that's fine, but it's still something to weigh, not something to be automatically accepted.

If people from both Mozilla and Microsoft came up with concerns about the feature and asked for updating the API, I am pretty confident the right move was to change the API.

The concern in question (#27) has nothing to do with 3- vs 4-arg; it's an issue to resolve about any new addition to addEventListener().

@RByers

This comment has been minimized.

Copy link
Member Author

RByers commented Apr 8, 2016

Just for the record, I think the code needed to feature-detect this -- while clever and interesting -- is long, ugly, and very very very very unlikely to be understood my most web developers. There is absolutely no doubt this is going to cause issues in the future to older browsers, and as noted debugging those issues is going to be very very difficult, so most developers will likely give up on web compat at this point.

If we want to fix this, let's design something that'll work in a reliable way (like how @supports works without needing explicit support).

Agreed having a better pattern for feature detection is worth some more thought. Filed #31.

@FremyCompany

This comment has been minimized.

Copy link

FremyCompany commented Apr 8, 2016

@tabatkins I disagree with you. You can use new dict entries like "capture" in any browser; if the browser does not understand it, it doesn't get the perf benefit, but that is not an issue.

In this case, you make older browsers misunderstand grossly the developer' intentions, by having them understand your dictionary as a Boolean, with the value "true" having a vastly different and often undesirable behavior that is not even a default in your dictionary. That is not web friendly at all.

To save a few chars for when you need a perf improvement in your browser, you will cause older browsers to break miserably by changing the semantics of the API under their feet. The trade-of balance is largely tipping towards "bad idea" in this case.

@tabatkins

This comment has been minimized.

Copy link
Collaborator

tabatkins commented Apr 8, 2016

@tabatkins I disagree with you. You can use new dict entries like "capture" in any browser; if the browser does not understand it, it doesn't get the perf benefit, but that is not an issue.

In this case, you make older browsers misunderstand grossly the developer' intentions, by having them understand your dictionary as a Boolean, with the value "true" having a vastly different and often undesirable behavior that is not even a default in your dictionary. That is not web friendly at all.

Incorrect - adding new arguments is only "safe" in the relatively rare cases where the new argument just turns on some browser optimizations or similar. "passive" happens to do that, but other arguments that are on the list to possibly add are definitely not like that - for example, limiting the phase you see the event in, or setting up "once-only" listeners. Most new dictionary args are exactly as dangerous to use naively as adding a new positional arg.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.