servo: Merge #19610 - style: Don't support a list of selectors in ::slotted yet (from emilio:slotted-list); r=xidorn
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 20 Dec 2017 11:09:50 -0600
changeset 397036 ba236a79315be955db76b1fc50df7aa735538e6a
parent 397035 98ca352b22c7332d6b6a609ae9783e85046bbced
child 397037 248cd7331f4a35a03b0c347122efbeed0a12b093
push id33123
push userncsoregi@mozilla.com
push dateThu, 21 Dec 2017 10:00:47 +0000
treeherdermozilla-central@06a19fbe2581 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersxidorn
milestone59.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
servo: Merge #19610 - style: Don't support a list of selectors in ::slotted yet (from emilio:slotted-list); r=xidorn Bug: 1425757 Reviewed-by: xidorn MozReview-Commit-ID: G0I0gM2sWTh Source-Repo: https://github.com/servo/servo Source-Revision: e074a1c62012e966b499d797b49b2efc08fe4007
servo/components/selectors/matching.rs
servo/components/selectors/parser.rs
servo/components/style/selector_map.rs
--- a/servo/components/selectors/matching.rs
+++ b/servo/components/selectors/matching.rs
@@ -550,28 +550,26 @@ fn matches_simple_selector<E, F>(
     flags_setter: &mut F,
 ) -> bool
 where
     E: Element,
     F: FnMut(&E, ElementSelectorFlags),
 {
     match *selector {
         Component::Combinator(_) => unreachable!(),
-        Component::Slotted(ref selectors) => {
+        Component::Slotted(ref selector) => {
             context.shared.nesting_level += 1;
             let result =
                 element.assigned_slot().is_some() &&
-                selectors.iter().any(|s| {
-                    matches_complex_selector(
-                        s.iter(),
-                        element,
-                        context.shared,
-                        flags_setter,
-                    )
-                });
+                matches_complex_selector(
+                    selector.iter(),
+                    element,
+                    context.shared,
+                    flags_setter,
+                );
             context.shared.nesting_level -= 1;
             result
         }
         Component::PseudoElement(ref pseudo) => {
             element.match_pseudo_element(pseudo, context.shared)
         }
         Component::LocalName(LocalName { ref name, ref lower_name }) => {
             let is_html = element.is_html_element_in_html_document();
--- a/servo/components/selectors/parser.rs
+++ b/servo/components/selectors/parser.rs
@@ -210,41 +210,49 @@ impl<Impl: SelectorImpl> SelectorList<Im
     }
 
     /// Creates a SelectorList from a Vec of selectors. Used in tests.
     pub fn from_vec(v: Vec<Selector<Impl>>) -> Self {
         SelectorList(SmallVec::from_vec(v))
     }
 }
 
+/// Parses one compound selector suitable for nested stuff like ::-moz-any, etc.
+fn parse_inner_compound_selector<'i, 't, P, Impl>(
+    parser: &P,
+    input: &mut CssParser<'i, 't>,
+) -> Result<Selector<Impl>, ParseError<'i, P::Error>>
+where
+    P: Parser<'i, Impl=Impl>,
+    Impl: SelectorImpl,
+{
+    let location = input.current_source_location();
+    let selector = Selector::parse(parser, input)?;
+    // Ensure they're actually all compound selectors.
+    if selector.iter_raw_match_order().any(|s| s.is_combinator()) {
+        return Err(location.new_custom_error(
+            SelectorParseErrorKind::NonCompoundSelector
+        ))
+    }
+
+    Ok(selector)
+}
+
 /// Parse a comma separated list of compound selectors.
 pub fn parse_compound_selector_list<'i, 't, P, Impl>(
     parser: &P,
     input: &mut CssParser<'i, 't>,
 ) -> Result<Box<[Selector<Impl>]>, ParseError<'i, P::Error>>
 where
     P: Parser<'i, Impl=Impl>,
     Impl: SelectorImpl,
 {
-    let location = input.current_source_location();
-    let selectors = input.parse_comma_separated(|input| {
-        Selector::parse(parser, input)
-    })?;
-
-    // Ensure they're actually all compound selectors.
-    if selectors
-        .iter()
-        .flat_map(|x| x.iter_raw_match_order())
-        .any(|s| s.is_combinator()) {
-        return Err(location.new_custom_error(
-            SelectorParseErrorKind::NonCompoundSelector
-        ))
-    }
-
-    Ok(selectors.into_boxed_slice())
+    input.parse_comma_separated(|input| {
+        parse_inner_compound_selector(parser, input)
+    }).map(|selectors| selectors.into_boxed_slice())
 }
 
 /// Ancestor hashes for the bloom filter. We precompute these and store them
 /// inline with selectors to optimize cache performance during matching.
 /// This matters a lot.
 ///
 /// We use 4 hashes, which is copied from Gecko, who copied it from WebKit.
 /// Note that increasing the number of hashes here will adversely affect the
@@ -756,18 +764,22 @@ pub enum Component<Impl: SelectorImpl> {
     LastOfType,
     OnlyOfType,
     NonTSPseudoClass(Impl::NonTSPseudoClass),
     /// The ::slotted() pseudo-element (which isn't actually a pseudo-element,
     /// and probably should be a pseudo-class):
     ///
     /// https://drafts.csswg.org/css-scoping/#slotted-pseudo
     ///
-    /// The selectors here are compound selectors, that is, no combinators.
-    Slotted(Box<[Selector<Impl>]>),
+    /// The selector here is a compound selector, that is, no combinators.
+    ///
+    /// NOTE(emilio): This should support a list of selectors, but as of this
+    /// writing no other browser does, and that allows them to put ::slotted()
+    /// in the rule hash, so we do that too.
+    Slotted(Selector<Impl>),
     PseudoElement(Impl::PseudoElement),
 }
 
 impl<Impl: SelectorImpl> Component<Impl> {
     /// Compute the ancestor hash to check against the bloom filter.
     fn ancestor_hash(&self, quirks_mode: QuirksMode) -> Option<u32> {
         match *self {
             Component::LocalName(LocalName { ref name, ref lower_name }) => {
@@ -992,24 +1004,19 @@ impl<Impl: SelectorImpl> ToCss for Compo
                 (_, _) => write!(dest, "{}n{:+}", a, b),
             }
         }
 
         match *self {
             Combinator(ref c) => {
                 c.to_css(dest)
             }
-            Slotted(ref selectors) => {
+            Slotted(ref selector) => {
                 dest.write_str("::slotted(")?;
-                let mut iter = selectors.iter();
-                iter.next().expect("At least one selector").to_css(dest)?;
-                for other in iter {
-                    dest.write_str(", ")?;
-                    other.to_css(dest)?;
-                }
+                selector.to_css(dest)?;
                 dest.write_char(')')
             }
             PseudoElement(ref p) => {
                 p.to_css(dest)
             }
             ID(ref s) => {
                 dest.write_char('#')?;
                 display_to_css_identifier(s, dest)
@@ -1300,17 +1307,17 @@ where
         Err(e) => Err(e)
     }
 }
 
 #[derive(Debug)]
 enum SimpleSelectorParseResult<Impl: SelectorImpl> {
     SimpleSelector(Component<Impl>),
     PseudoElement(Impl::PseudoElement),
-    SlottedPseudo(Box<[Selector<Impl>]>),
+    SlottedPseudo(Selector<Impl>),
 }
 
 #[derive(Debug)]
 enum QNamePrefix<Impl: SelectorImpl> {
     ImplicitNoNamespace, // `foo` in attr selectors
     ImplicitAnyNamespace, // `foo` in type selectors, without a default ns
     ImplicitDefaultNamespace(Impl::NamespaceUrl),  // `foo` in type selectors, with a default ns
     ExplicitNoNamespace,  // `|foo`
@@ -1719,23 +1726,23 @@ where
                 for state_selector in state_selectors.drain() {
                     builder.push_simple_selector(state_selector);
                 }
 
                 pseudo = true;
                 empty = false;
                 break
             }
-            SimpleSelectorParseResult::SlottedPseudo(selectors) => {
+            SimpleSelectorParseResult::SlottedPseudo(selector) => {
                 empty = false;
                 slot = true;
                 if !builder.is_empty() {
                     builder.push_combinator(Combinator::SlotAssignment);
                 }
-                builder.push_simple_selector(Component::Slotted(selectors));
+                builder.push_simple_selector(Component::Slotted(selector));
                 // FIXME(emilio): ::slotted() should support ::before and
                 // ::after after it, so we shouldn't break, but we shouldn't
                 // push more type selectors either.
                 break;
             }
         }
     }
     if empty {
@@ -1852,17 +1859,17 @@ where
             };
             let is_pseudo_element = !is_single_colon ||
                 P::pseudo_element_allows_single_colon(&name);
             if is_pseudo_element {
                 let parse_result = if is_functional {
                     if P::parse_slotted(parser) && name.eq_ignore_ascii_case("slotted") {
                         SimpleSelectorParseResult::SlottedPseudo(
                             input.parse_nested_block(|input| {
-                                parse_compound_selector_list(
+                                parse_inner_compound_selector(
                                     parser,
                                     input,
                                 )
                             })?
                         )
                     } else {
                         SimpleSelectorParseResult::PseudoElement(
                             input.parse_nested_block(|input| {
@@ -2484,17 +2491,20 @@ pub mod tests {
         assert!(parse("::slotted()").is_err());
         assert!(parse("::slotted(div)").is_ok());
         assert!(parse("::slotted(div).foo").is_err());
         assert!(parse("::slotted(div + bar)").is_err());
         assert!(parse("::slotted(div) + foo").is_err());
         assert!(parse("div ::slotted(div)").is_ok());
         assert!(parse("div + slot::slotted(div)").is_ok());
         assert!(parse("div + slot::slotted(div.foo)").is_ok());
-        assert!(parse("div + slot::slotted(.foo, bar, .baz)").is_ok());
+        assert!(parse("slot::slotted(div,foo)::first-line").is_err());
+        // TODO
+        assert!(parse("::slotted(div)::before").is_err());
+        assert!(parse("slot::slotted(div,foo)").is_err());
     }
 
     #[test]
     fn test_pseudo_iter() {
         let selector = &parse("q::before").unwrap().0[0];
         assert!(!selector.is_universal());
         let mut iter = selector.iter();
         assert_eq!(iter.next(), Some(&Component::PseudoElement(PseudoElement::Before)));
--- a/servo/components/style/selector_map.rs
+++ b/servo/components/style/selector_map.rs
@@ -410,16 +410,35 @@ fn specific_bucket_for<'a>(
         Component::ID(ref id) => Bucket::ID(id),
         Component::Class(ref class) => Bucket::Class(class),
         Component::LocalName(ref selector) => {
             Bucket::LocalName {
                 name: &selector.name,
                 lower_name: &selector.lower_name,
             }
         }
+        // ::slotted(..) isn't a normal pseudo-element, so we can insert it on
+        // the rule hash normally without much problem. For example, in a
+        // selector like:
+        //
+        //   div::slotted(span)::before
+        //
+        // It looks like:
+        //
+        //  [
+        //    LocalName(div),
+        //    Combinator(SlotAssignment),
+        //    Slotted(span),
+        //    Combinator::PseudoElement,
+        //    PseudoElement(::before),
+        //  ]
+        //
+        // So inserting `span` in the rule hash makes sense since we want to
+        // match the slotted <span>.
+        Component::Slotted(ref selector) => find_bucket(selector.iter()),
         _ => Bucket::Universal
     }
 }
 
 /// Searches a compound selector from left to right, and returns the appropriate
 /// bucket for it.
 #[inline(always)]
 fn find_bucket<'a>(mut iter: SelectorIter<'a, SelectorImpl>) -> Bucket<'a> {