From d79bb1aac2e2cf8be39525b1aceedc30aca28c62 Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Mon, 27 Jan 2025 21:19:30 +0100 Subject: [PATCH] LibWeb: Fix underinvalidation when inline style has custom properties We have an optimization that allows us to invalidate only the style of the element itself and mark descendants for inherited properties update when the "style" attribute changes (unless there are any CSS rules that use the "style" attribute, then we also invalidate all descendants that might be affected by those rules). This optimization was not taking into account that when the inline style has custom properties, we also need to invalidate all descendants whose style might be affected by them. This change fixes this bug by saving a flag in Element that indicates whether its style depends on any custom properties and then invalidating all descendants with this flag set when the "style" attribute changes. Unlike font relative lengths invalidation, for elements that depend on custom properties, we need to actually recompute the style, instead of individual properties, because values without expanded custom properties are gone after cascading, and it has to be done again. The test added for this change is a version of an existing test we had restructured such that it doesn't trigger aggressive style invalidation caused by DOM structured changes until the last moment when test results are printed. --- Libraries/LibWeb/CSS/Parser/Parser.cpp | 8 ++++++ Libraries/LibWeb/DOM/Element.cpp | 9 ++++--- Libraries/LibWeb/DOM/Element.h | 4 +++ Libraries/LibWeb/DOM/Node.cpp | 13 +++++++--- Libraries/LibWeb/DOM/Node.h | 10 +++---- ...SSStyleDeclaration-custom-properties-2.txt | 4 +++ ...SStyleDeclaration-custom-properties-2.html | 26 +++++++++++++++++++ 7 files changed, 62 insertions(+), 12 deletions(-) create mode 100644 Tests/LibWeb/Text/expected/css/CSSStyleDeclaration-custom-properties-2.txt create mode 100644 Tests/LibWeb/Text/input/css/CSSStyleDeclaration-custom-properties-2.html diff --git a/Libraries/LibWeb/CSS/Parser/Parser.cpp b/Libraries/LibWeb/CSS/Parser/Parser.cpp index 5a0f9014081..fa2d5769f00 100644 --- a/Libraries/LibWeb/CSS/Parser/Parser.cpp +++ b/Libraries/LibWeb/CSS/Parser/Parser.cpp @@ -9569,6 +9569,14 @@ NonnullRefPtr Parser::resolve_unresolved_style_value(DOM::Element Vector values_with_variables_expanded; HashMap> dependencies; + ScopeGuard mark_element_if_uses_custom_properties = [&] { + for (auto const& name : dependencies.keys()) { + if (is_a_custom_property_name_string(name)) { + element.set_style_uses_css_custom_properties(true); + return; + } + } + }; if (!expand_variables(element, pseudo_element, string_from_property_id(property_id), dependencies, unresolved_values_without_variables_expanded, values_with_variables_expanded)) return CSSKeywordValue::create(Keyword::Unset); diff --git a/Libraries/LibWeb/DOM/Element.cpp b/Libraries/LibWeb/DOM/Element.cpp index c2eeff851e7..3a21331c0de 100644 --- a/Libraries/LibWeb/DOM/Element.cpp +++ b/Libraries/LibWeb/DOM/Element.cpp @@ -1986,13 +1986,14 @@ ErrorOr Element::scroll_into_view(Optional const& old_value, Optional const& new_value) { Vector changed_properties; - ForceSelfStyleInvalidation force_self_invalidation = ForceSelfStyleInvalidation::No; + StyleInvalidationOptions style_invalidation_options; if (is_presentational_hint(attribute_name)) { - force_self_invalidation = ForceSelfStyleInvalidation::Yes; + style_invalidation_options.invalidate_self = true; } if (attribute_name == HTML::AttributeNames::style) { - force_self_invalidation = ForceSelfStyleInvalidation::Yes; + style_invalidation_options.invalidate_self = true; + style_invalidation_options.invalidate_elements_that_use_css_custom_properties = true; } else if (attribute_name == HTML::AttributeNames::class_) { Vector old_classes; Vector new_classes; @@ -2025,7 +2026,7 @@ void Element::invalidate_style_after_attribute_change(FlyString const& attribute } changed_properties.append({ .type = CSS::InvalidationSet::Property::Type::Attribute, .value = attribute_name }); - invalidate_style(StyleInvalidationReason::ElementAttributeChange, changed_properties, force_self_invalidation); + invalidate_style(StyleInvalidationReason::ElementAttributeChange, changed_properties, style_invalidation_options); } bool Element::is_hidden() const diff --git a/Libraries/LibWeb/DOM/Element.h b/Libraries/LibWeb/DOM/Element.h index 58c91cf9fba..3c785a79924 100644 --- a/Libraries/LibWeb/DOM/Element.h +++ b/Libraries/LibWeb/DOM/Element.h @@ -245,6 +245,9 @@ 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; } + // NOTE: The function is wrapped in a GC::HeapFunction immediately. HTML::TaskID queue_an_element_task(HTML::Task::Source, Function); @@ -496,6 +499,7 @@ private: Array m_scroll_offset; bool m_in_top_layer { false }; + bool m_style_uses_css_custom_properties { false }; OwnPtr m_counters_set; diff --git a/Libraries/LibWeb/DOM/Node.cpp b/Libraries/LibWeb/DOM/Node.cpp index 70bc47c0fce..4a20da50d41 100644 --- a/Libraries/LibWeb/DOM/Node.cpp +++ b/Libraries/LibWeb/DOM/Node.cpp @@ -462,7 +462,7 @@ void Node::invalidate_style(StyleInvalidationReason reason) document().schedule_style_update(); } -void Node::invalidate_style(StyleInvalidationReason reason, Vector const& properties, ForceSelfStyleInvalidation force_self_invalidation) +void Node::invalidate_style(StyleInvalidationReason reason, Vector const& properties, StyleInvalidationOptions options) { if (is_character_data()) return; @@ -477,7 +477,7 @@ void Node::invalidate_style(StyleInvalidationReason reason, Vector(node); - bool needs_style_recalculation = invalidation_set.needs_invalidate_whole_subtree() || element_has_properties_from_invalidation_set(element); + bool needs_style_recalculation = false; + if (invalidation_set.needs_invalidate_whole_subtree()) { + needs_style_recalculation = true; + } else if (element_has_properties_from_invalidation_set(element)) { + needs_style_recalculation = true; + } else if (options.invalidate_elements_that_use_css_custom_properties && element.style_uses_css_custom_properties()) { + needs_style_recalculation = true; + } if (needs_style_recalculation) { element.set_needs_style_update(true); } else { diff --git a/Libraries/LibWeb/DOM/Node.h b/Libraries/LibWeb/DOM/Node.h index 9ca7c641ba5..16213836dd8 100644 --- a/Libraries/LibWeb/DOM/Node.h +++ b/Libraries/LibWeb/DOM/Node.h @@ -301,12 +301,12 @@ public: [[nodiscard]] bool entire_subtree_needs_style_update() const { return m_entire_subtree_needs_style_update; } void set_entire_subtree_needs_style_update(bool b) { m_entire_subtree_needs_style_update = b; } - enum class ForceSelfStyleInvalidation : bool { - Yes, - No - }; void invalidate_style(StyleInvalidationReason); - void invalidate_style(StyleInvalidationReason, Vector const&, ForceSelfStyleInvalidation = ForceSelfStyleInvalidation::No); + struct StyleInvalidationOptions { + bool invalidate_self { false }; + bool invalidate_elements_that_use_css_custom_properties { false }; + }; + void invalidate_style(StyleInvalidationReason, Vector const&, StyleInvalidationOptions); void set_document(Badge, Document&); diff --git a/Tests/LibWeb/Text/expected/css/CSSStyleDeclaration-custom-properties-2.txt b/Tests/LibWeb/Text/expected/css/CSSStyleDeclaration-custom-properties-2.txt new file mode 100644 index 00000000000..2c1e1baa910 --- /dev/null +++ b/Tests/LibWeb/Text/expected/css/CSSStyleDeclaration-custom-properties-2.txt @@ -0,0 +1,4 @@ +style.getPropertyValue(--redcolor)=red +style.getPropertyPriority(--redcolor)= +rgb(255, 0, 0) +rgba(0, 0, 0, 0) diff --git a/Tests/LibWeb/Text/input/css/CSSStyleDeclaration-custom-properties-2.html b/Tests/LibWeb/Text/input/css/CSSStyleDeclaration-custom-properties-2.html new file mode 100644 index 00000000000..0f36bca346e --- /dev/null +++ b/Tests/LibWeb/Text/input/css/CSSStyleDeclaration-custom-properties-2.html @@ -0,0 +1,26 @@ + + + +