Repository navigation
Conversation
…d values
The margin/padding/inset/scroll-margin handler and the border handler
track whether the values they are buffering are physical or logical so
that a change of category flushes the buffered values as a fallback.
Their 4-side shorthand arms (`margin: ...`, `border: ...`) replaced the
buffered values without updating that category, and the handlers are
reused for every declaration block in a stylesheet, so a logical
property in any earlier block left the category at Logical and made a
later block's shorthand flush instead of being overwritten or folded:
div{margin-inline-start:2px}.a{margin:1px;margin:0}
-> div{margin-inline-start:2px}.a{margin:1px;margin:0}
Set the category in both shorthand arms, as every other arm does, so a
block minifies the same regardless of what preceded it.
Collaborator
Author
|
Updated 11:56 AM PT - Oct 21st, 2026
❌ @robobun, your commit b127b71 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38551That installs a local version of the PR into your bun-38551 --bun |
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 13 minutes. View limit details
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 |
Collaborator
Author
|
Status: on hold, converted to draft. Do not merge before #42042, #33684, #45825 and #38583. Without them, this change makes Reproduced on main 449877f with this branch merged in (
Main already prints each of these for the same rule with nothing before it. The PR body has the table, and the result of each row with its fix merged under this branch. The missed merge that this PR fixes is still on main: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
LGTM — two one-line assignments bringing the shorthand arms in line with every other buffering arm.
What was reviewed:
- Confirmed both changed arms buffer only physical values and clear the logical slots, so
category = Physical(S::SHORTHAND_CATEGORY, which isPhysicalfor all four specs) matches what's actually buffered — same asproperty_helper/set_border_helper!do everywhere else. - Traced the intra-block behavior change (
.a{margin-inline-start:1px;margin:0;margin-top:5px}and the border equivalent): the pre-shorthand logical value is still flushed first viaflush_helper_physical/the category check, so source order is preserved; only the post-shorthand longhand now folds in, which is cascade-equivalent. - Checked that the
Property::Borderarm already resets all logical border slots without flushing (pre-existing, correct per spec), so recordingPhysicalafterward doesn't drop anything that wasn't already dropped. - 24 new
minify_testcases cover the cross-block leak for all four size handlers plus border, the!importanthandler instance, unparsedvar(), merged/dropped rules, and pin four intra-block outputs that must not change.
Extended reasoning...
Overview
Two single-line additions: self.category = Physical; in the Property::Border match arm (border.rs) and self.category = S::SHORTHAND_CATEGORY; in the S::SHORTHAND arm (margin_padding.rs), plus a 24-case test block in css.test.ts. These are the only two arms in either handler that buffer values without recording the category of what they buffered; every other arm does so via property_helper/set_border_helper!/logical_property_helper. The stale category leaked across declaration blocks because the handlers are reused stylesheet-wide, causing spurious flushes that defeated shorthand collapsing in later rules.
Security risks
None. This is a CSS minifier optimization; the worst possible outcome is larger-but-still-correct CSS output. No untrusted-size arithmetic, no memory ownership, no user-facing API surface.
Level of scrutiny
Low-to-medium. The source change is mechanical — a single field write matching the pattern already used on every sibling arm — and the failure mode (an unnecessary flush) is by design harmless for correctness. The one place this changes output within a single block (logical → shorthand → physical longhand) was traced and produces cascade-equivalent CSS; the tests pin both the changed cases and four cases that must stay unchanged.
Other factors
The PR description is unusually thorough: it identifies the mechanism precisely, cross-references upstream lightningcss (same bug), verifies against release 1.4.0 and an unfixed debug build (20/24 new tests fail without the fix), and runs the full css and bundler-css suites. Test coverage spans margin/padding/inset/scroll-margin, both handler instances (!important), unparsed var() triggers, unrelated rules in between, dropped @media not all rules, and merged same-selector re-minification. No CODEOWNERS on src/css/. No prior reviewer comments to address.
This was referenced Aug 14, 2026
Contributor
There was a problem hiding this comment.
LGTM — two one-line assignments bringing the shorthand arms in line with every other buffering arm, with thorough test coverage.
What was reviewed
- Traced
SizeHandler::handle_propertyandBorderHandler::handle_property: every other arm that buffers a value setsself.category; the shorthand arms were the only ones that didn't, and now do. - Checked the intra-block behavior change (
margin-inline-start;margin;margin-topand theborderequivalent): the folded outputs are cascade-equivalent to the inputs, and the four pinned no-change cases confirm flush ordering between categories is preserved. - Tests reuse the existing
minify_testhelper and cover the cross-block leak for all four size handlers plus border,!important, unparsedvar(), merged same-selector rules, and a dropped@mediarule.
Extended reasoning...
Overview
Two one-line changes to the CSS minifier's property handlers plus 24 new minify_test cases. In src/css/properties/margin_padding.rs the S::SHORTHAND arm of SizeHandler::handle_property now sets self.category = S::SHORTHAND_CATEGORY after buffering the shorthand's four values; in src/css/properties/border.rs the Property::Border arm now sets self.category = Physical. Both mirror what property_helper/set_border_helper! already do on every other arm that buffers a value.
The bug was that a stale category from a logical property in an earlier rule (handlers are reused per stylesheet) caused a spurious flush between two consecutive physical shorthands in a later rule, so margin:1px;margin:0 stopped collapsing. The fix restores the invariant that category always matches the category of the currently buffered values.
Security risks
None. This is a minification-quality fix in the CSS printer. The pre-fix output was already valid CSS with the same cascade result, just larger; the change only affects how aggressively adjacent declarations are folded. No untrusted-input parsing, no memory ownership, no cross-thread state.
Level of scrutiny
Low-to-moderate. The production change is two lines, mechanically identical to what neighboring arms already do, and the failure mode of getting this wrong would be an extra flush (larger but still correct output) rather than incorrect CSS. I traced the two intra-block cases whose output does change: .a{margin-inline-start:1px;margin:0;margin-top:5px} now emits margin:5px 0 0 instead of margin:0;margin-top:5px (identical cascade), and .a{border-inline-start-color:red;border:1px solid red;border-top-color:red} now drops the redundant border-top-color:red (the Border arm was already resetting border_inline_start before this change, so the logical value was already being discarded — only the redundant physical longhand after it is now folded).
Other factors
- 24 new
minify_testcases in the existingtest/js/bun/css/css.test.ts, covering the cross-block leak for margin/padding/inset/scroll-margin, the border variant,!important(separate handler instance), unparsedvar()andmargin-inlinetriggers, merged same-selector re-minification, an@media not allrule that's dropped, plus four pinned outputs that must stay unchanged (verifying category-switch flushes still preserve source order in both directions). PR states 20/24 fail on the unfixed build. - The change intentionally diverges from upstream lightningcss 1.30.2, which has the same bug; the PR description documents this and the divergence is a strict improvement in output size with identical semantics.
- No CODEOWNERS for
src/css/. No prior human review comments to address.
This was referenced Aug 14, 2026
The handlers are reused for every declaration block, so minify_test, prefix_test and cssTest now additionally minify each source behind a dropped `@media not all` rule that leaves the logical/physical and prefix tracking handlers (normal and !important) in a non-default state, and require the output to be unchanged. Without the previous commit this flags 8 existing cases (border, margin and length shorthand folds); with it the file passes.
Contributor
There was a problem hiding this comment.
LGTM — the two-line source fix records category on the shorthand arms exactly like every sibling arm already does, and the output remains semantically-equivalent CSS in all cases (only smaller).
What was reviewed:
- Traced both handlers'
categoryinvariant: every other buffering arm (property_helper,set_border_helper!,logical_property_helper) sets it; these two were the only ones that didn't. - Checked the intra-block behavior change (
.a{margin-inline-start:1px;margin:0;margin-top:5px}and the border equivalent) preserves cascade order — the flush before the shorthand still runs, so source order is kept. - Reviewed the util.ts
precedingRuleguard: additive only, skips@charset/@import/@namespacecorrectly, and@media not allis dropped byMediaRule::minify'snever_matches()check so the expected output is unchanged.
Extended reasoning...
Overview
Two source lines: self.category = Physical; after Property::Border in border.rs, and self.category = S::SHORTHAND_CATEGORY; after the S::SHORTHAND arm in margin_padding.rs. Both arms fully replace the buffered values with physical ones but previously left category stale, which caused later declarations (in the same or a later block) to trigger an unnecessary flush and lose merging. The fix makes these arms consistent with every other arm in both handlers, all of which set category after buffering.
Tests: 24 direct cases in css.test.ts covering the cross-block leak for all four size handlers plus border, the !important handler instance, dropped/merged rules, and the four intra-block outputs that must not change. util.ts gains a systematic guard that reruns every existing minify_test/prefix_test/cssTest with a preceding @media not all{...} rule that dirties per-block handler state and is then dropped from the output — turning all 1166 existing tests into regression coverage for this bug class.
Security risks
None. This is CSS minification output-size behavior; the emitted CSS was already correct before the fix, just larger. No untrusted-input parsing paths are touched.
Level of scrutiny
Low-to-medium. The source change is mechanical (bring two arms in line with their siblings) and the failure mode is bounded to "correct but larger CSS". The one thing worth a second look was the util.ts test-infrastructure change since it affects every CSS test, but it is purely additive — it adds a second assertion per test without weakening any existing one, is well-commented, and passed on all CI lanes. The @media not all wrapper being dropped is guaranteed by MediaRule::minify's never_matches() path (src/css/rules/mod.rs:401).
Other factors
- No CODEOWNERS for the touched paths.
- The PR description traces the mechanism precisely, verifies against upstream lightningcss 1.30.2 (same bug there), and confirms 20/24 new cases fail on the unfixed build.
- The intra-block divergence from upstream (folding a longhand after logical→shorthand) produces the same cascade result as the input; this is pinned in the tests so it is intentional and reviewable.
- CI is green on the CSS lanes; the two unrelated failures are known flakes on Windows 2019.
Main replaced the generic `SizeHandler<S>` body with `SizeHandlerImpl`, which reads its configuration from a `SizeSpec` value. The constant `S::SHORTHAND_CATEGORY` no longer exists. The 4-side shorthand arm now reads `spec.shorthand_category`. The `flush_helper_physical` calls in that arm use the same field.
The `border` arm of `BorderHandler` never flushes. With the `category`
assignment there, `border; <logical border property>; border` in a
merged rule prints two `border` declarations where main prints one:
`.a{border:1px solid blue;border-inline-start-color:red}.a{border:2px solid green}`
goes from 26 to 48 bytes.
Remove the assignment, its four test cases, and the logical border
declaration of the second test pass.
…gets For targets that compile logical properties, an inline-axis declaration becomes `:dir()` rules. The minify of those rules puts the handler back in its physical state, so the second pass started from the same state as the first. The rule now uses block-axis declarations. Add cases with browser targets, two outputs that are longer after the merge, and two cases for the duplicate-rule check.
robobun
changed the title
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.
Beyond the inline findings, I also checked the harness re-run in test/js/bun/css/util.ts: the second expect only executes after the first assertion has passed, so it cannot mask or shadow a failure, and both calls go through the in-process cssInternals helpers rather than a subprocess, so the doubled run adds little time. The border arm being left out of the fix is stated in the PR description, so the sibling-handler exclusion is deliberate rather than missed.
Extended reasoning...
The change is a one-line self.category = spec.shorthand_category assignment in the 4-side shorthand arm of SizeHandlerImpl in src/css/properties/margin_padding.rs, matching what property_helper already does, plus a new describe block of ~30 minifier cases and a util.ts harness that re-runs every css test behind a dropped @ media rule. It touches no security-sensitive surface. The inline findings show the fix widens known fresh-rule defects (inset for old targets, duplicate-rule check ignoring custom properties) into rules following a logical property, and one test pins a wrong output, so a human should weigh those trade-offs before merging.
robobun
marked this pull request as draft
October 26, 2026 18:07
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
Collaborator
Author
|
Understood. The merge of the shorthand after a logical property stays as it is, and the extra bytes in the other cases are not worth the change. Nothing else to do here. |
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
div{margin-inline-start:2px}.a{margin:1px;margin:0}keeps both declarations, with the same meaning.bun build --minify(229,086 bytes, 228,616 here).margin_padding.rs:686) leavescategorystale, so the next declaration flushes the shorthand.Fix
category, asproperty_helperdoes. A stale value then only flushes an empty buffer.test/js/bun/css/css.test.ts(28 fail on thesrc/of main, 1341 pass here). Also rantest/js/bun/css/,test/bundler/css/,test/bundler/esbuild/css.test.ts.Background
categoryrecords which, and a change flushes the buffer.DeclarationHandlerper stylesheet holds every handler (css_parser.rs:2372), so their state carries over between rules.flush(): it misses a shorthand after a logical property in one rule. Theborderarm is left out on purpose (Notes).Downsides
div{margin-inline-start:9px}.a{margin:1px;margin-top:0;all:initial}becomes.a{all:initial;margin:0 1px 1px}, so the margin is not reset.bun_csscode.Notes
Mechanism
margin-inline-start:2pxsetscategory = Logical.flush()at the end of the rule empties the buffer and does not resetcategory.margin:1pxreaches the shorthand arm.flush_helper_physical(margin_padding.rs:742) sees a category change and flushes an empty buffer. The arm buffers the four values. Before this change,categorystayedLogical.margin:0sees a category change again. This time the flush writesmargin:1pxout, so the two declarations do not merge.The normal and the
!importantdeclarations of a sheet use twoDeclarationHandlervalues, so each has its owncategory. Bun also minifies a merged same-selector block after the rule that follows it. So.a{margin:1px}.a{margin:0}div{margin-inline-start:2px}keeps both declarations on main (51 bytes, 40 here).Inputs and outputs (
bun build --minify, debug builds of main f33c840 and of this branch)div{margin-inline-start:2px}.a{margin:1px;margin:0}div{margin-inline-start:2px}.a{margin:0}.a{margin:1px;margin:0}.a{margin:0}.a{margin:0}.a{margin-inline-start:1px;margin:0;margin-top:5px}.a{margin-inline-start:1px;margin:5px 0 0}div{inset-block-start:2px}.a{inset:1px;inset:0}div{top:2px}.a{inset:1px;inset:0}div{top:2px}.a{inset:0}.a{margin:0;margin-top:5px}behind a logical rule).inset-inline-startto:dir()rules. The minify of those rules resets the handler. So the inline-axis inset case (inset-inline-startbeforeinset:1px;inset:0) changes only without browser targets (--target=bun).margin:22px;margin:max(4%,22px)also changes only without browser targets. There it becomesmargin:max(4%,22px)behind a logical rule, as it already does in a fresh rule.Outputs that do not get shorter (same builds, sizes are for the whole output)
div{inset-block-start:2px}.a{inset:1px;left:5px}.a{inset:1px;left:5px}(34 bytes).a{inset:1px 1px 1px 5px}(37)div{margin-inline-start:2px}.a{margin:1.25rem;margin-left:0}.a{margin:1.25rem 1.25rem 1.25rem 0}(64)div{margin-inline-start:2px}.a{margin:1px;margin-top:2px}.b{color:red}.a{margin:2px;margin-top:var(--x)}.arule deleted (75).arule kept as.a{margin:2px 1px 1px}(97)flush()prints the 4-side shorthand when all four sides are set, with no length comparison. Each branch output above is what main prints for the same rule with no rule before it.Outputs that change meaning: the reason for the hold
In each row, main prints what the source means and this branch does not. Main prints the branch output when the
divor.xrule (or the leading logical declaration) is removed. So each defect is older than this change. The extra flush on main hides it after a logical property, and this change removes that flush.div{margin-inline-start:9px}.a{margin:1px;margin-top:0;all:initial}.a{margin:1px;all:initial;margin-top:0}.a{all:initial;margin:0 1px 1px}all:initialno longer resets the margin: it is0 1px 1px, not0allis printed before the buffered values (#42042).a{margin-inline-start:1px;margin:0;margin-top:5px;-webkit-margin-start:9px}.a{margin-inline-start:1px;margin:0;-webkit-margin-start:9px;margin-top:5px}.a{margin-inline-start:1px;-webkit-margin-start:9px;margin:5px 0 0}0, not9pxdiv{margin-inline-start:2px}.a{--x:1;margin:1px;margin:0}.b{color:red}.a{--y:2;margin:2px}.arule deleted with--x:1.aloses--xrules/style.rs:465) ignores custom property names (#33684, issue #44704).x{inset-block-start:2px}.y{inset:0;top:5px}.y{inset:0;top:5px}.y{inset:5px 0 0}insetignores the declaration, sotop:5pxis lostinsetis printed for targets without it (#38583)Bun.build({ minify: true })on debug builds. Three runs of each build print the same bytes.margin-top:0is the initial value. Withmargin-top:5px, main prints.a{margin:1px;all:initial;margin-top:5px}, which is also wrong.inset.Each row with its fix under this branch (local merges on main 449877f)
allshorthand reset the declarations before it #42042: the first row prints.a{all:initial}..a{margin-inline-start:1px;margin:5px 0 0;-webkit-margin-start:9px}. The third row prints.a{--x:1;margin:0}.b{color:red}.a{--y:2;margin:2px}. The base branch of css: resolve legacy aliases in the name table and keep them after the property they alias #45825 contains css: dispatch each declaration to the handler that owns its property #45747 and css: compare custom property names inPropertyIdequality and importance in duplicate-rule detection #33684, so this build has all three.bun_cssdoes not compile:error[E0609]: no field `shorthand_category` on type `&'static SizeSpec`(margin_padding.rs:731). css: emit the physical longhands instead of the inset shorthand for targets without it #38583 removes that field. WithPropertyCategory::Physicalon the new line, the fourth row prints.y{top:5px;bottom:0;left:0;right:0}.allshorthand reset the declarations before it #42042 and the css: resolve legacy aliases in the name table and keep them after the property they alias #45825 stack conflict with each other: 4 hunks indeclaration.rs,properties_generated.rsandproperties_impl.rs.Before this PR leaves draft: merge main, write
PropertyCategory::Physicalon the new line if #38583 landed, and run the four rows again.Reach
.cssfiles that an install of 53 CSS packages from npm puts innode_modules. 772 of them contain a logical margin, padding, inset or border property. Other files cannot change. 762 of the 772 build.Bun.build({ minify: true }): 29 files change. 22 get smaller, 7 keep their size with declarations in another order, none grows. The total is 2,468 bytes less. The packages are Bootstrap, Carbon, antd, ant-design-vue, Vuetify, Mantine, daisyUI and DocSearch.target: "bun"(no browser targets): 28 files change, 18 get smaller, 10 keep their size, none grows. The total is 588 bytes less..form-control::-webkit-file-upload-buttonrule and a.form-control::file-selector-buttonrule with the same declarations. For the second one Bun also prints a-webkit-copy. On main the stalecategorygives the second rule another declaration order, so that copy does not merge with the first rule.The
borderarmProperty::Border(border.rs:1413) has the same missing assignment. An earlier version of this change setcategorythere too. That arm never flushes, so the assignment does more there than remove a false flush. Measured with the assignment:.a{border:1px solid blue;border-inline-start-color:red}.a{border:2px solid green}goes from.a{border:2px solid green}(26 bytes) to.a{border:1px solid #00f;border:2px solid green}(48)..btn{border:1px solid #ccc;border-inline-start:4px solid blue}.btn{border:0}goes from 14 to 36 bytes.#38623 reworks that arm and contains the assignment. Until then, a rule after a logical border property still depends on earlier rules, and the second test pass has no logical border declaration.
Tests
describeblock "shorthand after a logical property in an earlier rule" has 27 cases. With thesrc/of main, 23 fail. The other 4 are outputs that must not change.minify_test,prefix_testandcssTestintest/js/bun/css/util.tsnow run each source a second time behind a dropped@media not allrule that leaves the handlers in a non-default state. The output must be the same. Sources with@charset,@importor@namespaceare exempt, because those at-rules must come first.:dir()rules for targets that compile logical properties, and the second pass would then start from the same state as the first.src/of main, the second pass fails 5 existing cases (4 inlength, 1 inmargin). That gives the 28 failures. With the change: 1341 pass, 0 fail, 69 skip, 3 todo.css.test.tstakes about 7 s, up from about 5.3 s.test/js/bun/css/: 2543 pass, 5 timeouts on the local debug build, no assertion failure. Four are incss-fuzz.test.tsand one is innested-selector-expansion.test.ts. Both files time out in the same way with thesrc/of main.test/bundler/css/: 184 pass.test/bundler/esbuild/css.test.ts: 57 pass, 1 todo.test/regression/issue/25794.test.ts: pass.Upstream
div{margin-inline-start:2px}.a{margin:0}for the first input, and the same for padding, scroll-margin and inset. 1.21.0 and 1.33.0 keep both declarations for margin, padding and scroll-margin. The arm in lightningcss has nocategoryassignment either. A search of the Bun and lightningcss trackers found no report..a{margin:1px}.a{margin:0}div{margin-inline-start:2px}is specific to Bun. All three lightningcss versions print.a{margin:0}.InsetSpecdeclares its shorthand asPhysical(margin_padding.rs:1243). lightningcss declares it as logical.Size
cargo build --release -p bun_css --libon x86_64 Linux, then the sum of the.textsections oflibbun_css.rlib: 1,473,076 bytes on main, 1,473,131 bytes here.SizeHandlerImpl::handle_propertygrows from 3,155 to 3,210 bytes. This is the cargo release profile without the link-time optimization of a full release build.Update for current main
Main replaced the generic
SizeHandler<S>body withSizeHandlerImpl, which reads aSizeSpecvalue. The first version of this change readS::SHORTHAND_CATEGORY, which no longer exists. The branch now includes main, and the line readsspec.shorthand_category. Theflush_helper_physicalcalls in the same arm read the same field.Related open pull requests
borderone.