[1.9] Fix #12411 removeClass() doesn't remove as expected #913

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
@matjae
Contributor

matjae commented Aug 28, 2012

An empty function call is not the same, as calling a function with an undefined variable.
If we check against !arguments.length instead of undefined, we can tell apart these situations.

OLD:
.removeClass()
removes all classes, as expected

.removeClass(undefined)
also removes all classes, which is wrong in my eyes

NEW:
.removeClass(undefined)
removes nothing, like null, false and "" do

Bugreport:
http://bugs.jquery.com/ticket/12411

EDIT:
jsFiddle: http://jsfiddle.net/jaggli/npkgS/

PS: I noticed this while calling element.removeClass(someObject.someVar);
where the object was defined, but someVar was undefined, so this is a pretty common situation, I think.

Update src/attributes.js
.removeClass() //removes all classes, as expected
.removeClass(window.nonExistentVariable) //also removes all classes, this is NOT what we would expect
@rwaldron

This comment has been minimized.

Show comment Hide comment
@rwaldron

rwaldron Aug 28, 2012

Member

I'm not fond of patches that come from the web interface (ie. "patch-1" is a red flag). It means that the code is untested and that worries me—however, I did test this and it passes the core test suite without issue. This isn't necessarily a good thing, because there are no tests for it to begin with.

I'm going to defer to @dmethvin on this one.

Member

rwaldron commented Aug 28, 2012

I'm not fond of patches that come from the web interface (ie. "patch-1" is a red flag). It means that the code is untested and that worries me—however, I did test this and it passes the core test suite without issue. This isn't necessarily a good thing, because there are no tests for it to begin with.

I'm going to defer to @dmethvin on this one.

@rwaldron

This comment has been minimized.

Show comment Hide comment
@rwaldron

rwaldron Aug 28, 2012

Member

... In the meantime, it would be great if you could add some tests that support this behaviour change

Member

rwaldron commented Aug 28, 2012

... In the meantime, it would be great if you could add some tests that support this behaviour change

@matjae

This comment has been minimized.

Show comment Hide comment
@matjae

matjae Aug 28, 2012

Contributor

Here we go. Tests added and jsFiddle http://jsfiddle.net/jaggli/npkgS/

Contributor

matjae commented Aug 28, 2012

Here we go. Tests added and jsFiddle http://jsfiddle.net/jaggli/npkgS/

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Aug 31, 2012

Member

/botio test

Member

gnarf commented Aug 31, 2012

/botio test

@jquerybot

This comment has been minimized.

Show comment Hide comment
@jquerybot

jquerybot Aug 31, 2012

From: jQuery Bot.io


Received

Command cmd_test from @gnarf37 received. Current queue size: 0

Live output at: http://swarm.jquery.org:8000/be3552f049b7b49/output.txt

From: jQuery Bot.io


Received

Command cmd_test from @gnarf37 received. Current queue size: 0

Live output at: http://swarm.jquery.org:8000/be3552f049b7b49/output.txt

@jquerybot

This comment has been minimized.

Show comment Hide comment
@jquerybot

jquerybot Aug 31, 2012

From: jQuery Bot.io


Success

Full output at http://swarm.jquery.org:8000/be3552f049b7b49/output.txt

Total script time: 17.44 mins

Swarm URL: http://swarm.jquery.org/job/685

From: jQuery Bot.io


Success

Full output at http://swarm.jquery.org:8000/be3552f049b7b49/output.txt

Total script time: 17.44 mins

Swarm URL: http://swarm.jquery.org/job/685

@dmethvin

This comment has been minimized.

Show comment Hide comment
@dmethvin

dmethvin Sep 10, 2012

Member

This could affect duck-punchers so it'll have to wait until 1.9.

Member

dmethvin commented Sep 10, 2012

This could affect duck-punchers so it'll have to wait until 1.9.

@dmethvin

This comment has been minimized.

Show comment Hide comment
@dmethvin

dmethvin Oct 22, 2012

Member

I agree on the use of arguments.length, since that's what we are now using for other APIs as well to determine whether an arg has been passed. I am okay with adding a unit test for undefined but do not want to add the others and codify their behavior. I'll change the unit tests before landing.

Member

dmethvin commented Oct 22, 2012

I agree on the use of arguments.length, since that's what we are now using for other APIs as well to determine whether an arg has been passed. I am okay with adding a unit test for undefined but do not want to add the others and codify their behavior. I'll change the unit tests before landing.

@dmethvin dmethvin closed this in 227c49a Oct 22, 2012

@sylus sylus referenced this pull request in wet-boew/wet-boew Jun 11, 2014

Closed

Function viewChange strips initial body classes #5579

mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014

Fix #12411, .removeClass(undefined) is a chaining no-op. Close gh-913.
.removeClass() //removes all classes, as documented
.removeClass(window.nonExistentVariable) // removes nothing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment