documentcloud / jammit

Industrial Strength Asset Packaging for Rails
http://documentcloud.github.com/jammit/
MIT License
1.16k stars 197 forks source link

Packaging multiple CSS file that have @charset directives generates invalid CSS. #120

Open scottwb opened 13 years ago

scottwb commented 13 years ago

Given file1.css:

@charset "UTF-8";
h1 {
  font-size: 24px;
}

And file2.css:

@charset "UTF-8";
h2 {
  font-size: 18px;
}

and an assets.yml that combines them, you end up with a single CSS file that looks like:

@charset "UTF-8";
h1 {
  font-size: 24px;
}
@charset "UTF-8";
h2 {
  font-size: 18px;
}

According to the W3, this is invalid. (See http://www.w3.org/International/questions/qa-css-charset ). Although most browsers deal with this quietly, Safari completely breaks down.

I propose that jammit should remove duplicate @charset directives and, if necessary, convert the encoding of input files so that they match the output encoding.

(For background on how and why I discovered this, see: http://sleeplesscoding.blogspot.com/2011/01/jammit-compass-and-safari-cant-we-all.html )

jashkenas commented 13 years ago

Want to contribute a patch for it?

scottwb commented 13 years ago

Was thinking about it :)

Unfortunately I am under the gun for a deadline right now and already have a quick hack workaround using sed to remove the duplicate directives, so it probably won't be something I can get to soon.

But...I take it you agree that this sounds like the right thing to do. If so, then perhaps I will take a stab at a patch after my immediate deadlines pass.

scullum commented 13 years ago

Was this issue closed because it was fixed in a release or because it isn't going to be fixed?

jashkenas commented 13 years ago

Because it looks like the patch isn't going to be contributed. If you want to take a stab at it -- feel free.

jaredmoody commented 13 years ago

Can this issue be re-opened? Just because the reporter isn't going to write a patch doesn't mean it's not an issue that should be addressed.

jashkenas commented 13 years ago

I'm not sure -- @charset seems like a really bad idea to add to your CSS: Firstly, you should be using an HTTP header to specify the character set, not a magic comment. Secondly, even the W3 page that describes it says:

Actually the charset attribute declaration is not supported by all browsers anyway, so that is another reason to avoid it.

And ...

Use an HTTP Content-Type header on the linked resource instead.

But, sure, reopened.

scottwb commented 13 years ago

FWIW I tend to agree that an HTTP header is preferable to a @charset directive.

My original problem stemmed from the fact that Sass/Compass added @charset directives automatically to each generated CSS file being combined. When I questioned @nex3 and @chriseppstein about it, they had some good rationale for it being the way it is.

I think the combination of Sass, Compass, and Jammit makes for an awesome set of tools, and it would be a shame to not be able to use them together. The lame workaround I wrote about in my blog post (linked in this issue's original posting) has been sufficient for my current needs, but I wouldn't feel good about trying to roll that method into a patch to go out with the main distro.

With the limited time I put into this, I wasn't able to come up with a reasonable solution for handling corner cases like multiple CSS files with conflicting @charset directives being combined into one. Perhaps one easy solution would be to detect that case and raise an error, rather than fretting about trying to do charset conversion.

jashkenas commented 13 years ago

I'm going to leave this one open as "patches welcome" for a while, and if no one steps up to take it ...

benatkin commented 13 years ago

I started working on a patch and accidentally had it running through YUI compressor rather than concatenating it.

This is done in two lines in YUI compressor.

I might finish this up if I can figure out the config settings that concatenate but don't compress with YUI or cssmin.js. I'm posting this comment so I'll be able to click on those links and use them as notes.

Also I'm curious if uglifier does anything with CSS. Since it requires JavaScript it might be nice to use cssmin.js in combination with it.