Bug 2067259 - Rework the criteria used to determine whether font-family names are quoted. r=firefox-style-system-reviewers,emilio

This reflects the decision in https://github.com/w3c/csswg-drafts/issues/5846,
and expectations of some recent WPTs (see later patch updating metadata).

Differential Revision: https://phabricator.services.mozilla.com/D325334

devtools/server/tests/browser/browser_style_utils_getFontPreviewData.js

@@ -26,7 +26,7 @@ add_task(async function () {
     );
     is(
       fontPreviewData.ctx.font,
-      `40px ${Services.appinfo.OS === "WINNT" ? "Arial" : `"Liberation Sans"`}, serif`,
+      `40px ${Services.appinfo.OS === "WINNT" ? "Arial" : "Liberation Sans"}, serif`,
       "Expected font style was used in the canvas"
     );
 
@@ -149,7 +149,7 @@ add_task(async function () {
     // Check font wrapped in double quotes
     is(
       getFontPreviewData(`"Zilla Bold"`, content.document).ctx.font,
-      `40px "Zilla Bold", serif`,
+      `40px Zilla Bold, serif`,
       "Expected font style was used in the canvas"
     );
 
@@ -173,7 +173,7 @@ add_task(async function () {
         `Menlo Bold, "Fira Code", 'Mono Lisa', monospace`,
         content.document
       ).ctx.font,
-      `40px "Menlo Bold", "Fira Code", "Mono Lisa", monospace, serif`,
+      "40px Menlo Bold, Fira Code, Mono Lisa, monospace, serif",
       "Expected font style was used in the canvas"
     );
 

servo/components/style/values/computed/font.rs

@@ -7,8 +7,8 @@
 use crate::Atom;
 use crate::derives::*;
 use crate::parser::{Parse, ParserContext};
+use crate::properties::CSSWideKeyword;
 use crate::typed_om::{ToTyped, TypedValue};
-use crate::values::CSSInteger;
 use crate::values::animated::ToAnimatedValue;
 use crate::values::computed::{
     Angle, Context, Integer, Length, NonNegativeLength, NonNegativeNumber, Number, Percentage,
@@ -23,7 +23,8 @@ use crate::values::specified::font::{
     self as specified, KeywordInfo, MAX_FONT_WEIGHT, MIN_FONT_WEIGHT,
 };
 use crate::values::specified::length::{FontBaseSize, LineHeightBase};
-use cssparser::{CssStringWriter, Parser, match_ignore_ascii_case, serialize_identifier};
+use crate::values::{CSSInteger, StyleParseErrorKind};
+use cssparser::{CssStringWriter, Parser, serialize_identifier};
 use malloc_size_of::{MallocSizeOf, MallocSizeOfOps};
 use num_traits::abs;
 use num_traits::cast::AsPrimitive;
@@ -707,10 +708,54 @@ impl GenericFontFamily {
 impl Parse for SingleFontFamily {
     /// Parse a font-family value.
     fn parse(context: &ParserContext, input: &mut Parser) -> Result<Self, ParseError> {
+        // CSS-wide keywords must be quoted if used as a font-family name.
+        // The 'default' keyword is also reserved.
+        // (https://drafts.csswg.org/css-values-4/#custom-idents)
+        let is_reserved_kw = |s: &str| -> bool {
+            CSSWideKeyword::from_ident(s).is_ok() || s.eq_ignore_ascii_case("default")
+        };
+
+        // Determine the canonical syntax for a given font-family name: either as a
+        // quoted string or as space-separated identifiers.
+        let canonical_syntax_for = |s: &str| -> FontFamilyNameSyntax {
+            // The name will need quoting if:
+            // - it contains a word that begins with a digit;
+            // - any non-alphanumeric/dash/underscore ASCII codepoints are present;
+            // - it has leading, trailing, or repeated spaces.
+            // (https://github.com/w3c/csswg-drafts/issues/5846)
+            let mut in_word = false;
+            for b in s.as_bytes() {
+                if in_word && *b == b' ' {
+                    in_word = false;
+                    continue;
+                }
+                if !in_word && b.is_ascii_digit() {
+                    return FontFamilyNameSyntax::Quoted;
+                }
+                if b.is_ascii_alphanumeric() || *b == b'-' || *b == b'_' || !b.is_ascii() {
+                    in_word = true;
+                    continue;
+                }
+                return FontFamilyNameSyntax::Quoted;
+            }
+            if !in_word {
+                return FontFamilyNameSyntax::Quoted;
+            }
+            // Quotes also required if it matches one of the CSS-wide keywords...
+            if is_reserved_kw(s) {
+                return FontFamilyNameSyntax::Quoted;
+            }
+            // ...or if it parses as a generic-font-family.
+            if GenericFontFamily::parse(context, &mut Parser::new(&s)).is_ok() {
+                return FontFamilyNameSyntax::Quoted;
+            }
+            FontFamilyNameSyntax::Identifiers
+        };
+
         if let Ok(value) = input.try_parse(|i| i.expect_string_cloned()) {
             return Ok(SingleFontFamily::FamilyName(FamilyName {
                 name: Atom::from(&*value),
-                syntax: FontFamilyNameSyntax::Quoted,
+                syntax: canonical_syntax_for(&value),
             }));
         }
 
@@ -719,46 +764,19 @@ impl Parse for SingleFontFamily {
         }
 
         let first_ident = input.expect_ident_cloned()?;
-        let reserved = match_ignore_ascii_case! { &first_ident,
-            // https://drafts.csswg.org/css-fonts/#propdef-font-family
-            // "Font family names that happen to be the same as a keyword value
-            //  (`inherit`, `serif`, `sans-serif`, `monospace`, `fantasy`, and `cursive`)
-            //  must be quoted to prevent confusion with the keywords with the same names.
-            //  The keywords ‘initial’ and ‘default’ are reserved for future use
-            //  and must also be quoted when used as font names.
-            //  UAs must not consider these keywords as matching the <family-name> type."
-            "inherit" | "initial" | "unset" | "revert" | "default" => true,
-            _ => false,
-        };
-
         let mut value = first_ident.as_ref().to_owned();
-        let mut serialize_quoted = value.contains(' ');
-
-        // These keywords are not allowed by themselves.
-        // The only way this value can be valid with with another keyword.
-        if reserved {
-            let ident = input.expect_ident()?;
-            serialize_quoted = serialize_quoted || ident.contains(' ');
-            value.push(' ');
-            value.push_str(ident);
-        }
         while let Ok(ident) = input.try_parse(|i| i.expect_ident_cloned()) {
-            serialize_quoted = serialize_quoted || ident.contains(' ');
             value.push(' ');
             value.push_str(&ident);
         }
-        let syntax = if serialize_quoted {
-            // For font family names which contains special white spaces, e.g.
-            // `font-family: \ a\ \ b\ \ c\ ;`, it is tricky to serialize them
-            // as identifiers correctly. Just mark them quoted so we don't need
-            // to worry about them in serialization code.
-            FontFamilyNameSyntax::Quoted
-        } else {
-            FontFamilyNameSyntax::Identifiers
-        };
+
+        if is_reserved_kw(&value) {
+            return Err(ParseError::custom(StyleParseErrorKind::UnspecifiedError));
+        }
+
         Ok(SingleFontFamily::FamilyName(FamilyName {
-            name: Atom::from(value),
-            syntax,
+            name: Atom::from(&*value),
+            syntax: canonical_syntax_for(&value),
         }))
     }
 }

testing/web-platform/meta/css/css-font-loading/fontfaceset-load-css-wide-keywords.html.ini

@@ -1,8 +0,0 @@
-[fontfaceset-load-css-wide-keywords.html]
-  expected:
-    if (os == "android") and fission: [OK, TIMEOUT]
-  [Loading value with CSS-wide keyword "revert-layer" causes SyntaxError (document)]
-    expected: FAIL
-
-  [Loading value with CSS-wide keyword "revert-layer" causes SyntaxError (worker)]
-    expected: FAIL

testing/web-platform/tests/css/css-fonts/parsing/font-family-invalid.html

@@ -19,6 +19,15 @@ test_invalid_value('font-family', 'Ahem!, sans-serif');
 test_invalid_value('font-family', 'test@foo, sans-serif');
 test_invalid_value('font-family', '#POUND, sans-serif');
 test_invalid_value('font-family', 'Hawaii 5-0, sans-serif');
+
+// CSS-wide keywords are only valid alone, not in a family-name list.
+test_invalid_value('font-family', 'FirstName, Initial, LastName');
+test_invalid_value('font-family', 'FirstName, inherit, LastName');
+test_invalid_value('font-family', 'FirstName, LastName, unset');
+test_invalid_value('font-family', 'default, FirstName, LastName');
+test_invalid_value('font-family', 'FirstName, LastName, revert');
+test_invalid_value('font-family', 'FirstName, LastName, revert-layer');
+test_invalid_value('font-family', 'FirstName, LastName, revert-rule');
 </script>
 </body>
 </html>

testing/web-platform/tests/css/cssom/font-family-serialization-001.html

@@ -129,6 +129,18 @@
     assert_equals(SetFontFamilyAndSerializeSpecified('"default"'), '"default"');
   }, "Specified: quoted 'default' stays quoted");
 
+  test(function() {
+    assert_equals(SetFontFamilyAndSerializeSpecified('"revert"'), '"revert"');
+  }, "Specified: quoted 'revert' stays quoted");
+
+  test(function() {
+    assert_equals(SetFontFamilyAndSerializeSpecified('"revert-layer"'), '"revert-layer"');
+  }, "Specified: quoted 'revert-layer' stays quoted");
+
+  test(function() {
+    assert_equals(SetFontFamilyAndSerializeSpecified('"revert-rule"'), '"revert-rule"');
+  }, "Specified: quoted 'revert-rule' stays quoted");
+
   // Names with special characters: must stay quoted.
 
   test(function() {