Repository navigation
css: parse calc(<channel> * <percentage>) in relative colors #38513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, as a unit value. The keywords are `<number>`s | ||
| /// (`calc(r * 50%)`), else percentages of their range as before (`calc(l + 10%)`). | ||
| 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); | ||
| } | ||
|
|
||
| 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 This fallback wraps each keyword's stored component as Extended reasoning...What the bug isThe second-pass fallback of Code pathFor
Why nothing else catches itThe RegressionBefore this PR, The stale
|
||
| } | ||
|
|
||
| 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)), | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The Extended reasoning...What the gap isThe Why this is a findingREVIEW.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 " Why nothing else covers itThe other new test blocks in this PR use specific spaces ( ImpactLow. All predefined Step-by-step proof of the omission
How to fixAdd one line to the 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"], | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.