Event: evaluate selector at add time #3097
Conversation
This ensures events with invalid selectors throw at attach time instead of event time. Fixes: jquery#3071
|
|
||
| // If the selector is invalid, throw any exceptions at attach time | ||
| if ( selector ) { | ||
| jQuery.find( selector, elem ); |
There was a problem hiding this comment.
This may or may not require a context elem.ownerDocument. If it's just testing validity, perhaps not, but can anyone think of a use case where the document would affect validity?
There was a problem hiding this comment.
None that I can think of. The only case I can think of that breaks is if a selector extension was added after the event was attached here, but before an event was fired. That's probably not common?
There was a problem hiding this comment.
Also note that this is inside jQuery.event.add() and the API surface is jQuery.fn.on(). If someone does $("tr").on("mouseover" ...) on a table with 100 rows it will check the selector 100 times. The parsed selector is cached and the selection set is small obviously, but is there a downside to moving this up to function on()?
There was a problem hiding this comment.
The only problem I see is that one would have to replicate the selector parameter parsing. Maybe a better approach would be to check if there is already an event handler with such a selector in the handlers list?
I may be wrong, but I think event handlers with selectors usually attach to a single object, as the whole point is to have to avoid listing all possible elements (which may be a lot, or not exist yet).
There was a problem hiding this comment.
@fsateler You are right, it won't be common for people to make a call to attach delegated events on a lot of elements at once. Also, this change does have the potential to throw errors where they did not before, in the case where the selector was invalid but no event ever occurred. We learned in gh-2824 that even obscure cases like that can cause trouble. So even if we moved this to on() it should be executed only if there is at least one element.
In short, I think you have this check in a good place.
|
@fsateler can you make that Anyone have thoughts on whether and how this should be documented? I am thinking it doesn't need docs, this behavior may have been what everyone expected anyway. |
|
Yes, I've just been busy during this week with $work. I'll push the change during the weekend. |
|
Updated for the |
|
Thanks, @fsateler! As it turns out, |
5 participants
Summary
This ensures events with invalid selectors throw at attach time instead
of event time. This is done by instantiating a Sizzle for the selector.
jQuery.find()is used instead ofjQuery.find.matchesSelector(as suggested in #3071 (comment)) , becausematchesSelectorrequires aDOMElement, which won't be if the element is actually document.Fixes: #3071
Checklist
Mark an
[x]for completed items, if you're not sure leave them unchecked and we can assist.Thanks! Bots and humans will be around shortly to check it out.