This patch removes all @charset declarations from CSS files and make sure the aggregated CSS file is standard compliant. The standard compliance is critical for themes, while Safari will not read styles after a @charset declaration if this declaration is not the first line in the CSS file. Additional we cannot simply remove the charset from the Themes CSS files, while this breaks all multilingual inline documentation.

All sites using the following themes and Drupal's aggregate and compress feature will be broken in Safari and other webkit browsers without the patch. Therefor this patch is required by the following Themes:

  • cod_organizing
  • deco
  • hiroshigeblue
  • sands
  • sands_css
  • slash
  • YAML
  • Zen
  • zental
  • zenzen

This is a D6 patch against current HEAD for the bug already reported in http://drupal.org/node/150759 for D5.

CommentFileSizeAuthor
common_css_compress.patch923 byteshass

Comments

kkaefer’s picture

kkaefer commented

If we want to remove all charset declaration, why don’t we just use @charset[^;]+; instead of this more advanced regex?

The problem here is that we can’t just move all @charset directives to the top of the file (one file can only have one encoding). Do we want to go that far and actually make Drupal interpret these directives and perform character set conversions if the file is not UTF-8? I think not. Most people don’t use UTF-8 characters in their CSS (it’s even illegal for some HTML attributes). If they use non-ascii chars in their comments, it’s fine because they are stripped anyway. Therefore, I don’t see a problem with this patch. We should document somewhere that the recommended encoding for all Drupal source code files is UTF-8.

hass’s picture

hass commented

The first time i build this patch i used your regex too... But i was made to spend more time on the regex and implemented the regex based on the the W3C docs definition. Therefore - this one is the standard compliant regex - based on http://www.w3.org/TR/CSS21/syndata.html rules. Please read http://drupal.org/node/150759 about the discussion and some bugs in earlier patches that should be fixed now.

hass’s picture

hass commented

And about the rest, we need to keep the compressed files to be standard compliant or do you thing standard compliance is not important?

If someone write a UTF8 encoding declaration, what is done often (see above projects list) the site gets broken in Safari. It is not wrong to use charset declaration with UTF8 and ecplise additional make us to insert this string on top, or eclipse cannot handle UFT8 css files correctly and falls back to ISO. Ok - this may be a bug in the editor, but eclipse haven't fixed this since 27 September 2001 (!!!).

You are German and may read http://www.yaml-fuer-drupal.de/de/einfuehrung/entwicklungsplatform about this big big sh**.

kkaefer’s picture

kkaefer commented

I never said that we shouldn’t remove these @charset declarations. However, if Eclipse has problems with UTF-8 files, it’s none of Drupal’s business.

hass’s picture

hass commented
Status: Needs review » Reviewed & tested by the community

after this bugfix been committed into D5 (http://drupal.org/node/150759), i think this is RTBC for D6, too.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs review

Please do not RTBC your own patches. Get someone to test. The idea seems to be good, the patch needs testing.

hass’s picture

hass commented

Ok, but the patch has been tested for D5... there is no change in functionality.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Fixed

Man, why do you open new issues for the same stuff once already done for 5.x? Drumm left http://drupal.org/node/150759 open for you and set it for 6.x to let you attach a ported patch there. Why make it harder to review the progress of how things came to be? Doh. You loose so much potential. This issue clearly shows that people are not heavily interested in this bug, so the result does not seem tested and polished. It would have been in much sooner if the reviews presented in the 5.x issue were behind it.

Anyway, I just read the discussion on the 5.x issue, so committed this patch.

hass’s picture

hass commented

Sorry, i often feel like (and some of my very old cases are self speaking) that often people working on HEAD are not looking back in older cases... they are staying there 1 year or so and nothing happens... so i created this case and tried to bring it in the dev D6 radar... THX

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)