Bug 1290825 - Reject various things that aren't user-action pseudo classes when they follow pseudo-elements. r=bz, a=lizzard
authorL. David Baron <dbaron@dbaron.org>
Mon, 29 Aug 2016 11:43:30 -0700
changeset 350030 25ca86f29088fae3ac4b8aa48a83a6a1f99bb7c8
parent 350029 1954ae1ff39c60de190c744337259b12d83c01d5
child 350031 d632eadae7e364e18124a2aab0936cf333850702
push id1230
push userjlund@mozilla.com
push dateMon, 31 Oct 2016 18:13:35 +0000
treeherdermozilla-release@5e06e3766db2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz, lizzard
bugs1290825, 922669
milestone50.0a2
Bug 1290825 - Reject various things that aren't user-action pseudo classes when they follow pseudo-elements. r=bz, a=lizzard The existing code, from bug 922669, in ParsePseudoSelector that allows things to come after a pseudo-element requires that the first character after the pseudo-element be a colon. However, this doesn't forbid things like ::-moz-color-swatch:hover#foo, which need to be errors in ParseSelector; those tests are added here. Furthermore, the error-checking in ParsePseudoSelector doesn't prevent the pseudo-element from being followed by a :not() or by an additional pseudo-element; to fix that this patch moves the error tests out of the pseudo-class condition and also has it test !isPseudoClass. Without the patch, the tests produced the following failures: TEST-UNEXPECTED-FAIL | layout/style/test/test_selectors.html | selector ::-moz-color-swatch:not(.foo) was a parser error - got "1402", expected "auto" TEST-UNEXPECTED-FAIL | layout/style/test/test_selectors.html | selector '::-moz-color-swatch:not(.foo)' plus EOF is parse error followed by a crash due to: Assertion failure: !(IsPseudoElement() && (mIDList || mAttrList)) (If pseudo-elements can have id or attribute selectors after them, specificity calculation must be updated), at /home/dbaron/builds/ssd/mozilla-central/mozilla/layout/style/StyleRule.cpp:503 in CalcWeightWithoutNegations from the "::-moz-color-swatch:hover#foo" test. With that test commented out (and still without the code changes), there is instead an additional pair of failures from the following test: TEST-UNEXPECTED-FAIL | layout/style/test/test_selectors.html | selector .foo::after:not(.bar) ~ h3 was a parser error - got "1406", expected "auto" TEST-UNEXPECTED-FAIL | layout/style/test/test_selectors.html | selector '.foo::after:not(.bar) ~ h3' plus EOF is parse error along with a failure due to an unexpected assertion: ###!!! ASSERTION: Shouldn't have negations: '!selector->mNegations', file /home/dbaron/builds/ssd/mozilla-central/mozilla/layout/style/nsCSSRuleProcessor.cpp, function AddRule, line 3415 With the patch, the tests pass. MozReview-Commit-ID: KxAFSQtPVhu
layout/style/nsCSSParser.cpp
layout/style/test/test_selectors.html
--- a/layout/style/nsCSSParser.cpp
+++ b/layout/style/nsCSSParser.cpp
@@ -6014,47 +6014,48 @@ CSSParserImpl::ParsePseudoSelector(int32
   if (parsingPseudoElement &&
       !isPseudoElement &&
       !isAnonBox) {
     REPORT_UNEXPECTED_TOKEN(PEPseudoSelNotPE);
     UngetToken();
     return eSelectorParsingStatus_Error;
   }
 
+  if (aSelector.IsPseudoElement()) {
+    CSSPseudoElementType type = aSelector.PseudoType();
+    if (!nsCSSPseudoElements::PseudoElementSupportsUserActionState(type)) {
+      // We only allow user action pseudo-classes on certain pseudo-elements.
+      REPORT_UNEXPECTED_TOKEN(PEPseudoSelNoUserActionPC);
+      UngetToken();
+      return eSelectorParsingStatus_Error;
+    }
+    if (!isPseudoClass || !pseudoClassIsUserAction) {
+      // CSS 4 Selectors says that pseudo-elements can only be followed by
+      // a user action pseudo-class.
+      REPORT_UNEXPECTED_TOKEN(PEPseudoClassNotUserAction);
+      UngetToken();
+      return eSelectorParsingStatus_Error;
+    }
+  }
+
   if (!parsingPseudoElement &&
       CSSPseudoClassType::negation == pseudoClassType) {
     if (aIsNegated) { // :not() can't be itself negated
       REPORT_UNEXPECTED_TOKEN(PEPseudoSelDoubleNot);
       UngetToken();
       return eSelectorParsingStatus_Error;
     }
     // CSS 3 Negation pseudo-class takes one simple selector as argument
     nsSelectorParsingStatus parsingStatus =
       ParseNegatedSimpleSelector(aDataMask, aSelector);
     if (eSelectorParsingStatus_Continue != parsingStatus) {
       return parsingStatus;
     }
   }
   else if (!parsingPseudoElement && isPseudoClass) {
-    if (aSelector.IsPseudoElement()) {
-      CSSPseudoElementType type = aSelector.PseudoType();
-      if (!nsCSSPseudoElements::PseudoElementSupportsUserActionState(type)) {
-        // We only allow user action pseudo-classes on certain pseudo-elements.
-        REPORT_UNEXPECTED_TOKEN(PEPseudoSelNoUserActionPC);
-        UngetToken();
-        return eSelectorParsingStatus_Error;
-      }
-      if (!pseudoClassIsUserAction) {
-        // CSS 4 Selectors says that pseudo-elements can only be followed by
-        // a user action pseudo-class.
-        REPORT_UNEXPECTED_TOKEN(PEPseudoClassNotUserAction);
-        UngetToken();
-        return eSelectorParsingStatus_Error;
-      }
-    }
     aDataMask |= SEL_MASK_PCLASS;
     if (eCSSToken_Function == mToken.mType) {
       nsSelectorParsingStatus parsingStatus;
       if (nsCSSPseudoClasses::HasStringArg(pseudoClassType)) {
         parsingStatus =
           ParsePseudoClassWithIdentArg(aSelector, pseudoClassType);
       }
       else if (nsCSSPseudoClasses::HasNthPairArg(pseudoClassType)) {
@@ -6486,23 +6487,17 @@ CSSParserImpl::ParseSelector(nsCSSSelect
   nsAutoPtr<nsAtomList> pseudoElementArgs;
   CSSPseudoElementType pseudoElementType = CSSPseudoElementType::NotPseudo;
 
   int32_t dataMask = 0;
   nsSelectorParsingStatus parsingStatus =
     ParseTypeOrUniversalSelector(dataMask, *selector, false);
 
   while (parsingStatus == eSelectorParsingStatus_Continue) {
-    if (eCSSToken_ID == mToken.mType) { // #id
-      parsingStatus = ParseIDSelector(dataMask, *selector);
-    }
-    else if (mToken.IsSymbol('.')) {    // .class
-      parsingStatus = ParseClassSelector(dataMask, *selector);
-    }
-    else if (mToken.IsSymbol(':')) {    // :pseudo
+    if (mToken.IsSymbol(':')) {    // :pseudo
       parsingStatus = ParsePseudoSelector(dataMask, *selector, false,
                                           getter_AddRefs(pseudoElement),
                                           getter_Transfers(pseudoElementArgs),
                                           &pseudoElementType);
       if (pseudoElement &&
           pseudoElementType != CSSPseudoElementType::AnonBox) {
         // Pseudo-elements other than anonymous boxes are represented with
         // a special ':' combinator.
@@ -6510,16 +6505,27 @@ CSSParserImpl::ParseSelector(nsCSSSelect
         aList->mWeight += selector->CalcWeight();
 
         selector = aList->AddSelector(':');
 
         selector->mLowercaseTag.swap(pseudoElement);
         selector->mClassList = pseudoElementArgs.forget();
         selector->SetPseudoType(pseudoElementType);
       }
+    } else if (selector->IsPseudoElement()) {
+      // Once we parsed a pseudo-element, we can only parse
+      // pseudo-classes (and only a limited set, which
+      // ParsePseudoSelector knows how to handle).
+      parsingStatus = eSelectorParsingStatus_Done;
+      UngetToken();
+      break;
+    } else if (eCSSToken_ID == mToken.mType) { // #id
+      parsingStatus = ParseIDSelector(dataMask, *selector);
+    } else if (mToken.IsSymbol('.')) {    // .class
+      parsingStatus = ParseClassSelector(dataMask, *selector);
     }
     else if (mToken.IsSymbol('[')) {    // [attribute
       parsingStatus = ParseAttributeSelector(dataMask, *selector);
       if (eSelectorParsingStatus_Error == parsingStatus) {
         SkipUntil(']');
       }
     }
     else {  // not a selector token, we're done
--- a/layout/style/test/test_selectors.html
+++ b/layout/style/test/test_selectors.html
@@ -1239,16 +1239,29 @@ function run() {
     test_balanced_unparseable("div \\\n p");
     test_balanced_unparseable("div\\\n p");
     test_balanced_unparseable("div \\\np");
     test_balanced_unparseable("div\\\np");
 
     // Test that :-moz-placeholder is parsable.
     test_parseable(":-moz-placeholder");
 
+    // Test that things other than user-action pseudo-classes are
+    // rejected after pseudo-elements.  Some of these tests rely on
+    // using a pseudo-element that supports a user-action pseudo-class
+    // after it, so we need to use the prefixed ::-moz-color-swatch,
+    // which is one of the ones with
+    // CSS_PSEUDO_ELEMENT_SUPPORTS_USER_ACTION_STATE (none of which are
+    // unprefixed).
+    test_parseable("::-moz-color-swatch:hover");
+    test_balanced_unparseable("::-moz-color-swatch:not(.foo)");
+    test_balanced_unparseable("::-moz-color-swatch:first-child");
+    test_balanced_unparseable("::-moz-color-swatch:hover#foo");
+    test_balanced_unparseable(".foo::after:not(.bar) ~ h3");
+
     run_deferred_tests();
 }
 
 var deferred_tests = [];
 
 function defer_clonedoc_tests(docurl, onloadfunc)
 {
     deferred_tests.push( { docurl: docurl, onloadfunc: onloadfunc } );