909170 - @supports conditions with tokens after a declaration's priority should evaluate to false

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

If we have

  @supports (color: green !important anything) {
    ...
  }

this currently causes the @supports rule to fail to parse, rather than evaluate to false.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: patch

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

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 3

[Cameron McCormack (:heycam)]

(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.

Comment 4

[Cameron McCormack (:heycam)]

(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.

Comment 5

[Cameron McCormack (:heycam)]

(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.

Comment 6

[Cameron McCormack (:heycam)]

(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

Comment 7

[Cameron McCormack (:heycam)]

Filed 925626 for that issue.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 9

[Cameron McCormack (:heycam)]

Attachment: patch (v1.1)

With checking for '}' removed from CheckEndSupportsCondition.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 11

[Cameron McCormack (:heycam)]

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

Comment 12

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ff43d5afe3e

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ff43d5afe3e