Skip to content
This repository has been archived by the owner on Mar 19, 2018. It is now read-only.

Is preventDefault being silently ignored on uncancelable events too error-prone? #3

Closed
RByers opened this issue Jul 3, 2015 · 19 comments

Comments

@RByers
Copy link
Member

RByers commented Jul 3, 2015

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.

@RByers RByers added the spec label Jul 3, 2015
@RByers RByers changed the title Should preventDefault throw? Is preventDefault being silently ignored on uncancelable events too error-prone? Jul 3, 2015
RByers added a commit that referenced this issue Jul 3, 2015
Partially addresses issue #3

Also tweak some spacing to make the definition look better.
@duanyao
Copy link

duanyao commented Jul 10, 2015

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

@RByers
Copy link
Member Author

RByers commented Jul 10, 2015

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
Copy link

duanyao commented Jul 10, 2015

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

@RByers
Copy link
Member Author

RByers commented Jul 10, 2015

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
Copy link

duanyao commented Jul 10, 2015

Yes, exactly.

@AshleyScirra
Copy link

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 RByers removed the spec label Jul 27, 2015
@RByers
Copy link
Member Author

RByers commented Sep 22, 2015

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

@RByers
Copy link
Member Author

RByers commented Sep 24, 2015

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
Copy link
Collaborator

annevk commented Sep 24, 2015

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

@smaug----
Copy link
Collaborator

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
Copy link
Collaborator

annevk commented Sep 24, 2015

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
Copy link
Member

foolip commented Sep 24, 2015

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----
Copy link
Collaborator

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

@RByers
Copy link
Member Author

RByers commented Sep 24, 2015

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
Copy link
Member

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
Copy link
Member Author

RByers commented Sep 24, 2015

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
Copy link
Member

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

@annevk
Copy link
Collaborator

annevk commented Sep 25, 2015

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
Copy link
Member Author

RByers commented Sep 25, 2015

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants