From e397ea5649096a5d661cbd9ad7d595dff637be43 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 23 Oct 2026 18:07:01 +0000 Subject: [PATCH 1/3] css: keep repeated font families and dedupe only the inserted system-ui fallbacks `FontHandler::flush` removed every family name that a list already had. That turned `font-family: monospace, monospace` (normalize.css) into the lone `monospace`, which browsers render at a smaller default size. The pass existed to remove the copies that `compatible_font_family` creates when it inserts the fallback names after `system-ui`. That function now lists each of its eight names once and changes no other entry. Every `system-ui` expansion prints as before. Delete the dedupe pass with `FontFamilyHashMap` and the `Eq` and `Hash` impls of `FontFamily`, which had no other user. --- src/css/properties/font.rs | 99 ++++++++++++++----------------------- test/js/bun/css/css.test.ts | 73 +++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 61 deletions(-) diff --git a/src/css/properties/font.rs b/src/css/properties/font.rs index 1f97fadfa022..13b2d04d4bed 100644 --- a/src/css/properties/font.rs +++ b/src/css/properties/font.rs @@ -322,14 +322,12 @@ pub enum FontFamily { /// A custom family name. // Arena-backed slice: the pointer targets bytes owned by the parser arena and is // only valid while that arena is alive (becomes `&'bump [u8]` once the 'bump - // lifetime is threaded through these types). With `*const [u8]`, derived - // PartialEq/Eq/Hash would compare by pointer; the manual impls below - // hash/compare by content (Wyhash over bytes). + // lifetime is threaded through these types). With `*const [u8]`, a derived + // PartialEq would compare by pointer; the manual impl below compares by + // content. FamilyName(*const [u8]), } -type FontFamilyHashMap = bun_collections::ArrayHashMap; - impl FontFamily { pub(crate) fn parse(input: &mut css::Parser) -> CssResult { if let Ok(value) = input.try_parse(|p| p.expect_string().map(std::ptr::from_ref::<[u8]>)) { @@ -417,12 +415,12 @@ impl FontFamily { } } - // eql / hash / deepClone — `PartialEq`/`Clone` hand-impls below; bridged to + // eql / deepClone — `PartialEq`/`Clone` hand-impls below; bridged to // `CssEql`/`DeepClone` via `bridge_clone_partialeq!` in `generics.rs`. } -// With `*const [u8]`, derived `PartialEq`/`Hash` would compare pointers; -// hand-roll to compare/hash the bytes by *content*. +// With `*const [u8]`, a derived `PartialEq` would compare pointers; +// hand-roll to compare the bytes by *content*. impl PartialEq for FontFamily { fn eq(&self, other: &Self) -> bool { match (self, other) { @@ -435,23 +433,6 @@ impl PartialEq for FontFamily { } } } -impl Eq for FontFamily {} - -impl core::hash::Hash for FontFamily { - fn hash(&self, state: &mut H) { - // Hash the active tag then the payload bytes. With `*const [u8]` a - // derived Hash would hash the pointer address, breaking - // FontFamilyHashMap dedupe semantics. - core::mem::discriminant(self).hash(state); - match self { - FontFamily::Generic(g) => g.hash(state), - FontFamily::FamilyName(p) => { - // SAFETY: arena-owned slice valid for the parse session. - unsafe { (&**p).hash(state) } - } - } - } -} impl Clone for FontFamily { fn clone(&self) -> Self { @@ -996,27 +977,6 @@ impl FontHandler { let line_height: Option = self.line_height.take(); let variant_caps: Option = self.variant_caps.take(); - if let Some(f) = family.as_mut() { - if f.len() > 1 { - // Dedupe - let mut seen: FontFamilyHashMap<()> = Default::default(); - - let mut i: usize = 0; - while i < f.len() { - use bun_collections::array_hash_map::MapEntry; - match seen.entry(f.at(i).clone()) { - MapEntry::Occupied(_) => { - let _ = f.ordered_remove(i); - } - MapEntry::Vacant(v) => { - v.insert(()); - i += 1; - } - } - } - } - } - if let (Some(_), Some(_), Some(_), Some(_), Some(_), Some(_), Some(variant_caps_v)) = ( family.as_ref(), size.as_ref(), @@ -1100,33 +1060,50 @@ const DEFAULT_SYSTEM_FONTS: &[&[u8]] = &[ b"Helvetica Neue", ]; +fn system_font(name: &'static [u8]) -> FontFamily { + FontFamily::FamilyName(std::ptr::from_ref::<[u8]>(name)) +} + #[inline] fn compatible_font_family( - _family: Option>, + family: Option>, is_supported: bool, ) -> Option> { - let mut family = _family; if is_supported { return family; } + let families = family?; + let Some(system_ui) = families.slice_const().iter().position(is_system_ui) else { + return Some(families); + }; + + // Unlike lightningcss, only the inserted names are deduped. Dropping any other repeated + // family can change rendering: https://github.com/parcel-bundler/lightningcss/issues/1221 + let mut listed = [false; DEFAULT_SYSTEM_FONTS.len()]; + let mut expanded = Vec::with_capacity(families.len() + DEFAULT_SYSTEM_FONTS.len()); + for (i, entry) in families.into_iter().enumerate() { + let fallback = DEFAULT_SYSTEM_FONTS + .iter() + .position(|&name| entry == system_font(name)); + if let Some(fallback) = fallback { + if listed[fallback] { + continue; + } + listed[fallback] = true; + } + expanded.push(entry); - if let Some(families) = family.as_mut() { - // Iterating the slice while inserting into `families` mid-loop would - // alias the &mut needed for `insert` (and `insert` may reallocate, - // invalidating the iterator). - // Capture the system-ui index first, drop the borrow, then - // perform the inserts using the captured index. - if let Some(i) = families.slice_const().iter().position(is_system_ui) { - for (j, name) in DEFAULT_SYSTEM_FONTS.iter().enumerate() { - families.insert( - i + j + 1, - FontFamily::FamilyName(std::ptr::from_ref::<[u8]>(*name)), - ); + if i == system_ui { + for (&name, is_listed) in DEFAULT_SYSTEM_FONTS.iter().zip(&mut listed) { + if !*is_listed { + *is_listed = true; + expanded.push(system_font(name)); + } } } } - family + Some(expanded) } #[inline] diff --git a/test/js/bun/css/css.test.ts b/test/js/bun/css/css.test.ts index 890ffa4dcd95..e88febf70a4a 100644 --- a/test/js/bun/css/css.test.ts +++ b/test/js/bun/css/css.test.ts @@ -5076,6 +5076,79 @@ describe("css tests", () => { }, ); + // Each name added after `system-ui` is listed once. Every other family stays as written. + for (const [source, expected] of [ + [ + "Roboto, system-ui, Roboto", + "Roboto, system-ui, -apple-system, BlinkMacSystemFont, Segoe UI, Noto Sans, Ubuntu, Cantarell, Helvetica Neue", + ], + [ + "system-ui, system-ui", + "system-ui, -apple-system, BlinkMacSystemFont, Segoe UI, Roboto, Noto Sans, Ubuntu, Cantarell, Helvetica Neue, system-ui", + ], + [ + "Foo, Foo, system-ui, monospace, monospace", + "Foo, Foo, system-ui, -apple-system, BlinkMacSystemFont, Segoe UI, Roboto, Noto Sans, Ubuntu, Cantarell, Helvetica Neue, monospace, monospace", + ], + [ + "system-ui, -apple-system, BlinkMacSystemFont, Segoe UI, Roboto, Noto Sans, Ubuntu, Cantarell, Helvetica Neue, Arial", + "system-ui, -apple-system, BlinkMacSystemFont, Segoe UI, Roboto, Noto Sans, Ubuntu, Cantarell, Helvetica Neue, Arial", + ], + ]) { + prefix_test(`.foo { font-family: ${source} }`, `.foo { font-family: ${expected}; }`, { firefox: 91 << 16 }); + } + + // The second rule merges into the first, and the merged declarations go through the font handler again. + prefix_test( + ".foo { font-family: system-ui, Arial } .foo { color: red }", + ".foo { color: red; font-family: system-ui, -apple-system, BlinkMacSystemFont, Segoe UI, Roboto, Noto Sans, Ubuntu, Cantarell, Helvetica Neue, Arial; }", + { firefox: 91 << 16 }, + ); + + // Browsers give the lone generic `monospace` a smaller default size than `monospace, monospace`. + minify_test( + "pre{font-family:monospace,monospace;font-size:1em}", + "pre{font-family:monospace,monospace;font-size:1em}", + ); + minify_test("code{font:1em monospace,monospace}", "code{font:1em monospace,monospace}"); + minify_test( + ".foo{font:10px serif;font-family:monospace,monospace;font-size:12px;font-style:italic;font-weight:bold;font-stretch:normal;font-variant-caps:small-caps;line-height:1.2em}", + ".foo{font:italic small-caps 700 12px/1.2em monospace,monospace}", + ); + minify_test("pre{font-family:monospace,monospace!important}", "pre{font-family:monospace,monospace!important}"); + minify_test( + "@media print{pre{font-family:monospace,monospace}}", + "@media print{pre{font-family:monospace,monospace}}", + ); + minify_test("kbd{font-family:monospace,monospace,monospace}", "kbd{font-family:monospace,monospace,monospace}"); + minify_test("kbd{font-family:monospace,serif,monospace}", "kbd{font-family:monospace,serif,monospace}"); + minify_test(".foo{font-family:Foo,Foo,serif}", ".foo{font-family:Foo,Foo,serif}"); + minify_test( + "a{font-family:monospace,monospace}b{font-family:monospace}", + "a{font-family:monospace,monospace}b{font-family:monospace}", + ); + minify_test("a{font-family:monospace,monospace}a{color:red}", "a{color:red;font-family:monospace,monospace}"); + + test("Bun.build keeps a repeated font family", async () => { + const build = async (minify: boolean) => { + const { outputs } = await Bun.build({ + entrypoints: ["/normalize.css"], + files: { + "/normalize.css": "pre{font-family:monospace,monospace;font-size:1em}code{font:1em monospace,monospace}", + }, + minify, + }); + // The unminified output starts with a comment that names the entry point relative to the cwd. + return (await outputs[0].text()).replace(/^\/\*.*\*\/\n/, ""); + }; + + expect({ minified: await build(true), unminified: await build(false) }).toEqual({ + minified: "pre{font-family:monospace,monospace;font-size:1em}code{font:1em monospace,monospace}\n", + unminified: + "pre {\n font-family: monospace, monospace;\n font-size: 1em;\n}\n\ncode {\n font: 1em monospace, monospace;\n}\n", + }); + }); + prefix_test( ` .foo { From c1d8144b27e52faa18ce99b858f401bbecd2677b Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 24 Oct 2026 01:45:05 +0000 Subject: [PATCH 2/3] css: drop only a later copy of a system-ui fallback that was inserted A name that the author lists before `system-ui` is not inserted. The expansion still removed the other copies of such a name, so `Roboto, Roboto, system-ui` lost one `Roboto` although no inserted name was involved. The expansion now removes an entry only when it repeats a name that was inserted ahead of it. The inserted names keep their place and their order, so every list without such a repeat prints as before. --- src/css/properties/font.rs | 16 ++++++++-------- test/js/bun/css/css.test.ts | 13 +++++++++++-- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/css/properties/font.rs b/src/css/properties/font.rs index 13b2d04d4bed..8432ebe21b27 100644 --- a/src/css/properties/font.rs +++ b/src/css/properties/font.rs @@ -1077,26 +1077,26 @@ fn compatible_font_family( return Some(families); }; - // Unlike lightningcss, only the inserted names are deduped. Dropping any other repeated - // family can change rendering: https://github.com/parcel-bundler/lightningcss/issues/1221 - let mut listed = [false; DEFAULT_SYSTEM_FONTS.len()]; + // Unlike lightningcss, only a later copy of an inserted name is dropped. Dropping any other + // repeat can change rendering: https://github.com/parcel-bundler/lightningcss/issues/1221 + let mut inserted = [true; DEFAULT_SYSTEM_FONTS.len()]; let mut expanded = Vec::with_capacity(families.len() + DEFAULT_SYSTEM_FONTS.len()); for (i, entry) in families.into_iter().enumerate() { let fallback = DEFAULT_SYSTEM_FONTS .iter() .position(|&name| entry == system_font(name)); if let Some(fallback) = fallback { - if listed[fallback] { + if i < system_ui { + inserted[fallback] = false; + } else if inserted[fallback] { continue; } - listed[fallback] = true; } expanded.push(entry); if i == system_ui { - for (&name, is_listed) in DEFAULT_SYSTEM_FONTS.iter().zip(&mut listed) { - if !*is_listed { - *is_listed = true; + for (fallback, &name) in DEFAULT_SYSTEM_FONTS.iter().enumerate() { + if inserted[fallback] { expanded.push(system_font(name)); } } diff --git a/test/js/bun/css/css.test.ts b/test/js/bun/css/css.test.ts index e88febf70a4a..621165c150db 100644 --- a/test/js/bun/css/css.test.ts +++ b/test/js/bun/css/css.test.ts @@ -5076,11 +5076,20 @@ describe("css tests", () => { }, ); - // Each name added after `system-ui` is listed once. Every other family stays as written. + // A later copy of a name added after `system-ui` is dropped. Every other family stays as written. for (const [source, expected] of [ + [ + "system-ui, Arial, Roboto", + "system-ui, -apple-system, BlinkMacSystemFont, Segoe UI, Roboto, Noto Sans, Ubuntu, Cantarell, Helvetica Neue, Arial", + ], + // `Roboto` comes before `system-ui`, so it is not added, and no copy of it is dropped. [ "Roboto, system-ui, Roboto", - "Roboto, system-ui, -apple-system, BlinkMacSystemFont, Segoe UI, Noto Sans, Ubuntu, Cantarell, Helvetica Neue", + "Roboto, system-ui, -apple-system, BlinkMacSystemFont, Segoe UI, Noto Sans, Ubuntu, Cantarell, Helvetica Neue, Roboto", + ], + [ + "Roboto, Roboto, system-ui", + "Roboto, Roboto, system-ui, -apple-system, BlinkMacSystemFont, Segoe UI, Noto Sans, Ubuntu, Cantarell, Helvetica Neue", ], [ "system-ui, system-ui", From 60af4ac9c03ea949000fec65531e2b7cfa808826 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 24 Oct 2026 04:11:32 +0000 Subject: [PATCH 3/3] css: keep the font family comments to one line each The comments on `FontFamily` and its `PartialEq` impl no longer mention the deleted `Hash` impl. --- src/css/properties/font.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/css/properties/font.rs b/src/css/properties/font.rs index 8432ebe21b27..5a274ba970cd 100644 --- a/src/css/properties/font.rs +++ b/src/css/properties/font.rs @@ -322,9 +322,8 @@ pub enum FontFamily { /// A custom family name. // Arena-backed slice: the pointer targets bytes owned by the parser arena and is // only valid while that arena is alive (becomes `&'bump [u8]` once the 'bump - // lifetime is threaded through these types). With `*const [u8]`, a derived - // PartialEq would compare by pointer; the manual impl below compares by - // content. + // lifetime is threaded through these types). With `*const [u8]`, derived + // PartialEq would compare by pointer, so the manual impl below compares by content. FamilyName(*const [u8]), } @@ -415,12 +414,11 @@ impl FontFamily { } } - // eql / deepClone — `PartialEq`/`Clone` hand-impls below; bridged to + // eql / deepClone: `PartialEq`/`Clone` hand-impls below, bridged to // `CssEql`/`DeepClone` via `bridge_clone_partialeq!` in `generics.rs`. } -// With `*const [u8]`, a derived `PartialEq` would compare pointers; -// hand-roll to compare the bytes by *content*. +// A derived `PartialEq` would compare the `*const [u8]` pointers, so this compares the bytes. impl PartialEq for FontFamily { fn eq(&self, other: &Self) -> bool { match (self, other) { @@ -1077,8 +1075,7 @@ fn compatible_font_family( return Some(families); }; - // Unlike lightningcss, only a later copy of an inserted name is dropped. Dropping any other - // repeat can change rendering: https://github.com/parcel-bundler/lightningcss/issues/1221 + // Unlike lightningcss, keep every other repeat: https://github.com/parcel-bundler/lightningcss/issues/1221 let mut inserted = [true; DEFAULT_SYSTEM_FONTS.len()]; let mut expanded = Vec::with_capacity(families.len() + DEFAULT_SYSTEM_FONTS.len()); for (i, entry) in families.into_iter().enumerate() {