From 5b68241958408eb390c7d634ee77544c0d7ef861 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Wed, 31 May 2023 11:37:02 +0200
Subject: [PATCH] style: Simplify :dir() implementation

This I noticed while working on the following patches. Shouldn't have
any behavior change: the behavior does in fact match the element state
flag semantics correctly if we do this. We did split the dir flags into
two element bits a while ago.

:not(:dir()) still behaves correctly of course, and we have tests for that.

Differential Revision: https://phabricator.services.mozilla.com/D130734
---
 components/style/gecko/selector_parser.rs     |  8 +++----
 components/style/gecko/wrapper.rs             |  4 ++--
 .../invalidation/element/element_wrapper.rs   | 22 -------------------
 .../invalidation/element/invalidation_map.rs  |  6 +----
 4 files changed, 6 insertions(+), 34 deletions(-)

diff --git a/components/style/gecko/selector_parser.rs b/components/style/gecko/selector_parser.rs
index 7028d554aa2..e9d2c28ccfb 100644
--- a/components/style/gecko/selector_parser.rs
+++ b/components/style/gecko/selector_parser.rs
@@ -162,7 +162,7 @@ impl NonTSPseudoClass {
             ([$(($css:expr, $name:ident, $state:tt, $flags:tt),)*]) => {
                 match *self {
                     $(NonTSPseudoClass::$name => flag!($state),)*
-                    NonTSPseudoClass::Dir(..) |
+                    NonTSPseudoClass::Dir(ref dir) => dir.element_state(),
                     NonTSPseudoClass::MozLocaleDir(..) |
                     NonTSPseudoClass::Lang(..) => ElementState::empty(),
                 }
@@ -186,10 +186,8 @@ impl NonTSPseudoClass {
         self.state_flag().is_empty() &&
             !matches!(
                 *self,
-                // :dir() depends on state only, but doesn't use state_flag
-                // because its semantics don't quite match.  Nevertheless, it
-                // doesn't need cache revalidation, because we already compare
-                // states for elements and candidates.
+                // :dir() depends on state only, but may have an empty
+                // state_flag for invalid arguments.
                 NonTSPseudoClass::Dir(_) |
                       // :-moz-is-html only depends on the state of the document and
                       // the namespace of the element; the former is invariant
diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs
index 95da8b986f6..09dbc6f06c1 100644
--- a/components/style/gecko/wrapper.rs
+++ b/components/style/gecko/wrapper.rs
@@ -1979,7 +1979,8 @@ impl<'le> ::selectors::Element for GeckoElement<'le> {
             NonTSPseudoClass::MozTopmostModalDialog |
             NonTSPseudoClass::Active |
             NonTSPseudoClass::Hover |
-            NonTSPseudoClass::MozAutofillPreview => {
+            NonTSPseudoClass::MozAutofillPreview |
+            NonTSPseudoClass::Dir(..) => {
                 self.state().intersects(pseudo_class.state_flag())
             },
             NonTSPseudoClass::AnyLink => self.is_link(),
@@ -2065,7 +2066,6 @@ impl<'le> ::selectors::Element for GeckoElement<'le> {
                     None => false,
                 }
             },
-            NonTSPseudoClass::Dir(ref dir) => self.state().intersects(dir.element_state()),
         }
     }
 
diff --git a/components/style/invalidation/element/element_wrapper.rs b/components/style/invalidation/element/element_wrapper.rs
index d79e1402228..2aa5749fdee 100644
--- a/components/style/invalidation/element/element_wrapper.rs
+++ b/components/style/invalidation/element/element_wrapper.rs
@@ -178,28 +178,6 @@ where
         // Some pseudo-classes need special handling to evaluate them against
         // the snapshot.
         match *pseudo_class {
-            // :dir is implemented in terms of state flags, but which state flag
-            // it maps to depends on the argument to :dir.  That means we can't
-            // just add its state flags to the NonTSPseudoClass, because if we
-            // added all of them there, and tested via intersects() here, we'd
-            // get incorrect behavior for :not(:dir()) cases.
-            //
-            // FIXME(bz): How can I set this up so once Servo adds :dir()
-            // support we don't forget to update this code?
-            #[cfg(feature = "gecko")]
-            NonTSPseudoClass::Dir(ref dir) => {
-                let selector_flag = dir.element_state();
-                if selector_flag.is_empty() {
-                    // :dir() with some random argument; does not match.
-                    return false;
-                }
-                let state = match self.snapshot().and_then(|s| s.state()) {
-                    Some(snapshot_state) => snapshot_state,
-                    None => self.element.state(),
-                };
-                return state.contains(selector_flag);
-            },
-
             // For :link and :visited, we don't actually want to test the
             // element state directly.
             //
diff --git a/components/style/invalidation/element/invalidation_map.rs b/components/style/invalidation/element/invalidation_map.rs
index a7c2b04df13..42a991e8c15 100644
--- a/components/style/invalidation/element/invalidation_map.rs
+++ b/components/style/invalidation/element/invalidation_map.rs
@@ -494,11 +494,7 @@ impl<'a> SelectorVisitor for SelectorDependencyCollector<'a> {
                 }
             },
             Component::NonTSPseudoClass(ref pc) => {
-                self.compound_state.element_state |= match *pc {
-                    #[cfg(feature = "gecko")]
-                    NonTSPseudoClass::Dir(ref dir) => dir.element_state(),
-                    _ => pc.state_flag(),
-                };
+                self.compound_state.element_state |= pc.state_flag();
                 *self.document_state |= pc.document_state_flag();
 
                 let attr_name = match *pc {