Conversation
|
Status: reproduced on bun 1.4.0 and main with Fix is in CI on the previous revision: the new tests passed on every lane; the only red was the alpine x64 lane, where |
|
Updated 6:11 PM PT - Aug 15th, 2026
✅ @robobun, your commit 33b5aef3b669a6dea9ae1a42a89d3d6f4c4f8cee passed in 🧪 To try this PR locally: bunx bun-pr 39205That installs a local version of the PR into your bun-39205 --bun |
|
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 (3)
WalkthroughChangesModern space-separated HSL/HWB numeric channel support
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — small, spec-cited parser extension with thorough coverage.
What was reviewed:
parse_hsl_hwb_channelfollows the existingtry_parse(CSSNumberFns::parse)-then-fallback pattern already used for rgb() and angle-or-number in the same file;try_parserestores position so percentage/none/relative-keyword paths are unchanged.- Legacy comma syntax stays percentage-only (gated on the existing
is_legacy_syntaxflag), andparse_percentageitself is untouched sorgb(10%, 20, 30)stays rejected — both pinned by tests. - Checked both callers (
parse_hsl_hwbandUnresolvedColor::parsein custom.rs); the/100.0normalization matches the unit valueparse_percentagealready returns, so clamping/serialization/downleveling are shared. - ~100 new test cases across
Bun.colorand the minifier: number/mixed spellings,calc(),none, out-of-range clamping paired with the percentage spelling, relative colors,color-mix(),var()alpha, and the invalid legacy forms.
Extended reasoning...
Overview
This PR extends parse_hsl_hwb_components in src/css/values/color.rs to accept the CSS Color 4 <number> form for the saturation/lightness channels of hsl() and the whiteness/blackness channels of hwb() in the modern (space-separated) syntax. The runtime change is 20 lines: a new parse_hsl_hwb_channel helper that, when not in legacy comma syntax, first tries input.try_parse(CSSNumberFns::parse) and stores the result divided by 100, then falls back to the existing parse_percentage. The two call sites in parse_hsl_hwb_components are updated to use it. The remaining ~180 diff lines are new tests in test/js/bun/css/color.test.ts and test/js/bun/css/css.test.ts.
Security risks
None. This is pure-Rust CSS token parsing with no allocation sizing, no FFI, and no unsafe blocks touched. The parsed number is immediately clamped to [0.0, 1.0] by the caller exactly as the percentage path already was, so out-of-range/NaN handling is unchanged.
Level of scrutiny
Low-to-medium. The runtime change is tiny, strictly additive (only inputs that previously failed to parse now succeed), and mirrors an established idiom in the same file — parse_number_or_percentage at line ~1962 and parse_angle_or_number at line ~1992 both do the same try_parse(CSSNumberFns::parse) followed by a fallback, so the calc()-rewind concern the PR description raises is already exercised on every rgb() call. I traced both callers of parse_hsl_hwb_components (parse_hsl_hwb in color.rs and UnresolvedColor::parse in properties/custom.rs) and both benefit from the change without any signature or semantic drift; the returned value is the same unit fraction the percentage path produces, so downstream clamping, hwb w+b normalization, serialization, and downleveling are shared.
Other factors
Test coverage is exceptionally thorough for a change this size: it covers the number form, mixed number/percentage, hsla(), alpha slash, none, decimals, exponent notation, calc() of both numbers and percentages, out-of-range values paired against their percentage equivalents, relative color syntax (both as the outer function and as the from origin), color-mix(), gradients, custom properties, var() fallbacks, the unresolved-alpha path, a prefix_test for downleveling, and negative pins on the legacy comma forms and rgb(10%, 20, 30). The PR description cites the spec sections, browser support, and lightningcss parity, and explicitly reasons through why previously-valid inputs cannot change path. No prior human review comments to address; CI is building.
There was a problem hiding this comment.
LGTM — small, spec-cited parser extension with the same try-number-then-percentage shape already used for rgb() and angle channels in this file.
What was reviewed:
parse_hsl_hwb_channelusestry_parse, so a percentage/none/relative keyword that fails the number attempt is not consumed; falls through to the unchangedparse_percentagepath.- Legacy comma syntax stays percentage-only (gated on the existing
is_legacy_syntax), and thecustom.rscaller passesallows_legacy=falseso thevar(--a)alpha path also gets numbers. CSSNumberFnswas already imported; the number-then-percentage fallback matches lines ~1962/1992 in the same file, socalc()recovery behaves as it does for rgb() channels.
Extended reasoning...
Overview
The PR extends CSS Color 4 parsing so hsl()/hwb() accept bare numbers for saturation/lightness and whiteness/blackness in the modern space-separated syntax, per https://www.w3.org/TR/css-color-4/#the-hsl-notation. The Rust change is two call sites in parse_hsl_hwb_components redirected to a new 12-line helper parse_hsl_hwb_channel, which does input.try_parse(CSSNumberFns::parse) (dividing by 100) before falling back to the existing ComponentParser::parse_percentage. The rest is ~140 new test cases across color.test.ts (Bun.color) and css.test.ts (minifier + downleveling).
Security risks
None. This is a widening of an existing CSS token parser to accept a number where only a percentage was accepted. No allocation sizing, no path handling, no untrusted length arithmetic. The parsed value is immediately .clamp(0.0, 1.0) as before, so out-of-range and non-finite inputs cannot escape into later stages.
Level of scrutiny
Low-to-medium. The change is mechanical and mirrors an established pattern in the same file: parse_number_or_percentage (line ~1962) and parse_angle_or_number (line ~1992) already do try_parse(CSSNumberFns::parse) followed by a fallback, so the try_parse-resets-on-failure and calc()-recovery semantics are already relied on here. The legacy-syntax gate reuses the existing is_legacy_syntax boolean unchanged, and the second caller in properties/custom.rs passes allows_legacy=false, so it correctly picks up the number path without needing edits.
Other factors
Test coverage is unusually thorough: number/percentage/mixed spellings, none, calc() of numbers and percentages, scientific notation, out-of-range clamping compared side-by-side with the percentage spelling, relative-color origins and channel keywords, color-mix(), custom properties, var() fallbacks, gradients, downleveling via prefix_test, and pinning tests that the legacy comma form and rgb(10%, 20, 30) stay rejected. The PR description states 41+38 of these fail on the released binary, satisfying the "fails without the fix" requirement. CSSNumberFns is already in scope (line 14), so no new imports. No outstanding reviewer comments.
In the modern syntax the saturation/lightness of hsl() and the whiteness/blackness of hwb() are <percentage> | <number> | none, with the number standing for that many percent, so hsl(120 50 40) is hsl(120 50% 40%). parse_hsl_hwb_components only accepted percentages, so Bun.color() returned null for these and the bundler passed them through as unknown tokens. The channels now go through ComponentParser::parse_unit_channel, which reads a <number> divided by the channel's percent basis and otherwise takes the percentage path. The legacy comma syntax still takes only percentages.
6976217 to
33b5aef
Compare
There was a problem hiding this comment.
LGTM — small, spec-cited parser extension that follows the existing try_parse(CSSNumberFns::parse) → fallthrough pattern already used in parse_number_or_percentage and parse_angle_or_number.
What was reviewed:
parse_unit_channelordering: number literal fails onnone/relative keywords (identifiers) andtry_parserewinds, soparse_percentagestill handles them — confirmed by thehsl(120 none 40)andhsl(from red h 50 l)cases.- Legacy comma syntax stays percentage-only via the existing
is_legacy_syntaxgate;parse_percentageitself is untouched sorgb(10%, 20, 30)still rejects. .clamp(0.0, 1.0)on the divided number matches the percentage path's storage, so downstream (hwb normalization, serialization, downleveling) is shared unchanged.
Extended reasoning...
Overview
The PR changes parse_hsl_hwb_components in src/css/values/color.rs to accept <number> (in addition to <percentage> and none) for the saturation/lightness channels of hsl() and whiteness/blackness of hwb() in the modern space-separated syntax, per CSS Color 4. The Rust change is ~20 lines: a new parse_unit_channel helper that tries CSSNumberFns::parse first (dividing by 100 to match the unit-value storage), then falls through to the existing parse_percentage. The legacy comma syntax keeps the percentage-only path. Two test files add ~100 cases covering both callers (Bun.color and the CSS minifier).
Security risks
None. This is pure CSS token parsing with no allocation, no unsafe, no FFI, and no user-controlled sizes or paths. Worst case for a bad input is a parse error (color left as unresolved tokens), which is the pre-existing behavior.
Level of scrutiny
Low-to-medium. The change is mechanical and mirrors two adjacent functions in the same impl block (parse_number_or_percentage at line 1972 and parse_angle_or_number at line 2002) that already do input.try_parse(CSSNumberFns::parse) followed by a fallthrough — so the try_parse rewind semantics and the calc() interaction the PR description discusses are already exercised by existing code paths. The stored value (number / 100.0) is exactly what parse_percentage produces for the equivalent percentage, so nothing downstream (clamping, NaN/none handling, hwb w+b normalization, serialization, prefix downleveling) needed to change.
Other factors
- Test coverage is thorough: number/percentage/mixed spellings,
nonein each slot, decimals, scientific notation,calc()of numbers and percentages, out-of-range clamping paired with the percentage spelling, relative-color origins,color-mix(), custom properties,var()fallback, gradient, theUnresolvedColor(/ var(--a)) path, and aprefix_testfor downleveling. Negative cases pin that legacy-commahsl(),hwb()with commas, andrgb(10%, 20, 30)stay rejected. - The PR description cross-checks every expected value against both the percentage spelling and lightningcss 1.30 output, and verified 41+38 of the new cases fail on the released binary (so the tests are load-bearing).
- CI is green across lanes per the status comment.
- No prior human review comments to address; no CODEOWNERS on this path.
|
#39200 (the lab-family counterpart) now uses this exact |
Problem
hsl()andhwb()do not parse when the saturation/lightness (whiteness/blackness) are written as numbers:Bun.color("hsl(120 50 40)", "hex")isnullwhile"hsl(120 50% 40%)"is"#339933";Bun.color("hwb(120 20 30)", "hex")isnullwhile the percentage spelling is"#33b333".bun buildas unknown tokens:a{color:hsl(120 50 40)}is emitted as written instead of#393, so it is not minified, gets no fallback for older targets, and cannot be the origin of a relative color or an operand ofcolor-mix().hsl(120 50 40 / var(--a))is left alone too.<percentage> | <number> | nonein the modern (space separated) syntax, the number meaning that many percent; the legacy comma syntax takes only percentages (https://www.w3.org/TR/css-color-4/#the-hsl-notation, https://www.w3.org/TR/css-color-4/#the-hwb-notation). Chrome 111, Safari 16.4 and Firefox 113 accept the number form, and lightningcss 1.30 minifies it to#393/#33b333.parse_hsl_hwb_componentsinsrc/css/values/color.rsreads both channels withComponentParser::parse_percentage, which accepts a percentage token,noneor a relative color keyword, never a number.color.rsthat read a<percentage> | <number> | nonechannel withparse_percentage. The other two are the lightness ofparse_labandparse_lch, which css: parse the number lightness and percentage a/b/chroma of lab(), lch(), oklab() and oklch() #39200 fixes (their a/b/chroma have the mirror image gap, a missing percentage form, also in css: parse the number lightness and percentage a/b/chroma of lab(), lch(), oklab() and oklch() #39200).rgb()andcolor()already go throughparse_number_or_percentage.Fix
ComponentParser::parse_unit_channel(input, percent_basis)reads such a channel: a<number>(CSSNumberFns::parse, socalc(25 * 2)works as well) divided by the basis, otherwise the existingparse_percentagepath.parse_hsl_hwb_componentscalls it withHSL_PERCENT_BASIS(100) in the modern syntax and keeps callingparse_percentagein the legacy syntax, which the existing comma check already identifies.none, thehwb()w + b normalization, serialization and downleveling are shared with the percentage spelling. Every expected value in the new tests is what the percentage spelling already produced, and byte for byte what lightningcss 1.30.2 emits for the number spelling./ 100: the lab family needs the same function with bases of 100 (lab(),lch()) and 1 (oklab(),oklch()), which is also how lightningcss structures it. css: parse the number lightness and percentage a/b/chroma of lab(), lch(), oklab() and oklch() #39200 currently adds its own copy (parse_lab_lightness, same body) and css: resolve relative color channel keywords as numbers in the function's range #38553 adds one under this name and signature for the relative color case; with this helper on main, css: parse the number lightness and percentage a/b/chroma of lab(), lch(), oklab() and oklch() #39200's rebase replaces its copy withparse_unit_channel(i, l_basis), and css: resolve relative color channel keywords as numbers in the function's range #38553's adds its relative keyword branch in front of the number branch here and drops its own copy and its ownHSL_PERCENT_BASIS. If either of them lands first instead, this PR rebases onto its helper the same way. None of the three PRs depends on the others to be correct; this one is the smallest and has no gamut coupling (the number spelled colors are in gamut by construction), which is why it is kept separate rather than folded into css: parse the number lightness and percentage a/b/chroma of lab(), lch(), oklab() and oklch() #39200.hsl(120, 50, 40)andhsla(120, 50, 40, .5)stay rejected as the spec and browsers require (lightningcss 1.30 accepts them; resolving a declaration that browsers drop would change what the page paints, so the tests pin the rejection).hwb()has no legacy syntax, sohwb(120, 20, 30)stays rejected.parse_percentageitself is unchanged, so the legacyrgb(10%, 20%, 30%)form, which also uses it, still rejectsrgb(10%, 20, 30).noneor a keyword is not a number, so the number attempt fails without consuming input.calc()is the only token both attempts descend into; a failedcalc()is not recorded as unrecoverable (only the type independent functions such asatan2()are), and thergb()channels have always tried number then percentage the same way.hsl()/hwb()take the same channel grammar:hsl(from red h 50 l)now resolves (#bf4040, as in lightningcss). That andhwb(from red h 20 b)are the only rows that overlap with css: resolve relative color channel keywords as numbers in the function's range #38553's tests. One interaction to be aware of: a number spelled origin such ashsl(from hsl(120 50 40) h calc(s + 10) l)now parses and therefore reaches the keyword arithmetic bug css: resolve relative color channel keywords as numbers in the function's range #38553 fixes, exactly as the percentage spelled origin already does today.parse_color_function(Bun.color, stylesheet colors, relative color origins,color-mix(), token lists such as custom properties andvar()fallbacks) andUnresolvedColor::parseinsrc/css/properties/custom.rs(hsl(... / var(--a))), which now emitshsl(120 50% 40%/var(--a)), orhsla(120, 50%, 40%, var(--a))for old targets, exactly like the percentage spelling.test/js/bun/css/color.test.ts(Bun.color: number and mixed spellings,hsla(), alpha,none,calc(), out of range values next to their percentage spellings, relative colors,color-mix(), the spellings that stay invalid) andtest/js/bun/css/css.test.ts(the same through the minifier, plus custom properties, avar()fallback, a gradient, thevar(--a)alpha path and a downlevelprefix_test). On the released binary 79 of the 102 new cases fail (41 of 53 and 38 of 49); the rest pin what must keep parsing or keep being rejected. With the change both files pass.test/js/bun/css/andtest/bundler/css/(including the WPT color and relative color files);cargo clippy -p bun_cssandcargo fmtare clean.Background
hsl(). The legacy one is comma separated (hsl(120, 50%, 40%)) and takes only percentages; the modern one is space separated (hsl(120 50% 40%),hsl(120 50 40), optional/ alpha) and takes percentages or numbers.hwb(), relative colors (hsl(from red h s l)) andhsla()written with spaces all use the modern grammar. The parser tells them apart by whether a comma follows the hue.HSL/HWBstructs store these channels as fractions of 1 (50%is 0.5), and every later stage (clamping, conversion toRGBA, printing) works on that. The percent basis of a channel is the<number>that means100%in its function, 100 here, so a number is stored asn / basisand becomes indistinguishable from the percentage spelling.ComponentParser: parses one channel of any color function. With afromorigin it first resolves channel keywords andcalc()of keywords against the origin, then falls back to a literal.parse_hsl_hwb_componentsis shared byparse_color_function(complete colors) andproperties/custom.rs, which uses it for anhsl()whose alpha is a token list it cannot resolve, such asvar(--a).Earlier revision
The first push implemented the number branch as a private
parse_hsl_hwb_channelfree function with the 100 hard coded. Review pointed out that this was a third copy of the helper #39200 and #38553 each add to the same file, so it was reshaped into the sharedComponentParser::parse_unit_channeldescribed above. The tests are unchanged.