Skip to content

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

Merged
devongovett merged 9 commits intoparcel-bundler:masterfrom
yisibl:fix-keyframes-name
Sep 7, 2022
Merged

fix: CSS-wide keywords and none in @keyframes cannot remove quotes#267
devongovett merged 9 commits intoparcel-bundler:masterfrom
yisibl:fix-keyframes-name

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.

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