Conversation
Rotate stored none as a zero rotation about the z axis and printed any such value as none, so rotate: 0deg minified to rotate: none. The two are not equivalent: per CSS Transforms 2, any rotate value other than none creates a stacking context and a containing block, like a non-none transform. Give Rotate an explicit None variant, matching Translate and Scale.
…lpha rgb(from #8 r g b) was folded to #000. CSS Color 5 says an omitted alpha in the relative color syntax defaults to the origin color's alpha rather than to 100%, so this should stay #8. All relative color functions (rgb, hsl, hwb, lab, lch, oklab, oklch, color) share parse_alpha, and Bun.color() uses the same parser.
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Status: ready for review. Everything that ran in CI is green; the red build status is a queue problem, not a test failure.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — two well-scoped CSS minifier correctness fixes with thorough coverage.
What was reviewed
Rotatestruct→enum: mirrors the existingTranslate/Scaleshape in the same file; all consumers (parse/to_css/to_transform/deep_cloneand the generic bridges inproperties_generated.rs) still line up.parse_alpha: confirmedparse_legacy_alphais unreachable whenparser.fromis set (bothis_legacy_syntaxsites gate onfrom.is_none()), so the single-helper fix covers every relative-color function.- Origin alpha is read from the same
components.3the explicitalphakeyword uses, and is populated after.resolve()/.resolve_missing(), sonone→0 and range are already handled. - Tests cover both entry points (minifier +
Bun.color()), every color function, explicit/1and/alpha,noneorigin alpha, nested/light-dark()origins, and the rotate 0deg/0turn/axis/merge-into-transform variants.
Extended reasoning...
Overview
Two independent CSS minifier value-semantics fixes:
src/css/properties/transform.rs:Rotatechanges from a struct (which conflatednonewith{0,0,1,0deg}) to an enum withNoneandXyzvariants, matchingTranslate/Scalein the same file and upstream lightningcss.to_cssnow only printsnonefor theNonevariant, sorotate: 0degis preserved (it creates a stacking context,nonedoes not).to_transformstill mapsNoneto the identityrotate3d, so folding into a precedingtransform:list is unchanged.src/css/values/color.rs:parse_alphanow returnsparser.from.components.3(the origin color's alpha) when no/is present in a relative color, per CSS Color 5. Absolute colors (fromisNone) still default to 1.0.- Tests: new
relative colorsblock incss.test.ts(rgb/hsl/hwb/lab/lch/oklab/oklch/color(), explicit alpha,nonealpha, nested,light-dark); expandedrotatecases (0deg/0turn/z 0deg/0 0 1 0deg/x 0deg, none↔0deg ordering, merge-into-transform); newrelative color syntaxblock incolor.test.tsexercisingBun.color(). One existing assertion (0 0 1 0deg→none) was corrected to0deg, which is the bug being fixed.
Security risks
None. Pure CSS value parsing/serialization in safe Rust; no untrusted length arithmetic, no allocation changes, no FFI, no auth/crypto.
Level of scrutiny
Medium. The Rotate change is a mechanical refactor to an established local pattern; I grep-verified no other site constructs or field-accesses Rotate outside transform.rs (the generated-property and generics bridges go through parse/to_css/deep_clone/eql, all of which are updated). The parse_alpha change is a 4-line semantic fix at the shared helper; I verified parse_legacy_alpha cannot co-occur with a from origin (both is_legacy_syntax computations at color.rs:1183 and :1391 require parser.from.is_none()), and that components.3 is the same value the explicit alpha keyword returns (color.rs:2250), so omitted and / alpha now agree by construction.
Other factors
- The origin alpha is already resolved (
.resolve()/.resolve_missing()beforeRelativeComponentParser::new), so anoneorigin alpha is 0 and no extra clamping is needed — covered by thergb(from rgb(0 0 0 / none) r g b)→#0000case. Rotateretains#[derive(Copy, Clone, PartialEq)], sodeep_clone(*self) andcss_eql_partialeq!remain valid.- PR description states the new tests fail on the released build and pass with the change, and that the WPT relative-color bundler suite (opaque origins) still passes.
- No prior human review comments to address; only a rate-limited coderabbit placeholder.
This branch has not been deployed
2 participants
Two CSS minifier value folds that change the meaning of the input. Both came out of a fuzzing round against lightningcss; the third finding from that round,
rem()folding with the divisor's sign, already has an open fix in #32286 and is not repeated here.Problem
rotate: 0degis minified torotate: none. The two are not equivalent: arotatevalue other thannonecreates a stacking context and a containing block for fixed/absolute descendants, exactly like a non-nonetransform, so the fold can change layout. lightningcss keeps0deg.Rotateinsrc/css/properties/transform.rswas a plain struct and parsednoneas{x: 0, y: 0, z: 1, angle: 0deg};to_cssprinted every value matching that pattern asnone, so a literal0deg,z 0degor0 0 1 0degbecamenonetoo./ alphais folded fully opaque:rgb(from #0008 r g b)becomes#000,hsl(from rgb(1 2 3 / .5) calc(h + 180) s l)becomes#030201. CSS Color 5 says an omitted alpha in the relative syntax defaults to the origin color's alpha (not to 100% as in the absolute syntax), so these should be#0008and#03020180. lightningcss has the same bug; this follows the spec and browsers.parse_alphainsrc/css/values/color.rsreturns1.0whenever there is no/, without looking atparser.from, which already holds the resolved origin color (it is what thealphakeyword reads).Bun.color()uses the same parser, soBun.color("rgb(from #0008 r g b)", "css")also returned"#000".Fix
Rotatebecomes an enum with aNonevariant and anXyz { x, y, z, angle }variant, the same shape asTranslateandScale(and as current upstream lightningcss).noneround-trips tonone, everything else prints its angle; thex/ykeyword and z-axis shortenings are unchanged.to_transformstill mapsNoneto the identityrotate3d(0, 0, 1, 0deg), so merging into a precedingtransformis unchanged.parse_alphareturns the origin's alpha when afromcolor is set and no/was written. Every relative-capable function (rgb,hsl,hwb,lab,lch,oklab,oklch,color()) goes through this one helper. The origin alpha is already resolved (nonebecomes 0, same as the explicitalphakeyword), so the explicit/ alphaand the omitted form now agree. The token-list path forrgb(... / var(--a))requires an explicit/and is not affected.test/js/bun/css/css.test.ts(transformblock, newrelative colorsblock): 5 rotate cases and 14 relative color cases fail on the released build and pass with this change; full file passes.test/js/bun/css/color.test.ts(newrelative color syntaxblock): fails on the released build, passes with this change; full file passes.test/bundler/css/(including the WPT relative color file, whose origins are all opaque) passes.Background
translate,rotateandscaleare separate properties that compose withtransform. Their initial valuenoneis special in CSS Transforms 2: it is the only value that does not establish a stacking context / containing block, which is why a minifier may not rewrite an identity rotation tononeeven though both render the same.rgb(from <origin> r g b / alpha)parses the origin color, converts it into the function's color space, and exposes its channels as keywords. In this codebaseComponentParser::fromholds those resolved channels (components.3is alpha) while the arguments are parsed;RelativeComponentParser::get_identis how thealphakeyword reads it.src/css/and are a port of lightningcss;minify_testintest/js/bun/css/runs a stylesheet through the minifier in-process and compares the output string.Repro
bun build --minifybefore:a{rotate:none}c{color:#000;background:#030201}after:
a{rotate:0deg}c{color:#0008;background:#03020180}lightningcss 1.33.0 on the same input:
a{rotate:0deg}andc{color:#000;...}(it shares the alpha bug).[review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file