Closed RByers closed 8 years ago
I would add I wasn't convinced by the discussion above that the handleEvent
approach can't work. I don't see how the mutability of handleEvent
is relevant. It doesn't say anything about passive
needing to be mutable. All that changes is at what point in the processing steps you read and store the passive
value (is it at registration or dispatch). handleEvent
can keep having the odd behavior while the new feature doesn't. As pointed out above, the this
difference is also minor and has at least two options (arrows or bind) for the few times it's an issue.
@jacobrossi, @smaug----, I get that this is a new situation and that we can't say anything for certain, but I'm still curious how you see the risk over time, where "peak accidental capturing" is and what choices, omissions or misunderstanding by web developers is the main driver of that risk. I assume you have some suspicion, however speculative.
As for the handleEvent
approach, it's definitely possible to spec and implement, but it'd have to be entirely custom behavior, as the second argument would be neither a dictionary nor a callback interface. It would also leave NodeFilter
as the web platform's only remaining callback interface, and the code for that bit of crazy legacy wouldn't be shared with addEventListener
's new second argument. (I recently measured to see if NodeFilter
could be made a callback function instead, but no can do.)
I think we can at least agree that the forwards compat risk will be non-zero.
Absolutely, and I'm now convinced that it's higher than the trivial levels I originally thought from gut instinct (my 0.015% of page views guesstimate is certainly non-trivial).
It would be possible to write a browser extension that wraps AEL and flips the third argument to true. Browsing around with this would give you more data on how much user-facing breakage would occur.
Is your hunch that this is likely to be a lot higher than my guesstimate (eg. much closer to 100% of typical inputs being affected than 20%)? If this turned out to be, say, 80% then I'd certainly be more concerned about the forward compat risk.
I just did a very quick unscientific experiment. I injected this code before load:
var oldael = EventTarget.prototype.addEventListener;
EventTarget.prototype.addEventListener = function(type, handler, capture) {
console.log('addEventListener', type);
oldael.call(this, type, handler, true);
};
Then I browsed around GMail and Facebook for a few minutes using mouse and keyboard. I didn't notice a single obvious issue. Certainly not a very scientific test, but enough to tell me that something as high as 80% is pretty unlikely. If you want to collect some more scientific data than that, I'd love to hear the results!
I'm still curious how you see the risk over time, where "peak accidental capturing" is and what choices, omissions or misunderstanding by web developers is the main driver of that risk. I assume you have some suspicion, however speculative.
No amount of data can predict that future, unfortunately. It is a very reasonable expectation that quantity "# 1" above will be significant for many years. Mozilla, Microsoft, and others have millions of customers with the valid constraints of scenarios like mission critical enterprise applications and complex IT rollout management that necessitate a slower rollout of latest browser versions.
As for the handleEvent approach, it's definitely possible to spec and implement, but it'd have to be entirely custom behavior, as the second argument would be neither a dictionary nor a callback interface.
Probably the most complex thing here is writing the spec text. The implementation is trivial. The cost of spec text authoring shouldn't contribute to our decision here.
Is your hunch that this is likely to be a lot higher than my guesstimate (eg. much closer to 100% of typical inputs being affected than 20%)?
I honestly haven't a clue. 20% sounds as good of a guess as any other without data. :-)
A more interesting scenario that can't easily be tested is the use of a touch event library. If the library updates to opt into passive listening but the hosting page does not, then the relative order of the library's listeners to the page's could be reversed. You can't test this scenario with my suggested methodology. After thinking further, my methodology could often still preserve the relative ordering of handlers as it assumes all listeners become capture listeners.
The implementation is trivial. The cost of spec text authoring shouldn't contribute to our decision here.
I agree we shouldn't give the spec text complexity much weight in this debate. But I don't think the implementation is trivial. At least in blink I believe this would require some pretty weird custom bindings code, and we're actively trying to remove custom bindings for maintenance and security reasons. And ensuring all that custom code behaved exactly the same as implementations in other browsers would require some non-trivial testing.
It seems like a tiny servicing patch that accepts a dictionary in the 3rd argument position would be a lot simpler implementation wise (and probably simpler than most security fixes which are presumably still getting into ESR browsers, right?). Not that I'm proposing that as a real solution (eg. we have plenty of Chrome Android users who never connect to Wi-Fi so generally don't get Chrome updates at all, security or otherwise!).
I would suspect you must have some sort of specialized binding logic here already given the definition of how handleEvent must work. At least in our codebase, this is just augmenting that. But I trust you understand the Blink event dispatch architecture far better than I.
Our servicing bar for feature-locked browsers isn't based on the size of the code change, and for various reasons as you point out, isn't effective.
I would suspect you must have some sort of specialized binding logic
Thinking further, maybe what you're proposing here is not some weird dictionary with special semantics for compat, but just to use/extend the existing callback interface pattern as discussed in #11? @paulirish is also telling me offline he likes that option. Let's re-open #11 to discuss that option in more detail.
Ah yep. From the web devs perspective this is basically the same thing. That's how I was approaching it.
In EdgeHTML, this is just a few lines in the existing fUseHandleEventCallback
branch of registration to save the mayCancel
bit to the listeners table. Then a few lines of code in the preventDefault
path to ignore the calls if the bit was set for the listener.
I think I'm up to date on this discussion, so I'll wade in.
el.addEventListener(evt, fn, { passive: true })
It's true that to use this for real, developers must feature detect it, in order to not accidentally kick some browsers's listeners into capturing phrase. I was originally a bit hesitant about this, as the detect looked quite hairy, but as I rewrote it and discussed with the Modernizr team, I feel comfortable with the detect and think developers would be okay with using it.
I'm also confident that between the documentation (MDN, etc) and general guidance, developers would reliably use the feature detect whenever they adopt the feature. That said, requiring the detect because of capture phase, which very very few developers understand, let alone use, is kind of a bummer.
el.addEventListener(evt, { handleEvent: fn, passive: true })
I like the look of this, and TBH handleEvent opens up some opportunities (e.g. easily removing handlers) that most developers are unaware of. But generally this works very well as no detect is required (assuming you're OK with breaking in <= IE8).
this
context will now be different, but most developers need to rebind to something else already; so the change is nearly moot.
this
. If you've got no problems in your browser, all other browsers will be fine. (And if you serve <=IE8, it'll throw. And that's fixable with a small polyfill) So, I think the 2nd arg has better ergonomics for both new old browsers. So I'll join the fun over in #11. :)
We've discussed this debate some more within Google, and although we still prefer the existing (3rd arg) approach, we agree we could live with the 4th arg one. We respect that Microsoft and Mozilla have had a different experience with web compat than we have, and it's clear that they feel MUCH more strongly about the need to address this concern than we do about the costs. So @jacobrossi and @smaug---- , if you still feel you really can't live with the 3rd arg design, then as a sign of goodwill we (Google) are willing to concede this point. I also spoke with @grorg offline and he agreed ("I think we should just accept it and move on"). Note that our position on forwards compatibility generally has not changed - we are not willing to limit platform evolution out of fear for the behavior of older browsers.
Concretely this change would mean that instead of telling developers that (for the foreseeable future) they must use code like this:
var supportsCaptureOption = false;
addEventListener("test", null, { get capture() { supportsCaptureOption = true; } });
function myAddEventListener(target, type, handler, options) {
var optionsOrCapture = options;
if (!supportsCaptureOption)
optionsOrCapture = options.capture;
target.addEventListener(type, handler, optionsOrCapture);
}
...
myAddEventListener(window, "touchstart", handler, {passive:true});
They should instead write:
function myAddEventListener(target, type, handler, options) {
target.addEventListener(type, handler, options.capture, options);
}
...
myAddEventListener(window, "touchstart", handler, {passive:true});
Or if they prefer for one-off uses, instead of:
var supportsPassiveOption = false;
addEventListener("test", null, { get passive() { supportsPassiveOption = true; } });
window.addEventListener("touchstart", handler, supportsPassiveOption ? {passive:true} : false);
Just do the ugly but trivial thing:
window.addEventLilstener("touchstart", handler, false, {passive:true});
The extra arg is clearly a wart, but it really doesn't seem THAT bad to us compared to explicit feature detection.
There is, however, one extra complication here. 0.5% of the top 10k websites already pass a boolean in the 4th arg position. So for web compat, we need to define the API to permit (but ignore) boolean values in the 4th arg position:
void addEventListener(DOMString type, EventListener? callback, optional boolean capture = false,
optional (EventListenerOptions or boolean) options);
dictionary EventListenerOptions {
boolean passive;
};
This is ugly, but only for the spec / implementors (shouldn't really affect developers), so I can accept it and move on.
@annevk @foolip WDYT?
I haven't seen any convincing attributes why 3rd param approach should be taken. We're talking about very old very core API here, so making breaking changing shouldn't be done lightly.
And in Gecko we'd anyway need to define the 4th param to be (EventListenerOptions or boolean) so that we can keep supporting the "allowUntrustedEvents" stuff.
A good thing is that unlike 3rd param as boolean, 4th param in Gecko defaults to true for web content. So passing {} as 4th param doesn't change the behavior.
I think the 4th arg is a good compromise, and, given the exhaustive discussion we've had on alternatives, it feels like a reasonable approach. I appreciate your willingness to consider these concerns!
I still disagree with the four parameter solution for extensibility of addEventListener(). I agree that the transition path for three parameters might be a little bumpy, but we're here designing the fundamentals because we believe the web to exist for several more decades, if not much longer. That at least goes for the WHATWG community which as far back as 2003-2004 revived interest in the foundations and how they should function, and I believe that goes for all of you too.
The future is longer than the past. We should take a little short term pain to gain long term improved developer ergonomics. If we are to ask other, e.g., @dherman or @domenic from TC39, I'm sure they would agree that developer ergonomics is of vital essence to the platform. It's not something to just brush away.
Also, there’s been a number of requests from developers which we can easily add once this is deployed:
These features will be easier to add if developers build the experience necessary to detect supported dictionary members. And these features will be much easier to get adopted if the API is developer-friendly.
(I'm also not sure why I'm not counted as representing Mozilla or why Mozilla is even listed as an entity in this discussion. Mozillians speak for themselves except for very rare circumstances. So given that Microsoft and Olli are opposed and everyone else in favor, I think there's still rough consensus for the current approach in the DOM Standard.)
To point out the obvious, having EventListenerOptions or boolean
where the boolean does nothing is a pretty bad API.
And in Gecko we'd anyway need to define the 4th param to be (EventListenerOptions or boolean) so that we can keep supporting the "allowUntrustedEvents" stuff.
@smaug----, do you mean to say that you'd keep this behavior even after you implement EventListenerOptions? The spec will have to say that the boolean does nothing, and so we wouldn't have interop if you keep it. Unless you're fairly certain you can remove it from Gecko, we might be stuck in that state for a long time.
do you mean to say that you'd keep this behavior even after you implement EventListenerOptions?
yes. No plans to remove the 4th param handling before there is some telemetry data about its usage (the interesting data is passing 'false' in web pages.)
At least EventListenerOptions or boolean
as 4th param would be a tad less ugly than as 3rd param, since it doesn't make the API crazy, like the spec is now. passing {} as 3rd means non-capturing, but passing !!{} means capturing.
(I'm starting to wonder if we shouldn't touch .addEventListener at all, but do something else to achieve the 'passive' handling. No proposals yet though.)
@annevk I think the main difference between our arguments is lifespan. I'm guessing feature detection would be necessary in most cases for 3-5 years (even based just on future usage of IE 11). Based on our previous discussions, it sounded like we were likely to define a new API as the replacement for addEventListener
in roughly that timeframe (to address a number of different issues). It now sounds that you think that's unlikely, is that true?
If we're likely to stick with addEventListener
as the primary event registration mechanism for the next decade or more then I agree with you that ergonomics outweigh forward compat. But if we're likely to switch anyway, then I think the medium term forward compat concerns dominate. So let's do a little poll to try to judge this:
@smaug--- / @jacobrossi: If we go with the 4th arg approach, are you guys willing to commit to investing some resources over the next couple years to work with Anne on defining a possible replacement for addEventListener
that solves this, and other ergonomics issues? Obviously we can't commit to ship something without knowing what it is, but we can agree the developer ergonomics problem is worthy of our investment and so commit the resources necessary to participate in design. I can commit to that for blink.
@annevk / @foolip / @domenic: If we go with the 3rd arg approach, do you feel that the overall addEventListener
ergonomics are acceptable enough that we can avoid investing in trying to get consensus on a replacement API for at least the next ~5 years? Obviously we may learn something new that changes the equation here, but can we say that based on what we know now we'll plan for extending via the EventListenerOptions
mechanism rather than looking for participation on a rename / more radical redesign? I can also commit to this path for blink.
I suspect that any kind of full replacement would be driven by improvements in the JavaScript language and those tend to take a long time. And the first such improvement that is on the horizon, observables, might well be made to work with the existing event system. So I was personally quite happy with the three parameter solution we found since it allows for extensions to the API to be made in the nearer future while slightly improving the ergonomics (over the long term).
I haven't been very involved with the brainstorming around an addEventListener
replacement, but if nobody knows of a feature request where a single event target and single event type is going to be insufficient, I suppose we'll have just have addEventListener
for the foreseeable future?
@annevk, can you elaborate on "A way to list a listener override the bubbling behavior of an event" and how it'd work with EventListenerOptions?
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.
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?
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.
@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.
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:
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).
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.
That would be a great feature to have, yeah.
Since there might still be some lurking controversy I created a PR so folks can object: https://github.com/whatwg/dom/pull/207.
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).
I figured that might not be enough incentive to invest resources, but totally true.
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.
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.
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).
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.
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.
@jacobrossi I think you're overstating things. #27 is pretty minor given the conclusions in the end.
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.
Because the unstable bit is pretty close to dead code in practice.
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
).
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.
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.
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.
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().
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.
@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 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.
Is the requirement to use feature detection or a polyfill OK?
Olli Pettay suggests:
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