diff --git a/.travis.yml b/.travis.yml index 52c6dc6a719..c9652e696a2 100644 --- a/.travis.yml +++ b/.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 diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index fe0b5079988..367220b27e8 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -145,7 +145,7 @@ impl SelectorMethods for Selector { } } -impl SelectorMethods for Arc> { +impl SelectorMethods for ComplexSelector { type Impl = Impl; fn visit(&self, visitor: &mut V) -> bool @@ -184,6 +184,9 @@ impl SelectorMethods for SimpleSelector { where V: SelectorVisitor, { 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(&self, visitor: &mut V) -> bool + fn visit(&self, _visitor: &mut V) -> bool where V: SelectorVisitor { true } } @@ -1498,4 +1501,26 @@ pub mod tests { specificity: specificity(1, 1, 0), })))); } + + struct TestVisitor { + seen: Vec, + } + + impl SelectorVisitor for TestVisitor { + type Impl = DummySelectorImpl; + + fn visit_simple_selector(&mut self, s: &SimpleSelector) -> 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())); + } } diff --git a/components/selectors/visitor.rs b/components/selectors/visitor.rs index 429c70c1cc8..2e0b52987f8 100644 --- a/components/selectors/visitor.rs +++ b/components/selectors/visitor.rs @@ -6,8 +6,7 @@ #![deny(missing_docs)] -use parser::{AttrSelector, Combinator, ComplexSelector, SelectorImpl}; -use std::sync::Arc; +use parser::{AttrSelector, Combinator, ComplexSelector, SelectorImpl, SimpleSelector}; /// 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) -> 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, _combinator_to_right: Option) -> bool { true diff --git a/components/style/gecko/selector_parser.rs b/components/style/gecko/selector_parser.rs index 18f56ed1228..adbd3a041bd 100644 --- a/components/style/gecko/selector_parser.rs +++ b/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) => { - selectors.iter().fold(ElementState::empty(), |state, s| { - state | complex_selector_to_state(s) - }) - } + NonTSPseudoClass::MozAny(..) => ElementState::empty(), } } } diff --git a/components/style/restyle_hints.rs b/components/style/restyle_hints.rs index 594246f4838..8256722494a 100644 --- a/components/style/restyle_hints.rs +++ b/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(&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) -> ElementState { - sel.compound_selector.iter().fold(ElementState::empty(), |state, s| { - state | selector_to_state(s) - }) -} - fn selector_to_state(sel: &SimpleSelector) -> ElementState { match *sel { - SimpleSelector::NonTSPseudoClass(ref pc) => SelectorImpl::pseudo_class_state_flag(pc), - SimpleSelector::Negation(ref negated) => { - negated.iter().fold(ElementState::empty(), |state, s| { - state | complex_selector_to_state(s) - }) - } + SimpleSelector::NonTSPseudoClass(ref pc) => pc.state_flag(), _ => ElementState::empty(), } } @@ -419,6 +421,8 @@ fn needs_cache_revalidation(sel: &SimpleSelector) -> 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. -/// -/// This is the struct responsible of adding dependencies for a given complex -/// selector. -pub struct SelectorDependencyVisitor<'a> { - dependency_set: &'a mut DependencySet, - needs_cache_revalidation: bool, + +/// The following visitor visits all the simple selectors for a given complex +/// selector, taking care of :not and :any combinators, collecting whether any +/// of them is sensitive to attribute or state changes. +struct SensitivitiesVisitor { + sensitivities: Sensitivities, + hint: RestyleHint, + needs_revalidation: bool, } -impl<'a> SelectorDependencyVisitor<'a> { - /// 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> { +impl SelectorVisitor for SensitivitiesVisitor { type Impl = SelectorImpl; fn visit_complex_selector(&mut self, - selector: &Arc>, - combinator: Option) - -> bool - { - let mut sensitivities = Sensitivities::new(); - for s in &selector.compound_selector { - sensitivities.states.insert(selector_to_state(s)); - if !self.needs_cache_revalidation { - self.needs_cache_revalidation = needs_cache_revalidation(s); - } - if !sensitivities.attrs { - sensitivities.attrs = is_attr_selector(s); - } + _: &ComplexSelector, + combinator: Option) -> bool { + self.hint |= combinator_to_restyle_hint(combinator); + self.needs_revalidation |= self.hint.contains(RESTYLE_LATER_SIBLINGS); + + true + } + + fn visit_simple_selector(&mut self, s: &SimpleSelector) -> bool { + self.sensitivities.states.insert(selector_to_state(s)); + + if !self.sensitivities.attrs { + self.sensitivities.attrs = is_attr_selector(s); + self.needs_revalidation = true; } - let hint = combinator_to_restyle_hint(combinator); - - 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, - }); + if !self.needs_revalidation { + self.needs_revalidation = needs_cache_revalidation(s); } 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>) + -> 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 ¤t.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)); +} diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 3d29ac68c2f..642eabd44bd 100644 --- a/components/style/stylist.rs +++ b/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 = - SelectorDependencyVisitor::new(&mut self.state_deps); - selector.visit(&mut visitor); - - if visitor.needs_cache_revalidation() { + let needs_cache_revalidation = + self.state_deps.note_selector(&selector.complex_selector); + if needs_cache_revalidation { self.selectors_for_cache_revalidation.push(selector.clone()); } } diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json index c8d994cd4db..c531e5e0762 100644 --- a/tests/wpt/mozilla/meta/MANIFEST.json +++ b/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" diff --git a/tests/wpt/mozilla/tests/css/negation-attr-dependence-ref.html b/tests/wpt/mozilla/tests/css/negation-attr-dependence-ref.html new file mode 100644 index 00000000000..3297637824d --- /dev/null +++ b/tests/wpt/mozilla/tests/css/negation-attr-dependence-ref.html @@ -0,0 +1,8 @@ + + +CSS Test Reference + +
+
Should be green
diff --git a/tests/wpt/mozilla/tests/css/negation-attr-dependence.html b/tests/wpt/mozilla/tests/css/negation-attr-dependence.html new file mode 100644 index 00000000000..99fb54b802f --- /dev/null +++ b/tests/wpt/mozilla/tests/css/negation-attr-dependence.html @@ -0,0 +1,15 @@ + + +CSS Test: Negation with attribute dependence correctly restyle siblings. + + + +
+
Should be green
+