From fb722e69f35cfa7639b971677d4031ae320959d5 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Wed, 17 May 2023 17:05:36 +0200 Subject: [PATCH] LibWeb: Resolve CSS custom properties on pseudo elements The resolved property sets are stored with the element in a per-pseudo-element array (same as for pseudo element layout nodes). Longer term, we should stop storing this with elements entirely and make it temporary state in StyleComputer somehow, so we don't waste memory keeping all the resolved properties around. This makes various gradients show up on https://shopify.com/ :^) --- ...seudo-element-with-custom-properties-2.txt | 60 +++++++++++++++++++ .../pseudo-element-with-custom-properties.txt | 9 +++ ...eudo-element-with-custom-properties-2.html | 32 ++++++++++ ...pseudo-element-with-custom-properties.html | 15 +++++ .../Libraries/LibWeb/CSS/StyleComputer.cpp | 50 +++++++++------- Userland/Libraries/LibWeb/CSS/StyleComputer.h | 4 +- Userland/Libraries/LibWeb/DOM/Element.cpp | 16 +++++ Userland/Libraries/LibWeb/DOM/Element.h | 5 +- .../WebContent/ConnectionFromClient.cpp | 8 +-- 9 files changed, 170 insertions(+), 29 deletions(-) create mode 100644 Tests/LibWeb/Layout/expected/pseudo-element-with-custom-properties-2.txt create mode 100644 Tests/LibWeb/Layout/expected/pseudo-element-with-custom-properties.txt create mode 100644 Tests/LibWeb/Layout/input/pseudo-element-with-custom-properties-2.html create mode 100644 Tests/LibWeb/Layout/input/pseudo-element-with-custom-properties.html diff --git a/Tests/LibWeb/Layout/expected/pseudo-element-with-custom-properties-2.txt b/Tests/LibWeb/Layout/expected/pseudo-element-with-custom-properties-2.txt new file mode 100644 index 0000000000..6e145c759e --- /dev/null +++ b/Tests/LibWeb/Layout/expected/pseudo-element-with-custom-properties-2.txt @@ -0,0 +1,60 @@ +Viewport <#document> at (0,0) content-size 800x600 children: not-inline + BlockContainer at (0,0) content-size 800x470.195312 [BFC] children: not-inline + BlockContainer at (8,8) content-size 784x454.195312 children: not-inline + BlockContainer <(anonymous)> at (8,8) content-size 784x21.835937 children: inline + line 0 width: 391.640625, height: 21.835937, bottom: 21.835937, baseline: 16.914062 + frag 0 from TextNode start: 0, length: 40, rect: [8,8 391.640625x21.835937] + "Variable set by inline style of element:" + TextNode <#text> + BreakNode
+ TextNode <#text> + BlockContainer at (8,29.835937) content-size 784x100 children: inline + line 0 width: 200, height: 100, bottom: 100, baseline: 16.914062 + frag 0 from BlockContainer start: 0, length: 0, rect: [8,29.835937 200x100] + BlockContainer <(anonymous)> at (8,29.835937) content-size 200x100 inline-block [BFC] children: inline + line 0 width: 0, height: 21.835937, bottom: 21.835937, baseline: 16.914062 + frag 0 from TextNode start: 0, length: 0, rect: [8,29.835937 0x21.835937] + "" + TextNode <#text> + BlockContainer <(anonymous)> at (8,129.835937) content-size 784x66.179687 children: inline + line 0 width: 0, height: 21.835937, bottom: 21.835937, baseline: 16.914062 + line 1 width: 0, height: 21.835937, bottom: 43.671875, baseline: 16.914062 + line 2 width: 441.269531, height: 22.507812, bottom: 66.179687, baseline: 16.914062 + frag 0 from TextNode start: 1, length: 42, rect: [8,172.835937 441.269531x21.835937] + "Variable set by CSS rule matching element:" + TextNode <#text> + BreakNode
+ BreakNode
+ TextNode <#text> + BreakNode
+ TextNode <#text> + BlockContainer at (8,196.015625) content-size 784x100 children: inline + line 0 width: 200, height: 100, bottom: 100, baseline: 16.914062 + frag 0 from BlockContainer start: 0, length: 0, rect: [8,196.015625 200x100] + BlockContainer <(anonymous)> at (8,196.015625) content-size 200x100 inline-block [BFC] children: inline + line 0 width: 0, height: 21.835937, bottom: 21.835937, baseline: 16.914062 + frag 0 from TextNode start: 0, length: 0, rect: [8,196.015625 0x21.835937] + "" + TextNode <#text> + BlockContainer <(anonymous)> at (8,296.015625) content-size 784x66.179687 children: inline + line 0 width: 0, height: 21.835937, bottom: 21.835937, baseline: 16.914062 + line 1 width: 0, height: 21.835937, bottom: 43.671875, baseline: 16.914062 + line 2 width: 520.605468, height: 22.507812, bottom: 66.179687, baseline: 16.914062 + frag 0 from TextNode start: 1, length: 49, rect: [8,339.015625 520.605468x21.835937] + "Variable set by CSS rule matching pseudo element:" + TextNode <#text> + BreakNode
+ BreakNode
+ TextNode <#text> + BreakNode
+ TextNode <#text> + BlockContainer at (8,362.195312) content-size 784x100 children: inline + line 0 width: 200, height: 100, bottom: 100, baseline: 16.914062 + frag 0 from BlockContainer start: 0, length: 0, rect: [8,362.195312 200x100] + BlockContainer <(anonymous)> at (8,362.195312) content-size 200x100 inline-block [BFC] children: inline + line 0 width: 0, height: 21.835937, bottom: 21.835937, baseline: 16.914062 + frag 0 from TextNode start: 0, length: 0, rect: [8,362.195312 0x21.835937] + "" + TextNode <#text> + BlockContainer <(anonymous)> at (8,462.195312) content-size 784x0 children: inline + TextNode <#text> diff --git a/Tests/LibWeb/Layout/expected/pseudo-element-with-custom-properties.txt b/Tests/LibWeb/Layout/expected/pseudo-element-with-custom-properties.txt new file mode 100644 index 0000000000..64f4b3fc9d --- /dev/null +++ b/Tests/LibWeb/Layout/expected/pseudo-element-with-custom-properties.txt @@ -0,0 +1,9 @@ +Viewport <#document> at (0,0) content-size 800x600 children: not-inline + BlockContainer at (0,0) content-size 800x16 [BFC] children: not-inline + BlockContainer at (8,8) content-size 784x0 children: not-inline + BlockContainer at (8,8) content-size 784x0 children: not-inline + BlockContainer <(anonymous)> at (8,8) content-size 500x100 positioned [BFC] children: inline + line 0 width: 0, height: 21.835937, bottom: 21.835937, baseline: 16.914062 + frag 0 from TextNode start: 0, length: 0, rect: [8,8 0x21.835937] + "" + TextNode <#text> diff --git a/Tests/LibWeb/Layout/input/pseudo-element-with-custom-properties-2.html b/Tests/LibWeb/Layout/input/pseudo-element-with-custom-properties-2.html new file mode 100644 index 0000000000..d896a851d8 --- /dev/null +++ b/Tests/LibWeb/Layout/input/pseudo-element-with-custom-properties-2.html @@ -0,0 +1,32 @@ + +Variable set by inline style of element:
+
+

+Variable set by CSS rule matching element:
+
+

+Variable set by CSS rule matching pseudo element:
+
diff --git a/Tests/LibWeb/Layout/input/pseudo-element-with-custom-properties.html b/Tests/LibWeb/Layout/input/pseudo-element-with-custom-properties.html new file mode 100644 index 0000000000..7114a04e1b --- /dev/null +++ b/Tests/LibWeb/Layout/input/pseudo-element-with-custom-properties.html @@ -0,0 +1,15 @@ +
\ No newline at end of file diff --git a/Userland/Libraries/LibWeb/CSS/StyleComputer.cpp b/Userland/Libraries/LibWeb/CSS/StyleComputer.cpp index 5929b78893..47fdbc5be1 100644 --- a/Userland/Libraries/LibWeb/CSS/StyleComputer.cpp +++ b/Userland/Libraries/LibWeb/CSS/StyleComputer.cpp @@ -668,16 +668,21 @@ static void set_property_expanding_shorthands(StyleProperties& style, CSS::Prope style.set_property(property_id, value); } -static RefPtr get_custom_property(DOM::Element const& element, FlyString const& custom_property_name) +static RefPtr get_custom_property(DOM::Element const& element, Optional pseudo_element, FlyString const& custom_property_name) { + if (pseudo_element.has_value()) { + if (auto it = element.custom_properties(pseudo_element).find(custom_property_name.to_string().to_deprecated_string()); it != element.custom_properties(pseudo_element).end()) + return it->value.value; + } + for (auto const* current_element = &element; current_element; current_element = current_element->parent_element()) { - if (auto it = current_element->custom_properties().find(custom_property_name.to_string().to_deprecated_string()); it != current_element->custom_properties().end()) + if (auto it = current_element->custom_properties({}).find(custom_property_name.to_string().to_deprecated_string()); it != current_element->custom_properties({}).end()) return it->value.value; } return nullptr; } -bool StyleComputer::expand_variables(DOM::Element& element, StringView property_name, HashMap>& dependencies, Parser::TokenStream& source, Vector& dest) const +bool StyleComputer::expand_variables(DOM::Element& element, Optional pseudo_element, StringView property_name, HashMap>& dependencies, Parser::TokenStream& source, Vector& dest) const { // Arbitrary large value chosen to avoid the billion-laughs attack. // https://www.w3.org/TR/css-variables-1/#long-variables @@ -705,7 +710,7 @@ bool StyleComputer::expand_variables(DOM::Element& element, StringView property_ auto const& source_function = value.function(); Vector function_values; Parser::TokenStream source_function_contents { source_function.values() }; - if (!expand_variables(element, property_name, dependencies, source_function_contents, function_values)) + if (!expand_variables(element, pseudo_element, property_name, dependencies, source_function_contents, function_values)) return false; NonnullRefPtr function = Parser::Function::create(FlyString::from_utf8(source_function.name()).release_value_but_fixme_should_propagate_errors(), move(function_values)); dest.empend(function); @@ -735,10 +740,10 @@ bool StyleComputer::expand_variables(DOM::Element& element, StringView property_ if (parent->has_cycles()) return false; - if (auto custom_property_value = get_custom_property(element, FlyString::from_utf8(custom_property_name).release_value_but_fixme_should_propagate_errors())) { + if (auto custom_property_value = get_custom_property(element, pseudo_element, FlyString::from_utf8(custom_property_name).release_value_but_fixme_should_propagate_errors())) { 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)) + if (!expand_variables(element, pseudo_element, custom_property_name, dependencies, custom_property_tokens, dest)) return false; continue; } @@ -750,7 +755,7 @@ bool StyleComputer::expand_variables(DOM::Element& element, StringView property_ if (!comma_token.is(Parser::Token::Type::Comma)) return false; var_contents.skip_whitespace(); - if (!expand_variables(element, property_name, dependencies, var_contents, dest)) + if (!expand_variables(element, pseudo_element, property_name, dependencies, var_contents, dest)) return false; } } @@ -850,7 +855,7 @@ bool StyleComputer::expand_unresolved_values(DOM::Element& element, StringView p return true; } -RefPtr StyleComputer::resolve_unresolved_style_value(DOM::Element& element, PropertyID property_id, UnresolvedStyleValue const& unresolved) const +RefPtr StyleComputer::resolve_unresolved_style_value(DOM::Element& element, Optional pseudo_element, PropertyID property_id, UnresolvedStyleValue const& unresolved) const { // Unresolved always contains a var() or attr(), unless it is a custom property's value, in which case we shouldn't be trying // to produce a different StyleValue from it. @@ -860,7 +865,7 @@ RefPtr StyleComputer::resolve_unresolved_style_value(DOM::Element& e Vector values_with_variables_expanded; HashMap> dependencies; - if (!expand_variables(element, string_from_property_id(property_id), dependencies, unresolved_values_without_variables_expanded, values_with_variables_expanded)) + if (!expand_variables(element, pseudo_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 }; @@ -882,7 +887,7 @@ void StyleComputer::cascade_declarations(StyleProperties& style, DOM::Element& e continue; auto property_value = property.value; if (property.value->is_unresolved()) { - if (auto resolved = resolve_unresolved_style_value(element, property.property_id, property.value->as_unresolved())) + if (auto resolved = resolve_unresolved_style_value(element, pseudo_element, property.property_id, property.value->as_unresolved())) property_value = resolved.release_nonnull(); } if (!property_value->is_unresolved()) @@ -897,7 +902,7 @@ void StyleComputer::cascade_declarations(StyleProperties& style, DOM::Element& e continue; auto property_value = property.value; if (property.value->is_unresolved()) { - if (auto resolved = resolve_unresolved_style_value(element, property.property_id, property.value->as_unresolved())) + if (auto resolved = resolve_unresolved_style_value(element, pseudo_element, property.property_id, property.value->as_unresolved())) property_value = resolved.release_nonnull(); } if (!property_value->is_unresolved()) @@ -907,13 +912,16 @@ void StyleComputer::cascade_declarations(StyleProperties& style, DOM::Element& e } } -static ErrorOr cascade_custom_properties(DOM::Element& element, Vector const& matching_rules) +static ErrorOr cascade_custom_properties(DOM::Element& element, Optional pseudo_element, Vector const& matching_rules) { size_t needed_capacity = 0; for (auto const& matching_rule : matching_rules) needed_capacity += verify_cast(matching_rule.rule->declaration()).custom_properties().size(); - if (auto const* inline_style = verify_cast(element.inline_style())) - needed_capacity += inline_style->custom_properties().size(); + + if (!pseudo_element.has_value()) { + if (auto const* inline_style = verify_cast(element.inline_style())) + needed_capacity += inline_style->custom_properties().size(); + } HashMap custom_properties; TRY(custom_properties.try_ensure_capacity(needed_capacity)); @@ -923,12 +931,14 @@ static ErrorOr cascade_custom_properties(DOM::Element& element, Vector(element.inline_style())) { - for (auto const& it : inline_style->custom_properties()) - custom_properties.set(it.key, it.value); + if (!pseudo_element.has_value()) { + if (auto const* inline_style = verify_cast(element.inline_style())) { + for (auto const& it : inline_style->custom_properties()) + custom_properties.set(it.key, it.value); + } } - element.set_custom_properties(move(custom_properties)); + element.set_custom_properties(pseudo_element, move(custom_properties)); return {}; } @@ -953,9 +963,7 @@ ErrorOr StyleComputer::compute_cascaded_values(StyleProperties& style, DOM } // Then we resolve all the CSS custom properties ("variables") for this element: - // FIXME: Look into how custom properties should interact with pseudo elements and support that properly. - if (!pseudo_element.has_value()) - TRY(cascade_custom_properties(element, matching_rule_set.author_rules)); + TRY(cascade_custom_properties(element, pseudo_element, matching_rule_set.author_rules)); // Then we apply the declarations from the matched rules in cascade order: diff --git a/Userland/Libraries/LibWeb/CSS/StyleComputer.h b/Userland/Libraries/LibWeb/CSS/StyleComputer.h index afdd7d96f4..2503075a9e 100644 --- a/Userland/Libraries/LibWeb/CSS/StyleComputer.h +++ b/Userland/Libraries/LibWeb/CSS/StyleComputer.h @@ -94,8 +94,8 @@ private: void compute_defaulted_property_value(StyleProperties&, DOM::Element const*, CSS::PropertyID, Optional) const; - RefPtr resolve_unresolved_style_value(DOM::Element&, PropertyID, UnresolvedStyleValue const&) const; - bool expand_variables(DOM::Element&, StringView property_name, HashMap>& dependencies, Parser::TokenStream& source, Vector& dest) const; + RefPtr resolve_unresolved_style_value(DOM::Element&, Optional, PropertyID, UnresolvedStyleValue const&) const; + bool expand_variables(DOM::Element&, Optional, StringView property_name, HashMap>& dependencies, Parser::TokenStream& source, Vector& dest) const; bool expand_unresolved_values(DOM::Element&, StringView property_name, Parser::TokenStream& source, Vector& dest) const; template diff --git a/Userland/Libraries/LibWeb/DOM/Element.cpp b/Userland/Libraries/LibWeb/DOM/Element.cpp index 5a032c3ce0..1f0d441ee8 100644 --- a/Userland/Libraries/LibWeb/DOM/Element.cpp +++ b/Userland/Libraries/LibWeb/DOM/Element.cpp @@ -1708,4 +1708,20 @@ void Element::set_computed_css_values(RefPtr style) m_computed_css_values = move(style); } +void Element::set_custom_properties(Optional pseudo_element, HashMap custom_properties) +{ + if (!pseudo_element.has_value()) { + m_custom_properties = move(custom_properties); + return; + } + m_pseudo_element_custom_properties[to_underlying(pseudo_element.value())] = move(custom_properties); +} + +HashMap const& Element::custom_properties(Optional pseudo_element) const +{ + if (!pseudo_element.has_value()) + return m_custom_properties; + return m_pseudo_element_custom_properties[to_underlying(pseudo_element.value())]; +} + } diff --git a/Userland/Libraries/LibWeb/DOM/Element.h b/Userland/Libraries/LibWeb/DOM/Element.h index b34efa081e..533dd3b716 100644 --- a/Userland/Libraries/LibWeb/DOM/Element.h +++ b/Userland/Libraries/LibWeb/DOM/Element.h @@ -158,8 +158,8 @@ public: ShadowRoot const* shadow_root_internal() const { return m_shadow_root.ptr(); } void set_shadow_root(JS::GCPtr); - void set_custom_properties(HashMap custom_properties) { m_custom_properties = move(custom_properties); } - HashMap const& custom_properties() const { return m_custom_properties; } + void set_custom_properties(Optional, HashMap custom_properties); + [[nodiscard]] HashMap const& custom_properties(Optional) const; void queue_an_element_task(HTML::Task::Source, JS::SafeFunction); @@ -318,6 +318,7 @@ private: RefPtr m_computed_css_values; HashMap m_custom_properties; + Array, to_underlying(CSS::Selector::PseudoElement::PseudoElementCount)> m_pseudo_element_custom_properties; Vector m_classes; diff --git a/Userland/Services/WebContent/ConnectionFromClient.cpp b/Userland/Services/WebContent/ConnectionFromClient.cpp index c248d6a0d0..e56628039c 100644 --- a/Userland/Services/WebContent/ConnectionFromClient.cpp +++ b/Userland/Services/WebContent/ConnectionFromClient.cpp @@ -455,14 +455,14 @@ Messages::WebContentServer::InspectDomNodeResponse ConnectionFromClient::inspect return builder.to_deprecated_string(); }; - auto serialize_custom_properties_json = [](Web::DOM::Element const& element) -> DeprecatedString { + auto serialize_custom_properties_json = [](Web::DOM::Element const& element, Optional pseudo_element) -> DeprecatedString { StringBuilder builder; auto serializer = MUST(JsonObjectSerializer<>::try_create(builder)); HashTable seen_properties; auto const* element_to_check = &element; while (element_to_check) { - for (auto const& property : element_to_check->custom_properties()) { + for (auto const& property : element_to_check->custom_properties(pseudo_element)) { if (!seen_properties.contains(property.key)) { seen_properties.set(property.key); MUST(serializer.add(property.key, property.value.value->to_string().release_value_but_fixme_should_propagate_errors().to_deprecated_string())); @@ -519,14 +519,14 @@ Messages::WebContentServer::InspectDomNodeResponse ConnectionFromClient::inspect auto pseudo_element_style = MUST(page().focused_context().active_document()->style_computer().compute_style(element, pseudo_element)); DeprecatedString computed_values = serialize_json(pseudo_element_style); DeprecatedString resolved_values = "{}"; - DeprecatedString custom_properties_json = "{}"; + DeprecatedString custom_properties_json = serialize_custom_properties_json(element, pseudo_element); DeprecatedString node_box_sizing_json = serialize_node_box_sizing_json(pseudo_element_node.ptr()); return { true, computed_values, resolved_values, custom_properties_json, node_box_sizing_json }; } DeprecatedString computed_values = serialize_json(*element.computed_css_values()); DeprecatedString resolved_values_json = serialize_json(element.resolved_css_values()); - DeprecatedString custom_properties_json = serialize_custom_properties_json(element); + DeprecatedString custom_properties_json = serialize_custom_properties_json(element, {}); DeprecatedString node_box_sizing_json = serialize_node_box_sizing_json(element.layout_node()); return { true, computed_values, resolved_values_json, custom_properties_json, node_box_sizing_json }; }