1364260 - stylo: Parsing difference for @supports condition between stylo and gecko

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Xidorn Quan [:xidorn] UTC+10]

There are two differences revealed from layout/style/test/test_supports_rules.html

1. whitespace between parens and declaration is stripped in Stylo:
> @supports ( Font:  20px serif ! Important) {}
rule.conditionText:
> Gecko: ( Font:  20px serif ! Important)
> Stylo: (Font:  20px serif ! Important)
Blink and Edge both match Gecko's behavior, so Stylo should change. The spec doesn't seem to be clear about this.

2. whitespace is a necessary separator between keywords ('and', 'or', 'not') and parents in Gecko, but not in Stylo:
* not/* */(colour: green)
* (color: green) and/* */(color: blue)
* (color: green)and (color: blue)
* (color: green) or/* */(color: blue)
* (color: green)or (color: blue)
All above do not match in Gecko, but match in Stylo.
Blink matches Gecko's behavior, and Edge kind of matches Gecko's behavior (in Edge, whitespace before keywords is not necessary, but that after is), so I suppose Stylo should change as well. The spec seems to support Stylo's behavior, though.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Simon, what do you think about this?

Comment 2

[Manish Goregaokar [:manishearth]]

layout/reftests/bugs/883987-1*.html also have @supports parsing differences, but here stylo matches chrome.


I feel like we should match the spec here for 2 since it results in us being more permissive.

Comment 4

[Simon Sapin (:SimonSapin)]

> The spec doesn't seem to be clear about this.

CSS serialization is generally very under-defined in specs. It is not clear to me how much of the details matter for web-compat.

For 1. this is because we store separately the property name and the value, and serialize like this:

impl ToCss for Declaration {
    fn to_css<W>(&self, dest: &mut W) -> fmt::Result
        where W: fmt::Write {
        dest.write_str(&self.prop)?;
        dest.write_str(":")?;
        // no space, the `val` already contains any possible spaces
        dest.write_str(&self.val)
    }
}

Note that we store the value as a string in CSS syntax, we throw away the result of value parsing.

By the way there’s at least on bug here, the property should be serialized with cssparser::serialize_identifier (though this only makes a difference for weird custom proprerty names like in @supports (--\@: foo) {}.

We can fix both this and the missing whitespace by storing a single string in CSS syntax for the entire declaration. But while we’re at it, why not do so for the entire condition? Why do we have a SupportsCondition enum at all? Do we want to preserve whitespace and comments in other places of the condition? Do we want to *not* preserve it?


For 2. my reading of the grammar at https://drafts.csswg.org/css-conditional/#at-supports is that whitespace is not required. An extract of it (inlined) is `OR S* (`. Both "or (" and "or/**/(" match that extract: an identifier token, zero or more whitespace characters, and a parenthesis. But "or(" does not because it is a function token.

I vaguely recall CSSWG discussing this and proposing to require whitespace to avoid the confusion with function tokens, similar to how calc(1em + 10px) requires whitespace around + or - so that it’s not tokenized as a positive or negative sign of the following number.

David (since you’re editor of this on), did the css-conditional spec change on this aspect?

Comment 5

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

(In reply to Simon Sapin (:SimonSapin) from comment #4)
> For 2. my reading of the grammar at
> https://drafts.csswg.org/css-conditional/#at-supports is that whitespace is
> not required. An extract of it (inlined) is `OR S* (`. Both "or (" and
> "or/**/(" match that extract: an identifier token, zero or more whitespace
> characters, and a parenthesis. But "or(" does not because it is a function
> token.
> 
> I vaguely recall CSSWG discussing this and proposing to require whitespace
> to avoid the confusion with function tokens, similar to how calc(1em + 10px)
> requires whitespace around + or - so that it’s not tokenized as a positive
> or negative sign of the following number.
> 
> David (since you’re editor of this on), did the css-conditional spec change
> on this aspect?

It was implemented in bug 841983, which points to a spec change that added the requirement for whitespace.

But that spec change was reverted in https://hg.csswg.org/drafts/rev/3091bbbdc67a and https://hg.csswg.org/drafts/rev/ad12dd71963c .

I thought we had a bug filed on reverting it, but I can't find it.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Going to align Gecko with the spec (and Stylo) for the second case, and submit a fix for the first case to Servo.

Comment 7

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

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #5)
> I thought we had a bug filed on reverting it, but I can't find it.

I might have been thinking of bug 1031976, which was (a) for media queries and (b) for a change in the opposite direction.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1364260 - Don't require whitespace between keywords and parens in supports rule condition.

Review commit: https://reviewboard.mozilla.org/r/159086/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159086/

Comment 9

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

Comment on attachment 8888169 [details]
Bug 1364260 - Don't require whitespace between keywords and parens in supports rule condition.

https://reviewboard.mozilla.org/r/159086/#review164488

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c16700eed14
Don't require whitespace between keywords and parens in supports rule condition. r=dbaron

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

The first case would be fixed in servo/servo#17813.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9c16700eed14