From 678760bd8736da40ef0cc8f4491d61c664465f7b Mon Sep 17 00:00:00 2001 From: Sam Atkins Date: Tue, 10 Aug 2021 17:01:26 +0100 Subject: [PATCH] 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(). :^) --- .../Libraries/LibWeb/CSS/Parser/Parser.cpp | 142 +++++++++++++----- Userland/Libraries/LibWeb/CSS/Parser/Parser.h | 1 + .../Libraries/LibWeb/CSS/StyleResolver.cpp | 48 ++---- Userland/Libraries/LibWeb/CSS/StyleValue.h | 22 +-- 4 files changed, 120 insertions(+), 93 deletions(-) diff --git a/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp b/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp index 47579402a5..454d00cb9e 100644 --- a/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp +++ b/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp @@ -2342,30 +2342,11 @@ RefPtr 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 font_style; RefPtr font_weight; RefPtr font_size; RefPtr line_height; - NonnullRefPtrVector font_families; + RefPtr 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 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 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 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 Parser::parse_font_family_value(ParsingContext const& context, Vector 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 font_families; + Vector 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 Parser::parse_list_style_value(ParsingContext const& context, Vector const& component_values) @@ -2758,6 +2816,10 @@ RefPtr Parser::parse_css_value(PropertyID property_id, TokenStream parse_flex_value(ParsingContext const&, Vector const&); static RefPtr parse_flex_flow_value(ParsingContext const&, Vector const&); static RefPtr parse_font_value(ParsingContext const&, Vector const&); + static RefPtr parse_font_family_value(ParsingContext const&, Vector const&, size_t start_index = 0); static RefPtr parse_list_style_value(ParsingContext const&, Vector const&); static RefPtr parse_overflow_value(ParsingContext const&, Vector const&); static RefPtr parse_text_decoration_value(ParsingContext const&, Vector const&); diff --git a/Userland/Libraries/LibWeb/CSS/StyleResolver.cpp b/Userland/Libraries/LibWeb/CSS/StyleResolver.cpp index edb5902525..8cef749945 100644 --- a/Userland/Libraries/LibWeb/CSS/StyleResolver.cpp +++ b/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(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(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(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; } diff --git a/Userland/Libraries/LibWeb/CSS/StyleValue.h b/Userland/Libraries/LibWeb/CSS/StyleValue.h index 49d131e125..17697b815c 100644 --- a/Userland/Libraries/LibWeb/CSS/StyleValue.h +++ b/Userland/Libraries/LibWeb/CSS/StyleValue.h @@ -855,37 +855,29 @@ private: class FontStyleValue final : public StyleValue { public: - static NonnullRefPtr create(NonnullRefPtr font_style, NonnullRefPtr font_weight, NonnullRefPtr font_size, NonnullRefPtr line_height, NonnullRefPtrVector&& font_families) { return adopt_ref(*new FontStyleValue(font_style, font_weight, font_size, line_height, move(font_families))); } + static NonnullRefPtr create(NonnullRefPtr font_style, NonnullRefPtr font_weight, NonnullRefPtr font_size, NonnullRefPtr line_height, NonnullRefPtr font_families) { return adopt_ref(*new FontStyleValue(font_style, font_weight, font_size, line_height, font_families)); } virtual ~FontStyleValue() override { } NonnullRefPtr font_style() const { return m_font_style; } NonnullRefPtr font_weight() const { return m_font_weight; } NonnullRefPtr font_size() const { return m_font_size; } NonnullRefPtr line_height() const { return m_line_height; } - NonnullRefPtrVector const& font_families() const { return m_font_families; } + NonnullRefPtr 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 font_style, NonnullRefPtr font_weight, NonnullRefPtr font_size, NonnullRefPtr line_height, NonnullRefPtrVector&& font_families) + FontStyleValue(NonnullRefPtr font_style, NonnullRefPtr font_weight, NonnullRefPtr font_size, NonnullRefPtr line_height, NonnullRefPtr 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 m_font_weight; NonnullRefPtr m_font_size; NonnullRefPtr m_line_height; - NonnullRefPtrVector m_font_families; + NonnullRefPtr m_font_families; // FIXME: Implement font-stretch and font-variant. };