CSS aggregation strips some essential whitespace within strings

subscribe.

From @gapple

... The only other area I can think of that may be a concern is within content: properties, but with whitespace collapsing in HTML is this an issue?

Yes. CSS generated content can be specified to have significant whitespace just like any PRE tag. For example...

.this_css_generated_content_has_significant_whitespace:before {
    white-space: pre;
    content: '\A\
#include  <stdio.h>\A\
int main(int argc, char **argv) {\A\
    int i;\A\
    for (i = 0; i < argc; i++) {\A\
        printf("Argument %d = \"%s\"\n", i, argv[i]);\A\
    }\A\
}';
}

Note that the "\A" (or \0000a) sequence at the end of each line is necessary to generate an actual linefeed within the content. The \ at the very end escapes the following linefeed within the CSS code (all linefeeds must be escaped within CSS strings).

Now that content: is supported in IE, it will likely be used more and more.

Here is a CSS compression case that is seriously broken, mangling content display.
The "content" string in a pseudo-attribute is wrongly modified:

li.node-readmore a:after {
  content: ": a word, and a phrase, etc.";
}

CSS compression erroneously removes spaces inside the string (around the ':' and ',' characters it seems).

By contrast, the following "content" string is left alone:

li.node-readmore a:after {
  content: " (a word - and a phrase)";
}

We should add @magnusk's case to the tests, as I think it's a slightly different case in terms of the processing code.

+++ modules/simpletest/files/css_test_files/css_input_without_import.css	8 Oct 2010 23:08:13 -0000
@@ -68,3 +68,16 @@
+ * 4.1.7 Rule sets, declaration blocks, and selectors ¶
+ * http://www.w3.org/TR/CSS2/syndata.html#rule-sets ¶

+++ modules/simpletest/files/css_test_files/css_input_without_import.css.unoptimized.css	8 Oct 2010 23:08:13 -0000
@@ -68,3 +68,16 @@
+ * 4.1.7 Rule sets, declaration blocks, and selectors ¶
+ * http://www.w3.org/TR/CSS2/syndata.html#rule-sets ¶

Trailing white-space.

Powered by Dreditor.

Tracking. The examples in #6 are very common. This also needs to be backported to 6.x when it is fixed.

I'm assuming this should be fixed in D8 & then brought back to D7.. Or closed if it is no longer an issue.

Confirming this is still an issue on Drupal 7.19; ran into it today with the following case:

img[style*="float: right;"] { ... }

This was working nicely to style images that CKEditor "aligns" to left or right via the inline style attribute. Since the markup has that space but the compressed CSS comes out to "float:right;", the rule doesn't get applied.

Marked #1996688: Aggregate and compress CSS files removes some essential whitespace as duplicate of this issue.

The same issue as #10 was reported in the above dup. issue.

tea.time, did you ever find a way to work around this issue?

This still needs to be fixed.

As stated by examples from #5 and #6, the issue is not just about CSS selectors, but about CSS strings instead.

#14: oh, you're right, apologies. It's just that "CSS strings" doesn't mean anything (or not at least AFAIK). Can't we say "CSS selectors and property values" then?

That's fine. W3C defines string as a value data type which can be used by either content property and attribute selectors values, also I think it's clear enough for PHP programmers to understand.

This bug is still causing problems on my site so I'm trying to find a fix myself. It seem like this is the bit of code causing the problem:

    // Regexp to match double quoted strings.
    $double_quot = '"[^"\\\\]*(?:\\\\.[^"\\\\]*)*"';
    // Regexp to match single quoted strings.
    $single_quot = "'[^'\\\\]*(?:\\\\.[^'\\\\]*)*'";

All it seems like this code is doing is stripping the spaces from single and double quoted strings in CSS files, right? This is where the code lives in the common file.

I'm tempted to just hack the core in my install to work around this issue in the meantime. I know this is a bad idea but I don't think a module can overwrite this functionality, can it?

EDIT: I guess it's more complex than just pulling these two things. Spaces in pseudo selectors are the thing getting stripped in my case and this will not fix that because colons are getting their trailing spaces stripped in this bit:

      # - Colon: Retain :pseudo-selectors.
      | ([\(:])\s+

I spent the day messing with this and here's my potential solution to the problem: http://ideone.com/YJOaQj

Basically I'm adding in a preg_replace_callback that uses some revised regex to ignore colons within quote marks. It looks like this:

'/"[^"]*"|([:])\s+/ims'

I got help with the regex from stackoverflow so there might be a more efficient way of doing it but this works for all of my test cases. If I knew I to make a drupal patch I'd put one together. Maybe someone can help with that part?

This patch fixes the problems I'm having with this issue on 7.x. Can anyone else test and verify.

Let's try this again with the status and version updated to reflect my patch.

Could you add test coverage for this? Without tests, this is almost certain to regress at some point!

Perhaps instead of using a bunch of regex to aggregate the CSS, the CSS should be parsed with an actual CSS parser and re-serialized to create the aggregated file. If you did that, it should be relatively easy to change:

div {color: red;}
div {color: yellow; color: blue;}

Into:
div {color: blue;}

That could be a big savings in a typical theme which does a big reset followed by redefining many of the reset properties.

This ought to be a more robust solution.

There already exists OSS code to do this parsing in web browsers and validators, and I think the W3C's CSS Validator must have a serializer to create the "Valid CSS information" at the bottom of their results page.

http://jigsaw.w3.org/css-validator/

Has this issue been abandoned?

I don't have the know-how or time to further look into why my patch failed testing. I know I've been using it on my production D7 server for a year now without any issue.

There are two fails: "Optimized CSS file has expected contents" and "Optimized CSS file (loaded from an URL) has expected contents". You need to look at the test file to see exactly what it is testing for. It is possible that the test does not test for exactly what it should be testing for, so it needs to be fixed.

Spent another day with this. Apparently the regex I'm using in my patch will look for a string enclosed by quotes:
/"[^"]*"
If it sees that, it stops stripping whitespace. If it does not, it continues stripping whitespace from after a colon:
([:])\s+

Something about the way the comment_hacks.css file looks after being optimized by this script is different enough that the test fails. Writing out the optimized files with and without the patch reveal this:

NEW:
comment-in-quotes-with-escaped:before{content:'/* \" \' */';}.this_rule_must_stay{color: #F00;background-color: #FFF;}.comment-in-quotes-with-escaped:after{content: "*/ \" \ '";}

OLD:
comment-in-quotes-with-escaped:before{content:'/* \" \' */';}.this_rule_must_stay{color:#F00;background-color:#FFF;}.comment-in-quotes-with-escaped:after{content:"*/ \" \ '";}

The difference is small but it is there. Basically my regex is seeing that first escaped quote, matching it with the second one and considering everything inside to be a quoted string. Since that matches, the regex stops stripping whitespace after the colon and the test fails.

I'll keep playing with the regex to see if I can figure out how to make it skip escaped quotes and then also write a test case for this. It will probably take me some time since this is all new to me but I am working on it.

Better regex and a test to make sure this fix stays in place.

I used this patch to resolve an issue similar to #4.
It works fine and after eyeballing the regex code I'm convinced that it works as intended.
Thanks for the patch:)

This looks like it still needs to go into Drupal 8.

I don't have a working Drupal 8 environment to write the patch against. All of my Drupal environments are running PHP 5.3 so I'm stuck on D7 for now. Once I get some time to do that update I'll then work on this patch.

Was drupal_load_stylesheet_content() removed in D8? I could only find a reference to it when grepping in a CSS comment in:
core//tests/Drupal/Tests/Core/Asset/css_test_files/comment_hacks.css

I guess that code got moved over to core//lib/Drupal/Core/Asset/CssOptimizer.php

I think I made the changes as appropriate, let's see what the bot says.

@johns996 - you may be able to test this on SimplyTest.me easily enough. Also there are great tools to help make it easier to test on localhost or even GetPantheon or Acquia. Lots of steps before upgrading your other Drupal environments.

I don't know how to address the error that is spit out by this function.

This shouldn't be equal if spaces have been removed properly:
$this->assertEquals($expected, $this->optimizer->optimize($css_asset), 'Group of file CSS assets optimized correctly.');

I just ran into this issue when using a pseudo rule injecting a coma that wants a space after it so my tags look nice. I figured out a workaround for my use case and thought I'd share it for any other dev wondering the wilderness. So if you have something like:

.field-name-field-tags .field-item:after {
  content: ", ";
}

The workaround until a real fix is in place is to use the unicode for space:

.field-name-field-tags .field-item:after {
  content: ",\0020";
}

I doubt this will solve selector issues (e.g. tea.time's issues) but should work anywhere a pseudo element that is needing to output comas and colons. E.g. magnusk example would output correctly if written as:

li.node-readmore a:after {
  content: ": \0020 a word,\0020 and a phrase,\0020 etc.";
}

Note you will have to be careful with escaping as a colon or many other characters directly before the "\0020" will alter the result.

Please fix and backport this to D7, this issue is an absolute killer (as OP already stated).

Took already an hour to figure the actual root cause.

Let me add that this issue must have been imported at some point, I work with "content: ', '" for quite some time now and also with CSS aggregation, this did not happen from the first place IMO.

Thanks to whoever fixes it, and to #37 for the pragmatic WA too. :)

My patch works just fine for D7. I've been using it in production for two years now without issue. I'm hoping to get some help at the next DrupalCon writing this for D8, backporting it for D7 and then writing the necessary tests. So if you can wait a few months I should have something by June.

Don't get me wrong; I know it now and the said WA is just ok. But I think this is a real anger-drawer, I don't want to figure how many guys out there invest hours on this. Aggregation is recommended in every second Drupal 101 blog post, so this might have some impact.

I found it hard to grab, even more since I first suspected some change accidentally related to just CSS pseudo content, and I consider myself at least a bit used to debug-pains.

So, I can wait alright, but not sure if the issue should, too ;)

+1 for patch #28 :-)
Thanks johns996.
For includes/common.inc, I had to apply patch manually though. Maybe because it was coded for previous versions of Drupal... Here I am -still- on Drupal 7.43.

I never did get this written for D8 at drupalcon. I blame the crappy wifi.

Updated patch #33.

This works great!

Just updated the version and removed some tags.

Thanks a lot for this.

I don't understand why for PHP7 there are 58 errors :-/ but for 5.5 is working

It passed for 5.5.

Going to mark it as RTBC again because the PHP7 failing tests aren't related to this issue.

diff --git a/core/lib/Drupal/Core/Asset/CssOptimizer.php b/core/lib/Drupal/Core/Asset/CssOptimizer.php
index 1a3cb90..e573d85 100644
--- a/core/lib/Drupal/Core/Asset/CssOptimizer.php
+++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
@@ -217,7 +217,7 @@ protected function processCss($contents, $optimize = FALSE) {
         # - Colon: Retain :pseudo-selectors.
         | ([\(:])\s+
       >xS',
         // Only one of the three capturing groups will match, so its reference
         // will contain the wanted value and the references for the
         // two non-matching groups will be replaced with empty strings.
         '$1$2$3$4',

The three needs changing to "four" since we are adding a capturing group.

Plus, (and more importantly) it'd be good to add test coverage for the cases in #6 too.

Alright.

New patch with test coverage for the cases in #6 and the comment in the CssOptimizer.php was updated.

Thanks!

we need test with both variants of quotes?

#46 tested both variants, I didn't want to do it differently.

Your patch looks great!

Thanks.

Testing both variants (single/double) is good, though i'd think the tests vaplas has covering the escaped single and escaped double quotes ... ah, double as covering both basic variants.

However, #53 is passing tests, and is directly covering the final issues raised in this issue (and no more), so i'd be inclined to RTBC #53 and leave anything else for follow-up.

I just noticed to my patch is not in the file list! why!? :-)

Both patches look great to me and if we can finally close this 6 years old issue once and for all I'm agree to commit any of these two.

So I will update the summary. pointing these two patches.

both patches looks, ok, I'm going to put this as RTBC to let the core committers decide which patch is going to be committed

I think the tests prove that #55 is the one to go for. However considering the change:

+++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
@@ -214,11 +216,11 @@ protected function processCss($contents, $optimize = FALSE) {
+      >xSs',

I think we should be a little cautious of this change. The test coverage here looks good and I think everything is fine so I'm going to commit to 8.3.x and leave out of 8.2.x for the time being. For the D7 port there should be a separate issue opened as per the backport policy.

Committed dc583b7 and pushed to 8.3.x. Thanks!

FILE: ...s/devdisk/dev/drupal/core/lib/Drupal/Core/Asset/CssOptimizer.php
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------
 209 | ERROR | [x] Concat operator must be surrounded by a single
     |       |     space
 209 | ERROR | [x] Concat operator must be surrounded by a single
     |       |     space
 209 | ERROR | [x] Concat operator must be surrounded by a single
     |       |     space
 209 | ERROR | [x] Concat operator must be surrounded by a single
     |       |     space
----------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Fixed coding standards fails on commit.

This is in 8.3.x and 8.4.x - and 8.2.x is irrelevant.

We need the backport for D7

#28 is a working and tested D7 patch

As per https://www.drupal.org/core/backport-policy - we need a new Drupal 7 issue opened.

#2877131: [D7] CSS aggregation strips some essential whitespace within strings