Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/css/values/calc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1771,10 +1771,10 @@ impl CalcValue for Percentage {
fn into_calc(self) -> Calc<Self> {
Calc::Value(Box::new(self))
}
fn from_calc(c: Calc<Self>, _input: &mut css::Parser) -> CssResult<Self> {
fn from_calc(c: Calc<Self>, input: &mut css::Parser) -> CssResult<Self> {
match c {
Calc::Value(v) => Ok(*v),
_ => Ok(Percentage { v: f32::NAN }),
_ => Err(input.new_custom_error(css::ParserError::invalid_value)),
}
}
#[inline]
Expand Down
29 changes: 20 additions & 9 deletions src/css/values/color.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2184,18 +2184,29 @@ impl RelativeComponentParser {
Err(input.new_error_for_next_token())
}

/// A math function the `<number>` pass could not fold, evaluated as a
/// `<percentage>` and returned as a unit value. A keyword is its channel as
/// a percentage of the channel's range here, which is what folds
/// `min(r, g)` (`reduce_args` only compares values) and what gives
/// `calc(l + 10%)` its pre-existing meaning. Callers try `parse_number`
/// first, so a math function over plain numbers gets the CSS Color 5
/// meaning, in which the keywords are `<number>`s.
/// A `<percentage>` math function over the channel keywords, as a unit value.
fn parse_percentage(input: &mut css::Parser, this: &RelativeComponentParser) -> CssResult<f32> {
match Calc::<Percentage>::parse_with(input, this, |ctx, ident| {
if let Ok(unit_value) = input.try_parse(|i| {
RelativeComponentParser::parse_percentage_calc(i, this, |ctx, ident| {
Some(Calc::Number(ctx.get_number(ident, ChannelType::NUMBER)?))
})
}) {
return Ok(unit_value);
}

// Keyword as a percentage of its range: `calc(l + 10%)`, `min(r, g)`.
RelativeComponentParser::parse_percentage_calc(input, this, |ctx, ident| {
let (unit_value, _) = ctx.get_ident(ident, ChannelType::NUMBER)?;
Some(Calc::Value(Box::new(Percentage { v: unit_value })))
}) {
})
Comment on lines +2202 to +2205

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 This fallback wraps each keyword's stored component as Percentage{v: unit_value} and discards its range, so in lab()/lch() — where l has range 100 but a/b/c have range 1 — lab(from lab(50% 20 30) min(l, a) a b) compares 0.5 vs 20 and folds to lab(50% 20 30) instead of the spec-correct lab(20% 20 30); before this PR a was CT_NUM, didn't match ChannelType::PERCENTAGE, and the color was left unparsed for the browser to compute correctly. Separately, the description's reduce_args bullet (Number↔Number folding, min(r, .5), line-height: min(1.5, 1.2), min(NaN, .5), min(10px, 1.5)) and its tests are not in the diff — restoring that arm would fold min(l, a) correctly in the Calc<f32> pass and make this fallback unreachable for keyword-only min/max; otherwise this pass should compare range-scaled values (or reject when the arguments' ranges differ) and the description/table should be updated.

Extended reasoning...

What the bug is

The second-pass fallback of RelativeComponentParser::parse_percentage (color.rs:2202-2205) resolves each channel keyword via get_ident, takes the stored component (unit_value), discards the returned range, and wraps it as Calc::Value(Percentage{v: unit_value}). This is what lets min(r, g) fold when reduce_args cannot compare Calc::Numbers, and it is only correct when every keyword in the expression has the same range. This PR introduces per-channel ranges, and in lab()/lch() lightness has range 100 while a/b/c (and alpha) have range 1 — so min(l, a) compares 0.5 against 20 and picks the wrong argument.

Code path

For lab(from lab(50% 20 30) min(l, a) a b), the lightness position calls parse_unit_channel(i, 100). parse_lab sets from.ranges = (100, 1, 1); the origin stores components = (0.5, 20, 30, 1.0).

  1. self.parse_number → parse_calc<f32> (color.rs:2234-2246): l → get_number → 0.5×100 = Number(50); a → 20×1 = Number(20). reduce_args (calc.rs:1090) only matches if let Calc::Value(val) = &*arg, so both Numbers survive → Calc::Function(Min) → rejected (only Calc::Value/Calc::Number accepted at 2241-2246).
  2. parse_percentage pass 1 (color.rs:2196-2200, keyword→Calc::Number(get_number(...))): identical [Number(50), Number(20)] → Function → parse_percentage_calc (2214) only accepts Calc::Value → rejected.
  3. parse_percentage pass 2 (color.rs:2202-2205): l → get_ident → (0.5, 100), discards 100, wraps 0.5; a → get_ident → (20, 1), discards 1, wraps 20. reduce_args compares Percentage{0.5} vs Percentage{20}: pushes 0.5, then partial_cmp(20, 0.5) = Greater ≠ Less → drops 20. Returns Percentage{0.5} → Ok(0.5).
  4. parse_unit_channel returns 0.5 → lab(50% 20 30).

Why nothing else catches it

The Calc<f32> pass computes the correct numbers (50, 20) but cannot fold them because reduce_args has no Calc::Number arm. The first Calc<Percentage> pass has the same problem. Only the second pass folds, and it does so on the wrong axis. Every existing min/max-of-keywords test in the PR (min(r, g) in color()/rgb(), min(s, l) in hsl(), max(w, b) in hwb()) uses keywords whose ranges are equal, so stored-value comparison happens to give the same answer as range-scaled comparison.

Regression

Before this PR, LAB had types = (CT_PCT, CT_NUM, CT_NUM) and the old parse_percentage looked up keywords with ChannelType::PERCENTAGE, so a (CT_NUM) returned None from get_ident, the Calc<Percentage> pass errored, and the whole color fell back to unparsed tokens — the browser then computed min(50, 20) = 20 → lab(20% 20 30). After this PR, all channels are CT_NUM and the lookup uses ChannelType::NUMBER, so a resolves and bun emits a wrong constant — strictly worse than the pre-PR passthrough, and exactly the failure mode the PR description warns against for rgb() alpha. The same applies to lch(from ... min(l, c) c h). (min(l, alpha) was already wrong pre-PR since alpha matched PERCENTAGE; min(l, a)/min(l, b)/min(l, c) are newly broken.)

The stale reduce_args claim

The PR description's Fix section says "reduce_args: min()/max() also reduce Calc::Number arguments against each other … min(r, .5) folds too … line-height: min(1.5, 1.2) minifies to 1.2", and the Verified section lists min(r, .5), min(NaN, .5), min(10px, 1.5) as pinned tests. None of that is in the diff: the calc.rs hunk only touches Percentage::from_calc; reduce_args at ~1090 still has no Calc::Number arm; grep of test/js/bun/css/ finds none of those four cases; and the doc comment on the new parse_percentage ("min(r, g), which reduce_args only folds over values") confirms the author knows reduce_args was not changed. So min(r, .5) does not fold (both passes end with mixed [Number, Number] or [Value, Number] args), line-height: min(1.5, 1.2) stays min(1.5,1.2), and the before/after table's rows for both are wrong. The review timeline (claude[bot] at 13:59 references "the new reduce_args arms", then a fallback-pass redesign landed at 19:35) suggests the reduce_args change and its tests were dropped during the rework and the description was never updated (CLAUDE.md #11: "NEVER overstate what you got done").

Step-by-step proof

Input: lab(from lab(50% 20 30) min(l, a) a b).

Per CSS Color 5 the channel keywords are <number>s in the function's own range: l = 50, a = 20. min(50, 20) = 20. In the lightness position 20 means l = 20/100 = 0.2 stored → lab(20% 20 30). Chrome/Firefox and lightningcss (which leaves it unparsed) agree.

Bun after this PR: pass 1 → [Number(50), Number(20)], reduce_args pushes both → Function(Min) → rejected. Pass-2 Calc<Percentage> (Number keyword): same → rejected. Pass-2 fallback (Value keyword): [Value(Pct{0.5}), Value(Pct{20})]; reduce_args pushes 0.5, then compares 20 vs 0.5 → Greater, order is Less, so 20 is dropped. Reduced to [Value(0.5)] → returns 0.5 → lab(50% 20 30). Wrong.

How to fix

Two independently sufficient options:

  • Restore the dropped reduce_args change (a Calc::Number↔Calc::Number comparison arm, plus the four claimed tests). Then the Calc<f32> pass folds min(l, a) to Number(20), parse_calc accepts it, parse_unit_channel returns 20/100 = 0.2 → lab(20% 20 30), and the buggy fallback is never reached for keyword-only min/max. This also delivers what the description already claims (min(r, .5), line-height: min(1.5, 1.2)).
  • Or, keep reduce_args as-is, correct the description/table, and make this fallback compare on the range-scaled axis — e.g. wrap get_number(ident, ChannelType::NUMBER)? / <range of the position being parsed> as the Percentage value so all keywords land on the same scale — or simply reject (return None) when the arguments' ranges differ so the color falls back to unparsed tokens as before.

Either way, add lab(from lab(50% 20 30) min(l, a) a b) (and the lch analogue) to the test matrix.

}

fn parse_percentage_calc(
input: &mut css::Parser,
this: &RelativeComponentParser,
keyword: impl Fn(&RelativeComponentParser, &[u8]) -> Option<Calc<Percentage>> + Copy,
) -> CssResult<f32> {
match Calc::<Percentage>::parse_with(input, this, keyword) {
Ok(Calc::Value(v)) => Ok(v.v),
_ => Err(input.new_custom_error(css::ParserError::invalid_value)),
}
Expand Down
97 changes: 97 additions & 0 deletions test/js/bun/css/color.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,103 @@ describe("css string output parses back to the same color", () => {
});
});

// https://drafts.csswg.org/css-color-5/#relative-colors
// Inside a math function a channel keyword is a <number> in the function's
// range, so multiplying it by a <percentage> gives a <percentage> of the
// channel. Every value in the first three groups is lightningcss 1.30 output.
describe("relative color calc() with percentages", () => {
const origin = "color(srgb .8 .4 .2)";

test.each([
["calc(r * 50%)", ".4"],
["calc(50% * r)", ".4"],
["calc(r * -50%)", "-.4"],
["calc(r * 50% + 10%)", ".5"],
["calc(50% - r * 25%)", ".3"],
["calc(r / 2 * 100%)", ".4"],
["calc(r * g * 50%)", ".16"],
["calc((r + g) * 50%)", ".6"],
["min(r * 50%, 30%)", ".3"],
["max(r * 50%, 30%)", ".4"],
["round(r * 100%, 30%)", ".9"],
["mod(r * 100%, 30%)", ".2"],
["hypot(r * 100%, 30%)", ".8544"],
])("%s is a percentage of the channel", (expr, expected) => {
expect(color(`color(from ${origin} srgb ${expr} g b)`, "css")).toBe(`color(srgb ${expected} .4 .2)`);
});

const spaces: [space: string, channels: string, printed?: string][] = [
["srgb", "g b"],
["srgb-linear", "g b"],
["display-p3", "g b"],
["prophoto-rgb", "g b"],
["rec2020", "g b"],
["xyz-d50", "y z"],
["xyz-d65", "y z", "xyz"],
["xyz", "y z"],
];
Comment on lines +578 to +587

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The spaces array covers 8 of the 9 predefined color() spaces whose channel types this PR changes from CT_PCT to CT_NUM, but omits a98-rgb (the A98 struct at color.rs:1826 received the identical change). Add ["a98-rgb", "g b"] to complete the matrix per REVIEW.md "Cover the variant matrix, not just the repro".

Extended reasoning...

What the gap is

The spaces test.each array (test/js/bun/css/color.test.ts:578-587) enumerates the predefined color() spaces to verify calc(<channel> * <percentage>) works in each: srgb, srgb-linear, display-p3, prophoto-rgb, rec2020, xyz-d50, xyz-d65, xyz. It omits a98-rgb, which is the ninth predefined space the CSS Color 4 spec defines and whose types tuple this PR changes from (CT_PCT, CT_PCT, CT_PCT) to (CT_NUM, CT_NUM, CT_NUM) at src/css/values/color.rs:1826, identically to the other eight.

Why this is a finding

REVIEW.md → Tests reviewers reject → "Cover the variant matrix, not just the repro. Every sibling entry point receiving the same fix". The PR description explicitly claims coverage of "color() in every space", and A98 is one of the 9 define_colorspace! invocations receiving the CT_PCT → CT_NUM change. The test whose stated purpose is "verify this works in every color() space" should list every space it received the change in.

Why nothing else covers it

The other new test blocks in this PR use specific spaces (srgb, display-p3, xyz) as representatives; only the spaces array is the exhaustive per-space sweep. a98-rgb appears once in css.mdx as a documentation example but nowhere in the new test coverage.

Impact

Low. All predefined color() spaces route through the identical parse_predefined → parse_number_or_percentage code path; the only per-space input to that path is the types() tuple, and every tuple was changed uniformly. There is no distinct code path for a98-rgb that could diverge from the other 8. The missing entry only guards against a future accidental revert of A98's types tuple — it does not hide any correctness risk today.

Step-by-step proof of the omission

  1. src/css/values/color.rs diff changes types on 9 define_colorspace! invocations that participate in color(): SRGB, SRGBLinear, P3, A98, ProPhoto, Rec2020, XYZd50, XYZd65 (plus LAB/LCH/OKLAB/OKLCH/HSL/HWB which are covered by the separate per-function test block).
  2. Of the predefined-color()-space subset, the spaces array lists 8: srgb, srgb-linear, display-p3, prophoto-rgb, rec2020, xyz-d50, xyz-d65, xyz (the last two both map to XYZd65, so 7 distinct structs).
  3. a98-rgb (→ A98) is the only predefined color() space in the diff not present in the array.
  4. A98 { r, g, b } uses the same channel names as the other RGB-like spaces, so the entry is ["a98-rgb", "g b"].

How to fix

Add one line to the spaces array:

const spaces: [space: string, channels: string, printed?: string][] = [
  ["srgb", "g b"],
  ["srgb-linear", "g b"],
  ["display-p3", "g b"],
  ["a98-rgb", "g b"],
  ["prophoto-rgb", "g b"],
  ...

test.each(spaces)("color(from ... %s calc(<channel> * <percentage>))", (space, channels, printed = space) => {
const [b, c] = channels.split(" ");
expect(color(`color(from color(${space} .8 .4 .2) ${space} ${b} calc(${b} * 50%) calc(50% * ${c}))`, "css")).toBe(
`color(${printed} .4 .2 .1)`,
);
});

// The keyword is the channel's <number> in the function's own range (r is 200
// in rgb(), s is 50 in hsl(), l is 50 in lab() and .5 in oklab()), not the
// unit value the color is stored as.
test.each([
["rgb(from rgb(200 100 50) calc(r * 50%) g b)", "#ff6432"],
["rgb(from rgb(200 100 50) calc(r * 0.1%) g b)", "#336432"],
["rgb(from rgb(200 100 50) r g b / calc(r * 0.1%))", "#c8643233"],
["rgb(from rgb(200 100 50 / .5) r g b / calc(alpha * 50%))", "#c8643240"],
["hsl(from hsl(120 50% 40%) h calc(s * 50%) l)", "#0c0"],
["hsl(from hsl(120 50% 40%) h calc(s * 1%) l)", "#393"],
["hsl(from hsl(120 50% 40%) h s calc(l * 1.5%))", "#6c6"],
["hsl(from hsl(120 50% 40%) h s l / calc(s * 0.5%))", "#33993340"],
["hwb(from hwb(120 20% 30%) h calc(w * 2%) b)", "#66b366"],
["hwb(from hwb(120 20% 30% / .5) h w b / calc(alpha * 50%))", "#33b33340"],
["lab(from lab(50% 20 30) calc(l * 1.2%) a b)", "lab(60% 20 30)"],
["lab(from lab(50% 20 30) l a b / calc(l * 1%))", "lab(50% 20 30 / .5)"],
["lch(from lch(50% 30 120) calc(l * 1%) c h)", "lch(50% 30 120)"],
["lch(from lch(50% 30 120 / .5) l c h / calc(alpha * 50%))", "lch(50% 30 120 / .25)"],
["oklab(from oklab(50% .1 .1) calc(l * 50%) a b)", "oklab(25% .1 .1)"],
["oklab(from oklab(50% .1 .1 / .5) l a b / calc(50% * alpha))", "oklab(50% .1 .1 / .25)"],
["oklch(from oklch(50% .1 120) calc(l * 50%) c h)", "oklch(25% .1 120)"],
["oklch(from oklch(50% .1 120 / .5) l c h / calc(alpha * 50%))", "oklch(50% .1 120 / .25)"],
["color(from color(srgb .8 .4 .2 / .5) srgb r g b / calc(alpha * 50%))", "color(srgb .8 .4 .2 / .25)"],
])("%s", (input, expected) => {
expect(color(input, "css")).toBe(expected);
});

// Forms bun accepted before, with the keyword standing for a percentage of its
// range (the documented `calc(l + 15%)`), still mean what they did.
test.each([
["lab(from lab(50% 20 30) calc(l + 15%) a b)", "lab(65% 20 30)"],
["hsl(from hsl(120 50% 40%) h calc(s - 10%) l)", "#3d8f3d"],
["rgb(from rgb(200 100 50 / .5) r g b / calc(alpha - 20%))", "#c864324d"],
[`color(from ${origin} srgb calc(r + 10%) g b)`, "color(srgb .9 .4 .2)"],
[`color(from ${origin} srgb min(r, 50%) g b)`, "color(srgb .5 .4 .2)"],
[`color(from ${origin} srgb min(r, g) g b)`, "color(srgb .4 .4 .2)"],
[`color(from ${origin} srgb calc(r * .5) g b)`, "color(srgb .4 .4 .2)"],
])("%s still parses", (input, expected) => {
expect(color(input, "css")).toBe(expected);
});

test.each([
`color(from ${origin} srgb calc(r * 50% + .1) g b)`,
`color(from ${origin} srgb calc(r * 50% - g) g b)`,
`color(from ${origin} srgb calc(r * 50% * 10%) g b)`,
// Without relative color syntax there is no keyword to fall back on; these
// used to evaluate to NaN and parse as a `none` channel.
"color(srgb calc(1 + 50%) 0 0)",
"color(srgb calc(50% + 1) 0 0)",
"hsl(120 calc(50% + 1) 40%)",
])("%s mixes a number and a percentage and does not parse", input => {
expect(color(input, "css")).toBeNull();
});
});

describe("input forms", () => {
test.each([
["a named color", "red"],
Expand Down
65 changes: 65 additions & 0 deletions test/js/bun/css/css.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,71 @@ describe("css tests", () => {
minify_test(`a { opacity: calc(NaN) }`, `a{opacity:0}`);
minify_test(`a { rotate: calc(NaN * 1deg) }`, `a{rotate:0deg}`);
minify_test(`a { transition-duration: calc(NaN * 1s) }`, `a{transition-duration:0s}`);

// A <number> and a <percentage> cannot be added: the declaration is invalid and
// is left as written, not evaluated to NaN and serialized as 0.
minify_test(`a { opacity: calc(50% + 1) }`, `a{opacity:calc(50% + 1)}`);
minify_test(`a { opacity: calc(1 - 50%) }`, `a{opacity:calc(1 - 50%)}`);
minify_test(`a { opacity: calc(50% + 25%) }`, `a{opacity:.75}`);
minify_test(`a { opacity: calc(50% * 1.5) }`, `a{opacity:.75}`);
});

describe("relative color calc() with percentages", () => {
// https://drafts.csswg.org/css-color-5/#relative-colors: a channel keyword is
// a <number> in the function's range, so `calc(r * 50%)` is a <percentage> of
// the channel. Expected values are lightningcss output.
minify_test(
".foo { color: color(from color(srgb .8 .4 .2) srgb calc(r * 50%) calc(50% * g) b) }",
".foo{color:color(srgb .4 .2 .2)}",
);
minify_test(
".foo { color: color(from color(display-p3 .8 .4 .2) display-p3 r calc(g * 50% + 10%) calc(50% - b * 50%)) }",
".foo{color:color(display-p3 .8 .3 .4)}",
);
minify_test(
".foo { color: color(from color(srgb .8 .4 .2) srgb min(r * 50%, 30%) max(g * 50%, 30%) b) }",
".foo{color:color(srgb .3 .3 .2)}",
);
minify_test(
".foo { color: color(from color(srgb .8 .4 .2) srgb round(r * 100%, 30%) g b) }",
".foo{color:color(srgb .9 .4 .2)}",
);
minify_test(
".foo { color: color(from color(srgb .8 .4 .2 / .5) srgb r g b / calc(alpha * 50%)) }",
".foo{color:color(srgb .8 .4 .2/.25)}",
);
minify_test(".foo { color: rgb(from rgb(200 100 50) calc(r * 50%) g b) }", ".foo{color:#ff6432}");
minify_test(".foo { color: rgb(from rgb(200 100 50) calc(r * 0.1%) g b) }", ".foo{color:#336432}");
minify_test(".foo { color: rgb(from rgb(200 100 50) r g b / calc(r * 0.1%)) }", ".foo{color:#c8643233}");
minify_test(".foo { color: rgb(from rgb(200 100 50 / .5) r g b / calc(alpha * 50%)) }", ".foo{color:#c8643240}");
// Not `rgb(255 0 0/calc(alpha*50%))`, which no browser accepts.
minify_test(".foo { color: rgb(from red r g b / calc(alpha * 50%)) }", ".foo{color:#ff000080}");
minify_test(".foo { color: hsl(from hsl(120 50% 40%) h calc(s * 50%) l) }", ".foo{color:#0c0}");
minify_test(".foo { color: hsl(from hsl(120 50% 40%) h s calc(l * 1.5%)) }", ".foo{color:#6c6}");
minify_test(".foo { color: hsl(from hsl(120 50% 40%) h s l / calc(s * 0.5%)) }", ".foo{color:#33993340}");
minify_test(".foo { color: hwb(from hwb(120 20% 30%) h calc(w * 2%) b) }", ".foo{color:#66b366}");
minify_test(".foo { color: lab(from lab(50% 20 30) calc(l * 1.2%) a b) }", ".foo{color:lab(60% 20 30)}");
minify_test(".foo { color: lab(from lab(50% 20 30) l a b / calc(l * 1%)) }", ".foo{color:lab(50% 20 30/.5)}");
minify_test(".foo { color: lch(from lch(50% 30 120) calc(l * 1%) c h) }", ".foo{color:lch(50% 30 120)}");
minify_test(".foo { color: oklab(from oklab(50% .1 .1) calc(l * 50%) a b) }", ".foo{color:oklab(25% .1 .1)}");
minify_test(
".foo { color: oklch(from oklch(50% .1 120 / .5) l c h / calc(alpha * 50%)) }",
".foo{color:oklch(50% .1 120/.25)}",
);

// The keyword-as-percentage forms bun accepted before keep their meaning.
minify_test(".foo { color: lab(from lab(50% 20 30) calc(l + 15%) a b) }", ".foo{color:lab(65% 20 30)}");
minify_test(".foo { color: rgb(from rgb(200 100 50 / .5) r g b / calc(alpha - 20%)) }", ".foo{color:#c864324d}");
minify_test(
".foo { color: color(from color(srgb .8 .4 .2) srgb min(r, 50%) calc(g + 10%) b) }",
".foo{color:color(srgb .5 .5 .2)}",
);

minify_test(
".foo { color: color(from red srgb calc(r * 50% + .1) g b) }",
".foo{color:color(from red srgb calc(r*50% + .1)g b)}",
);
minify_test(".foo { color: color(srgb calc(1 + 50%) 0 0) }", ".foo{color:color(srgb calc(1 + 50%)0 0)}");
});
describe("calc stack overflow", () => {
// https://github.com/oven-sh/bun/issues/20128
Expand Down
Loading