[CLOSED] Consider removing or patching toggleClass()

[core-ai-bot]

Issue by peterflynn
Thursday Apr 04, 2013 at 06:06 GMT
Originally opened as adobe/brackets#3338

---

jQuery's toggleClass() is an easy method to misuse: the 2nd argument is a boolean, but it won't work as intended unless it's literally true or false. Truthy/falsy values are ignored.

Some of our code explicitly acknowledges this via Boolean() casts, or by using an if with add/RemoveClass() instead (e.g. Menu.js). But lots of our code simply skates by because it happens to use !. And in at least one place -- ViewUtils -- we don't even have that assurance. (See #3184 for another case of how easy it is to get snared by this).

From what I understand, jQuery does this to remain compatible with the jQuery-UI-patched version of toggleClass() where the 2nd argument could also be a number that means something totally different (animation). So there's little chance of a "fix" in future jQuery versions.

So to reduce brittleness, it seems like we should either:
a) Have a policy of not using toggleClass() anywhere. Use the more verbose add/removeClass() form instead.
b) Have a policy of not using toggleClass() anywhere. Create our own toggleClass() utility function that we use instead.
c) Create a little jQuery plugin that patches the standard jQuery.toggleClass() to behave more safely.

Personally I lean toward C since it doesn't require constant enforcement... but the downside is if we ever start using jQuery UI later, then this API would behave differently than the jQ UI docs specify.

[core-ai-bot]

Comment by redmunds
Monday Apr 08, 2013 at 16:27 GMT

---

I wasn't aware of that behavior, but I think it's bad enough that we should fix it in jQuery and submit a pull request.

I think toggleClass() is only appropriate in only a very few, simple cases, so we can convert to add/removeClass() in most places and the few remaining cases can be fixed in place (with a comment) until jQuery is fixed.

[core-ai-bot]

Comment by pthiess
Monday Apr 08, 2013 at 18:16 GMT

---

Reviewed

[core-ai-bot]

Comment by peterflynn
Wednesday May 01, 2013 at 21:53 GMT

---

@redmunds We'd never be able to merge a patch upstream -- that idea was rejected a while back because it would break backwards-compatibility with the way jQuery UI grafts on animation capabilities. (IMHO, this is a perfect example of the downside to jQuery's "endless overloading" API design... but that architectural choice was set in stone long ago).

[core-ai-bot]

Comment by peterflynn
Tuesday May 14, 2013 at 00:11 GMT

---

Pull request from@WebsiteDeveloper has landed, following approach B (except in a few places where it was cleaner to go with A). I've updated the coding style guide to mention that we should not use $.toggleClass(). Feels good enough to close for me.