Problem/Motivation

Proposed resolution

from @ridgerunner in #460448-28: Some CSS rules are broken once CSS aggregation is enabled

Your regex violates whitespace within strings. e.g. it destroys the following valid rule taken from the CSS spec: 4.1.7 Rule sets, declaration blocks, and selectors:

  p[example="public class foo\
  {\
      private int x;\
  \
      foo(int x) {\
          this.x = x;\
      }\
  \
  }"] { color: red }

A correct solution will never, ever, ever, change anything within any CSS string.

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?

Remaining tasks

Files: 
CommentFileSizeAuthor
#55 interdiff-46-54.patch64.61 KBvaplas
#55 936316-54.patch3.75 KBvaplas
#46 936316-46.patch62.69 KBaerozeppelin
#46 interdiff-936316-33-46.txt2.72 KBaerozeppelin
#46 test-only-fail-936316-46.patch61.51 KBaerozeppelin
#33 drupal-936316-d8-33.patch62.59 KBmgifford
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,957 pass(es), 1 fail(s), and 0 exception(s). View
#28 drupal-936316-28.patch123.69 KBjohns996
PASSED: [[SimpleTest]]: [MySQL] 41,067 pass(es). View
#20 drupal-936316-19.patch1.17 KBjohns996
FAILED: [[SimpleTest]]: [MySQL] 40,041 pass(es), 2 fail(s), and 0 exception(s). View
#19 drupal-936316-19.patch1.17 KBjohns996
FAILED: [[SimpleTest]]: [MySQL] 40,391 pass(es), 2 fail(s), and 0 exception(s). View
#1 drupal-936316-1.patch3.78 KBgapple
FAILED: [[SimpleTest]]: [MySQL] 26,016 pass(es), 1 fail(s), and 0 exception(es). View

Comments

gapple’s picture

FileSize
3.78 KB
FAILED: [[SimpleTest]]: [MySQL] 26,016 pass(es), 1 fail(s), and 0 exception(es). View

Here's a quick patch to add a test for the example from the CSS spec

sun’s picture

Priority: Normal » Minor
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-936316-1.patch, failed testing.

Jacine’s picture

subscribe.

ridgerunner’s picture

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.

magnusk’s picture

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)";
}
sun’s picture

Title: CSS Agregation does not respect whitespace within strings » CSS Aggregation strips some whitespace within strings
Priority: Minor » Normal

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.

xjm’s picture

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

mgifford’s picture

Version: 7.x-dev » 8.x-dev

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

tea.time’s picture

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.

neRok’s picture

Title: CSS Aggregation strips some whitespace within strings » CSS Aggregation strips some essential whitespace within strings

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.

johns996’s picture

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

Wim Leers’s picture

Title: CSS Aggregation strips some essential whitespace within strings » CSS aggregation strips some essential whitespace within selectors

This still needs to be fixed.

danillonunes’s picture

Title: CSS aggregation strips some essential whitespace within selectors » CSS aggregation strips some essential whitespace within strings

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

Wim Leers’s picture

#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?

danillonunes’s picture

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.

johns996’s picture

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+
johns996’s picture

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?

johns996’s picture

FileSize
1.17 KB
FAILED: [[SimpleTest]]: [MySQL] 40,391 pass(es), 2 fail(s), and 0 exception(s). View

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

johns996’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
1.17 KB
FAILED: [[SimpleTest]]: [MySQL] 40,041 pass(es), 2 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, drupal-936316-19.patch, failed testing.

Wim Leers’s picture

Issue tags: +Needs tests

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

Liam Morland’s picture

Issue tags: -Needs tests

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/

dougmorin0’s picture

Issue summary: View changes

Has this issue been abandoned?

johns996’s picture

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.

Liam Morland’s picture

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.

johns996’s picture

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.

johns996’s picture

FileSize
123.69 KB
PASSED: [[SimpleTest]]: [MySQL] 41,067 pass(es). View

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

johns996’s picture

Status: Needs work » Needs review
karimei’s picture

Status: Needs review » Reviewed & tested by the community

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:)

David_Rothstein’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +needs backport to D7

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

johns996’s picture

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.

mgifford’s picture

Status: Needs work » Needs review
FileSize
62.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,957 pass(es), 1 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 33: drupal-936316-d8-33.patch, failed testing.

hass’s picture

mgifford’s picture

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.');

magicmyth’s picture

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.

doitDave’s picture

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. :)

johns996’s picture

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.

doitDave’s picture

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 ;)

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ezoulou’s picture

+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.

Wim Leers’s picture

Issue tags: +Needs tests
johns996’s picture

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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

aerozeppelin’s picture

The last submitted patch, 46: test-only-fail-936316-46.patch, failed testing.

gnuget’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

This works great!

Just updated the version and removed some tags.

Thanks a lot for this.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 46: 936316-46.patch, failed testing.

gnuget’s picture

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

gnuget’s picture

Status: Needs work » Reviewed & tested by the community

It passed for 5.5.

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
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.

gnuget’s picture

Status: Needs work » Needs review
FileSize
63.39 KB
3.95 KB

Alright.

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

Thanks!

vaplas’s picture

FileSize
2.9 KB
63.77 KB

Why chosen "comment_hacks.css" for testing problem of quotes? May be new file?

What mean p, padding-left: 5px? It would be fun if we use q, quotes: none, no?)

And #46 not working for IS [example=... I added PCRE_DOTALL option (now options looks like xSs), but i'm not sure with this.

Also added cases with escaped quotes.

The three needs changing to "four"

done.

test coverage for the cases in #6

done.

vaplas’s picture

Sorry, I forgot about CssOptimizerUnitTest.php.

#53:

+++ b/core/tests/Drupal/Tests/Core/Asset/css_test_files/comment_hacks.css
@@ -10,6 +10,31 @@
+li.node-readmore a:after {
+  content: ": a word, and a phrase, etc.";
+}
+
+li.node-readmore a:after {
+  content: ': a word, and a phrase, etc.';
+}
+
+li.node-readmore a:after {
+  content: " (a word - and a phrase)";
+}
+
+li.node-readmore a:after {
+  content: ' (a word - and a phrase)';
+}

we need test with both variants of quotes?

gnuget’s picture

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.

mlncn’s picture

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.

vaplas’s picture

I do not agree that the #53 green more than the #55, because #53 passed tests randomly :) Also we do not rely on the fact that the points of #52 - is the final barrier before committing :)

gnuget’s picture

Issue summary: View changes

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.

dmouse’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

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.

  • alexpott committed dc583b7 on 8.3.x
    Issue #936316 by vaplas, johns996, aerozeppelin, mgifford, gapple,...

  • alexpott committed dc583b7 on 8.4.x
    Issue #936316 by vaplas, johns996, aerozeppelin, mgifford, gapple,...

  • alexpott committed dc583b7 on 8.4.x
    Issue #936316 by vaplas, johns996, aerozeppelin, mgifford, gapple,...

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

Status: Patch (to be ported) » Fixed

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

hass’s picture

Version: 8.4.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)

We need the backport for D7

johns996’s picture

#28 is a working and tested D7 patch

alexpott’s picture

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

hass’s picture

Version: 7.x-dev » 8.4.x-dev
Status: Patch (to be ported) » Fixed
hass’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.