drupal_load_stylesheet_content() removes whitespace improperly, creating invalid CSS

according to the CSS specs:

5.2.1 Grouping
When several selectors share the same declarations, they may be grouped into a comma-separated list.

So separating the selectors by just a newline is invalid CSS IMO, so I don't think we should fix this...

I apologize, the indentation used in the example made me think of multiple selectors. You're right, this does look like an error.

Very simple patch, explanation is good, marking rtbc.

Hm. I'd feel a lot more comfortable committing this if we had tests to prove that we don't break something else in the meantime.

We should be able to use a small core CSS file for testing - no need to duplicate the whole of Garland's here. Not sure what you mean by testing the old broken code as valid - ideally the test should fail without your patch, and pass with it.

From http://www.w3.org/TR/CSS2/syndata.html#whitespace

The token S in the grammar above stands for white space. Only the characters "space" (U+0020), "tab" (U+0009), "line feed" (U+000A), "carriage return" (U+000D), and "form feed" (U+000C) can occur in white space. Other space-like characters, such as "em-space" (U+2003) and "ideographic space" (U+3000), are never part of white space.

The patch above is unlikely to handle "form feed" or 0x0c properly.

Submitting for re-testing.

Pushing to critical as this can break some CSS files.

Thank you for noticing this. I like what you're doing here but the optimized CSS files now have a bunch of line breaks that they don't need to have. Before patch:

.field .field-label,.field .field-label-inline,.field .field-label-inline-first{font-weight:bold;}.node-unpublished{background-color:#fff4f4;}.preview .node{background-color:#ffffea;}

After patch:

.field .field-label,.field .field-label-inline,.field .field-label-inline-first{font-weight:bold;}

.node-unpublished{background-color:#fff4f4;}.preview .node{background-color:#ffffea;}

As you can see, we have two line breaks here that we don't need to have. Is there anyway we could remove the line breaks, but add an extra space instead for the use case you provided so that the CSS still stays valid?

That sounds reasonable. This is aggregation, not compression. It seems like if we want to compress it further, we'd want a pluggable framework for the CSS. Setting it to RTBC!

My nitpicking side got the better of me.

Wheee!

Wow, sorry. It's apparently taken me a very long time to get back to this issue! :(

I've reviewed this and mostly found minor formatting stuff, and a couple of things that look like bugs.

@rfay, could you explain this?

The reality is that linefeeds are valid whitespace in CSS, so removing them breaks the spec. In an earlier version of the patch, I replaced them with spaces, but decided later that that added risk to the patch.

We should maybe have a reference off to said spec in the PHPDoc comments of drupal_load_stylesheet_content() because atm output like:

...color:#494949;}.this
+.is
+.a
+.test{font:1em/100%...

...looks like a bug in our optimizer.

Here's the review. Note that since this doesn't change APIs, I probably won't get back to it again before code freeze. But it's marked as a critical bug so it will get fixed before D7 ships.

---

+++ includes/common.inc	17 Jul 2009 05:43:44 -0000
@@ -2654,8 +2654,7 @@
       \s*([@{}:;,]|\)\s|\s\()\s* |  # Remove whitespace around separators, but keep space around parentheses.
-      /\*([^*\\\\]|\*(?!/))+\*/ |   # Remove comments that are not CSS hacks.
-      [\n\r]                        # Remove line breaks.
+      /\*([^*\\\\]|\*(?!/))+\*/    # Remove comments that are not CSS hacks.

Looks like that # comment needs to be indented to be inline with the others.

+++ modules/simpletest/tests/common.test	17 Jul 2009 05:43:45 -0000
@@ -282,6 +282,47 @@
+ * CSS Unit Tests

Comments need to end in a period.

+++ modules/simpletest/tests/common.test	17 Jul 2009 05:43:45 -0000
@@ -282,6 +282,47 @@
+      'name' => 'CSS Unit Testss',

"Tests" ;)

+++ modules/simpletest/tests/common.test	17 Jul 2009 05:43:45 -0000
@@ -282,6 +282,47 @@
+    /**
+   * Tests basic CSS loading with and without optimization (drupal_load_stylesheet())
+   */

Indentation is off here. Must end with a period as well.

+++ modules/simpletest/tests/common.test	17 Jul 2009 05:43:45 -0000
@@ -282,6 +282,47 @@
+  // Array of files to test. Original = .css,
+  // unoptimized expected output = .css.unoptimized.css,
+  //   optimized expected = .css.optimized.css
+  // They live in the simpletest/files/css_test_files directory

Indentation/spacing is off here. These comments should be indented to the same width as the body of the function.

That said, these are probably slightly too detailed comments (it's spending a lot of time explaining stuff that's easily deduceable by reading the code). I'd go with something like:

// Test with two sample CSS files; one using the @import tag, and one without.

+++ modules/simpletest/tests/common.test	17 Jul 2009 05:43:45 -0000
@@ -282,6 +282,47 @@
+      $unoptimized_output = drupal_load_stylesheet_content($content, FALSE);
+      
+      $expected = file_get_contents("$path/$file.unoptimized.css");

Trailing whitespace in the middle there.

+++ modules/simpletest/files/css_test_files/css_input_with_import.css	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,34 @@
+/**
+  * @file Basic css that uses @import
+*/

Indentation off here.

+++ modules/simpletest/files/css_test_files/css_input_with_import.css	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,34 @@
+body {
+  margin: 0;
+  padding: 0;
+  background: #edf5fa;
+  font: 76%/170% Verdana, sans-serif;
+  color: #494949;
+}
...
+textarea, select {
+  font: 1em/160% Verdana, sans-serif;
+  color: #494949;
+}

Is this test data relevant at all? If not, let's remove it. Keep the test data as simple as possible.

+++ modules/simpletest/files/css_test_files/css_input_without_import.css.unoptimized.css	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,6 @@
+/* $Id: style.css,v 1.57 2009/05/31 00:51:41 webchick Exp $ */
+
+/**
+  * @file Basic css that does not use 
+}
+

Is this file intentionally all messed up? I can't see how that could possibly be valid CSS.

If it is intentional, please add more comments here so someone doesn't try and "fix" it later.

+++ modules/simpletest/files/css_test_files/css_input_without_import.css	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,33 @@
+/**
+  * @file Basic css that does not use @import
+*/

Indentation.

+++ modules/simpletest/files/css_test_files/css_input_with_import.css.unoptimized.css	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,34 @@
+/**
+  * @file Basic css that uses @import
+*/
+
+
+
+

Indentation.

Also, what's up with all of those blank lines? Part of the test case? If so, maybe document that in the @file comment? I could see someone "fixing" this later.

23 days to code freeze. Better review yourself.

Found a couple other small indentation problems. Other then that it looks really good. Here's the updated patch.

This looks great. I keep wanting to remove the extra blank linkes in test.css - but it's a test, so it ought to be realistic.

Great, thanks for the fixes. Let's maybe tweak our whitespace algorithm in the future so that it strips those because I agree it does look very odd.

Committed to HEAD. Marking needs porting to 6.x, although it's possible one of the other patches has it; I kinda lost track.

Doing what I said I was doing. :P

The test bot doesn't like this now it's committed - 1 fail being reported from 'CSS unit tests' for every new test result.

I can reproduce locally.

The first lines of the files do not match:

< /* $Id: css_input_without_import.css,v 1.1 2009/10/05 02:48:38 webchick Exp $ */
---
> /* $Id: css_input_without_import.css.unoptimized.css,v 1.1 2009/10/05 02:48:38 webchick Exp $ */

Something like this will get the test passing.

That makes sense, looks a bit odd but I can't think of anything better either, and it's an odd bug in the first place.

Almost. The file name was changed in one line but not the other.

However, that test needs an overhaul anyway. So take this.

Wow, that's totally bizarre. I have no idea why this wasn't failing tests while it was in the queue. Sorry, guys. :(

Anyway, committed to HEAD.

As an added bonus, Sun switched it to a DrupalUnitTestCase, which will improve testing performance! YAY!

Backport to D6

Still yup.

Committed to Drupal 6 too, thanks.

I'm not sure if I supposed to reopen this ticket or open a new one or just leave it but:

• We could remove the complately blank lines (/^$/ or /(^[\r\n]*|[\r\n]+)[\s\t]*[\r\n]+/)
• We could remove the trailing and beginning spaces and tabs (trim()?)

I'll show you an example:

Before:

[empty lines]
[empty lines]
.node-unpublished{background-color:#fff4f4;}.preview .node{background-color:#ffffea;}#node-admin-filter ul{list-style-type:none;padding:0;margin:0;width:100%;}#node-admin-buttons{float:left;
  margin-left:0.5em;
  clear:right;}td.revision-current{background:#ffc;}.node-form .form-text{display:block;width:95%;}.node-form .container-inline .form-text{display:inline;width:auto;}.node-form .standard{clear:both;}.node-form textarea{display:block;width:95%;}.node-form .attachments fieldset{float:none;display:block;}.terms-inline{display:inline;}

After:

.node-unpublished{background-color:#fff4f4;}.preview .node{background-color:#ffffea;}#node-admin-filter ul{list-style-type:none;padding:0;margin:0;width:100%;}#node-admin-buttons{float:left;
margin-left:0.5em;
clear:right;}td.revision-current{background:#ffc;}.node-form .form-text{display:block;width:95%;}.node-form .container-inline .form-text{display:inline;width:auto;}.node-form .standard{clear:both;}.node-form textarea{display:block;width:95%;}.node-form .attachments fieldset{float:none;display:block;}.terms-inline{display:inline;}

Hi, I'm not sure if re-opening this bug is appropriate...

I have encountered another similar problem with 6.16. For my custom theme, the following CSS:
#foo :visited { }
gets turned into
#foo:visited { }
which is obviously not the same... Quick workaround for me is to use this instead:
#foo *:visited { }

By the way, I would actually prefer to get all CSS aggregated into one file, but *without* munging it at the same time. That's not possible, the current setting is "all or nothing". :-/

Updates happen in #444228: Optimize CSS option causes php cgi to segfault in pcre function "match"
Please do not reopen this issue