diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index 66867000fda..606fd6f479d 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -169,27 +169,6 @@ impl SelectorInner { } } - /// Creates a clone of this selector with everything to the left of - /// (and including) the rightmost ancestor combinator removed. So - /// the selector |span foo > bar + baz| will become |bar + baz|. - /// This is used for revalidation selectors in servo. - /// - /// The bloom filter hashes are copied, even though they correspond to - /// parts of the selector that have been stripped out, because they are - /// still useful for fast-rejecting the reduced selectors. - pub fn slice_to_first_ancestor_combinator(&self) -> Self { - let maybe_pos = self.complex.iter_raw() - .position(|s| s.as_combinator() - .map_or(false, |c| c.is_ancestor())); - match maybe_pos { - None => self.clone(), - Some(index) => SelectorInner { - complex: self.complex.slice_to(index), - ancestor_hashes: self.ancestor_hashes.clone(), - }, - } - } - /// Creates a SelectorInner from a Vec of Components. Used in tests. pub fn from_vec(vec: Vec>) -> Self { let complex = ComplexSelector::from_vec(vec); diff --git a/components/style/stylist.rs b/components/style/stylist.rs index a5dce393e29..cda795d559c 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -27,7 +27,8 @@ use selectors::Element; use selectors::bloom::BloomFilter; use selectors::matching::{AFFECTED_BY_STYLE_ATTRIBUTE, AFFECTED_BY_PRESENTATIONAL_HINTS}; use selectors::matching::{ElementSelectorFlags, StyleRelations, matches_selector}; -use selectors::parser::{Component, Selector, SelectorInner, SelectorMethods, LocalName as LocalNameSelector}; +use selectors::parser::{Combinator, Component, Selector, SelectorInner, SelectorIter}; +use selectors::parser::{SelectorMethods, LocalName as LocalNameSelector}; use selectors::visitor::SelectorVisitor; use shared_lock::{Locked, SharedRwLockReadGuard, StylesheetGuards}; use sink::Push; @@ -375,10 +376,7 @@ impl Stylist { #[inline] fn note_for_revalidation(&mut self, selector: &Selector) { if needs_revalidation(selector) { - // For revalidation, we can skip everything left of the first ancestor - // combinator. - let revalidation_sel = selector.inner.slice_to_first_ancestor_combinator(); - self.selectors_for_cache_revalidation.push(revalidation_sel); + self.selectors_for_cache_revalidation.push(selector.inner.clone()); } } @@ -923,18 +921,47 @@ impl Drop for Stylist { /// Visitor determine whether a selector requires cache revalidation. /// /// Note that we just check simple selectors and eagerly return when the first -/// need for revalidation is found, so we don't need to store state on the visitor. +/// need for revalidation is found, so we don't need to store state on the +/// visitor. +/// +/// Also, note that it's important to check the whole selector, due to cousins +/// sharing arbitrarily deep in the DOM, not just the rightmost part of it +/// (unfortunately, though). +/// +/// With cousin sharing, we not only need to care about selectors in stuff like +/// foo:first-child, but also about selectors like p:first-child foo, since the +/// two parents may have shared style, and in that case we can test cousins +/// whose matching depends on the selector up in the chain. +/// +/// TODO(emilio): We can optimize when matching only siblings to only match the +/// rightmost selector until a descendant combinator is found, I guess, and in +/// general when we're sharing at depth `n`, to the `n + 1` sequences of +/// descendant combinators. +/// +/// I don't think that in presence of the bloom filter it's worth it, though. struct RevalidationVisitor; impl SelectorVisitor for RevalidationVisitor { type Impl = SelectorImpl; - /// Check whether a rightmost sequence of simple selectors containing this - /// simple selector to be explicitly matched against both the style sharing - /// cache entry and the candidate. + + fn visit_complex_selector(&mut self, + _: SelectorIter, + combinator: Option) -> bool { + let is_sibling_combinator = + combinator.map_or(false, |c| c.is_sibling()); + + !is_sibling_combinator + } + + + /// Check whether sequence of simple selectors containing this simple + /// selector to be explicitly matched against both the style sharing cache + /// entry and the candidate. /// - /// We use this for selectors that can have different matching behavior between - /// siblings that are otherwise identical as far as the cache is concerned. + /// We use this for selectors that can have different matching behavior + /// between siblings that are otherwise identical as far as the cache is + /// concerned. fn visit_simple_selector(&mut self, s: &Component) -> bool { match *s { Component::AttrExists(_) | @@ -970,23 +997,7 @@ impl SelectorVisitor for RevalidationVisitor { /// Returns true if the given selector needs cache revalidation. pub fn needs_revalidation(selector: &Selector) -> bool { let mut visitor = RevalidationVisitor; - - // We only need to consider the rightmost sequence of simple selectors, so - // we can stop at the first combinator. This is because: - // * If it's an ancestor combinator, we can ignore everything to the left - // because matching won't differ between siblings. - // * If it's a sibling combinator, then we know we need revalidation. - let mut iter = selector.inner.complex.iter(); - for ss in &mut iter { - if !ss.visit(&mut visitor) { - return true; - } - } - - // If none of the simple selectors in the rightmost sequence required - // revalidation, we need revalidation if and only if the combinator is a - // sibling combinator. - iter.next_sequence().map_or(false, |c| c.is_sibling()) + !selector.visit(&mut visitor) } /// Map that contains the CSS rules for a specific PseudoElement diff --git a/tests/unit/style/stylist.rs b/tests/unit/style/stylist.rs index 4f7ba303e08..ea89dfde06b 100644 --- a/tests/unit/style/stylist.rs +++ b/tests/unit/style/stylist.rs @@ -109,13 +109,11 @@ fn test_revalidation_selectors() { "span + div", "span ~ div", - // Revalidation selectors that will get sliced. - "td > h1[dir]", - "td > span + h1[dir]", - "table td > span + div ~ h1[dir]", + // Selectors in the ancestor chain (needed for cousin sharing). + "p:first-child span", ]).into_iter() .filter(|s| needs_revalidation(&s)) - .map(|s| s.inner.slice_to_first_ancestor_combinator().complex) + .map(|s| s.inner.complex) .collect::>(); let reference = parse_selectors(&[ @@ -147,10 +145,8 @@ fn test_revalidation_selectors() { "span + div", "span ~ div", - // Revalidation selectors that got sliced. - "h1[dir]", - "span + h1[dir]", - "span + div ~ h1[dir]", + // Selectors in the ancestor chain (needed for cousin sharing). + "p:first-child span", ]).into_iter() .map(|s| s.inner.complex) .collect::>(); diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json index 5f70f04ee64..5b451022e62 100644 --- a/tests/wpt/mozilla/meta/MANIFEST.json +++ b/tests/wpt/mozilla/meta/MANIFEST.json @@ -1211,6 +1211,18 @@ {} ] ], + "css/bug-1361013-cousin-sharing.html": [ + [ + "/_mozilla/css/bug-1361013-cousin-sharing.html", + [ + [ + "/_mozilla/css/bug-1361013-cousin-sharing-ref.html", + "==" + ] + ], + {} + ] + ], "css/button_css_width.html": [ [ "/_mozilla/css/button_css_width.html", @@ -7262,6 +7274,11 @@ {} ] ], + "css/bug-1361013-cousin-sharing-ref.html": [ + [ + {} + ] + ], "css/button_css_width_ref.html": [ [ {} @@ -20835,6 +20852,14 @@ "2ebf9c8f963a2f3971a3c1b64b6b01825eacdedc", "support" ], + "css/bug-1361013-cousin-sharing-ref.html": [ + "3451cb4533d650f6f624b89548ce7117bcc8bef3", + "support" + ], + "css/bug-1361013-cousin-sharing.html": [ + "95c8c8d33cca3d6e969338a98c60ec9804256ef5", + "reftest" + ], "css/bug_1345483.html": [ "41b55af9d5c95910b1af74c0fbffb24e20bbe869", "testharness" diff --git a/tests/wpt/mozilla/tests/css/bug-1361013-cousin-sharing-ref.html b/tests/wpt/mozilla/tests/css/bug-1361013-cousin-sharing-ref.html new file mode 100644 index 00000000000..9c06cce2a1a --- /dev/null +++ b/tests/wpt/mozilla/tests/css/bug-1361013-cousin-sharing-ref.html @@ -0,0 +1,8 @@ + + +
+

+

This should be green.

+
diff --git a/tests/wpt/mozilla/tests/css/bug-1361013-cousin-sharing.html b/tests/wpt/mozilla/tests/css/bug-1361013-cousin-sharing.html new file mode 100644 index 00000000000..44f78042bca --- /dev/null +++ b/tests/wpt/mozilla/tests/css/bug-1361013-cousin-sharing.html @@ -0,0 +1,13 @@ + + +Bug 1361013: selectors for revalidation account for cousins sharing style + + + +
+

+

This should be green.

+