diff --git a/servo/components/style/dom_apis.rs b/servo/components/style/dom_apis.rs index 8f764dc8958d..685b5b4e8a03 100644 --- a/servo/components/style/dom_apis.rs +++ b/servo/components/style/dom_apis.rs @@ -9,6 +9,7 @@ use crate::context::QuirksMode; use crate::dom::{TDocument, TElement, TNode, TShadowRoot}; use crate::invalidation::element::invalidator::{DescendantInvalidationLists, Invalidation}; use crate::invalidation::element::invalidator::{InvalidationProcessor, InvalidationVector}; +use crate::invalidation::element::invalidation_map::Dependency; use crate::Atom; use selectors::attr::CaseSensitivity; use selectors::matching::{self, MatchingContext, MatchingMode}; @@ -130,7 +131,7 @@ where { results: &'a mut Q::Output, matching_context: MatchingContext<'a, E::Impl>, - selector_list: &'a SelectorList, + dependencies: &'a [Dependency], } impl<'a, E, Q> InvalidationProcessor<'a, E> for QuerySelectorProcessor<'a, E, Q> @@ -143,6 +144,11 @@ where true } + fn check_outer_dependency(&mut self, _: &Dependency, _: E) -> bool { + debug_assert!(false, "How? We should only have parent-less dependencies here!"); + true + } + fn collect_invalidations( &mut self, element: E, @@ -171,11 +177,10 @@ where self_invalidations }; - for selector in self.selector_list.0.iter() { + for dependency in self.dependencies.iter() { target_vector.push(Invalidation::new( - selector, + dependency, self.matching_context.current_host.clone(), - 0, )) } @@ -642,10 +647,13 @@ pub fn query_selector( if root_element.is_some() || !invalidation_may_be_useful { query_selector_slow::(root, selector_list, results, &mut matching_context); } else { + let dependencies = selector_list.0.iter().map(|selector| { + Dependency::for_full_selector_invalidation(selector.clone()) + }).collect::>(); let mut processor = QuerySelectorProcessor:: { results, matching_context, - selector_list, + dependencies: &dependencies, }; for node in root.dom_children() { diff --git a/servo/components/style/invalidation/element/document_state.rs b/servo/components/style/invalidation/element/document_state.rs index 7fc51338b1ed..4659eb42f5ee 100644 --- a/servo/components/style/invalidation/element/document_state.rs +++ b/servo/components/style/invalidation/element/document_state.rs @@ -6,6 +6,7 @@ use crate::dom::TElement; use crate::element_state::DocumentState; +use crate::invalidation::element::invalidation_map::Dependency; use crate::invalidation::element::invalidator::{DescendantInvalidationLists, InvalidationVector}; use crate::invalidation::element::invalidator::{Invalidation, InvalidationProcessor}; use crate::invalidation::element::state_and_attributes; @@ -65,6 +66,11 @@ where E: TElement, I: Iterator, { + fn check_outer_dependency(&mut self, _: &Dependency, _: E) -> bool { + debug_assert!(false, "how, we should only have parent-less dependencies here!"); + true + } + fn collect_invalidations( &mut self, _element: E, @@ -81,10 +87,14 @@ where // We pass `None` as a scope, as document state selectors aren't // affected by the current scope. + // + // FIXME(emilio): We should really pass the relevant host for + // self.rules, so that we invalidate correctly if the selector + // happens to have something like :host(:-moz-window-inactive) + // for example. self_invalidations.push(Invalidation::new( - &dependency.selector, + &dependency.dependency, /* scope = */ None, - 0, )); } } diff --git a/servo/components/style/invalidation/element/invalidation_map.rs b/servo/components/style/invalidation/element/invalidation_map.rs index f9195cb4cd28..b61475c7687f 100644 --- a/servo/components/style/invalidation/element/invalidation_map.rs +++ b/servo/components/style/invalidation/element/invalidation_map.rs @@ -57,7 +57,7 @@ pub struct Dependency { /// /// We'd generate: /// - /// * One dependency for .quz (offset: 0, parent: None) + /// * One dependency for .qux (offset: 0, parent: None) /// * One dependency for .baz pointing to B with parent being a /// dependency pointing to C. /// * One dependency from .bar pointing to C (parent: None) @@ -88,6 +88,22 @@ pub enum DependencyInvalidationKind { } impl Dependency { + /// Creates a dummy dependency to invalidate the whole selector. + /// + /// This is necessary because document state invalidation wants to + /// invalidate all elements in the document. + /// + /// The offset is such as that Invalidation::new(self) returns a zero + /// offset. That is, it points to a virtual "combinator" outside of the + /// selector, so calling combinator() on such a dependency will panic. + pub fn for_full_selector_invalidation(selector: Selector) -> Self { + Self { + selector_offset: selector.len() + 1, + selector, + parent: None, + } + } + /// Returns the combinator to the right of the partial selector this /// dependency represents. /// @@ -147,14 +163,14 @@ impl SelectorMapEntry for StateDependency { /// The same, but for document state selectors. #[derive(Clone, Debug, MallocSizeOf)] pub struct DocumentStateDependency { - /// The selector that is affected. We don't need to track an offset, since - /// when it changes it changes for the whole document anyway. + /// We track `Dependency` even though we don't need to track an offset, + /// since when it changes it changes for the whole document anyway. #[cfg_attr( feature = "gecko", ignore_malloc_size_of = "CssRules have primary refs, we measure there" )] #[cfg_attr(feature = "servo", ignore_malloc_size_of = "Arc")] - pub selector: Selector, + pub dependency: Dependency, /// The state this dependency is affected by. pub state: DocumentState, } @@ -269,7 +285,7 @@ impl InvalidationMap { if !document_state.is_empty() { let dep = DocumentStateDependency { state: document_state, - selector: selector.clone(), + dependency: Dependency::for_full_selector_invalidation(selector.clone()), }; self.document_state_selectors.try_push(dep)?; } diff --git a/servo/components/style/invalidation/element/invalidator.rs b/servo/components/style/invalidation/element/invalidator.rs index f0f55595dff4..4f5d005c120a 100644 --- a/servo/components/style/invalidation/element/invalidator.rs +++ b/servo/components/style/invalidation/element/invalidator.rs @@ -7,10 +7,10 @@ use crate::context::StackLimitChecker; use crate::dom::{TElement, TNode, TShadowRoot}; -use crate::selector_parser::SelectorImpl; +use crate::invalidation::element::invalidation_map::{Dependency, DependencyInvalidationKind}; use selectors::matching::matches_compound_selector_from; use selectors::matching::{CompoundSelectorMatchingResult, MatchingContext}; -use selectors::parser::{Combinator, Component, Selector}; +use selectors::parser::{Combinator, Component}; use selectors::OpaqueElement; use smallvec::SmallVec; use std::fmt; @@ -34,6 +34,27 @@ where false } + /// When a dependency from a :where or :is selector matches, it may still be + /// the case that we don't need to invalidate the full style. Consider the + /// case of: + /// + /// div .foo:where(.bar *, .baz) .qux + /// + /// We can get to the `*` part after a .bar class change, but you only need + /// to restyle the element if it also matches .foo. + /// + /// Similarly, you only need to restyle .baz if the whole result of matching + /// the selector changes. + /// + /// This function is called to check the result of matching the "outer" + /// dependency that we generate for the parent of the `:where` selector, + /// that is, in the case above it should match + /// `div .foo:where(.bar *, .baz)`. + /// + /// Returning true unconditionally here is over-optimistic and may + /// over-invalidate. + fn check_outer_dependency(&mut self, dependency: &Dependency, element: E) -> bool; + /// The matching context that should be used to process invalidations. fn matching_context(&mut self) -> &mut MatchingContext<'a, E::Impl>; @@ -127,7 +148,11 @@ enum InvalidationKind { /// relative to a current element we are processing, must be restyled. #[derive(Clone)] pub struct Invalidation<'a> { - selector: &'a Selector, + /// The dependency that generated this invalidation. + /// + /// Note that the offset inside the dependency is not really useful after + /// construction. + dependency: &'a Dependency, /// The right shadow host from where the rule came from, if any. /// /// This is needed to ensure that we match the selector with the right @@ -138,6 +163,8 @@ pub struct Invalidation<'a> { /// /// This order is a "parse order" offset, that is, zero is the leftmost part /// of the selector written as parsed / serialized. + /// + /// It is initialized from the offset from `dependency`. offset: usize, /// Whether the invalidation was already matched by any previous sibling or /// ancestor. @@ -149,16 +176,21 @@ pub struct Invalidation<'a> { } impl<'a> Invalidation<'a> { - /// Create a new invalidation for a given selector and offset. + /// Create a new invalidation for matching a dependency. pub fn new( - selector: &'a Selector, + dependency: &'a Dependency, scope: Option, - offset: usize, ) -> Self { + debug_assert!( + dependency.selector_offset == dependency.selector.len() + 1 || + dependency.invalidation_kind() != DependencyInvalidationKind::Element, + "No point to this, if the dependency matched the element we should just invalidate it" + ); Self { - selector, + dependency, scope, - offset, + // + 1 to go past the combinator. + offset: dependency.selector.len() + 1 - dependency.selector_offset, matched_by_any_previous: false, } } @@ -174,7 +206,7 @@ impl<'a> Invalidation<'a> { // for the weird pseudos in . // // We should be able to do better here! - match self.selector.combinator_at_parse_order(self.offset - 1) { + match self.dependency.selector.combinator_at_parse_order(self.offset - 1) { Combinator::Descendant | Combinator::LaterSibling | Combinator::PseudoElement => true, Combinator::Part | Combinator::SlotAssignment | @@ -188,7 +220,7 @@ impl<'a> Invalidation<'a> { return InvalidationKind::Descendant(DescendantInvalidationKind::Dom); } - match self.selector.combinator_at_parse_order(self.offset - 1) { + match self.dependency.selector.combinator_at_parse_order(self.offset - 1) { Combinator::Child | Combinator::Descendant | Combinator::PseudoElement => { InvalidationKind::Descendant(DescendantInvalidationKind::Dom) }, @@ -206,7 +238,7 @@ impl<'a> fmt::Debug for Invalidation<'a> { use cssparser::ToCss; f.write_str("Invalidation(")?; - for component in self.selector.iter_raw_parse_order_from(self.offset) { + for component in self.dependency.selector.iter_raw_parse_order_from(self.offset) { if matches!(*component, Component::Combinator(..)) { break; } @@ -763,201 +795,241 @@ where context.current_host = invalidation.scope; matches_compound_selector_from( - &invalidation.selector, + &invalidation.dependency.selector, invalidation.offset, context, &self.element, ) }; - let mut invalidated_self = false; - let mut matched = false; - match matching_result { + let next_invalidation = match matching_result { + CompoundSelectorMatchingResult::NotMatched => { + return SingleInvalidationResult { + invalidated_self: false, + matched: false, + } + }, CompoundSelectorMatchingResult::FullyMatched => { debug!(" > Invalidation matched completely"); - matched = true; - invalidated_self = true; + // We matched completely. If we're an inner selector now we need + // to go outside our selector and carry on invalidating. + let mut cur_dependency = invalidation.dependency; + loop { + cur_dependency = match cur_dependency.parent { + None => return SingleInvalidationResult { + invalidated_self: true, + matched: true, + }, + Some(ref p) => &**p, + }; + + debug!(" > Checking outer dependency {:?}", cur_dependency); + + // The inner selector changed, now check if the full + // previous part of the selector did, before keeping + // checking for descendants. + if !self.processor.check_outer_dependency(cur_dependency, self.element) { + return SingleInvalidationResult { + invalidated_self: false, + matched: false, + } + } + + if cur_dependency.invalidation_kind() == DependencyInvalidationKind::Element { + continue; + } + + debug!(" > Generating invalidation"); + break Invalidation::new(cur_dependency, invalidation.scope) + } }, CompoundSelectorMatchingResult::Matched { next_combinator_offset, } => { - let next_combinator = invalidation - .selector - .combinator_at_parse_order(next_combinator_offset); - matched = true; - - if matches!(next_combinator, Combinator::PseudoElement) { - // 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 = invalidation - .selector - .iter_raw_parse_order_from(next_combinator_offset + 1) - .skip_while(|c| matches!(**c, Component::NonTSPseudoClass(..))) - .next() - .unwrap(); - - let pseudo = match *pseudo_selector { - Component::PseudoElement(ref pseudo) => pseudo, - _ => unreachable!( - "Someone seriously messed up selector parsing: \ - {:?} at offset {:?}: {:?}", - invalidation.selector, next_combinator_offset, pseudo_selector, - ), - }; - - // FIXME(emilio): This is not ideal, and could not be - // accurate if we ever have stateful element-backed eager - // pseudos. - // - // Ideally, we'd just remove element-backed eager pseudos - // altogether, given they work fine without it. Only gotcha - // is that we wouldn't style them in parallel, which may or - // may not be an issue. - // - // Also, this could be more fine grained now (perhaps a - // RESTYLE_PSEUDOS hint?). - // - // Note that we'll also restyle the pseudo-element because - // it would match this invalidation. - if self.processor.invalidates_on_eager_pseudo_element() { - if pseudo.is_eager() { - invalidated_self = true; - } - // If we start or stop matching some marker rules, and - // don't have a marker, then we need to restyle the - // element to potentially create one. - // - // Same caveats as for other eager pseudos apply, this - // could be more fine-grained. - if pseudo.is_marker() && self.element.marker_pseudo_element().is_none() { - invalidated_self = true; - } - - // FIXME: ::selection doesn't generate elements, so the - // regular invalidation doesn't work for it. We store - // the cached selection style holding off the originating - // element, so we need to restyle it in order to invalidate - // it. This is still not quite correct, since nothing - // triggers a repaint necessarily, but matches old Gecko - // behavior, and the ::selection implementation needs to - // change significantly anyway to implement - // https://github.com/w3c/csswg-drafts/issues/2474. - if pseudo.is_selection() { - invalidated_self = true; - } - } - } - - let next_invalidation = Invalidation { - selector: invalidation.selector, + Invalidation { + dependency: invalidation.dependency, scope: invalidation.scope, offset: next_combinator_offset + 1, matched_by_any_previous: false, - }; - - debug!( - " > Invalidation matched, next: {:?}, ({:?})", - next_invalidation, next_combinator - ); - - let next_invalidation_kind = next_invalidation.kind(); - - // We can skip pushing under some circumstances, and we should - // because otherwise the invalidation list could grow - // exponentially. - // - // * First of all, both invalidations need to be of the same - // kind. This is because of how we propagate them going to - // the right of the tree for sibling invalidations and going - // down the tree for children invalidations. A sibling - // invalidation that ends up generating a children - // invalidation ends up (correctly) in five different lists, - // not in the same list five different times. - // - // * Then, the invalidation needs to be matched by a previous - // ancestor/sibling, in order to know that this invalidation - // has been generated already. - // - // * Finally, the new invalidation needs to be - // `effective_for_next()`, in order for us to know that it is - // still in the list, since we remove the dependencies that - // aren't from the lists for our children / siblings. - // - // To go through an example, let's imagine we are processing a - // dom subtree like: - // - //
- // - // And an invalidation list with a single invalidation like: - // - // [div div div] - // - // When we process the invalidation list for the outer div, we - // match it, and generate a `div div` invalidation, so for the - //
child we have: - // - // [div div div, div div] - // - // With the first of them marked as `matched`. - // - // When we process the
child, we don't match any of - // them, so both invalidations go untouched to our children. - // - // When we process the second
, we match _both_ - // invalidations. - // - // However, when matching the first, we can tell it's been - // matched, and not push the corresponding `div div` - // invalidation, since we know it's necessarily already on the - // list. - // - // Thus, without skipping the push, we'll arrive to the - // innermost
with: - // - // [div div div, div div, div div, div] - // - // While skipping it, we won't arrive here with duplicating - // dependencies: - // - // [div div div, div div, div] - // - let can_skip_pushing = next_invalidation_kind == invalidation_kind && - invalidation.matched_by_any_previous && - next_invalidation.effective_for_next(); - - if can_skip_pushing { - debug!( - " > Can avoid push, since the invalidation had \ - already been matched before" - ); - } else { - match next_invalidation_kind { - InvalidationKind::Descendant(DescendantInvalidationKind::Dom) => { - descendant_invalidations - .dom_descendants - .push(next_invalidation); - }, - InvalidationKind::Descendant(DescendantInvalidationKind::Part) => { - descendant_invalidations.parts.push(next_invalidation); - }, - InvalidationKind::Descendant(DescendantInvalidationKind::Slotted) => { - descendant_invalidations - .slotted_descendants - .push(next_invalidation); - }, - InvalidationKind::Sibling => { - sibling_invalidations.push(next_invalidation); - }, - } } }, - CompoundSelectorMatchingResult::NotMatched => {}, + }; + + debug_assert_ne!( + next_invalidation.offset, + 0, + "Rightmost selectors shouldn't generate more invalidations", + ); + + let mut invalidated_self = false; + let next_combinator = next_invalidation + .dependency + .selector + .combinator_at_parse_order(next_invalidation.offset - 1); + + if matches!(next_combinator, Combinator::PseudoElement) { + // 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 + .dependency + .selector + .iter_raw_parse_order_from(next_invalidation.offset) + .skip_while(|c| matches!(**c, Component::NonTSPseudoClass(..))) + .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. + // + // Ideally, we'd just remove element-backed eager pseudos + // altogether, given they work fine without it. Only gotcha + // is that we wouldn't style them in parallel, which may or + // may not be an issue. + // + // Also, this could be more fine grained now (perhaps a + // RESTYLE_PSEUDOS hint?). + // + // Note that we'll also restyle the pseudo-element because + // it would match this invalidation. + if self.processor.invalidates_on_eager_pseudo_element() { + if pseudo.is_eager() { + invalidated_self = true; + } + // If we start or stop matching some marker rules, and + // don't have a marker, then we need to restyle the + // element to potentially create one. + // + // Same caveats as for other eager pseudos apply, this + // could be more fine-grained. + if pseudo.is_marker() && self.element.marker_pseudo_element().is_none() { + invalidated_self = true; + } + + // FIXME: ::selection doesn't generate elements, so the + // regular invalidation doesn't work for it. We store + // the cached selection style holding off the originating + // element, so we need to restyle it in order to invalidate + // it. This is still not quite correct, since nothing + // triggers a repaint necessarily, but matches old Gecko + // behavior, and the ::selection implementation needs to + // change significantly anyway to implement + // https://github.com/w3c/csswg-drafts/issues/2474. + if pseudo.is_selection() { + invalidated_self = true; + } + } + } + + debug!( + " > Invalidation matched, next: {:?}, ({:?})", + next_invalidation, next_combinator + ); + + let next_invalidation_kind = next_invalidation.kind(); + + // We can skip pushing under some circumstances, and we should + // because otherwise the invalidation list could grow + // exponentially. + // + // * First of all, both invalidations need to be of the same + // kind. This is because of how we propagate them going to + // the right of the tree for sibling invalidations and going + // down the tree for children invalidations. A sibling + // invalidation that ends up generating a children + // invalidation ends up (correctly) in five different lists, + // not in the same list five different times. + // + // * Then, the invalidation needs to be matched by a previous + // ancestor/sibling, in order to know that this invalidation + // has been generated already. + // + // * Finally, the new invalidation needs to be + // `effective_for_next()`, in order for us to know that it is + // still in the list, since we remove the dependencies that + // aren't from the lists for our children / siblings. + // + // To go through an example, let's imagine we are processing a + // dom subtree like: + // + //
+ // + // And an invalidation list with a single invalidation like: + // + // [div div div] + // + // When we process the invalidation list for the outer div, we + // match it, and generate a `div div` invalidation, so for the + //
child we have: + // + // [div div div, div div] + // + // With the first of them marked as `matched`. + // + // When we process the
child, we don't match any of + // them, so both invalidations go untouched to our children. + // + // When we process the second
, we match _both_ + // invalidations. + // + // However, when matching the first, we can tell it's been + // matched, and not push the corresponding `div div` + // invalidation, since we know it's necessarily already on the + // list. + // + // Thus, without skipping the push, we'll arrive to the + // innermost
with: + // + // [div div div, div div, div div, div] + // + // While skipping it, we won't arrive here with duplicating + // dependencies: + // + // [div div div, div div, div] + // + let can_skip_pushing = next_invalidation_kind == invalidation_kind && + invalidation.matched_by_any_previous && + next_invalidation.effective_for_next(); + + if can_skip_pushing { + debug!( + " > Can avoid push, since the invalidation had \ + already been matched before" + ); + } else { + match next_invalidation_kind { + InvalidationKind::Descendant(DescendantInvalidationKind::Dom) => { + descendant_invalidations + .dom_descendants + .push(next_invalidation); + }, + InvalidationKind::Descendant(DescendantInvalidationKind::Part) => { + descendant_invalidations.parts.push(next_invalidation); + }, + InvalidationKind::Descendant(DescendantInvalidationKind::Slotted) => { + descendant_invalidations + .slotted_descendants + .push(next_invalidation); + }, + InvalidationKind::Sibling => { + sibling_invalidations.push(next_invalidation); + }, + } } SingleInvalidationResult { invalidated_self, - matched, + matched: true, } } } diff --git a/servo/components/style/invalidation/element/state_and_attributes.rs b/servo/components/style/invalidation/element/state_and_attributes.rs index f6fa933e44e7..828bc99ed20b 100644 --- a/servo/components/style/invalidation/element/state_and_attributes.rs +++ b/servo/components/style/invalidation/element/state_and_attributes.rs @@ -85,8 +85,8 @@ pub fn check_dependency( dependency: &Dependency, element: &E, wrapper: &W, - context: &mut MatchingContext<'_, SelectorImpl>, -) + mut context: &mut MatchingContext<'_, E::Impl>, +) -> bool where E: TElement, W: selectors::Element, @@ -166,6 +166,14 @@ where true } + fn check_outer_dependency(&mut self, dependency: &Dependency, element: E) -> bool { + // We cannot assert about `element` having a snapshot here (in fact it + // most likely won't), because it may be an arbitrary descendant or + // later-sibling of the element we started invalidating with. + let wrapper = ElementWrapper::new(element, &*self.shared_context.snapshot_map); + check_dependency(dependency, &element, &wrapper, &mut self.matching_context) + } + fn matching_context(&mut self) -> &mut MatchingContext<'a, E::Impl> { &mut self.matching_context } @@ -447,7 +455,7 @@ where /// Check whether a dependency should be taken into account. #[inline] fn check_dependency(&mut self, dependency: &Dependency) -> bool { - check_dependency(&self.element, &self.wrapper, &mut self.matching_context) + check_dependency(dependency, &self.element, &self.wrapper, &mut self.matching_context) } fn scan_dependency(&mut self, dependency: &'selectors Dependency) { @@ -484,9 +492,8 @@ where debug_assert_ne!(dependency.selector_offset, dependency.selector.len()); let invalidation = Invalidation::new( - &dependency.selector, + &dependency, self.matching_context.current_host.clone(), - dependency.selector.len() - dependency.selector_offset + 1, ); match invalidation_kind {