Skip to content
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

addClass() called with an array doesn't add all of the classes if one of them is an empty string #4998

Closed
MatmaRex opened this issue Jan 6, 2022 · 10 comments · Fixed by #5003

Comments

@MatmaRex
Copy link

MatmaRex commented Jan 6, 2022

Description

When addClass() is called with an array, and one of the classes in the array is an empty string, it doesn't add all of the classes (it stops processing them after the empty string).

Minimal test case:

$( '<div>' ).addClass( [ 'a', '', 'b' ] ).attr( 'class' )

Expected output: a b
Actual output: a

Link to test case

https://jsbin.com/voxetucoho/edit?js,console

@mgol mgol added Attributes Bug Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labels Jan 7, 2022
@mgol
Copy link
Member

mgol commented Jan 7, 2022

Thanks for the report. This is caused by this line:

while ( ( clazz = classes[ j++ ] ) ) {

I wonder if there's any concrete reason why we're not checking for length here. I know there have been some issues related to this property in old IE but perhaps not when we're dealing with a regular array.

@mgol
Copy link
Member

mgol commented Jan 7, 2022

Note, though, that an empty string is not a valid class name and if you pass it to the native classList.add, it will throw an error. Array input to addClass requires valid classes to be present in the input so the proposed input is invalid.

That said, the current behavior may still looks surprising so I'd be open to changing this.

@timmywil
Copy link
Member

timmywil commented Jan 7, 2022

I'd be open to changing this as well. It makes sense to apply as many valid classes as possible, and I could see a case where a variable is passed that happens to be empty string.

@MatmaRex
Copy link
Author

MatmaRex commented Jan 7, 2022

Note, though, that an empty string is not a valid class name and if you pass it to the native classList.add, it will throw an error. Array input to addClass requires valid classes to be present in the input so the proposed input is invalid.

It is not valid for sure, but jQuery is happy to ignore other invalid class names, such as ' ' (a space).

$( '<div>' ).addClass( [ ' ' ] ).attr( 'class' )

Output: undefined

It is also happy to clean up and tweak them to make them valid:

$( '<div>' ).addClass( [ 'a b', ' c ' ] ).attr( 'class' )

Output: a b c

If you'd like these to throw an exception, that'd be fine by me, but I'd expect all invalid class names to behave consistently.

I'd be open to changing this as well. It makes sense to apply as many valid classes as possible, and I could see a case where a variable is passed that happens to be empty string.

Yes, when I discovered this bug, the empty string was actually generated by a library very far away from the place where it was added as a class.

@mgol
Copy link
Member

mgol commented Jan 7, 2022

If you'd like these to throw an exception, that'd be fine by me, but I'd expect all invalid class names to behave consistently.

I think we're all leaning to making it work for an empty string as well. Would you like to submit a PR with a fix?

@MatmaRex
Copy link
Author

MatmaRex commented Jan 7, 2022

I'd love to, but I find it uncomfortable to contribute to projects that require me to sign a CLA first.

(Also, your links to the CLA do not work, e.g. in CONTRIBUTING.md it links to https://contribute.jquery.org/cla/ which just redirects to https://openjsf.org/.)

@mgol
Copy link
Member

mgol commented Jan 7, 2022

Thanks for the info, we'll update the links. More information is available at https://openjsf.org/about/the-openjs-foundation-cla/.

I understand the CLA reservations but, unfortunately, we are not allowed to merge code from contributors that haven't signed it. If you don't want to do that, we'll fix it ourselves.

@timmywil timmywil removed the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Jan 10, 2022
@timmywil timmywil added this to the 3.6.1 milestone Jan 10, 2022
mgol added a commit to mgol/jquery that referenced this issue Jan 20, 2022
This change makes jQuery skip falsy values in `addClass( array )`
& `removeClass( array )` instead of stopping iteration when the first falsy
value is detected. This makes code like:
```js
elem.addClass( [ "a", "", "b" ] );
```
add both the `a` & `b` classes.

The code was also optimized for size a bit.

Fixes jquerygh-4998
@mgol
Copy link
Member

mgol commented Jan 20, 2022

PR: #5003.

mgol added a commit that referenced this issue Jan 24, 2022
This change makes jQuery skip falsy values in `addClass( array )`
& `removeClass( array )` instead of stopping iteration when the first falsy
value is detected. This makes code like:
```js
elem.addClass( [ "a", "", "b" ] );
```
add both the `a` & `b` classes.

The code was also optimized for size a bit so it doesn't increase the
minified gzipped size.

Fixes gh-4998
Closes gh-5003
mgol added a commit to mgol/jquery that referenced this issue Jan 24, 2022
This change makes jQuery skip falsy values in `addClass( array )`
& `removeClass( array )` instead of stopping iteration when the first falsy
value is detected. This makes code like:
```js
elem.addClass( [ "a", "", "b" ] );
```
add both the `a` & `b` classes.

The code was also optimized for size a bit so it doesn't increase the
minified gzipped size.

Fixes jquerygh-4998
Closes jquerygh-5003

(partially cherry picked from commit a338b40)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants
@MatmaRex @timmywil @mgol and others