Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 2440 #2520

Merged
merged 2 commits into from
Mar 26, 2015
Merged

Fix 2440 #2520

merged 2 commits into from
Mar 26, 2015

Conversation

lukeapage
Copy link
Member

Don't do weird pulling apart of rules and rulesets, just bubble imports and charsets.

I think the code was left over from when we didn't bubble rulesets out of the nesting before calling toCSS

@seven-phases-max would you review? It looks and feels safe to me. Your example output

.a {
  @charset "utf-8";
  @import "test.csS";
}

is unchanged before and after

@lukeapage
Copy link
Member Author

okay I'll just merge this then

lukeapage added a commit that referenced this pull request Mar 26, 2015
@lukeapage lukeapage merged commit e411f54 into master Mar 26, 2015
@seven-phases-max
Copy link
Member

Oh, sorry, I simply missed you asked me to review (probably it came surrounded with a couple of other new issues/PRs and I simply skipped a few incl. this one, just aways trusting your PRs as being safe unless the stuff they fix/change is too weird itself).

Now by looking at the code I realize we missed one thing in #2440. According to http://www.w3.org/TR/css3-syntax/#charset-rule @charset probably must go before comments i.e. charset > comments (if any) > imports. Though I can't find if they explicitly state that charset detection stage takes place before comments elimination but I guess it does (obviously comments have to be parsed according to the same encoding option).

@lukeapage
Copy link
Member Author

Yes, the code should do that - it inserts at charsetNodeIndex which is only incremented by other charset nodes...

@seven-phases-max seven-phases-max deleted the bugfix/2440 branch May 31, 2017 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants