If we have @supports (color: green !important anything) { ... } this currently causes the @supports rule to fail to parse, rather than evaluate to false.

The first test I added there doesn't currently fail, but it seemed like it was worth adding.

Comment on attachment 795225 [details] [diff] [review] patch This mostly seems good, except I don't see why you should be checking for }. It seems like the only valid character here is a ). I'm also a bit confused by the very last bit of CSSParserImpl::ParseSupportsConditionInParens given the current spec: if (!(ExpectSymbol(')', true))) { REPORT_UNEXPECTED_TOKEN(PESupportsConditionExpectedCloseParen); SkipUntil(')'); return false; } it seems wrong to return false when there is a matching ')', since that means that general_enclosed was matched. (aConditionMet should be false, but the return value should be true.) It seems like fixing that bit of code to do: aConditionMet = false; return true; instead of: return false; would fix this bug without this addition of code. (In general, I'm suspicious of any parsing code that looks ahead to the next token to see if the current structure is complete.) Sorry for taking so long to get to this.

(In reply to David Baron [:dbaron] (needinfo? me) from comment #2) > This mostly seems good, except I don't see why you should be checking for }. > It seems like the only valid character here is a ). Yeah, you're right. I think I was confusing myself with a case like this: <style>@media screen { @supports (color: green }</style> where I was thinking that the "}" should imply the closing ")", but only EOF does that. And even if it did, the @supports rule wouldn't be valid since it needs to have its own braces. > I'm also a bit confused by the very last bit of > CSSParserImpl::ParseSupportsConditionInParens given the current spec: > > if (!(ExpectSymbol(')', true))) { > REPORT_UNEXPECTED_TOKEN(PESupportsConditionExpectedCloseParen); > SkipUntil(')'); > return false; > } > > it seems wrong to return false when there is a matching ')', since that > means that general_enclosed was matched. (aConditionMet should be false, > but the return value should be true.) Did you mean "when there is no matching ')'", or did you miss the "!" in the code? Either way, though, if we don't encounter the ')' but we do encounter EOF, we want to return true. So that's another change to make.

(In reply to Cameron McCormack (:heycam) from comment #3) > Either way, though, if we don't encounter the ')' but we do encounter EOF, > we want to return true. So that's another change to make. Oh, my mistake; ExpectSymbol(')') will already return true if it encounters EOF.

(In reply to Cameron McCormack (:heycam) from comment #3) > > I'm also a bit confused by the very last bit of > > CSSParserImpl::ParseSupportsConditionInParens given the current spec: > > > > if (!(ExpectSymbol(')', true))) { > > REPORT_UNEXPECTED_TOKEN(PESupportsConditionExpectedCloseParen); > > SkipUntil(')'); > > return false; > > } > > > > it seems wrong to return false when there is a matching ')', since that > > means that general_enclosed was matched. (aConditionMet should be false, > > but the return value should be true.) > > Did you mean "when there is no matching ')'", or did you miss the "!" in the > code? Looking again, you're broader point is right. If we didn't find a ')' at this point, it means that the *outer* parens formed a general_enclosed. Testing, @supports ((color: green) +) or (color: green) { body { background-color: green; } } does not apply the background-color, but it should.

(In reply to Cameron McCormack (:heycam) from comment #5) > Looking again, you're broader point is right. If we didn't find a ')' at > this point, it means that the *outer* parens formed a general_enclosed. *your

Filed 925626 for that issue.

(In reply to Cameron McCormack (:heycam) from comment #5) > (In reply to Cameron McCormack (:heycam) from comment #3) > > > I'm also a bit confused by the very last bit of > > > CSSParserImpl::ParseSupportsConditionInParens given the current spec: > > > > > > if (!(ExpectSymbol(')', true))) { > > > REPORT_UNEXPECTED_TOKEN(PESupportsConditionExpectedCloseParen); > > > SkipUntil(')'); > > > return false; > > > } > > > > > > it seems wrong to return false when there is a matching ')', since that > > > means that general_enclosed was matched. (aConditionMet should be false, > > > but the return value should be true.) > > > > Did you mean "when there is no matching ')'", or did you miss the "!" in the > > code? > > Looking again, you're broader point is right. If we didn't find a ')' at > this point, it means that the *outer* parens formed a general_enclosed. Right -- the !ExpectSymbol means that there wasn't a ')' immediately, but it's presumably still coming later -- either at the point SkipUntil skips until, or if that's EOF, then the ')' implied by the EOF.

With checking for '}' removed from CheckEndSupportsCondition.

So now that bug 925626 landed, why is the code change here needed? It seems like that should cover this. (And it avoids "looking ahead" to see if the current structure is done, which I think tends to lead to problems more often than not.) If you agree it's not needed, maybe land just the tests? (Part 25 of bug 773296 (CSS variables) currently depends on this added function, but shouldn't after addressing my review comments on it in bug 773296 comment 150.)

Yes, I agree it's no longer needed, and I'll just land the tests.