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

RESOLVED FIXED in mozilla29

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

unspecified
mozilla29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
If we have

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

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

Updated

4 years ago
Summary: duplicate priorities in an @supports condition should evaluates to false → @supports conditions with tokens after a declaration's priority should evaluate to false
(Assignee)

Comment 1

4 years ago
Created attachment 795225 [details] [diff] [review]
patch

The first test I added there doesn't currently fail, but it seemed like it was worth adding.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #795225 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Blocks: 773296
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.
Attachment #795225 - Flags: review?(dbaron) → review-
(Assignee)

Comment 3

4 years ago
(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.
Flags: needinfo?(dbaron)
(Assignee)

Comment 4

4 years ago
(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.
(Assignee)

Comment 5

4 years ago
(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.
Flags: needinfo?(dbaron)
(Assignee)

Comment 6

4 years ago
(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
(Assignee)

Comment 7

4 years ago
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.
(Assignee)

Comment 9

4 years ago
Created attachment 815740 [details] [diff] [review]
patch (v1.1)

With checking for '}' removed from CheckEndSupportsCondition.
Attachment #795225 - Attachment is obsolete: true
Attachment #815740 - Flags: review?(dbaron)
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.)
Flags: needinfo?(cam)
(Assignee)

Comment 11

4 years ago
Yes, I agree it's no longer needed, and I'll just land the tests.
Flags: needinfo?(cam)
(Assignee)

Comment 12

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ff43d5afe3e
https://hg.mozilla.org/mozilla-central/rev/8ff43d5afe3e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Attachment #815740 - Flags: review?(dbaron)
You need to log in before you can comment on or make changes to this bug.