Auto merge of #16364 - emilio:dynamic-not, r=bholley

style: Fix dynamic changes of attributes when combined with :not.

This makes the dependency tracker properly recurse into simple selectors inside the current complex selector to find the appropriate dependencies.

We can't still remove the outer visitor because we need it for stuff like `:not(.foo + bar)`, but I plan to get rid of it in a followup as long as try comes back green.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16364)
<!-- Reviewable:end -->

.travis.yml

@@ -22,6 +22,7 @@ matrix:
          - bash etc/ci/lockfile_changed.sh
          - bash etc/ci/manifest_changed.sh
          - ./mach cargo test -p selectors
+         - ./mach cargo test -p style
       cache:
         directories:
           - .cargo

components/selectors/parser.rs

@@ -145,7 +145,7 @@ impl<Impl: SelectorImpl> SelectorMethods for Selector<Impl> {
     }
 }
 
-impl<Impl: SelectorImpl> SelectorMethods for Arc<ComplexSelector<Impl>> {
+impl<Impl: SelectorImpl> SelectorMethods for ComplexSelector<Impl> {
     type Impl = Impl;
 
     fn visit<V>(&self, visitor: &mut V) -> bool
@@ -184,6 +184,9 @@ impl<Impl: SelectorImpl> SelectorMethods for SimpleSelector<Impl> {
         where V: SelectorVisitor<Impl = Impl>,
     {
         use self::SimpleSelector::*;
+        if !visitor.visit_simple_selector(self) {
+            return false;
+        }
 
         match *self {
             Negation(ref negated) => {
@@ -1161,7 +1164,7 @@ pub mod tests {
     impl SelectorMethods for PseudoClass {
         type Impl = DummySelectorImpl;
 
-        fn visit<V>(&self, visitor: &mut V) -> bool
+        fn visit<V>(&self, _visitor: &mut V) -> bool
             where V: SelectorVisitor<Impl = Self::Impl> { true }
     }
 
@@ -1498,4 +1501,26 @@ pub mod tests {
             specificity: specificity(1, 1, 0),
         }))));
     }
+
+    struct TestVisitor {
+        seen: Vec<String>,
+    }
+
+    impl SelectorVisitor for TestVisitor {
+        type Impl = DummySelectorImpl;
+
+        fn visit_simple_selector(&mut self, s: &SimpleSelector<DummySelectorImpl>) -> bool {
+            let mut dest = String::new();
+            s.to_css(&mut dest).unwrap();
+            self.seen.push(dest);
+            true
+        }
+    }
+
+    #[test]
+    fn visitor() {
+        let mut test_visitor = TestVisitor { seen: vec![], };
+        parse(":not(:hover) ~ label").unwrap().0[0].visit(&mut test_visitor);
+        assert!(test_visitor.seen.contains(&":hover".into()));
+    }
 }

components/selectors/visitor.rs

@@ -6,8 +6,7 @@
 
 #![deny(missing_docs)]
 
-use parser::{AttrSelector, Combinator, ComplexSelector, SelectorImpl};
+use parser::{AttrSelector, Combinator, ComplexSelector, SelectorImpl, SimpleSelector};
-use std::sync::Arc;
 
 /// A trait to visit selector properties.
 ///
@@ -24,12 +23,17 @@ pub trait SelectorVisitor {
         true
     }
 
+    /// Visit a simple selector.
+    fn visit_simple_selector(&mut self, _: &SimpleSelector<Self::Impl>) -> bool {
+        true
+    }
+
     /// Visits a complex selector.
     ///
     /// Gets the combinator to the right of the selector, or `None` if the
     /// selector is the leftmost one.
     fn visit_complex_selector(&mut self,
-                              _: &Arc<ComplexSelector<Self::Impl>>,
+                              _: &ComplexSelector<Self::Impl>,
                               _combinator_to_right: Option<Combinator>)
                               -> bool {
         true

components/style/gecko/selector_parser.rs

@@ -8,7 +8,6 @@ use cssparser::{Parser, ToCss};
 use element_state::ElementState;
 use gecko_bindings::structs::CSSPseudoClassType;
 use gecko_bindings::structs::nsIAtom;
-use restyle_hints::complex_selector_to_state;
 use selector_parser::{SelectorParser, PseudoElementCascadeType};
 use selectors::parser::{ComplexSelector, SelectorMethods};
 use selectors::visitor::SelectorVisitor;
@@ -313,11 +312,7 @@ impl NonTSPseudoClass {
                 match *self {
                     $(NonTSPseudoClass::$name => flag!($state),)*
                     $(NonTSPseudoClass::$s_name(..) => flag!($s_state),)*
-                    NonTSPseudoClass::MozAny(ref selectors) => {
+                    NonTSPseudoClass::MozAny(..) => ElementState::empty(),
-                        selectors.iter().fold(ElementState::empty(), |state, s| {
-                            state | complex_selector_to_state(s)
-                        })
-                    }
                 }
             }
         }

components/style/restyle_hints.rs

@@ -17,7 +17,7 @@ use selector_parser::{AttrValue, NonTSPseudoClass, Snapshot, SelectorImpl};
 use selectors::{Element, MatchAttr};
 use selectors::matching::{ElementSelectorFlags, StyleRelations};
 use selectors::matching::matches_complex_selector;
-use selectors::parser::{AttrSelector, Combinator, ComplexSelector, SimpleSelector};
+use selectors::parser::{AttrSelector, Combinator, ComplexSelector, SelectorMethods, SimpleSelector};
 use selectors::visitor::SelectorVisitor;
 use std::clone::Clone;
 use std::sync::Arc;
@@ -279,11 +279,26 @@ impl<'a, E> Element for ElementWrapper<'a, E>
     fn match_non_ts_pseudo_class<F>(&self,
                                     pseudo_class: &NonTSPseudoClass,
                                     relations: &mut StyleRelations,
-                                    _: &mut F)
+                                    _setter: &mut F)
                                     -> bool
         where F: FnMut(&Self, ElementSelectorFlags),
     {
-        let flag = SelectorImpl::pseudo_class_state_flag(pseudo_class);
+        // :moz-any is quite special, because we need to keep matching as a
+        // snapshot.
+        #[cfg(feature = "gecko")]
+        {
+            if let NonTSPseudoClass::MozAny(ref selectors) = *pseudo_class {
+                return selectors.iter().any(|s| {
+                    matches_complex_selector(s,
+                                             self,
+                                             None,
+                                             relations,
+                                             _setter)
+                })
+            }
+        }
+
+        let flag = pseudo_class.state_flag();
         if flag.is_empty() {
             return self.element.match_non_ts_pseudo_class(pseudo_class,
                                                           relations,
@@ -365,22 +380,9 @@ impl<'a, E> Element for ElementWrapper<'a, E>
     }
 }
 
-/// Returns the union of any `ElementState` flags for components of a
-/// `ComplexSelector`.
-pub fn complex_selector_to_state(sel: &ComplexSelector<SelectorImpl>) -> ElementState {
-    sel.compound_selector.iter().fold(ElementState::empty(), |state, s| {
-        state | selector_to_state(s)
-    })
-}
-
 fn selector_to_state(sel: &SimpleSelector<SelectorImpl>) -> ElementState {
     match *sel {
-        SimpleSelector::NonTSPseudoClass(ref pc) => SelectorImpl::pseudo_class_state_flag(pc),
+        SimpleSelector::NonTSPseudoClass(ref pc) => pc.state_flag(),
-        SimpleSelector::Negation(ref negated) => {
-            negated.iter().fold(ElementState::empty(), |state, s| {
-                state | complex_selector_to_state(s)
-            })
-        }
         _ => ElementState::empty(),
     }
 }
@@ -419,6 +421,8 @@ fn needs_cache_revalidation(sel: &SimpleSelector<SelectorImpl>) -> bool {
         SimpleSelector::FirstOfType |
         SimpleSelector::LastOfType |
         SimpleSelector::OnlyOfType => true,
+        // FIXME(emilio): This sets the "revalidation" flag for :any, which is
+        // probably expensive given we use it a lot in UA sheets.
         SimpleSelector::NonTSPseudoClass(ref p) => p.state_flag().is_empty(),
         _ => false,
     }
@@ -483,61 +487,38 @@ struct Dependency {
     sensitivities: Sensitivities,
 }
 
-/// A visitor struct that collects information for a given selector.
+
-///
+/// The following visitor visits all the simple selectors for a given complex
-/// This is the struct responsible of adding dependencies for a given complex
+/// selector, taking care of :not and :any combinators, collecting whether any
-/// selector.
+/// of them is sensitive to attribute or state changes.
-pub struct SelectorDependencyVisitor<'a> {
+struct SensitivitiesVisitor {
-    dependency_set: &'a mut DependencySet,
+    sensitivities: Sensitivities,
-    needs_cache_revalidation: bool,
+    hint: RestyleHint,
+    needs_revalidation: bool,
 }
 
-impl<'a> SelectorDependencyVisitor<'a> {
+impl SelectorVisitor for SensitivitiesVisitor {
-    /// Create a new `SelectorDependencyVisitor`.
-    pub fn new(dependency_set: &'a mut DependencySet) -> Self {
-        SelectorDependencyVisitor {
-            dependency_set: dependency_set,
-            needs_cache_revalidation: false,
-        }
-    }
-
-    /// Returns whether this visitor has encountered a simple selector that needs
-    /// cache revalidation.
-    pub fn needs_cache_revalidation(&self) -> bool {
-        self.needs_cache_revalidation
-    }
-}
-
-impl<'a> SelectorVisitor for SelectorDependencyVisitor<'a> {
     type Impl = SelectorImpl;
 
     fn visit_complex_selector(&mut self,
-                              selector: &Arc<ComplexSelector<SelectorImpl>>,
+                              _: &ComplexSelector<SelectorImpl>,
-                              combinator: Option<Combinator>)
+                              combinator: Option<Combinator>) -> bool {
-                              -> bool
+        self.hint |= combinator_to_restyle_hint(combinator);
-    {
+        self.needs_revalidation |= self.hint.contains(RESTYLE_LATER_SIBLINGS);
-        let mut sensitivities = Sensitivities::new();
+
-        for s in &selector.compound_selector {
+        true
-            sensitivities.states.insert(selector_to_state(s));
+    }
-            if !self.needs_cache_revalidation {
+
-                self.needs_cache_revalidation = needs_cache_revalidation(s);
+    fn visit_simple_selector(&mut self, s: &SimpleSelector<SelectorImpl>) -> bool {
-            }
+        self.sensitivities.states.insert(selector_to_state(s));
-            if !sensitivities.attrs {
+
-                sensitivities.attrs = is_attr_selector(s);
+        if !self.sensitivities.attrs {
-            }
+            self.sensitivities.attrs = is_attr_selector(s);
+            self.needs_revalidation = true;
         }
 
-        let hint = combinator_to_restyle_hint(combinator);
+        if !self.needs_revalidation {
-
+            self.needs_revalidation = needs_cache_revalidation(s);
-        self.needs_cache_revalidation |= sensitivities.attrs;
-        self.needs_cache_revalidation |= hint.intersects(RESTYLE_LATER_SIBLINGS);
-
-        if !sensitivities.is_empty() {
-            self.dependency_set.add_dependency(Dependency {
-                selector: selector.clone(),
-                hint: hint,
-                sensitivities: sensitivities,
-            });
         }
 
         true
@@ -574,6 +555,59 @@ impl DependencySet {
         }
     }
 
+    /// Adds a selector to this `DependencySet`, and returns whether it may need
+    /// cache revalidation, that is, whether two siblings of the same "shape"
+    /// may have different style due to this selector.
+    pub fn note_selector(&mut self,
+                         selector: &Arc<ComplexSelector<SelectorImpl>>)
+                         -> bool
+    {
+        let mut combinator = None;
+        let mut current = selector;
+
+        let mut needs_revalidation = false;
+
+        loop {
+            let mut sensitivities_visitor = SensitivitiesVisitor {
+                sensitivities: Sensitivities::new(),
+                hint: RestyleHint::empty(),
+                needs_revalidation: false,
+            };
+
+            for ss in &current.compound_selector {
+                ss.visit(&mut sensitivities_visitor);
+            }
+
+            needs_revalidation |= sensitivities_visitor.needs_revalidation;
+
+            let SensitivitiesVisitor {
+                sensitivities,
+                mut hint,
+                ..
+            } = sensitivities_visitor;
+
+            hint |= combinator_to_restyle_hint(combinator);
+
+            if !sensitivities.is_empty() {
+                self.add_dependency(Dependency {
+                    sensitivities: sensitivities,
+                    hint: hint,
+                    selector: current.clone(),
+                })
+            }
+
+            match current.next {
+                Some((ref next, next_combinator)) => {
+                    current = next;
+                    combinator = Some(next_combinator);
+                }
+                None => break,
+            }
+        }
+
+        needs_revalidation
+    }
+
     /// Create an empty `DependencySet`.
     pub fn new() -> Self {
         DependencySet {
@@ -650,7 +684,7 @@ impl DependencySet {
             debug_assert!((!state_changes.is_empty() && !dep.sensitivities.states.is_empty()) ||
                           (attrs_changed && dep.sensitivities.attrs),
                           "Testing a known ineffective dependency?");
-            if (attrs_changed || state_changes.intersects(dep.sensitivities.states)) && !hint.intersects(dep.hint) {
+            if (attrs_changed || state_changes.intersects(dep.sensitivities.states)) && !hint.contains(dep.hint) {
                 // We can ignore the selector flags, since they would have already been set during
                 // original matching for any element that might change its matching behavior here.
                 let matched_then =
@@ -671,3 +705,26 @@ impl DependencySet {
         }
     }
 }
+
+#[test]
+#[cfg(all(test, feature = "servo"))]
+fn smoke_restyle_hints() {
+    use cssparser::Parser;
+    use selector_parser::SelectorParser;
+    use stylesheets::{Origin, Namespaces};
+    let namespaces = Namespaces::default();
+    let parser = SelectorParser {
+        stylesheet_origin: Origin::Author,
+        namespaces: &namespaces,
+    };
+
+    let mut dependencies = DependencySet::new();
+
+    let mut p = Parser::new(":not(:active) ~ label");
+    let selector = Arc::new(ComplexSelector::parse(&parser, &mut p).unwrap());
+    dependencies.note_selector(&selector);
+    assert_eq!(dependencies.len(), 1);
+    assert_eq!(dependencies.state_deps.len(), 1);
+    assert!(!dependencies.state_deps[0].sensitivities.states.is_empty());
+    assert!(dependencies.state_deps[0].hint.contains(RESTYLE_LATER_SIBLINGS));
+}

components/style/stylist.rs

@@ -19,7 +19,7 @@ use properties::{self, CascadeFlags, ComputedValues};
 #[cfg(feature = "servo")]
 use properties::INHERIT_ALL;
 use properties::PropertyDeclarationBlock;
-use restyle_hints::{RestyleHint, DependencySet, SelectorDependencyVisitor};
+use restyle_hints::{RestyleHint, DependencySet};
 use rule_tree::{CascadeLevel, RuleTree, StrongRuleNode, StyleSource};
 use selector_parser::{SelectorImpl, PseudoElement, Snapshot};
 use selectors::Element;
@@ -28,7 +28,6 @@ use selectors::matching::{AFFECTED_BY_ANIMATIONS, AFFECTED_BY_TRANSITIONS};
 use selectors::matching::{AFFECTED_BY_STYLE_ATTRIBUTE, AFFECTED_BY_PRESENTATIONAL_HINTS};
 use selectors::matching::{ElementSelectorFlags, StyleRelations, matches_complex_selector};
 use selectors::parser::{Selector, SimpleSelector, LocalName as LocalNameSelector, ComplexSelector};
-use selectors::parser::SelectorMethods;
 use shared_lock::{Locked, SharedRwLockReadGuard, StylesheetGuards};
 use sink::Push;
 use smallvec::VecLike;
@@ -294,11 +293,9 @@ impl Stylist {
                     self.rules_source_order += 1;
 
                     for selector in &style_rule.selectors.0 {
-                        let mut visitor =
+                        let needs_cache_revalidation =
-                            SelectorDependencyVisitor::new(&mut self.state_deps);
+                            self.state_deps.note_selector(&selector.complex_selector);
-                        selector.visit(&mut visitor);
+                        if needs_cache_revalidation {
-
-                        if visitor.needs_cache_revalidation() {
                             self.selectors_for_cache_revalidation.push(selector.clone());
                         }
                     }

tests/wpt/mozilla/meta/MANIFEST.json

@@ -3743,6 +3743,18 @@
      {}
     ]
    ],
+   "css/negation-attr-dependence.html": [
+    [
+     "/_mozilla/css/negation-attr-dependence.html",
+     [
+      [
+       "/_mozilla/css/negation-attr-dependence-ref.html",
+       "=="
+      ]
+     ],
+     {}
+    ]
+   ],
    "css/negative-calc-cv.html": [
     [
      "/_mozilla/css/negative-calc-cv.html",
@@ -8299,6 +8311,11 @@
      {}
     ]
    ],
+   "css/negation-attr-dependence-ref.html": [
+    [
+     {}
+    ]
+   ],
    "css/negative-calc-cv-ref.html": [
     [
      {}
@@ -22501,6 +22518,14 @@
    "a82c94a9a4a18cda6009e467993cace8babe7591",
    "support"
   ],
+  "css/negation-attr-dependence-ref.html": [
+   "560c5d8b523f6d4cad6e15e5604f51646647d487",
+   "support"
+  ],
+  "css/negation-attr-dependence.html": [
+   "27bec4af590a2e6d5e88b54ed37fd254ef0f0a99",
+   "reftest"
+  ],
   "css/negative-calc-cv-ref.html": [
    "a42f34470ead45d5865562618f6538fde263a359",
    "support"

tests/wpt/mozilla/tests/css/negation-attr-dependence-ref.html

@@ -0,0 +1,8 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>CSS Test Reference</title>
+<style>
+  .foo + div { background: green; width: 100px; height: 100px; }
+</style>
+<div class="foo"></div>
+<div>Should be green</div>

tests/wpt/mozilla/tests/css/negation-attr-dependence.html

@@ -0,0 +1,15 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>CSS Test: Negation with attribute dependence correctly restyle siblings.</title>
+<link rel="match" href="negation-attr-dependence-ref.html">
+<style>
+  :not(.foo) + div { background: green; width: 100px; height: 100px; }
+</style>
+<link rel="matches" href="negation-attr-dependence-ref.html">
+<div class="foo"></div>
+<div>Should be green</div>
+<script>
+window.onload = function() {
+  document.querySelector('div').className = "";
+}
+</script>