From 43444db8a80f8312122b6629cb7d050ac4f065c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 24 May 2019 01:09:15 +0000 Subject: [PATCH] style: Cleanup selector-matching for nested pseudo-elements, match ::slotted correctly when there's no selector before it, and add tests. D29542 fixed the bogus checks that was making nested pseudo-elements match author rules. This adds tests and ends up being just a cleanup, though as it turns out we it also fixes an issue with ::slotted() matched from Element.matches. Differential Revision: https://phabricator.services.mozilla.com/D27529 --- components/selectors/builder.rs | 6 ------ components/selectors/matching.rs | 13 +++---------- components/selectors/parser.rs | 18 +++++++++++------- components/selectors/tree.rs | 4 ++++ components/style/dom.rs | 2 +- components/style/gecko/wrapper.rs | 11 ++++++++--- .../invalidation/element/element_wrapper.rs | 4 ++++ components/style/style_adjuster.rs | 5 +---- components/style/style_resolver.rs | 10 +++------- 9 files changed, 35 insertions(+), 38 deletions(-) diff --git a/components/selectors/builder.rs b/components/selectors/builder.rs index b548ceb83ce..c3e733cbeb0 100644 --- a/components/selectors/builder.rs +++ b/components/selectors/builder.rs @@ -84,12 +84,6 @@ impl SelectorBuilder { self.current_len = 0; } - /// Returns true if no simple selectors have ever been pushed to this builder. - #[inline(always)] - pub fn is_empty(&self) -> bool { - self.simple_selectors.is_empty() - } - /// Returns true if combinators have ever been pushed to this builder. #[inline(always)] pub fn has_combinators(&self) -> bool { diff --git a/components/selectors/matching.rs b/components/selectors/matching.rs index aa200cc4b26..6554f2f9cc4 100644 --- a/components/selectors/matching.rs +++ b/components/selectors/matching.rs @@ -331,11 +331,9 @@ where return false; } - // Advance to the non-pseudo-element part of the selector, but let the - // context note that . - if iter.next_sequence().is_none() { - return true; - } + // Advance to the non-pseudo-element part of the selector. + let next_sequence = iter.next_sequence().unwrap(); + debug_assert_eq!(next_sequence, Combinator::PseudoElement); } let result = @@ -452,10 +450,6 @@ where }, Combinator::Part => element.containing_shadow_host(), Combinator::SlotAssignment => { - debug_assert!( - context.current_host.is_some(), - "Should not be trying to match slotted rules in a non-shadow-tree context" - ); debug_assert!(element .assigned_slot() .map_or(true, |s| s.is_html_slot_element())); @@ -677,7 +671,6 @@ where Component::Slotted(ref selector) => { // are never flattened tree slottables. !element.is_html_slot_element() && - element.assigned_slot().is_some() && context.shared.nest(|context| { matches_complex_selector(selector.iter(), element, context, flags_setter) }) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index a924bbc17d0..f049555730c 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -2002,9 +2002,7 @@ where }, SimpleSelectorParseResult::SlottedPseudo(selector) => { state.insert(SelectorParsingState::AFTER_SLOTTED); - if !builder.is_empty() { - builder.push_combinator(Combinator::SlotAssignment); - } + builder.push_combinator(Combinator::SlotAssignment); builder.push_simple_selector(Component::Slotted(selector)); }, SimpleSelectorParseResult::PseudoElement(p) => { @@ -2012,9 +2010,7 @@ where if !p.accepts_state_pseudo_classes() { state.insert(SelectorParsingState::AFTER_NON_STATEFUL_PSEUDO_ELEMENT); } - if !builder.is_empty() { - builder.push_combinator(Combinator::PseudoElement); - } + builder.push_combinator(Combinator::PseudoElement); builder.push_simple_selector(Component::PseudoElement(p)); }, } @@ -2828,7 +2824,10 @@ pub mod tests { assert_eq!( parse("::before"), Ok(SelectorList::from_vec(vec![Selector::from_vec( - vec![Component::PseudoElement(PseudoElement::Before)], + vec![ + Component::Combinator(Combinator::PseudoElement), + Component::PseudoElement(PseudoElement::Before), + ], specificity(0, 0, 1) | HAS_PSEUDO_BIT, )])) ); @@ -2836,6 +2835,7 @@ pub mod tests { parse("::before:hover"), Ok(SelectorList::from_vec(vec![Selector::from_vec( vec![ + Component::Combinator(Combinator::PseudoElement), Component::PseudoElement(PseudoElement::Before), Component::NonTSPseudoClass(PseudoClass::Hover), ], @@ -2846,6 +2846,7 @@ pub mod tests { parse("::before:hover:hover"), Ok(SelectorList::from_vec(vec![Selector::from_vec( vec![ + Component::Combinator(Combinator::PseudoElement), Component::PseudoElement(PseudoElement::Before), Component::NonTSPseudoClass(PseudoClass::Hover), Component::NonTSPseudoClass(PseudoClass::Hover), @@ -2958,6 +2959,7 @@ pub mod tests { specificity(0, 0, 0), )])) ); + assert_eq!( parse_ns(":not(svg|*)", &parser), Ok(SelectorList::from_vec(vec![Selector::from_vec( @@ -3032,6 +3034,8 @@ pub mod tests { Some(&Component::PseudoElement(PseudoElement::Before)) ); assert_eq!(iter.next(), None); + assert_eq!(iter.next_sequence(), Some(Combinator::PseudoElement)); + assert_eq!(iter.next(), None); assert_eq!(iter.next_sequence(), None); } diff --git a/components/selectors/tree.rs b/components/selectors/tree.rs index 52599893d2f..611e40d6027 100644 --- a/components/selectors/tree.rs +++ b/components/selectors/tree.rs @@ -47,9 +47,13 @@ pub trait Element: Sized + Clone + Debug { /// /// This is guaranteed to be called in a pseudo-element. fn pseudo_element_originating_element(&self) -> Option { + debug_assert!(self.is_pseudo_element()); self.parent_element() } + /// Whether we're matching on a pseudo-element. + fn is_pseudo_element(&self) -> bool; + /// Skips non-element nodes fn prev_sibling_element(&self) -> Option; diff --git a/components/style/dom.rs b/components/style/dom.rs index 633dce83655..5ee8443b91d 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -779,7 +779,7 @@ pub trait TElement: /// element-backed pseudo-element, in which case we return the originating /// element. fn rule_hash_target(&self) -> Self { - if self.implemented_pseudo_element().is_some() { + if self.is_pseudo_element() { self.pseudo_element_originating_element() .expect("Trying to collect rules for a detached pseudo-element") } else { diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index cec55130c4a..a1443d1ad8d 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -1098,7 +1098,7 @@ impl<'le> TElement for GeckoElement<'le> { type TraversalChildrenIterator = GeckoChildrenIterator<'le>; fn inheritance_parent(&self) -> Option { - if self.implemented_pseudo_element().is_some() { + if self.is_pseudo_element() { return self.pseudo_element_originating_element(); } @@ -1471,7 +1471,7 @@ impl<'le> TElement for GeckoElement<'le> { #[inline] fn skip_item_display_fixup(&self) -> bool { debug_assert!( - self.implemented_pseudo_element().is_none(), + !self.is_pseudo_element(), "Just don't call me if I'm a pseudo, you should know the answer already" ); self.is_root_of_native_anonymous_subtree() @@ -1918,9 +1918,14 @@ impl<'le> ::selectors::Element for GeckoElement<'le> { Some(shadow.host()) } + #[inline] + fn is_pseudo_element(&self) -> bool { + self.implemented_pseudo_element().is_some() + } + #[inline] fn pseudo_element_originating_element(&self) -> Option { - debug_assert!(self.implemented_pseudo_element().is_some()); + debug_assert!(self.is_pseudo_element()); let parent = self.closest_anon_subtree_root_parent()?; // FIXME(emilio): Special-case for s diff --git a/components/style/invalidation/element/element_wrapper.rs b/components/style/invalidation/element/element_wrapper.rs index 6e8e504c226..91f0705853c 100644 --- a/components/style/invalidation/element/element_wrapper.rs +++ b/components/style/invalidation/element/element_wrapper.rs @@ -366,6 +366,10 @@ where self.element.is_root() } + fn is_pseudo_element(&self) -> bool { + self.element.is_pseudo_element() + } + fn pseudo_element_originating_element(&self) -> Option { self.element .pseudo_element_originating_element() diff --git a/components/style/style_adjuster.rs b/components/style/style_adjuster.rs index 63af60e16ab..21b6fbd60ca 100644 --- a/components/style/style_adjuster.rs +++ b/components/style/style_adjuster.rs @@ -734,10 +734,7 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> { E: TElement, { if cfg!(debug_assertions) { - if element - .and_then(|e| e.implemented_pseudo_element()) - .is_some() - { + if element.map_or(false, |e| e.is_pseudo_element()) { // It'd be nice to assert `self.style.pseudo == Some(&pseudo)`, // but we do resolve ::-moz-list pseudos on ::before / ::after // content, sigh. diff --git a/components/style/style_resolver.rs b/components/style/style_resolver.rs index 11c81283a13..180de3ff281 100644 --- a/components/style/style_resolver.rs +++ b/components/style/style_resolver.rs @@ -233,7 +233,7 @@ where let mut pseudo_styles = EagerPseudoStyles::default(); - if self.element.implemented_pseudo_element().is_none() { + if !self.element.is_pseudo_element() { let layout_parent_style_for_pseudo = if primary_style.style().is_display_contents() { layout_parent_style } else { @@ -293,10 +293,6 @@ where layout_parent_style: Option<&ComputedValues>, pseudo: Option<&PseudoElement>, ) -> ResolvedStyle { - debug_assert!( - self.element.implemented_pseudo_element().is_none() || pseudo.is_none(), - "Pseudo-elements can't have other pseudos!" - ); debug_assert!(pseudo.map_or(true, |p| p.is_eager())); let implemented_pseudo = self.element.implemented_pseudo_element(); @@ -477,8 +473,8 @@ where ); debug_assert!(pseudo_element.is_eager()); debug_assert!( - self.element.implemented_pseudo_element().is_none(), - "Element pseudos can't have any other pseudo." + !self.element.is_pseudo_element(), + "Element pseudos can't have any other eager pseudo." ); let mut applicable_declarations = ApplicableDeclarationList::new();