Fix hasClass("undefined") for text and comment nodes, with test case #418

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@olov

olov commented Jun 21, 2011

See #9630

@@ -146,7 +146,7 @@ jQuery.fn.extend({
hasClass: function( selector ) {
var className = " " + selector + " ";
for ( var i = 0, l = this.length; i < l; i++ ) {
- if ( (" " + this[i].className + " ").replace(rclass, " ").indexOf( className ) > -1 ) {
+ if ( this[i].className !== undefined && (" " + this[i].className + " ").replace(rclass, " ").indexOf( className ) > -1 ) {

This comment has been minimized.

@timmywil

timmywil Jun 21, 2011

Member

I wonder if we could safely do this by doing...

if ( this[i].className && ... )

We can skip other falsy values besides undefined as well.

@timmywil

timmywil Jun 21, 2011

Member

I wonder if we could safely do this by doing...

if ( this[i].className && ... )

We can skip other falsy values besides undefined as well.

This comment has been minimized.

@olov

olov Jun 21, 2011

I attempted that first as well but found that it changes the current behavior of hasClass(""). I chose to limit this patch to fixing the hasClass("undefined") bug.

Having said that I think that it would be wise to also look over the expected behavior of hasClass for selector "" and undefined (not "undefined") as well. It isn't obvious to me what result you'd like for those, although I'd assume that undefined coerced into "undefined" isn't what you want. A type-guard at the beginning of the function would fix that easily.

@olov

olov Jun 21, 2011

I attempted that first as well but found that it changes the current behavior of hasClass(""). I chose to limit this patch to fixing the hasClass("undefined") bug.

Having said that I think that it would be wise to also look over the expected behavior of hasClass for selector "" and undefined (not "undefined") as well. It isn't obvious to me what result you'd like for those, although I'd assume that undefined coerced into "undefined" isn't what you want. A type-guard at the beginning of the function would fix that easily.

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Jun 21, 2011

Member

Closed ticket - nodes of type !1 are not subject to add|removeClass

http://bugs.jquery.com/ticket/9630

Member

rwaldron commented Jun 21, 2011

Closed ticket - nodes of type !1 are not subject to add|removeClass

http://bugs.jquery.com/ticket/9630

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Jun 21, 2011

Member

rwaldron makes a good point.

Member

timmywil commented Jun 21, 2011

rwaldron makes a good point.

@timmywil timmywil closed this Jun 21, 2011

@olov

This comment has been minimized.

Show comment
Hide comment
@olov

olov Jun 22, 2011

timmywil just as an FYI the ticket got reopened.

olov commented Jun 22, 2011

timmywil just as an FYI the ticket got reopened.

@olov

This comment has been minimized.

Show comment
Hide comment
@olov

olov Jun 22, 2011

For the record #419 now has the bug fix

olov commented Jun 22, 2011

For the record #419 now has the bug fix

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