Closed Bug 887741 Opened 6 years ago Closed 6 years ago

Allow CSS at-rules in declaration lists


(Core :: CSS Parsing and Computation, defect)

Not set





(Reporter: SimonSapin, Assigned: SimonSapin)




(Keywords: dev-doc-needed, Whiteboard: [DocArea=CSS])


(1 file, 1 obsolete file)

CSS 2.1 defines @page rules as containing at-rules and declarations, even though no at-rule supported in that context is defined yet. (Such rules are defined in CSS Paged Media Level 3.)

Anything unknown/invalid is already ignored by the parser, but being aware of at-rules makes a difference is where the parser recovers and starts parsing the next declaration:

@page {
    /* Level 3 stuff, not supported in Mozilla yet: */
    @bottom-center { content: counter(page) }

    margin: 3cm;

Per CSS 2.1, the @bottom-center rule ends with its {} block, and is followed by a valid declaration. Our current behavior is to consider the at-rule an invalid declaration that goes until the next semi-colon, including the 'margin' declaration, and ignore all of it.

Test case, print preview should show a big margin around "Test":
data:text/html,<!doctype html><style>@page{margin:0;@top-left{}margin:5cm}</style>Test

Sanity check: Page margins are supported at all.
data:text/html,<!doctype html><style>@page{margin:0;margin:5cm}</style>Test

Additionally, the CSS WG recently decided to extend this behavior to all declaration lists, not just @page. This is specified in Syntax Level 3. As for @page in CSS 2.1, this error recovery allows future extensions. It should be done early even if we don’t define such extensions yet.

Test case for style rules:
data:text/html,<!doctype html><style>body{background:red;@foo{}background:green}</style>Test

Test case for style attributes:
data:text/html,<!doctype html><body style="background:red;@foo{}background:green">Test

This should apply similarly to at-rules that use (descriptor) declaration syntax.
The test I added passes with this code change, but I’m waiting for push access to Try to see if this broke anything else.
Attachment #777808 - Flags: review?(dbaron)
Try results:
The failures seem unrelated.
Comment on attachment 777808 [details] [diff] [review]
First patch: implementation + one mochitest

>+      if(eCSSToken_AtKeyword == tk->mType) {

space between if and (.

>+        SkipAtRule(checkForBraces);
>+        return true;  // Not a declaration, but not not skip until ';'

s/not not/don't/
(or "do not")

It probably would have been better to write this as a testharness.js test so it could be contributed to the css3-syntax test suite, but probably not worth redoing now

Attachment #777808 - Flags: review?(dbaron) → review+
Is testharness.js supported in Mozilla code? doesn’t mention it. Anyway, this should be easy enough to port.
Attachment #777808 - Attachment is obsolete: true
Keywords: checkin-needed
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Whiteboard: [DocArea=CSS]
Depends on: 1197842
You need to log in before you can comment on or make changes to this bug.