css: scope animation-name to its @keyframes hash in CSS modules (#18921) - #33322
Conversation
Fixes oven-sh#18921. `animation` and `animation-name` were never wired into the hand-maintained structured CSS property table, so they fell back to UnparsedProperty and their referenced `@keyframes` name was emitted verbatim — the CSS-modules scoping in `AnimationName::to_css` was never reached. The `@keyframes` name was scoped but the reference was not, so the animation silently broke. Wire `animation` (shorthand) and `animation-name` (longhand), including the vendor-prefixed forms, as structured properties mirroring the existing `transition` property. Their names now flow through `AnimationName::to_css` and are scoped to match the `@keyframes` rule. Also fix `Animation::to_css` so a name-less shorthand (e.g. `animation: 3s linear 1s infinite alternate`) keeps its components instead of collapsing to `none`: Bun prints the `Animation` struct directly and, unlike lightningcss, has no declaration handler to keep it off that lossy path.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds support for the ChangesAnimation property support
Related Issues: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thank you, will pick this up shortly. |
…eywords Pins code paths that the structured animation property wiring makes reachable for the first time: the timeline component in the shorthand, the zero-duration-nonzero-delay guard (mirrors the transition test), the quoted-string animation-name form in CSS modules, and CSS-wide keywords surviving the structured parse.
Bridges the five field types that only had PartialEq (the four component enums plus AnimationTimeline) so the existing #[derive(CssEql)] proc-macro applies, matching the Font struct precedent. Removes the need to keep the eql body in sync when fields are added.
- redundant_closure: pass EasingFunction::is_ident directly to is_some_and - trivially_copy_pass_by_ref: AnimationIterationCount is now Copy, take self by value in to_css
…base The variants were deleted earlier on this branch; #33322 (merged to main) added deep_clone with an unreachable arm for them. Rebase merged both without a textual conflict, leaving a reference to non-existent variants.
What does this PR do?
Fixes #18921.
In CSS modules, the
animationshorthand andanimation-namelonghand did not scope their referenced@keyframesname to the hashed name the@keyframesrule receives. The keyframes definition was scoped but the reference was not, so the two diverged and the animation silently broke:Root cause:
animation/animation-namewere never wired into Bun's (hand-maintained) structured CSS property table, so they fell back toUnparsedProperty(raw tokens) and their names were emitted verbatim — the CSS-modules scoping that lives inAnimationName::to_csswas never reached.Fix:
animation(shorthand) andanimation-name(longhand), including the vendor-prefixed forms, as structured properties, mirroring the existingtransitionproperty. Their names now flow through the scoping-awareAnimationName::to_cssand are scoped to match the@keyframesrule.Animation::to_cssso a name-less shorthand (e.g.animation: 3s linear 1s infinite alternate) keeps its components instead of collapsing tonone. Bun prints theAnimationstruct directly and, unlike Lightning CSS, has no declaration handler to keep it off that lossy path.As a side effect,
animation/animation-nameare now serialized in canonical form (name last, redundant trailingnonedropped, numbers normalized) — consistent with how Bun already handlestransition, and with Lightning CSS and esbuild. All changes are lossless.How did you verify your code works?
New tests (each fails on stable Bun, passes with this change):
test/bundler/css/css-modules.test.ts— asserts theanimationshorthand andanimation-namelonghand reference the same hashed name as their@keyframesrule (the exact repro from Inconsistent hashing behavior with CSS animation names and keyframes in CSS modules #18921), and that the unscoped names do not survive.test/js/bun/css/css.test.ts— ananimationserialization matrix: canonical ordering, no data loss for name-less shorthands,none, vendor prefix, multiple comma-separated animations, and theanimation-namelonghand.All existing CSS suites pass with the change:
test/js/bun/css/css.test.tstest/bundler/css/css-modules.test.tstest/bundler/css/view-transition-23600.test.tstest/bundler/esbuild/css.test.ts🤖 Generated with Claude Code