Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Closed
Perelandric wants to merge1 commit intojquery:masterfromPerelandric:fix_rquickExpr
Closed

Fixed rquickExpr to require 1 or more chars for ID#1682

Perelandric wants to merge1 commit intojquery:masterfromPerelandric:fix_rquickExpr

Conversation

@Perelandric
Copy link

jQuery'srquickExpr allows for an ID with zero or more characters. An ID with zero characters isn't useful for fetching an element from the DOM, andgetElementById will 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.

TherquickExpr in 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

@Perelandric
Copy link
Author

Alternate fix would be to allow the regex to continue to match, but make thegetElementById selection conditional.

https://github.com/jquery/jquery/blob/master/src/core/init.js#L68-L70

// HANDLE: $(#id)} else if (match[2]) {    elem = document.getElementById( match[2] );    // Support: Blackberry 4.6    // gEBID returns nodes no longer in the document (#6963)    if ( elem && elem.parentNode ) {        // Inject the element directly into the jQuery object        this.length = 1;        this[0] = elem;    }    this.context = document;    this.selector = selector;    return this;} else {    return this;}

@markelog
Copy link
Member

You would rather see it throwing a error?

This is bydesign.

@Perelandric
Copy link
Author

@markelog

Just so I'm understanding correctly, it is by design that$("#") behaves differently from$(document).find("#")?

@markelog
Copy link
Member

Hm,@rwaldron what is your thoughts on it?

@markelogmarkelog reopened thisOct 7, 2014
@dmethvin
Copy link
Member

Don't both of those return an empty set? What change in behavior are you advocating for one or both cases?

@dmethvindmethvin reopened thisOct 7, 2014
@dmethvin
Copy link
Member

I just want to say how ridiculously easy Github makes it to push the wrong button.

@markelog
Copy link
Member

:-)

@markelog
Copy link
Member

Don't both of those return an empty set?

$(document).find("#") would be passed directly to Sizzle, which would throw an error. WhereasjQuery("#") would usegetElementById as an optimization technic, which would return an empty set.

Which looks as inconsistency... which would affect very small amount of users

@arthurvr
Copy link
Member

According tohttps://html.spec.whatwg.org/multipage/dom.html#the-id-attribute:

The value must be unique amongst all the IDs in the element's home subtree and must contain at least one character. The value must not contain any space characters.

So, I think jQuery('#') must throw an error.

@dmethvin
Copy link
Member

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 returningundefined rather than a set. The old behavior seems clearly bad, there's no other case I know of where a chaining method would returnundefined.

@gibson042
Copy link
Member

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 fromrquickExpr takes no extra code at all.

@dmethvin
Copy link
Member

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

@gibson042 do you want to fix this in Sizzle?

@gibson042
Copy link
Member

It should already be correct in Sizzle... all this PR needs is a unit test.

@markelog
Copy link
Member

It should already be correct in Sizzle... all this PR needs is a unit test.

Unit test already falling with this one, need to edit it and it would be finished

@dmethvin
Copy link
Member

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.

@dmethvindmethvin self-assigned thisDec 3, 2014
dmethvin added a commit that referenced this pull requestDec 3, 2014
@dmethvindmethvin added this to the3.0.0 milestoneDec 8, 2014
@markelogmarkelog mentioned this pull requestNov 16, 2015
@markelogmarkelog mentioned this pull requestDec 22, 2015
@locklockbot locked asresolvedand limited conversation to collaboratorsJan 19, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

No reviews

Assignees

@dmethvindmethvin

Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@Perelandric@markelog@dmethvin@arthurvr@gibson042

[8]ページ先頭

©2009-2025 Movatter.jp