Merge pull request #1755 from Hamza0101/fix/korean-sans-fallback

Fonts: a missing Korean sans face falls back to a sans one (#1692)

crates/compose/src/shape.rs

@@ -317,6 +317,20 @@ impl StyleTable<'_> {
     }
 }
 
+/// What a substitute face's own name says about its style, for choosing a CJK fallback chain.
+///
+/// `None` for the bundled default and last-resort families: they stand in for a font that could
+/// not be resolved at all, so their names say nothing about what the document asked for. Without
+/// this, "Source Sans 3" made every unresolved family look like a sans one (#1692, noted in
+/// review).
+fn style_of_substitute(family: &str) -> Option<bool> {
+    let standin = [designcraft_fonts::DEFAULT_FAMILY, designcraft_fonts::FALLBACK_FAMILY];
+    if standin.iter().any(|s| s.eq_ignore_ascii_case(family)) {
+        return None;
+    }
+    designcraft_fonts::sans_by_name(family)
+}
+
 /// Resolve and shape one paragraph. Its drop cap ends at byte `drop.0` (`range.start` without
 /// one) and takes the character style `drop.1` ("" = none).
 pub(crate) struct ShapeInput<'a, 'font> {
@@ -813,8 +827,19 @@ fn shape_run_raw(
     // The language picks the fallback for CJK characters the primary font lacks. Without
     // fallback, the primary font sets them: the shaper gives its .notdef (glyph 0).
     let lang = designcraft_doc::language_tag(&p.language);
+    // A missing Korean face is replaced by one of its own style: the requested name says whether
+    // it was Dotum/Gothic or Batang/Myungjo, and if it does not, the substitute's own name does,
+    // which keeps today's chain (#1692).
+    // The substitute's own name is consulted only when it is a real substitute: a requested
+    // family that resolves to nothing comes back as the bundled default, whose name holds "Sans",
+    // and every unclassified missing font would then have taken the sans chain (noted in review).
+    let prefer_sans = designcraft_fonts::sans_by_name(&p.font_family).or_else(|| style_of_substitute(&primary.family));
     let fallback = |c: char| {
-        if auto_leading.glyph_fallback { db.fallback_for(c, primary.id(), lang).unwrap_or_else(|| primary.clone()) } else { primary.clone() }
+        if auto_leading.glyph_fallback {
+            db.fallback_for_styled(c, primary.id(), lang, prefer_sans).unwrap_or_else(|| primary.clone())
+        } else {
+            primary.clone()
+        }
     };
     let strong = |c| match unicode_bidi::bidi_class(c) {
         unicode_bidi::BidiClass::AL => Some(true),
@@ -1144,8 +1169,11 @@ fn shape_segment(
     if let Some(label) = replacement.filter(|_| auto_leading.glyph_fallback) {
         let lang = designcraft_doc::language_tag(&p.language);
         let mut segments: Vec<(usize, usize, Arc<FontFace>)> = Vec::new();
+        // The same for every character of the label: worked out once, not per character.
+        let prefer_sans = designcraft_fonts::sans_by_name(&p.font_family).or_else(|| style_of_substitute(&face.family));
         for (at, c) in label.char_indices() {
-            let selected = if face.covers(c) { face.clone() } else { db.fallback_for(c, face.id(), lang).unwrap_or_else(|| face.clone()) };
+            let selected =
+                if face.covers(c) { face.clone() } else { db.fallback_for_styled(c, face.id(), lang, prefer_sans).unwrap_or_else(|| face.clone()) };
             if let Some((_, end, previous)) = segments.last_mut().filter(|(_, _, previous)| previous.id() == selected.id()) {
                 *end = at + c.len_utf8();
                 let _ = previous;
@@ -1495,3 +1523,23 @@ mod tests {
         assert!(t.elapsed().as_secs() < 5);
     }
 }
+
+#[cfg(test)]
+mod style_of_substitute_tests {
+    use super::style_of_substitute;
+
+    /// A face that stands in for a font that could not be resolved says nothing about the style
+    /// the document asked for. `FALLBACK_FAMILY` is "Source Sans 3" and `DEFAULT_FAMILY` is
+    /// "Source Serif 4", so reading their names would have forced the sans chain on every
+    /// unresolved family -- and the serif one on the other (#1692, noted in review).
+    #[test]
+    fn a_stand_in_face_says_nothing_about_the_requested_style() {
+        assert_eq!(style_of_substitute(designcraft_fonts::FALLBACK_FAMILY), None);
+        assert_eq!(style_of_substitute(designcraft_fonts::DEFAULT_FAMILY), None);
+        assert_eq!(style_of_substitute("source sans 3"), None, "however it is spelled");
+        // A real substitute still speaks for itself.
+        assert_eq!(style_of_substitute("Malgun Gothic"), Some(true));
+        assert_eq!(style_of_substitute("Batang"), Some(false));
+        assert_eq!(style_of_substitute("Helvetica"), None, "a name that says neither");
+    }
+}

crates/fonts/src/fontdb.rs

@@ -138,6 +138,7 @@ pub(crate) use face::{measured_top, plausible};
 use fallback::FallbackSnapshotHook;
 #[cfg(not(target_arch = "wasm32"))]
 use fallback::SysFallback;
+pub use fallback::sans_by_name;
 pub use names::{base_style, same_style};
 use outline::FlipPen;
 pub use scoped::ScopedFonts;

crates/fonts/src/fontdb/db.rs

@@ -199,6 +199,11 @@ impl FontDb {
         self.scoped(0).fallback_for(c, exclude, language)
     }
 
+    /// [`ScopedFonts::fallback_for_styled`] on the shared scope.
+    pub fn fallback_for_styled(&self, c: char, exclude: u32, language: Option<&str>, prefer_sans: Option<bool>) -> Option<Arc<FontFace>> {
+        self.scoped(0).fallback_for_styled(c, exclude, language, prefer_sans)
+    }
+
     /// The box drawn for `face`'s missing glyph (.notdef) when the font's own has no outline, in
     /// font units, y-down: a hollow rectangle across the glyph's advance, cap height tall. `None`
     /// when the font draws its own.

crates/fonts/src/fontdb/fallback.rs

@@ -92,7 +92,62 @@ const TRADITIONAL_CHINESE_FALLBACKS: &[&str] = &[
     "Source Han Sans TC",
 ];
 
-const KOREAN_FALLBACKS: &[&str] = &[
+/// Korean fallbacks for a missing **sans** (Dotum/Gothic) face, system faces first.
+///
+/// [`KOREAN_FALLBACKS`] is serif-first, so on Windows the first installed entry is Batang and a
+/// document whose Korean text asked for KoPub Dotum or BM Dohyeon came out in a serif face --
+/// 5,298 characters of one school document, turning it from sans to serif (#1692).
+pub(super) const KOREAN_SANS_FALLBACKS: &[&str] = &[
+    "Malgun Gothic",
+    "Apple SD Gothic Neo",
+    "Dotum",
+    "Gulim",
+    "Noto Sans CJK KR",
+    "Source Han Sans K",
+    "Noto Sans KR",
+    "Noto Serif CJK KR",
+    "Batang",
+];
+
+/// Whether a Korean family name says the face is sans (`Some(true)`) or serif (`Some(false)`).
+/// `None` when the name does not say, so the caller decides.
+///
+/// Korean family names carry the style: Dotum (돋움), Gulim (굴림) and Gothic (고딕) are sans;
+/// Batang (바탕), Myungjo/Myeongjo (명조) and Gungsuh (궁서) are serif. A missing font's name is
+/// the only thing left to go on, since the font itself is not there to classify (#1692).
+pub fn sans_by_name(family: &str) -> Option<bool> {
+    let lower = family.to_lowercase();
+    let has = |needles: &[&str]| needles.iter().any(|n| lower.contains(n));
+    // A Korean serif name comes first: "Myungjo Gothic" does not exist, but a name holding both
+    // reads as the serif it names, and "Sans" inside "Noto Sans Myungjo" would otherwise win.
+    if has(&["batang", "myungjo", "myeongjo", "gungsuh", "\u{BC14}\u{D0D5}", "\u{BA85}\u{C870}", "\u{AD81}\u{C11C}"]) {
+        return Some(false);
+    }
+    // Then the word itself, which outranks every sans indicator: "Sweet Gothic Serif" is a serif
+    // family whatever "Gothic" suggests. "Sans serif" and "sans-serif" end with the word and do
+    // not mean it, so the phrase goes before the word is looked for.
+    if lower.replace("sans-serif", "sans").replace("sans serif", "sans").contains("serif") {
+        return Some(false);
+    }
+    if has(&[
+        "dotum",
+        "gothic",
+        "gulim",
+        "sans",
+        "headline",
+        "graphic",
+        "dohyeon",
+        "\u{B3CB}\u{C6C0}",
+        "\u{ACE0}\u{B515}",
+        "\u{AD74}\u{B9BC}",
+        "\u{ADF8}\u{B798}\u{D53D}",
+    ]) {
+        return Some(true);
+    }
+    None
+}
+
+pub(super) const KOREAN_FALLBACKS: &[&str] = &[
     "AppleMyungjo",
     "Apple SD Gothic Neo",
     "Batang",
@@ -146,9 +201,10 @@ fn japanese_chain() -> &'static [&'static str] {
 /// The CJK fallback chain for `c` in `language` (a BCP 47 tag). The script decides where it is
 /// unambiguous (Hangul is Korean, kana Japanese, Bopomofo Traditional Chinese); ideographs and CJK
 /// punctuation follow the language, and have no chain without a CJK one.
-pub(super) fn cjk_chain(c: char, language: Option<&str>) -> Option<&'static [&'static str]> {
+pub(super) fn cjk_chain(c: char, language: Option<&str>, prefer_sans: Option<bool>) -> Option<&'static [&'static str]> {
+    let korean = if prefer_sans == Some(true) { KOREAN_SANS_FALLBACKS } else { KOREAN_FALLBACKS };
     match c as u32 {
-        0x1100..=0x11FF | 0x3130..=0x318F | 0xA960..=0xA97F | 0xAC00..=0xD7FF | 0xFFA0..=0xFFDC => return Some(KOREAN_FALLBACKS),
+        0x1100..=0x11FF | 0x3130..=0x318F | 0xA960..=0xA97F | 0xAC00..=0xD7FF | 0xFFA0..=0xFFDC => return Some(korean),
         0x3040..=0x30FF | 0x31F0..=0x31FF | 0xFF66..=0xFF9F | 0x1B000..=0x1B16F => return Some(japanese_chain()),
         0x3100..=0x312F | 0x31A0..=0x31BF => return Some(TRADITIONAL_CHINESE_FALLBACKS),
         // Radicals, CJK symbols and punctuation, kanbun, strokes, enclosed and compatibility
@@ -171,7 +227,7 @@ pub(super) fn cjk_chain(c: char, language: Option<&str>) -> Option<&'static [&'s
     let language = language?;
     match language.split('-').next()? {
         "ja" => Some(japanese_chain()),
-        "ko" => Some(KOREAN_FALLBACKS),
+        "ko" => Some(korean),
         "zh" if language.starts_with("zh-Hant") => Some(TRADITIONAL_CHINESE_FALLBACKS),
         "zh" => Some(SIMPLIFIED_CHINESE_FALLBACKS),
         _ => None,

crates/fonts/src/fontdb/scoped.rs

@@ -402,7 +402,14 @@ impl ScopedFonts<'_> {
     /// then load order, the document's fonts last) that covers `c`. On native, system fonts are
     /// cataloged and loaded lazily the first time no loaded face covers a character.
     pub fn fallback_for(&self, c: char, exclude: u32, language: Option<&str>) -> Option<Arc<FontFace>> {
-        if let Some(chain) = cjk_chain(c, language).or_else(|| script_chain(c))
+        self.fallback_for_styled(c, exclude, language, None)
+    }
+
+    /// [`Self::fallback_for`] following the style of the font that was asked for: `Some(true)`
+    /// takes the sans chain, so a missing Dotum or Gothic face is replaced by a sans one instead
+    /// of by the first installed serif (#1692). `None` keeps the chain as it was.
+    pub fn fallback_for_styled(&self, c: char, exclude: u32, language: Option<&str>, prefer_sans: Option<bool>) -> Option<Arc<FontFace>> {
+        if let Some(chain) = cjk_chain(c, language, prefer_sans).or_else(|| script_chain(c))
             && let Some(f) = chain.iter().find_map(|family| self.chain_face(family, c, exclude))
         {
             return Some(f);

crates/fonts/src/fontdb/tests/fallback.rs

@@ -1,6 +1,7 @@
 //! CJK fallback follows the character's language (and the script where it is unambiguous).
 
 use crate::fontdb::FontDb;
+use crate::fontdb::fallback::{KOREAN_FALLBACKS, KOREAN_SANS_FALLBACKS, cjk_chain, sans_by_name};
 use crate::testing::font_with;
 
 const HAN: char = '直';
@@ -157,3 +158,70 @@ fn malayalam_fonts_load_from_the_font_folders_when_first_needed() {
     assert!(!db.is_loaded("Noto Sans Malayalam"));
     assert_eq!(family(&db, KA, None).as_deref(), Some("Noto Sans Malayalam"));
 }
+
+/// A missing Korean face is replaced by one of its own style. The name is all there is to go
+/// on, since the font itself is not installed to classify (#1692).
+#[test]
+fn a_korean_family_name_says_sans_or_serif() {
+    for sans in ["KoPub Dotum Bold", "KHNP Han Dotum", "BM Dohyeon", "Malgun Gothic", "\u{B3CB}\u{C6C0}\u{CCB4}", "\u{ACE0}\u{B515}", "Noto Sans KR"]
+    {
+        assert_eq!(sans_by_name(sans), Some(true), "{sans} is sans");
+    }
+    for serif in ["Batang", "HCR Batang", "KoPub Myungjo", "\u{BA85}\u{C870}", "Gungsuh", "Noto Serif KR"] {
+        assert_eq!(sans_by_name(serif), Some(false), "{serif} is serif");
+    }
+    // A name that says neither leaves the choice to the caller.
+    for unknown in ["Times New Roman", "Helvetica", "Arial Unicode MS", ""] {
+        assert_eq!(sans_by_name(unknown), None, "{unknown} says neither");
+    }
+}
+
+/// "sans serif" ends with the word it is not. The bare "serif" needle matched first, so
+/// "Microsoft Sans Serif" -- and the CSS generic -- classified as serif and a missing Hangul
+/// glyph took the serif-first chain (#1692).
+#[test]
+fn a_sans_serif_name_is_not_read_as_serif() {
+    for sans in ["sans-serif", "Microsoft Sans Serif", "MS Sans Serif", "Lucida Sans Serif"] {
+        assert_eq!(sans_by_name(sans), Some(true), "{sans} is sans");
+    }
+    // The bare word still decides a name that holds nothing else.
+    for serif in ["serif", "Source Serif 4", "DejaVu Serif", "Noto Serif KR"] {
+        assert_eq!(sans_by_name(serif), Some(false), "{serif} is serif");
+    }
+    // And a Korean serif name still outranks "Sans", which is why it is tested first of all.
+    assert_eq!(sans_by_name("Noto Sans Myungjo"), Some(false));
+    assert_eq!(sans_by_name("Myungjo Gothic"), Some(false));
+    // And the word outranks every sans indicator: Sweet Gothic Serif is a serif family
+    // whatever "Gothic" suggests.
+    for serif in ["Sweet Gothic Serif", "Gothic Serif", "Sans Forgetica Serif"] {
+        assert_eq!(sans_by_name(serif), Some(false), "{serif} names itself serif");
+    }
+    // Korean serif names written in Hangul: 바탕 (Batang), 명조 (Myungjo), 궁서 (Gungsuh). 바탕 was
+    // spelled with the wrong second syllable, so a Hangul Batang name was not recognised.
+    for serif in ["\u{BC14}\u{D0D5}\u{CCB4}", "HCR \u{BC14}\u{D0D5}", "\u{D55C}\u{CEF4} \u{BA85}\u{C870}", "\u{AD81}\u{C11C}\u{CCB4}"] {
+        assert_eq!(sans_by_name(serif), Some(false), "{serif} is serif");
+    }
+    // Neither word present, so the caller still decides.
+    assert_eq!(sans_by_name("monospace"), None);
+    assert_eq!(sans_by_name("system-ui"), None);
+}
+
+/// The reported bug is the order of the chain: on Windows the first installed entry of the
+/// serif-first chain is Batang, so every missing Dotum face came out serif (#1692).
+#[test]
+fn the_sans_chain_puts_a_sans_face_before_batang() {
+    let hangul = '\u{D55C}';
+    let sans = cjk_chain(hangul, None, Some(true)).expect("Hangul has a chain");
+    let serif = cjk_chain(hangul, None, None).expect("Hangul has a chain");
+    assert_eq!(serif, KOREAN_FALLBACKS, "without a style the chain is unchanged");
+    assert_eq!(sans, KOREAN_SANS_FALLBACKS);
+
+    let batang = |chain: &[&str]| chain.iter().position(|f| *f == "Batang");
+    let first_sans = |chain: &[&str]| chain.iter().position(|f| sans_by_name(f) == Some(true));
+    assert!(first_sans(sans) < batang(sans), "the sans chain reaches a sans face first: {sans:?}");
+    // And the Windows and macOS system faces are the first two entries, so an installed
+    // system font is preferred to a downloaded one.
+    assert_eq!(&sans[..2], &["Malgun Gothic", "Apple SD Gothic Neo"]);
+    // The serif chain still leads with serif faces, so a missing Myungjo is not made sans.
+    assert_eq!(sans_by_name(serif[0]), Some(false), "the serif chain still leads serif: {:?}", serif[0]);
+}

crates/fonts/src/lib.rs

@@ -20,7 +20,7 @@ mod vertical;
 pub use features::{FontFeatures, font_features};
 pub use fontdb::{
     DOCUMENT_FONTS_FOLDER, DocumentFonts, FALLBACK_FAMILY, FaceRef, FontDb, FontFace, FontSource, MAX_DOCUMENT_FONT_BYTES, MAX_DOCUMENT_FONT_FILES,
-    MAX_DOCUMENT_FONTS_TOTAL, ReloadedFonts, ScopedFonts, base_style, bundled, same_style, system_font_dirs,
+    MAX_DOCUMENT_FONTS_TOTAL, ReloadedFonts, ScopedFonts, base_style, bundled, same_style, sans_by_name, system_font_dirs,
 };
 pub use group::{FamilyInfo, FontGroup, sort_for_menu};
 pub use harfrust::Feature;