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

Is modifying the value of cancelable OK? #2

Closed RByers closed 8 years ago

RByers commented 9 years ago

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.

RByers commented 9 years ago

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 cancelable is that bad - not dissimilar to how currentTarget changes during dispatch.

smaug---- commented 9 years ago

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.

RByers commented 9 years ago

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:

This property is valuable for stable composition of handler behavior. It ensures that a mayCancel=false handler which accidentally triggers code that invokes preventDefault behaves consistently regardless of what other handlers happen to be installed for the event. As a result, mayCancel can reliably be adopted incrementally. One component being upgraded to leverage mayCancel=false doesn't risk exposing bugs in other components not necessarily under control of the same author.

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.

smaug---- commented 9 years ago

Sounds like overengineering to me.

If you really need to pass different kind of Event to certain listeners, then just clone the event, but set .cancelable to false, something hackish like: foo.addEventListener("foobarEvent", function (event) { var e = new Event(event.type, { ..., cancelable: false}); realListener(e); }, { mayCancel: false} )

RByers commented 9 years ago

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?

smaug---- commented 9 years ago

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.

RByers commented 9 years ago

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 capture option, mayCancel determines what phase the listeners runs at. Listeners run during one of the UNCANCELABLE phases will get an event with cancelable=false. Is that any better? This has dispatch ordering differences from my current proposal, but that might be OK.

And global reasoning should be totally fine. That is how preventDefault() works anyway.

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.

smaug---- commented 9 years ago

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.

RByers commented 9 years ago

So if you want per listener .cancelling, why not pass a new instance of Event to such listener?

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.

And that behavior can be implemented using script if actually needed.

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?

smaug---- commented 9 years ago

yes, I mean it wouldn't need to be part of the spec. Replacing addEventListener should be totally fine (at least as fine as any polyfill) in case someone actually needs it.

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 per listener as I thought.

RByers commented 9 years ago

yes, I mean it wouldn't need to be part of the spec. Replacing addEventListener should be totally fine (at least as fine as any polyfill) in case someone actually needs it.

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.

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"

But what's so bad about cancelability becoming a mutable property? Maybe we need to define an internal trulyCancelable variable that's immutable and redefine the exposed cancelable property in terms of "is currently cancelable" (implemented as trulyCancelable && currentListener.mayCancel).

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.

Yes, I agree. This is a good reason to prefer event cloning over a mutable cancelable. I was thinking this was unusual enough that it wasn't really worth worrying about. But maybe that's wrong.

But I need to sleep on with this stuff. Perhaps I'm not so totally against making .canceling per listener as I thought.

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

dtapuska commented 9 years ago

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) { window.firstEvent = event; }, { mayCancel: true}); node.addEventListener('foo', function (event) { // querying window.firstEvent should be the same as it was when first dispatched... }, { mayCancel: false});

foolip commented 9 years ago

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 https://github.com/RByers/EventListenerOptions/issues/2#issuecomment-119735282

Trying to call preventDefault() in could silently do nothing or throw an exception, and it doesn't really seem important what the value of cancelable is, or why does it matter?

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.

RByers commented 9 years ago

"please call this event listener after all of the normal ones, and at a time when canceling it has no effect".

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 stopPropagation), then it may be hard to get adoption. I believe ad networks are pretty cagey about any changes to their metrics (since it's used to compute billing, etc.). Ideally I think there'd be no change in event order at all. I can try to check with some potential customers though, maybe some change in handler invocation order would be tolerable.

Trying to call preventDefault() in could silently do nothing or throw an exception, and it doesn't really seem important what the value of cancelable is, or why does it matter?

The value of cancelable matters primarily because that's the mechanism by which preventDefault "silently does nothing" (that's how preventDefault is currently defined). Plus it's already the way that scripts can tell whether preventDefault will have any effect (eg. during uninteruptable scrolling, touchmove events should be marked cancelable=false).

Does the "delay the event to a time when it can't do any damage" model make sense?

The 'damage' I'm most worried about here is between multiple listeners installed with mayCancel=false. So delaying all of them to run after the normal handlers doesn't really help the fundamental issue here I don't think. But if we're going to ignore calls to preventDefault on the events delivered to these handlers, then it doesn't matter when the handlers are run - the events are effectively immutable (for the property we care about here).

It sounds like @smaug--- and @dtapuska both like the 'dispatch a copy of the event with cancelable=false' model. I think that works for me (though I'll probably have to do some measurements to see if any real-world cases could result in noticeably higher GC pressure - eg. for events firing at 60hz with dozens of mayCancel=false listeners). Do you see any problems with that model @foolip?

RByers commented 9 years ago

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 touchstart handlers active for some target element:

  1. The active handler A - may (in some non-obvious code path) occasionally need to call preventDefault
  2. The passive handler P - never calls preventDefault. This comes from a library like Google Analytics which is served (and authored) separately from the main app.

Now we ship mayCancel support and advertise how it can help with scroll performance. If P updates to use mayCancel=false first then everything is great - it'll have no behavior change on this particular site because A still requires a cancelable handler.

But what if if the author of A sees all the evangelism and attempts to use mayCancel=false? If we do nothing special, then that change works fine (there's a mayCancel=true handler on the page still, so the preventDefault call they didn't notice still works, there aren't even any warnings in devTools - not that they'd be likely to get noticed anyway). Now later P decides to upgrade to use mayCancel=false. As soon as they do this, the behavior on this site changes - some scenario that's not supposed to scroll suddenly does (maybe users complain that panning a map doesn't work anymore because the page moves under their finger).

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 mayCancel=false.

So the only "order" that's relevant here is the timing of when different scripts update to take advantage of mayCancel=false. It makes no difference if these are capturing, target or bubbling handlers etc. The principle is that we want developers to be able to reason locally about the change they make - by using mayCancel=false for their handler, it affects the observable behavior for their handler only. Of course performance is still a global-reasoning problem, but that's unavoidable (and typical for performance).

@foolip does that help at all?

annevk commented 9 years ago

Wait, cancelable is an indication of the code that dispatches the event. And therefore a property of the event that should not be changed.

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.

foolip commented 9 years ago

Ideally I think there'd be no change in event order at all.

Actually, handler invocation order really has nothing to do with the fundamental issue here.

@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 target.dispatchEvent(event) call cannot have solved the problem, because then you will have had to make the same problematic thread/process jump.

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 nonCancelingListener is called before potentiallyCancelingListener here, then no problem can have been solved, because however nice nonCancelingListener is, potentiallyCancelingListener will be able to mess it up.

How can the order of event listeners be maintained, unless the intention is that all listeners need to use mayCancel before mayCancel on any listener has any effect? That seems weird, making it hard to reason about your code because its timing depends on other event listeners.

foolip commented 9 years ago

A mock proposal in the style of MutationObserver, where the callback order does change, and doesn't depend on other event listeners:

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 addEventListener, but the callback is never inside dispatchEvent() but instead at a later time, possibly at the next animation frame, or something fluffy like "as soon as possible after the real events". The ability to collate events isn't really central, but seems natural if there are multiple events pending.

smaug---- commented 9 years ago

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

foolip commented 9 years ago

please no more pressure to animation frame

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.

annevk commented 9 years ago

Is that quantified somehow or just a suspicion?

smaug---- commented 9 years ago

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.

foolip commented 9 years ago

I'm afraid to derail the discussion with the EventObserver proposal, but the thing that we need to figure out is when these callbacks are invoked, regardless of the API shape. I've tinkered a bit with the DOM spec, and think that a step before the return in dispatch might be what's needed. That step would queue a microtask to invoke the listeners that are known at that point. Not sure if that should be a new event phase or not.

annevk commented 9 years ago

"please call this event listener after all of the normal ones, and at a time when canceling it has no effect"

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?

annevk commented 9 years ago

If we are planning such drastic changes this will really take a lot longer to design I think.

foolip commented 9 years ago

To be clear, in https://github.com/RByers/EventListenerOptions/issues/2#issuecomment-120667412 I'm trying to figure out what the changes to the processing model for { mayCancel: false } would be, never mind EventObserver for now.

If the listeners aren't invoked after dispatchEvent() has returned, then what is it that makes preventDefault() have no effect, especially in cases where the different kinds of event listeners are interleaved?

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.

foolip commented 9 years ago

I said:

It seems to me that any solution where a new kind of never-canceling listener is still run in this same target.dispatchEvent(event) call cannot have solved the problem, because then you will have had to make the same problematic thread/process jump.

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.

annevk commented 9 years ago

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.

foolip commented 9 years ago

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 cancelable during dispatch sounds very nice.

annevk commented 9 years ago

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, preventDefault() becomes a no-op. This does require all listeners to opt-in, but that is the only thing that makes sense I think.

foolip commented 9 years ago

Well, how global? If the non-canceling event listener itself fires a cancelable event (e.g. via click()), it'd be weird if that event couldn't be canceled.

annevk commented 9 years ago

Ah yeah, that would be weird. So instead we'd set some flag on the event object and then unset it after invocation.

foolip commented 9 years ago

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 preventDefault() didn't do anything, or it could throw an exception if we'd like that.)

RByers commented 9 years ago

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.

foolip commented 9 years ago

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.

RByers commented 9 years ago

Exactly. I opened #6 to discuss making the implications (or lack thereof) on timing explicit.

RByers commented 8 years ago

Let me attempt to summarize the conclusion from this long thread:

I'll work on writing this up as part of #19

RByers commented 8 years ago

Done