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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@Perelandric

Perelandric commented Oct 7, 2014

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

The rquickExpr 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

This comment has been minimized.

Show comment
Hide comment
@Perelandric

Perelandric Oct 7, 2014

Alternate fix would be to allow the regex to continue to match, but make the getElementById 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;
}

Perelandric commented Oct 7, 2014

Alternate fix would be to allow the regex to continue to match, but make the getElementById 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

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Oct 7, 2014

Member

You would rather see it throwing a error?

This is by design.

Member

markelog commented Oct 7, 2014

You would rather see it throwing a error?

This is by design.

@markelog markelog closed this Oct 7, 2014

@Perelandric

This comment has been minimized.

Show comment
Hide comment
@Perelandric

Perelandric Oct 7, 2014

@markelog

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

Perelandric commented Oct 7, 2014

@markelog

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

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Oct 7, 2014

Member

Hm, @rwaldron what is your thoughts on it?

Member

markelog commented Oct 7, 2014

Hm, @rwaldron what is your thoughts on it?

@markelog markelog reopened this Oct 7, 2014

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Oct 7, 2014

Member

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

Member

dmethvin commented Oct 7, 2014

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

@dmethvin dmethvin closed this Oct 7, 2014

@dmethvin dmethvin reopened this Oct 7, 2014

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Oct 7, 2014

Member

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

Member

dmethvin commented Oct 7, 2014

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

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Oct 7, 2014

Member

:-)

Member

markelog commented Oct 7, 2014

:-)

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Oct 7, 2014

Member

Don't both of those return an empty set?

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

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

Member

markelog commented Oct 7, 2014

Don't both of those return an empty set?

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

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

@arthurvr

This comment has been minimized.

Show comment
Hide comment
@arthurvr

arthurvr Oct 7, 2014

Member

According to https://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.

Member

arthurvr commented Oct 7, 2014

According to https://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

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Oct 7, 2014

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

Member

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

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Oct 8, 2014

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

Member

gibson042 commented Oct 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 rquickExpr takes no extra code at all.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Oct 8, 2014

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.

Member

dmethvin commented Oct 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

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Oct 13, 2014

Member

@gibson042 do you want to fix this in Sizzle?

Member

dmethvin commented Oct 13, 2014

@gibson042 do you want to fix this in Sizzle?

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Oct 13, 2014

Member

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

Member

gibson042 commented Oct 13, 2014

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

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Oct 13, 2014

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

Member

markelog commented Oct 13, 2014

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

@Perelandric Perelandric reopened this Oct 13, 2014

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Dec 3, 2014

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.

Member

dmethvin commented Dec 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.

@dmethvin dmethvin self-assigned this Dec 3, 2014

dmethvin added a commit that referenced this pull request Dec 3, 2014

Core: Throw an error on $("#") rather than returning 0-length collection
Closes gh-1682

Thanks @goob for the issue report!
(cherry picked from commit 80022c8)

@dmethvin dmethvin closed this in 80022c8 Dec 3, 2014

@dmethvin dmethvin added this to the 3.0.0 milestone Dec 8, 2014

@markelog markelog referenced this pull request Nov 16, 2015

Closed

2.2 Release progress #2723

@markelog markelog referenced this pull request Dec 22, 2015

Closed

1.12 Release progress #2787

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment