Closed Bug 1364850 Opened 3 years ago Closed 3 years ago

stylo: Store pseudo-element selectors as another Component.

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(1 file, 1 obsolete file)

They're rare enough, and heycam notices that in the future we may need to deal with pseudo selectors at arbitrary points in the selector, so...
Depends on: 1364914
Comment on attachment 8867993 [details]
Bug 1364850: Move PseudoElement to be just another combinator in selectors.

https://reviewboard.mozilla.org/r/139536/#review142992

Thanks for doing this!

::: servo/components/selectors/matching.rs:77
(Diff revision 1)
> +/// What kind of selector matching mode we should use.
> +///
> +/// There are two modes of selector matching. The difference is only noticeable
> +/// in presence of pseudo-elements.
> +#[derive(Debug, PartialEq)]
> +pub enum MatchingMode {

This strikes me as a bit hard to grok for the caller.

How about MatchingMode {
  Normal,
  ForStatelessPseudoElement,
}

And then document the precise semantics of ForStatelessPseudoElement in the doc comments?

::: servo/components/selectors/matching.rs:86
(Diff revision 1)
> +    ///
> +    /// This is the default.

I don't think it makes sense for this to be the default. And I don't see that happening anywhere anyway?

::: servo/components/selectors/matching.rs:110
(Diff revision 1)
> +    pub fn new(
> +        matching_mode: MatchingMode,
> +        bloom_filter: Option<&'a BloomFilter>)
> +        -> Self

Nit: put matching_mode directly after the open-paren.

::: servo/components/selectors/matching.rs:159
(Diff revision 1)
>      true
>  }
>  
>  /// Determines whether the given element matches the given complex selector.
> -pub fn matches_selector<E, F>(selector: &SelectorInner<E::Impl>,
> +pub fn matches_selector<E, F>(selector: &Selector<E::Impl>,
>                                element: &E,

I think this can continue to accept a SelectorInner, because we don't need specificity here.

::: servo/components/selectors/matching.rs:238
(Diff revision 1)
> +    match context.matching_mode {
> +        MatchingMode::DontIgnore => {}, // Do nothing.

I'd pref if matches!(...) instead.

::: servo/components/selectors/matching.rs:241
(Diff revision 1)
> +{
> +    let mut iter = selector.inner.complex.iter();
> +    match context.matching_mode {
> +        MatchingMode::DontIgnore => {}, // Do nothing.
> +        MatchingMode::IgnoreStatelessPseudoElements => {
> +            if selector.has_pseudo_element() {

Per IRC discussion, I'd like to get rid of this branch, and just assert that we have a pseudo if matching in pseudo-matching mode. That will let us get rid of the distinction between the inner and non-inner variants, and pass the inner everywhere.

::: servo/components/selectors/matching.rs:266
(Diff revision 1)
> +/// Matches an inner selector.
> +///
> +/// NOTE: This function requires `context.matching_mode` to be
> +/// `MatchingMode::DontIgnore`.
> +///
> +/// Use `matches_selector` if you need to skip pseudos.
> +pub fn matches_inner_selector<E, F>(
> +    selector: &SelectorInner<E::Impl>,
> +    element: &E,
> +    context: &mut MatchingContext,
> +    flags_setter: &mut F)
> +    -> bool
> +     where E: Element,
> +           F: FnMut(&E, ElementSelectorFlags),
> +{
> +    debug_assert_eq!(context.matching_mode, MatchingMode::DontIgnore);
> +    // Use the bloom filter to fast-reject.
> +    if let Some(filter) = context.bloom_filter {
> +        if !may_match::<E>(&selector, filter) {
> +            return false;
> +        }
> +    }
> +
> +    matches_complex_selector(&selector.complex, element, context, flags_setter)
> +}

This can go away now.

::: servo/components/selectors/matching.rs:293
(Diff revision 1)
> -pub fn matches_complex_selector<E, F>(selector: &ComplexSelector<E::Impl>,
> +///
> +/// NOTE: This function requires `context.matching_mode` to be
> +/// `MatchingMode::DontIgnore`.
> +///
> +/// Use `matches_selector` if you need to skip pseudos.

This comment will need to be updated.

::: servo/components/selectors/parser.rs:207
(Diff revision 1)
> +    fn borrow(&self) -> &SelectorInner<Impl> {
> +        &self.inner
> +    }
> +}
> +
> +const HAS_PSEUDO_BIT: u32 = 1 << 30;

I guess we probably still want to keep this bit around so that stylist insertion doesn't have to traverse the rightmost SS each time it inserts something into the hash.

::: servo/components/selectors/parser.rs:214
(Diff revision 1)
> +    /// Returns whether this selector is of the form `*|*`, without accounting
> +    /// for pseudo-elements.
> +    pub fn is_universal_selector(&self) -> bool {
> +        let mut iter = self.inner.complex.iter();
> +        match iter.next() {
> +            None => true,
> +            Some(&Component::PseudoElement(..)) => iter.next_sequence().is_none(),

Can you clarify exactly what the pseudo handling is here? It's not obvious to me from the comment and the impl.

::: servo/components/selectors/parser.rs:412
(Diff revision 1)
> -        debug_assert!(self.next_combinator.is_none(), "Should call take_combinator!");
> +        if self.next_combinator.is_some() {
> +            return None;
> +        }

Hm, why the change in semantics here? I'm not necessarily opposed, but would like to understand why.

::: servo/components/selectors/parser.rs:423
(Diff revision 1)
> +                if let Component::PseudoElement(..) = *x {
> +                    // Iff we have anything to our left, insert a child
> +                    // combinator here.
> +                    if self.iter.clone().next().is_some() {
> +                        self.next_combinator = Some(Combinator::Child);
> +                    }
> +                }
> +                Some(x)
> +            },

Per IRC discussion, I think we should make :: a bonafide combinator, and then remove this.

::: servo/components/style/stylist.rs:840
(Diff revision 1)
> +            debug_assert_eq!(element.parent_element(),
> +                             element.closest_non_native_anonymous_ancestor());

Err, why is this true? How does this handle stuff like -moz-number-spin-box, which inherits from another NAC pseudo?

::: servo/components/style/stylist.rs:1549
(Diff revision 1)
>          true
>      }
>  }
>  
>  /// Retrieve the first ID name in the selector, or None otherwise.
>  pub fn get_id_name(selector: &SelectorInner<SelectorImpl>) -> Option<Atom> {

This function, and the ones like it, will need to be modified to check if take_combinator() returns ::, and keep going if so.
Attachment #8867993 - Flags: review?(bobbyholley) → review-
Attachment #8867992 - Attachment is obsolete: true
Comment on attachment 8867993 [details]
Bug 1364850: Move PseudoElement to be just another combinator in selectors.

https://reviewboard.mozilla.org/r/139536/#review142992

> This strikes me as a bit hard to grok for the caller.
> 
> How about MatchingMode {
>   Normal,
>   ForStatelessPseudoElement,
> }
> 
> And then document the precise semantics of ForStatelessPseudoElement in the doc comments?

Sounds good. Done.

> I don't think it makes sense for this to be the default. And I don't see that happening anywhere anyway?

Yeah, turns out I'm not the best at cleaning up my own patches when it's late in the night, oh well :)

> Nit: put matching_mode directly after the open-paren.

(block indentation is the convention now, according to https://github.com/rust-lang-nursery/fmt-rfcs, but will do)

> I'd pref if matches!(...) instead.

I wanted to be explicit, but I just used `==` instead now.

> Per IRC discussion, I'd like to get rid of this branch, and just assert that we have a pseudo if matching in pseudo-matching mode. That will let us get rid of the distinction between the inner and non-inner variants, and pass the inner everywhere.

Yup, done.

> Can you clarify exactly what the pseudo handling is here? It's not obvious to me from the comment and the impl.

So that works for the same reason the pseudo assert needed to change below. And it pretty much returns true for `*|*::pseudo`, which is the only thing we actually use it for.

(I added tests for this btw)

> Hm, why the change in semantics here? I'm not necessarily opposed, but would like to understand why.

This was needed because after calling `next()` with a `PseudoElement`, we did set the combinator, and thus the caller never got a `None` back. With the explicit combinator that's not needed anymore.

> Per IRC discussion, I think we should make :: a bonafide combinator, and then remove this.

Done.

> Err, why is this true? How does this handle stuff like -moz-number-spin-box, which inherits from another NAC pseudo?

That was indeed not true, fixed.

> This function, and the ones like it, will need to be modified to check if take_combinator() returns ::, and keep going if so.

Yup, I noticed that and was changing when you appeared over IRC ;)
Depending on which one lands first, this will affect Selector::is_universal added in https://github.com/servo/servo/pull/16890
Comment on attachment 8867993 [details]
Bug 1364850: Move PseudoElement to be just another combinator in selectors.

https://reviewboard.mozilla.org/r/139536/#review143060

::: servo/components/selectors/matching.rs:78
(Diff revision 2)
> +///
> +/// There are two modes of selector matching. The difference is only noticeable
> +/// in presence of pseudo-elements.
> +#[derive(Debug, PartialEq)]
> +pub enum MatchingMode {
> +    /// Don't ignore any pseudo-element selector.

nit: selectors.

::: servo/components/selectors/matching.rs:81
(Diff revision 2)
> +    /// Ignores any stateless pseudo-element selectors to the right of the
> +    /// selector.

Hm, "to the right of"? I might say "in the rightmost sequence of simple selectors".

::: servo/components/selectors/matching.rs:208
(Diff revision 2)
> +pub fn matches_selector<E, F>(
> +    selector: &SelectorInner<E::Impl>,
> +    element: &E,
> +    context: &mut MatchingContext,
> +    flags_setter: &mut F)
> +    -> bool
> +     where E: Element,
> +           F: FnMut(&E, ElementSelectorFlags),
> +{

Please use indentation that's consistent with the rest of the file. If we decide to switch to block indentation at some point, we should do that across the entire crate, so as to avoid having inconsistent style.

::: servo/components/selectors/matching.rs:230
(Diff revision 2)
> +pub fn matches_complex_selector<E, F>(
> +    complex_selector: &ComplexSelector<E::Impl>,
> -                                      element: &E,
> +    element: &E,
> -                                      context: &mut MatchingContext,
> +    context: &mut MatchingContext,
> -                                      flags_setter: &mut F)
> +    flags_setter: &mut F)
> -                                      -> bool
> +    -> bool
>       where E: Element,
>             F: FnMut(&E, ElementSelectorFlags),

Same here.

::: servo/components/selectors/parser.rs:26
(Diff revision 2)
> +    /// of it.
> +    fn supports_pseudo_class(
> +        &self,
> +        _pseudo_class: &<Self::Impl as SelectorImpl>::NonTSPseudoClass)
> +        -> bool

Same here.

::: servo/components/selectors/parser.rs:238
(Diff revision 2)
> +        debug_assert!(false, "has_pseudo_element lied!");
> +        None

We should just panic!() here IMO.

::: servo/components/selectors/parser.rs:439
(Diff revision 2)
>          result.skip_until_ancestor();
>          result
>      }
>  
> -    /// Skips a sequence of simple selectors and all subsequent sequences until an
> -    /// ancestor combinator is reached.
> +    /// Skips a sequence of simple selectors and all subsequent sequences until
> +    /// non-pseudo-element ancestor combinator is reached.

Missing "a".

::: servo/components/selectors/parser.rs:443
(Diff revision 2)
> -    /// Skips a sequence of simple selectors and all subsequent sequences until an
> -    /// ancestor combinator is reached.
> +    /// Skips a sequence of simple selectors and all subsequent sequences until
> +    /// non-pseudo-element ancestor combinator is reached.
>      fn skip_until_ancestor(&mut self) {
>          loop {
>              while let Some(_) = self.0.next() {}
> -            if self.0.next_sequence().map_or(true, |x| x.is_ancestor()) {
> +            if self.0.next_sequence().map_or(true, |x| x.is_ancestor() && !x.is_pseudo_element()) {

I think it's probably better to just match!(Child | Descendant) here.

::: servo/components/selectors/parser.rs:461
(Diff revision 2)
>              return next;
>          }
>  
>          // See if there are more sequences. If so, skip any non-ancestor sequences.
>          if let Some(combinator) = self.0.next_sequence() {
> -            if !combinator.is_ancestor() {
> +            if !combinator.is_ancestor() || combinator.is_pseudo_element() {

Same here.

::: servo/components/selectors/parser.rs:476
(Diff revision 2)
>  pub enum Combinator {
>      Child,  //  >
>      Descendant,  // space
>      NextSibling,  // +
>      LaterSibling,  // ~
> +    /// A dummy combinator we use right before of pseudo-elements.

Parse error. I'd say "to the left of".

Though really I'd say "Represents the :: before a pseudo-element. This isn't really a combinator per spec, but we can treat it like one in most places".

::: servo/components/selectors/parser.rs:642
(Diff revision 2)
>  
>  impl<Impl: SelectorImpl> Debug for Selector<Impl> {
>      fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
>          f.write_str("Selector(")?;
>          self.to_css(f)?;
> -        write!(f, ", specificity = 0x{:x})", self.specificity)
> +        write!(f, ", specificity = 0x{:x})", self.specificity_and_flags)

Either change the title or use the specificity() accessor.

::: servo/components/style/gecko/wrapper.rs:1058
(Diff revision 2)
>      fn parent_element(&self) -> Option<Self> {
>          let parent_node = self.as_node().parent_node();
>          parent_node.and_then(|n| n.as_element())
>      }
>  
> +    fn pseudo_element_parent(&self) -> Option<Self> {

We should call this originating_element. And maybe we should assert that we're a pseudo here?

::: servo/components/style/gecko/wrapper.rs:1248
(Diff revision 2)
> +    fn match_pseudo_element(
> +        &self,
> +        pseudo_element: &PseudoElement,
> +        _context: &mut MatchingContext)

Formatting.

::: servo/components/style/restyle_hints.rs:408
(Diff revision 2)
> +    fn match_pseudo_element(
> +        &self,
> +        pseudo_element: &PseudoElement,
> +        context: &mut MatchingContext)
> +        -> bool

Formatting.

::: servo/components/style/stylist.rs:674
(Diff revision 2)
> +        let mut matching_context =
> +            MatchingContext::new(MatchingMode::ForStatelessPseudoElement, None);
> +

So, maybe I'm being dense, but given that we no longer pass the pseudo state, and given that we're passing ForStatelessPseudoElement, how do we handle stateful pseudo-elements?

Or is the issue actually that ForStatelessPseudoElement also effectively handles stateful pseudo elements, and therefore my proposed name was wrong, and it should be ForPseudoElement?

::: servo/components/style/stylist.rs:835
(Diff revision 2)
> +        let rule_hash_target = if is_implemented_pseudo {
> +            element.closest_non_native_anonymous_ancestor().unwrap()
> +        } else {
> +            *element
> +        };

Add a comment here saying that this causes us to rulehash pseudo selectors based on the selector for the originating element.

::: servo/components/style/stylist.rs:869
(Diff revision 2)
> -        if element.matches_user_and_author_rules() {
> +        // NB: the following condition, although it may look somewhat
> +        // inaccurate, would be equivalent to something like:
> +        //
> +        //     element.matches_user_and_author_rules() ||
> +        //     (is_implemented_pseudo &&
> +        //      rule_hash_target.matches_user_and_author_rules())
> +        //
> +        // Which may be more what you would probably expect.
> +        if rule_hash_target.matches_user_and_author_rules() {

I might just simplify this comment and say "NB: For non-pseudo-elements, rule_hash_target is just the element. For pseudos, it's the originating element".

::: servo/components/style/stylist.rs:1542
(Diff revision 2)
> +/// Get the first relevant component of a selector.
> +///
> +/// We ignore pseudo-element sequences.
> +fn get_first<F, R>(selector: &SelectorInner<SelectorImpl>, mut f: F) -> Option<R>

Let's call this find_from_right, and make the comment say:

"Searches the selector from right to left, beginning to the left of the ::pseudo-element (if any), and ending at the first combinator. The first non-None value returned from |f| is returned."
Attachment #8867993 - Flags: review?(bobbyholley) → review+
Comment on attachment 8867993 [details]
Bug 1364850: Move PseudoElement to be just another combinator in selectors.

https://reviewboard.mozilla.org/r/139536/#review143060

> Either change the title or use the specificity() accessor.

err, right, too much `s/specificity/specificity_and_flags`

> We should call this originating_element. And maybe we should assert that we're a pseudo here?

We assert we're NAC already in get_closer_non_nac_ancestor, but sure.

> So, maybe I'm being dense, but given that we no longer pass the pseudo state, and given that we're passing ForStatelessPseudoElement, how do we handle stateful pseudo-elements?
> 
> Or is the issue actually that ForStatelessPseudoElement also effectively handles stateful pseudo elements, and therefore my proposed name was wrong, and it should be ForPseudoElement?

So the key point here is that we match pseudo-elements using `MatchingMode::Normal`, except for `Eager` pseudos (and probing/lazy pseudo-style resolution, for which we use `ForStatelessPseudoElement`).

For actual pseudo-elements, we match _against_ the pseudo, which includes matching against the style pseudo-classes to the right of the selector.
https://github.com/servo/servo/pull/16900
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.