Closed Bug 814566 Opened 7 years ago Closed 7 years ago

allow invalid syntax with balanced brackets inside @supports conditions

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: heycam, Assigned: heycam)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

To update to changes in the css3-conditional spec, we need to successfully parse invalid syntax inside the parentheses of a condition (as long as it has balanced brackets) and have it evaluate to false.  There's also the new functional notation for conditions that initially all also evaluate to false.
Attached patch patch (obsolete) — Splinter Review
I changed the PESkipAtRuleEOF string since it's used for skipping to the end of both known and unknown at-rules.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #684596 - Flags: review?(dbaron)
Attached patch patch (v1.1)Splinter Review
I overlooked that "()" should be a valid condition that evaluates to false.
Attachment #684596 - Attachment is obsolete: true
Attachment #684596 - Flags: review?(dbaron)
Attachment #684913 - Flags: review?(dbaron)
Blocks: 814923
Blocks: 779917
Comment on attachment 684913 [details] [diff] [review]
patch (v1.1)

>Bug 814566 - Allow invalid syntax (with balanced brackets) inside @supports conditions. (v1.1) r?

s/brackets/parentheses/

(In US English, we have:
  parentheses: ()
  brackets: []
  braces: {}
.  I realize Imperial English doesn't match, but parentheses
is unambiguous and brackets is confusing to many.)

The same goes for the <meta name="assert"> for a bunch of the tests.


css.properties:
  Whenever you change the semantics of a localized string,
  you should also change the key so that localizers know
  they have to re-translate it.  At minimum, you can do
  this by adding a "2" to the end of the name.

  (This requires matching changes in nsCSSParser.cpp.)

>+PESupportsConditionStartEOF='not', '(' or function

Please use a comma before the "or".

>+PESupportsConditionExpectedStart=Expected 'not', '(' or function while parsing supports condition but found '%1$S'.

Ditto.

>diff --git a/layout/reftests/w3c-css/submitted/conditional3/css-supports-032.xht b/layout/reftests/w3c-css/submitted/conditional3/css-supports-032.xht

The assert should probably mention evaluating to false too.

And the test title is clearly wrong (maybe copy from 033?).

(I skimmed some of the later tests, though.)

>-  void SkipUntil(PRUnichar aStopSymbol);
>+  bool SkipUntil(PRUnichar aStopSymbol);

Comment that it returns true when the stop symbol is found and false
for EOF?

>+  if (mToken.IsSymbol('(') ||
>+      mToken.mType == eCSSToken_Function ||
>+      mToken.mType == eCSSToken_Bad_URL) {

I think treating URL and Bad_URL differently is probably a bad
idea.  I think you should send eCSSToken_URL into ParseSupportsCondition
too, but for that case it should just set the condition to false
and return without looking for a ).


ParseSupportsConditionInParens needs to be more careful to UngetToken()
when it finds something that it doesn't accept.  In particular, the
check for:
>+  if (mToken.IsSymbol(')')) {
>+    aConditionMet = false;
>+    return true;
>+  }
is bogus and should be removed, but it's covering up a missing (and more
general) bug with missing UngetToken call or calls.  In particular, you
need an UngetToken in this if:
>+  if (!mToken.IsSymbol('(')) {
>+    REPORT_UNEXPECTED_TOKEN(PESupportsConditionExpectedOpenParenOrFunction);
>     return false;
>   }
(Note that ExpectSymbol has an UngetToken inside it.)

(Note that the function really has three possible results: consumed a
condition, hit EOF, or consumed nothing.  It doesn't look to me like
there are any callers that care about the difference between the second
and third, so I think you should be ok, but you should double-check my
judgment that none of the callers care.)

r=dbaron with those things fixed
Attachment #684913 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] from comment #3)
> >+  if (mToken.IsSymbol('(') ||
> >+      mToken.mType == eCSSToken_Function ||
> >+      mToken.mType == eCSSToken_Bad_URL) {
> 
> I think treating URL and Bad_URL differently is probably a bad
> idea.  I think you should send eCSSToken_URL into ParseSupportsCondition
> too, but for that case it should just set the condition to false
> and return without looking for a ).

Should the grammar in the spec be updated to:

  general_enclosed
    : ( FUNCTION | URI | BAD_URI | '(' ) (any|unused)* ')' S*

?
https://hg.mozilla.org/mozilla-central/rev/7023232a96f5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.