LibWeb: Parse multiple font-family values

Apart from now gathering comma-separated font-family names into a
StyleValueList, this also corrects the logic for parsing a single
font-family. Previously, we did not handle unquoted font names at all,
and now they are handled including when they are several words separated
by whitespace.

Modified the logic for `font` to use `parse_font_family_value()`.
`FontStyleValue.font_families()` is now a StyleValueList instead of a
vector, so that it can be better handled in StyleResolver.

We also finally remove the CSS::ParsingContext in
set_property_expanding_shorthands(). :^)

Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp

@@ -2342,30 +2342,11 @@ RefPtr<StyleValue> Parser::parse_font_value(ParsingContext const& context, Vecto
         return false;
     };
 
-    auto is_font_family = [](StyleValue const& value) -> bool {
-        if (value.is_string())
-            return true;
-        switch (value.to_identifier()) {
-        case ValueID::Cursive:
-        case ValueID::Fantasy:
-        case ValueID::Monospace:
-        case ValueID::Serif:
-        case ValueID::SansSerif:
-        case ValueID::UiMonospace:
-        case ValueID::UiRounded:
-        case ValueID::UiSerif:
-        case ValueID::UiSansSerif:
-            return true;
-        default:
-            return false;
-        }
-    };
-
     RefPtr<StyleValue> font_style;
     RefPtr<StyleValue> font_weight;
     RefPtr<StyleValue> font_size;
     RefPtr<StyleValue> line_height;
-    NonnullRefPtrVector<StyleValue> font_families;
+    RefPtr<StyleValue> font_families;
     // FIXME: Implement font-stretch and font-variant.
 
     // FIXME: Handle system fonts. (caption, icon, menu, message-box, small-caption, status-bar)
@@ -2412,24 +2393,11 @@ RefPtr<StyleValue> Parser::parse_font_value(ParsingContext const& context, Vecto
                 }
             }
 
-            // Consume font-family
-            // FIXME: Handle multiple font-families separated by commas, for fallback purposes.
-            if (i + 1 < component_values.size()) {
-                auto& font_family_part = component_values[i + 1];
-                auto maybe_font_family = parse_css_value(context, PropertyID::Font, font_family_part);
-                if (!maybe_font_family) {
-                    // Single-word font-families may not be quoted. We convert it to a String for convenience.
-                    if (font_family_part.is(Token::Type::Ident))
-                        maybe_font_family = StringStyleValue::create(font_family_part.token().ident());
-                    else
-                        return nullptr;
-                } else if (!is_font_family(*maybe_font_family)) {
-                    dbgln("Unable to parse '{}' as a font-family.", font_family_part.to_debug_string());
-                    return nullptr;
-                }
-
-                font_families.append(maybe_font_family.release_nonnull());
-            }
+            // Consume font-families
+            auto maybe_font_families = parse_font_family_value(context, component_values, i + 1);
+            if (!maybe_font_families)
+                return nullptr;
+            font_families = maybe_font_families.release_nonnull();
             break;
         }
 
@@ -2443,7 +2411,7 @@ RefPtr<StyleValue> Parser::parse_font_value(ParsingContext const& context, Vecto
     if (unset_value_count < normal_count)
         return nullptr;
 
-    if (!font_size || font_families.is_empty())
+    if (!font_size || !font_families)
         return nullptr;
 
     if (!font_style)
@@ -2453,7 +2421,97 @@ RefPtr<StyleValue> Parser::parse_font_value(ParsingContext const& context, Vecto
     if (!line_height)
         line_height = IdentifierStyleValue::create(ValueID::Normal);
 
-    return FontStyleValue::create(font_style.release_nonnull(), font_weight.release_nonnull(), font_size.release_nonnull(), line_height.release_nonnull(), move(font_families));
+    return FontStyleValue::create(font_style.release_nonnull(), font_weight.release_nonnull(), font_size.release_nonnull(), line_height.release_nonnull(), font_families.release_nonnull());
+}
+
+RefPtr<StyleValue> Parser::parse_font_family_value(ParsingContext const& context, Vector<StyleComponentValueRule> const& component_values, size_t start_index)
+{
+    auto is_generic_font_family = [](ValueID identifier) -> bool {
+        switch (identifier) {
+        case ValueID::Cursive:
+        case ValueID::Fantasy:
+        case ValueID::Monospace:
+        case ValueID::Serif:
+        case ValueID::SansSerif:
+        case ValueID::UiMonospace:
+        case ValueID::UiRounded:
+        case ValueID::UiSerif:
+        case ValueID::UiSansSerif:
+            return true;
+        default:
+            return false;
+        }
+    };
+
+    auto is_comma_or_eof = [&](size_t i) -> bool {
+        if (i < component_values.size()) {
+            auto& maybe_comma = component_values[i];
+            if (!maybe_comma.is(Token::Type::Comma))
+                return false;
+        }
+        return true;
+    };
+
+    // Note: Font-family names can either be a quoted string, or a keyword, or a series of custom-idents.
+    // eg, these are equivalent:
+    //     font-family: my cool     font\!, serif;
+    //     font-family: "my cool font!", serif;
+    NonnullRefPtrVector<StyleValue> font_families;
+    Vector<String> current_name_parts;
+    for (size_t i = start_index; i < component_values.size(); ++i) {
+        auto& part = component_values[i];
+
+        if (part.is(Token::Type::String)) {
+            // `font-family: my cool "font";` is invalid.
+            if (!current_name_parts.is_empty())
+                return nullptr;
+            if (!is_comma_or_eof(i + 1))
+                return nullptr;
+            font_families.append(StringStyleValue::create(part.token().string()));
+            i++;
+            continue;
+        }
+        if (part.is(Token::Type::Ident)) {
+            // If this is a valid identifier, it's NOT a custom-ident and can't be part of a larger name.
+            auto maybe_ident = parse_css_value(context, PropertyID::FontFamily, part);
+            if (maybe_ident) {
+                // CSS-wide keywords are not allowed
+                if (maybe_ident->is_builtin())
+                    return nullptr;
+                if (is_generic_font_family(maybe_ident->to_identifier())) {
+                    // Can't have a generic-font-name as a token in an unquoted font name.
+                    if (!current_name_parts.is_empty())
+                        return nullptr;
+                    if (!is_comma_or_eof(i + 1))
+                        return nullptr;
+                    font_families.append(maybe_ident.release_nonnull());
+                    i++;
+                    continue;
+                }
+            }
+            current_name_parts.append(part.token().ident());
+            continue;
+        }
+        if (part.is(Token::Type::Comma)) {
+            if (current_name_parts.is_empty())
+                return nullptr;
+            font_families.append(StringStyleValue::create(String::join(' ', current_name_parts)));
+            current_name_parts.clear();
+            // Can't have a trailing comma
+            if (i + 1 == component_values.size())
+                return nullptr;
+            continue;
+        }
+    }
+
+    if (!current_name_parts.is_empty()) {
+        font_families.append(StringStyleValue::create(String::join(' ', current_name_parts)));
+        current_name_parts.clear();
+    }
+
+    if (font_families.is_empty())
+        return nullptr;
+    return StyleValueList::create(move(font_families));
 }
 
 RefPtr<StyleValue> Parser::parse_list_style_value(ParsingContext const& context, Vector<StyleComponentValueRule> const& component_values)
@@ -2758,6 +2816,10 @@ RefPtr<StyleValue> Parser::parse_css_value(PropertyID property_id, TokenStream<S
         if (auto parsed_value = parse_font_value(m_context, component_values))
             return parsed_value;
         break;
+    case PropertyID::FontFamily:
+        if (auto parsed_value = parse_font_family_value(m_context, component_values))
+            return parsed_value;
+        break;
     case PropertyID::ListStyle:
         if (auto parsed_value = parse_list_style_value(m_context, component_values))
             return parsed_value;

Userland/Libraries/LibWeb/CSS/Parser/Parser.h

@@ -185,6 +185,7 @@ private:
     static RefPtr<StyleValue> parse_flex_value(ParsingContext const&, Vector<StyleComponentValueRule> const&);
     static RefPtr<StyleValue> parse_flex_flow_value(ParsingContext const&, Vector<StyleComponentValueRule> const&);
     static RefPtr<StyleValue> parse_font_value(ParsingContext const&, Vector<StyleComponentValueRule> const&);
+    static RefPtr<StyleValue> parse_font_family_value(ParsingContext const&, Vector<StyleComponentValueRule> const&, size_t start_index = 0);
     static RefPtr<StyleValue> parse_list_style_value(ParsingContext const&, Vector<StyleComponentValueRule> const&);
     static RefPtr<StyleValue> parse_overflow_value(ParsingContext const&, Vector<StyleComponentValueRule> const&);
     static RefPtr<StyleValue> parse_text_decoration_value(ParsingContext const&, Vector<StyleComponentValueRule> const&);

Userland/Libraries/LibWeb/CSS/StyleResolver.cpp

@@ -147,32 +147,8 @@ static bool contains(Edge a, Edge b)
     return a == b || b == Edge::All;
 }
 
-static inline bool is_font_family(StyleValue const& value)
-{
-    if (value.is_builtin_or_dynamic())
-        return true;
-    if (value.is_string())
-        return true;
-    switch (value.to_identifier()) {
-    case ValueID::Cursive:
-    case ValueID::Fantasy:
-    case ValueID::Monospace:
-    case ValueID::Serif:
-    case ValueID::SansSerif:
-    case ValueID::UiMonospace:
-    case ValueID::UiRounded:
-    case ValueID::UiSerif:
-    case ValueID::UiSansSerif:
-        return true;
-    default:
-        return false;
-    }
-}
-
 static void set_property_expanding_shorthands(StyleProperties& style, CSS::PropertyID property_id, StyleValue const& value, DOM::Document& document, bool is_internally_generated_pseudo_property = false)
 {
-    CSS::ParsingContext context(document);
-
     if (is_pseudo_property(property_id) && !is_internally_generated_pseudo_property) {
         dbgln("Ignoring non-internally-generated pseudo property: {}", string_from_property_id(property_id));
         return;
@@ -498,8 +474,7 @@ static void set_property_expanding_shorthands(StyleProperties& style, CSS::Prope
         if (value.is_font()) {
             auto& font_shorthand = static_cast<CSS::FontStyleValue const&>(value);
             style.set_property(CSS::PropertyID::FontSize, font_shorthand.font_size());
-            // FIXME: Support multiple font-families
-            style.set_property(CSS::PropertyID::FontFamily, font_shorthand.font_families().first());
+            set_property_expanding_shorthands(style, CSS::PropertyID::FontFamily, *font_shorthand.font_families(), document);
             style.set_property(CSS::PropertyID::FontStyle, font_shorthand.font_style());
             style.set_property(CSS::PropertyID::FontWeight, font_shorthand.font_weight());
             style.set_property(CSS::PropertyID::LineHeight, font_shorthand.line_height());
@@ -520,21 +495,18 @@ static void set_property_expanding_shorthands(StyleProperties& style, CSS::Prope
     }
 
     if (property_id == CSS::PropertyID::FontFamily) {
-        if (value.is_component_value_list()) {
-            auto parts = static_cast<CSS::ValueListStyleValue const&>(value).values();
-            // FIXME: Handle multiple font-families separated by commas, for fallback purposes.
-            for (auto& part : parts) {
-                auto value = Parser::parse_css_value(context, property_id, part);
-                if (!value)
-                    return;
-                if (is_font_family(*value))
-                    style.set_property(CSS::PropertyID::FontFamily, *value);
-                break;
+        if (value.is_value_list()) {
+            auto& values_list = static_cast<StyleValueList const&>(value).values();
+            // FIXME: Support multiple font-families
+            if (!values_list.is_empty()) {
+                style.set_property(CSS::PropertyID::FontFamily, values_list.first());
             }
             return;
         }
-
-        style.set_property(CSS::PropertyID::FontFamily, value);
+        if (value.is_builtin() || value.is_string() || value.is_identifier()) {
+            style.set_property(CSS::PropertyID::FontFamily, value);
+            return;
+        }
         return;
     }
 

Userland/Libraries/LibWeb/CSS/StyleValue.h

@@ -855,37 +855,29 @@ private:
 
 class FontStyleValue final : public StyleValue {
 public:
-    static NonnullRefPtr<FontStyleValue> create(NonnullRefPtr<StyleValue> font_style, NonnullRefPtr<StyleValue> font_weight, NonnullRefPtr<StyleValue> font_size, NonnullRefPtr<StyleValue> line_height, NonnullRefPtrVector<StyleValue>&& font_families) { return adopt_ref(*new FontStyleValue(font_style, font_weight, font_size, line_height, move(font_families))); }
+    static NonnullRefPtr<FontStyleValue> create(NonnullRefPtr<StyleValue> font_style, NonnullRefPtr<StyleValue> font_weight, NonnullRefPtr<StyleValue> font_size, NonnullRefPtr<StyleValue> line_height, NonnullRefPtr<StyleValue> font_families) { return adopt_ref(*new FontStyleValue(font_style, font_weight, font_size, line_height, font_families)); }
     virtual ~FontStyleValue() override { }
 
     NonnullRefPtr<StyleValue> font_style() const { return m_font_style; }
     NonnullRefPtr<StyleValue> font_weight() const { return m_font_weight; }
     NonnullRefPtr<StyleValue> font_size() const { return m_font_size; }
     NonnullRefPtr<StyleValue> line_height() const { return m_line_height; }
-    NonnullRefPtrVector<StyleValue> const& font_families() const { return m_font_families; }
+    NonnullRefPtr<StyleValue> font_families() const { return m_font_families; }
 
     virtual String to_string() const override
     {
-        StringBuilder string_builder;
-        string_builder.appendff("Font style: {}, weight: {}, size: {}, line_height: {}, families: [",
-            m_font_style->to_string(), m_font_weight->to_string(), m_font_size->to_string(), m_line_height->to_string());
-        for (auto& family : m_font_families) {
-            string_builder.append(family.to_string());
-            string_builder.append(",");
-        }
-        string_builder.append("]");
-
-        return string_builder.to_string();
+        return String::formatted("Font style: {}, weight: {}, size: {}, line_height: {}, families: {}",
+            m_font_style->to_string(), m_font_weight->to_string(), m_font_size->to_string(), m_line_height->to_string(), m_font_families->to_string());
     }
 
 private:
-    FontStyleValue(NonnullRefPtr<StyleValue> font_style, NonnullRefPtr<StyleValue> font_weight, NonnullRefPtr<StyleValue> font_size, NonnullRefPtr<StyleValue> line_height, NonnullRefPtrVector<StyleValue>&& font_families)
+    FontStyleValue(NonnullRefPtr<StyleValue> font_style, NonnullRefPtr<StyleValue> font_weight, NonnullRefPtr<StyleValue> font_size, NonnullRefPtr<StyleValue> line_height, NonnullRefPtr<StyleValue> font_families)
         : StyleValue(Type::Font)
         , m_font_style(font_style)
         , m_font_weight(font_weight)
         , m_font_size(font_size)
         , m_line_height(line_height)
-        , m_font_families(move(font_families))
+        , m_font_families(font_families)
     {
     }
 
@@ -893,7 +885,7 @@ private:
     NonnullRefPtr<StyleValue> m_font_weight;
     NonnullRefPtr<StyleValue> m_font_size;
     NonnullRefPtr<StyleValue> m_line_height;
-    NonnullRefPtrVector<StyleValue> m_font_families;
+    NonnullRefPtr<StyleValue> m_font_families;
     // FIXME: Implement font-stretch and font-variant.
 };