From c8c2a8df56de842a63c99c7adfd50265957241f7 Mon Sep 17 00:00:00 2001 From: Sam Atkins Date: Thu, 5 Aug 2021 15:26:04 +0100 Subject: [PATCH] LibWeb: Make '0' always be both a number and a length in CSS A '0' token can be interpreted both as a Number, and as a Length. This is problematic as in our CSS parser, we often call parse_css_value() first, to figure out what something is, and then assign it. So we do not know in advance whether we want a Length or not. Previously, it always got parsed as a Length, and then every place that expected a NumericStyleValue had to also check for a Length(0), which is easy to forget to do. In particular, this was causing issues with the `flex` property parsing. To solve this, we now always parse 0 as a NumericStyleValue, and NSVs of 0 pretend to be a Length(0px) when asked. In two places, we were casting to a LengthStyleValue* based on is_length(), which no longer works, so those have been adjusted to use `StyleValue::to_length()` instead. They also now check for `is_numeric()` first, to avoid the extra conversion to a Length and back. Possibly this opens up new issues elsewhere. In my testing it seems fine, but until we can get CSS test suites running, it's hard to know for certain. --- .../Libraries/LibWeb/CSS/Parser/Parser.cpp | 7 +++++- .../Libraries/LibWeb/CSS/StyleProperties.cpp | 22 ++++++++++--------- Userland/Libraries/LibWeb/CSS/StyleValue.h | 5 ++++- 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp b/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp index a0d9da7371..91bbaad40f 100644 --- a/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp +++ b/Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp @@ -1503,7 +1503,12 @@ Optional Parser::parse_length(ParsingContext const& context, StyleCompon RefPtr Parser::parse_length_value(ParsingContext const& context, StyleComponentValueRule const& component_value) { - if (component_value.is(Token::Type::Dimension) || component_value.is(Token::Type::Number) || component_value.is(Token::Type::Percentage)) { + // Numbers with no units can be lengths, in two situations: + // 1) We're in quirks mode, and it's an integer. + // 2) It's a 0. + // We handle case 1 here. Case 2 is handled by NumericStyleValue pretending to be a LengthStyleValue if it is 0. + if (component_value.is(Token::Type::Dimension) || component_value.is(Token::Type::Percentage) + || (context.in_quirks_mode() && component_value.is(Token::Type::Number) && component_value.token().m_value.string_view() != "0"sv)) { auto length = parse_length(context, component_value); if (length.has_value()) return LengthStyleValue::create(length.value()); diff --git a/Userland/Libraries/LibWeb/CSS/StyleProperties.cpp b/Userland/Libraries/LibWeb/CSS/StyleProperties.cpp index caefdc203e..b066a96e8b 100644 --- a/Userland/Libraries/LibWeb/CSS/StyleProperties.cpp +++ b/Userland/Libraries/LibWeb/CSS/StyleProperties.cpp @@ -299,12 +299,13 @@ Optional StyleProperties::flex_grow_factor() const auto value = property(CSS::PropertyID::FlexGrow); if (!value.has_value()) return {}; - if (value.value()->is_length() && verify_cast(value.value().ptr())->to_length().raw_value() == 0) + if (value.value()->is_numeric()) { + auto numeric = verify_cast(value.value().ptr()); + return numeric->value(); + } + if (value.value()->is_length() && value.value()->to_length().raw_value() == 0) return { 0 }; - if (!value.value()->is_numeric()) - return {}; - auto numeric = verify_cast(value.value().ptr()); - return numeric->value(); + return {}; } Optional StyleProperties::flex_shrink_factor() const @@ -312,12 +313,13 @@ Optional StyleProperties::flex_shrink_factor() const auto value = property(CSS::PropertyID::FlexShrink); if (!value.has_value()) return {}; - if (value.value()->is_length() && verify_cast(value.value().ptr())->to_length().raw_value() == 0) + if (!value.value()->is_numeric()) { + auto numeric = verify_cast(value.value().ptr()); + return numeric->value(); + } + if (value.value()->is_length() && value.value()->to_length().raw_value() == 0) return { 0 }; - if (!value.value()->is_numeric()) - return {}; - auto numeric = verify_cast(value.value().ptr()); - return numeric->value(); + return {}; } Optional StyleProperties::justify_content() const { diff --git a/Userland/Libraries/LibWeb/CSS/StyleValue.h b/Userland/Libraries/LibWeb/CSS/StyleValue.h index 9fd8019e6d..8790379ee7 100644 --- a/Userland/Libraries/LibWeb/CSS/StyleValue.h +++ b/Userland/Libraries/LibWeb/CSS/StyleValue.h @@ -239,7 +239,7 @@ public: bool is_identifier() const { return type() == Type::Identifier; } bool is_image() const { return type() == Type::Image; } bool is_string() const { return type() == Type::String; } - bool is_length() const { return type() == Type::Length; } + virtual bool is_length() const { return type() == Type::Length; } bool is_custom_property() const { return type() == Type::CustomProperty; } bool is_numeric() const { return type() == Type::Numeric; } bool is_value_list() const { return type() == Type::ValueList; } @@ -303,6 +303,9 @@ public: return adopt_ref(*new NumericStyleValue(value)); } + virtual bool is_length() const override { return m_value == 0; } + virtual Length to_length() const override { return Length(0, Length::Type::Px); } + float value() const { return m_value; } String to_string() const override { return String::formatted("{}", m_value); }