David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Description • 9 years ago

Per https://www.w3.org/Bugs/Public/show_bug.cgi?id=27422 we should be able to remove CSSCharsetRule.  Chrome has removed it.

	Kevin Wern Assignee
	Comment 1 • 9 years ago

Hey, I'm kind of new to this whole system.  I want to work on this--is there a way to assign this bug to myself, or am I supposed to comment here and have someone change it?

-Kevin

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 2 • 9 years ago

For now, I can change it, since you don't yet have the privileges described in https://developer.mozilla.org/en-US/docs/What_to_do_and_what_not_to_do_in_Bugzilla .

As for what to do here:  the first thing to do is probably to double-check what Chrome has done, which I *believe* is continuing to parse and support @charset rules, but just creating no representation in the object model for them.

You'll then probably want to write a sequence of at least two, and maybe more, separate patches to fix this bug.

The first would be to change the code that handles @charset rules to set the @charset without actually creating a charset rule object.  You'd then want to adjust any existing mochitests to reflect that new behavior, and if there aren't any existing ones, write a mochitest for it.  You can find this code starting from CSSParserImpl::ParseCharsetRule in layout/style/nsCSSParser.cpp .

The second would be to remove the code related to CSSCharsetRule, which
https://mxr.mozilla.org/mozilla-central/search?string=CSSCharsetRule
should give you a good way to find.

(I'm assuming you already know how to pull and build the source code.)

	Philip Jägenstedt [:foolip]
	Comment 3 • 9 years ago

Correct, there was no observable change to the parsing, there's just no CSSCharsetRule created and exposed.

	Simon Pieters [:zcorpan]
	Comment 4 • 9 years ago

(Note that the CHARSET_RULE constant on CSSRule is not removed. This is consistent with the DOM spec.)

	Kevin Wern Assignee
	Comment 5 • 9 years ago

Hey,

Sorry for the delay, I've had a really busy week, on top of the fact that a lot of the time in this bug went into the learning curve of building, mochitest, and other stuff (especially reading/double checking the code is being removed properly).

Anyway, I finished all the functional patching, my only question really concerns testing.  A few of the tests depend on CharsetRule being a part of the object model, which I've removed right now but am planning to reincorporate with the new functionality.  An example is in layout/style/tests/test_css_eof_handling.html, which uses the cssRules array and requires it have at least one member.

The behavior I'd expect for a replacement test would be to just use the determined charset of the test document (with the proper environment for the browser to choose to use the charset rule) as an indicator of whether or not the rule was parsed properly.  Should I put these replacements in a new file for this bug because the behavior is so different?  And then are the tests in layout/reftests/css-charset good enough for testing the basic functionality of @charset, or should I be writing more?  I just want to make sure I'm being thorough with this.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 6 • 9 years ago

(In reply to kevin.m.wern from comment #5)
> reincorporate with the new functionality.  An example is in
> layout/style/tests/test_css_eof_handling.html, which uses the cssRules array
> and requires it have at least one member.

For that particular example, I think it's fine to just remove the two tests in question, since the behavior is now untestable from a Web standards perspective.

	Kevin Wern Assignee
	Comment 7 • 9 years ago

Patch for removing the generation of a rule object in ParseCharsetRule

	Kevin Wern Assignee
	Comment 8 • 9 years ago

Patch that removes code concerning the CharsetRule object

	Kevin Wern Assignee
	Comment 9 • 9 years ago

Remove test cases involving csscharsetrule objects

	Kevin Wern Assignee
	Comment 10 • 9 years ago

When I was first trying to build the code, I had to remove 'override' to get it to succeed.  Not relevant to this bug but I figured it'd be easy to look into if it needs to be fixed.

	Kevin Wern Assignee
	Comment 11 • 9 years ago

The last patch is not relevant to this bug, but it is something I found when I was first trying to build the code.  I figured I should at least mention it rather than omit the patch.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 12 • 9 years ago

Comment on attachment 8590704 [details] [diff] [review]
patch-fixbuildbug.patch

Not sure why you needed this, but it hasn't been needed by anybody else, it's not relevant to this bug, and it looks like it ought to work fine (since ThenValue inherits from ThenValueBase which inherits from Consumer, which has a method with an identical signature).

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 13 • 9 years ago

Comment on attachment 8590697 [details] [diff] [review]
patch-parsecharsetrule.patch

It would probably be good to add a comment that there is no rule object created for an @charset rule.  Otherwise the code might be confusing.

In addition to that, you probably want a commit message that says something like:

Bug 1148694 patch 1 - Stop creating a CharsetRule object when parsing @charset rules.  r=dbaron


r=dbaron with that

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 14 • 9 years ago

Comment on attachment 8590700 [details] [diff] [review]
patch-removecharsetrule.patch

I'd suggest a commit message like:

Bug 1148694 patch 2 - Remove interface and implementation of CSSCharsetRule.  r=dbaron


r=dbaron with that

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 15 • 9 years ago

Comment on attachment 8590703 [details] [diff] [review]
patch-fixtests.patch

The change to test_css_eof_handling.html looks fine.

I'm a little more worried about the change for test_rule_insertion.html.  How did the test fail with the patch?

Also, it's not clear to me what the rule insertion methods should do now.  I think the change in the first patch will cause them to throw exceptions, since CSSParserImpl::ParseRule throws NS_ERROR_DOM_SYNTAX_ERR if no rule is produced.  Is that what's supposed to happen?  What do other browsers that have implemented this change do?

Finally, I think you will also need to change dom/tests/mochitest/general/test_interfaces.html.  (The second patch should have caused it to fail.  Did it not?)  (I can find a DOM peer to review that; :khuey is probably willing.)


Sorry for not getting to this sooner.

	Kevin Wern Assignee
	Comment 16 • 9 years ago

In the final version, I will most likely combine this with patch-fixtests when I'm done looking at rule insertion

	Kevin Wern Assignee
	Comment 17 • 9 years ago

Based on chromium/blink, it seems as if they are only parsing @charset as part of a sheet.  Other than that, they are setting up their parser to start from @-internal-rule in the hierarchy when ParseRule is called.  They use a similar function to our Loader's to actually determine the charset.

https://chromium.googlesource.com/chromium/blink/+/master/Source/core/html/parser/TextResourceDecoder.cpp

And the parser changes here:

http://src.chromium.org/viewvc/blink/trunk/Source/core/css/parser/CSSGrammar.y?r1=186292&r2=186291&pathrev=186292

https://src.chromium.org/viewvc/blink/trunk/Source/core/css/parser/BisonCSSParser-in.cpp?revision=185966&pathrev=186292 (not a change, but supports what I was mentioning)

So I can definitely suppress the NS_ERROR_DOM_SYNTAX_ERR with a conditional, but as for the question of what insertRule should do, I have no idea.  It seems basically unsupported for @charset, which makes sense I guess for a rule with strict positional requirements that actually has no position at all.

Should I be acting as if it is unsupported?  If so, should I remove CHARSET_RULE from conditionals?  I just want to see if I'm on the right track before moving forward.

	Simon Pieters [:zcorpan]
	Comment 18 • 9 years ago

(In reply to kevin.m.wern from comment #17)
> but as for the question of what insertRule should do, I have no idea.

insertRule('@charset "utf-8";', 0) should throw SyntaxError per spec. Blink does this also AFAICT.

http://dev.w3.org/csswg/cssom/#insert-a-css-rule step 1 invokes "parse a CSS rule"

http://dev.w3.org/csswg/cssom/#parse-a-css-rule step 3 does a grammar check.

http://dev.w3.org/csswg/css-syntax-3/#charset-rule - there is no grammar for @charset, so it is an invalid rule.

http://dev.w3.org/csswg/cssom/#parse-a-css-rule step 3 returns syntax error.

http://dev.w3.org/csswg/cssom/#insert-a-css-rule step 2 throws SyntaxError.

> If so, should I remove
> CHARSET_RULE from conditionals?

I'm not certain what this refers to be see comment 4

	Kevin Wern Assignee
	Comment 19 • 9 years ago

I think blink actually parses the first charset rule based on Grammar.y, but based on the documentation Simon showed me it seems that all the charset instances should return a syntax error.  So I did not modify the surrounding behavior of nsCSSParser.

	Kevin Wern Assignee
	Comment 20 • 9 years ago

Nothing changed other than the commit message

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 22 • 9 years ago

Comment on attachment 8595214 [details] [diff] [review]
Parse charset changes with comments

>From 37f7c51cae2390663cbab0107dbf58a883da73c0 Mon Sep 17 00:00:00 2001
>From: Kevin Wern <kwern@Kevins-MacBook-Pro.local>

Oops, just noticed this.  You should put a real email address here!


>Subject: [PATCH] Bug 1148694 patch 1 - Stop creating a CharsetRule object
> when parsing @charset rules. r=dbaron

Hopefully this will get wrapped back to a single line when somebody imports the patch in order to land it.

>+  // CharsetRule object no longer created from @charset - Bug 1148694

It's better for comments to describe the state of the code rather than a change that was previously made.  The comment should say something like:

 // It's intentional that we don't create a rule object for @charset rules.


r=dbaron with that

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 23 • 9 years ago

Comment on attachment 8595216 [details] [diff] [review]
Remove charset rule with correct commit message

Same thing about the email address, but otherwise fine.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 24 • 9 years ago

I think everything looks good except for this question from comment 15:

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #15)
> I'm a little more worried about the change for test_rule_insertion.html. 
> How did the test fail with the patch?

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 25 • 9 years ago

Comment on attachment 8595220 [details] [diff] [review]
Merge of two test patches

Asking khuey to rubber-stamp the test_interfaces.html change.

	Kevin Wern Assignee
	Comment 26 • 9 years ago

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #24)
> I think everything looks good except for this question from comment 15:
> 
> (In reply to David Baron [:dbaron] ⏰UTC-7 from comment #15)
> > I'm a little more worried about the change for test_rule_insertion.html. 
> > How did the test fail with the patch?

The following test failures were outputted to the terminal:
671 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_rule_insertion.html | <0,0,0,0,4> inserting @charset 'UTF-8'; into style sheet - expected PASS
672 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_rule_insertion.html | <0,0,1,0,4> inserting @charset 'UTF-8'; into style sheet - expected PASS
673 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_rule_insertion.html | <0,0,2,0,4> inserting @charset 'UTF-8'; into style sheet - expected PASS
674 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_rule_insertion.html | <1,0,0,0,4> inserting @charset 'UTF-8'; into style sheet - expected PASS
675 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_rule_insertion.html | <1,0,1,0,4> inserting @charset 'UTF-8'; into style sheet - expected PASS
676 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_rule_insertion.html | <1,0,2,0,4> inserting @charset 'UTF-8'; into style sheet - expected PASS
677 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_rule_insertion.html | <2,0,0,0,4> inserting @charset 'UTF-8'; into style sheet - expected PASS
678 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_rule_insertion.html | <2,0,1,0,4> inserting @charset 'UTF-8'; into style sheet - expected PASS
679 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_rule_insertion.html | <2,0,2,0,4> inserting @charset 'UTF-8'; into style sheet - expected PASS
SUITE-END | took 23s

The GUI reflects this.  For the test, @charset is inserted with every outer rule combination. The exception results from no rule object being created in ParseRule, which inherently returns a syntax error, and a failure result from InsertRule.  This coincides with the CSS specs Simon posted.

	Simon Pieters [:zcorpan]
	Comment 27 • 9 years ago

(In reply to kevin.m.wern from comment #19)
> I think blink actually parses the first charset rule based on Grammar.y, but
> based on the documentation Simon showed me it seems that all the charset
> instances should return a syntax error.  So I did not modify the surrounding
> behavior of nsCSSParser.

When parsing a full stylesheet, a leading @charset rule is not a syntax error; see step 3 of http://dev.w3.org/csswg/css-syntax/#parse-stylesheet

The only difference is if you log syntax errors to devtools I suppose.

	Kevin Wern Assignee
	Comment 28 • 9 years ago

Ok, that makes more sense--I didn't really look around on that document other than around where you linked me.  So I guess the @charset should be removed entirely from ParseAtRule and handled elsewhere.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 29 • 9 years ago

(In reply to kevin.m.wern from comment #28)
> Ok, that makes more sense--I didn't really look around on that document
> other than around where you linked me.  So I guess the @charset should be
> removed entirely from ParseAtRule and handled elsewhere.

It already is handled elsewhere, in that the actual function of the @charset rule is handled in SheetLoadData::OnDetermineCharset in layout/style/Loader.cpp.  But that's at a level prior to parsing.

Does your patch cause a style sheet with a valid @charset rule to report an error to the error console?

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 30 • 9 years ago

If you upload revised patches reflecting the comments, I can push them to try to check for any other test failures.

(comment 29 also needs answering)

	Kevin Wern Assignee
	Comment 31 • 9 years ago

> (comment 29 also needs answering)

Just checked.  There were no errors thrown.  I checked using console logging in devtools, in case there is somewhere I missed.

	Kevin Wern Assignee
	Comment 32 • 9 years ago

Email is now a real email, newline removed from commit message (idk how that got there), and comment revised.

	Kevin Wern Assignee
	Comment 33 • 9 years ago

Fix email

	Kevin Wern Assignee
	Comment 34 • 9 years ago

Changed email

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 35 • 9 years ago

Here's a try run to test those:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=391facb54086

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 36 • 9 years ago

(And I added the r=khuey to the third patch as well.)

	Pulsebot
	Comment 37 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/74509f3957cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e76f9a26430
https://hg.mozilla.org/integration/mozilla-inbound/rev/a00db37abe24

	Carsten Book [:Tomcat]
	Comment 38 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/74509f3957cf
https://hg.mozilla.org/mozilla-central/rev/8e76f9a26430
https://hg.mozilla.org/mozilla-central/rev/a00db37abe24

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 39 • 9 years ago

And since I forgot to say so sooner:  thanks for fixing this!

	Jean-Yves Perrier [:teoli]
	Comment 40 • 9 years ago

Doc updated:
https://developer.mozilla.org/en-US/docs/Web/API/CSSRule
and
https://developer.mozilla.org/en-US/Firefox/Releases/40#CSSOM