From 82b8c1dc6a37006f4d1f24a44c8cb45798d803bd Mon Sep 17 00:00:00 2001
From: David Shin <dshin@mozilla.com>
Date: Mon, 30 Oct 2023 14:19:09 +0000
Subject: [PATCH] Bug 1860373: Don't collapse invalidations from selectors
 shared by nesting. r=emilio

Differential Revision: https://phabricator.services.mozilla.com/D192085
---
 .../invalidation/element/relative_selector.rs |  58 ++++++----
 .../invalidation/has-css-nesting-shared.html  | 105 ++++++++++++++++++
 2 files changed, 140 insertions(+), 23 deletions(-)
 create mode 100644 testing/web-platform/tests/css/selectors/invalidation/has-css-nesting-shared.html

diff --git a/servo/components/style/invalidation/element/relative_selector.rs b/servo/components/style/invalidation/element/relative_selector.rs
index c3e9f1b6f397..fd4372058e88 100644
--- a/servo/components/style/invalidation/element/relative_selector.rs
+++ b/servo/components/style/invalidation/element/relative_selector.rs
@@ -270,11 +270,9 @@ struct RelativeSelectorInvalidation<'a> {
     dependency: &'a Dependency,
 }
 
-#[derive(Clone, Copy, Hash, Eq, PartialEq)]
-struct InvalidationKey(SelectorKey, DependencyInvalidationKind);
-
 type ElementDependencies<'a> = SmallVec<[(Option<OpaqueElement>, &'a Dependency); 1]>;
 type Dependencies<'a, E> = SmallVec<[(E, ElementDependencies<'a>); 1]>;
+type AlreadyInvalidated<'a, E> = SmallVec<[(E, Option<OpaqueElement>, &'a Dependency); 2]>;
 
 /// Interface for collecting relative selector dependencies.
 pub struct RelativeSelectorDependencyCollector<'a, E>
@@ -285,8 +283,7 @@ where
     /// a relative selector invalidation.
     dependencies: FxHashMap<E, ElementDependencies<'a>>,
     /// Dependencies that created an invalidation right away.
-    /// Maps an invalidation into the affected element, and its scope & dependency.
-    invalidations: FxHashMap<InvalidationKey, (E, Option<OpaqueElement>, &'a Dependency)>,
+    invalidations: AlreadyInvalidated<'a, E>,
     /// The top element in the subtree being invalidated.
     top: E,
     /// Optional context that will be used to try and skip invalidations
@@ -313,6 +310,25 @@ impl<'a, E: TElement + 'a> Default for ToInvalidate<'a, E> {
     }
 }
 
+fn dependency_selectors_match(a: &Dependency, b: &Dependency) -> bool {
+    if a.invalidation_kind() != b.invalidation_kind() {
+        return false;
+    }
+    if SelectorKey::new(&a.selector) != SelectorKey::new(&b.selector) {
+        return false;
+    }
+    let mut a_parent = a.parent.as_ref();
+    let mut b_parent = b.parent.as_ref();
+    while let (Some(a_p), Some(b_p)) = (a_parent, b_parent) {
+        if SelectorKey::new(&a_p.selector) != SelectorKey::new(&b_p.selector) {
+            return false;
+        }
+        a_parent = a_p.parent.as_ref();
+        b_parent = b_p.parent.as_ref();
+    }
+    a_parent.is_none() && b_parent.is_none()
+}
+
 impl<'a, E> RelativeSelectorDependencyCollector<'a, E>
 where
     E: TElement,
@@ -320,7 +336,7 @@ where
     fn new(top: E, optimization_context: Option<OptimizationContext<'a, E>>) -> Self {
         Self {
             dependencies: FxHashMap::default(),
-            invalidations: FxHashMap::default(),
+            invalidations: AlreadyInvalidated::default(),
             top,
             optimization_context,
         }
@@ -328,21 +344,21 @@ where
 
     fn insert_invalidation(
         &mut self,
-        key: InvalidationKey,
         element: E,
         dependency: &'a Dependency,
         host: Option<OpaqueElement>,
     ) {
-        self.invalidations
-            .entry(key)
-            .and_modify(|(e, h, d)| {
+        match self.invalidations.iter_mut().find(|(_, _, d)| dependency_selectors_match(dependency, d)) {
+            Some((e, h, d)) => {
                 // Just keep one.
-                if d.selector_offset <= dependency.selector_offset {
-                    return;
+                if d.selector_offset > dependency.selector_offset {
+                    (*e, *h, *d) = (element, host, dependency);
                 }
-                (*e, *h, *d) = (element, host, dependency);
-            })
-            .or_insert_with(|| (element, host, dependency));
+            },
+            None => {
+                self.invalidations.push((element, host, dependency));
+            }
+        }
     }
 
     /// Add this dependency, if it is unique (i.e. Different outer dependency or same outer dependency
@@ -377,10 +393,6 @@ where
                     return;
                 }
                 self.insert_invalidation(
-                    InvalidationKey(
-                        SelectorKey::new(&dependency.selector),
-                        dependency.invalidation_kind(),
-                    ),
                     element,
                     dependency,
                     host,
@@ -392,8 +404,8 @@ where
     /// Get the dependencies in a list format.
     fn get(self) -> ToInvalidate<'a, E> {
         let mut result = ToInvalidate::default();
-        for (key, (element, host, relative_dependency)) in self.invalidations {
-            match key.1 {
+        for (element, host, dependency) in self.invalidations {
+            match dependency.invalidation_kind() {
                 DependencyInvalidationKind::Normal(_) => {
                     unreachable!("Inner selector in invalidation?")
                 },
@@ -403,12 +415,12 @@ where
                             element != self.top,
                             element,
                             host,
-                            relative_dependency,
+                            dependency,
                         ) {
                             continue;
                         }
                     }
-                    let dependency = relative_dependency.parent.as_ref().unwrap();
+                    let dependency = dependency.parent.as_ref().unwrap();
                     result.invalidations.push(RelativeSelectorInvalidation {
                         kind,
                         host,
diff --git a/testing/web-platform/tests/css/selectors/invalidation/has-css-nesting-shared.html b/testing/web-platform/tests/css/selectors/invalidation/has-css-nesting-shared.html
new file mode 100644
index 000000000000..7ce0ac3b04c3
--- /dev/null
+++ b/testing/web-platform/tests/css/selectors/invalidation/has-css-nesting-shared.html
@@ -0,0 +1,105 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>:has() invalidation with nesting where the selector is shared</title>
+<link rel="author" title="David Shin" href="mailto:dshin@mozilla.com">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<link rel="help" href="https://drafts.csswg.org/selectors/#relational">
+<style>
+div, main { color: grey }
+#outer1:has(.test) {
+  & #subject1_1 {
+    color: red;
+  }
+  & + #subject1_2 {
+    color: orangered;
+  }
+}
+
+#outer2:has(.test) {
+  & .ancestor {
+    & #subject2_1 {
+      color: green;
+    }
+    & + #subject2_2 {
+      color: lightgreen;
+    }
+  }
+}
+
+#outer3:is(:has(.test) .outer) {
+  & #subject3_1 {
+    color: blue;
+  }
+  & + #subject3_2 {
+    color: skyblue;
+  }
+}
+</style>
+<main id="main">
+  <div>
+    <div id="outer1">
+      <div id="trigger1"></div>
+      <div id="subject1_1"></div>
+    </div>
+    <div id="subject1_2"></div>
+  </div>
+  <div id="outer2">
+    <div id="trigger2"></div>
+    <div class="ancestor">
+      <div id="subject2_1"></div>
+    </div>
+    <div id="subject2_2"></div>
+  </div>
+  <div id="trigger3">
+    <div id="outer3" class="outer">
+      <div id="subject3_1"></div>
+    </div>
+    <div id="subject3_2"></div>
+  </div>
+</main>
+<script>
+const grey = 'rgb(128, 128, 128)';
+const red = 'rgb(255, 0, 0)';
+const orangered = 'rgb(255, 69, 0)';
+const green = 'rgb(0, 128, 0)';
+const lightgreen = 'rgb(144, 238, 144)';
+const blue = 'rgb(0, 0, 255)';
+const skyblue = 'rgb(135, 206, 235)';
+
+const colors = {
+  red: {
+    descendant: red,
+    sibling: orangered,
+  },
+  green: {
+    descendant: green,
+    sibling: lightgreen,
+  },
+  blue: {
+    descendant: blue,
+    sibling: skyblue,
+  },
+};
+
+function testColor(testName, element, color) {
+  test(function() {
+    assert_equals(getComputedStyle(element).color, color);
+  }, testName);
+}
+
+function testClassChange(trigger, targetDescendant, targetSibling, expected)
+{
+  trigger.classList.add('test');
+  testColor(`add .test to ${trigger.id} - check ${targetDescendant.id}`, targetDescendant, colors[expected].descendant);
+  testColor(`add .test to ${trigger.id} - check ${targetSibling.id}`, targetSibling, colors[expected].sibling);
+  trigger.classList.remove('test');
+  testColor(`remove .test from ${trigger.id} - check ${targetDescendant.id}`, targetDescendant, grey);
+  testColor(`remove .test from ${trigger.id} - check ${targetSibling.id}`, targetSibling, grey);
+}
+
+testClassChange(trigger1, subject1_1, subject1_2, 'red');
+testClassChange(trigger2, subject2_1, subject2_2, 'green');
+testClassChange(trigger3, subject3_1, subject3_2, 'blue');
+
+</script>