Last Comment Bug 511909 - [css3-conditional] allow @-rules inside of @media and @-moz-document
: [css3-conditional] allow @-rules inside of @media and @-moz-document
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: mozilla11
Assigned To: Jet Villegas (:jet)
:
Mentors:
: 567573 683632 (view as bug list)
Depends on:
Blocks: 709578
  Show dependency treegraph
 
Reported: 2009-08-21 10:00 PDT by Andreas Jung
Modified: 2012-04-08 08:07 PDT (History)
6 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (2.46 KB, text/html)
2009-08-21 10:01 PDT, Andreas Jung
no flags Details
Testcase (1.23 KB, text/html)
2009-08-21 10:03 PDT, Andreas Jung
no flags Details
Proposed patch (2.34 KB, patch)
2011-12-02 17:48 PST, Jet Villegas (:jet)
dbaron: review-
Details | Diff | Review
Test case for nested @media and @-moz-document (4.26 KB, text/html)
2011-12-08 16:56 PST, Jet Villegas (:jet)
dbaron: feedback+
Details
Revised patch per review comments. (6.35 KB, patch)
2011-12-08 17:03 PST, Jet Villegas (:jet)
dbaron: review-
Details | Diff | Review
Revised patch per review comments. (16.46 KB, patch)
2011-12-09 16:27 PST, Jet Villegas (:jet)
dbaron: review+
Details | Diff | Review
Final patch per review comments. r=dbaron (16.42 KB, patch)
2011-12-09 16:45 PST, Jet Villegas (:jet)
no flags Details | Diff | Review

Description Andreas Jung 2009-08-21 10:00:55 PDT
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.
Comment 1 Andreas Jung 2009-08-21 10:01:51 PDT
Created attachment 395858 [details]
Testcase
Comment 2 Andreas Jung 2009-08-21 10:03:17 PDT
Created attachment 395859 [details]
Testcase
Comment 3 Andreas Jung 2011-04-23 09:42:14 PDT
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.
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-11-30 18:58:09 PST
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).
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-11-30 18:59:07 PST
*** Bug 567573 has been marked as a duplicate of this bug. ***
Comment 6 Jet Villegas (:jet) 2011-12-02 17:48:55 PST
Created attachment 578792 [details] [diff] [review]
Proposed patch

Patch allows nested @rules. Could probably use better error handling. Need feedback.
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-12-02 19:32:31 PST
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.
Comment 8 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-12-02 19:42:36 PST
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.
Comment 9 Jet Villegas (:jet) 2011-12-08 16:56:21 PST
Created attachment 580247 [details]
Test case for nested @media and @-moz-document

Test-case in mochitest format.
Comment 10 Jet Villegas (:jet) 2011-12-08 17:03:44 PST
Created attachment 580255 [details] [diff] [review]
Revised patch per review comments.
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-12-09 15:35:25 PST
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.)
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-12-09 16:03:07 PST
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.
Comment 13 Jet Villegas (:jet) 2011-12-09 16:27:23 PST
Created attachment 580584 [details] [diff] [review]
Revised patch per review comments.

Includes C++ changes requested, new error string, test case, and makefile changes.
Comment 14 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-12-09 16:36:15 PST
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.
Comment 15 Jet Villegas (:jet) 2011-12-09 16:45:04 PST
Created attachment 580586 [details] [diff] [review]
Final patch per review comments. r=dbaron
Comment 16 Boris Zbarsky [:bz] 2011-12-14 22:27:37 PST
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
Comment 18 Camille Bissuel 2012-04-08 08:07:47 PDT
*** Bug 683632 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.