[css3-conditional] allow @-rules inside of @media and @-moz-document

RESOLVED FIXED in mozilla11

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: Andreas Jung, Assigned: jet)

Tracking

({dev-doc-complete})

Trunk
mozilla11
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.3pre) Gecko/20090819 Shiretoko/3.5.3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.3pre) Gecko/20090819 Shiretoko/3.5.3pre

It would be useful (e.g. for user styles) if you could write a style for a specific domain that affects only screen / only print / ... layout.
This is not possible at the moment, because the @ rules can't be nested.
See testcase.

Reproducible: Always

Steps to Reproduce:
1.Open the testcase.

Actual Results:  
All lines should be green, but only the lines which use either @media or @-moz-document are green. The other two lines which use @media and @-moz-document are red.


Expected Results:  
All lines should be green.
(Reporter)

Comment 1

8 years ago
Created attachment 395858 [details]
Testcase
(Reporter)

Comment 2

8 years ago
Created attachment 395859 [details]
Testcase
Attachment #395858 - Attachment is obsolete: true
(Reporter)

Comment 3

6 years ago
Bug 652014 seems to indicate that this is by design / by the CSS standard.

If that's the case it is a pity because this means that it is only possible to create a user style that is site and media specific by using stupid hacks or not possible at all.

It would be really nice if there was a solution for this problem.
css3-conditional proposes changing the standard here, and WebKit already implements that.

Basically, inside of @media and @-moz-document we should change to accepting any @-rules that can occur after a style rule (in other words, @-rules other than the three that have to be at the beginning of the sheet: @charset, @import, and @namespace).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: @media and @-moz-document can't be used combined → [css3-conditional] allow @-rules inside of @media and @-moz-document
Duplicate of this bug: 567573
OS: Windows XP → All
Priority: -- → P3
Hardware: x86 → All
(Assignee)

Comment 6

6 years ago
Created attachment 578792 [details] [diff] [review]
Proposed patch

Patch allows nested @rules. Could probably use better error handling. Need feedback.
Assignee: nobody → jet
Status: NEW → ASSIGNED
Attachment #578792 - Flags: review?(dbaron)
Comment on attachment 578792 [details] [diff] [review]
Proposed patch

>diff -r 87da67ee3e59 layout/style/nsCSSParser.cpp
>--- a/layout/style/nsCSSParser.cpp	Mon Nov 21 02:55:50 2011 -0500
>+++ b/layout/style/nsCSSParser.cpp	Fri Dec 02 17:45:20 2011 -0800
>@@ -336,7 +336,8 @@
> 
>   bool ParseRuleSet(RuleAppendFunc aAppendFunc, void* aProcessData,
>                       bool aInsideBraces = false);
>-  bool ParseAtRule(RuleAppendFunc aAppendFunc, void* aProcessData);
>+  bool ParseAtRule(RuleAppendFunc aAppendFunc, void* aProcessData,
>+                      bool aInAtRule = false);

Local style is that "bool aInAtRule" should line up with "RuleAppendFunc aAppendFunc".  It's not being followed a few lines above as a result of the automated PRBool -> bool conversion that happened recently.

Additionally, it seems wrong to have a default value for this parameter -- it's better for all callers to pass it explicitly so that it's clear there's a choice to be made.

> bool
> CSSParserImpl::ParseAtRule(RuleAppendFunc aAppendFunc,
>-                           void* aData)
>+                           void* aData, 
>+                           bool aInAtRule /* =false */)

So ParseAtRule needs to do an additional check based on aInAtRule, since we don't want to allow any of the rules that set |newSection| to something other than eCSSSection_General inside of other @-rules.  That could be done by changing:
  if (!(this->*parseFunc)(aAppendFunc, aData)) {
to:
  // Inside of @-rules, only the rules that can occur anywhere
  // are allowed.
  if ((aInAtRule && newSection != eCSSSection_General) ||
      !(this->*parseFunc)(aAppendFunc, aData)) {


Additionally, we don't want ParseAtRule to mutate mSection when aInAtRule is true; mSection is about what section we're up to at the top-level of the sheet. 


>-{
>-  // If we ever allow nested at-rules, we need to be very careful about
>-  // the error handling rules in the CSS spec.  In particular, we need
>-  // to pass in to ParseAtRule whether we're inside a block, we need to
>-  // ensure that all the individual at-rule parsing functions terminate
>-  // immediately when they hit a '}', and then we need to pass whether
>-  // we're inside a block to SkipAtRule below.

So this is the hard part -- auditing ParseMediaRule, ParseMozDocumentRule, ParseFontFaceRule, ParsePageRule, and ParseKeyframesRule to make sure they behave correctly (though you can ignore anything they do inside of another pair of {}).

ParsePageRule is obviously correct.

ParseKeyframesRule is also correct (not much of interest outside the {}).

ParseFontFaceRule the same (even less of interest outside the {}).

ParseMozDocumentRule, however, needs a bunch of work: it's missing an UngetToken call in the failure case near the start of the function -- but we should only call UngetToken if the GetToken call succeeded, so the condition there needs to be split.  I think the other two failure cases there are fine, though.

ParseMediaRule is the most complicated, so I left it for last.  The only interesting part is whether GatherMedia behaves correctly.  GatherMedia, in turn, requires an *additional* boolean input since its aInAtRule currently refers to whether it's *for* an at-rule, for which the parsing rules are slightly different for whether it's *in* an at-rule.  GatherMedia currently has an option to terminate its SkipUntil call on "," or to terminate on ",", "{", or ";", but we need it to terminate on "}" as well (I think only combined with the latter list), and also to UngetToken on that "}" like it does for "{" and ";".

Then (inside of GatherMedia) there's also the question of whether ParseMediaQuery is ok.  It also has an aInAtRule parameter and also needs a second parameter just like GatherMedia, since it has a very similar check about 15 lines in, and again about 10 lines in to its main for loop.  Other than that those two changes it looks ok.

>@@ -1976,7 +1973,7 @@
>   if (!ExpectSymbol('{', true)) {
>     return false;
>   }
>-
>+  

Looks like your editor introduced some trailing whitespace here.


This also needs some automated tests, probably mochitests (to be placed in layout/style/test/).  See:
https://developer.mozilla.org/en/Mochitest
along with the other tests in the directory.  (Tests should probably test both that the object model is what is expected and that the nested evaluation of the rules works correctly.  It would also be good to test the cases that should be errors, e.g., @import inside @media, via the object model.)  It would probably also be good to have tests for the interesting cases I described above that need fixing.

The parts you've done look good, but as I've said there's a bit more needed here.
Attachment #578792 - Flags: review?(dbaron) → review-
Also, it may be a useful exercise to at try writing the tests for the things that I suggest need fixing *before* doing the fixes:  both to understand why I'm asking for those changes and to check that the fixes are fixing something.
(Assignee)

Comment 9

6 years ago
Created attachment 580247 [details]
Test case for nested @media and @-moz-document

Test-case in mochitest format.
Attachment #395859 - Attachment is obsolete: true
Attachment #580247 - Flags: feedback?(dbaron)
(Assignee)

Comment 10

6 years ago
Created attachment 580255 [details] [diff] [review]
Revised patch per review comments.
Attachment #578792 - Attachment is obsolete: true
Attachment #580255 - Flags: review?(dbaron)
Attachment #395859 - Attachment is obsolete: false
Comment on attachment 580255 [details] [diff] [review]
Revised patch per review comments.

You should set up your hg diff settings per:
https://developer.mozilla.org/en/Installing_Mercurial#Configuration
(In particular, I'm mostly missing the showfunc = 1.)

>+  // Inside of @-rules, only the rules that can occur anywhere
>+  // are allowed.
>+  if ((aInAtRule && newSection != eCSSSection_General) ||
>+      !(this->*parseFunc)(aAppendFunc, aData)) {
>     // Skip over invalid at rule, don't advance section
>     OUTPUT_ERROR();

I realized I missed something about this one last time:  there's an
error reporting issue here.  If the first half of the || fails, we also
want to report an error to the console.

There's actually already an appropriate message -- which I probably
should have noticed you removing the last time around and asked you to
remove from the .properties file.  However, since you can reuse it, it
should stay.  But see:
http://mxr.mozilla.org/mozilla-central/search?string=PEGroupRuleNestedAtRu

So you probably want something like:

  bool unnestable = aInAtRule && newSection != eCSSSection_General;
  if (unnestable) {
    REPORT_UNEXPECTED_TOKEN(PEGroupRuleNestedAtRule);
  }
  if (unnestable || !(this->*parseFunc)(aAppendFunc, aData)) {
    // Skip over invalid at rule, don't advance section
    OUTPUT_ERROR();
    ...

>+  if(!aInAtRule){

local style has a space both before ( and after ).

>+    if (!GetToken(true)){
>+      REPORT_UNEXPECTED_EOF(PEGroupRuleEOF);
>+      delete urls;
>+      return false;  

This probably deserves its own error message rather than reusing
PEGroupRuleEOF.

Also, there are two spaces after the "return false".



Finally, the mochitest needs to be incorporated into the patch.  It
should be put in a file in layout/style/test and added to the makefile.


r=dbaron with those things fixed... though I should probably take a
quick look at the revised patch.

(I also need to look at the test, which I'll do shortly.)
Attachment #580255 - Flags: review?(dbaron) → review-
Comment on attachment 580247 [details]
Test case for nested @media and @-moz-document

>  #red {
>    color: red;
>  }

Generally we don't use red as a pass condition.  Pick another color?


><!-- should parse -->
><style type="text/css">
>
>@media screen {
>  @-moz-document regexp(".*test_bug511909.*"),}
>    #mx {
>        color: red;
>    }
>  }
>}
>
></style>
>  
><!-- should parse -->
><style type="text/css">
>    
>@-moz-document regexp(".*test_bug511909.*"){
>  @media screen ,}
>    #mxx {
>      color: blue;
>    }
>  }
>}
>
></style>

In these two cases, you should probably change the "screen" to "print" and the "test_bug5119099" to "not_this_url" so that it's clear that the rules actually aren't inside an @-rule.


Other than that, this looks fine, although it's not the most efficient way to write a mochitest for this sort of thing.  With a more javascript-y approach like what, say, test_selectors.html uses, it's easier to add a lot of tests quickly.  But this is probably fine for this.
Attachment #580247 - Flags: feedback?(dbaron) → feedback+
(Assignee)

Comment 13

6 years ago
Created attachment 580584 [details] [diff] [review]
Revised patch per review comments.

Includes C++ changes requested, new error string, test case, and makefile changes.
Attachment #580255 - Attachment is obsolete: true
Attachment #580584 - Flags: review?(dbaron)
Comment on attachment 580584 [details] [diff] [review]
Revised patch per review comments.

r=dbaron if you incorporate comment 12.

Also, best to not have the "no newline at end of file" for the test.
Attachment #580584 - Flags: review?(dbaron) → review+
(Assignee)

Comment 15

6 years ago
Created attachment 580586 [details] [diff] [review]
Final patch per review comments. r=dbaron
Attachment #580584 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed

Updated

6 years ago
Blocks: 709578

Updated

6 years ago
Keywords: dev-doc-needed
For future reference, it would be really nice to have a User line and checkin comment in the diff.

https://hg.mozilla.org/mozilla-central/rev/ba1d8b3a53e4
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
This one was complex to document.
I've created:
https://developer.mozilla.org/en/CSS/Syntax where, among other things, nested statements are defined
https://developer.mozilla.org/en/CSS/At-rule where Conditional Group Rules are defined

and updated the at-rules articles:
https://developer.mozilla.org/en/CSS/@media
https://developer.mozilla.org/en/CSS/@charset
https://developer.mozilla.org/en/CSS/@import

https://developer.mozilla.org/en/CSS/@keyframes
https://developer.mozilla.org/en/CSS/@document (for @-moz-document)
https://developer.mozilla.org/en/CSS/@font-face
https://developer.mozilla.org/en/CSS/@page

and of course Fx 11 for devs: https://developer.mozilla.org/en/Firefox_11_for_developers
Assignee: jet → Jan.Varga
Component: Style System (CSS) → SQL
Keywords: dev-doc-needed → dev-doc-complete
QA Contact: style-system → owen.marshall+bmo
Version: unspecified → Trunk

Updated

6 years ago
Assignee: Jan.Varga → jet
Component: SQL → Style System (CSS)
QA Contact: owen.marshall+bmo → style-system

Updated

5 years ago
Duplicate of this bug: 683632
You need to log in before you can comment on or make changes to this bug.