The following browser hack is killed by the drupal_build_css_cache() function and therefor destroys my theme.

/* Commented Backslash Hack hides rule from IE5-Mac \*/
#nav_main a { float: none; }
/* End IE5-Mac hack */

please correct the wrong regexp in drupal_build_css_cache():

/\*([^*\\\\]|\*(?!/))+\*/ |   # Remove comments that are not CSS hacks.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ejort’s picture

Just as an FYI, I believe you can also use an "@media" block to hide styles from IE5 mac. eg:

@media all {
  #nav_main a { float: none; } /* not mac ie 5 */
}

Has the bonus of not relying on a parsing quirk to do its job and should therefore be more robust in the face of different CSS compression techniques.

hass’s picture

Status: Active » Closed (fixed)

Thank you very much for your hint. i learned this by your tipp and the hack in the CSS was already inside a media {} and therefor ineffective. So we are going to remove the senseless hack and all will be good. So i close this case... aside i learned i cannot add a CSS style without a media attribute... what's not so good if you have the media tags inside your css only.

hass’s picture

Component: base system » system.module
Priority: Critical » Normal
Status: Closed (fixed) » Active

i will reopen this now, while i found code in drupal core that will not work as desired...

[modules/system/default.css]
/* Hides from IE-mac \*/
* html .clear-block {
  height: 1%;
}
.clear-block {
  display: block;
}
/* End hide from IE-mac */

...after this code is compressed my drupal core this CSS part will be visible for IEMac and not hidden.

ejort’s picture

Replacing the commented backslash hack with an @media all { ... } block should fix this issue without any side effects (that I know about). That's the form we use in our standard stylesheets.

hass’s picture

Version: 5.1 » 6.x-dev

We need to fix this for CSS aggregate and compress feature.

Any regex guru out there able to fix this regex for the Commented Backslash Hack?

$contents = 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', $contents);
Crell’s picture

IMO, the solution isn't to make the compressor more complex to handle badly formed code, it's to remove the badly formed code from Drupal in the first place. Let's either use @media for that code block or simply remove the IE-Mac hiding entirely. (I mean really, that's like supporting Netscape 3.)

Stefan Nagtegaal’s picture

I'm all for removing the "IE/Mac"-hack.. Any selfrepecting person on a Mac brows' the web with Safri (free from Apple), Camina or Firefox..

Let's face it, IE for Mac is dead since Apple released Safari and Mozilla released Firefox for Mac. Let's remove this ugly piece of code.

hass’s picture

Status: Active » Needs review
FileSize
584 bytes

I'm with you... i only thought not to change the lines to keep the position-is-everything solution as is.

Here is a patch for review.

hass’s picture

Title: css_cache kills "Commented Backslash Hack" » Remove outdated IE5-mac hacks for build_css_cache regex
Dries’s picture

Status: Needs review » Reviewed & tested by the community

Committed. :)

hass’s picture

Status: Reviewed & tested by the community » Fixed

thx

hass’s picture

Status: Fixed » Patch (to be ported)

Aside - what's about D5? This patch should apply, too...

hass’s picture

Version: 6.x-dev » 5.x-dev
Status: Patch (to be ported) » Needs review

patch apply's cleanly to D5, too.

Steven’s picture

Status: Needs review » Needs work

This patch is silly. By removing the two comments, but leaving everything else intact, the .clear-block { display: inline-block; } is now always overridden by the later .clear-block { display: block; } style. It's dead CSS and it takes a whopping 5 seconds of looking at defaults.css to discover this.

The excuse to "leave the position-is-everything hack intact" is silly and just an excuse to not dig deeper.

Dries’s picture

I've rolled back the patch so we can have a better look at it.

hass’s picture

Status: Needs work » Needs review
FileSize
631 bytes

yeah, you are right. corrected patch attached.

ejort’s picture

Unfortunately I believe that the display { inline-block; } is still required (or at least something else needs to be added) for IE 7 support.

IE 7 doesn't trigger the hasLayout property (responsible for it's float clearing behaviour) when there is a height set as older IE's used to do. So the current 'height: 1%;' will only work for IE 6 and below.

What DOES trigger hasLayout in IE 7 though is setting 'display' to 'inline-block'. Another aspect of hasLayout is that once it's set on an element it can't be removed, so despite the 'display' property later being changed to 'block', the hasLayout triggering side effect remains.

More info on this is here if you're interested: http://www.tanfa.co.uk/archives/show.asp?var=300

Another alternative is to put 'zoom: 1;' directly after the 'height: 1%;'. The 'zoom' property is another hasLayout trigger that works in IE 6 & 7. If we don't care about Win IE < 6 then we could also ditch the 'height: 1%;' altogether.

hass’s picture

Thank you for the link. so my first patch was correct, isn't it!? As i remember zoom have some side effects i cannot remember...

We shouldn't reinvent or try to reinvent the wheel... this is why i said - keep the very well documented example code as is.

drumm’s picture

Status: Needs review » Closed (won't fix)

Only necessary bug fixes will go into Drupal 5.x to avoid potentially breaking themes.

hass’s picture

Version: 5.x-dev » 6.x-dev
Status: Closed (won't fix) » Needs review

Move back to D6...

BioALIEN’s picture

If we're not going to port this to D5.x branch, then I believe we should use the @media all to fix the bug for D6.x and get rid of as much CSS hacks as possible. If removing this CSS hack introduces bugs for IE7, then we can always add this to the special ie7-fix.css stylesheet that ships with core.

Frankly, some of it was destroying our D5 themes, and I can't bear to see this code remain in D6. Experienced designers almost always know what hacks to apply to get their themes working correctly.

hass’s picture

We don't have any bugs with this markup free clearing hack... we only have bugs with the backslash hack for MacIE5. This make the CSS compressor destroying the IEMac5 hack. The rest of this markup free clearing hack is valid and must stay as is.

We must get http://drupal.org/node/113607 in to solve some of this bugs that occur by the buggy regex that cleans up the comments in css and is the source of this problems.

hass’s picture

We have a much better aggregator now, but the problem is as is - the optimization regex break the MAC hack and produce problematic CSS.

Please take a look into #8 patch, please.

Stefan Nagtegaal’s picture

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

Here's a new patch which works for 100%.
It drops IE:Mac support, and fixes up the hasLayout problems in IE7.

Please review and apply...

Stefan Nagtegaal’s picture

FileSize
718 bytes

Forgot the patch... (bummer)

lilou’s picture

Patch still applied against CVS HEAD.

cburschka’s picture

The code looks okay, but it should be run by the W3C CSS validator as well.

Status: Needs review » Needs work

The last submitted patch failed testing.

lilou’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

casey’s picture

Issue tags: +CSS aggregation

Tagging. There are duplicate/related issues.

casey’s picture

Status: Needs work » Needs review
FileSize
2.2 KB

Lets drop support for IE-Mac in D7. Is it actually used?

hass’s picture

Status: Needs review » Needs work

CSS hacks does not mean IE Mac... IE6 also may need hacks...

casey’s picture

Status: Needs work » Needs review

IE6 doesn't *support* backslash hacks!

seutje’s picture

hmm, personally I wouldn't fiddle with the clearfix stuff, it's been heavily tested and backwards compatibility has no downsides at all in this case

also, doesn't IE7 need that inline-block to block stuff?

casey’s picture

Drupal's clearfix is based upon http://www.positioniseverything.net/easyclearing.html

display: inline-block; is for IE for Mac only which has a market share worldwide of like 1E-10 percent. It's total nonsense to support that browser.

The exception in the regexp for removing comments is only there for the IE-Mac backslash hack (http://www.sam-i-am.com/work/sandbox/css/mac_ie5_hack.html).

ejort’s picture

Status: Needs review » Needs work

@casey: see #17, the inline-block isn't just for IE5, it also triggers hasLayout for IE7, even if it's overridden immediately after by the "display: block" declaration. If we remove it, we need to include a zoom:1 or something that triggers hasLayout to ensure that this still works for IE7 as the "height: 1%" line which does this for IE 5 & 6 doesn't work for IE7.

JohnAlbin’s picture

Just found this issue.

GOD, YES!

IE5/mac is long dead and having support for it in D7 is just embarrassing. It also causing us a great deal of pain. See #444228: Optimize CSS option causes php cgi to segfault in pcre function "match"

casey’s picture

I don't see us removing the clearfix class from D7 (I hope from D8 though). So we need an updated version of clearfix without the IE5/mac hack.

seutje and ejort are however right on my patch; it's not correct.

http://www.webtoolkit.info/css-clearfix.html gives an alternate clearfix which might the easiest solution for us.

JohnAlbin’s picture

Over in #444228: Optimize CSS option causes php cgi to segfault in pcre function "match", I noticed this comment and am posting my response here since its more appropriate.

If we follow this path, we have to place a warning at the "Performance page" bellow the "Enable CSS optimization" about hacks not supported.

I'd like to state for the record, that I disagree with that assertion. If IE for Mac is not supported, we just list that on the Drupal project's requirements page. We don't need to litter the UI with warnings about a long dead obscure browser not working with a particular feature.

casey’s picture

Status: Needs work » Needs review
FileSize
2.05 KB

If we do this, we can solve #444228: Optimize CSS option causes php cgi to segfault in pcre function "match" by removing support for IE-mac hacks (nobody uses that browser and we never tested it anyway).

JohnAlbin’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed the proposed changes very carefully. I've been using clearfix successfully for 6 years, so changes always need to be scrutinized. I spent about an hour making sure all the CSS is necessary. The IE7 "addition" is necessary because the old MacIE5 "display: inline-block" rule also triggered hasLayout in IE7. Since we're removing the IE5 bit we need the IE7 line. Even still, this is less code. and will help out with #444228: Optimize CSS option causes php cgi to segfault in pcre function "match".

Thanks, casey!

casey’s picture

Damien Tournoud’s picture

Priority: Normal » Critical

Actually, this might be critical, now that we removed the support for MacIE5 hacks from the CSS aggregator :)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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

DamienMcKenna’s picture

Is there any value in backporting this? I mean, IE-Mac probably wouldn't even work on a modern OSX machine anyway and has been discontinued for over five years (as compared to IE6 which is still officially supported).