Conversation
…on's range In the relative color syntax every channel keyword is a <number> in the range of the function it is used in: r/g/b are 0..255 in rgb(), s/l and lab()/lch() l are 0..100, alpha and color() channels 0..1. The relative component parser handed out the unit values the colorspace structs store instead (r = 0.78 for 200, l = 0.5 for 50%) and typed every keyword and calc() result as a percentage, so rgb(from c calc(r + 10) g b) clamped to 255 and hsl()/lab() calc(l + 10) produced a NaN channel. Each function now records the range of its channels on the relative parser, keywords resolve to value * range, and the channels written as percentages divide the numbers they parse by that range again. The percentage pass (what folds min(r, g), and calc(l + 10%)) is unchanged.
|
Warning Review limit reached
Next review available in: 1 minute 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 a maintainer. Fix and tests are in, review findings are addressed, CI is green on every lane that has run.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a substantive redesign of relative-color channel resolution — removing ChannelType::PERCENTAGE, threading per-function ranges through the parser, and shifting output for a whole class of inputs — a human look would still be worthwhile, especially given the stated interactions with #38513/#38486/#38487.
Checked: CT_PCT/PERCENTAGE have no remaining references; parse_rgb_components/parse_hsl_hwb_components callers in custom.rs are covered by the / var(--a) tests; parse_alpha goes through parse_number_or_percentage which now returns Number for keywords (alpha range is 1, so unscaled); the legacy comma-syntax rgb() path still uses the untouched ComponentParser::parse_percentage.
Extended reasoning...
Overview
This PR reworks how CSS relative-color channel keywords resolve in src/css/values/color.rs. It adds a ranges: (f32, f32, f32) field to RelativeComponentParser, introduces per-function range constants (RGB_CHANNEL_RANGE 255, HSL_CHANNEL_RANGE 100, LAB_LIGHTNESS_RANGE 100, OKLAB_LIGHTNESS_RANGE 1), threads an l_range argument through parse_lab/parse_lch, replaces get_ident with a (value, range) pair plus a get_number that multiplies them, adds ComponentParser::parse_unit_channel, and removes ChannelType::PERCENTAGE (every non-hue channel becomes NUMBER). parse_angle_or_number's Calc<Angle> pass now emits Calc::Number for non-hue keywords instead of wrapping them as degrees. 89 new test cases across css.test.ts and color.test.ts plus a docs fix.
Security risks
None. Pure parser/constant-folding logic over CSS color literals; no I/O, no allocation lifetime changes, no user-controlled paths or exec.
Level of scrutiny
Medium-high. The change is well-scoped to one file but is a semantic redesign, not a mechanical fix: it changes bundler output for many relative-color spellings, deletes an enum variant used across every define_colorspace! entry, and deliberately flips one previously-accepted input (lch(from c l c calc(c + 10deg))) to rejected. A maintainer should confirm the ranges-on-the-parser approach and the PERCENTAGE-removal are the design they want, particularly since #38513 is described as re-typing the same fallback pass.
Other factors
Test coverage is thorough (every color function, converted origins, light-dark(), missing channels, keyword-as-alpha, the custom.rs / var(--a) path, hue typing rules, and the min()/max() percentage-pass fallback), and every asserted value is stated to match lightningcss 1.30.2. I confirmed CT_PCT/PERCENTAGE have no remaining references anywhere under src/css, that custom.rs's calls to parse_rgb_components/parse_hsl_hwb_components are exercised by the new tests, and that alpha (range 1) and the legacy comma-syntax rgb() percentage path are unaffected. What keeps this from auto-approval is scope: it's a ~250-line parser semantics change with documented cross-PR interactions, not a simple/obvious fix.
|
Heads-up: #38513 (relative color |
parse_lab/parse_lch take l_basis like lightningcss 1.30 does, the factor on RelativeComponentParser is percent_basis, and the lightness values are passed as literals at the call sites. No behavior change. Drops the hsl() lightness clamping case from the tests; it is not what this change is about.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 8:05 PM PT - Aug 14th, 2026
❌ @robobun, your commit 9ecdaf2 has some failures in 🧪 To try this PR locally: bunx bun-pr 38553That installs a local version of the PR into your bun-38553 --bun |
|
Self-review done; three follow-up commits since the first push, no behavior change in any of them:
CI on the intermediate commits was cancelled by the later pushes (all finished lanes were green); the run for the current head is in progress. |
…ge pass The second pass substituted the stored component for a keyword, which compared lab's l (stored as a unit value) against a and b (stored as numbers) on different scales once every channel was admitted there: lab(from lab(50% 20 30) min(l, a) a b) folded to 50% instead of 20%, and max(a, b) in the lightness slot to 3000%. alpha had the same problem against r or s before this branch. The pass now takes the basis of the channel being parsed and substitutes number / basis, so every keyword is compared as the number CSS Color 5 defines it as.
|
be7ca3d fixes the finding from the line review above: the second (percentage) pass substituted a keyword's stored component, which compared channels with different bases on different scales. It now substitutes |
There was a problem hiding this comment.
The mixed-scale min(l, a) regression I flagged earlier is fixed in be7ca3d — parse_percentage now goes through get_number and divides by the position's basis, and the exact repros (lab min(l,a), max(a,b), lch min(l,c)) are asserted in both test files. I re-checked the other parse_percentage callers with the new scaling (rgb min(r, alpha), hsl min(s, alpha), alpha min(alpha, r)) and they all reduce to the test-asserted values. No further issues found; a human look is still worthwhile given the storage-vs-basis design divergence from upstream and the sequencing with #38513/#38487/#33047.
Extended reasoning...
Overview
This PR rewires relative-color channel keyword resolution in src/css/values/color.rs: drops ChannelType::PERCENTAGE, retypes every non-hue channel as NUMBER, adds percent_basis to RelativeComponentParser (set by parse_rgb_components/parse_hsl_hwb_components/parse_lab/parse_lch), introduces parse_unit_channel, and rewrites parse_number_or_percentage/parse_percentage so keywords resolve to component * percent_basis — the CSS-range <number> the spec says they are. 88 test cases across css.test.ts and color.test.ts cover every function, converted/light-dark origins, alpha-as-channel, the / var() path, hue typing, and the min/max second pass; docs example is corrected.
Follow-up on prior review
My earlier review found that the rewritten parse_percentage wrapped the raw stored component (discarding percent_basis) as a Percentage, so lab's unit-scale l (0.5) and raw-scale a (20) were compared directly and min(l, a) folded to 50% instead of 20%. Commit be7ca3d replaces that with get_number(ident) / percent_basis, which puts every keyword on the same scale before comparison. I traced the fix through every caller: for lab lightness (basis 100) l→50/100, a→20/100, min→0.2 → 20%; for rgb channel (basis 255) r→200/255, alpha→1/255, min→1/255 → #1; for alpha (basis 1) alpha→1, r→200, min→1 → opaque. All match the newly-added assertions.
Security risks
None. This is deterministic float arithmetic on parsed CSS tokens; no I/O, no allocation of untrusted sizes, no external effects. Worst case is a wrong color value or a declaration passed through unfolded.
Level of scrutiny
Medium-high. The logic itself is contained and the test matrix is thorough (validated byte-for-byte against lightningcss 1.30.2), but this deliberately diverges from upstream's fix shape (upstream moved struct storage to CSS ranges; this keeps Bun's unit-value storage and carries the basis on the parser instead). That is a design choice with downstream consequences for #38513 (stacked on this, retypes the same second pass), #38487 (one-line srgb-linear fix that becomes (CT_NUM, CT_NUM, CT_NUM) after this), and the out-of-gamut policy in #33047 (which is why non-relative <number> lightness is gated behind from here). A maintainer should confirm the sequencing and that keeping unit-value storage is the intended path.
Other factors
The two remaining comment-cop flags (lines ~1495 and ~2181) are on 2- and 4-line doc comments explaining why alpha/color() use basis 1 and how the second pass compares mixed-scale keywords — both are the kind of non-obvious semantics REVIEW.md says to keep. The author already trimmed the longer comments in 665179f/0a4db974.
There was a problem hiding this comment.
I re-reviewed after be7ca3d and didn't find any further bugs — the parse_percentage scaling issue I flagged earlier is fixed correctly (keywords now compare as number / percent_basis of the channel being parsed), and the added min(l, a) / max(a, b) / min(r, alpha) tests cover it. Given this is a ~240-line rework of the relative color parser's channel-typing model with stated interactions across #38513, #38487 and #33047, a human sign-off from someone familiar with the CSS subsystem would still be worthwhile.
What was reviewed: the percent_basis plumbing through every color function (rgb 255, hsl/hwb 100, lab/lch l_basis, color()/alpha 1); the ChannelType::PERCENTAGE removal and every define_colorspace! retyping; the angle-calc pass now returning Calc::Number for non-hue keywords; and that ComponentParser::parse_percentage (legacy rgb% path) is only reached when from.is_none() so relative inputs never hit it.
Extended reasoning...
Overview
This PR fixes CSS Color 5 relative color syntax so channel keywords (r, s, l, etc.) resolve as <number>s in the target function's range instead of Bun's internal unit-value storage. It touches src/css/values/color.rs (~130 added / ~110 removed): adds a percent_basis field to RelativeComponentParser, sets it per-function in parse_rgb_components / parse_hsl_hwb_components / parse_lab / parse_lch, retypes every non-hue channel from CT_PCT to CT_NUM, replaces RelativeComponentParser::parse_number_or_percentage and parse_percentage with a single get_number-based path, and adds ComponentParser::parse_unit_channel. 96 new test cases across two files, plus a docs fix for an example that was emitting lch(none ...).
Prior finding addressed
My earlier review found that the rewritten parse_percentage compared mixed-basis keywords on incompatible scales (min(l, a) in lab folded to 50% instead of 20%). The fix in be7ca3d threads the channel's percent_basis into the pass and substitutes get_number(ident) / percent_basis, so all keywords are on the same scale before comparison. I traced the six new mixed-basis test cases through the code and they fold correctly; the same-basis cases (min(r, g), calc(l + 10%)) are byte-identical to before as claimed.
Security risks
None. This is pure CSS value parsing with no I/O, allocation from untrusted sizes, or path handling. Inputs that don't fold are passed through unparsed for the browser.
Level of scrutiny
Moderate-to-high. This is production bundler output — bun build on any stylesheet using relative color syntax — and Bun.color(). The change is not mechanical: it swaps the channel-type model (removing PERCENTAGE entirely), introduces a scaling factor that must be set correctly at every call site, and deliberately diverges from upstream's storage-model fix while aiming for identical output. The PR body enumerates every probe whose output changes and every one that must not; the test suite pins both.
Other factors
The PR body explicitly names three interacting PRs: #38513 (stacked on this branch, retypes the same parse_percentage pass), #38487 (srgb-linear r channel type — this PR rewrites the same line but preserves the (sic) bug), and #33047 (out-of-gamut folding, which is why non-relative <number> lightness is deliberately gated out). A maintainer familiar with the CSS subsystem should confirm the landing order and that the percent_basis approach (vs. upstream's storage change) is the direction they want, since it's called out as a divergence that a later PR may delete. The comment-cop threads are all resolved; the two remaining doc comments are justified in-thread.
|
Heads up on overlap: #39205 adds the non relative |
This branch has not been deployed
1 participant
Problem
calc(r * 2)and bare keywords happen to come out right, which is why this went unnoticed since the CSS parser landed. No user has reported it; it was found by diffingBun.coloragainst lightningcss. It does reach users: the idiom in our own docs,lch(from purple calc(l + 15) c h), is emitted into the stylesheet aslch(none 66.8 327)(anonelightness, i.e. black) bybun buildon 1.4.0, and every browser has shipped this syntax since 2024.<number>in the range of the function being written:r/g/bare 0..255 inrgb(),s/l(andhwb()w/b) andlab()/lch()lare 0..100,oklab()/oklch()l,color()channels andalphaare 0..1 (https://drafts.csswg.org/css-color-5/#relative-colors).src/css/values/color.rs:RelativeComponentParser::newcopies the origin's components as the colorspace structs store them (unit values:ris 0.784 for 200,lis 0.5 for 50%), thedefine_colorspace!tables type those channels as percentages, andRelativeComponentParser::parse_number_or_percentagereturns a keyword orcalc()result asNumberOrPercentage::Percentage. Socalc(r + 10)is 0.784 + 10 read as a percentage, andhsl()/lab()lightness goes throughparse_percentage, where keyword + number ends inPercentage::from_calcreturning NaN. The same typing also rejected spec-valid positions: a number literal fors/l, anda/b/alphaanywhere a<number>is accepted (lab(from c a b l),hsl(from c s s l)).Fix
rgb(),hsl()andlab()channels in their CSS ranges, so a keyword is simply the stored value. Bun's structs store unit values (andBun.color, color-mix and the serializers are built on that), so this PR keeps the storage and carries upstream's percent basis on the relative parser instead:RelativeComponentParsergainspercent_basis, set by the function being parsed (255 inparse_rgb_components, 100 inparse_hsl_hwb_components, the newl_basisargument ofparse_lab/parse_lch, 100 or 1, which is upstream's argument of the same name;color()keeps 1). The fixing line isget_number: a keyword resolves tocomponent * percent_basis, andparse_identand thecalc()callback use it.ComponentParser::parse_number_or_percentagereturns those asNumber, whichrgb()already divides by 255 andcolor()/alpha already use as is. If the structs ever move to CSS-range storage like upstream,percent_basisbecomes 1 everywhere and is deleted.hsl()/hwb()second and third channel,lab()/lch()/oklab()/oklch()lightness) go through the newComponentParser::parse_unit_channel(basis): in relative syntax it parses a<number>(keyword,calc(), or literal) and divides by the basis, otherwise a<percentage>exactly as before. css-color-4 also allows a plain<number>there outside relative syntax (lab(50 20 30),labnot supported in Bun.color #16727). That is deliberately not added here: the out-of-gamut origins intest/bundler/css/wpt/relative_color_out_of_gamut.test.tsare currently left unfolded only becauselab(100 104.3 -50.9)does not parse, and making it parse without also changing how out-of-gamut origins are folded produces the#fffoutputs that were rejected in Bun.color: return null for unconvertible colors, fix hsl/lab output, lab() and named-color parsing #33047's review. The doc comment onparse_unit_channelsays so; dropping itsfromgate is the one-line change for whichever PR lands the gamut policy.ChannelType::PERCENTAGEis gone (upstream's tables have the same shape): every non-hue channel andalphaisNUMBER, hues stayANGLE, so a keyword is accepted wherever the function takes a<number>and the hue keyword only where it takes a hue. In the<angle>calc pass a non-hue keyword is now aCalc::Numberinstead of being read as degrees (also upstream's rule), socalc(s * 1deg)works andcalc(s + 30deg)is rejected; the only inputs that folded before and are rejected now arelab()a/bandlch()cused inside an anglecalc(), e.g.lch(from c l c calc(c + 10deg)).RelativeComponentParser::parse_percentage, what foldsmin(r, g), and what givescalc(l + 10%)its meaning) now takes the basis of the channel being parsed and substitutesnumber / basisfor a keyword, so everything in it is compared as the number CSS Color 5 defines. Main substituted the stored component, which was only right while every keyword in the expression had the same basis as the channel:rgb(from c min(r, alpha) g b)folded to min(0.784, 1), i.e. 200, instead of min(200, 1), and oncea/b/care admitted there (they are numbers now)lab(from lab(50% 20 30) min(l, a) a b)would have folded to 50% instead of 20% (caught in review). Same-basis expressions are byte-identical to before. Upstream types the keywords as numbers in this pass instead; css: parse calc(<channel> * <percentage>) in relative colors #38513 ports that (it also needsPercentage::from_calcto reject a number, otherwise the keyword + percentage spellings turn intononechannels), which folds the samemin()/max()cases to the same values in the number pass and decides the fate of the+ 10%spellings. The two PRs edit the same function, so whichever lands second has a small rebase; both orders end at upstream's shape.srgb-linearrchannel type. This PR rewrites the sametypes = ...line (theCT_PCTconstant it uses no longer exists) but leavesras the(sic)angle it is on main, so that fix is still needed and becomes(CT_NUM, CT_NUM, CT_NUM)after this. css: keep rotate: 0deg out of none, and keep the origin alpha in relative colors #38486 (origin alpha) is unaffected: alpha is not scaled.calc(h + 30), and themin()/max()folds, which bun folded before this change too). Pass-through outputs do not move: the basis multiplies stored values thatrgb()rounds and the other functions divide straight back, and the relative color WPT expectations pass unchanged.docs/bundler/css.mdxusedcalc(l + 15%), which browsers reject; it now uses the spec spelling this change makes work, and both output lines are whatbun buildprints (thevar()line is passed through; it was shown as computed before).bun bd test test/js/bun/css/css.test.ts(newrelative colorsblock, 66 cases through the minifier: every function, converted andlight-dark()origins, a missing channel, keywords used as alpha and alpha as a channel, the/ var(--a)path insrc/css/properties/custom.rs, the hue rules, themin()/max()pass) andbun bd test test/js/bun/css/color.test.ts(30 cases throughBun.color, including the lines above). Without thesrc/change 37 of the 66 and 18 of the 30 fail on the released binary; the rest pin behavior that must not move. With it both files pass, and so dotest/bundler/css/(including the relative color WPT file) and the rest oftest/js/bun/css/, apart from the 20k-rule case innested-vendor-prefix-duplication.test.ts, which has no colors in it and sits at its 5 s budget on this loaded machine (css: parse calc(<channel> * <percentage>) in relative colors #38513 measured it at 4.9 s on main too), andcss-fuzz.test.ts, which is skipped in CI.Background
rgb(from <origin> r g b / alpha)converts the origin color into the function's color space and lets each channel be written as a keyword naming one of the converted channels, acalc()over them, or a literal.100%stands for, 255 for anrgb()channel, 100 forhsl()saturation orlab()lightness, 1 for alpha. It is also the factor between Bun's stored unit value and the<number>CSS uses for the channel, which is what the relative parser needs.ComponentParserparses one channel of any color function; when afromorigin is present it first asks itsRelativeComponentParser(built from the converted origin) to resolve keywords and keywordcalc()s, and falls back to literal parsing.parse_rgb_componentsandparse_hsl_hwb_componentsare shared withcustom.rs, which handlesrgb(... / var(--x))by resolving the channels and keeping the alpha as tokens.Calc<f32>evaluates a math function whose terms are numbers, substituting keywords through a callback;Calc<Percentage>does the same with percentage terms and is the second pass described above.Every probe whose output changes (Bun.color(x, "css"); the right column is also lightningcss 1.30.2's answer)
Unchanged, as intended: every bare keyword pass-through,
calc(r * 2),calc(h + 30),calc(h + 30deg),color()in every space, every alpha written as a number or percentage,min(r, g),calc(l + 10%), and the hue keyword being rejected outside hue positions.[review] gate passed · iteration 2 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 2
evidence per changed file