Bug 1856018 - Make forgiving selectors serialize. r=dshin

As per resolution in w3c/csswg-drafts#8356

Safari already does this. Expand the tests they tweaked to be useful
again (test that when non-forgiving parsing is used, then the selector
is invalid).

Differential Revision: https://phabricator.services.mozilla.com/D189666

layout/style/test/test_selectors.html

@@ -1299,14 +1299,15 @@
     test_balanced_unparseable("::-moz-color-swatch:hover#foo");
     test_balanced_unparseable(".foo::after:not(.bar) ~ h3");
 
-    for (let [selector, expected_serialization_when_parseable] of [
-      ["::-moz-color-swatch:where(.foo)", "::-moz-color-swatch:where()"],
-      ["::-moz-color-swatch:is(.foo)", "::-moz-color-swatch:is()"],
-      ["::-moz-color-swatch:where(p, :hover)", "::-moz-color-swatch:where(:hover)"],
-      ["::-moz-color-swatch:is(p, :hover)", "::-moz-color-swatch:is(:hover)"],
+    for (let selector of [
+      "::-moz-color-swatch:where(.foo)",
+      "::-moz-color-swatch:is(.foo)",
+      "::-moz-color-swatch:where(p, :hover)",
+      "::-moz-color-swatch:is(p, :hover)",
     ]) {
       test_parseable(selector);
-      should_serialize_to(selector, expected_serialization_when_parseable);
+      should_serialize_to(selector, selector);
+      ok(!CSS.supports(`selector(${selector})`), "supports should report false for forging selector parse failure");
     }
 
     run_deferred_tests();

servo/components/malloc_size_of/lib.rs

@@ -771,7 +771,8 @@ where
             Component::ParentSelector |
             Component::Nth(..) |
             Component::Host(None) |
-            Component::RelativeSelectorAnchor => 0,
+            Component::RelativeSelectorAnchor |
+            Component::Invalid(..) => 0,
         }
     }
 }

servo/components/selectors/builder.rs

@@ -351,7 +351,8 @@ where
             Component::ExplicitNoNamespace |
             Component::DefaultNamespace(..) |
             Component::Namespace(..) |
-            Component::RelativeSelectorAnchor => {
+            Component::RelativeSelectorAnchor |
+            Component::Invalid(..) => {
                 // Does not affect specificity
             },
         }

servo/components/selectors/matching.rs

@@ -1100,6 +1100,7 @@ where
             );
             anchor.map_or(false, |a| a == element.opaque())
         },
+        Component::Invalid(..) => false,
     }
 }
 

servo/components/selectors/parser.rs

@@ -18,7 +18,7 @@ use cssparser::{BasicParseError, BasicParseErrorKind, ParseError, ParseErrorKind
 use cssparser::{CowRcStr, Delimiter, SourceLocation};
 use cssparser::{Parser as CssParser, ToCss, Token};
 use precomputed_hash::PrecomputedHash;
-use servo_arc::{ThinArc, UniqueArc};
+use servo_arc::{Arc, ThinArc, UniqueArc};
 use smallvec::SmallVec;
 use std::borrow::{Borrow, Cow};
 use std::fmt::{self, Debug};
@@ -371,6 +371,7 @@ impl SelectorKey {
 }
 
 /// Whether or not we're using forgiving parsing mode
+#[derive(PartialEq)]
 enum ForgivingParsing {
     /// Discard the entire selector list upon encountering any invalid selector.
     /// This is the default behavior for almost all of CSS.
@@ -443,34 +444,28 @@ impl<Impl: SelectorImpl> SelectorList<Impl> {
         P: Parser<'i, Impl = Impl>,
     {
         let mut values = SmallVec::new();
+        let forgiving = recovery == ForgivingParsing::Yes && parser.allow_forgiving_selectors();
         loop {
-            let selector = input.parse_until_before(Delimiter::Comma, |i| {
-                parse_selector(parser, i, state, parse_relative)
+            let selector = input.parse_until_before(Delimiter::Comma, |input| {
+                let start = input.position();
+                let mut selector = parse_selector(parser, input, state, parse_relative);
+                if forgiving && (selector.is_err() || input.expect_exhausted().is_err()) {
+                    input.expect_no_error_token()?;
+                    selector = Ok(Selector::new_invalid(input.slice_from(start)));
+                }
+                selector
             });
 
-            let was_ok = selector.is_ok();
-            match selector {
-                Ok(selector) => values.push(selector),
-                Err(err) => match recovery {
-                    ForgivingParsing::No => return Err(err),
-                    ForgivingParsing::Yes => {
-                        if !parser.allow_forgiving_selectors() {
-                            return Err(err);
-                        }
-                    },
-                },
-            }
+            debug_assert!(!forgiving || selector.is_ok());
+            values.push(selector?);
 
-            loop {
-                match input.next() {
-                    Err(_) => return Ok(SelectorList(values)),
-                    Ok(&Token::Comma) => break,
-                    Ok(_) => {
-                        debug_assert!(!was_ok, "Shouldn't have got a selector if getting here");
-                    },
-                }
+            match input.next() {
+                Ok(&Token::Comma) => {},
+                Ok(_) => unreachable!(),
+                Err(_) => break,
             }
         }
+        Ok(SelectorList(values))
     }
 
     /// Replaces the parent selector in all the items of the selector list.
@@ -1045,6 +1040,7 @@ impl<Impl: SelectorImpl> Selector<Impl> {
                     Combinator(..) |
                     Host(None) |
                     Part(..) |
+                    Invalid(..) |
                     RelativeSelectorAnchor => component.clone(),
                     ParentSelector => {
                         specificity += parent_specificity;
@@ -1168,6 +1164,65 @@ impl<Impl: SelectorImpl> Selector<Impl> {
 
         true
     }
+
+    /// Parse a selector, without any pseudo-element.
+    #[inline]
+    pub fn parse<'i, 't, P>(
+        parser: &P,
+        input: &mut CssParser<'i, 't>,
+    ) -> Result<Self, ParseError<'i, P::Error>>
+    where
+        P: Parser<'i, Impl = Impl>,
+    {
+        parse_selector(
+            parser,
+            input,
+            SelectorParsingState::empty(),
+            ParseRelative::No,
+        )
+    }
+
+    pub fn new_invalid(s: &str) -> Self {
+        fn check_for_parent(input: &mut CssParser, has_parent: &mut bool) {
+            while let Ok(t) = input.next() {
+                match *t {
+                    Token::Function(_) |
+                    Token::ParenthesisBlock |
+                    Token::CurlyBracketBlock |
+                    Token::SquareBracketBlock => {
+                        let _ = input.parse_nested_block(|i| -> Result<(), ParseError<'_, BasicParseError>> {
+                            check_for_parent(i, has_parent);
+                            Ok(())
+                        });
+                    },
+                    Token::Delim('&') => {
+                        *has_parent = true;
+                    }
+                    _ => {},
+                }
+                if *has_parent {
+                    break;
+                }
+            }
+        }
+        let mut has_parent = false;
+        {
+            let mut parser = cssparser::ParserInput::new(s);
+            let mut parser = CssParser::new(&mut parser);
+            check_for_parent(&mut parser, &mut has_parent);
+        }
+        Self(ThinArc::from_header_and_iter(
+            SpecificityAndFlags {
+                specificity: 0,
+                flags: if has_parent {
+                    SelectorFlags::HAS_PARENT
+                } else {
+                    SelectorFlags::empty()
+                },
+            },
+            std::iter::once(Component::Invalid(Arc::new(String::from(s.trim()))))
+        ))
+    }
 }
 
 #[derive(Clone)]
@@ -1831,6 +1886,8 @@ pub enum Component<Impl: SelectorImpl> {
     ///
     /// Same comment as above re. the argument.
     Has(Box<[RelativeSelector<Impl>]>),
+    /// An invalid selector inside :is() / :where().
+    Invalid(Arc<String>),
     /// An implementation-dependent pseudo-element selector.
     PseudoElement(#[shmem(field_bound)] Impl::PseudoElement),
 
@@ -2369,6 +2426,7 @@ impl<Impl: SelectorImpl> ToCss for Component<Impl> {
                 dest.write_str(")")
             },
             NonTSPseudoClass(ref pseudo) => pseudo.to_css(dest),
+            Invalid(ref css) => dest.write_str(css),
             RelativeSelectorAnchor => Ok(()),
         }
     }
@@ -2519,25 +2577,6 @@ fn try_parse_combinator<'i, 't, P, Impl>(input: &mut CssParser<'i, 't>) -> Resul
     }
 }
 
-impl<Impl: SelectorImpl> Selector<Impl> {
-    /// Parse a selector, without any pseudo-element.
-    #[inline]
-    pub fn parse<'i, 't, P>(
-        parser: &P,
-        input: &mut CssParser<'i, 't>,
-    ) -> Result<Self, ParseError<'i, P::Error>>
-    where
-        P: Parser<'i, Impl = Impl>,
-    {
-        parse_selector(
-            parser,
-            input,
-            SelectorParsingState::empty(),
-            ParseRelative::No,
-        )
-    }
-}
-
 /// * `Err(())`: Invalid selector, abort
 /// * `Ok(false)`: Not a type selector, could be something else. `input` was not consumed.
 /// * `Ok(true)`: Length 0 (`*|*`), 1 (`*|E` or `ns|*`) or 2 (`|E` or `ns|E`)
@@ -4202,7 +4241,7 @@ pub mod tests {
 
         assert!(parse("foo:where()").is_ok());
         assert!(parse("foo:where(div, foo, .bar baz)").is_ok());
-        assert!(parse_expected("foo:where(::before)", Some("foo:where()")).is_ok());
+        assert!(parse("foo:where(::before)").is_ok());
     }
 
     #[test]

testing/web-platform/meta/css/selectors/is-where-shadow.html.ini

@@ -1,5 +1,3 @@
 [is-where-shadow.html]
-  expected:
-    if (os == "android") and fission: [OK, TIMEOUT]
   [:is() inside :host-context()]
     expected: FAIL

testing/web-platform/tests/css/css-scoping/slotted-parsing.html

@@ -27,13 +27,14 @@
   test_valid_selector("::slotted([attr]:hover)");
   test_valid_selector("::slotted(:not(.a))");
 
-  test_valid_selector("::slotted(*):is()");
-  test_valid_selector("::slotted(*):is(:hover)");
-  test_valid_selector("::slotted(*):is(#id)");
+  test_valid_forgiving_selector("::slotted(*):is()");
+  test_valid_forgiving_selector("::slotted(*):is(:hover)");
+  test_valid_forgiving_selector("::slotted(*):is(#id)");
 
-  test_valid_selector("::slotted(*):where()");
-  test_valid_selector("::slotted(*):where(:hover)");
-  test_valid_selector("::slotted(*):where(#id)");
+  test_valid_forgiving_selector("::slotted(*):where()");
+  test_valid_forgiving_selector("::slotted(*):where(:hover)");
+  test_valid_forgiving_selector("::slotted(*):where(#id)");
+  test_valid_forgiving_selector("::slotted(*):where(::before)");
 
   // Allow tree-abiding pseudo elements after ::slotted
   test_valid_selector("::slotted(*)::before");