Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dbaron, Assigned: kevin.m.wern, Mentored)

Tracking

({dev-doc-complete})

Trunk
mozilla40
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(3 attachments, 8 obsolete attachments)

Per https://www.w3.org/Bugs/Public/show_bug.cgi?id=27422 we should be able to remove CSSCharsetRule.  Chrome has removed it.
Mentor: dbaron
Whiteboard: [lang=c++]
(Assignee)

Comment 1

4 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
Flags: needinfo?(dbaron)
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.)
Assignee: nobody → kevin.m.wern
Flags: needinfo?(dbaron)
Correct, there was no observable change to the parsing, there's just no CSSCharsetRule created and exposed.

Comment 4

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

Comment 5

4 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.
(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.
(Assignee)

Comment 7

4 years ago
Posted patch patch-parsecharsetrule.patch (obsolete) — Splinter Review
Patch for removing the generation of a rule object in ParseCharsetRule
(Assignee)

Comment 8

4 years ago
Posted patch patch-removecharsetrule.patch (obsolete) — Splinter Review
Patch that removes code concerning the CharsetRule object
(Assignee)

Comment 9

4 years ago
Posted patch patch-fixtests.patch (obsolete) — Splinter Review
Remove test cases involving csscharsetrule objects
(Assignee)

Comment 10

4 years ago
Posted patch patch-fixbuildbug.patch (obsolete) — Splinter Review
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.
(Assignee)

Comment 11

4 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.
Attachment #8590697 - Flags: review?(dbaron)
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).
Attachment #8590704 - Attachment is obsolete: true
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
Attachment #8590697 - Flags: review?(dbaron) → review+
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
Attachment #8590700 - Flags: review+
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.
Flags: needinfo?(kevin.m.wern)
(Assignee)

Comment 16

4 years ago
In the final version, I will most likely combine this with patch-fixtests when I'm done looking at rule insertion
Flags: needinfo?(kevin.m.wern)
(Assignee)

Comment 17

4 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.

Comment 18

4 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
(Assignee)

Comment 19

4 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.
Attachment #8590697 - Attachment is obsolete: true
(Assignee)

Comment 20

4 years ago
Nothing changed other than the commit message
Attachment #8590700 - Attachment is obsolete: true
(Assignee)

Comment 21

4 years ago
Posted patch Merge of two test patches (obsolete) — Splinter Review
Attachment #8590703 - Attachment is obsolete: true
Attachment #8594586 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8595220 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8595216 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8595214 - Flags: review?(dbaron)
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
Attachment #8595214 - Flags: review?(dbaron) → review+
Comment on attachment 8595216 [details] [diff] [review]
Remove charset rule with correct commit message

Same thing about the email address, but otherwise fine.
Attachment #8595216 - Flags: review?(dbaron) → review+
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?
Flags: needinfo?(kevin.m.wern)
Comment on attachment 8595220 [details] [diff] [review]
Merge of two test patches

Asking khuey to rubber-stamp the test_interfaces.html change.
Attachment #8595220 - Flags: review?(khuey)
(Assignee)

Comment 26

4 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.
Flags: needinfo?(kevin.m.wern)

Comment 27

4 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.
(Assignee)

Comment 28

4 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.
(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?
Flags: needinfo?(kevin.m.wern)
Attachment #8595220 - Flags: review?(dbaron) → review+
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)
(Assignee)

Comment 31

4 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.
Flags: needinfo?(kevin.m.wern)
(Assignee)

Comment 32

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

Comment 33

4 years ago
Fix email
Attachment #8595216 - Attachment is obsolete: true
(Assignee)

Comment 34

4 years ago
Changed email
(Assignee)

Updated

4 years ago
Attachment #8595220 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8595214 - Attachment is obsolete: true
(And I added the r=khuey to the third patch as well.)
And since I forgot to say so sooner:  thanks for fixing this!
You need to log in before you can comment on or make changes to this bug.