From d1fbb7b51efdec9f2e418cde5dcbb1dacaedc80c Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Sun, 27 Jul 2025 19:41:21 +0200 Subject: [PATCH] LibWeb: Invalidate less elements affected by CSS custom properties Before this change, whenever element's attributes changed, we would add a flag to "pending invalidation", indicating that all descendants whose style uses CSS custom properties needed to be recomputed. This resulted in severe overinvalidation, because we would run invalidation regardless of whether any custom property on affected element actually changed. This change takes another approach, and now we decide whether descendant's style needs to be recomputed based on whether ancestor's style recomputation results in a change of custom properties, though this approach adds a little overhead to style computation as now we have to compare old vs new hashmap of custom properties. This brings substantial improvement on discord and x.com where, before this change, advantage of using invalidation sets was lost and we had to recompute all descendants, because almost all of them use custom properties. --- Libraries/LibWeb/CSS/Parser/ValueParsing.cpp | 6 +++-- Libraries/LibWeb/CSS/StyleComputer.cpp | 18 +++++++++---- Libraries/LibWeb/CSS/StyleComputer.h | 6 ++--- Libraries/LibWeb/CSS/StyleProperty.cpp | 7 +++++ Libraries/LibWeb/CSS/StyleProperty.h | 2 ++ .../CSS/StyleValues/UnresolvedStyleValue.h | 2 ++ Libraries/LibWeb/DOM/Document.cpp | 24 ++++++++++------- Libraries/LibWeb/DOM/Element.cpp | 21 +++++---------- Libraries/LibWeb/DOM/Element.h | 11 +++++--- Libraries/LibWeb/DOM/Node.cpp | 4 +-- Libraries/LibWeb/DOM/Node.h | 1 - Libraries/LibWeb/DOM/StyleInvalidator.cpp | 26 ++++++------------- Libraries/LibWeb/DOM/StyleInvalidator.h | 10 ++----- 13 files changed, 71 insertions(+), 67 deletions(-) diff --git a/Libraries/LibWeb/CSS/Parser/ValueParsing.cpp b/Libraries/LibWeb/CSS/Parser/ValueParsing.cpp index 4127436b97d..6bf231d44f4 100644 --- a/Libraries/LibWeb/CSS/Parser/ValueParsing.cpp +++ b/Libraries/LibWeb/CSS/Parser/ValueParsing.cpp @@ -4339,8 +4339,10 @@ NonnullRefPtr Parser::resolve_unresolved_style_value(Parsin NonnullRefPtr Parser::resolve_unresolved_style_value(DOM::AbstractElement& element, GuardedSubstitutionContexts& guarded_contexts, PropertyIDOrCustomPropertyName property, UnresolvedStyleValue const& unresolved) { // AD-HOC: Report that we might rely on custom properties. - // FIXME: This over-invalidates. Find a way of invalidating only when we need to - specifically, when var() is used. - element.element().set_style_uses_css_custom_properties(true); + if (unresolved.includes_attr_function()) + element.element().set_style_uses_attr_css_function(); + if (unresolved.includes_var_function()) + element.element().set_style_uses_var_css_function(); // To replace substitution functions in a property prop: auto const& property_name = property.visit( diff --git a/Libraries/LibWeb/CSS/StyleComputer.cpp b/Libraries/LibWeb/CSS/StyleComputer.cpp index ad3cb3bf727..953d8628d2e 100644 --- a/Libraries/LibWeb/CSS/StyleComputer.cpp +++ b/Libraries/LibWeb/CSS/StyleComputer.cpp @@ -2454,17 +2454,17 @@ GC::Ref StyleComputer::create_document_style() const return style; } -GC::Ref StyleComputer::compute_style(DOM::Element& element, Optional pseudo_element) const +GC::Ref StyleComputer::compute_style(DOM::Element& element, Optional pseudo_element, Optional did_change_custom_properties) const { - return *compute_style_impl(element, move(pseudo_element), ComputeStyleMode::Normal); + return *compute_style_impl(element, move(pseudo_element), ComputeStyleMode::Normal, did_change_custom_properties); } -GC::Ptr StyleComputer::compute_pseudo_element_style_if_needed(DOM::Element& element, Optional pseudo_element) const +GC::Ptr StyleComputer::compute_pseudo_element_style_if_needed(DOM::Element& element, Optional pseudo_element, Optional did_change_custom_properties) const { - return compute_style_impl(element, move(pseudo_element), ComputeStyleMode::CreatePseudoElementStyleIfNeeded); + return compute_style_impl(element, move(pseudo_element), ComputeStyleMode::CreatePseudoElementStyleIfNeeded, did_change_custom_properties); } -GC::Ptr StyleComputer::compute_style_impl(DOM::Element& element, Optional pseudo_element, ComputeStyleMode mode) const +GC::Ptr StyleComputer::compute_style_impl(DOM::Element& element, Optional pseudo_element, ComputeStyleMode mode, Optional did_change_custom_properties) const { build_rule_cache_if_needed(); @@ -2488,6 +2488,9 @@ GC::Ptr StyleComputer::compute_style_impl(DOM::Element& elem PseudoClassBitmap attempted_pseudo_class_matches; auto matching_rule_set = build_matching_rule_set(element, pseudo_element, attempted_pseudo_class_matches, did_match_any_pseudo_element_rules, mode); + DOM::AbstractElement abstract_element { element, pseudo_element }; + auto old_custom_properties = abstract_element.custom_properties(); + // Resolve all the CSS custom properties ("variables") for this element: // FIXME: Also resolve !important custom properties, in a second cascade. if (!pseudo_element.has_value() || pseudo_element_supports_property(*pseudo_element, PropertyID::Custom)) { @@ -2533,6 +2536,11 @@ GC::Ptr StyleComputer::compute_style_impl(DOM::Element& elem auto computed_properties = compute_properties(element, pseudo_element, cascaded_properties); computed_properties->set_attempted_pseudo_class_matches(attempted_pseudo_class_matches); + + if (did_change_custom_properties.has_value() && abstract_element.custom_properties() != old_custom_properties) { + *did_change_custom_properties = true; + } + return computed_properties; } diff --git a/Libraries/LibWeb/CSS/StyleComputer.h b/Libraries/LibWeb/CSS/StyleComputer.h index ed098b197e5..f0110a85806 100644 --- a/Libraries/LibWeb/CSS/StyleComputer.h +++ b/Libraries/LibWeb/CSS/StyleComputer.h @@ -145,8 +145,8 @@ public: [[nodiscard]] GC::Ref create_document_style() const; - [[nodiscard]] GC::Ref compute_style(DOM::Element&, Optional = {}) const; - [[nodiscard]] GC::Ptr compute_pseudo_element_style_if_needed(DOM::Element&, Optional) const; + [[nodiscard]] GC::Ref compute_style(DOM::Element&, Optional = {}, Optional did_change_custom_properties = {}) const; + [[nodiscard]] GC::Ptr compute_pseudo_element_style_if_needed(DOM::Element&, Optional, Optional did_change_custom_properties) const; [[nodiscard]] RuleCache const& get_pseudo_class_rule_cache(PseudoClass) const; @@ -217,7 +217,7 @@ private: [[nodiscard]] MatchingRuleSet build_matching_rule_set(DOM::Element const&, Optional, PseudoClassBitmap& attempted_pseudo_class_matches, bool& did_match_any_pseudo_element_rules, ComputeStyleMode) const; LogicalAliasMappingContext compute_logical_alias_mapping_context(DOM::Element&, Optional, ComputeStyleMode, MatchingRuleSet const&) const; - [[nodiscard]] GC::Ptr compute_style_impl(DOM::Element&, Optional, ComputeStyleMode) const; + [[nodiscard]] GC::Ptr compute_style_impl(DOM::Element&, Optional, ComputeStyleMode, Optional did_change_custom_properties) const; [[nodiscard]] GC::Ref compute_cascaded_values(DOM::Element&, Optional, bool did_match_any_pseudo_element_rules, ComputeStyleMode, MatchingRuleSet const&, Optional, ReadonlySpan properties_to_cascade) const; static RefPtr find_matching_font_weight_ascending(Vector const& candidates, int target_weight, float font_size_in_pt, bool inclusive); static RefPtr find_matching_font_weight_descending(Vector const& candidates, int target_weight, float font_size_in_pt, bool inclusive); diff --git a/Libraries/LibWeb/CSS/StyleProperty.cpp b/Libraries/LibWeb/CSS/StyleProperty.cpp index 334376c9c0b..03573b33919 100644 --- a/Libraries/LibWeb/CSS/StyleProperty.cpp +++ b/Libraries/LibWeb/CSS/StyleProperty.cpp @@ -11,4 +11,11 @@ namespace Web::CSS { StyleProperty::~StyleProperty() = default; +bool StyleProperty::operator==(StyleProperty const& other) const +{ + if (important != other.important || property_id != other.property_id || custom_name != other.custom_name) + return false; + return value->equals(*other.value); +} + } diff --git a/Libraries/LibWeb/CSS/StyleProperty.h b/Libraries/LibWeb/CSS/StyleProperty.h index 12964bb75ff..ba880781017 100644 --- a/Libraries/LibWeb/CSS/StyleProperty.h +++ b/Libraries/LibWeb/CSS/StyleProperty.h @@ -23,6 +23,8 @@ struct StyleProperty { CSS::PropertyID property_id; NonnullRefPtr value; FlyString custom_name {}; + + bool operator==(StyleProperty const& other) const; }; } diff --git a/Libraries/LibWeb/CSS/StyleValues/UnresolvedStyleValue.h b/Libraries/LibWeb/CSS/StyleValues/UnresolvedStyleValue.h index f69bf3730e9..9d88b5fbdce 100644 --- a/Libraries/LibWeb/CSS/StyleValues/UnresolvedStyleValue.h +++ b/Libraries/LibWeb/CSS/StyleValues/UnresolvedStyleValue.h @@ -25,6 +25,8 @@ public: Vector const& values() const { return m_values; } bool contains_arbitrary_substitution_function() const { return m_substitution_functions_presence.has_any(); } + bool includes_attr_function() const { return m_substitution_functions_presence.attr; } + bool includes_var_function() const { return m_substitution_functions_presence.var; } virtual bool equals(CSSStyleValue const& other) const override; diff --git a/Libraries/LibWeb/DOM/Document.cpp b/Libraries/LibWeb/DOM/Document.cpp index 0f3ccb4a411..8a62b3dbaff 100644 --- a/Libraries/LibWeb/DOM/Document.cpp +++ b/Libraries/LibWeb/DOM/Document.cpp @@ -1427,7 +1427,7 @@ void Document::update_layout(UpdateLayoutReason reason) } } -[[nodiscard]] static CSS::RequiredInvalidationAfterStyleChange update_style_recursively(Node& node, CSS::StyleComputer& style_computer, bool needs_inherited_style_update) +[[nodiscard]] static CSS::RequiredInvalidationAfterStyleChange update_style_recursively(Node& node, CSS::StyleComputer& style_computer, bool needs_inherited_style_update, bool recompute_elements_depending_on_custom_properties) { bool const needs_full_style_update = node.document().needs_full_style_update(); CSS::RequiredInvalidationAfterStyleChange invalidation; @@ -1440,12 +1440,14 @@ void Document::update_layout(UpdateLayoutReason reason) // We will still recompute style for the children, though. bool is_display_none = false; + bool did_change_custom_properties = false; CSS::RequiredInvalidationAfterStyleChange node_invalidation; if (is(node)) { - if (needs_full_style_update || node.needs_style_update()) { - node_invalidation = static_cast(node).recompute_style(); + auto& element = static_cast(node); + if (needs_full_style_update || node.needs_style_update() || (recompute_elements_depending_on_custom_properties && element.style_uses_var_css_function())) { + node_invalidation = element.recompute_style(did_change_custom_properties); } else if (needs_inherited_style_update) { - node_invalidation = static_cast(node).recompute_inherited_style(); + node_invalidation = element.recompute_inherited_style(); } is_display_none = static_cast(node).computed_properties()->display().is_none(); } @@ -1464,12 +1466,16 @@ void Document::update_layout(UpdateLayoutReason reason) node.set_needs_style_update(false); invalidation |= node_invalidation; + if (did_change_custom_properties) { + recompute_elements_depending_on_custom_properties = true; + } + bool children_need_inherited_style_update = !invalidation.is_none(); - if (needs_full_style_update || node.child_needs_style_update() || children_need_inherited_style_update) { + if (needs_full_style_update || node.child_needs_style_update() || children_need_inherited_style_update || recompute_elements_depending_on_custom_properties) { if (node.is_element()) { if (auto shadow_root = static_cast(node).shadow_root()) { if (needs_full_style_update || shadow_root->needs_style_update() || shadow_root->child_needs_style_update()) { - auto subtree_invalidation = update_style_recursively(*shadow_root, style_computer, children_need_inherited_style_update); + auto subtree_invalidation = update_style_recursively(*shadow_root, style_computer, children_need_inherited_style_update, recompute_elements_depending_on_custom_properties); if (!is_display_none) invalidation |= subtree_invalidation; } @@ -1477,8 +1483,8 @@ void Document::update_layout(UpdateLayoutReason reason) } node.for_each_child([&](auto& child) { - if (needs_full_style_update || child.needs_style_update() || children_need_inherited_style_update || child.child_needs_style_update()) { - auto subtree_invalidation = update_style_recursively(child, style_computer, children_need_inherited_style_update); + if (needs_full_style_update || child.needs_style_update() || children_need_inherited_style_update || child.child_needs_style_update() || recompute_elements_depending_on_custom_properties) { + auto subtree_invalidation = update_style_recursively(child, style_computer, children_need_inherited_style_update, recompute_elements_depending_on_custom_properties); if (!is_display_none) invalidation |= subtree_invalidation; } @@ -1523,7 +1529,7 @@ void Document::update_style() style_computer().reset_ancestor_filter(); - auto invalidation = update_style_recursively(*this, style_computer(), false); + auto invalidation = update_style_recursively(*this, style_computer(), false, false); if (!invalidation.is_none()) invalidate_display_list(); if (invalidation.rebuild_stacking_context_tree) diff --git a/Libraries/LibWeb/DOM/Element.cpp b/Libraries/LibWeb/DOM/Element.cpp index 06672b16da3..50df89c7aa6 100644 --- a/Libraries/LibWeb/DOM/Element.cpp +++ b/Libraries/LibWeb/DOM/Element.cpp @@ -683,10 +683,12 @@ static CSS::RequiredInvalidationAfterStyleChange compute_required_invalidation(C return invalidation; } -CSS::RequiredInvalidationAfterStyleChange Element::recompute_style() +CSS::RequiredInvalidationAfterStyleChange Element::recompute_style(bool& did_change_custom_properties) { VERIFY(parent()); + m_style_uses_attr_css_function = false; + m_style_uses_var_css_function = false; m_affected_by_has_pseudo_class_in_subject_position = false; m_affected_by_has_pseudo_class_in_non_subject_position = false; m_affected_by_has_pseudo_class_with_relative_selector_that_has_sibling_combinator = false; @@ -697,7 +699,7 @@ CSS::RequiredInvalidationAfterStyleChange Element::recompute_style() m_sibling_invalidation_distance = 0; auto& style_computer = document().style_computer(); - auto new_computed_properties = style_computer.compute_style(*this); + auto new_computed_properties = style_computer.compute_style(*this, {}, did_change_custom_properties); // Tables must not inherit -libweb-* values for text-align. // FIXME: Find the spec for this. @@ -737,7 +739,7 @@ CSS::RequiredInvalidationAfterStyleChange Element::recompute_style() style_computer.push_ancestor(*this); auto pseudo_element_style = computed_properties(pseudo_element); - auto new_pseudo_element_style = style_computer.compute_pseudo_element_style_if_needed(*this, pseudo_element); + auto new_pseudo_element_style = style_computer.compute_pseudo_element_style_if_needed(*this, pseudo_element, did_change_custom_properties); // TODO: Can we be smarter about invalidation? if (pseudo_element_style && new_pseudo_element_style) { @@ -2472,24 +2474,13 @@ void Element::invalidate_style_after_attribute_change(FlyString const& attribute { Vector changed_properties; StyleInvalidationOptions style_invalidation_options; - if (is_presentational_hint(attribute_name)) { + if (is_presentational_hint(attribute_name) || style_uses_attr_css_function()) { style_invalidation_options.invalidate_self = true; } - if (style_uses_css_custom_properties()) { - // A css custom property can be hooked on to this element by any attribute - // so invalidate elements and rerender them in that scenario - style_invalidation_options.invalidate_elements_that_use_css_custom_properties = true; - } - if (attribute_name == HTML::AttributeNames::style) { style_invalidation_options.invalidate_self = true; - // even if we don't have custom properties, the new "style" attribute could add one - style_invalidation_options.invalidate_elements_that_use_css_custom_properties = true; } else if (attribute_name == HTML::AttributeNames::class_) { - // adding or removing classes can add new custom properties to this element - style_invalidation_options.invalidate_elements_that_use_css_custom_properties = true; - Vector old_classes; Vector new_classes; if (old_value.has_value()) diff --git a/Libraries/LibWeb/DOM/Element.h b/Libraries/LibWeb/DOM/Element.h index da24c609bc6..abf3a63da6f 100644 --- a/Libraries/LibWeb/DOM/Element.h +++ b/Libraries/LibWeb/DOM/Element.h @@ -201,7 +201,7 @@ public: void run_attribute_change_steps(FlyString const& local_name, Optional const& old_value, Optional const& value, Optional const& namespace_); - CSS::RequiredInvalidationAfterStyleChange recompute_style(); + CSS::RequiredInvalidationAfterStyleChange recompute_style(bool& did_change_custom_properties); CSS::RequiredInvalidationAfterStyleChange recompute_inherited_style(); Optional use_pseudo_element() const { return m_use_pseudo_element; } @@ -256,8 +256,10 @@ public: void set_custom_properties(Optional, HashMap custom_properties); [[nodiscard]] HashMap const& custom_properties(Optional) const; - bool style_uses_css_custom_properties() const { return m_style_uses_css_custom_properties; } - void set_style_uses_css_custom_properties(bool value) { m_style_uses_css_custom_properties = value; } + bool style_uses_attr_css_function() const { return m_style_uses_attr_css_function; } + void set_style_uses_attr_css_function() { m_style_uses_attr_css_function = true; } + bool style_uses_var_css_function() const { return m_style_uses_var_css_function; } + void set_style_uses_var_css_function() { m_style_uses_var_css_function = true; } // NOTE: The function is wrapped in a GC::HeapFunction immediately. HTML::TaskID queue_an_element_task(HTML::Task::Source, Function); @@ -605,7 +607,8 @@ private: bool m_in_top_layer : 1 { false }; bool m_rendered_in_top_layer : 1 { false }; - bool m_style_uses_css_custom_properties : 1 { false }; + bool m_style_uses_attr_css_function : 1 { false }; + bool m_style_uses_var_css_function : 1 { false }; bool m_affected_by_has_pseudo_class_in_subject_position : 1 { false }; bool m_affected_by_has_pseudo_class_in_non_subject_position : 1 { false }; bool m_affected_by_direct_sibling_combinator : 1 { false }; diff --git a/Libraries/LibWeb/DOM/Node.cpp b/Libraries/LibWeb/DOM/Node.cpp index 241dc014018..77cbdcc98a0 100644 --- a/Libraries/LibWeb/DOM/Node.cpp +++ b/Libraries/LibWeb/DOM/Node.cpp @@ -515,11 +515,11 @@ void Node::invalidate_style(StyleInvalidationReason reason, Vector const&, StyleInvalidationOptions); diff --git a/Libraries/LibWeb/DOM/StyleInvalidator.cpp b/Libraries/LibWeb/DOM/StyleInvalidator.cpp index 180cf17cba5..fde3a354940 100644 --- a/Libraries/LibWeb/DOM/StyleInvalidator.cpp +++ b/Libraries/LibWeb/DOM/StyleInvalidator.cpp @@ -26,15 +26,12 @@ void StyleInvalidator::invalidate(Node& node) m_pending_invalidations.clear(); } -void StyleInvalidator::add_pending_invalidation(GC::Ref node, CSS::InvalidationSet&& invalidation_set, bool invalidate_elements_that_use_css_custom_properties) +void StyleInvalidator::add_pending_invalidation(GC::Ref node, CSS::InvalidationSet&& invalidation_set) { - auto& pending_invalidation = m_pending_invalidations.ensure(node, [&] { - return PendingInvalidation {}; + auto& pending_invalidation_set = m_pending_invalidations.ensure(node, [&] { + return CSS::InvalidationSet {}; }); - pending_invalidation.invalidation_set.include_all_from(invalidation_set); - if (invalidate_elements_that_use_css_custom_properties) { - pending_invalidation.invalidate_elements_that_use_css_custom_properties = true; - } + pending_invalidation_set.include_all_from(invalidation_set); } // This function makes a full pass over the entire DOM and: @@ -51,24 +48,17 @@ void StyleInvalidator::perform_pending_style_invalidations(Node& node, bool inva } auto previous_subtree_invalidations_sets_size = m_subtree_invalidation_sets.size(); - auto previous_invalidate_elements_that_use_css_custom_properties = m_invalidate_elements_that_use_css_custom_properties; - ScopeGuard restore_state = [this, previous_subtree_invalidations_sets_size, previous_invalidate_elements_that_use_css_custom_properties] { + ScopeGuard restore_state = [this, previous_subtree_invalidations_sets_size] { m_subtree_invalidation_sets.shrink(previous_subtree_invalidations_sets_size); - m_invalidate_elements_that_use_css_custom_properties = previous_invalidate_elements_that_use_css_custom_properties; }; if (!invalidate_entire_subtree) { - auto pending_invalidation = m_pending_invalidations.get(node); - if (pending_invalidation.has_value()) { - m_subtree_invalidation_sets.append(pending_invalidation->invalidation_set); - m_invalidate_elements_that_use_css_custom_properties = m_invalidate_elements_that_use_css_custom_properties || pending_invalidation->invalidate_elements_that_use_css_custom_properties; + auto invalidation_set = m_pending_invalidations.get(node); + if (invalidation_set.has_value()) { + m_subtree_invalidation_sets.append(*invalidation_set); } auto affected_by_invalidation_sets_or_invalidation_flags = [this](Element const& element) { - if (m_invalidate_elements_that_use_css_custom_properties && element.style_uses_css_custom_properties()) { - return true; - } - for (auto& invalidation_set : m_subtree_invalidation_sets) { if (element.includes_properties_from_invalidation_set(invalidation_set)) return true; diff --git a/Libraries/LibWeb/DOM/StyleInvalidator.h b/Libraries/LibWeb/DOM/StyleInvalidator.h index 0f40494b39f..f85bcc4fccd 100644 --- a/Libraries/LibWeb/DOM/StyleInvalidator.h +++ b/Libraries/LibWeb/DOM/StyleInvalidator.h @@ -18,7 +18,7 @@ class StyleInvalidator : public GC::Cell { public: void invalidate(Node& node); - void add_pending_invalidation(GC::Ref, CSS::InvalidationSet&&, bool invalidate_elements_that_use_css_custom_properties); + void add_pending_invalidation(GC::Ref, CSS::InvalidationSet&&); bool has_pending_invalidations() const { return !m_pending_invalidations.is_empty(); } virtual void visit_edges(Cell::Visitor& visitor) override; @@ -26,14 +26,8 @@ public: private: void perform_pending_style_invalidations(Node& node, bool invalidate_entire_subtree); - struct PendingInvalidation { - bool invalidate_elements_that_use_css_custom_properties { false }; - CSS::InvalidationSet invalidation_set; - }; - HashMap, PendingInvalidation> m_pending_invalidations; - + HashMap, CSS::InvalidationSet> m_pending_invalidations; Vector m_subtree_invalidation_sets; - bool m_invalidate_elements_that_use_css_custom_properties { false }; }; }