Bug 1664718 - Allow :is() and :where() to have better error recovery. r=xidorn

Adjust is-where-parsing.html to work with both the new and old behavior,
and add a test for the new behavior.

Depends on D90049

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

layout/style/test/test_selectors.html

@@ -1244,9 +1244,14 @@ function runTests() {
     test_balanced_unparseable(":not(:nth-child(()))");
 
     // :-moz-any()
-    test_balanced_unparseable(":-moz-any()");
-    test_balanced_unparseable(":-moz-any('foo')");
-    let parseable_if_any_is_is = SpecialPowers.getBoolPref("layout.css.moz-any-is-is.enabled") ? test_parseable : test_balanced_unparseable;
+    let parseable_if_any_is_is_and_has_better_error_recovery =
+      SpecialPowers.getBoolPref("layout.css.moz-any-is-is.enabled") &&
+      SpecialPowers.getBoolPref("layout.css.is-and-where-better-error-recovery.enabled")
+      ? test_parseable : test_balanced_unparseable;
+    parseable_if_any_is_is_and_has_better_error_recovery(":-moz-any()");
+    parseable_if_any_is_is_and_has_better_error_recovery(":-moz-any('foo')");
+    let parseable_if_any_is_is = SpecialPowers.getBoolPref("layout.css.moz-any-is-is.enabled")
+      ? test_parseable : test_balanced_unparseable;
     parseable_if_any_is_is(":-moz-any(div p)");
     parseable_if_any_is_is(":-moz-any(div ~ p)");
     parseable_if_any_is_is(":-moz-any(div~p)");
@@ -1261,7 +1266,7 @@ function runTests() {
     test_parseable(":-moz-any(div,p,:link,span:focus)");
     test_parseable(":-moz-any(:active,:focus)");
     test_parseable(":-moz-any(:active,:link:focus)");
-    test_balanced_unparseable(":-moz-any(div,:nonexistentpseudo)");
+    parseable_if_any_is_is_and_has_better_error_recovery(":-moz-any(div,:nonexistentpseudo)");
     var any_elts = "<input type='text'><a href='http://www.example.com/'></a><div></div><a name='foo'>";
     test_selector_in_html(":-moz-any(a,input)", any_elts,
                           bodychildset([0, 1, 3]), bodychildset([2]));
@@ -1311,10 +1316,6 @@ function runTests() {
     test_parseable("::-moz-color-swatch:not(:hover)");
     test_parseable("::-moz-color-swatch:where(:hover)");
     test_balanced_unparseable("::-moz-color-swatch:not(.foo)");
-    test_balanced_unparseable("::-moz-color-swatch:where(.foo)");
-    test_balanced_unparseable("::-moz-color-swatch:is(.foo)");
-    test_balanced_unparseable("::-moz-color-swatch:where(p, :hover)");
-    test_balanced_unparseable("::-moz-color-swatch:is(p, :hover)");
     test_balanced_unparseable("::-moz-color-swatch:first-child");
     test_balanced_unparseable("::-moz-color-swatch:host");
     test_balanced_unparseable("::-moz-color-swatch:host(div)");
@@ -1322,6 +1323,23 @@ function runTests() {
     test_balanced_unparseable("::-moz-color-swatch:hover#foo");
     test_balanced_unparseable(".foo::after:not(.bar) ~ h3");
 
+    {
+      let better_error_recovery = SpecialPowers.getBoolPref("layout.css.is-and-where-better-error-recovery.enabled");
+      let expected_fn = better_error_recovery ? test_parseable : test_balanced_unparseable;
+
+      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)"],
+      ]) {
+        expected_fn(selector);
+        if (better_error_recovery) {
+          should_serialize_to(selector, expected_serialization_when_parseable);
+        }
+      }
+    }
+
     test_parseable(":-moz-broken");
     test_parseable(":-moz-loading");
 

modules/libpref/init/StaticPrefList.yaml

@@ -6551,6 +6551,16 @@
   value: true
   mirror: always
 
+# Whether :is() and :where() ignore errors inside their selector lists
+# internally, rather than failing to parse altogether.
+#
+# See https://github.com/w3c/csswg-drafts/issues/3264
+- name: layout.css.is-and-where-better-error-recovery.enabled
+  type: RelaxedAtomicBool
+  value: @IS_EARLY_BETA_OR_EARLIER@
+  mirror: always
+  rust: true
+
 # Whether frame visibility tracking is enabled globally.
 - name: layout.framevisibility.enabled
   type: bool

servo/components/selectors/parser.rs

@@ -251,6 +251,11 @@ pub trait Parser<'i> {
         false
     }
 
+    /// The error recovery that selector lists inside :is() and :where() have.
+    fn is_and_where_error_recovery(&self) -> ParseErrorRecovery {
+        ParseErrorRecovery::IgnoreInvalidSelector
+    }
+
     /// Whether the given function name is an alias for the `:is()` function.
     fn is_is_alias(&self, _name: &str) -> bool {
         false
@@ -329,6 +334,17 @@ pub struct SelectorList<Impl: SelectorImpl>(
     #[shmem(field_bound)] pub SmallVec<[Selector<Impl>; 1]>,
 );
 
+/// How to treat invalid selectors in a selector list.
+pub enum ParseErrorRecovery {
+    /// Discard the entire selector list, this is the default behavior for
+    /// almost all of CSS.
+    DiscardList,
+    /// Ignore invalid selectors, potentially creating an empty selector list.
+    ///
+    /// This is the error recovery mode of :is() and :where()
+    IgnoreInvalidSelector,
+}
+
 impl<Impl: SelectorImpl> SelectorList<Impl> {
     /// Parse a comma-separated list of Selectors.
     /// <https://drafts.csswg.org/selectors/#grouping>
@@ -341,26 +357,42 @@ impl<Impl: SelectorImpl> SelectorList<Impl> {
     where
         P: Parser<'i, Impl = Impl>,
     {
-        Self::parse_with_state(parser, input, SelectorParsingState::empty())
+        Self::parse_with_state(parser, input, SelectorParsingState::empty(), ParseErrorRecovery::DiscardList)
     }
 
+    #[inline]
     fn parse_with_state<'i, 't, P>(
         parser: &P,
         input: &mut CssParser<'i, 't>,
         state: SelectorParsingState,
+        recovery: ParseErrorRecovery,
     ) -> Result<Self, ParseError<'i, P::Error>>
     where
         P: Parser<'i, Impl = Impl>,
     {
         let mut values = SmallVec::new();
         loop {
-            values.push(input.parse_until_before(Delimiter::Comma, |input| {
+            let selector = input.parse_until_before(Delimiter::Comma, |input| {
                 parse_selector(parser, input, state)
-            })?);
+            });
+
+            let was_ok = selector.is_ok();
+            match selector {
+                Ok(selector) => values.push(selector),
+                Err(err) => match recovery {
+                    ParseErrorRecovery::DiscardList => return Err(err),
+                    ParseErrorRecovery::IgnoreInvalidSelector => {},
+                },
+            }
+
+            loop {
                 match input.next() {
                     Err(_) => return Ok(SelectorList(values)),
-                Ok(&Token::Comma) => continue,
-                Ok(_) => unreachable!(),
+                    Ok(&Token::Comma) => break,
+                    Ok(_) => {
+                        debug_assert!(!was_ok, "Shouldn't have got a selector if getting here");
+                    }
+                }
             }
         }
     }
@@ -1251,18 +1283,18 @@ impl<Impl: SelectorImpl> Debug for LocalName<Impl> {
     }
 }
 
-fn serialize_selector_list<'a, Impl, I, W>(mut iter: I, dest: &mut W) -> fmt::Result
+fn serialize_selector_list<'a, Impl, I, W>(iter: I, dest: &mut W) -> fmt::Result
 where
     Impl: SelectorImpl,
     I: Iterator<Item = &'a Selector<Impl>>,
     W: fmt::Write,
 {
-    let first = iter
-        .next()
-        .expect("Empty SelectorList, should contain at least one selector");
-    first.to_css(dest)?;
+    let mut first = true;
     for selector in iter {
+        if !first {
             dest.write_str(", ")?;
+        }
+        first = false;
         selector.to_css(dest)?;
     }
     Ok(())
@@ -2266,6 +2298,7 @@ where
         parser,
         input,
         state | SelectorParsingState::DISALLOW_PSEUDOS,
+        parser.is_and_where_error_recovery(),
     )?;
     Ok(component(inner.0.into_vec().into_boxed_slice()))
 }

servo/components/style/gecko/selector_parser.rs

@@ -14,7 +14,7 @@ use crate::string_cache::{Atom, Namespace, WeakAtom, WeakNamespace};
 use crate::values::serialize_atom_identifier;
 use cssparser::{BasicParseError, BasicParseErrorKind, Parser};
 use cssparser::{CowRcStr, SourceLocation, ToCss, Token};
-use selectors::parser::SelectorParseErrorKind;
+use selectors::parser::{SelectorParseErrorKind, ParseErrorRecovery};
 use selectors::parser::{self as selector_parser, Selector};
 use selectors::visitor::SelectorVisitor;
 use selectors::SelectorList;
@@ -340,6 +340,15 @@ impl<'a, 'i> ::selectors::Parser<'i> for SelectorParser<'a> {
         true
     }
 
+    #[inline]
+    fn is_and_where_error_recovery(&self) -> ParseErrorRecovery {
+        if static_prefs::pref!("layout.css.is-and-where-better-error-recovery.enabled") {
+            ParseErrorRecovery::IgnoreInvalidSelector
+        } else {
+            ParseErrorRecovery::DiscardList
+        }
+    }
+
     #[inline]
     fn parse_part(&self) -> bool {
         true

testing/web-platform/meta/css/selectors/is-where-error-recovery.tentative.html.ini

@@ -0,0 +1,2 @@
+[is-where-error-recovery.tentative.html]
+  prefs: [layout.css.is-and-where-better-error-recovery.enabled:true]

testing/web-platform/tests/css/selectors/is-where-error-recovery.tentative.html

@@ -0,0 +1,54 @@
+<!doctype html>
+<title>CSS Selectors: :is() and :where() error recovery</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="help" href="https://github.com/w3c/csswg-drafts/issues/3676">
+<link rel="help" href="https://drafts.csswg.org/selectors-4/#matches">
+<link rel="help" href="https://drafts.csswg.org/selectors-4/#zero-matches">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<style id="test-sheet">
+  random-selector { color: blue; }
+</style>
+<div id="test-div"></div>
+<script>
+  let rule = document.getElementById("test-sheet").sheet.cssRules[0];
+  test(function() {
+    for (let pseudo of ["is", "where"]) {
+      rule.selectorText = "random-selector";
+      let invalidSelector = `:${pseudo}(:total-nonsense)`;
+      rule.selectorText = invalidSelector;
+      assert_not_equals(
+        rule.selectorText,
+        "random-selector",
+        "Should've parsed",
+      );
+      assert_not_equals(
+        rule.selectorText,
+        invalidSelector,
+        "Should not be considered valid and parsed as-is",
+      );
+      let emptyList = `:${pseudo}()`;
+      assert_equals(
+        rule.selectorText,
+        emptyList,
+        "Should be serialized as an empty selector-list",
+      );
+      assert_equals(document.querySelector(emptyList), null, "Should never match, but should parse");
+      for (let mixedList of [
+        `:${pseudo}(:total-nonsense, #test-div)`,
+        `:${pseudo}(:total-nonsense and-more-stuff, #test-div)`,
+        `:${pseudo}(weird-token || and-more-stuff, #test-div)`,
+      ]) {
+        rule.selectorText = mixedList;
+        assert_equals(
+          rule.selectorText,
+          `:${pseudo}(#test-div)`,
+          `${mixedList}: Should ignore invalid selectors`,
+        );
+        let testDiv = document.getElementById("test-div");
+        assert_equals(document.querySelector(mixedList), testDiv, "Should correctly match");
+        assert_equals(getComputedStyle(testDiv).color, "rgb(0, 0, 255)", "test element should be blue");
+      }
+    }
+  });
+</script>

testing/web-platform/tests/css/selectors/is-where-parsing.html

@@ -5,18 +5,22 @@
 <link rel="help" href="https://drafts.csswg.org/selectors-4/#zero-matches">
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
+<style id="test-sheet">
+  random-selector { color: blue; }
+</style>
 <script>
+  let rule = document.getElementById("test-sheet").sheet.cssRules[0];
   function assert_valid(expected_valid, pattern, description) {
     test(function() {
       for (let pseudo of ["is", "where"]) {
-        let valid = false;
         let selector = pattern.replace("{}", ":" + pseudo)
-        try {
-          document.querySelector(selector);
-          valid = true;
-        } catch (ex) {}
-
-        assert_equals(valid, expected_valid, `${description}: ${selector}`);
+        rule.selectorText = "random-selector";
+        rule.selectorText = selector;
+        (expected_valid ? assert_equals : assert_not_equals)(
+          rule.selectorText,
+          selector,
+          `${description}: ${selector}`
+        );
       }
     }, description);
   }