diff --git a/src/css/properties/font.rs b/src/css/properties/font.rs index 1f97fadfa022..5a274ba970cd 100644 --- a/src/css/properties/font.rs +++ b/src/css/properties/font.rs @@ -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 = 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 +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) { @@ -435,23 +431,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 +975,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 +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>, + 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, 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] diff --git a/test/js/bun/css/css.test.ts b/test/js/bun/css/css.test.ts index 890ffa4dcd95..621165c150db 100644 --- a/test/js/bun/css/css.test.ts +++ b/test/js/bun/css/css.test.ts @@ -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 {