814566 - allow invalid syntax with balanced brackets inside @supports conditions

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

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.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: patch

I changed the PESkipAtRuleEOF string since it's used for skipping to the end of both known and unknown at-rules.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: patch (v1.1)

I overlooked that "()" should be a valid condition that evaluates to false.

Comment 3

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

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

Comment 4

[Cameron McCormack (:heycam)]

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

?

Comment 5

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7023232a96f5

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7023232a96f5