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

RESOLVED FIXED in mozilla21

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: kennyluck, Assigned: dbaron)

Tracking

({dev-doc-complete})

unspecified
mozilla21
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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

5 years ago
can I be assigned this bug???
(Reporter)

Comment 2

5 years ago
(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.
Assignee: nobody → mailto.rajesh05
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
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.)
Assignee: mailto.rajesh05 → dbaron
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [good first bug][mentor=kennyluck][lang=c++]
(Assignee)

Comment 4

4 years ago
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.)
Attachment #712083 - Flags: review?(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.
Attachment #712083 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 6

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bb11a09bb20
https://hg.mozilla.org/mozilla-central/rev/5bb11a09bb20
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
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
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
Keywords: dev-doc-complete
You need to log in before you can comment on or make changes to this bug.