Last Comment Bug 765599 - CSSStyleSheet.insertRule should throw when there are more than one rule
: CSSStyleSheet.insertRule should throw when there are more than one rule
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla21
Assigned To: David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
:
Mentors:
https://www.w3.org/Bugs/Public/show_b...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-17 11:32 PDT by Kang-Hao (Kenny) Lu [:kennyluck]
Modified: 2013-08-11 00:41 PDT (History)
7 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Make CSS insertRule methods throw SYNTAX_ERR when given an empty rule or more than one rule. (29.92 KB, patch)
2013-02-08 22:45 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
Details | Diff | Review

Description Kang-Hao (Kenny) Lu [:kennyluck] 2012-06-17 11:32:58 PDT
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 2012-08-11 12:34:55 PDT
can I be assigned this bug???
Comment 2 Kang-Hao (Kenny) Lu [:kennyluck] 2012-08-12 01:25:33 PDT
(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-7 (review requests must explain patch) 2013-02-08 21:51:28 PST
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-7 (review requests must explain patch) 2013-02-08 22:45:31 PST
Created attachment 712083 [details] [diff] [review]
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 [:bz] (Out June 25-July 6) 2013-02-09 18:12:12 PST
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-7 (review requests must explain patch) 2013-02-09 22:57:52 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bb11a09bb20
Comment 7 Ryan VanderMeulen [:RyanVM] 2013-02-10 10:22:15 PST
https://hg.mozilla.org/mozilla-central/rev/5bb11a09bb20
Comment 8 Kohei Yoshino [:kohei] 2013-02-23 23:31:09 PST
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

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