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

hasClassWithNative_ throws an error when trying to check for several classes #209

Closed
antoniopol06 opened this issue Feb 8, 2017 · 7 comments
Assignees
Labels

Comments

@antoniopol06
Copy link

antoniopol06 commented Feb 8, 2017

The hasClass method throws an error if it is invoked with a list of several classes when classList is available, for example:

import {dom} from 'metal-dom';

let el = document.createElement('div');
el.classList.add('foo');
el.classList.add('bar');

dom.hasClass(el, 'foo bar'); // throws 

But, if classList is not available it works without errors.

@fernandosouza
Copy link
Contributor

Hello, @antoniopol06. Can you share the error message?

@fernandosouza
Copy link
Contributor

It's probably the token native error message alerting that the white space is not allowed. Uncaught DOMException: Failed to execute 'contains' on 'DOMTokenList': The token provided ('foo bar') contains HTML space characters, which are not valid in tokens.

We try to follow the DOM native API but our implementation of hasClass method is producing 2 different behaviours. We are going to look into it. Thank you.

@eduardolundgren
Copy link
Contributor

The hasClass only accepts one class per time, this matches the native behavior of classList contains.

@fernandosouza
Copy link
Contributor

Hey, @eduardolundgren. hasClass follow the native behaviour. But the method also has a fallback for browsers that don't have support for classList (hasClassWithoutNative_), and this method uses indexOf() to find the given class name inside element.className. In this case, you can pass multiple class names separated by space.

This is what @antoniopol06 has reported.

In case to follow the same behaviour, the fallback method should also throw an error when it receives more than one class name.

@eduardolundgren
Copy link
Contributor

Got it. Yes they should behave the same (as the native one).

@fernandosouza fernandosouza self-assigned this Feb 9, 2017
fernandosouza pushed a commit to fernandosouza/metal.js that referenced this issue Mar 16, 2017
@fernandosouza
Copy link
Contributor

fernandosouza commented Mar 16, 2017

Correcting what I said before, throws an error seems not be the right approach for this. Once tokenList.contains() specification says: The contains(token) method, when invoked, must return true if token is in tokens, and false otherwise.

So, it says nothing about throws an error if white spaces are passed. And throws an error seems to be a Chrome implementation only.

In this case, we should return false if white spaces are passed.

@p2kmgcl p2kmgcl mentioned this issue Apr 30, 2017
@jbalsas
Copy link
Contributor

jbalsas commented Sep 13, 2017

Fixed in #231!

@jbalsas jbalsas closed this as completed Sep 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants