-
Notifications
You must be signed in to change notification settings - Fork 139
Is modifying the value of cancelable OK? #2
Comments
Olli Pettay has argued against this, saying the API should be just a hint - not affecting anything else. What do others think? Personally I don't think changing |
In particular, clarify the value of the behavior of Event.cancelable (Issue #2).
I could just repeat myself. Event.cancelable shouldn't change. The flag passed to addEventListener should be just a hint, and rest of the API should work consistently within every listener call. |
@smaug---- Thanks, I understand that opinion. But what about the arguments I list in the note for mayCancel:
Basically, I'm worried that if we treat this as just a "hint" it'll ultimately be impossible for large sites/framework to incrementally and reliably upgrade to this feature because there's no isolation between components. |
Sounds like overengineering to me. If you really need to pass different kind of Event to certain listeners, then |
But the composition issue can't be resolved by the one component looking to do the right thing. I.e. in order to convince the google ads team to set this bit on their touch handlers, I probably need to show why setting this bit isn't going to change behavior of the millions of pages their script is loaded into. If their change can effect the behavior of other handlers on the page (even if those are technically buggy), then they'll be much more resistant to make the change. It doesn't help if they add code like you suggest to their handler (they know their handler is following the rules). Also software engineering is all about enabling developers to reason locally. I worry about any API that requires global reasoning "if ALL handlers are ... then ..." as global reasoning is generally impossible in any sufficiently large real-world application that is a mashup of components from various places. Maybe I'm being overly paranoid though. @esprehn, @jacobrossi what's your opinion? |
Now I'm lost what you want with this bug. You seem to want .cancelable to be per listener (which makes no sense, since .cancelable is a property of an Event, and we aren't changing Event API here), but then you say no, you don't want that after all since ... something about composition. |
I do want cancelable to be set per listener. I.e. when a given listener promises not to cancel an event, then calls to preventDefault on the event will be completely ignored for the duration of that listener. I.e. they can't promise not to cancel and then trigger some behavior change by cancelling. If they could then whether or not their call to preventDefault has any effect would depend on what all the other (well-behaved) listeners for that event happened to do. Maybe another way to think about this is as the addition of 3 new phases to the event model. Before the CAPTURE phase there's a UNCANCELABLE-CAPTURE phase, before AT-TARGET there's UNCANCELABLE-AT-TARGET and before BUBBLING there UNCANCELABLE-BUBBLING. Then like the
This is why scripts that are designed to be used on any site to passively monitor (but not change) behavior (like google analytics) need to be careful to avoid any global actions like preventDefault (they don't have their own UI so can't ever preventDefault an event without risking breaking something else on the page). There is a subset of things passive scripts should be safe to do without having to reason about all the other scripts running on the page. Adding a simple event listener (that doesn't cancel it's events) one of those things that should be completely passive - not changing the behavior of other code on the page. |
So if you want per listener .cancelling, why not pass a new instance of Event to such listener? And that behavior can be implemented using script if actually needed. |
Yes, we might be able to define it this way. It would have other observable side effects though. Eg. today a handler can stash additional properties onto an Event object and expect other handlers to see those properties. Or JS may create an event object with additional properties and expect all handlers to see those. These 'mayCancel=false' handlers would end up being pretty special in that they'd get a clone of the DOM Event instead of the actual Event. Maybe that would be ok? That design would fit with the idea that these are completely passive handlers which can't mutate the event in any way.
You mean it wouldn't need to be part of the spec? How would one script implement this in a way that affects all scripts on the page? It's of course possible to do this by hooking addEventListener, but that's generally frowned upon (at least outside the context of polyfills) right? |
yes, I mean it wouldn't need to be part of the spec. Replacing addEventListener should be totally fine It is that .cancelable is a property of the Event. changing .cancelable per listener feels like a lie: "hey I'm actually cancelable, but not telling that to you" Also it becomes rather hairy if some listener stores a pointer to the event and .cancelling has value X at that point, and later, say some function call inside some other listener uses the same event and now the value of .canceling is Y. But I need to sleep on with this stuff. Perhaps I'm not so totally against making .canceling |
But if it's part of the spec, then browser engines would implement the copying automatically, right? No need for any extra JS on browsers that support the spec. Of course we'd produce a polyfill developers could choose to use on browsers with no support for EventListenerOptions.
But what's so bad about cancelability becoming a mutable property? Maybe we need to define an internal
Yes, I agree. This is a good reason to prefer event cloning over a mutable
Ok, cool - thanks a ton for the debate here, it's very helpful! I'm personally OK with the 'send an uncancelable clone' option (although I need to figure out exactly what the implications of event cloning are for custom event types - how exactly do we specify the behavior of cloning?). I could possibly also be convinced that this is all unnecessary and your 'just a hint' approach is good enough (it's certainly simpler). I'd want to get some more input from other folks though (I'm hoping I can get some Google Analytics folks to weigh in here). |
I agree with @smaug---- that the event shouldn't mutate; but we should send a clone of it so if somebody holds a reference we are ok. Roughly... node.addEventListener('foo', function (event) { |
After reading through this thread I'm seeing the new API as "please call this event listener after all of the normal ones, and at a time when canceling it has no effect". So perhaps one or three new event phases that all come after the existing ones, but not interleaved as in #2 (comment) Trying to call Does the "delay the event to a time when it can't do any damage" model make sense? We already have events that are synced with animation frames, so if it were possible to listen to events but ask for them to be collected and delivered as a batch at the next animation frame, that might have some of the properties we're looking for. |
Maybe. I'm a little worried about any change in semantics we introduce. Eg. if ad tracking scripts will occasionally see fewer events because their 'capturing' handler no longer runs before other handlers on the page (which may invoke
The value of
The 'damage' I'm most worried about here is between multiple listeners installed with It sounds like @smaug--- and @dtapuska both like the 'dispatch a copy of the event with |
Actually, handler invocation order really has nothing to do with the fundamental issue here. Let me try a more concrete example. Imagine an app with two
Now we ship But what if if the author of A sees all the evangelism and attempts to use So in rolling out this change, the author of P (who did everything right) gets complaints that they broke sites and so they need to revert their change (possibly forever). And we're stuck never getting the most critical scripts to opt-in to So the only "order" that's relevant here is the timing of when different scripts update to take advantage of @foolip does that help at all? |
Wait, That you observe the event in a way where you cannot cancel it, does not change the fact that the event is cancelable if you add some other listener. If we are in fact talking about changing dispatch semantics, we need to have a longer chat about what that means and whether doing that through listeners is the right way. |
@RByers, despite your very helpful explanation, I'm afraid I'm missing something fundamental, because the time and thus order of event delivery seems very central to me. This is the problem as I understand it in the browser, in pseudo-C++: void handleNativeEventWhichMayScroll(NativeEvent nativeEvent) {
Point point = nativeEvent.point();
if (havePotentiallyCancelingEventListenersAtPoint(point)) {
// thread/process jump to main thread here, which may be busy right now
Event event = Event::fromNative(nativeEvent);
EventTarget target = event.target();
bool wasCanceled = !target.dispatchEvent(event);
// thread/process jump back here
if (!wasCanceled) {
scroll();
}
} else {
scroll();
}
} It seems to me that any solution where a new kind of never-canceling listener is still run in this same To state the same from the point of view of JavaScript: function nonCancelingListener(event) { console.log(event); }
function potentiallyCancelingListener(event) { if (something) event.preventDefault(); }
document.body.addEventListener('touchmove', nonCancelingListener, { mayCancel: false });
document.body.addEventListener('touchmove', potentiallyCancelingListener, { mayCancel: true }); If How can the order of event listeners be maintained, unless the intention is that all listeners need to use |
A mock proposal in the style of callback interface EventObserverCallback {
void handleEvents(sequence<Event> events);
};
[Constructor(EventObserverCallback callback)]
interface EventObserver {
void observe(EventTarget target, EventObserverInit options);
// maybe disconnect() and takeEvents() too
};
dictionary EventObserverInit {
required DOMString type; // e.g. "touchstart"
boolean capture = false;
}; It would be like |
(random comment related to "possibly at the next animation frame" , please no more pressure to animation frame. It is becoming an issue that browsers are idle most of the time, but then when animation frame ticks, tons of different callbacks need to run and that may slow down graphic updates.) |
Good point, I suppose it's better to deliver events as soon as possible, and the scripts can request or wait for an animation frame only when that makes sense. |
Is that quantified somehow or just a suspicion? |
If that is a question about animation frame usage, then yes, I tried to make Gecko to follow the spec regarding resize event handling, but that caused too much pressure around animation frame tick and ended up regressing painting performance. The patch got backed out, and now trying to figure out how to change the spec. |
I'm afraid to derail the discussion with the |
I don't understand why we'd want to do this. That would drastically complicate the dispatch model. Why can't we require that all listeners opt-in? |
If we are planning such drastic changes this will really take a lot longer to design I think. |
To be clear, in #2 (comment) I'm trying to figure out what the changes to the processing model for If the listeners aren't invoked after The only other thing that comes to mind is something around the last step of invoke, to restore the canceled flag to its value before running the listener. |
I said:
I now realize this isn't true. The important thing is that one doesn't have to wait for the result of the event dispatch in the main thread, but this doesn't imply that the order of event listeners in the main thread has to change. |
I'm still having a hard time understanding what you're saying. This is how events work today: function dispatcher() {
target.dispatchEvent(ev) // listeners run
if(!ev.defaultPrevented) {
defaultAction()
}
} I don't think we want to change that. |
Sorry, I'm rambling, trying to understand what the processing model should actually be. We still need to change something about how events work today, after all, and I don't think cloning the event or changing |
What makes the most sense to me is that we'd change https://dom.spec.whatwg.org/#concept-event-listener-invoke by placing a guard (some kind of "global" flag) around step 6 if this "mayCancel" thing is set. While the guard is set, |
Well, how global? If the non-canceling event listener itself fires a cancelable event (e.g. via |
Ah yeah, that would be weird. So instead we'd set some flag on the event object and then unset it after invocation. |
Yeah, something like the dispatch flag. (That'd be like restoring the canceled flag after calling the callback like I speculated above, except one could actually tell that |
Yep, something like this sounds right to me. Thanks! @foolip, I think the main difference in our thinking here is that I'm only trying to avoid blocking scroll on JS when there are NO handlers that may block that scroll. Sounds like you'd like to also get scroll going after running the blocking handlers (but before the non-blocking ones), right? IMHO, once we have to block on any handler, we've taken most of the perf hit and there's not much we can do. The problems in practice tend not to be about slow handlers themselves, but other long running tasks on the main thread. |
It doesn't make sense if unpacked, but I thought that the events would reach the main thread at different times depending on the type of handler, that the non-blocking case would be "asynchronous" and thus arrive "later", which would be a complication. But the only difference is that one doesn't need to wait for the result in one case, so it's all about providing that information. |
Exactly. I opened #6 to discuss making the implications (or lack thereof) on timing explicit. |
Let me attempt to summarize the conclusion from this long thread:
I'll work on writing this up as part of #19 |
It's a little weird to say that 'Event.cancelable' changes value in the context of a particular handler. The alternative of an event being uncancelable only when ALL handlers have asked for that seems worse though. I guess we could describe this as the handler getting a mutated copy of the event? But that would be annoying (possibly expensive) to implement and probably impossible to polyfill.
The text was updated successfully, but these errors were encountered: