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

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
3 months ago
6 days ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks: 2 bugs)

53 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

3 months ago
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.
(Assignee)

Comment 1

3 months ago
Simon, what do you think about this?
Flags: needinfo?(simon.sapin)
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.
(Assignee)

Updated

3 months ago
Duplicate of this bug: 1363662
> 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?
Flags: needinfo?(simon.sapin) → needinfo?(dbaron)
(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.
Flags: needinfo?(dbaron)
Assignee: nobody → simon.sapin
Priority: -- → P2
(Assignee)

Comment 6

8 days ago
Going to align Gecko with the spec (and Stylo) for the second case, and submit a fix for the first case to Servo.
Assignee: simon.sapin → xidorn+moz
(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 hidden (mozreview-request)

Comment 9

8 days ago
mozreview-review
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
Attachment #8888169 - Flags: review?(dbaron) → review+

Comment 10

7 days ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9c16700eed14
Don't require whitespace between keywords and parens in supports rule condition. r=dbaron
(Assignee)

Comment 11

7 days ago
The first case would be fixed in servo/servo#17813.
https://hg.mozilla.org/mozilla-central/rev/9c16700eed14
Status: NEW → RESOLVED
Last Resolved: 6 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.