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

fix: CSS-wide keywords and none in @keyframes cannot remove quotes #267

Merged
merged 9 commits into from
Sep 7, 2022

Conversation

yisibl
Copy link
Contributor

@yisibl yisibl commented Sep 1, 2022

In the @keyframes rule, this is an invalid syntax if the <keyframes-name> is CSS-wide keywords or none keyword.

<keyframes-name> = <custom-ident> | <string>

Before this PR, we would always remove the quotes, which would result in generating an invalid CSS if the quotes happened to be CSS-wide keywords.

CSS-wide keywords =  initial | inherit | unset | default | revert | revert-layer

Before

/* Input */
@keyframes "revert" {}
@keyframes "none" {}

/* Output */
@keyframes revert {}
@keyframes none {}

After

/* Input */
@keyframes "revert" {}
@keyframes "none" {}

/* Output */
@keyframes "revert" {}
@keyframes "none" {}

Throw an error

With the new parsing method, the following cases will throw an error, which was previously considered a legal CSS rule.

/* Input */
@keyframes revert {}
@keyframes none {}

/* Output */
Unexpected token Ident("revert")
Unexpected token Ident("none")

@yisibl yisibl force-pushed the fix-keyframes-name branch from c08068a to 4dc1f3e Compare September 1, 2022 16:29
@yisibl yisibl force-pushed the fix-keyframes-name branch from 4dc1f3e to f9c49fe Compare September 5, 2022 11:17
Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! We'll need to update AnimationName in properties/animation.rs the same way as KeyframesName.

src/rules/keyframes.rs Outdated Show resolved Hide resolved
if context.unused_symbols.contains(keyframes.name.0.as_ref()) {
if context
.unused_symbols
.contains(&keyframes.name.to_css_string(Default::default()).unwrap())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will result in the name being quoted/escaped, but the unused_symbols list won't be. You'll need to do a match on the keyframe name to pull out the embedded string.

@yisibl
Copy link
Contributor Author

yisibl commented Sep 7, 2022

We'll need to update AnimationName in properties/animation.rs the same way as KeyframesName.

Can I fix it in the next PR? Here I want to focus on @keyframes.

@devongovett devongovett merged commit aa13001 into parcel-bundler:master Sep 7, 2022
@yisibl yisibl deleted the fix-keyframes-name branch September 15, 2022 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants