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
- Review #53's patch and review #55's patch, both looks ok but are different.
- Commit!
Comment | File | Size | Author |
---|---|---|---|
#55 | interdiff-46-54.patch | 64.61 KB | Anonymous (not verified) |
#55 | 936316-54.patch | 3.75 KB | Anonymous (not verified) |
#46 | 936316-46.patch | 62.69 KB | aerozeppelin |
#46 | interdiff-936316-33-46.txt | 2.72 KB | aerozeppelin |
#46 | test-only-fail-936316-46.patch | 61.51 KB | aerozeppelin |
Comments
Comment #1
gappleHere's a quick patch to add a test for the example from the CSS spec
Comment #2
sunComment #4
Jacinesubscribe.
Comment #5
ridgerunner CreditAttribution: ridgerunner commentedFrom @gapple
Yes. CSS generated content can be specified to have significant whitespace just like any PRE tag. For example...
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.Comment #6
magnusk CreditAttribution: magnusk commentedHere is a CSS compression case that is seriously broken, mangling content display.
The "content" string in a pseudo-attribute is wrongly modified:
CSS compression erroneously removes spaces inside the string (around the ':' and ',' characters it seems).
By contrast, the following "content" string is left alone:
Comment #7
sunWe should add @magnusk's case to the tests, as I think it's a slightly different case in terms of the processing code.
Trailing white-space.
Powered by Dreditor.
Comment #8
xjmTracking. The examples in #6 are very common. This also needs to be backported to 6.x when it is fixed.
Comment #9
mgiffordI'm assuming this should be fixed in D8 & then brought back to D7.. Or closed if it is no longer an issue.
Comment #10
tea.time CreditAttribution: tea.time commentedConfirming this is still an issue on Drupal 7.19; ran into it today with the following case:
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.Comment #11
neRok CreditAttribution: neRok commentedMarked #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.
Comment #12
johns996 CreditAttribution: johns996 commentedtea.time, did you ever find a way to work around this issue?
Comment #13
Wim LeersThis still needs to be fixed.
Comment #14
danillonunes CreditAttribution: danillonunes commentedAs stated by examples from #5 and #6, the issue is not just about CSS selectors, but about CSS strings instead.
Comment #15
Wim Leers#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?
Comment #16
danillonunes CreditAttribution: danillonunes commentedThat'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.
Comment #17
johns996 CreditAttribution: johns996 commentedThis 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:
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:
Comment #18
johns996 CreditAttribution: johns996 commentedI 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?
Comment #19
johns996 CreditAttribution: johns996 commentedThis patch fixes the problems I'm having with this issue on 7.x. Can anyone else test and verify.
Comment #20
johns996 CreditAttribution: johns996 commentedLet's try this again with the status and version updated to reflect my patch.
Comment #22
Wim LeersCould you add test coverage for this? Without tests, this is almost certain to regress at some point!
Comment #23
Liam MorlandPerhaps 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:
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/
Comment #24
dougmorin0 CreditAttribution: dougmorin0 commentedHas this issue been abandoned?
Comment #25
johns996 CreditAttribution: johns996 commentedI 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.
Comment #26
Liam MorlandThere 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.
Comment #27
johns996 CreditAttribution: johns996 commentedSpent 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.
Comment #28
johns996 CreditAttribution: johns996 commentedBetter regex and a test to make sure this fix stays in place.
Comment #29
johns996 CreditAttribution: johns996 commentedComment #30
karimei CreditAttribution: karimei commentedI 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:)
Comment #31
David_Rothstein CreditAttribution: David_Rothstein commentedThis looks like it still needs to go into Drupal 8.
Comment #32
johns996 CreditAttribution: johns996 commentedI 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.
Comment #33
mgiffordWas
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.
Comment #35
hass CreditAttribution: hass commentedComment #36
mgiffordI 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.');
Comment #37
magicmyth CreditAttribution: magicmyth commentedI 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:
The workaround until a real fix is in place is to use the unicode for space:
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:
Note you will have to be careful with escaping as a colon or many other characters directly before the "\0020" will alter the result.
Comment #38
doitDave CreditAttribution: doitDave commentedPlease 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. :)
Comment #39
johns996 CreditAttribution: johns996 commentedMy 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.
Comment #40
doitDave CreditAttribution: doitDave commentedDon'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 ;)
Comment #42
ezoulou CreditAttribution: ezoulou commented+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.
Comment #43
Wim LeersComment #44
johns996 CreditAttribution: johns996 commentedI never did get this written for D8 at drupalcon. I blame the crappy wifi.
Comment #46
aerozeppelin CreditAttribution: aerozeppelin at California State University San Bernardino commentedUpdated patch #33.
Comment #48
gnugetThis works great!
Just updated the version and removed some tags.
Thanks a lot for this.
Comment #50
gnugetI don't understand why for PHP7 there are 58 errors :-/ but for 5.5 is working
Comment #51
gnugetIt passed for 5.5.
Going to mark it as RTBC again because the PHP7 failing tests aren't related to this issue.
Comment #52
alexpottThe 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.
Comment #53
gnugetAlright.
New patch with test coverage for the cases in #6 and the comment in the CssOptimizer.php was updated.
Thanks!
Comment #54
Anonymous (not verified) CreditAttribution: Anonymous commentedWhy 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 useq
,quotes: none
, no?)And #46 not working for IS
[example=..
. I added PCRE_DOTALL option (now options looks likexSs
), but i'm not sure with this.Also added cases with escaped quotes.
done.
done.
Comment #55
Anonymous (not verified) CreditAttribution: Anonymous commentedSorry, I forgot about CssOptimizerUnitTest.php.
#53:
we need test with both variants of quotes?
Comment #56
gnugetwe need test with both variants of quotes?
#46 tested both variants, I didn't want to do it differently.
Your patch looks great!
Thanks.
Comment #57
mlncn CreditAttribution: mlncn at Agaric for Drutopia commentedTesting 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.
Comment #58
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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 :)
Comment #59
gnugetI 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.
Comment #60
dmouseboth patches looks, ok, I'm going to put this as RTBC to let the core committers decide which patch is going to be committed
Comment #61
alexpottI think the tests prove that #55 is the one to go for. However considering the change:
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!
Fixed coding standards fails on commit.
Comment #66
alexpottThis is in 8.3.x and 8.4.x - and 8.2.x is irrelevant.
Comment #67
hass CreditAttribution: hass commentedWe need the backport for D7
Comment #68
johns996 CreditAttribution: johns996 commented#28 is a working and tested D7 patch
Comment #69
alexpottAs per https://www.drupal.org/core/backport-policy - we need a new Drupal 7 issue opened.
Comment #70
hass CreditAttribution: hass commentedComment #71
hass CreditAttribution: hass commented#2877131: [D7] CSS aggregation strips some essential whitespace within strings