-
Notifications
You must be signed in to change notification settings - Fork 139
Is preventDefault being silently ignored on uncancelable events too error-prone? #3
Comments
Partially addresses issue #3 Also tweak some spacing to make the definition look better.
How about making |
I don't understand - how would that help? I think it's intentional that once an event is cancelled (by |
For any event, changing |
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 I agree, that's an option. |
Yes, exactly. |
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.) |
Re-opened the default value debate in #17. We'll come back to this once that's resolved. |
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? |
If we do that, passive would become a distinct flag from cancelable. And we'd likely need to expose it on |
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. |
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 |
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 |
a bit odd if preventDefault() throwing in an event listener depends on the other event listeners. |
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. |
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:
|
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 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. |
That's fair. I'm OK with that. |
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. |
Agreed
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). |
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.
The text was updated successfully, but these errors were encountered: