#menu :hover > a.main {background:#76b9fd;color:#FFF;}
#menu :hover div{display:block;}

The above CSS rules do not work once CSS aggregation has been enabled.

Keeping just these rules out in a seperate un-aggregated CSS file fixes the problem, but I assume there is something within the CSS aggregator that is taking these rules apart.

Both rules are part of a drop down menu.

The first one when working keeps the master li a link background colour when in the div dropdown box, when it isn't working, the li a element goes back to the standard background colour and link colour so the user isn't clear which menu item they are on.

The second one when working displays the single div under a list element in the drop down, when it isn't working every drop down opens regardless of which list element you put your mouse over.

Working example can be found at http://www.action.org.uk

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jamestombs’s picture

Version: 5.15 » 5.18

Any fixes for this?

Upgraded to 5.18 and still an issue with both parts of CSS.

jamestombs’s picture

Component: system.module » base system

Found the problem.

In common.inc, line 1664->1668.

    $data = preg_replace('<
      \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.
      >x', '\1', $data);

The actual problem is line 1665.

\s*([@{}:;,]|\)\s|\s\()\s* |  # Remove whitespace around separators, but keep space around parentheses.

CSS works after removing : from the regular expression.

\s*([@{};,]|\)\s|\s\()\s* |  # Remove whitespace around separators, but keep space around parentheses.

Is there any reason that this is included? As far as I am aware of :hover is valid, although it may not necessarily work in older browsers, but it does work.

jamestombs’s picture

Status: Active » Needs review
drumm’s picture

Version: 5.18 » 7.x-dev
Status: Needs review » Closed (works as designed)

This is probably a problem in the development version as well, moving to 7.x.

A bare :hover is probably not what you want. I am guessing it implies #menu *:hover > a.main. Did you mean #menu:hover > a.main, removing the space before colon? Or should there be some element, #id, or .class before the colon?

jamestombs’s picture

No, there is a space before :hover, I guess the alternative is to have multiple CSS rules for each html element that is present:

#menu a:hover > a.main, #menu li:hover > a.main {background:#76b9fd;color:#FFF;}

I guess that would work, which could be similar to *:hover, but I am not sure how the different browsers will treat it.

Some more testing is required, will set up a test site when I get a moment and try it.

O_P_M’s picture

Version: 7.x-dev » 6.13
ridgerunner’s picture

This issue is definitely a real bug exactly as described above. The noted regex erroneously removes all whitespace preceding a colon. Although it may not be clear just how valid :hover is on its own, there is no doubt that other pseudo-classes are certainly valid with space in front of the colon. e.g. According to the CSS 2.1 specification section: 5.11.2, the :link pseudo-class is perfectly valid on its own for HTML 4 and is equivalent to a:link.

This issue could be handled by the patch soon to be commited over on the: Optimize CSS option causes php cgi to segfault in pcre function "match" issue.

ridgerunner’s picture

This issue is most certainly a bug. Changing status from "by design" to "active".

ridgerunner’s picture

Version: 6.13 » 7.x-dev
Status: Closed (works as designed) » Active

Also changing version to 7.x-dev...

casey’s picture

Issue tags: +CSS aggregation

Tagging

gapple’s picture

Jacine’s picture

.

sun’s picture

Any attempt to fix this should start with patch that contains (only) tests, to prove that the bug exists. Afterwards, fix it.

gapple’s picture

from @ericduran in #933006: Aggregation and Compression breaks certain CSS pseudo selectors

The solutions are:
- we don't strip white space around the ":"
- we don't use ":" selectors alone without a "*" in front of them (which I think is a horrible idea)
- we change the regex to not replace the white space before ":".

In this patch I'm taking the 1st approach in that we don't strip white spaces around the ":".

While the first option is simple, it will affect colons anywhere within the stylesheet. When simply removing the colon from the regex, the tests fail on expressions such as color: #F00;, which can safely have the space removed. It should be possible to add a second expression that only operates on colons within braces, though it would add processing overhead; it would have to be determined if the processing / file size trade off is worth it.

sun’s picture

Assigned: Unassigned » sun
Status: Active » Needs review
FileSize
4.47 KB

Tests.

sun’s picture

FileSize
6.14 KB

WTF @ that regex. Still trying to figure it out via debugging.

Status: Needs review » Needs work

The last submitted patch, drupal.css-pseudo.16.patch, failed testing.

Jacine’s picture

Here's some more stuff to test with:

BEFORE

some :pseudo .thing {
  -moz-border-radius: 3px;
  -webkit-border-radius: 3px;
  border-radius: 3px;
  filter: progid:DXImageTransform.Microsoft.Shadow(color=#000000, direction='180', strength='10');
  -ms-filter: "progid:DXImageTransform.Microsoft.Shadow(color=#000000, direction='180', strength='10')";
}

::-moz-selection {
  background: #000;
  color:#fff;

}
::selection {
  background: #000;
  color: #fff;
}

@media print {
  * {
    background: #000 !important;
    color: #fff !important;
  }
  @page {
    margin: 0.5cm;
  }
}

@media screen and (max-device-width: 480px) {
  background: #000;
  color: #fff;
}

textarea, select {
  font: 1em/160% Verdana, sans-serif;
  color: #494949;
}

AFTER

some :pseudo .thing{-moz-border-radius:3px;-webkit-border-radius:3px;border-radius:3px;filter:progid:DXImageTransform.Microsoft.Shadow(color=#000000,direction='180',strength='10');-ms-filter:"progid:DXImageTransform.Microsoft.Shadow(color=#000000,direction='180',strength='10')";}::-moz-selection{background:#000;color:#fff;}::selection{background:#000;color:#fff;}@media print{*{background:#000!important;color:#fff!important;}@page{margin:0.5cm;}}@media screen and (max-device-width:480px){background:#000;color:#fff;}textarea,select{font:1em/160% Verdana,sans-serif;color:#494949;}

I'm not positive about the after code, still need to test myself.

sun’s picture

Status: Needs work » Needs review
FileSize
8.52 KB

Thanks! Added all of that. I can only guess the @media stuff is what has been the undocumented part of the regex (without tests).

So I guess this one is RTBC.

Jacine’s picture

Status: Needs review » Reviewed & tested by the community

Tests pass on my end, they have been updated to account for common CSS techniques, and the problem is fixed. You are my hero.

sun’s picture

Status: Reviewed & tested by the community » Needs work

Reworking the comments, Jacine will post one more special thing.

Jacine’s picture

Sorry, one more thing here...

tr:nth-child(2n+1){background:#ffc;font-style:italic;}

This wont work as:

tr:nth-child (2n+1) {background:#ffc;font-style:italic;}

sun’s picture

Status: Needs work » Needs review
FileSize
8.85 KB

Improved the comments.

ericduran’s picture

Nice, by the time I came to check it out, sun has it fixed. Nice. I'm off to test it.

sun’s picture

FileSize
8.92 KB

Improved comments once more.

Jacine’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @sun (and @ericduran for your earlier attempts)!

Never mind my comment in #22, I misunderstood what was happening with parenthesis, and it's not a problem.

This is great. I am actually starting to like tests, and even have a general understand of what's going on in the regex because of your awesome comments.

ericduran’s picture

2nd rtbc,

Oh and @sun thanks for the comments, my regex skills just multiply from reviewing the patches in this issue.

ridgerunner’s picture

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.

sun’s picture

That may be true, but 1) this issue has a focus. 2) The existing regex is solely corrected to fix this bug. 3) The patch adds unit tests to prevent this particular bug from appearing again.

The problem you are raising might be true in certain edge-case scenarios, so if you really care for it, then please open a separate issue for it. Thanks!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

gapple’s picture

Issue tags: -Needs tests

Should this be backported to D5/D6?

--
I've posted an issue for @ridgerunner's comment #936316: CSS aggregation strips some essential whitespace within strings

webchick’s picture

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

Sounds like it, yeah.

gapple’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.17 KB

Here's a patch for D6.

D5 combines whitespace handling and comment removal in one regex, so I'm not sure of the correct approach. I also vaguely remember a bug in D6 about newline handling in CSS that still may be an issue in D5 since the regex removes all newline characters.

bfroehle’s picture

Status: Needs review » Needs work

The (?| regex construction is only present in PCRE 7.2 and newer, which is uncommon enough to warrant some heated debate in #962318: Require PCRE Version >= 7.2 regarding whether Drupal 7 should require a specific version or PCRE. In particular, CentOS 5.x only has PCRE version 6.6 which barfs when trying to compile the regex.

I suspect that the installed base of D6 is generally older, and thus even less amenable to the patch in #33.

Setting to needs work for D6, but the reality is that a lot of D7 users would be happy if the regex could be rewritten to not use the (?| construction.

ngmaloney’s picture

Subscribe

chx’s picture

Version: 6.x-dev » 7.x-dev
Priority: Normal » Critical
Status: Needs work » Needs review
FileSize
1.99 KB

We need to remove ?| to accomodate the dinosaur called RHEL5. I thought that PHP 5.2 would free us from the clutches of that but no: we can't require PCRE 7.2. So I have changed to using a preg_replace_callback.

sun’s picture

Status: Needs review » Reviewed & tested by the community

yay, thanks chx! :)

webchick’s picture

Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Yay for (slightly) easier to grok code!

I also asked whether we should benchmark this, but since it only happens on the aggregation process, it's not called often enough for a few ms to make a difference.

Committed to HEAD. Thanks!

Moving back down to 6.x.

webchick’s picture

Priority: Critical » Normal
ridgerunner’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.42 KB

The recent addition of a callback function is unnecessary (as was the (?|...|...) branch reset construct it replaced). There is a much simpler solution.

The regex in question has three mutually exclusive global alternatives, each with its own capture group. Only one of these alternatives will ever match at any given position within the subject text. When the first alternative matches, the regex engine doesn't bother checking the other alternatives - the result is that the second two capture groups ($2 and $3) contain undefined values. If the third alternative matches, the first two capture groups contain empty strings. However, it is perfectly ok to use all three capture groups within the replacement string of preg_replace(), even if their values are undefined - they are simply converted to an empty string. Thus, the replace string can simply include all three backreferences like so: '$1$2$3'. Only one will contain a value - the other two will always contain empty strings.

The following simplified regex illustrates the three approaches:

// Sun's method from post #25 uses the "Branch reset" construct:
$content = preg_replace('/(?|(A)|(B)|(C))/', '$1', $content);

// Chx's method from post #36 uses a (slow) callback:
$content = preg_replace_callback('/(A)|(B)|(C)/', '_callback', $content);

// A simpler method:
$content = preg_replace('/(A)|(B)|(C)/', '$1$2$3', $content);

The results after a replacement are identical for all three methods. My benchmark tests show that the callback method is six times slower than either of the other two.

Status: Needs review » Needs work

The last submitted patch, drupal-460448-40.patch, failed testing.

casey’s picture

Version: 6.x-dev » 7.x-dev
bfroehle’s picture

Status: Needs work » Needs review

#40: drupal-460448-40.patch queued for re-testing.

sun’s picture

Nice one. I wonder whether it would be worth to add an inline comment or slightly tweak the existing to explain what's going on, or do you guys think that's trivial?

gapple’s picture

FileSize
1.51 KB

I think a comment would help avoid future WTF moments. I usually expect replacements to contain something, so knowing that they won't in this instance could make comprehending the expression quicker for anyone unfamiliar with what the CSS aggregator does.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thank you! :)

Status: Reviewed & tested by the community » Needs work
Issue tags: -CSS aggregation

The last submitted patch, drupal-460448-45.patch, failed testing.

bfroehle’s picture

Status: Needs work » Needs review
Issue tags: +CSS aggregation

#45: drupal-460448-45.patch queued for re-testing.

KoCo’s picture

+1 subscribe

KoCo’s picture

+1 subscribe

KoCo’s picture

+1

sun’s picture

#45: drupal-460448-45.patch queued for re-testing.

sun’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to HEAD. Hopefully for the last time. ;) Thanks for the optimization, ridgerunner!

Moving back down to 6.x.

magnusk’s picture

Status: Needs work » Patch (to be ported)

Here is another case that is broken.
The "content" string in a pseudo-attribute should not be 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).

magnusk’s picture

Version: 6.x-dev » 7.0
Status: Patch (to be ported) » Needs work
gapple’s picture

Version: 7.0 » 6.x-dev

@magnusk
A separate issue was opened earlier to address whitespace being removed in strings, and already includes a patch with a test to verify the error.
#936316: CSS aggregation strips some essential whitespace within strings

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.03 KB

D6 backport.

sun’s picture

Assigned: sun » Unassigned
Issue summary: View changes

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.