From ae5e0d49d816b219d3817ceab8b994fbe946442f Mon Sep 17 00:00:00 2001 From: David Shin Date: Tue, 13 Jun 2023 13:21:42 +0000 Subject: [PATCH] style: Move `ANCHORS_RELATIVE_SELECTOR` out of nsINode flags Move the flag to ComputedValueFlags, like `CONSIDERED_RELATIVE_SELECTOR`. Differential Revision: https://phabricator.services.mozilla.com/D180726 --- components/selectors/context.rs | 29 ++++++++++++++---- components/selectors/matching.rs | 17 ++++------- components/style/dom.rs | 4 --- components/style/gecko/wrapper.rs | 8 ----- .../style/properties/computed_value_flags.rs | 8 ++++- components/style/sharing/mod.rs | 6 +++- components/style/style_resolver.rs | 30 ++++++++++++------- 7 files changed, 59 insertions(+), 43 deletions(-) diff --git a/components/selectors/context.rs b/components/selectors/context.rs index fc620baa08f..d8c461660d3 100644 --- a/components/selectors/context.rs +++ b/components/selectors/context.rs @@ -99,6 +99,21 @@ impl QuirksMode { } } +/// Whether or not this matching considered relative selector. +#[derive(Clone, Copy, Debug, PartialEq)] +pub enum RelativeSelectorMatchingState { + /// Was not considered for any relative selector. + None, + /// Relative selector was considered for a match, but the element to be + /// under matching would not anchor the relative selector. i.e. The + /// relative selector was not part of the first compound selector (in match + /// order). + Considered, + /// Same as above, but the relative selector was part of the first compound + /// selector (in match order). + ConsideredAnchor, +} + /// Data associated with the matching process for a element. This context is /// used across many selectors for an element, so it's not appropriate for /// transient data that applies to only a single selector. @@ -146,7 +161,7 @@ where /// The current element we're anchoring on for evaluating the relative selector. current_relative_selector_anchor: Option, - pub considered_relative_selector: bool, + pub considered_relative_selector: RelativeSelectorMatchingState, quirks_mode: QuirksMode, needs_selector_flags: NeedsSelectorFlags, @@ -200,7 +215,7 @@ where pseudo_element_matching_fn: None, extra_data: Default::default(), current_relative_selector_anchor: None, - considered_relative_selector: false, + considered_relative_selector: RelativeSelectorMatchingState::None, _impl: ::std::marker::PhantomData, } } @@ -329,12 +344,14 @@ where where F: FnOnce(&mut Self) -> R, { - // TODO(dshin): Nesting should be rejected at parse time. - let original_relative_selector_anchor = self.current_relative_selector_anchor.take(); + debug_assert!( + self.current_relative_selector_anchor.is_none(), + "Nesting should've been rejected at parse time" + ); self.current_relative_selector_anchor = Some(anchor); + self.considered_relative_selector = RelativeSelectorMatchingState::Considered; let result = self.nest(f); - self.current_relative_selector_anchor = original_relative_selector_anchor; - self.considered_relative_selector = true; + self.current_relative_selector_anchor = None; result } diff --git a/components/selectors/matching.rs b/components/selectors/matching.rs index 25f831df2de..f347dc4078e 100644 --- a/components/selectors/matching.rs +++ b/components/selectors/matching.rs @@ -60,18 +60,13 @@ bitflags! { /// The element has an empty selector, so when a child is appended we /// might need to restyle the parent completely. const HAS_EMPTY_SELECTOR = 1 << 4; - - /// This element has a relative selector that anchors to it, or may do so - /// if its descendants or its later siblings change. - const ANCHORS_RELATIVE_SELECTOR = 1 << 5; } } impl ElementSelectorFlags { /// Returns the subset of flags that apply to the element. pub fn for_self(self) -> ElementSelectorFlags { - self & (ElementSelectorFlags::HAS_EMPTY_SELECTOR | - ElementSelectorFlags::ANCHORS_RELATIVE_SELECTOR) + self & ElementSelectorFlags::HAS_EMPTY_SELECTOR } /// Returns the subset of flags that apply to the parent. @@ -370,12 +365,10 @@ fn matches_relative_selectors( element: &E, context: &mut MatchingContext, ) -> bool { - if context.needs_selector_flags() { - // If we've considered anchoring `:has()` selector while trying to match this element, - // mark it as such, as it has implications on style sharing (See style sharing - // code for further information). - element.apply_selector_flags(ElementSelectorFlags::ANCHORS_RELATIVE_SELECTOR); - } + // If we've considered anchoring `:has()` selector while trying to match this element, + // mark it as such, as it has implications on style sharing (See style sharing + // code for further information). + context.considered_relative_selector = RelativeSelectorMatchingState::ConsideredAnchor; for RelativeSelector { match_hint, selector, diff --git a/components/style/dom.rs b/components/style/dom.rs index 015e1eb80b6..9ef067e76f2 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -900,10 +900,6 @@ pub trait TElement: &self, display: &Display, ) -> euclid::default::Size2D>; - - /// Returns true if this element anchors a relative selector, now or after - /// a DOM mutation. - fn anchors_relative_selector(&self) -> bool; } /// TNode and TElement aren't Send because we want to be careful and explicit diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index 3396d3ca583..a4d35df4274 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -894,9 +894,6 @@ fn selector_flags_to_node_flags(flags: ElementSelectorFlags) -> u32 { if flags.contains(ElementSelectorFlags::HAS_EMPTY_SELECTOR) { gecko_flags |= NODE_HAS_EMPTY_SELECTOR; } - if flags.contains(ElementSelectorFlags::ANCHORS_RELATIVE_SELECTOR) { - gecko_flags |= NODE_ANCHORS_RELATIVE_SELECTOR; - } gecko_flags } @@ -1730,11 +1727,6 @@ impl<'le> TElement for GeckoElement<'le> { } } } - - fn anchors_relative_selector(&self) -> bool { - use crate::gecko_bindings::structs::NODE_ANCHORS_RELATIVE_SELECTOR; - self.flags() & NODE_ANCHORS_RELATIVE_SELECTOR != 0 - } } impl<'le> PartialEq for GeckoElement<'le> { diff --git a/components/style/properties/computed_value_flags.rs b/components/style/properties/computed_value_flags.rs index d84311de084..8ac90aee27f 100644 --- a/components/style/properties/computed_value_flags.rs +++ b/components/style/properties/computed_value_flags.rs @@ -121,6 +121,10 @@ bitflags! { /// Whether the style evaluated any relative selector. const CONSIDERED_RELATIVE_SELECTOR = 1 << 24; + + /// Whether the style evaluated the matched element to be an anchor of + /// a relative selector. + const ANCHORS_RELATIVE_SELECTOR = 1 << 25; } } @@ -153,7 +157,9 @@ impl ComputedValueFlags { /// Flags that are an input to the cascade. #[inline] fn cascade_input_flags() -> Self { - Self::USES_VIEWPORT_UNITS_ON_CONTAINER_QUERIES | Self::CONSIDERED_RELATIVE_SELECTOR + Self::USES_VIEWPORT_UNITS_ON_CONTAINER_QUERIES | + Self::CONSIDERED_RELATIVE_SELECTOR | + Self::ANCHORS_RELATIVE_SELECTOR } /// Returns the flags that are always propagated to descendants. diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index 4a4ab5f9a80..a78eb724c5d 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -632,7 +632,11 @@ impl StyleSharingCache { // matching `.foo`, even if `:has()` may not even be used. Ideally we'd // have something like `RelativeSelectorConsidered::RightMost`, but the // element flag is required for invalidation, and this reduces more tracking. - if element.anchors_relative_selector() { + if style + .style + .0 + .flags + .intersects(ComputedValueFlags::ANCHORS_RELATIVE_SELECTOR) { debug!("Failing to insert to the cache: may anchor relative selector"); return; } diff --git a/components/style/style_resolver.rs b/components/style/style_resolver.rs index a082b2cbfff..7ebcb97eefb 100644 --- a/components/style/style_resolver.rs +++ b/components/style/style_resolver.rs @@ -16,7 +16,7 @@ use crate::rule_tree::StrongRuleNode; use crate::selector_parser::{PseudoElement, SelectorImpl}; use crate::stylist::RuleInclusion; use log::Level::Trace; -use selectors::matching::{MatchingContext, NeedsSelectorFlags}; +use selectors::matching::{MatchingContext, NeedsSelectorFlags, RelativeSelectorMatchingState}; use selectors::matching::{MatchingMode, VisitedHandlingMode}; use servo_arc::Arc; @@ -503,16 +503,24 @@ where } } } - - if matching_context.considered_relative_selector { - // This is a bit awkward - ideally, the flag is set directly where `considered_relative_selector` - // is; however, in that context, the implementation detail of `extra_data` is not visible, so - // it's done here. A trait for manipulating the flags is an option, but not worth it for a single flag. - matching_context - .extra_data - .cascade_input_flags - .insert(ComputedValueFlags::CONSIDERED_RELATIVE_SELECTOR); - } + // This is a bit awkward - ideally, the flag is set directly where `considered_relative_selector` + // is; however, in that context, the implementation detail of `extra_data` is not visible, so + // it's done here. A trait for manipulating the flags is an option, but not worth it for a single flag. + match matching_context.considered_relative_selector { + RelativeSelectorMatchingState::None => (), + RelativeSelectorMatchingState::Considered => { + matching_context + .extra_data + .cascade_input_flags + .insert(ComputedValueFlags::CONSIDERED_RELATIVE_SELECTOR); + }, + RelativeSelectorMatchingState::ConsideredAnchor => { + matching_context.extra_data.cascade_input_flags.insert( + ComputedValueFlags::ANCHORS_RELATIVE_SELECTOR | + ComputedValueFlags::CONSIDERED_RELATIVE_SELECTOR, + ); + }, + }; MatchingResults { rule_node,