[CSS] Serialize invalid selectors inside <forgiving-selector-list>

https://bugs.webkit.org/show_bug.cgi?id=258688 rdar://111861948 Reviewed by Antti Koivisto and Darin Adler. For the serialization round-trip to work with the presence of invalid selectors containing a nesting selector, the spec mandates that invalid selectors inside forgiving selector list (such as :is(..) or :where(...)) should be kept in serialization (and be considered when deciding whether a selector is "nest-containing" or not). w3c/csswg-drafts#8356 (comment) * LayoutTests/TestExpectations: * LayoutTests/imported/w3c/web-platform-tests/css/css-nesting/cssom-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-nesting/nest-containing-forgiving-expected.html: * LayoutTests/imported/w3c/web-platform-tests/css/css-nesting/nest-containing-forgiving-ref.html: * LayoutTests/imported/w3c/web-platform-tests/css/css-nesting/nest-containing-forgiving.html: * LayoutTests/imported/w3c/web-platform-tests/css/css-scoping/slotted-parsing.html: * LayoutTests/imported/w3c/web-platform-tests/css/selectors/is-where-error-recovery.html: * LayoutTests/imported/w3c/web-platform-tests/css/selectors/is-where-parsing.html: * LayoutTests/imported/w3c/web-platform-tests/css/selectors/parsing/parse-has-disallow-nesting-has-inside-has.html: * LayoutTests/imported/w3c/web-platform-tests/css/selectors/parsing/parse-has.html: * Source/WebCore/css/CSSSelector.cpp: (WebCore::simpleSelectorSpecificity): (WebCore::CSSSelector::selectorText const): (WebCore::CSSSelector::hasExplicitNestingParent const): * Source/WebCore/css/CSSSelector.h: * Source/WebCore/css/SelectorChecker.cpp: (WebCore::canMatchHoverOrActiveInQuirksMode): (WebCore::SelectorChecker::checkOne const): * Source/WebCore/css/parser/CSSSelectorParser.cpp: (WebCore::CSSSelectorParser::consumeForgivingSelectorList): * Source/WebCore/css/parser/CSSSelectorParser.h: * Source/WebCore/cssjit/SelectorCompiler.cpp: (WebCore::SelectorCompiler::constructFragmentsInternal): * Source/WebCore/style/RuleSet.cpp: (WebCore::Style::RuleSet::addRule): * Source/WebCore/style/RuleSetBuilder.cpp: (WebCore::Style::RuleSetBuilder::resolveSelectorListWithNesting): Canonical link: https://commits.webkit.org/266762@main
WebKit · Aug 10, 2023 · 7f69507 · 7f69507
1 parent 9b8c53b
commit 7f69507
Show file tree

Hide file tree

Showing 18 changed files with 86 additions and 33 deletions.
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
@@ -6265,8 +6265,6 @@ imported/w3c/web-platform-tests/html/browsers/the-window-object/window-open-noop
 imported/w3c/web-platform-tests/html/browsers/the-window-object/window-open-noopener.html?_parent [ Skip ]
 imported/w3c/web-platform-tests/html/browsers/the-window-object/window-open-noopener.html?_top [ Skip ]
 
-imported/w3c/web-platform-tests/css/css-nesting/nest-containing-forgiving.html [ ImageOnlyFailure ]
-
 # Needs non-zero line gap in 'Arial'
 fast/text/text-box-edge-no-half-leading-simple.html [ Skip ]
 fast/text/text-box-edge-no-half-leading-with-line-height-simple.html [ Skip ]

diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-nesting/cssom-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/css/css-nesting/cssom-expected.txt
@@ -10,5 +10,5 @@ PASS Simple CSSOM manipulation of subrules 7
 PASS Simple CSSOM manipulation of subrules 8
 PASS Simple CSSOM manipulation of subrules 9
 PASS Simple CSSOM manipulation of subrules 10
-FAIL Simple CSSOM manipulation of subrules 11 assert_equals: invalid rule containing ampersand is kept in serialization expected ".a {\n  :is(!& .foo, .b) { color: green; }\n}" but got ".a {\n  :is(.b) { color: green; }\n}"
+PASS Simple CSSOM manipulation of subrules 11
 
diff --git a/...s/imported/w3c/web-platform-tests/css/css-nesting/nest-containing-forgiving-expected.html b/...s/imported/w3c/web-platform-tests/css/css-nesting/nest-containing-forgiving-expected.html
@@ -15,4 +15,5 @@
 <body>
   <p>Tests pass if <strong>block is green</strong></p>
   <div class="test"></div>
+  <div class="test"></div>
 </body>
diff --git a/...tTests/imported/w3c/web-platform-tests/css/css-nesting/nest-containing-forgiving-ref.html b/...tTests/imported/w3c/web-platform-tests/css/css-nesting/nest-containing-forgiving-ref.html
@@ -15,4 +15,5 @@
 <body>
   <p>Tests pass if <strong>block is green</strong></p>
   <div class="test"></div>
+  <div class="test"></div>
 </body>
diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-nesting/nest-containing-forgiving.html b/LayoutTests/imported/w3c/web-platform-tests/css/css-nesting/nest-containing-forgiving.html
@@ -17,11 +17,18 @@
     }
   }
 
+  .does-not-exist {
+    :is(.test-2, :unknown(div,&)) {
+      background-color: green;
+    }
+  }
+
   body * + * {
     margin-top: 8px;
   }
 </style>
 <body>
   <p>Tests pass if <strong>block is green</strong></p>
   <div class="test test-1"></div>
+  <div class="test test-2"></div>
 </body>
diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-scoping/slotted-parsing.html b/LayoutTests/imported/w3c/web-platform-tests/css/css-scoping/slotted-parsing.html
@@ -28,12 +28,12 @@
   test_valid_selector("::slotted(:not(.a))");
 
   test_valid_selector("::slotted(*):is()");
-  test_valid_selector("::slotted(*):is(:hover)", "::slotted(*):is()");
-  test_valid_selector("::slotted(*):is(#id)", "::slotted(*):is()");
+  test_valid_selector("::slotted(*):is(:hover)");
+  test_valid_selector("::slotted(*):is(#id)");
 
   test_valid_selector("::slotted(*):where()");
-  test_valid_selector("::slotted(*):where(:hover)", "::slotted(*):where()");
-  test_valid_selector("::slotted(*):where(#id)", "::slotted(*):where()");
+  test_valid_selector("::slotted(*):where(:hover)");
+  test_valid_selector("::slotted(*):where(#id)");
 
   // Allow tree-abiding pseudo elements after ::slotted
   test_valid_selector("::slotted(*)::before");

diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/selectors/is-where-error-recovery.html b/LayoutTests/imported/w3c/web-platform-tests/css/selectors/is-where-error-recovery.html
@@ -23,18 +23,12 @@
         "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",
+        invalidSelector,
+        "Should be parsed as-is (but not be considered valid)",
       );
-      assert_equals(document.querySelector(emptyList), null, "Should never match, but should parse");
+      assert_equals(document.querySelector(rule.selectorText), null, "Should never match, but should parse");
       for (let mixedList of [
         `:${pseudo}(:total-nonsense, #test-div)`,
         `:${pseudo}(:total-nonsense and-more-stuff, #test-div)`,
@@ -43,8 +37,8 @@
         rule.selectorText = mixedList;
         assert_equals(
           rule.selectorText,
-          `:${pseudo}(#test-div)`,
-          `${mixedList}: Should ignore invalid selectors`,
+          mixedList,
+          `${mixedList}: Should parse invalid selectors`,
         );
         let testDiv = document.getElementById("test-div");
         assert_equals(document.querySelector(mixedList), testDiv, "Should correctly match");

diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/selectors/is-where-parsing.html b/LayoutTests/imported/w3c/web-platform-tests/css/selectors/is-where-parsing.html
@@ -10,7 +10,7 @@
 </style>
 <script>
   let rule = document.getElementById("test-sheet").sheet.cssRules[0];
-  function assert_valid(expected_valid, pattern, expected_pattern, description) {
+  function assert_valid(expected_parseable, pattern, expected_pattern, description) {
     test(function() {
       for (let pseudo of ["is", "where"]) {
         let selector = pattern.replace("{}", ":" + pseudo);
@@ -19,7 +19,7 @@
           expected_selector = expected_pattern.replace("{}", ":" + pseudo);
         rule.selectorText = "random-selector";
         rule.selectorText = selector;
-        (expected_valid ? assert_equals : assert_not_equals)(
+        (expected_parseable ? assert_equals : assert_not_equals)(
           rule.selectorText,
           expected_selector,
           `${description}: ${selector}`
@@ -42,7 +42,8 @@
   assert_valid(true, "{}(:hover, :active)", null, "Pseudo-classes inside");
   assert_valid(true, "{}(div):hover", null, "Pseudo-classes after");
   assert_valid(true, "{}(div)::before", null, "Pseudo-elements after");
-  assert_valid(false, "{}(::before)", null, "Pseudo-elements inside");
+  // Should ask clarification from CSSWG
+  assert_valid(true, "{}(::before)", null, "Pseudo-elements inside");
 
   assert_valid(true, "{}(div) + bar", null, "Combinators after");
   assert_valid(true, "::part(foo):is(:hover)", null, "After part with simple pseudo-class");

diff --git a/...c/web-platform-tests/css/selectors/parsing/parse-has-disallow-nesting-has-inside-has.html b/...c/web-platform-tests/css/selectors/parsing/parse-has-disallow-nesting-has-inside-has.html
@@ -8,6 +8,6 @@
 <script src="/css/support/parsing-testcommon.js"></script>
 <script>
   test_invalid_selector('.a:has(.b:has(.c))');
-  test_valid_selector('.a:has(:is(.b:has(.c)))', '.a:has(:is())');
-  test_valid_selector('.a:has(:is(.b:has(.c), .d))', '.a:has(:is(.d))');
+  test_valid_selector('.a:has(:is(.b:has(.c)))');
+  test_valid_selector('.a:has(:is(.b:has(.c), .d))');
 </script>
diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/selectors/parsing/parse-has.html b/LayoutTests/imported/w3c/web-platform-tests/css/selectors/parsing/parse-has.html
@@ -37,5 +37,5 @@
   test_invalid_selector(':has()');
   test_invalid_selector(':has(123)');
   test_invalid_selector(':has(.a, 123)');
-  test_valid_selector(':has(:is(.a, 123))', ':has(:is(.a))');
+  test_valid_selector(':has(:is(.a, 123))');
 </script>
diff --git a/Source/WebCore/css/CSSSelector.cpp b/Source/WebCore/css/CSSSelector.cpp
@@ -182,6 +182,8 @@ SelectorSpecificity simpleSelectorSpecificity(const CSSSelector& simpleSelector)
             return maxSpecificity(simpleSelector.selectorList());
         return SelectorSpecificityIncrement::ClassC;
     case CSSSelector::Match::Unknown:
+    case CSSSelector::Match::ForgivingUnknown:
+    case CSSSelector::Match::ForgivingUnknownNestContaining:
         return 0;
     }
     ASSERT_NOT_REACHED();
@@ -398,11 +400,13 @@ String CSSSelector::selectorText(StringView separator, StringView rightSide) con
         if (cs->match() == Match::Id) {
             builder.append('#');
             serializeIdentifier(cs->serializingValue(), builder);
-        } else if (cs->match() == CSSSelector::Match::NestingParent) {
+        } else if (cs->match() == Match::NestingParent) {
             builder.append('&');
-        } else if (cs->match() == CSSSelector::Match::Class) {
+        } else if (cs->match() == Match::Class) {
             builder.append('.');
             serializeIdentifier(cs->serializingValue(), builder);
+        } else if (cs->match() == Match::ForgivingUnknown || cs->match() == Match::ForgivingUnknownNestContaining) {
+            builder.append(cs->value());
         } else if (cs->match() == Match::PseudoClass) {
             switch (cs->pseudoClassType()) {
 #if ENABLE(FULLSCREEN_API)
@@ -1040,8 +1044,12 @@ void CSSSelector::replaceNestingParentByPseudoClassScope()
 bool CSSSelector::hasExplicitNestingParent() const
 {
     auto checkForExplicitParent = [] (const CSSSelector& selector) {
-        if (selector.match() == CSSSelector::Match::NestingParent)
+        if (selector.match() == Match::NestingParent)
             return true;
+
+        if (selector.match() == Match::ForgivingUnknownNestContaining)
+            return true;
+
         return false;
     };
 

diff --git a/Source/WebCore/css/CSSSelector.h b/Source/WebCore/css/CSSSelector.h
@@ -83,7 +83,9 @@ struct PossiblyQuotedIdentifier {
             Begin, // css3: E[foo^="bar"]
             End, // css3: E[foo$="bar"]
             PagePseudoClass,
-            NestingParent // &
+            NestingParent, // &
+            ForgivingUnknown,
+            ForgivingUnknownNestContaining
         };
 
         enum class RelationType : uint8_t {
@@ -325,15 +327,17 @@ struct PossiblyQuotedIdentifier {
 
     private:
         unsigned m_relation : 4 { static_cast<unsigned>(RelationType::DescendantSpace) }; // enum RelationType.
-        mutable unsigned m_match : 4 { static_cast<unsigned>(Match::Unknown) }; // enum Match.
+        mutable unsigned m_match : 5 { static_cast<unsigned>(Match::Unknown) }; // enum Match.
         mutable unsigned m_pseudoType : 8 { 0 }; // PseudoType.
+        // 17 bits
         unsigned m_isLastInSelectorList : 1 { false };
         unsigned m_isFirstInTagHistory : 1 { true };
         unsigned m_isLastInTagHistory : 1 { true };
         unsigned m_hasRareData : 1 { false };
         unsigned m_isForPage : 1 { false };
         unsigned m_tagIsForNamespaceRule : 1 { false };
         unsigned m_caseInsensitiveAttributeValueMatching : 1 { false };
+        // 24 bits
 #if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
         unsigned m_destructorHasBeenCalled : 1 { false };
 #endif

diff --git a/Source/WebCore/css/SelectorChecker.cpp b/Source/WebCore/css/SelectorChecker.cpp
@@ -629,6 +629,8 @@ static bool canMatchHoverOrActiveInQuirksMode(const SelectorChecker::LocalContex
             return true;
         case CSSSelector::Match::NestingParent:
         case CSSSelector::Match::Unknown:
+        case CSSSelector::Match::ForgivingUnknown:
+        case CSSSelector::Match::ForgivingUnknownNestContaining:
             ASSERT_NOT_REACHED();
             break;
         }
@@ -696,6 +698,14 @@ bool SelectorChecker::checkOne(CheckingContext& checkingContext, const LocalCont
         return anyAttributeMatches(element, selector, attr, caseSensitive);
     }
 
+    if (selector.match() == CSSSelector::Match::ForgivingUnknown || selector.match() == CSSSelector::Match::ForgivingUnknownNestContaining)
+        return false;
+
+    if (selector.match() == CSSSelector::Match::NestingParent) {
+        ASSERT_NOT_REACHED();
+        return false;
+    }
+
     if (selector.match() == CSSSelector::Match::PseudoClass) {
         // Handle :not up front.
         if (selector.pseudoClassType() == CSSSelector::PseudoClassType::Not) {

diff --git a/Source/WebCore/css/parser/CSSSelectorParser.cpp b/Source/WebCore/css/parser/CSSSelectorParser.cpp
@@ -160,19 +160,41 @@ CSSSelectorList CSSSelectorParser::consumeForgivingSelectorList(CSSParserTokenRa
     Vector<std::unique_ptr<CSSParserSelector>> selectorList;
 
     auto consumeForgiving = [&] {
+        auto initialRange = range;
+        auto unknownSelector = [&] {
+            auto unknownSelector = makeUnique<CSSParserSelector>();
+            auto unknownRange = initialRange.makeSubRange(initialRange.begin(), range.begin());
+            unknownSelector->setMatch(CSSSelector::Match::ForgivingUnknown);
+            // We store the complete range content for serialization.
+            unknownSelector->setValue(AtomString { unknownRange.serialize() });
+            // If the range contains a nesting selector, we mark this unknown selector as "nest containing" (it will be used during rule set building)
+            for (auto& token : unknownRange) {
+                if (token.type() == DelimiterToken && token.delimiter() == '&') {
+                    unknownSelector->setMatch(CSSSelector::Match::ForgivingUnknownNestContaining);
+                    break;
+                }
+            }
+            return unknownSelector;
+        };
+
         auto selector = consumeSelector(range);
 
         if (m_failedParsing && !m_disableForgivingParsing) {
             selector = { };
             m_failedParsing = false;
         }
-        if (!range.atEnd() && range.peek().type() != CommaToken) {
+
+        // Range is not over and next token is not a comma (means there is more to this selector) so this selector is unknown.
+        // Consume until next comma and add the full range as an unknown selector to the selector list.
+        if ((!range.atEnd() && range.peek().type() != CommaToken) || !selector) {
             while (!range.atEnd() && range.peek().type() != CommaToken)
                 range.consume();
+            if (!m_disableForgivingParsing)
+                selectorList.append(unknownSelector());
             return;
         }
-        if (selector)
-            selectorList.append(WTFMove(selector));
+
+        selectorList.append(WTFMove(selector));
     };
 
     consumeForgiving();

diff --git a/Source/WebCore/css/parser/CSSSelectorParser.h b/Source/WebCore/css/parser/CSSSelectorParser.h
@@ -111,7 +111,10 @@ class CSSSelectorParser {
     const CSSSelectorParserContext m_context;
     const RefPtr<StyleSheetContents> m_styleSheet;
     CSSParserEnum::IsNestedContext m_isNestedContext { CSSParserEnum::IsNestedContext::No };
+
+    // FIXME: This m_failedParsing is ugly and confusing, we should look into removing it (the return value of each function already convey this information).
     bool m_failedParsing { false };
+
     bool m_disallowPseudoElements { false };
     bool m_disallowHasPseudoClass { false };
     bool m_resistDefaultNamespace { false };

diff --git a/Source/WebCore/cssjit/SelectorCompiler.cpp b/Source/WebCore/cssjit/SelectorCompiler.cpp
@@ -1545,6 +1545,9 @@ static FunctionType constructFragmentsInternal(const CSSSelector* rootSelector,
         case CSSSelector::Match::Unknown:
             ASSERT_NOT_REACHED();
             return FunctionType::CannotMatchAnything;
+        case CSSSelector::Match::ForgivingUnknown:
+        case CSSSelector::Match::ForgivingUnknownNestContaining:
+            return FunctionType::CannotMatchAnything;
         }
 
         auto relation = selector->relation();

diff --git a/Source/WebCore/style/RuleSet.cpp b/Source/WebCore/style/RuleSet.cpp
@@ -215,6 +215,8 @@ void RuleSet::addRule(RuleData&& ruleData, CascadeLayerIdentifier cascadeLayerId
             }
             break;
         case CSSSelector::Match::Unknown:
+        case CSSSelector::Match::ForgivingUnknown:
+        case CSSSelector::Match::ForgivingUnknownNestContaining:
         case CSSSelector::Match::NestingParent:
         case CSSSelector::Match::PagePseudoClass:
             break;

diff --git a/Source/WebCore/style/RuleSetBuilder.cpp b/Source/WebCore/style/RuleSetBuilder.cpp
@@ -227,7 +227,6 @@ void RuleSetBuilder::resolveSelectorListWithNesting(StyleRuleWithNesting& rule)
         return;
 
     auto resolvedSelectorList = CSSSelectorParser::resolveNestingParent(rule.originalSelectorList(), parentResolvedSelectorList);
-    ASSERT(!resolvedSelectorList.hasExplicitNestingParent());
     rule.wrapperAdoptSelectorList(WTFMove(resolvedSelectorList));    
 }