Skip to content

css: keep repeated font families and dedupe only the inserted system-ui fallbacks #46662

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 3 commits into from
Closed

css: keep repeated font families and dedupe only the inserted system-ui fallbacks #46662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e397ea5
css: keep repeated font families and dedupe only the inserted system-…
robobun Oct 23, 2026
c1d8144
css: drop only a later copy of a system-ui fallback that was inserted
robobun Oct 24, 2026
60af4ac
css: keep the font family comments to one line each
robobun Oct 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 34 additions & 60 deletions src/css/properties/font.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,13 +323,10 @@ pub enum FontFamily {
// 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).
// PartialEq would compare by pointer, so the manual impl below compares by content.
FamilyName(*const [u8]),
}

type FontFamilyHashMap<V> = bun_collections::ArrayHashMap<FontFamily, V>;

impl FontFamily {
pub(crate) fn parse(input: &mut css::Parser) -> CssResult<Self> {
if let Ok(value) = input.try_parse(|p| p.expect_string().map(std::ptr::from_ref::<[u8]>)) {
Expand Down Expand Up @@ -417,12 +414,11 @@ 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*.
// 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) {
Expand All @@ -435,23 +431,6 @@ impl PartialEq for FontFamily {
}
}
}
impl Eq for FontFamily {}

impl core::hash::Hash for FontFamily {
fn hash<H: core::hash::Hasher>(&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 {
Expand Down Expand Up @@ -996,27 +975,6 @@ impl FontHandler {
let line_height: Option<LineHeight> = self.line_height.take();
let variant_caps: Option<FontVariantCaps> = 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(),
Expand Down Expand Up @@ -1100,33 +1058,49 @@ 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<Vec<FontFamily>>,
family: Option<Vec<FontFamily>>,
is_supported: bool,
) -> Option<Vec<FontFamily>> {
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, 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() {
let fallback = DEFAULT_SYSTEM_FONTS
.iter()
.position(|&name| entry == system_font(name));
if let Some(fallback) = fallback {
if i < system_ui {
inserted[fallback] = false;
} else if inserted[fallback] {
continue;
}
}
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 (fallback, &name) in DEFAULT_SYSTEM_FONTS.iter().enumerate() {
if inserted[fallback] {
expanded.push(system_font(name));
}
}
}
}

family
Some(expanded)
}

#[inline]
Expand Down
82 changes: 82 additions & 0 deletions test/js/bun/css/css.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5076,6 +5076,88 @@ describe("css tests", () => {
},
);

// 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",
],
[
"Roboto, 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 {
Expand Down
Loading