Skip to content

Commit

Permalink
style: Fix a no-longer valid assumption in pseudo-element matching / …
Browse files Browse the repository at this point in the history
…invalidation code.

After bug 1632647, we can have pseudo-classes inside :not / :is /
:where, which the invalidation and matching code weren't handling.

Add a few tests for this stuff working as expected.

Differential Revision: https://phabricator.services.mozilla.com/D76160
  • Loading branch information
emilio committed Jun 3, 2020
1 parent a40b2b6 commit bd23e05
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 19 deletions.
15 changes: 7 additions & 8 deletions components/selectors/matching.rs
Expand Up @@ -322,14 +322,13 @@ where
},
}

// The only other parser-allowed Component in this sequence is a state
// class. We just don't match in that case.
if let Some(s) = iter.next() {
debug_assert!(
matches!(*s, Component::NonTSPseudoClass(..)),
"Someone messed up pseudo-element parsing"
);
return false;
for component in &mut iter {
// The only other parser-allowed Components in this sequence are
// state pseudo-classes, or one of the other things that can contain
// them.
if !component.matches_for_stateless_pseudo_element() {
return false;
}
}

// Advance to the non-pseudo-element part of the selector.
Expand Down
41 changes: 41 additions & 0 deletions components/selectors/parser.rs
Expand Up @@ -1054,6 +1054,47 @@ impl<Impl: SelectorImpl> Component<Impl> {
}
}

/// Whether this component is valid after a pseudo-element. Only intended
/// for sanity-checking.
pub fn maybe_allowed_after_pseudo_element(&self) -> bool {
match *self {
Component::NonTSPseudoClass(..) => true,
Component::Negation(ref components) => components.iter().all(|c| c.maybe_allowed_after_pseudo_element()),
Component::Is(ref selectors) |
Component::Where(ref selectors) => {
selectors.iter().all(|selector| {
selector.iter_raw_match_order().all(|c| c.maybe_allowed_after_pseudo_element())
})
},
_ => false,
}
}

/// Whether a given selector should match for stateless pseudo-elements.
///
/// This is a bit subtle: Only selectors that return true in
/// `maybe_allowed_after_pseudo_element` should end up here, and
/// `NonTSPseudoClass` never matches (as it is a stateless pseudo after
/// all).
pub(crate) fn matches_for_stateless_pseudo_element(&self) -> bool {
debug_assert!(
self.maybe_allowed_after_pseudo_element(),
"Someone messed up pseudo-element parsing: {:?}",
*self
);
match *self {
Component::Negation(ref components) => {
!components.iter().all(|c| c.matches_for_stateless_pseudo_element())
},
Component::Is(ref selectors) | Component::Where(ref selectors) => {
selectors.iter().any(|selector| {
selector.iter_raw_match_order().all(|c| c.matches_for_stateless_pseudo_element())
})
},
_ => false,
}
}

pub fn visit<V>(&self, visitor: &mut V) -> bool
where
V: SelectorVisitor<Impl = Impl>,
Expand Down
28 changes: 17 additions & 11 deletions components/style/invalidation/element/invalidator.rs
Expand Up @@ -871,23 +871,29 @@ where
// This will usually be the very next component, except for
// the fact that we store compound selectors the other way
// around, so there could also be state pseudo-classes.
let pseudo_selector = next_invalidation
let pseudo = next_invalidation
.dependency
.selector
.iter_raw_parse_order_from(next_invalidation.offset)
.skip_while(|c| matches!(**c, Component::NonTSPseudoClass(..)))
.flat_map(|c| {
if let Component::PseudoElement(ref pseudo) = *c {
return Some(pseudo);
}

// TODO: Would be nice to make this a diagnostic_assert! of
// sorts.
debug_assert!(
c.maybe_allowed_after_pseudo_element(),
"Someone seriously messed up selector parsing: \
{:?} at offset {:?}: {:?}",
next_invalidation.dependency, next_invalidation.offset, c,
);

None
})
.next()
.unwrap();

let pseudo = match *pseudo_selector {
Component::PseudoElement(ref pseudo) => pseudo,
_ => unreachable!(
"Someone seriously messed up selector parsing: \
{:?} at offset {:?}: {:?}",
next_invalidation.dependency, next_invalidation.offset, pseudo_selector,
),
};

// FIXME(emilio): This is not ideal, and could not be
// accurate if we ever have stateful element-backed eager
// pseudos.
Expand Down

0 comments on commit bd23e05

Please sign in to comment.