#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
Comment | File | Size | Author |
---|---|---|---|
#58 | css_aggregation_breaks_rules-460448-d6-58.patch | 2.03 KB | Albert Volkman |
#45 | drupal-460448-45.patch | 1.51 KB | gapple |
#40 | drupal-460448-40.patch | 1.42 KB | ridgerunner |
#36 | 460448_callback.patch | 1.99 KB | chx |
#33 | drupal-460448-33-D6.patch | 2.17 KB | gapple |
Comments
Comment #1
jamestombs CreditAttribution: jamestombs commentedAny fixes for this?
Upgraded to 5.18 and still an issue with both parts of CSS.
Comment #2
jamestombs CreditAttribution: jamestombs commentedFound the problem.
In common.inc, line 1664->1668.
The actual problem is line 1665.
CSS works after removing : from the regular expression.
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.
Comment #3
jamestombs CreditAttribution: jamestombs commentedComment #4
drummThis 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?Comment #5
jamestombs CreditAttribution: jamestombs commentedNo, there is a space before :hover, I guess the alternative is to have multiple CSS rules for each html element that is present:
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.
Comment #6
O_P_M CreditAttribution: O_P_M commentedComment #7
ridgerunner CreditAttribution: ridgerunner commentedThis 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.
Comment #8
ridgerunner CreditAttribution: ridgerunner commentedThis issue is most certainly a bug. Changing status from "by design" to "active".
Comment #9
ridgerunner CreditAttribution: ridgerunner commentedAlso changing version to 7.x-dev...
Comment #10
casey CreditAttribution: casey commentedTagging
Comment #11
gappleMarked #933006: Aggregation and Compression breaks certain CSS pseudo selectors as a duplicate of this issue
Comment #12
Jacine.
Comment #13
sunAny attempt to fix this should start with patch that contains (only) tests, to prove that the bug exists. Afterwards, fix it.
Comment #14
gapplefrom @ericduran in #933006: Aggregation and Compression breaks certain CSS pseudo selectors
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.Comment #15
sunTests.
Comment #16
sunWTF @ that regex. Still trying to figure it out via debugging.
Comment #18
JacineHere's some more stuff to test with:
BEFORE
AFTER
I'm not positive about the after code, still need to test myself.
Comment #19
sunThanks! 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.
Comment #20
JacineTests pass on my end, they have been updated to account for common CSS techniques, and the problem is fixed. You are my hero.
Comment #21
sunReworking the comments, Jacine will post one more special thing.
Comment #22
JacineSorry, 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;}
Comment #23
sunImproved the comments.
Comment #24
ericduran CreditAttribution: ericduran commentedNice, by the time I came to check it out, sun has it fixed. Nice. I'm off to test it.
Comment #25
sunImproved comments once more.
Comment #26
JacineThank 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.
Comment #27
ericduran CreditAttribution: ericduran commented2nd rtbc,
Oh and @sun thanks for the comments, my regex skills just multiply from reviewing the patches in this issue.
Comment #28
ridgerunner CreditAttribution: ridgerunner commentedYour 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:
A correct solution will never, ever, ever, change anything within any CSS string.
Comment #29
sunThat 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!
Comment #30
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #31
gappleShould this be backported to D5/D6?
--
I've posted an issue for @ridgerunner's comment #936316: CSS aggregation strips some essential whitespace within strings
Comment #32
webchickSounds like it, yeah.
Comment #33
gappleHere'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.
Comment #34
bfroehle CreditAttribution: bfroehle commentedThe
(?|
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.Comment #35
ngmaloney CreditAttribution: ngmaloney commentedSubscribe
Comment #36
chx CreditAttribution: chx commentedWe 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.
Comment #37
sunyay, thanks chx! :)
Comment #38
webchickYay 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.
Comment #39
webchickComment #40
ridgerunner CreditAttribution: ridgerunner commentedThe 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:
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.
Comment #42
casey CreditAttribution: casey commentedComment #43
bfroehle CreditAttribution: bfroehle commented#40: drupal-460448-40.patch queued for re-testing.
Comment #44
sunNice 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?
Comment #45
gappleI 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.
Comment #46
sunThank you! :)
Comment #48
bfroehle CreditAttribution: bfroehle commented#45: drupal-460448-45.patch queued for re-testing.
Comment #49
KoCo CreditAttribution: KoCo commented+1 subscribe
Comment #50
KoCo CreditAttribution: KoCo commented+1 subscribe
Comment #51
KoCo CreditAttribution: KoCo commented+1
Comment #52
sun#45: drupal-460448-45.patch queued for re-testing.
Comment #53
sunComment #54
webchickCommitted to HEAD. Hopefully for the last time. ;) Thanks for the optimization, ridgerunner!
Moving back down to 6.x.
Comment #55
magnusk CreditAttribution: magnusk commentedHere is another case that is broken.
The "content" string in a pseudo-attribute should not be modified:
CSS compression erroneously removes spaces inside the string (around the ':' and ',' characters it seems).
Comment #56
magnusk CreditAttribution: magnusk commentedComment #57
gapple@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
Comment #58
Albert Volkman CreditAttribution: Albert Volkman commentedD6 backport.
Comment #59
sun