Repository navigation
Conversation
…rgets without it The inset handler always emitted the 4-side `inset` shorthand, and merged top/right/bottom/left longhands into it, even when the browser targets do not support `inset` (Chrome < 87, Firefox < 63, Safari < 14.1). The fallback branch in SizeHandler::flush was keyed on SHORTHAND_CATEGORY, which every spec sets to Physical, so it never ran. Gate the 4-side shorthand on a per-spec compat feature instead: PHYSICAL_SHORTHAND_FEATURE is LogicalInset for inset (the compat entry that already gates inset-block/inset-inline in the same handler) and None for margin, padding and scroll-margin. When the targets lack it, flush writes the four physical longhands, matching lightningcss.
Collaborator
Author
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe CSS size-property handler now feature-gates four-side shorthand emission for inset. Added tests cover inset minification and bundler output across browser targets. ChangesInset shorthand compatibility
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the main change: emitting physical longhands for targets without inset support. |
| 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 results, performance impact, and known limitations. It does not use the template headings exactly, but it provides th… |
Full details: Description check
-
Explanation
The description clearly explains the problem, root cause, fix, scope, verification results, 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}
Contributor
There was a problem hiding this comment.
LGTM — small, well-scoped correctness fix in the CSS minifier's inset handling.
What was reviewed:
- Verified the removed guard (
SHORTHAND_CATEGORY != Logical || logical_supported) was vacuously true for all four specs, somargin/padding/scroll-marginbehavior is unchanged (their newPHYSICAL_SHORTHAND_FEATUREisNone). - Confirmed the
is_supportedhelper is a pure refactor of the two inlinematchblocks it replaces, andis_none_oris already used elsewhere in the tree. - Checked
Feature::LogicalInsetcompat data (Chrome 87 = 5701632) matches the 86/87 boundary tests, and that routing throughcontext.should_compile_logicalkeeps the style-attribute exemption and include/exclude flags consistent with the rest of the family.
Extended reasoning...
Overview
The PR fixes a CSS minifier bug where the inset shorthand was emitted (or synthesized from top/right/bottom/left) even when the configured browser targets predate its support. The fix adds a new PHYSICAL_SHORTHAND_FEATURE: Option<Feature> associated constant to SizeHandlerSpec, sets it to Some(Feature::LogicalInset) for InsetSpec and None for the other three specs, and gates the shorthand branch of SizeHandler::flush on it. A small is_supported helper factors out the Option<Feature> → should_compile_logical check that already appeared twice. Two files touched: src/css/properties/margin_padding.rs (~25 net lines) and test/js/bun/css/css.test.ts (11 new test cases in a new describe("inset") block).
Security risks
None. This is compat-table-driven output selection in the CSS printer; no untrusted-size arithmetic, no unsafe, no allocation or lifetime changes, no user-controlled paths.
Level of scrutiny
Low-to-medium. The change is a targeted correctness fix in a well-understood generic handler, ported from lightningcss semantics and cross-checked against lightningcss 1.30.2 output per the PR description. The functional diff is effectively two lines (the new constant on InsetSpec and the if shorthand_supported guard); the rest is a behavior-preserving refactor plus doc comments. I traced the removed condition and confirmed it was always true for every existing spec, so no unrelated family can regress. The Chrome 87 boundary in the tests matches the LogicalInset entry in src/css/compat.rs (5701632 = 87 << 16).
Other factors
Test coverage is thorough for a change this size: old-target lowering, exact 86/87 boundary, 4-value shorthand mapping to the correct sides, !important propagation, composition with a following inset-inline, an unparsed var() case, a no-targets pin, a supported-targets pin, and a pin that margin/padding/scroll-margin are unaffected. The PR description explicitly justifies why SHORTHAND_CATEGORY was not flipped instead (it would break folding that #38551 pins) and scopes out an unrelated pre-existing bug shared with lightningcss. No prior review comments on the timeline. Full css.test.ts suite reported passing.
Conflict: src/css/properties/margin_padding.rs. Main replaced the SizeHandlerSpec trait constants with the static SizeSpec table (#39770), so the old hunks do not apply. This merge takes main's file. The next commit adds the target gate to the table.
… bit SizeSpec.shorthand_category becomes the bool shorthand_needs_feature, true for inset only. SizeHandlerImpl::flush emits the 4-side shorthand only when the bit is clear or the targets support the spec's feature (LogicalInset). Otherwise it emits top, bottom, left and right. The 4-side shorthand arm passes the literal PropertyCategory::Physical to its four flush checks, as every spec row did, so buffering does not change. Tests: both sides of each LogicalInset boundary, the inset cases of lightningcss test_position, the states that must not change, and the default browser targets through bun build on both backends.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Collaborator
Author
|
Updated 7:53 AM PT - Oct 22nd, 2026
✅ @robobun, your commit b62f3595c82c9d047c6dff94e265b040848808b0 passed in 🧪 To try this PR locally: bunx bun-pr 38583That installs a local version of the PR into your bun-38583 --bun |
Contributor
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/margin_padding.rs— Users who writeinset-inline-start: 5px; inset: 0getleft: 5pxapplied instead ofleft: 0in every browser, because the earlier logical value is flushed into a higher-specificity direction rule. The shorthand arm at src/css/properties/margin_padding.rs:690-734 calls flush_helper_physical withPhysicalwhileself.categoryisLogical, so :759-764 flushes the bufferedinset-inline-startthrough logical_prop_helper (:952-962) into context.ltr/rtl, and then overwrites the slots without updatingself.category. Fix: in the shorthand arm, drop buffered logical slots instead of flushing them when the shorthand supersedes all four sides, and setself.categoryso later longhands merge; mirror lightningcss, which clears the logical slots and emits onlyinset: 0.Why this was flagged
Input
.a{inset-inline-start:5px;inset:0}through any bundler or Bun.build CSS path. handle_property forinset-inline-startsetsself.category = Logicalat :822. Theinsetarm at :694 passesPropertyCategory::Physical; flush_helper_physical at :759 seescategory != self.categoryand calls flush, which at :951-962 routes the lone inline_start into context.add_logical_rule asleft:5px/right:5pxdirection rules. The arm then sets the four physical slots at :726-729 and clears the logical ones but leavesself.categoryas Logical. Final flush emitstop:0;bottom:0;left:0;right:0, and the generated direction ruleleft:5pxis printed after with higher specificity, overriding the author's laterinset: 0. The base branch emits the same overriding rule, so this is pre-existing, but the PR explicitly ports lightningcss semantics for this arm and lightningcss does not emit the stale value at all. The one-line remedy is to clear rather than flush the logical slots when a four-side shorthand arrives.Verification: pre-existing (acknowledged at src/css/properties/margin_padding.rs:692-693). Trigger: a lone logical inset longhand followed by
insetat targets lackingFeature::LogicalInset.flush_helper_physicalflushes whenevercategory != self.category(:759-764), so theleft:5pxdirection rule wins overleft:0. On the base the identical flush happened, so merging makes nothing worse.
The rows accepted the kept family too, so a build that applies no targets at all passed them.
Contributor
There was a problem hiding this comment.
LGTM — the latest push resolves the default-target assertion nit, and the source change is small and behavior-preserving outside inset.
What was reviewed:
flushguard (src/css/properties/margin_padding.rs:846):!shorthand_needs_feature || logical_supportedis always true for margin/padding/scroll-margin, matching the oldPhysical != Logicalguard; onlyInsetSpecnow reads the targets.- Shorthand arm: the literal
PropertyCategory::Physicalequals the value every spec row carried, so no output change for targets withinset. - Searched src/ for stale
shorthand_category/category:macro arguments — none remain; all foursize_handler_spec!sites updated. - Test rows: boundary matrix covers both sides of each LogicalInset version, and the bundler rows now assert one expected output per target set.
Extended reasoning...
The change replaces the shorthand_category enum field on SizeSpec with a shorthand_needs_feature bool (true only for inset) and makes the 4-side inset shorthand fold conditional on the LogicalInset compat feature, emitting top/bottom/left/right for older targets; the 29-line Rust diff is in src/css/properties/margin_padding.rs with ~570 lines of tests in test/js/bun/css/css.test.ts and test/bundler/esbuild/css.test.ts. It touches no security-sensitive surface. Approve was decided by the source change being mechanical and provably behavior-preserving for the non-inset handlers, the single commit since the prior review addressing the one nit raised, no CODEOWNER covering the changed paths, and no human reviewer objections in the timeline; the two earlier pre-existing inline notes (the lightningcss divergence on earlier-declaration flushing, and the Firefox 63-65 gap) are not introduced by this PR.
Collaborator
Author
|
A data point for the merge order. I ran the two open PRs that merge clean with this one, each applied together with this fix, on a debug build. The build had the fix in the form of the first version of this PR (an
The 11 cases of the first version of this PR pass in both combinations. Neither PR has a textual conflict with this one, so GitHub shows them as mergeable. The PR that lands second must update those four expectations, in addition to the |
Jarred-Sumner
added a commit
that referenced
this pull request
Oct 27, 2026
### What does this PR do? Consolidates the open pull requests on the CSS parser (`src/css`: parsing, minifying, lowering, printing, CSS modules) into one. 179 open pull requests were in scope. Each was judged by one rule: **the output must mean to a browser what the input meant.** Every claim was run through a build of `main`, a build with the change, lightningcss 1.33 and Chromium 156 (CSSOM, computed values, painted pixels for colors and gradients), and counted against 10,116 real stylesheets: the published CSS of about 190 popular npm packages, plus stylesheets from open source repositories. 107 land here, whole or in part, most of them cut down. The source diff is net negative. Of the 10,116, 9,509 are stylesheets that someone ships. The rest are test fixtures of other tools (521), Tailwind v4 source that needs its plugin (59) and SCSS, PostCSS or template source (27). | 9,509 shipped stylesheets | `main` | this branch | |---|---|---| | fail to build | 71 | 35 | | ... of those, valid CSS that Bun rejects | | 0 | | minified output reads differently in Chromium than the input | 552 | 2 | | CSS modules with a name that is referenced but not defined, or exported but not defined | 166 | 0 | | all 10,116 | `main` | this branch | |---|---|---| | fail to build | 192 | 141 | | minified output reads differently in Chromium than the input | 593 | 15 | | build on `main`, fail here | | 0 | | right on `main`, wrong here (minified, and at the default targets) | | 0 | | size of the minified output, no targets | 162.32 MB | 162.47 MB (+0.093%) | | size of the output at the default targets | 224.1 MB | 210.2 MB (-6.2%) | A syntax error is a build error, with one exception: the IE star hack `*zoom: 1` is skipped, because tachyons and handsontable ship it. The 35 shipped stylesheets that fail are invalid CSS: 28 have a selector that Chromium drops (semantic-ui-css, swiper, @ionic/core, Adobe Spectrum, xp.css), 2 an `@import` after other rules, and one each `@supports not X and Y`, a nested `@keyframes`, a stray comma, a selector list in `::slotted()`, `@media screen\0` (govuk-frontend). The 2 that read differently fold a `color-mix()` inside a gradient (see the known gaps). The growth of the minified output is rules and declarations that `main` deleted by mistake: 87 KB of rules, 45 KB of prefixed declarations that only browser targets can rule out. #### Popular CSS that `main` breaks today - normalize.css, modern-normalize, sanitize.css: `font-family: monospace, monospace` is collapsed to `monospace`, which changes the font size from 16px to 13px (#46662). - CKEditor 5, Tabler, Carbon, VitePress, Ionic, PatternFly, Excalidraw: a rule of custom properties is deleted as a "duplicate" of a later rule with the same selector and other custom properties. 37 stylesheets lose 1,681 custom properties (#33684). - Material Components, daisyUI, DocSearch, plyr: a `var()` size loses to its own fallback (#42101). - nextra, daisyUI, CKEditor: declarations written after a nested rule are moved above it (#45462). - antd 4, purecss, govuk-frontend: browser hacks in selector lists start to apply everywhere (#42100). - foundation-sites, flatpickr, 98.css, blueprintjs, scalar, react-datepicker: the build fails on valid CSS (#46756, #38552, #38534). tachyons, handsontable: it fails on the IE star hack (#46751). - nodejs.org, Mantine: a CSS module exports classes that no rule defines, because the `@apply` or `@mixin` in them is moved out of the rule (#46759). - hint.css and 42 more: `z-index: 1000001` and `1000000` become the same number (#46715). - Adobe Spectrum: `--version: 5.0.1` prints as `5.1`. - A quarter of all stylesheets: minified output has a stray space in `align-items: center` (#41993). #### Behavior changes to look at first 1. **`light-dark()` is kept as written at the default browser targets** (#42009). The lowered form `var(--buncss-light, a) var(--buncss-dark, b)` is invalid unless a compiled `color-scheme` covers the element: 184 of the 215 real stylesheets that get it never define the variable, so every browser drops the declaration. The docs section on the polyfill is removed. 2. **`border` is not built from longhands** (#42072): `border-width:0;border-style:solid;border-color:red` stays three declarations, because `border` also resets `border-image`. 305 real stylesheets grow by 30 KB in total; `bootstrap.min.css` by 113 bytes, 5 gzipped. No real stylesheet was found where the old merge shows, so this is the one to drop if size matters more. 14 existing expectations change. 3. **A CSS module no longer exports names that only occur inside `:global(...)`** (#45487). 73 real modules lose 202 export keys, all of them names that no rule has. `import { g }` or `composes: g` of such a name becomes a build error. 4. **Container names and `@counter-style` names stay global in a CSS module** (#45247 by @mikamikasuki). `main` hashes the `@container` name but not `container-name`, so a named query never matches. postcss-modules (Vite, css-loader, Next.js) keeps them global; lightningcss and esbuild hash both. Global keeps a container that is queried from another file or from JS working. 5. **Syntax errors are build errors, also two that `main` lets through**: a media query that stops short (`@media screen and {`, `@media not {`, `@import "a.css" screen and;`) and `@media (width <) foo {`. No stylesheet of the 10,116 has either. A condition that is valid by the grammar though no browser knows it, `( ... )` or `fn( ... )`, is kept as written (#46756): `(min-width:0\0)`, `(not(hover))`. 6. **Declarations after a nested rule stay where they were written**, as a nested declarations rule (#45462). Changes the output of 199 real stylesheets; in a fuzz of 2,850 nested programs compared by computed style, `main` was wrong in 1,056 and the pull request's code, before it was trimmed here, in none. 7. **`Bun.color`**: `color()` inputs are converted instead of returned as a string (`Bun.color("color(srgb 1 0 0)", "hex")` was `"color(srgb 1 0 0)"`); `currentColor`, system colors and `light-dark()` give `null` in the converting formats instead of the keyword as a string; a relative color or `color-mix()` outside sRGB is clipped, not gamut-mapped (`hsl(from oklch(100% .399 336.3) h s l)` is `#ff5bff`, was `#fff`). 8. The `:-moz-any()` and `:-webkit-any()` copies of rules with `:is()` are gone at the default targets (#45945, #42029). The `:-webkit-any()` copy outranked the original rule: `.title{color:blue} :is(h1,h2,h3){color:red}` painted red. 5.8% of all default-target output was the `:-moz-any()` copies. 9. `Property` grows from 216 to 224 bytes: `Url` stores its text as a slice so that two `url()` values can be compared (#46895). The pull request packed a pointer and a `u32` behind `unsafe` to avoid that. #### Not in this PR, needs a decision - **Raise the default browser targets (#40369) and add a `cssTarget` option (#40368).** Today: Chrome 80, Edge 80, Firefox 78, Safari 14, Opera 67, and no public way to change them. At Vite 8's floor the output of the 9,924 stylesheets is 15.3% smaller (3.9% gzipped). About a dozen fixes here only matter at today's floor, and code for older targets is unreachable without the option. - Vendor prefixes for `user-select`, `appearance`, `hyphens` and five more (#45814): 1,116 real stylesheets lack a prefix they need at the default targets. And for `backdrop-filter` (#44648): 173. - Clip instead of gamut-map in the sRGB fallback of a plain wide-gamut color: deletes about 90 lines, changes every pinned fallback hex. If the mapping stays, #44647 by @oddharsh makes it closer to what browsers paint (colors off by 2/255 or more: 168 to 110 of 3,170 real ones). #### Changes **Rules: duplicates, merging, nesting, fallbacks** - The `:-moz-any()` copy of every rule with `:is()` or `:not(a, b)` is gone: Firefox 78 has `:is()` (#45945). 1,164 real stylesheets - Keep declarations written after a nested rule in place with a nested declarations rule (#45462). 379 real stylesheets - With nesting compiled away, an unknown at-rule inside a style rule was moved out of it: `.wrapper{@apply flex}` printed a bare `@apply flex;` and the rule was gone, so a CSS module exported a class that no rule defines. It now stays in a block under the compiled selector. 72 real modules with Tailwind's `@apply`, 25 with Mantine's `@mixin` (#46759). 316 real stylesheets - Keep !important in the rules generated for logical properties and color-scheme (#38597). 311 real stylesheets - With browser targets, remove an earlier duplicate rule only when its declaration block is equal (#46861). 171 real stylesheets - Compare custom property names in `PropertyId` equality and importance in duplicate-rule detection (#33684). 40 real stylesheets - Keep same-condition @supports rules and empty named @layer blocks (#46733). 31 real stylesheets - Keep target-added vendor prefix passes on cloned and re-minified rules, and never print a bare `:full-screen` (#43339). 12 real stylesheets - Keep the selectors on rules added for color fallbacks and logical properties (#43240). 3 real stylesheets - A later declaration of a property drops the `@supports` color copies of the earlier one, which printed after the rule and won (#45560). 2 real stylesheets - Do not merge style rules in a stylesheet that holds `revert-rule` (#46953). 1 real stylesheets - `@import ... supports(-webkit-mask: none)` kept its prefix (#38589) **Properties and shorthands** - Print align-items:center without a stray space when no safe/unsafe keyword is given (#41993). 3,317 real stylesheets - Emit the physical longhands instead of the inset shorthand for targets without it (#38583). 1,452 real stylesheets - Keep the stops of an upward gradient as written, and drop a `50%` hint only at the midpoint (#46790). 104 real stylesheets - Emit unparsed border-block-* declarations when compiling logical borders (#38582). 89 real stylesheets - Keep repeated font families and dedupe only the inserted system-ui fallbacks (#46662). 88 real stylesheets - A `width` / `height` / `min-*` / `max-*` with `var()` or `env()` was printed before the plain fallback written above it, so the fallback won (#42101). 26 real stylesheets - Keep box-orient and box-direction when flex-direction or flex-flow follows (#45672). 16 real stylesheets - Write held prefixed background declarations at every flush (#46882). 6 real stylesheets - Derive equality for Size2D and BorderRadius, and keep every prefix in the border-radius handler (#45665). 2 real stylesheets - Print `none` instead of an empty border-image, mask-border or mask-box-image value (#45143). 2 real stylesheets - Keep font/transition sub-properties on the side of the shorthand they were written on (#41990). 2 real stylesheets - CSS-wide keywords are not names: `font:12px Arial;font-family:inherit` printed `font:12px inherit`, `color-scheme:inherit` and `color-scheme:auto` printed `normal`, `transition-property:unset` was folded into `transition` (#42022). 2 real stylesheets - Only fold font longhands into `font` when the rule declared a `font` (#45459) - Keep a font family name quoted when its identifiers do not read back as the same name (#46606) - Stop rewriting valid positions into ones browsers reject (#45197) - `border` is no longer built from longhands unless the rule also sets what `border` resets (`border-image`), and an author's `border-image` reset is no longer dropped. `transition` is likewise only written where the rule wrote one, because it resets `transition-behavior` (#42072) - `border; border-image: <gradient>; border-image-slice: 1` lost the gradient in every default build (#42018) - A prefixed `linear-gradient` keeps its direction, written or generated: the prefixed syntax names the starting side and measures angles differently (#46697) **At-rules** - Print @font-face format() keywords as strings (#37979). 439 real stylesheets - Print @supports `not` conditions without a leading space (#38559). 88 real stylesheets - Reset after the number token of a unicode-range (#45835). 33 real stylesheets - Keep `<general-enclosed>` media and container conditions as written (#46756). 24 real stylesheets - A strict range such as `(width < 768px)` lowered for old targets still matched at exactly 768px. The step is now `.02px`. The `not all and` form for other units is not taken (#46806). 18 real stylesheets - Keep scanning an @supports condition after a nested block (#38552). 9 real stylesheets - Every `@counter-style name` printed as `@counter-stylename`, which browsers drop (also #46692). In a CSS module the name now stays global, like the `list-style` that refers to it (#33679). 9 real stylesheets - Keep a keyframe whose block has an invalid item (#46288). 2 real stylesheets - Read an @Property initial-value on the stylesheet's parser, keep an unmatched value as written (#45806). 2 real stylesheets - `@media {}` is `all`. A media query that stops after `and`, `or` or `not` (`@media screen and {`) is a build error; `main` printed `not all` or dropped the condition, silently. The `@layer` and type selector parts are not taken (#46869). 2 real stylesheets - Accept a name-only `@container` and a `style()` feature with no value (#46689). 1 real stylesheets - Parse `<timeline-range-name> <percentage>` keyframe selectors (#45652) - Allow `or` inside parentheses after a media type (#45088) - Print the `=` comparison in media and container queries as `=` (#45071) - Parse the @font-palette-values font-family descriptor as a list of family names (#45484) **Selectors** - Never wrap a selector list in :is() (#46787). 414 real stylesheets - Stop emitting :-webkit-any() copies of :is() that outrank the original rule (#42029). 373 real stylesheets - With browser targets, a selector list with a vendor-prefixed member was split, which revives hacks that rely on other engines dropping the whole list: `.s *::-ms-backdrop,.s{flex:auto}` applied everywhere. Rewritten; the list is printed as written (#42100). 42 real stylesheets - Print the vendor prefix on downleveled :any-link, :read-only, :read-write, :placeholder-shown and :autofill (#42131). 12 real stylesheets - Fix nesting lowering of a `&` that does not lead its selector (#43047). 8 real stylesheets - `@scope`: `&` and implicit selectors compile to `:where(:scope)`, relative selectors and bare declarations parse (#43165). 7 real stylesheets - Only reject bare :global / :local pseudo-classes in CSS modules (#38534). 5 real stylesheets - Stop truncating `@scope to (...)` and stop replacing `&` in the scope-end selector (#43146). 2 real stylesheets - `[xl|href$=".pdf"]` lost its namespace prefix (#45159). 2 real stylesheets - Match An+B keywords exactly and take only `+` as the leading sign (#42953). 1 real stylesheets - Parse ::details-content and ::picker() like ::part() (#45504). 1 real stylesheets - Parse :window-inactive as an ordinary pseudo-class, and accept it after ::selection (#45815) - Print :nth-col(1) and :nth-last-col(1) in functional form (#41575) - `li::before::marker` failed the build (#45810) - `&` inside `:nth-child(... of S)` is resolved when nesting is compiled away. The second selector serializer (326 lines) is deleted (#43024) **Colors and `Bun.color`** - Keep light-dark() as written under the default browser targets (#42009). 227 real stylesheets - Number and percentage channels parse in `lab()`, `lch()`, `oklab()`, `oklch()`, `hsl()` and `hwb()`; the hue of `lch` / `oklch` is no longer scaled by alpha (#45084). 130 real stylesheets - `Bun.color` returns `null` for `currentColor`, system colors and `light-dark()` in the converting formats, instead of the CSS keyword as a string (#33047). 130 real stylesheets - Relative colors: channel keywords are on the scale of their function (`rgb(from #369 calc(r + 10) g b)`), the origin's alpha is kept, a relative color kept as written gets no `@supports` copies, and a result outside sRGB is not folded (#45362). 31 real stylesheets - Keep rotate: 0deg out of none, and keep the origin alpha in relative colors (#38486). 13 real stylesheets - Compute the D50 reference white in f32 so greys convert to lab(l 0 0) like lightningcss (#38535). 7 real stylesheets - Return a missing hue for a grey from conversions into lch, oklch, hsl and hwb (#45201). 5 real stylesheets - Fix color(a98-rgb), srgb-linear relative colors and color-mix(in xyz-d50) (#38487). 4 real stylesheets - Match named colors without regard to ASCII case (#45092). 3 real stylesheets - Fold min(), max() and clamp() over plain numbers (#39515) - `calc(l - .08)` in a percentage channel of a relative color became NaN and printed `#000` (also #45970) (#45971) - Bun.color: convert color() inputs instead of returning them as a CSS string (#38488) - A `color-mix(in srgb, ...)` result outside sRGB prints `color(srgb ...)` instead of a gamut-mapped hex (#39269) **`calc()` and numbers** - Rounding to 6 significant digits ate into the integer part: `z-index:2147483647` became `2147480000`, `max-width:9999999px` became `10000000px`, `--x:1234567.5` became `1234570`. It now only cuts a fraction short: a value kept as tokens prints its source text, a typed value prints every integer digit and at least one digit of the fraction (#46715). 43 real stylesheets - A number without a unit is no longer repaired into `px` (`height:1.5715` is dropped by a standards-mode browser, so it prints as written), `calc(-1 * 0)` is not a length, and on a property of the range [0,∞] (sizes, border radii, padding, border and outline widths, gap, `flex-basis`, `font-size`) a `calc()` that folds below zero prints `0` instead of an invalid negative literal, and a negative literal no longer replaces the valid declaration before it (`width:10px;width:-5px` printed `width:-5px`) (#42104). 24 real stylesheets - Print numbers from the f32's shortest digits, not its f64 expansion (#38565). 17 real stylesheets - Print negative numbers below 1 with a single minus sign (#38516) - Keep math functions over two different length units as written (#38489) - Fold rem() with a truncated quotient, so it keeps the sign of the dividend (#32286) - A one-argument `min()` / `max()` around a sum printed without any function (`width:100% - 20px`) (#46145) - `round()` breaks ties upward and handles a negative step as browsers do (#45249) **CSS modules** - A CSS module exports its `@keyframes` names (`styles.spin`). Hashing is unchanged (#46720). 145 real stylesheets - A name inside `:global(...)` prints plain but was exported as `name_<hash>`, a class no rule has (also #38573) (#45487). 90 real stylesheets - Keep container names global in CSS modules (#45247). 6 real stylesheets - Tag the keyframes name when an `animation` value falls back to raw tokens (#46034). 6 real stylesheets - In a plain stylesheet `composes: a from "./b.css"` was treated as an import: the file was inlined and an absolute path printed (#45961). 4 real stylesheets - Drop a rejected composes instead of failing the file at print time (#38538). 1 real stylesheets - Keep every rule's custom properties for the composes conflict check (#38523) - Keep composes working after a nested rule in the same block (#38520) - `IdentOrRef::hash` read 2 of 16 bytes, so in release builds every class of a module hashed alike (#30784) **Tokens, `url()`, diagnostics** - The block skip stack no longer allocates on every block exit (#39533) - Report the diagnostic's line without its line breaks in position.lineText (#38342). 192 real stylesheets - Compare `url()` values by their text (#46895). 26 real stylesheets - Values kept as tokens keep the blanks they were written with instead of having them re-invented at print time: `attr(x type(<number>))` printed `type(< number> )`, which browsers drop. Minified output still drops the blanks that cannot matter (#45577). 19 real stylesheets - The IE star hack (`*zoom:1`) failed the whole build. The declaration is skipped, as every browser does. It is the one syntax error that builds: tachyons, handsontable and jasmine ship it (#46751). 8 real stylesheets - The path of a copied asset spliced into `url()` is always quoted, so a file name with a space or a parenthesis cannot end the `url()` (#41557). 7 real stylesheets - Move the skip loop of parse_until_before out of the generic body (#39520) - Remove dead code from bun_css: the unused `impl Default for Property` (#45618) - Default the at-rule and qualified-rule parser traits, drop the url_to_css shim (#31999) **Speed** Four inputs that took quadratic or exponential time, or produced output out of proportion, are now linear, with no new limit and no extra pass (#31639, #42243, #38531, #43421). **Tests only** - `css-fuzz.test.ts` never ran in CI and timed out on debug builds; it is a deterministic table now (#38500) - `nth-anplusb-ident.test.ts` counts heaps in a child process (#46331) - `color.test.ts` checks the ansi-16 sweep with one `expect()` (#42824) - Not CSS, but it failed this PR's CI and fails on `main` too: `ws-proxy.test.ts` on Windows, "a message sent during a TLS renegotiation arrives after it". The helper server now renegotiates from the callback of the frame's write (#46170) **No pull request covered these** - `--v: 5.0.1` printed as `5.1`, `--w: 1.0.0` as `10`, `--x: 1+2` as `12`, `2n+1` as `2n1`: a number token that touches another keeps its source text. - `color-scheme: auto` printed as `normal`, and `color-scheme: dark light` as `light dark`. - A bare `:global` or `:local` in a CSS module sets the scope of the names after it, up to the end of the selector, as in css-loader. It was a build error. - The scrollbar states (`a:horizontal`, `::-webkit-scrollbar{&:horizontal{}}`) are valid in a plain compound, as in WebKit and Blink. - Without browser targets, `-webkit-box-align`, `-ms-flex-align` and the like were dropped next to their standard form (flatpickr, foundation-sites, materialize-css: 28 stylesheets), and `-webkit-fill-available` before `-moz-available` (beercss). - `(-o-min-device-pixel-ratio: 2/1)` printed as `2`. - `sin()`, `cos()` and `tan()` are exact at multiples of 90deg: `calc(100px * cos(90deg))` printed `-.00000437114px`. - A `display-p3` color at targets that have P3 but not the other `color()` spaces (Safari 10.1 to 14) got itself as a fallback. - `width: rem(-18px, 5px)` folds to a negative literal once `rem()` is right; it prints `0`. - Consecutive nested declarations rules, which arise when same-query `@media` blocks in one rule merge, are folded into one and count against the existing selector expansion limit as the `&{}` rules they replace did. #### Known and not fixed here - Four longhands are still folded into a `transition` the author did not write where the targets add a prefixed copy of a property (`mask-image` at Chrome 80): only the shorthand gives the copy its own times. - A later `background`, `box-shadow` or `border-*` declaration still loses to the `@supports` copy of an earlier `var()` value of the same property with a wide-gamut color. Same on `main`; #45560 is taken for the properties of the fallback handler and for unparsed values only. - A merged rule prints the unprefixed name in the passes of the rule it absorbed: `.d::-webkit-input-placeholder{}.d::-moz-placeholder{}` prints `::placeholder` for the second (#42106, not taken: it breaks the merge of prefix-equivalent rules). - The `border`, `outline` and `font` shorthands, `line-height`, `scroll-padding`, `background-size` and a few more still print a `calc()` that folds below zero as a negative literal. No real stylesheet has one. - `all` is not a property Bun knows, so `a{margin-top:1px;all:unset}` prints `all` first. Same on `main`. - A selector kept as raw tokens loses the blank before `*` when minified: `--sel: .dark *` prints `.dark*`. Same on `main`. - `width:calc(0% + 12px)` folds to `12px`, which lays out differently in a table cell. Same on `main`. - The `:dir()` / `:lang()` rules of a lowered logical property print before the nested rules of their rule, so a logical declaration written after a nested rule of equal specificity still loses to it. Same on `main`. - A fraction is still rounded to 6 significant digits (`12345.678` prints `12345.7`, `33.3333333333%` prints `33.3333%`). Keeping every digit would add up to 282 KB over the corpus. - `@keyframes "quoted"` is hashed in a module but not exported. - In a module, a keyframes name inside a `var()` fallback is not hashed: `animation: var(--enter, fade 1s)`. Same on `main`. - A `color-mix()` or relative color folded to a hex inside a gradient changes the interpolation from Oklab to sRGB (VS Code, clawhub). Printing `color(srgb ..)` instead needs a fallback for `mask-image`, `list-style-image` and custom properties at the default targets, which have none. - A comma list in `@container` still fails the build. - After a bare `:global`, names that are hashed at print time (`::view-transition-old(foo)`) are still hashed; inside `:global(...)` they are not. #### Pull requests in scope that do not land here - Duplicates of a pull request that lands here: #36961 (of #37979), #38478 (of #46662), #38480 (of #46733), #38482 (of #46715), #38491 (of #38500), #38508 (of #45201), #38513 (of #45362), #38573 (of #45487), #38640 (of #38500), #45525 (of #45247), #45589 (of #46756), #45970 (of #45971) - Already fixed on `main`: #38674 - Right, but for input nobody writes (0 of 10,116 stylesheets, and not new syntax): #31916, #38526, #38592, #39349, #41139, #41801, #42042, #42484, #43078, #45604, #45908, #46086, #46221, #46224, #46273, #46694, #46707, #46737, #46755, #46816, #46870, #46934 - Right or harmless, but not worth their code, their bigger output or the behavior they change: #30905, #31924, #33332, #34426, #36225, #38006, #38512, #38547, #38551, #38561, #38572, #38576, #38607, #38623, #38667, #38692, #42106, #42912, #42918, #43407, #43434, #44700, #45442, #45546, #45747, #45759, #45825, #45946, #46061, #46127, #46206 - Need a decision (see above), stay open: #40368, #40369, #44647, #44648, #45814 - Stays open: #45787 Out of scope: CSS linking and chunking in the bundler, the dev server, docs-only pull requests, and repository-wide sweeps that touch `src/css` in passing. Thanks to @mikamikasuki (#45247), @alii (#39520, #31999) and @oddharsh, whose measurements on #40369 and #41990 were used. Fixes #16727. Fixes #24770. Fixes #30772. Fixes #44704. Fixes #45089. Fixes #45237. Fixes #46872. ### How did you verify your code works? - The 35 CSS test files pass on a debug build, also with `FORCE_COLOR=1`. 600+ of the tests in this branch fail on a build of `main`. An earlier head of this branch passed all 179 CI jobs that ran. - The reproduction of each issue listed above was run on `main` (wrong) and on this branch (right). - Differential run over the 10,116 stylesheets, `main` against this branch, minified without targets and through `Bun.build` at the default targets: no stylesheet that builds on `main` fails; Chromium 156 compares the output with the input rule by rule (CSSOM, then computed values on a sized element in a standards-mode document, colors as painted pixels in sRGB and P3). The comparison has 195 control pairs that must read equal or different. For stylesheets that read differently on both builds, mostly because of lowering, the sets of differences were compared; the three new ones are two minifier fixtures that are wrong on both and one comparison artifact. - CSS modules are checked for consistency instead, since hashed names defeat the comparison: every reference points at a name the output defines, every export exists. - `Bun.color`: 59 inputs in 13 formats on both builds; every changed cell was read.
Collaborator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
bun buildfoldstop:0;right:0;bottom:0;left:0intoinset:0and keeps an author'sinset:0. The default targets (Chrome 80, Safari 14) dropinset, so an overlay covers nothing.SizeHandlerImpl::flush(src/css/properties/margin_padding.rs:843) isspec.shorthand_category != Logical || logical_supported. Every spec row isPhysical, so it never reads the targets.Fix
SizeSpec.shorthand_categorybecomes the boolshorthand_needs_feature,truefor inset only. WithoutLogicalInset,flushprintstop,bottom,left,right, as lightningcss does.PropertyCategory::Physical, the value every row had, so output for targets withinsetdoes not change.test/js/bun/css/css.test.ts(25insetrows fail without the fix) andcss/InsetFamilyDefaultTargets-*intest/bundler/esbuild/css.test.ts. Also rantest/js/bun/css/andtest/bundler/css/.Background
SizeHandlerbuffers a block's margin, padding, scroll-margin or inset declarations.flushwrites four sides back as one shorthand.Feature::LogicalInset(src/css/compat.rs) is the version row of the inset family: Chrome 87, Safari 14.1. It already compilesinset-blocktotopandbottom.Logicalfor inset: eachinsetthen flushes the buffered sides, and 8,000 merged.a{inset:0}rules print 64,003 bytes (Bun: 11).Downsides
test/js/bun/css/files. No option sets the targets.@mediablocks with aninsetvalue the targets lack cost as four longhands do: 4.6 s for 1.7 s at N=4000 (inset:1dvh, Chrome 80).bun build --no-bundle(bun build --no-bundle: minify css with the build target's browser targets #38544) andinsetin@keyframes.Notes
History of this PR. The first form (Aug 14) added
PHYSICAL_SHORTHAND_FEATURE: Option<Feature>to theSizeHandlerSpectrait. #39770 then replaced the trait constants with one staticSizeSpectable per handler, so no source hunk applied. This form is written on the table. The contract and the 11 test cases of the first form stay. AnOption<Feature>column on the table repeats theLogicalInsetlookup on each inset flush. The bool reuses the answer thatflushalready has.Deleted.
SizeSpec.shorthand_categoryand thecategory:argument ofsize_handler_spec!. The field had two readers: the fold guard inflush, and the four flush checks of the 4-side shorthand arm. Every row hadPhysical, so the guard was constant and the arm always compared againstPhysical.Difference from lightningcss that stays. lightningcss passes
Logicalfor inset to the flush checks of the shorthand arm. It then prints an earliertoporinsetas a separate declaration. Bun replaces the buffered value, as before this PR..a{inset:1px;inset:0}.a{inset:0}.a{inset:1px;inset:0}.a{top:1px;inset:0}.a{inset:0}.a{top:1px;inset:0}.a{inset:1px;inset:0}.a{top:0;bottom:0;left:0;right:0}.a{top:1px;inset:0}.a{top:0;bottom:0;left:0;right:0}lightningcss
test_position. It has 12 inset cases. 11 pass with this PR (10 before). The case that still fails isinset-block-start:2px;inset-block-end:3px;inset:4px;inset-inline-start:4px;inset-inline-end:5px: Bun prints the overriddeninset-blockbeforeinset. It is a todo row. #38576 changes that arm.Default output. The 15 sheets of
test/js/bun/css/files, each throughbun build <file> --minify, on release builds of main (9634520) and of this branch.insetdeclarations on mainThe other 6 sheets are byte-identical. Total: 2,732,962 to 2,734,496 bytes raw, 290,279 to 290,517 bytes with
gzip -9. The only difference between the outputs is 71insetdeclarations that become 71 sets oftop,bottom,left,right.Measurements. Release builds of main (9634520) and of this branch. The two trees differ only in
margin_padding.rs. Tools: gdb, nm, size, objdump. valgrind, perf and bloaty are not installed on the machine.cmp). These are the 15 sheets and 67 declaration sequences through the internal minify entry, each with no targets and with Chrome 87, Edge 87, Firefox 63, Safari 14.1. Also 8 margin, padding and scroll-margin inputs at Chrome 60, Firefox 50, Safari 10, and the 15 sheets throughbun build --target=bun --minify.Feature::is_compatibleruns 5,000 times for 1,000.aN{inset:0}rules at Chrome 87 and 3,000 times at Chrome 80, on both builds (gdb breakpoint hit count)..aN{top:0}gives the same counts.SizeSpectables is 304 bytes on both builds. The bool is at the offset of the enum byte (0x126).nm -S):flush1,950 to 1,984 bytes,handle_property3,407 to 3,382,flush_helper_physical283 to 285. That is +11 bytes.sizeof the stripped binary gives text 107,350,736, data 116,336 and bss 1,819,264 on both builds.stepicount, no targets): oneinset:0ormargin:0declaration throughhandle_property368 to 355.flushwith four sides 434 to 427.flushwith one side 407 to 403. An emptyflushis 18 on both. The guard is 3 instructions where it was 6, with 1 conditional branch on both.@mediaflood: N copies of@media (min-width:1px){.a{inset:1dvh}}at Chrome 80.1dvhis a value that Chrome 80 lacks, so each copy stays as a fallback. Visits of the inset handler (gdb conditional breakpoint): 10,099 to 40,096 at N=100 and 40,199 to 160,196 at N=200, which is N²+N-1 to 4N²+N-4. User CPU, medians of 5 interleaved runs: 96 to 279 ms at N=1000, 387 to 1,128 ms at N=2000, 1,665 to 4,641 ms at N=4000 (spread at N=4000: 109 ms on main, 179 ms on this branch). The same flood withtop:1dvhtakes 1.2 s on both builds, so the path is quadratic on main too (css: minify a run of merged same-query @media rules once #38531). Withinset:0it takes 7 ms on main and 11 ms on this branch.Tests.
test/js/bun/css/css.test.ts,describe("inset"): the 11 cases of the first form, both sides of eachLogicalInsetboundary (Chrome, Edge, Firefox, Safari, iOS Safari) and IE 11, the states that must not change (no targets, and targets withinset), four-longhand folds of margin, padding and scroll-margin at old targets, Chrome 80 rows for merged rules, a nested rule, an@mediarule and three sides, and the 12 inset cases of lightningcsstest_position.test/bundler/esbuild/css.test.ts:css/InsetFamilyDefaultTargets-cliand-apibuild one sheet with the default targets and expecttop,bottom,left,rightin place ofinset. A change of the default targets (css: bump the default browser targets to Vite's baseline widely available set #40369) must update these rows.css/InsetFamilyTargetBunkeepsinsetfor--target=bun.insetblock and the 2 default-target rows fail. With the fix:css.test.ts1375 pass, 0 fail.test/bundler/esbuild/css.test.ts60 pass, 0 fail.test/bundler/css/184 pass, 0 fail.test/regression/issue/25794.test.tspasses.test/js/bun/css/on the ASAN debug build: 2574 pass, 8 fail. The 8 are tests with a 5 s or 10 s budget incss-fuzz.test.ts,nested-selector-expansion.test.tsandtoken-list-backtracking.test.ts. They time out in the debug build. On release builds of main and of this branch all 15 tests of those three files pass (8.3 s and 8.8 s).Not in this PR.
bun build --no-bundle x.cssgives the minifier no targets (src/bundler/transpiler.rspassesMinifyOptions::default()), so no lowering runs there. See bun build --no-bundle: minify css with the build target's browser targets #38544.@keyframesdo not pass through the property handlers (src/css/rules/mod.rs, theCssRule::Keyframesarm).inset: var(--x)stays as written, as in lightningcss.inset-inline-start:5px;inset:0at old targets: the shorthand flushes the logical value first, as on main. See css: drop the logical longhands a 4-side margin/padding shorthand overrides instead of flushing them #38576.inset.Feature::LogicalInsetis the row ofinset-inline-start(Firefox 63), and the shorthand needs Firefox 66. lightningcss has the same gap. No build reaches these versions today: the default floor is Firefox 78.Other open PRs on this handler. #38623 adds
self.category = spec.shorthand_categoryto the shorthand arm. After this PR that line must sayPropertyCategory::Physical.git merge-fileon the two versions ofmargin_padding.rsgives 0 conflict hunks and that 1 reference. 3 of its 24 inset rows expectinsetat Safari 8 and change (read from its test, not run). #38551 and #38576 need the same one-token change. AScrollPaddingSpec(#38561) passesshorthand_needs_feature: false.[human-review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
root cause · written by the author bot
The CSS minifier unconditionally collapsed the four physical inset longhands (top, right, bottom, left) into the
insetshorthand, even for browser targets that do not support it, so output silently broke on older browsers. The fix in the size-property handler feature-gates shorthand emission forinseton the configured targets while leaving margin, padding, and scroll-margin collapsing unchanged, so the longhands are preserved when the target lacks support. Tests now cover inset expansion and preservation across targets, value forms, declaration ordering, and bundler output.