Closed Bug 431948 Opened 14 years ago Closed 13 years ago

An isolated semi-colon invalidates a whole at-media block

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed
status1.9.1 --- wontfix

People

(Reporter: bugzilla, Assigned: zwol)

References

Details

(Keywords: testcase, verified1.9.2)

Attachments

(2 files, 3 obsolete files)

Imagine I type fast and place a semi-colon outside a ruleset, like this:

body {color: red;}

h1 {color: green};

The CSS parser will report 2 warnings:

"
Warning: Selector expected.  Ruleset ignored due to bad selector.
and
Warning: Unexpected end of file while searching for closing } of invalid rule set.
"
and the <h1> node will be nevertheless green.

Now, if I do

body {color: red;}

@media screen
{
  h1 {color: green}

  ;
}

then I get a 3rd warning
"
Warning: Unexpected end of file while searching for closing } of invalid rule set.  Unexpected end of file while searching for end of @media or @-moz-document rule.  Unrecognized at-rule or error parsing at-rule '
'.
"
and the <h1> node becomes red, not green. 

Other browsers (Opera 9.27, Opera 9.50, IE 8 beta 1) render the <h1> node green.

Steps to reproduce:
Load upcoming testcase

Actual results in Seamonkey 2.0a1pre rv:1.9pre build 2008050202 under XP Pro:
<h1> node is red

Expected results:
<h1> node is green

Notes:
https://bugs.webkit.org/show_bug.cgi?id=14346#c4
http://samples.msdn.microsoft.com/csstestpages/Chapter_4/invalid-decl-at-rule-001.htm
Reduced testcase
Keywords: testcase
Another important and relevant point is that CSS 2.1 validation will report 2 errors (grammar parsing) and will report the "Valid CSS information" as


body  {
color : red;
} 

@media screen  {
h1 {
color : green;
}
} 

http://jigsaw.w3.org/css-validator/validator?uri=https%3A%2F%2Fbugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D319093&profile=css21&usermedium=all&warning=2&lang=en
I searched for a duplicate and the closest I could find is bug 231652 but this isn't bug 231652.
CONFIRMING
Status: UNCONFIRMED → NEW
Ever confirmed: true
I think this is a duplicate of another bug on matching braces.  (I know it's been discussed on www-style; I'm not sure whether it was filed.)  In particular, brace-matching of the container needs to be higher priority than eating the next rule.
I have searched again for a duplicate (searching for a comment with brac & match and only brac and only match) with you as a commenter, with or without Style System as component and did not find such duplicate.
Could it be bug 193657 ? bug 427688 ?
No.  Don't worry about it.
Duplicate of this bug: 439370
Duplicate of this bug: 422764
Transferring schrep's nomination.
Flags: wanted1.9.1?
Flags: wanted1.9.1? → wanted1.9.1+
Here's a possible patch for this bug.  It's simple and possibly too eager -- it causes nsCSSParser::SkipRuleset() to return whenever it encounters a top-level semicolon.  That neatly handles this case, but also means that a construct like this:

  a ; b { color: red }

will discard the "a;" and treat "b { color: red }" as a valid rule with selector "b".  In the abstract, I think this is the Right Thing.  I do see that there are two subcases of style/test/test_parse_rule.html that are specifically looking for "b { color: red }" to *not* be a valid rule, and it also breaks Acid2, but again, I think that's abstractly the wrong error recovery strategy.  I haven't checked exactly what the standard says -- I dimly remember this coming up on w3-style but not what the resolution was, if any.
Assignee: nobody → zweinberg
Status: NEW → ASSIGNED
Attachment #370554 - Flags: superreview?(dbaron)
Attachment #370554 - Flags: review?(dbaron)
In the alternative, perhaps we can stop skipping only when we come to a top-level close brace.  This is good enough to pass test_parse_rule.html but still not enough to pass Acid2, but I can't figure out what Acid2 wants that's different.  Also there may still be a bunch of reftest failures, it's still running.
Attachment #370560 - Flags: superreview?(dbaron)
Attachment #370560 - Flags: review?(dbaron)
actually seems like there aren't any new reftest failures with patch #2.
Attachment #370554 - Flags: superreview?(dbaron)
Attachment #370554 - Flags: superreview-
Attachment #370554 - Flags: review?(dbaron)
Attachment #370554 - Flags: review-
Comment on attachment 370554 [details] [diff] [review]
patch: stop skipping a ruleset at a top-level semicolon

This one isn't what we want to do; the other patch is closer.
Comment on attachment 370560 [details] [diff] [review]
alternative patch: stop skipping at an outer close brace

This is close to what we want to do, but not quite.

The only change I think we should make is that you should make this change effective *only* when we're parsing rules inside of a {}.  So you should pass a boolean to ParseRuleSet and SkipRuleSet indicating whether you're inside of a block, and stop skipping on } only when you are.

A testcase to test that should be:

#a{color:green}
foo(} #a{color:red}) {}
#b{color:green}

r=dbaron with that changed... but I probably want to look at the new patch.


Sorry for taking so long to get to this.
Attachment #370560 - Flags: superreview?(dbaron)
Attachment #370560 - Flags: review?(dbaron)
Attachment #370560 - Flags: review-
Here's the revised patch, doing exactly as you suggested.  Limiting the special treatment of '}' to within @-rules appears to make Acid2 happy.  Reftests are running now.
Attachment #370554 - Attachment is obsolete: true
Attachment #370560 - Attachment is obsolete: true
Attachment #388284 - Flags: review?(dbaron)
Attachment #388284 - Flags: review?(dbaron) → review+
Comment on attachment 388284 [details] [diff] [review]
revised patch that only changes the behavior inside @-rules

>-  void SkipRuleSet();
>+  void SkipRuleSet(PRBool aNested);

Maybe call aNested aInsideBraces instead?

r=dbaron

Sorry for the delay (again).
With s/aNested/aInsideBraces/ as requested; no other changes.
Attachment #395164 - Flags: review+
Keywords: checkin-needed
Flags: wanted1.9.2?
Attachment #388284 - Attachment is obsolete: true
Comment on attachment 395164 [details] [diff] [review]
final patch
[Checkin: Comment 19]


http://hg.mozilla.org/mozilla-central/rev/d57c7bf46486
Attachment #395164 - Attachment description: final patch → final patch [Checkin: Comment 19]
Attachment #395164 - Flags: approval1.9.2?
Attachment #395164 - Flags: approval1.9.1.3?
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Comment on attachment 395164 [details] [diff] [review]
final patch
[Checkin: Comment 19]

We should not take this on 1.9.1.
Attachment #395164 - Flags: approval1.9.1.3?
Flags: wanted1.9.1+
Hm, why not?  1.9.1 is only barely out and the bug is old.
We shouldn't make Web-compatibility or extension-compatibility changes on stable branches after the first release on them unless we have to for security.
(And certainly not before we ship betas of them on a later release branch.  We really get negligible testing on security updates before we ship them to large numbers of users.)
Comment on attachment 395164 [details] [diff] [review]
final patch
[Checkin: Comment 19]

a1.9.2=dbaron
Attachment #395164 - Flags: approval1.9.2? → approval1.9.2+
Keywords: checkin-needed
Whiteboard: [c-n: 1.9.2]
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090902 Minefield/3.7a1pre (.NET CLR 3.5.30729)

Also verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a2pre) Gecko/20090904 Namoroka/3.6a2pre (.NET CLR 3.5.30729)
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2
Flags: wanted1.9.2?
Flags: wanted1.9.2-
Flags: blocking1.9.2-
You need to log in before you can comment on or make changes to this bug.