Repository navigation
Conversation
|
Updated 7:06 AM PT - Oct 29th, 2026
❌ @robobun, your commit 01d0796 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 47284That installs a local version of the PR into your bun-47284 --bun |
|
Status
|
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
 |
| Layer / File(s) | Summary |
|---|---|
Parse-time value classification src/css/properties/custom.rs |
TokenList detects var() and env() substitutions recursively. UnparsedProperty::is_kept_at_parse_time returns true for values with substitutions or a single case-insensitive inherit, initial, or unset identifier. |
Declaration merge behavior src/css/properties/prefix_handler.rs, test/js/bun/css/css.test.ts |
The prefix handler replaces an earlier unparsed declaration only when the new value is kept at parse time or matches the held value. Otherwise, it appends the new declaration. Tests cover color and text-shadow merging, including unsupported values, fallbacks, and CSS-wide keywords. |
Suggested reviewers: jarred-sumner, alii
Priority:
Merge Risk: 
5ce07
-
The change preserves fallbacks for unsupported values. No merge-blocking issue remains.
Pre-merge checks: 
-
Check name Status Explanation Title check The title names the main change: the earlier color or text-shadow declaration is kept when a later unparsed value has no var() or env(). The changeset matches this, since prefix_handler.rs replaces th… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description clearly explains the problem, root cause, fix, scope, verification steps, test coverage, performance impact, and known limitations. It does not use the template headings exactly, but i… Full details: Title check
-
Explanation
The title names the main change: the earlier color or text-shadow declaration is kept when a later unparsed value has no var() or env(). The changeset matches this, since prefix_handler.rs replaces the earlier declaration only for kept-at-parse-time values and the new is_kept_at_parse_time predicate supports it. The title is specific and concise, and it is one sentence.
Full details: Description check
-
Explanation
The description clearly explains the problem, root cause, fix, scope, verification steps, test coverage, performance impact, and known limitations. It does not use the template headings exactly, but it provides the required information in equivalent sections.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/css/properties/prefix_handler.rs— pre-existing: Users targeting an older browser get nocolorat all when a typed wide-gamut color follows an unparsed one, e.g..a{color:foo(1);color:lab(50% 20 30)}at chrome80 printsfoo(1);lab(50% 20 30)with no#a16945fallback. The typed branch skipsget_fallbacksat src/css/properties/prefix_handler.rs:46 whenever the slot is held, even when the held value is anProperty::Unparsedthatis_kept_at_parse_time()rejects, so nothing readable by the target remains. Fix: generate the typed value's fallbacks unless the held declaration is one every target reads (a typed compatible value, or an unparsed value kept at parse time), for bothcolorandtext-shadow.Why this was flagged
Input
.a{color:foo(1);color:lab(50% 20 30)}minified with chrome 80 targets through FallbackHandler::handle_property.foo(1)is Property::Unparsed; src/css/properties/prefix_handler.rs:141-142 pushes it and setscolor = Some(0). The typedlab()then reaches the handle_unprefixed! macro: src/css/properties/prefix_handler.rs:46 tests$self_field.is_none(), which is false, so the fallbacks closure at prefix_handler.rs:81-89 never runs and no#a16945is pushed; prefix_handler.rs:51-56 then pusheslab()because it is not compatible with chrome 80. Output is.a{color:foo(1);color:lab(50% 20 30)}; Chrome 80 parses neither and the element has no author color, whereas.a{color:lab(50% 20 30)}alone prints.a{color:#a16945;color:lab(50% 20 30)}. The base branch produces the same output for this input, so this is pre-existing. The guard at line 46 assumes the held slot is readable by every target, which the newis_kept_at_parse_time()predicate (src/css/properties/custom.rs:1413) now makes explicit is not true for an unparsed value without var()/env().Verification: pre-existing — the base branch produces the same output by the same route. In src/css/properties/prefix_handler.rs, lines 140-143 push
foo(1)at slot 0 and setcolor = Some(0); line 46if $self_field.is_none()is then false, so the fallbacks closure (lines 81-89) never runs, and lines 55-56 pushlab(). Output:.a{color:foo(1);color:lab(50% 20 30)}with no#a16945, so a Chrome 80 target reads neither declaration and gets nocolor. -
🟣
src/css/properties/prefix_handler.rs— Users at a non-lab target who write an unparsed color containing lab() followed by a typed lab() color get the wrong color in lab-capable browsers: the@ supportscopy of the earlier unparsed value prints after the rule and beats the later typed declaration. The typed push at src/css/properties/prefix_handler.rs:51-56 never calls remove_conditional_property, unlike the replace path at :58, and the diff now routes more unparsed values into dest alongside a live typed successor. Fix: every path that writes a later declaration of the same property, including the push at :55-56, must remove the staged conditional copies of the earlier declaration.Why this was flagged
Trigger: an unparsed
colorwhose tokens contain a lab()/lch()/oklab() color (color-mix, contrast-color, any unknown function) followed by a typed lab() color, minified with browsers that lack lab support (chrome80 in the tests). First declaration goes through prefix_handler.rs:133 and stages an@ supports (color:lab(0% 0 0))copy via context.rs:387. Second declaration hits the typed macro:$self_fieldis Some so the push branch at prefix_handler.rs:51-56 runs because is_compatible is false; that branch does not call remove_conditional_property (only the replace branch at :58 does). The staged copy is emitted after the rule and, in a lab-capable browser that also reads the unparsed function (color-mix is supported wherever lab is), it wins over the typedlab(60% 0 0)the author wrote last. Base has the same push path, but the PR's own comment at context.rs:357 states the invariant ("The @ supports copies print after the rule, so they would beat a later declaration") and the diff adds the equal/kept predicate without applying that invariant to this sibling site.Verification: Pre-existing (base fails identically by the same route; the diff touches only the sibling unparsed branch and neither widens nor narrows this). Only the replace branch at src/css/properties/prefix_handler.rs:57-59 calls
context.remove_conditional_property(property.property_id()); the push branch at :51-56 never does, so the staged copy survives.
|
The two findings outside the diff are confirmed on main 7942008 and on this branch. Both builds print the same output, so this PR does not change either one.
Both are in the typed arm, which this PR leaves as it is. The description now lists them with the other limits that are the same as on main. The four inline threads have their own replies. |
5ce072e to
9c31edf
Compare
…as no var() or env() FallbackHandler wrote every unparsed `color` or `text-shadow` over the earlier declaration of the property. That is right only for a value that no browser drops when it parses the declaration: one with `var()` or `env()`, or `inherit`, `initial` or `unset`. A browser that cannot read any other value drops the declaration and uses the earlier one, so `color: red; color: rgb(from color(display-p3 0 1 0) r g b)` lost its fallback. UnparsedProperty::is_kept_at_parse_time holds that rule. The handler replaces the earlier declaration when the rule holds or the two declarations are equal. In every other case it appends the new one.
9c31edf to
01d0796
Compare
Problem
bun builddeletes acolorortext-shadowfallback..a{color:#000;color:contrast-color(#ff69b4)}prints.a{color:contrast-color(#ff69b4)}, so a browser with nocontrast-color()has nocolor.@csstools/postcss-contrast-color-functionprints it. Since css: consolidate the open CSS parser fixes #47025,color:red;color:rgb(from color(display-p3 0 1 0) r g b)losesredtoo.Property::Unparsedbranch ofFallbackHandler::handle_property(src/css/properties/prefix_handler.rs:134) writes every unparsed value over the earlier declaration.Fix
UnparsedProperty::is_kept_at_parse_time()holds (var(),env(),inherit,initial,unset) or the two are equal. Otherwise it appends.var()andenv()after substitution. It drops any other value that it cannot read and uses the earlier declaration.test/js/bun/css/css.test.ts(73 new rows, 57 fail on main). Alsotest/js/bun/css,test/bundler/css,test/bundler/esbuild/css.test.ts.Background
Property::Unparsed):var(), or unknown syntax.FallbackHandlerkeeps the index of the lastcolorandtext-shadow. A typed value overwrites that slot only when every target reads it.color/text-shadowpairs grow. css: keep an earlier gap or flex-basis declaration when a target lacks the later one #46694 (same class) closed with "Reopen it if the input shows up in real CSS".Downsides
text-shadow:X;text-shadow:noneprints both, asbox-shadowdoes on main: +165 B in one real stylesheet.color, take quadratic time, aswidth:foo(i)does on main: 8,000 pairs 4.2 s, was 0.04 s..text+768 B. An unparsedcolorcosts 88 instructions inhandle_property, was 85.Notes
Real input
Built with release builds of main 7942008 and of this branch,
bun build <file> --minify:pride-palette.csscolor:#000000; color:contrast-color(#ff69b4), 232 rulesassets/css/material.css(recomedicales.fr)color:#4150f5; color:contrast-color(primary), 7 rulesviewer/app/styles/viewer.csscolor:#0000ee; color:-webkit-linkcolor:-webkit-link, 2,049 Bcontrast-color(primary). Chrome 153 computesrgb(65, 80, 245)from the source and from the output of this branch. From the output of main it computes the inherited color.@csstools/postcss-contrast-color-functionshows its output:color: rgb(0, 0, 0); color: contrast-color(oklch(82% 0.2 330));. Main prints that pair withoutcolor:#000..a{color:#00f942}. This branch keepsred. It does not bring back the folded color.--target=bun.History
colorandtext-shadow. css: consolidate the open CSS parser fixes #47025 took its size half. The rule here is narrower: a value withvar()orenv()still replaces the earlier declaration, so those outputs do not change.gapandflex-basisvalues) closed as right, but with no real input and no new syntax. The stylesheets above are real input.ifinprefix_handler.rs, so the second of the two to merge needs a rebase of one hunk.varis considered valid at parse time ... Anything not containingvarmust be preserved in case a later value is invalid in the current engine."contrast-color(primary)orexpression().Not fixed here
Two more sites drop held state for an unparsed value. A flush at either one makes other output wrong at the default targets, so they need their own design:
BorderRadiusHandler(src/css/properties/border_radius.rs:213):.a{border-start-start-radius:1px;border-start-start-radius:foo(1)}prints onlyfoo(1). A flush sends the first corner to a:dir()rule, which is printed after the rule and wins over a laterborder-radius.BorderHandler(src/css/properties/border.rs:1457):.a{border-image:linear-gradient(red,blue) 1;border:foo(1)}prints.a{border:foo(1)}.Limits that are the same as on main:
color:red;color:var(--)andcolor:red;color:var(--x,!)print only thevar(). Chrome 153 drops both at parse time.VAR(--x)in upper case is kept as a function, so the earlier declaration stays.@supportscopy of an earliervar()declaration is dropped by any later declaration of the property:color:var(--c,lab(50% 20 30));color:foo(1)printscolor:var(--c,#a16945);color:foo(1)at Chrome 80.colorandtext-shadowcollapseX; var(). Other properties print both.color:foo(1);color:lab(50% 20 30)prints as written at Chrome 80. The typed arm adds fallbacks only for the first declaration of the property (prefix_handler.rs:46).@supportscopy of an earlier token value:color:foo(lab(50% 20 30));color:lab(60% 0 0)at Chrome 80 prints the copycolor:foo(lab(50% 20 30))after the rule, where it wins overlab(60% 0 0). The push atprefix_handler.rs:55does not callremove_conditional_property.Output that grows with the same meaning
text-shadow:1px 1px red;text-shadow:none: 20 to 44 B. The typed parser oftext-shadowtakes nononeand nocalc(),min(),max()orclamp()length, so these values are tokens. kristensen-kor/tablecraftstyle-template.csshas three such pairs: 10,433 to 10,598 B.color:red;color:foo(1);color:var(--x)printscolor:red;color:var(--x): 18 to 28 B.color:red;color:foo(1);color:blueprintscolor:red;color:#00f: 14 to 24 B..a{color:red;color:foo(1)}printcolor:redn times: 20,017 B for 2,000 rules. Main prints 20,025 B for 2,000.a{color:red;color:lab(50% 20 30)}rules.@supports (color:contrast-color(red)){.b{color:red;color:contrast-color(blue)}}is 69 B on main and 79 B here.Cost
Release LTO x64 builds of main 7942008 and of this branch on that base. The head differs from the measured build in comments and one named constant.
FallbackHandler::handle_property, instructions per call (qemu-user): 28, 36, 97 or 102 for a declaration that is not an unparsedcolorortext-shadow, the same on both builds. Unparsedcolorwith an empty slot: 85 to 88. Unparsedtext-shadow: 79 to 80. The difference is register moves. Over the 15 stylesheets oftest/js/bun/css/files: 887,386 to 892,015 instructions in 24,683 calls.is_kept_at_parse_timeran 0 times in 45,777DeclarationHandler::handle_propertycalls over those 15 stylesheets (gdb hit counts).X; var()pair: +55 instructions.var()in anrgb()alpha: +95. Per pair that now keeps both: +249 and one more 224 BPropertyin the list..text83,513,897 to 83,514,665 B.handle_property5,767 to 5,891 B. Predicate and walk 536 B. The stripped binary has the same file size.sizeofFallbackHandler32 andUnparsedProperty48 on both builds..a,.b{color:foo(0)}then n pairs.a{color:foo(i)}.b{color:foo(i)}, user CPU at n = 2,000 / 4,000 / 8,000: 0.24 / 1.02 / 4.25 s here, 0.01 / 0.02 / 0.04 s on main.width:foo(i)on main: 0.17 / 0.69 / 3.04 s. Each merge minifies the whole merged block again (src/css/rules/mod.rs:1202), and the block now holds onecolorper value. 2,000 and 4,000 adjacent.a{color:red;color:foo(1)}rules stay linear: 8,000 and 16,000handle_propertycalls.Self-review
Addressed: the real input and the history above, test rows for each clause of the predicate and for the slot that moves, the one-line comment. In part: the two sites under "Not fixed here" and the merge cost are described here. No tracking issue exists for them.