From d5a82040e3b11402da8f74adbfffb5ace659c5db Mon Sep 17 00:00:00 2001
From: Aliaksandr Kalenik <kalenik.aliaksandr@gmail.com>
Date: Fri, 24 Jan 2025 22:48:45 +0100
Subject: [PATCH] LibWeb: Fix underinvalidation of :nth-child using
 invalidation sets

For all invalidation properties nested into nth-child argument list we
need to invalidate whole subtree to make sure style of sibling elements
will be recalculated.
---
 .../LibWeb/CSS/StyleInvalidationData.cpp      | 39 ++++++++++++++-----
 Libraries/LibWeb/CSS/StyleInvalidationData.h  |  2 +-
 ...-child-of-ids-without-empty-text-node.html | 30 ++++++++++++++
 3 files changed, 61 insertions(+), 10 deletions(-)
 create mode 100644 Tests/LibWeb/Ref/input/wpt-import/css/selectors/invalidation/nth-child-of-ids-without-empty-text-node.html

diff --git a/Libraries/LibWeb/CSS/StyleInvalidationData.cpp b/Libraries/LibWeb/CSS/StyleInvalidationData.cpp
index ec3e3f0911a..9a5326d32dd 100644
--- a/Libraries/LibWeb/CSS/StyleInvalidationData.cpp
+++ b/Libraries/LibWeb/CSS/StyleInvalidationData.cpp
@@ -95,7 +95,14 @@ enum class ExcludePropertiesNestedInNotPseudoClass : bool {
     Yes,
 };
 
-static void build_invalidation_sets_for_simple_selector(Selector::SimpleSelector const& selector, InvalidationSet& invalidation_set, ExcludePropertiesNestedInNotPseudoClass exclude_properties_nested_in_not_pseudo_class, StyleInvalidationData& rule_invalidation_data)
+enum class InsideNthChildPseudoClass {
+    No,
+    Yes
+};
+
+static InvalidationSet build_invalidation_sets_for_selector_impl(StyleInvalidationData& style_invalidation_data, Selector const& selector, InsideNthChildPseudoClass inside_nth_child_pseudo_class);
+
+static void build_invalidation_sets_for_simple_selector(Selector::SimpleSelector const& selector, InvalidationSet& invalidation_set, ExcludePropertiesNestedInNotPseudoClass exclude_properties_nested_in_not_pseudo_class, StyleInvalidationData& style_invalidation_data, InsideNthChildPseudoClass inside_nth_child_selector)
 {
     switch (selector.type) {
     case Selector::SimpleSelector::Type::Class:
@@ -126,8 +133,12 @@ static void build_invalidation_sets_for_simple_selector(Selector::SimpleSelector
             break;
         if (exclude_properties_nested_in_not_pseudo_class == ExcludePropertiesNestedInNotPseudoClass::Yes && pseudo_class.type == PseudoClass::Not)
             break;
+        InsideNthChildPseudoClass inside_nth_child_pseudo_class_for_nested = inside_nth_child_selector;
+        if (AK::first_is_one_of(pseudo_class.type, PseudoClass::NthChild, PseudoClass::NthLastChild, PseudoClass::NthOfType, PseudoClass::NthLastOfType)) {
+            inside_nth_child_pseudo_class_for_nested = InsideNthChildPseudoClass::Yes;
+        }
         for (auto const& nested_selector : pseudo_class.argument_selector_list) {
-            auto rightmost_invalidation_set_for_selector = rule_invalidation_data.build_invalidation_sets_for_selector(*nested_selector);
+            auto rightmost_invalidation_set_for_selector = build_invalidation_sets_for_selector_impl(style_invalidation_data, *nested_selector, inside_nth_child_pseudo_class_for_nested);
             invalidation_set.include_all_from(rightmost_invalidation_set_for_selector);
         }
         break;
@@ -137,7 +148,7 @@ static void build_invalidation_sets_for_simple_selector(Selector::SimpleSelector
     }
 }
 
-InvalidationSet StyleInvalidationData::build_invalidation_sets_for_selector(Selector const& selector)
+static InvalidationSet build_invalidation_sets_for_selector_impl(StyleInvalidationData& style_invalidation_data, Selector const& selector, InsideNthChildPseudoClass inside_nth_child_pseudo_class)
 {
     auto const& compound_selectors = selector.compound_selectors();
     int compound_selector_index = compound_selectors.size() - 1;
@@ -155,7 +166,7 @@ InvalidationSet StyleInvalidationData::build_invalidation_sets_for_selector(Sele
                 if (pseudo_class.type == PseudoClass::Has)
                     in_has = true;
             }
-            collect_properties_used_in_has(simple_selector, *this, in_has);
+            collect_properties_used_in_has(simple_selector, style_invalidation_data, in_has);
         }
 
         if (is_rightmost) {
@@ -169,23 +180,28 @@ InvalidationSet StyleInvalidationData::build_invalidation_sets_for_selector(Sele
             //                      which means invalidation_set_for_rightmost_selector should be empty
             for (auto const& simple_selector : simple_selectors) {
                 InvalidationSet s;
-                build_invalidation_sets_for_simple_selector(simple_selector, s, ExcludePropertiesNestedInNotPseudoClass::No, *this);
+                build_invalidation_sets_for_simple_selector(simple_selector, s, ExcludePropertiesNestedInNotPseudoClass::No, style_invalidation_data, inside_nth_child_pseudo_class);
                 s.for_each_property([&](auto const& invalidation_property) {
-                    auto& descendant_invalidation_set = descendant_invalidation_sets.ensure(invalidation_property, [] { return InvalidationSet {}; });
+                    auto& descendant_invalidation_set = style_invalidation_data.descendant_invalidation_sets.ensure(invalidation_property, [] { return InvalidationSet {}; });
                     descendant_invalidation_set.set_needs_invalidate_self();
+                    if (inside_nth_child_pseudo_class == InsideNthChildPseudoClass::Yes) {
+                        // When invalidation property is nested in nth-child selector like p:nth-child(even of #t1, #t2, #t3)
+                        // we need to make all siblings are invalidated.
+                        descendant_invalidation_set.set_needs_invalidate_whole_subtree();
+                    }
                 });
             }
 
             for (auto const& simple_selector : simple_selectors) {
-                build_invalidation_sets_for_simple_selector(simple_selector, invalidation_set_for_rightmost_selector, ExcludePropertiesNestedInNotPseudoClass::Yes, *this);
+                build_invalidation_sets_for_simple_selector(simple_selector, invalidation_set_for_rightmost_selector, ExcludePropertiesNestedInNotPseudoClass::Yes, style_invalidation_data, inside_nth_child_pseudo_class);
             }
         } else {
             VERIFY(previous_compound_combinator != Selector::Combinator::None);
             for (auto const& simple_selector : simple_selectors) {
                 InvalidationSet s;
-                build_invalidation_sets_for_simple_selector(simple_selector, s, ExcludePropertiesNestedInNotPseudoClass::No, *this);
+                build_invalidation_sets_for_simple_selector(simple_selector, s, ExcludePropertiesNestedInNotPseudoClass::No, style_invalidation_data, inside_nth_child_pseudo_class);
                 s.for_each_property([&](auto const& invalidation_property) {
-                    auto& descendant_invalidation_set = descendant_invalidation_sets.ensure(invalidation_property, [] {
+                    auto& descendant_invalidation_set = style_invalidation_data.descendant_invalidation_sets.ensure(invalidation_property, [] {
                         return InvalidationSet {};
                     });
                     // If the rightmost selector's invalidation set is empty, it means there's no
@@ -209,4 +225,9 @@ InvalidationSet StyleInvalidationData::build_invalidation_sets_for_selector(Sele
     return invalidation_set_for_rightmost_selector;
 }
 
+void StyleInvalidationData::build_invalidation_sets_for_selector(Selector const& selector)
+{
+    (void)build_invalidation_sets_for_selector_impl(*this, selector, InsideNthChildPseudoClass::No);
+}
+
 }
diff --git a/Libraries/LibWeb/CSS/StyleInvalidationData.h b/Libraries/LibWeb/CSS/StyleInvalidationData.h
index 848711230cb..6c478edc7cd 100644
--- a/Libraries/LibWeb/CSS/StyleInvalidationData.h
+++ b/Libraries/LibWeb/CSS/StyleInvalidationData.h
@@ -20,7 +20,7 @@ struct StyleInvalidationData {
     HashTable<FlyString> tag_names_used_in_has_selectors;
     HashTable<PseudoClass> pseudo_classes_used_in_has_selectors;
 
-    InvalidationSet build_invalidation_sets_for_selector(Selector const& selector);
+    void build_invalidation_sets_for_selector(Selector const& selector);
 };
 
 }
diff --git a/Tests/LibWeb/Ref/input/wpt-import/css/selectors/invalidation/nth-child-of-ids-without-empty-text-node.html b/Tests/LibWeb/Ref/input/wpt-import/css/selectors/invalidation/nth-child-of-ids-without-empty-text-node.html
new file mode 100644
index 00000000000..b148b9a1977
--- /dev/null
+++ b/Tests/LibWeb/Ref/input/wpt-import/css/selectors/invalidation/nth-child-of-ids-without-empty-text-node.html
@@ -0,0 +1,30 @@
+<!DOCTYPE html>
+<meta charset="utf-8" />
+<title>CSS Selectors Invalidation: :nth-child(... of IDs)</title>
+<link rel="author" title="Zach Hoffman" href="mailto:zach@zrhoffman.net">
+<link rel="match" href="../../../../../expected/wpt-import/css/selectors/invalidation/nth-child-of-class-ref.html">
+<link rel="help" href="https://drafts.csswg.org/selectors-4/#child-index">
+<style>
+  p:nth-child(even of #t1, #t2, #t3, #t4, #t5) {
+    color: green;
+  }
+</style>
+<!--
+This test is modified version of nth-child-of-ids.html that does not contain empty text node
+between <div> and <script> elements. This is important, because insertion of this empty text node
+causes style invalidation that does not use invalidation sets (at the time of writing this comment),
+so we cannot check if modification of id on #t2 correctly invalidates style of sibling <p> elements.
+-->
+<div>
+  <p>Ignored</p>
+  <p>Ignored</p>
+  <p id="t1">Not ignored</p>
+  <p id="t2">Selectively ignored</p>
+  <p id="t3">Not ignored</p>
+  <p id="t4">Not ignored</p>
+  <p id="t5">Not ignored</p>
+  <p>Ignored</p>
+</div><script>
+  document.documentElement.offsetTop;
+  t2.id = "new-id";
+</script>
\ No newline at end of file