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 preventDefault being silently ignored on uncancelable events too error-prone? #3

Closed RByers closed 9 years ago

RByers commented 9 years ago

Some have argued that it's too error prone to have preventDefault be silently ignored on uncancelable events. In Chrome I've added a devtools warning to try to help spot these. But we could consider throwing (for this new special case). Personally I think it's more webby to just stick with the currently specified behavior.

duanyao commented 9 years ago

How about making defaultPrevented read-write and let it supercede preventDefault()? We can throw on modifying defaultPrevented for uncancelable events.

RByers commented 9 years ago

I don't understand - how would that help? I think it's intentional that once an event is cancelled (by preventDefault) it can never be un-canceled. A read-write defaultPrevented would be bad in that regard.

duanyao commented 9 years ago

For any event, changing defaultPrevented from true to false won't be allowed and may throw.

RByers commented 9 years ago

Oh, so you're suggesting this is a new API to indicate cancelation (so free from compat concerns with existing code) and as such we're free to be stricter - throwing on invalid use? eg. throw unless setting to true and cancelable is true.

I agree, that's an option.

duanyao commented 9 years ago

Yes, exactly.

AshleyScirra commented 9 years ago

I think if mayCancel defaults to false, preventDefault() should be silently ignored (so in the default case you don't get an exception for making a mistake/ignoring the default). If mayCancel defaults to true, then if it is set to false the developer has explicitly indicated they will not call preventDefault(), and thus calling preventDefault() should throw an exception since it contradicts the intent. As a developer I don't like things silently not happening, so I would prefer that mayCancel defaults to true and preventDefault() throws an exception if called when mayCancel is false. (Defaulting mayCancel to true is also backwards compatible.)

RByers commented 9 years ago

Re-opened the default value debate in #17. We'll come back to this once that's resolved.

RByers commented 9 years ago

Now that we've inverted the sense to 'passive' which is consistently false by default (#17), perhaps throwing (as @AshleyScirra suggests) is a good idea?

@annevk @smaug---- @domenic WDYT?

annevk commented 9 years ago

If we do that, passive would become a distinct flag from cancelable. And we'd likely need to expose it on Event.

smaug---- commented 9 years ago

We certainly can't make preventDefault() throwing. That wouldn't be backwards compatible and I would be surprised if that didn't break tons of pages. Or is this bug only about those event listeners which have been registered with mayCancel: false (or passive: true or whatever the feature will look like)?

annevk commented 9 years ago

If all relevant event listeners have passive set to true, an event would be dispatched whose passive flag would be set. And that would then make preventDefault() throw. So it would be backwards compatible.

foolip commented 9 years ago

It's been a while, but wasn't the model we arrived at that each event listener would have a passive flag, and that while invoking that event listener, the event would have an internal flag causing preventDefault() to do nothing, or perhaps throw? I don't recognize this "if all event listeners are passive, then ..." model.

smaug---- commented 9 years ago

a bit odd if preventDefault() throwing in an event listener depends on the other event listeners.

RByers commented 9 years ago

Right, my suggestion here was only that preventDefault could throw if invoked on a listener with the "passive flag" set. Doesn't affect any existing code, doesn't make the behavior depend on other listeners.

However, @annevk's point is valid - to avoid triggering a throw, script should probably have a way to ask whether it's currently in a passive listener.

Also since we're never going to throw for the existing scenario of calling preventDefault on a cancelable=true event, it would seem inconsistent for the passive listener case to be different.

So I suggest we leave this as is - silently ignore with advice to UAs to provide a debugging aid (like a console warning) for any case where preventDefault doesn't cause defaultPrevented to be set.

ojanvafai commented 9 years ago

Is exposing passive on Event bad or hard?

If we fail silently, I worry about developers using libraries. They don't know what the library does under the hood and will expect it to work right. It means that library authors won't have a way of ensuring their code works correctly and library users won't have a way of knowing they didn't screw up without exhaustive manual testing.

I think this probably isn't a problem where the library is registering the event handler itself and calling it's own function, but it could be a problem in cases where the user is providing the callback to the library or where the user is registering the handler to a library callback.

Console logging is valuable, but doesn't get some valuable things:

  1. Logging errors in the wild to your server
  2. The breakages will often be subtle
RByers commented 9 years ago

No, not hard to expose. The main reason not to is the philosophy that the listener state should not modify properties of the event itself. See discussion in #2.

If someone was concerned about catching this in the wild, they could actually build their own detection. Just hook Event.preventDefault and look for cases where Event.cancelable was false but he defaultPrevented bit didn't change from false to true after the preventDefault call. I guess the issue is that it's unlikely people will do this, but it's not uncommon for sites to have field monitoring for unhandled exceptions.

But, as much as I prefer fail-hard to fail-subtly myself, there's a fair amount of precedent here (eg. I don't hear many complaints about developers trying to cancel uncancelable TouchEvents - in fact they'd probably complain more if we didn't silently ignore their buggy code) . And in practice I think it's pretty unlikely that an accidentally ignored preventDefault will be catastrophic to the page experience.

ojanvafai commented 9 years ago

That's fair. I'm OK with that.

annevk commented 9 years ago

Just to recap, I'm okay with not throwing. Especially for methods that did not historically throw that might be better.

I do think we should change cancelable to false when a specification decides to dispatch a different type of event based observing solely passive listeners. It's observable either way, so better be honest.

RByers commented 9 years ago

Agreed

I do think we should change cancelable to false when a specification decides to dispatch a different type of event based observing solely passive listeners.

Minor nit: we should think of this not as observing the passive listeners, but restricting observation to only the non-passive ones (i.e. "all listeners passive" should be identical to "no listeners at all". Passive is the way to opt-out of observability).