LibWeb: Do CSS var() expansion in a separate pass

By expanding all the var() values first, we allow var() to occur
anywhere, even as arguments to CSS functions.

2 files changed, 80 insertions, 53 deletions

diff --git a/Userland/Libraries/LibWeb/CSS/StyleComputer.cpp b/Userland/Libraries/LibWeb/CSS/StyleComputer.cpp
index 36691a0f3b..c3ee482b1e 100644
--- a/Userland/Libraries/LibWeb/CSS/StyleComputer.cpp
+++ b/Userland/Libraries/LibWeb/CSS/StyleComputer.cpp
@@ -556,12 +556,8 @@ static RefPtr<StyleValue> get_custom_property(DOM::Element const& element, FlySt
     return nullptr;
 }
 
-bool StyleComputer::expand_unresolved_values(DOM::Element& element, StringView property_name, HashMap<FlyString, NonnullRefPtr<PropertyDependencyNode>>& dependencies, Parser::TokenStream<Parser::ComponentValue>& source, Vector<Parser::ComponentValue>& dest) const
+bool StyleComputer::expand_variables(DOM::Element& element, StringView property_name, HashMap<FlyString, NonnullRefPtr<PropertyDependencyNode>>& dependencies, Parser::TokenStream<Parser::ComponentValue>& source, Vector<Parser::ComponentValue>& dest) const
 {
-    // FIXME: Do this better!
-    // We build a copy of the tree of ComponentValues, with all var()s and attr()s replaced with their contents.
-    // This is a very naive solution, and we could do better if the CSS Parser could accept tokens one at a time.
-
     // Arbitrary large value chosen to avoid the billion-laughs attack.
     // https://www.w3.org/TR/css-variables-1/#long-variables
     const size_t MAX_VALUE_COUNT = 16384;
@@ -580,51 +576,75 @@ bool StyleComputer::expand_unresolved_values(DOM::Element& element, StringView p
 
     while (source.has_next_token()) {
         auto const& value = source.next_token();
-        if (value.is_function()) {
-            if (value.function().name().equals_ignoring_case("var"sv)) {
-                Parser::TokenStream var_contents { value.function().values() };
-                var_contents.skip_whitespace();
-                if (!var_contents.has_next_token())
-                    return false;
+        if (!value.is_function()) {
+            dest.empend(value);
+            continue;
+        }
+        if (!value.function().name().equals_ignoring_case("var"sv)) {
+            auto const& source_function = value.function();
+            Vector<Parser::ComponentValue> function_values;
+            Parser::TokenStream source_function_contents { source_function.values() };
+            if (!expand_variables(element, property_name, dependencies, source_function_contents, function_values))
+                return false;
+            NonnullRefPtr<Parser::Function> function = Parser::Function::create(source_function.name(), move(function_values));
+            dest.empend(function);
+            continue;
+        }
 
-                auto const& custom_property_name_token = var_contents.next_token();
-                if (!custom_property_name_token.is(Parser::Token::Type::Ident))
-                    return false;
-                auto custom_property_name = custom_property_name_token.token().ident();
-                if (!custom_property_name.starts_with("--"sv))
-                    return false;
+        Parser::TokenStream var_contents { value.function().values() };
+        var_contents.skip_whitespace();
+        if (!var_contents.has_next_token())
+            return false;
+
+        auto const& custom_property_name_token = var_contents.next_token();
+        if (!custom_property_name_token.is(Parser::Token::Type::Ident))
+            return false;
+        auto custom_property_name = custom_property_name_token.token().ident();
+        if (!custom_property_name.starts_with("--"sv))
+            return false;
+
+        // Detect dependency cycles. https://www.w3.org/TR/css-variables-1/#cycles
+        // We do not do this by the spec, since we are not keeping a graph of var dependencies around,
+        // but rebuilding it every time.
+        if (custom_property_name == property_name)
+            return false;
+        auto parent = get_dependency_node(property_name);
+        auto child = get_dependency_node(custom_property_name);
+        parent->add_child(child);
+        if (parent->has_cycles())
+            return false;
+
+        if (auto custom_property_value = get_custom_property(element, custom_property_name)) {
+            VERIFY(custom_property_value->is_unresolved());
+            Parser::TokenStream custom_property_tokens { custom_property_value->as_unresolved().values() };
+            if (!expand_variables(element, custom_property_name, dependencies, custom_property_tokens, dest))
+                return false;
+            continue;
+        }
 
-                // Detect dependency cycles. https://www.w3.org/TR/css-variables-1/#cycles
-                // We do not do this by the spec, since we are not keeping a graph of var dependencies around,
-                // but rebuilding it every time.
-                if (custom_property_name == property_name)
-                    return false;
-                auto parent = get_dependency_node(property_name);
-                auto child = get_dependency_node(custom_property_name);
-                parent->add_child(child);
-                if (parent->has_cycles())
-                    return false;
+        // Use the provided fallback value, if any.
+        var_contents.skip_whitespace();
+        if (var_contents.has_next_token()) {
+            auto const& comma_token = var_contents.next_token();
+            if (!comma_token.is(Parser::Token::Type::Comma))
+                return false;
+            var_contents.skip_whitespace();
+            if (!expand_variables(element, property_name, dependencies, var_contents, dest))
+                return false;
+        }
+    }
+    return true;
+}
 
-                if (auto custom_property_value = get_custom_property(element, custom_property_name)) {
-                    VERIFY(custom_property_value->is_unresolved());
-                    Parser::TokenStream custom_property_tokens { custom_property_value->as_unresolved().values() };
-                    if (!expand_unresolved_values(element, custom_property_name, dependencies, custom_property_tokens, dest))
-                        return false;
-                    continue;
-                }
+bool StyleComputer::expand_unresolved_values(DOM::Element& element, StringView property_name, Parser::TokenStream<Parser::ComponentValue>& source, Vector<Parser::ComponentValue>& dest) const
+{
+    // FIXME: Do this better!
+    // We build a copy of the tree of ComponentValues, with all var()s and attr()s replaced with their contents.
+    // This is a very naive solution, and we could do better if the CSS Parser could accept tokens one at a time.
 
-                // Use the provided fallback value, if any.
-                var_contents.skip_whitespace();
-                if (var_contents.has_next_token()) {
-                    auto const& comma_token = var_contents.next_token();
-                    if (!comma_token.is(Parser::Token::Type::Comma))
-                        return false;
-                    var_contents.skip_whitespace();
-                    if (!expand_unresolved_values(element, property_name, dependencies, var_contents, dest))
-                        return false;
-                }
-                continue;
-            }
+    while (source.has_next_token()) {
+        auto const& value = source.next_token();
+        if (value.is_function()) {
             if (value.function().name().equals_ignoring_case("attr"sv)) {
                 // https://drafts.csswg.org/css-values-5/#attr-substitution
                 Parser::TokenStream attr_contents { value.function().values() };
@@ -653,7 +673,7 @@ bool StyleComputer::expand_unresolved_values(DOM::Element& element, StringView p
                     if (!comma_token.is(Parser::Token::Type::Comma))
                         return false;
                     attr_contents.skip_whitespace();
-                    if (!expand_unresolved_values(element, property_name, dependencies, attr_contents, dest))
+                    if (!expand_unresolved_values(element, property_name, attr_contents, dest))
                         return false;
                     continue;
                 }
@@ -665,7 +685,7 @@ bool StyleComputer::expand_unresolved_values(DOM::Element& element, StringView p
             auto const& source_function = value.function();
             Vector<Parser::ComponentValue> function_values;
             Parser::TokenStream source_function_contents { source_function.values() };
-            if (!expand_unresolved_values(element, property_name, dependencies, source_function_contents, function_values))
+            if (!expand_unresolved_values(element, property_name, source_function_contents, function_values))
                 return false;
             NonnullRefPtr<Parser::Function> function = Parser::Function::create(source_function.name(), move(function_values));
             dest.empend(function);
@@ -675,7 +695,7 @@ bool StyleComputer::expand_unresolved_values(DOM::Element& element, StringView p
             auto const& source_block = value.block();
             Parser::TokenStream source_block_values { source_block.values() };
             Vector<Parser::ComponentValue> block_values;
-            if (!expand_unresolved_values(element, property_name, dependencies, source_block_values, block_values))
+            if (!expand_unresolved_values(element, property_name, source_block_values, block_values))
                 return false;
             NonnullRefPtr<Parser::Block> block = Parser::Block::create(source_block.token(), move(block_values));
             dest.empend(move(block));
@@ -693,10 +713,16 @@ RefPtr<StyleValue> StyleComputer::resolve_unresolved_style_value(DOM::Element& e
     // to produce a different StyleValue from it.
     VERIFY(unresolved.contains_var_or_attr());
 
-    Vector<Parser::ComponentValue> expanded_values;
+    Parser::TokenStream unresolved_values_without_variables_expanded { unresolved.values() };
+    Vector<Parser::ComponentValue> values_with_variables_expanded;
+
     HashMap<FlyString, NonnullRefPtr<PropertyDependencyNode>> dependencies;
-    Parser::TokenStream unresolved_values { unresolved.values() };
-    if (!expand_unresolved_values(element, string_from_property_id(property_id), dependencies, unresolved_values, expanded_values))
+    if (!expand_variables(element, string_from_property_id(property_id), dependencies, unresolved_values_without_variables_expanded, values_with_variables_expanded))
+        return {};
+
+    Parser::TokenStream unresolved_values_with_variables_expanded { values_with_variables_expanded };
+    Vector<Parser::ComponentValue> expanded_values;
+    if (!expand_unresolved_values(element, string_from_property_id(property_id), unresolved_values_with_variables_expanded, expanded_values))
         return {};
 
     if (auto parsed_value = Parser::Parser::parse_css_value({}, Parser::ParsingContext { document() }, property_id, expanded_values))
diff --git a/Userland/Libraries/LibWeb/CSS/StyleComputer.h b/Userland/Libraries/LibWeb/CSS/StyleComputer.h
index 8a22f750d3..5a010af73f 100644
--- a/Userland/Libraries/LibWeb/CSS/StyleComputer.h
+++ b/Userland/Libraries/LibWeb/CSS/StyleComputer.h
@@ -87,7 +87,8 @@ private:
     void compute_defaulted_property_value(StyleProperties&, DOM::Element const*, CSS::PropertyID, Optional<CSS::Selector::PseudoElement>) const;
 
     RefPtr<StyleValue> resolve_unresolved_style_value(DOM::Element&, PropertyID, UnresolvedStyleValue const&) const;
-    bool expand_unresolved_values(DOM::Element&, StringView property_name, HashMap<FlyString, NonnullRefPtr<PropertyDependencyNode>>& dependencies, Parser::TokenStream<Parser::ComponentValue>& source, Vector<Parser::ComponentValue>& dest) const;
+    bool expand_variables(DOM::Element&, StringView property_name, HashMap<FlyString, NonnullRefPtr<PropertyDependencyNode>>& dependencies, Parser::TokenStream<Parser::ComponentValue>& source, Vector<Parser::ComponentValue>& dest) const;
+    bool expand_unresolved_values(DOM::Element&, StringView property_name, Parser::TokenStream<Parser::ComponentValue>& source, Vector<Parser::ComponentValue>& dest) const;
 
     template<typename Callback>
     void for_each_stylesheet(CascadeOrigin, Callback) const;