Closed Bug 887741 Opened 6 years ago Closed 6 years ago

Allow CSS at-rules in declaration lists

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: SimonSapin, Assigned: SimonSapin)

References

()

Details

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

Attachments

(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.)

http://www.w3.org/TR/CSS21/page.html#page-box

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.

http://www.w3.org/blog/CSS/2013/06/10/resolutions-94/
http://dev.w3.org/csswg/css-syntax/#parse-a-list-of-declarations

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: https://tbpl.mozilla.org/?tree=Try&rev=2fca9551cc28
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

r=dbaron
Attachment #777808 - Flags: review?(dbaron) → review+
Is testharness.js supported in Mozilla code? https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing doesn’t mention it. Anyway, this should be easy enough to port.
Attachment #777808 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0f097f7ae11d
Status: NEW → RESOLVED
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.