fix(opentype/STAT/ital_axis): skip static fonts

profile-designspace/Cargo.toml

@@ -9,7 +9,7 @@ crate-type = ["cdylib"]
 
 [dependencies]
 fontspector-checkapi = { path = "../fontspector-checkapi" }
-norad = "0.16.0"
+norad = "0.17.0"
 serde_json = { workspace = true }
 quick-xml = { version = "0.38.0", features = ["serialize"] }
 

profile-fontwerk/src/checks/fontwerk/mod.rs

@@ -2,7 +2,6 @@
 mod names;
 pub use names::name_consistency;
 pub use names::name_entries;
-pub use names::required_name_ids;
 mod fstype;
 pub use fstype::fstype;
 mod glyph_coverage;

profile-fontwerk/src/checks/fontwerk/names.rs

@@ -86,75 +86,6 @@ fn name_entries(f: &Testable, context: &Context) -> CheckFnResult {
     })
 }
 
-#[check(
-    id = "fontwerk/required_name_ids",
-    rationale = "
-        Required names for Fontwerk fonts:
-        - Copyright (0)
-        - Family Name (1)
-        - Subfamily Name (2)
-        - Unique ID (3)
-        - Full Name (4)
-        - Version String (5)
-        - PostScript Name (6)
-        - Trademark (7)
-        - Manufacturer (8)
-        - Designer(s) (9)
-        - Description (10)
-        - Vendor URL (11)
-        - Designer URL (12)
-        - License Description (13)
-        - License URL (14)
-        - Typographic Family Name (16)
-        - Typographic Subfamily Name (17)
-        - Variations PostScript Name Prefix (25) (if variable font)
-    ",
-    title = "Required name ids in name table"
-)]
-fn required_name_ids(t: &Testable, context: &Context) -> CheckFnResult {
-    let font = testfont!(t);
-    if !font.has_table(b"name") {
-        return Ok(Status::just_one_fail("lacks-table", "No name table."));
-    }
-    let mut bad_names: Vec<String> = vec![];
-
-    let name_PEL_codes = get_name_PEL_codes(font.font());
-    for code in name_PEL_codes {
-        let mut missing_name_ids: Vec<_> = vec![];
-        for id in [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 16, 17, 25] {
-            let name_id = StringId::from(id);
-            if let Some(_name_string) =
-                get_name_entry_string(&font.font(), code.0, code.1, code.2, name_id)
-            {
-                continue;
-            } else {
-                if id == 25 && !font.is_variable_font() {
-                    // Skip Variations PostScript Name Prefix if not a variable font
-                    continue;
-                }
-                missing_name_ids.push(id);
-            }
-        }
-        if !missing_name_ids.is_empty() {
-            bad_names.push(format!(
-                "Missing required name IDs {missing_name_ids:?} for {code:?}.",
-            ));
-        }
-    }
-
-    Ok(if bad_names.is_empty() {
-        Status::just_one_pass()
-    } else {
-        Status::just_one_fail(
-            "bad-name-table-entries",
-            &format!(
-                "The following issues have been found:\n\n{}",
-                bullet_list(context, bad_names)
-            ),
-        )
-    })
-}
-
 #[check(
     id = "fontwerk/name_consistency",
     rationale = "
@@ -519,21 +450,4 @@ mod tests {
             assert_eq!(result.severity, *expected_severity);
         }
     }
-
-    #[test]
-    fn test_required_name_ids() {
-        let contents = include_bytes!(
-            "../../../../fontspector-py/data/test/montserrat/Montserrat-Regular.ttf"
-        );
-        let testable = Testable::new_with_contents("demo.ttf", contents.to_vec());
-        let context = Context {
-            ..Default::default()
-        };
-        let result = required_name_ids_impl(&testable, &context)
-            .unwrap()
-            .next()
-            .unwrap();
-        let expected_message = "The following issues have been found:\n\n* Missing required name IDs [7, 10, 16, 17] for (1, 0, 0).\n* Missing required name IDs [7, 10, 16, 17] for (3, 1, 1033).";
-        assert_eq!(result.message, Some(expected_message.to_string()));
-    }
 }

profile-fontwerk/src/lib.rs

@@ -38,12 +38,18 @@ impl fontspector_checkapi::Plugin for Fontwerk {
             .add_section("Fontwerk Checks")
             .add_and_register_check(checks::fontwerk::name_entries)
             .add_and_register_check(checks::fontwerk::name_consistency)
-            .add_and_register_check(checks::fontwerk::required_name_ids)
             .add_and_register_check(checks::fontwerk::fstype)
             .add_and_register_check(checks::fontwerk::glyph_coverage)
             .add_and_register_check(checks::fontwerk::weightclass)
             // TODO: implement other Fontwerk checks
             // .add_and_register_check("fontwerk/names_match_default_fvar")
+            .include_profile("universal")
+            .with_configuration_defaults(
+                "universal/required_name_ids",
+                HashMap::from([
+                    ("required_name_ids".to_string(), json!([0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 16, 17, 25])),
+                ]),
+            )
             .with_configuration_defaults(
                 "opentype/vendor_id",
                 HashMap::from([

profile-googlefonts/src/checks/googlefonts/metadata/validate.rs

@@ -42,6 +42,8 @@ fn category_hints(family_name: &str) -> Option<&'static str> {
     }
     None
 }
+const VALID_CLASSIFICATIONS: &[&str] = &["Display", "Handwriting", "Monospace", "Symbols"];
+const VALID_STROKES: &[&str] = &["Serif", "Slab Serif", "Sans Serif"];
 
 fn clean_url(url: &str) -> String {
     let mut cleaned = url.trim().to_string();
@@ -243,6 +245,40 @@ fn validate(c: &Testable, _context: &Context) -> CheckFnResult {
         ));
     }
 
+    // The METADATA.pb file can only contain specific predefined values for the
+    // 'stroke' and 'classifications' fields:
+
+    // Valid stroke values: Serif, Slab Serif, Sans Serif
+    // Valid classifications values: Display, Handwriting, Monospace, Symbols
+
+    // Any other values are invalid and will cause issues with the Google Fonts API.
+
+    if let Some(stroke) = msg.stroke.as_ref() {
+        if !stroke.is_empty() && !VALID_STROKES.contains(&stroke.as_str()) {
+            problems.push(Status::fail(
+                "invalid-stroke",
+                &format!(
+                    "METADATA.pb stroke field contains invalid value '{}'. Valid values are: {}",
+                    stroke,
+                    VALID_STROKES.join(", ")
+                ),
+            ));
+        }
+    }
+
+    for classification in &msg.classifications {
+        if !VALID_CLASSIFICATIONS.contains(&classification.as_str()) {
+            problems.push(Status::fail(
+                "invalid-classification",
+                &format!(
+                    "METADATA.pb classifications field contains invalid value '{}'. Valid values are: {}",
+                    classification,
+                    VALID_CLASSIFICATIONS.join(", ")
+                ),
+            ));
+        }
+    }
+
     return_result(problems)
 }
 

profile-opentype/src/checks/opentype/STAT/ital_axis.rs

@@ -1,29 +1,39 @@
-use fontations::skrifa::raw::{
-    tables::stat::{AxisValue, AxisValueTableFlags},
-    ReadError, TableProvider,
+use fontations::skrifa::MetadataProvider;
+use fontations::{
+    skrifa::raw::{
+        tables::stat::{AxisValue, AxisValueTableFlags},
+        ReadError, TableProvider,
+    },
+    types::NameId,
 };
 use fontspector_checkapi::{prelude::*, FileTypeConvert, TestFont};
 
-fn segment_vf_collection(fonts: Vec<TestFont>) -> Vec<(Option<TestFont>, Option<TestFont>)> {
+fn segment_collection(fonts: Vec<TestFont>) -> Vec<(Option<TestFont>, Option<TestFont>)> {
     let mut roman_italic = vec![];
     let (italics, mut non_italics): (Vec<_>, Vec<_>) = fonts
         .into_iter()
-        .partition(|f| f.filename.to_str().unwrap_or_default().contains("-Italic["));
+        .partition(|f| f.filename.to_str().unwrap_or_default().contains("Italic"));
     for italic in italics.into_iter() {
         // Find a matching roman
-        let suspected_roman = italic
-            .filename
-            .to_str()
-            .unwrap_or_default()
-            .replace("-Italic[", "[");
-        if let Some(index) = non_italics
-            .iter()
-            .position(|f| f.filename.to_str().unwrap_or_default() == suspected_roman)
+        if let Some(name) = italic
+            .font()
+            .localized_strings(NameId::FAMILY_NAME)
+            .english_or_first()
         {
-            let roman = non_italics.swap_remove(index);
-            roman_italic.push((Some(roman), Some(italic)));
-        } else {
-            roman_italic.push((None, Some(italic)));
+            let suspected_roman_family_name = name.to_string();
+            if let Some(index) = non_italics.iter().position(|f| {
+                f.font()
+                    .localized_strings(NameId::FAMILY_NAME)
+                    .english_or_first()
+                    .expect("No Family Name")
+                    .to_string()
+                    == suspected_roman_family_name
+            }) {
+                let roman = non_italics.swap_remove(index);
+                roman_italic.push((Some(roman), Some(italic)));
+            } else {
+                roman_italic.push((None, Some(italic)));
+            }
         }
     }
     // Now add all the remaining non-italic fonts
@@ -190,7 +200,7 @@ fn check_ital_is_binary_and_last(t: &TestFont, is_italic: bool) -> Result<Vec<St
 #[check(
     id = "opentype/STAT/ital_axis",
     rationale = "
-        Check that related Upright and Italic VFs have an
+        Check that related Upright and Italic have an
         'ital' axis in the STAT table.
 
         Since the STAT table can be used to create new instances, it is
@@ -209,13 +219,29 @@ fn check_ital_is_binary_and_last(t: &TestFont, is_italic: bool) -> Result<Vec<St
     proposal = "https://github.com/fonttools/fontbakery/issues/3668",
     proposal = "https://github.com/fonttools/fontbakery/issues/3669",
     implementation = "all",
-    title = "Ensure VFs have 'ital' STAT axis."
+    title = "Ensure Fonts have 'ital' STAT axis."
 )]
 fn ital_axis(c: &TestableCollection, _context: &Context) -> CheckFnResult {
     let fonts = TTF.from_collection(c);
+    for font in fonts.iter() {
+        if font.has_table(b"gvar") && !font.has_table(b"STAT") {
+            // variable font must have a STAT table
+            return Ok(Status::just_one_fail(
+                "no-stat-table",
+                &format!("Variable font is missing the 'STAT' table."),
+            ));
+        } else if !font.has_table(b"gvar") && !font.has_table(b"STAT") {
+            // static font is recommend to have a STAT table
+            return Ok(Status::just_one_warn(
+                "no-stat-table",
+                &format!("Static font is missing the 'STAT' table."),
+            ));
+        }
+    }
     let mut problems = vec![];
 
-    for pair in segment_vf_collection(fonts).into_iter() {
+    for pair in segment_collection(fonts).into_iter() {
+        print!("Pair: {:?}\n", pair);
         match pair {
             (Some(roman), Some(italic)) => {
                 // These should definitely both have an ital axis
@@ -241,3 +267,97 @@ fn ital_axis(c: &TestableCollection, _context: &Context) -> CheckFnResult {
     }
     return_result(problems)
 }
+
+#[cfg(test)]
+mod tests {
+    #![allow(clippy::unwrap_used)]
+
+    use super::*;
+    use fontspector_checkapi::StatusCode;
+    use fontspector_checkapi::{Testable, TestableType};
+
+    use fontspector_checkapi::codetesting::{
+        assert_results_contain, run_check_with_config, test_able,
+    };
+    use std::collections::HashMap;
+
+    #[test]
+    fn test_segment_collection_var() {
+        let testable_1 = test_able("notosans/NotoSans-VariableFont_wdth,wght.ttf");
+        let testable_2 = test_able("notosans/NotoSans-Italic-VariableFont_wdth,wght.ttf");
+        let testables: Vec<Testable> = vec![testable_1, testable_2];
+        let collection = TestableCollection {
+            testables,
+            directory: "".to_string(),
+        };
+        let fonts = TTF.from_collection(&collection);
+        let pairs = segment_collection(fonts);
+        assert_eq!(pairs.len(), 1);
+        for (roman, italic) in pairs.into_iter() {
+            assert!(roman.is_some());
+            assert!(italic.is_some());
+        }
+    }
+
+    #[test]
+    fn test_segment_collection_static() {
+        let testable_1 = test_able("montserrat/Montserrat-Regular.ttf");
+        let testable_2 = test_able("montserrat/Montserrat-Italic.ttf");
+        let testable_3 = test_able("montserrat/Montserrat-Bold.ttf");
+        let testable_4 = test_able("montserrat/Montserrat-BoldItalic.ttf");
+        let testable_5 = test_able("montserrat/Montserrat-Light.ttf");
+        let testable_6 = test_able("montserrat/Montserrat-LightItalic.ttf");
+        let testables: Vec<Testable> = vec![
+            testable_1, testable_2, testable_3, testable_4, testable_5, testable_6,
+        ];
+        let collection = TestableCollection {
+            testables,
+            directory: "".to_string(),
+        };
+        let fonts = TTF.from_collection(&collection);
+        let pairs = segment_collection(fonts);
+        assert_eq!(pairs.len(), 3);
+        for (roman, italic) in pairs.into_iter() {
+            assert!(roman.is_some());
+            assert!(italic.is_some());
+        }
+    }
+
+    #[test]
+    fn test_ital_axis_static_fonts_missing_stat() {
+        let testable_1 = test_able("montserrat/Montserrat-Regular.ttf");
+        let testable_2 = test_able("montserrat/Montserrat-Italic.ttf");
+        let testables: Vec<Testable> = vec![testable_1, testable_2];
+        let collection = TestableCollection {
+            testables,
+            directory: "".to_string(),
+        };
+        let results = run_check_with_config(
+            ital_axis,
+            TestableType::Collection(&collection),
+            HashMap::new(),
+        );
+        assert_results_contain(
+            &results,
+            StatusCode::Warn,
+            Some("no-stat-table".to_string()),
+        );
+    }
+
+    #[test]
+    fn test_ital_axis_skip_static_fonts() {
+        let testable_1 = test_able("notosans/static/NotoSans-Black.ttf");
+        let testable_2 = test_able("notosans/static/NotoSans-BlackItalic.ttf");
+        let testables: Vec<Testable> = vec![testable_1, testable_2];
+        let collection = TestableCollection {
+            testables,
+            directory: "".to_string(),
+        };
+        let results = run_check_with_config(
+            ital_axis,
+            TestableType::Collection(&collection),
+            HashMap::new(),
+        );
+        assert_results_contain(&results, StatusCode::Pass, None);
+    }
+}

profile-universal/src/checks/mod.rs

@@ -44,6 +44,7 @@ mod missing_small_caps_glyphs;
 mod nested_components;
 mod no_mac_entries;
 mod os2_metrics_match_hhea;
+mod required_name_ids;
 mod required_tables;
 mod rupee;
 mod sfnt_version;
@@ -114,6 +115,7 @@ pub use missing_small_caps_glyphs::missing_small_caps_glyphs;
 pub use nested_components::nested_components;
 pub use no_mac_entries::no_mac_entries;
 pub use os2_metrics_match_hhea::os2_metrics_match_hhea;
+pub use required_name_ids::required_name_ids;
 pub use required_tables::required_tables;
 pub use rupee::rupee;
 pub use sfnt_version::sfnt_version;