- Notifications
You must be signed in to change notification settings - Fork20.5k
Fixed rquickExpr to require 1 or more chars for ID#1682
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Perelandric commentedOct 7, 2014
Alternate fix would be to allow the regex to continue to match, but make the https://github.com/jquery/jquery/blob/master/src/core/init.js#L68-L70 |
markelog commentedOct 7, 2014
You would rather see it throwing a error? This is bydesign. |
Perelandric commentedOct 7, 2014
Just so I'm understanding correctly, it is by design that |
markelog commentedOct 7, 2014
Hm,@rwaldron what is your thoughts on it? |
dmethvin commentedOct 7, 2014
Don't both of those return an empty set? What change in behavior are you advocating for one or both cases? |
dmethvin commentedOct 7, 2014
I just want to say how ridiculously easy Github makes it to push the wrong button. |
markelog commentedOct 7, 2014
:-) |
markelog commentedOct 7, 2014
Which looks as inconsistency... which would affect very small amount of users |
arthurvr commentedOct 7, 2014
According tohttps://html.spec.whatwg.org/multipage/dom.html#the-id-attribute:
So, I think jQuery('#') must throw an error. |
dmethvin commentedOct 7, 2014
It doesn't throw an error if you enter from the console I guess. So I wrote the test case that I wish was already on the ticket.http://jsfiddle.net/8wdfL6ph/ There are limited number of places and cases that jQuery throws errors on bad selector input. For example in the quoted spec you gave, we don't throw an error if there are duplicated IDs, or if the ID had escaped space characters for that matter. Usually the result is an empty set, but that's not guaranteed since the input is invalid. So it seems strange to go our of our way to single out one specific invalid input and throw an error just on that. Even in#4321 the main reason it was fixed was because it was returning |
gibson042 commentedOct 8, 2014
While we don't really go out of our way, Sizzle has been increasingly diligent about rejecting invalid input. I support letting Sizzle handle this particular case, especially since excepting it from |
dmethvin commentedOct 8, 2014
That should make it fall into the general case and die in the current convenient place, which is fine. I was more concerned about identifying cases we should deal with explicitly. |
dmethvin commentedOct 13, 2014
@gibson042 do you want to fix this in Sizzle? |
gibson042 commentedOct 13, 2014
It should already be correct in Sizzle... all this PR needs is a unit test. |
markelog commentedOct 13, 2014
Unit test already falling with this one, need to edit it and it would be finished |
dmethvin commentedDec 3, 2014
We have a unit test for this with the old 0-length behavior so we're changing things up. Since it's a 3.0 i'm okay with that. |
jQuery's
rquickExprallows for an ID with zero or more characters. An ID with zero characters isn't useful for fetching an element from the DOM, andgetElementByIdwill throw an error in Firefox, but not in Chrome.While a selector like
$("#")is unlikely to be manually written, it's more likely to be encountered with generated selectors.The
rquickExprin Sizzle does require at least one character after#, so this will bring uniformity to the optimization.Sizzle line 135:https://github.com/jquery/sizzle/blob/709e1db5bcb42e9d761dd4a8467899dd36ce63bc/src/sizzle.js#L135