Closed
Bug 1364260
Opened 8 years ago
Closed 7 years ago
stylo: Parsing difference for @supports condition between stylo and gecko
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: xidorn, Assigned: xidorn)
References
Details
Attachments
(1 file)
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 2•8 years ago
|
||
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•8 years ago
|
||
> 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)
Comment 5•8 years ago
|
||
(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)
Updated•8 years ago
|
Assignee: nobody → simon.sapin
Priority: -- → P2
Assignee | ||
Comment 6•7 years 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
Comment 7•7 years ago
|
||
(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•7 years 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 years 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 years ago
|
||
The first case would be fixed in servo/servo#17813.
Comment 12•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years 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.
Description
•