Open Bug 816045 Opened 12 years ago Updated 2 years ago

Should pass @supports W3C test

Categories

(Core :: CSS Parsing and Computation, defect)

x86
macOS
defect

Tracking

()

People

(Reporter: zcorpan, Unassigned)

References

(Blocks 1 open bug)

Details

http://hg.csswg.org/test/file/5f94e4b03ed9/contributors/opera/submitted/css3-conditional has @supports tests from Opera. I just submitted the js/ folder. Nightly passes the reftests but fails 9 of 11 subtests in http://hg.csswg.org/test/raw-file/5f94e4b03ed9/contributors/opera/submitted/css3-conditional/js/001.html
I also noticed that http://trac.webkit.org/export/131783/trunk/LayoutTests/css3/supports.html has a few fails in Opera and Firefox (apart from the test expecting the quotes to be absent, I think CSSOM requires the quotes). I haven't investigated which tests are correct per spec.
Does the spec actually define whitespace behavior?  A few of the test fails are about the exact indentation of the serialization of nested @-rules (we seem to only indent the first line; the test expects the first and last lines to be indented, a sane indentation would indent all lines).  By the way, the harness output is NOT useful for figuring this out, since it doesn't use preformatted whitespace...

For the other failures, they are in order:

1)  CSSRule.KEYFRAMES_RULE is undefined; we have CSSRule.MOZ_KEYFRAMES_RULE instead.  We
    should probably fix that, but it has nothing to do with @supports.
2)  insertRule doesn't allow nesting @-rules.  We should change that, since the parser
    does allow it.
3)  Removing a rule we never inserted obviously doesn't work.
4)  The test that expects a TypeError while calling insertRule on a non-group rule is
    clearly just broken.
5)  We don't put extra parens around the supports condition for the and/or cases (last
    two tests).
For #4, isn't TypeError the right exception to throw, because the style rule is not a CSSGroupingRule, and thus doesn't have an insertRule method?

For #5, I don't think there is anything in the spec that says what the serialization should be.  AFAICT it's still an open issue on the spec.  Our implementation returns the exact string that was used when parsing the rule, trimmed of white space on either end.
> because the style rule is not a CSSGroupingRule, and thus doesn't have an insertRule
> method?

Oh, trying to call undefined will throw a TypeError?  Yeah, that works.  Due to #2+#3 the test is currently running insertRule on a rule that actually is a group rule....
Depends on: 816431
Depends on: 816720
I changed a reftest because it looks like the spec had changed: http://hg.csswg.org/test/rev/a9c10115a98c

If the js test is wrong or should be relaxed to allow different serializations, that can be fixed in the test. I don't have time to look into that right now, but if someone else wants to fix it, feel free. :-)
I think your change to the test there is wrong: "not(" should parse correctly now but be interpreted as a "general_enclosed" that evaluates to false.
Doesn't it match supports_condition_in_parens?
The token stream for "(not(foo: baz))" is:

  '(' FUNCTION IDENT ':' S IDENT ')' ')'

which I think parses like this:

  supports_condition
    => supports_condition_in_parens
    => '(' S* supports_condition ')' S*
    => '(' supports_condition ')' S*
    => '(' supports_condition_in_parens ')' S*
    => '(' general_enclosed ')' S*
    => '(' ( FUNCTION | '(' ) (any|unused)* ')' S* ')' S*
    => '(' FUNCTION (any|unused)* ')' S* ')' S*
    => '(' FUNCTION IDENT ':' S IDENT ')' S* ')' S*
    => '(' FUNCTION IDENT ':' S IDENT ')' ')' S*
    => '(' FUNCTION IDENT ':' S IDENT ')' ')'
Hmm. I'm not sure I follow why you get to 
    => '(' supports_condition_in_parens ')' S*
rather than
    => '(' supports_negation ')' S*
but if that's the case, is that a spec bug or deliberate?
Because the input "not(" is tokenised as FUNCTION, not as the IDENT "not" followed by "(".  There needs to be a space after the "not" for it to be tokenised as an ident.  (This is despite the fact the grammar has S* after NOT in supports_negation -- the tokeniser has already matched "not(" into FUNCTION.)

I think this is deliberate.
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.