-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat: @keyframes now support <string> type AND will convert it into ident if possible... #2556
Conversation
…to ident in mangle mode
p.advance() | ||
} else if !p.expect(css_lexer.TIdent) && !p.eat(css_lexer.TString) && !p.peek(css_lexer.TOpenBrace) { | ||
} else if !(p.expect(css_lexer.TIdent) && p.expect(css_lexer.TString)) && !p.peek(css_lexer.TOpenBrace) { | ||
// Consider string names a syntax error even though they are allowed by | ||
// the specification and they work in Firefox because they do not work in | ||
// Chrome or Safari. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if you saw this comment or not but lack of support for this was deliberate. This is currently a Firefox-only feature, which I don't think makes sense to support natively in esbuild at this time.
I agree that the bug that changes the contents of @keyframes
rules with unsupported syntax should still be fixed. These should be passed through unmodified. I will fix that part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I currently working on blink, and the webkit one is already merged, I submitted a PR to SWC project as well, so I think the change will be good for all, and all majar browser vendor prefixed one support string type name BTW
ref: WebKit/WebKit#4234
https://chromium-review.googlesource.com/c/chromium/src/+/3865188
swc-project/swc#5894
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evanw This is not a Firefox-only syntax, the prefixed @-webkit-keyframes / @-moz-keyframes
always support <string>
, and there are UseCounter in Chrome that say: The usage counter for strings in prefixed case is too high
.
I saw you submitted a commit to fix #2555 which makes it an unrecognized CSS rule, I think it need to be reverted to land my PR land to conform the CSS standard |
It should already confirm to the CSS standard after the latest release. The syntax should now be passed through unmodified, so you should now be able to use this syntax with esbuild's latest release. Do you have some CSS code for which that isn't true? |
I believe the commit you linked to in your previous comment should already implement what this PR is trying to do. One difference is that the string is always converted to an identifier, but this is because Chrome has refused to implement support for string names (at least as far as I can tell). Leaving the string name as-is would result in broken code in all Chromium-based browsers. |
... Plus
animation
andanimation-name
mangling as wellfixes: #2555