765599 - CSSStyleSheet.insertRule should throw when there are more than one rule

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Kang-Hao (Kenny) Lu [:kennyluck]]

See URL for the original spec error report. I think the spec is OK clear in this case, though.

Returning a NS_ERROR_DOM_SYNTAX_ERR if there's any more token after the conditional in [1] seems to work. It might be helpful to read [2] too, to see if some other codes can be cleaned up.

The hard part might be finding a file in /layout/style/test/ to put this test case in...

[1] http://hg.mozilla.org/mozilla-central/file/b1a0fb2bdbf7/layout/style/nsCSSParser.cpp#l1015
[2] http://hg.mozilla.org/mozilla-central/file/b1a0fb2bdbf7/layout/style/nsCSSStyleSheet.cpp#l1765

Comment 1

[rajesh kumar]

can I be assigned this bug???

Comment 2

[Kang-Hao (Kenny) Lu [:kennyluck]]

(In reply to rajesh kumar from comment #1)
> can I be assigned this bug???

Yes, and I just did that. I am sending you a mail for further instructions.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I just wrote a patch for this, after clarifying the spec, before finding this bug report.


The spec issue, from the css3-conditional disposition of comments, is:

Issue 5:
Issue in spec about insertRule, followup to Issue 7
also raised as first issue in http://lists.w3.org/Archives/Public/www-style/2013Feb/0228.html
Raised by: David Baron, Florian Rivoal
Proposed resolution: http://lists.w3.org/Archives/Public/www-style/2013Feb/0229.html
Florian agrees: http://lists.w3.org/Archives/Public/www-style/2013Feb/0228.html
Edited: http://lists.w3.org/Archives/Public/www-style/2013Feb/0256.html
(Commenter agrees, of course.)

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Make CSS insertRule methods throw SYNTAX_ERR when given an empty rule or more than one rule.

This implements the proposed spec clarification in
http://lists.w3.org/Archives/Public/www-style/2013Feb/0229.html which
makes us compatible with WebKit on the insertRule tests in this patch.

I confirmed that the test reports 7 failures without the patch, but
passes with the patch.  (I'm a little disturbed by the way our
testharness.js integration elides runs of successive passes.)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 712083 [details] [diff] [review]
Make CSS insertRule methods throw SYNTAX_ERR when given an empty rule or more than one rule.

Reporting PERuleTrailing when !*aResult is a bit odd.  Shouldn't we only do that when GetToken() returns true?

Given that we now check for whitespace-only things in the CSS parser and throw,
do we still need the explicit IsEmpty() checks in nsCSSStyleSheet::InsertRuleInternal and nsCSSStyleSheet::InsertRuleIntoGroup?

r=me with those addresed.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5bb11a09bb20

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5bb11a09bb20

Comment 8

[Kohei Yoshino]

I've added this bug to the compatibility doc. Please correct the info if I'm wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_21

Comment 9

[Jean-Yves Perrier [:teoli]]

I also added a note in:
https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleSheet/insertRule
and
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/21