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

CSS classes starting with - is always unused and -- throws #1710

Closed
kaisermann opened this issue Aug 31, 2018 · 4 comments · Fixed by #3552
Closed

CSS classes starting with - is always unused and -- throws #1710

kaisermann opened this issue Aug 31, 2018 · 4 comments · Fixed by #3552
Assignees
Labels

Comments

@kaisermann
Copy link
Member

kaisermann commented Aug 31, 2018

CSS allow classes starting with -, but the svelte compiler flags them as unused REPL

With double -- it's even worse REPL

@kaisermann kaisermann changed the title CSS classes starting with - CSS classes starting with - is always unused and -- throws Aug 31, 2018
@Conduitry
Copy link
Member

Conduitry commented Sep 3, 2018

The 'identifier is expected' exception that's thrown in the second example is thrown by the underlying css-tree library we're using for CSS parsing. According to this SO answer, beginning a class name with a double hyphen is actually illegal according to the spec. I don't know how we want to handle that. We could see whether a later version of css-tree allows this, or we could just not worry about it, since it's apparently illegal.

The issue with class names beginning with a single hyphen does seem to be an issue on our end though.

edit: There is a highly upvoted comment on that answer though that says "The W3C says that the use of a leading '-' or '_' should be reserved for vendor-specific CSS extensions" - I don't know whether that affects what we want to do here. Probably not?

@Conduitry Conduitry added bug awaiting submitter needs a reproduction, or clarification labels Sep 3, 2018
@Conduitry
Copy link
Member

Conduitry commented Aug 9, 2019

The problem is that /\b-foo\b/.test('-foo') is false.

When testing whether a selector with a class matches against an element, we are using the same implementation as when we are checking whether an attribute matches according to the ~= operator. (I don't know whether this is correct in all cases.) The way we are doing that is by wrapping it in \b and turning it into a regex. (I also don't know whether this is correct in all cases.) But at least one of those two is incorrect, because it results in testing /\b-foo\b/ against the string -foo, which does not match, and the selector is seen as unused.

Edit: Checking on what ~= means precisely, and it sounds like that probably is an acceptable thing to do for class matching. So this means the issue is using \b word boundaries to implement them, which don't do the right thing in certain cases, e.g. with a leading hyphen.

@Conduitry
Copy link
Member

Reworking the handling of all of the operators in Selector.ts to not use regular expressions is probably the best way to do this, which will also fix other weird edge cases with selectors being interpreted as regular expressions. For example, the selector [title='['] throws an error during compilation because we are trying to construct the regex ^[$.

@Conduitry Conduitry self-assigned this Aug 9, 2019
@Conduitry Conduitry removed the awaiting submitter needs a reproduction, or clarification label Aug 9, 2019
Conduitry added a commit to Conduitry/svelte that referenced this issue Aug 29, 2019
@Conduitry
Copy link
Member

Circling back on this a little bit -

What I have now is in this branch. It's failing a test though (and it's breaking some other things that are not currently tested for) because the class/etc names in selectors are no longer getting unescaped, and so are incorrectly seen as not matching the appropriate elements. I'll need to look into exactly what sorts of escaping can happen in those.

What we're doing currently is just treating them as regexes and sort of taking care of the \ unescaping that way, but that's dangerous and the source of some of the other stuff I mentioned above,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants