IE: Stylesheets ignored after 30 link/style tags

hass - March 1, 2008 - 18:06
Project:Drupal
Version:7.x-dev
Component:base system
Category:bug report
Priority:critical
Assigned:Unassigned
Status:needs work
Description

There is an important issue with the style sheets we are adding. I have been run into this issue at RTL layout development and lost many hours on figuring out - why the IE layout was broken and finally found http://support.microsoft.com/kb/262161.

We should change the way how we add CSS files to the page to solve this issue. Please keep in mind - since we added RTL support, the RTL people will be the very first running into this issue, but others with many modules installed will run into the same issues. This bug makes development VERY difficult and impossible, while i cannot turn on css aggregation in development.

If we go the following way we can keep the single files, but we end up with "one" style tag only - what finally solves the issues!

<style type="text/css" media="all">
@import "/drupal6/sites/all/themes/mytheme/node.css";
@import "/drupal6/sites/all/themes/mytheme/default.css";
@import "/drupal6/sites/all/themes/mytheme/admin.css";
@import "/drupal6/sites/all/themes/mytheme/test1.css";
@import "/drupal6/sites/all/themes/mytheme/test2.css";
@import "/drupal6/sites/all/themes/mytheme/test3.css";
@import "/drupal6/sites/all/themes/mytheme/test4.css";
@import "/drupal6/sites/all/themes/mytheme/test5.css";
</style>

#1

Trendy - March 3, 2008 - 10:39

Hello,
The problem, I had just made. Imports of over 37 individual css files from different modules. The IE7 has not completely accepted the layout. If you build a big side wants to many modules, such problems arise here soon. I think there is a fundamental solution required, as well as module developers CSS files.

greetings Trendy

#2

dman - March 3, 2008 - 11:23

I guess the fundamental solution is turn on css aggregation. It'll make your pages load faster too.
It's a bitch for developing ... but some form of css aggregation is the answer to this bug one way or another anyway.

#3

hass - March 3, 2008 - 14:32

The above code example will solve this in fundamental manner... developing a site CSS is nearly impossible with aggregate and compress activated!

#4

dman - March 3, 2008 - 14:53

True 'nuff.
Reading the issue, I see that the problem is only with the number of tags in the doc, not the actual number of stylesheets, (which I thought may have been a more understandable limitation of the code)!

#5

hass - March 3, 2008 - 17:24

Well, it counts the number of <style></style> and <link type="text/css" ... /> not the number of files added with @import... so you can add 1000 @import files inside a <style></style> without any problems. 1000 is untested, but it solved the limitation troubles at 30 for me...

#6

brahms - April 25, 2008 - 12:36

I totally agree with this request to use only one style tag per media type to solve the issue with the IE limitation. Using css aggregation is definititely no solution for me, because I am using private file download method and for security reasons I do have to use this. Looking at the function drupal_get_css() in common.inc, it does not seem to be much work to implement the request of hass (by the way: thank you very much hass for prividing the excellent yaml-for-drupal theme to the drupal community!). Maybe it would make sense to use a new variable to control whether to use one style tag per media type or to use one style tag per css file import as it is done now. I hope drupal's core team will accept this request, I don't like core patches at all and this would be needed otherwise.

#7

brahms - April 29, 2008 - 15:49

Here is a workaround for this issue for Drupal 5.7 (I know this may not be the right place because this issue is written for Drupal 6.1 but maybe someone has enough time to port my modification of 5.7 to 6.1...)

First I defined a new system variable "combine_css" which can be enabled under admin/settings/performance. The code for this is at the end of function system_performance_settings() (right after the form element for "preprocess_css") in the file system.module:

<?php
$form
['bandwidth_optimizations']['combine_css'] = array(
   
'#type' => 'radios',
   
'#title' => t('Combine CSS files per media type'),
   
'#default_value' => intval(variable_get('combine_css', FALSE)),
   
'#options' => array(t('Disabled'), t('Enabled')),
   
'#description' => t("Internet Explorer ignores css stylesheets after 30 link/style tags (see this <a href=\"http://support.microsoft.com/kb/262161\" title=\"IE: Stylesheets ignored after 30 link/style tags\" rel=\"nofollow\"> issue</a> on drupal.org. By enabling this option all import links for css files are combined into one &lt;style&gt; tag per media type to reduce the number of tags."),
  );
?>

Finally I modified the function drupal_get_css() in the file common.inc. If the system variable combine_css is enabled, drupal_get_css() combines the import commands for css stylesheets per media type into one <style> tag. No preprocessing of module and theme stylesheets is respected. If combine_css is disabled, everything works like it did without my modifications.

<?php
function drupal_get_css($css = NULL) {
 
$output = '';
  if (!isset(
$css)) {
   
$css = drupal_add_css();
  }

 
$preprocess_css = variable_get('preprocess_css', FALSE);
 
$combine_css = variable_get('combine_css', FALSE);

 
$directory = file_directory_path();
 
$is_writable = is_dir($directory) && is_writable($directory) && (variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC) == FILE_DOWNLOADS_PUBLIC);

  foreach (
$css as $media => $types) {
   
// If CSS preprocessing is off, we still need to output the styles.
    // Additionally, go through any remaining styles if CSS preprocessing is on and output the non-cached ones.
   
if ($combine_css) {
     
$no_module_preprocess_media = '';
     
$no_theme_preprocess_media = '';
     
$output_media = '';
    }
    foreach (
$types as $type => $files) {
      foreach (
$types[$type] as $file => $preprocess) {
        if (!
$preprocess || !($is_writable && $preprocess_css)) {
         
// If a CSS file is not to be preprocessed and it's a module CSS file, it needs to *always* appear at the *top*,
          // regardless of whether preprocessing is on or off.
         
if (!$preprocess && $type == 'module') {
            if (
$combine_css) {
             
$no_module_preprocess_media .= '@import "'. base_path() . $file .'";' ."\n";
            } else {
             
$no_module_preprocess .= '<style type="text/css" media="'. $media .'">@import "'. base_path() . $file .'";</style>' ."\n";
            }
          }
         
// If a CSS file is not to be preprocessed and it's a theme CSS file, it needs to *always* appear at the *bottom*,
          // regardless of whether preprocessing is on or off.
         
else if (!$preprocess && $type == 'theme') {
            if (
$combine_css) {
             
$no_theme_preprocess_media .= '@import "'. base_path() . $file .'";' ."\n";
            } else {
             
$no_theme_preprocess .= '<style type="text/css" media="'. $media .'">@import "'. base_path() . $file .'";</style>' ."\n";
            }
          }
          else {
            if (
$combine_css) {
             
$output_media .= '@import "'. base_path() . $file .'";' ."\n";
            } else {
             
$output .= '<style type="text/css" media="'. $media .'">@import "'. base_path() . $file .'";</style>' ."\n";
            }
          }
        }
      }
    }

    if (
$is_writable && $preprocess_css) {
     
$filename = md5(serialize($types)) .'.css';
     
$preprocess_file = drupal_build_css_cache($types, $filename);
      if (
$combine_css) {
       
$output .= '@import "'. base_path() . $preprocess_file .'";'. "\n";
      } else {
       
$output .= '<style type="text/css" media="'. $media .'">@import "'. base_path() . $preprocess_file .'";</style>'. "\n";
      }
    }

    if (
$combine_css && $no_module_preprocess_media) {
     
$no_module_preprocess_media = '<style type="text/css" media="'. $media .'">'. $no_module_preprocess_media .'</style>'. "\n";
     
$no_module_preprocess .= $no_module_preprocess_media;
    }
    if (
$combine_css && $no_theme_preprocess_media) {
     
$no_theme_preprocess_media = '<style type="text/css" media="'. $media .'">'. $no_theme_preprocess_media .'</style>'. "\n";
     
$no_theme_preprocess .= $no_theme_preprocess_media;
    }
    if (
$combine_css && $output_media) {
     
$output_media = '<style type="text/css" media="'. $media .'">'. $output_media .'</style>'. "\n";
     
$output .= $output_media;
    }
  }

  return
$no_module_preprocess . $output . $no_theme_preprocess;
}
?>

I tested this code without CSS aggregation and a yaml-for-drupal theme. It works in IE 7 and Firefox 2.0.0.14. Now IE 7 processes all CSS stylesheets!

#8

te-brian - May 5, 2008 - 19:59

Hey All,

For those of you weary about messing with core includes, we were able to find a workaround for this problem that doesn't solve the problem at a core level, but does work for the time being.

The lead up to this revelation came from our initial instinct to write a custom module that would replace drupal_get_css with our own get_css function (which we would then have to call in our template.php). This custom module would combine like media types into one style tag and hopefully solve the 30+ sheets problem. Well, looking at the code for drupal_get_css made us actually look at how drupal_add_css works and this is when we first noticed the fact that drupal_add_css already has a parameter that allows you to mark a css to not be preprocessed (read: aggregated).

So... we simply make sure that all drupal_add_css calls in our template.php are told not to preprocess (note: we are using the zen theme as our starting point):

drupal_add_css(path_to_subtheme() .'/layout.css', 'theme', 'all', $preprocess_theme);

$preprocess_theme is set to FALSE at the top of our template.php so that it will only need to be changed once when it is time to aggregate all css.

Then, just make sure that css aggregation is turned ON in the system settings. Now, all the core and module css style sheets will be aggregated and you can control which template style sheets are not for development purposes. This strategy will solve the 30+ style sheet IE problem up until the point that your theme has more than 30 style sheets (and you could preprocess theme style sheets that are "stable" to free up more space). The added benefit is that development is a bit quicker because the page loads much quicker with all the module and core style sheets aggregated.

Hope this helps someone,
Brian

#9

hass - May 5, 2008 - 20:27

@Brian: i don't like to duplicate nor support hacked core functions swapped in a custom module... it would be much better to get core fixed. If someone would write a well working and core worthy clean patch I'm sure that the core maintainers will not refuse this request. We might have a good chance to get this into D6.3...

I know i maintained the @charset core bugfix in D5 times and this one have been fixed somewhat quickly, too. People affected by this issue could replace the common.inc or patch themself if we have a working common.inc. I'm reluctant about adding an additional module and changing all core API function to mymodule_add_css().

I will not answer on any support requests about such hacks.

#10

hass - May 5, 2008 - 21:54
Status:active» needs review

Here is a 5 line changing patch that seem to fix the issue.

Try it out and get the core maintainers eyes on this case :-)

AttachmentSize
common_style_limit_D5_2008050501.patch 2.19 KB
common_style_limit_D6_2008050501.patch 2.32 KB
common_style_limit_HEAD_2008050501.patch 2.35 KB
Testbed results
common_style_limit_D5_2008050501.patchfailedFailed: Failed to apply patch. Detailed results
common_style_limit_D6_2008050501.patchfailedFailed: Failed to apply patch. Detailed results
common_style_limit_HEAD_2008050501.patchfailedFailed: Failed to apply patch. Detailed results

#11

hass - May 6, 2008 - 20:11
Status:needs review» needs work

Damn, the media declaration breaks IE6 and below. Selfhtml.org is no trustworthy resource...

#12

Gábor Hojtsy - May 8, 2008 - 09:20

Well, hass, as suggested above, we could still do grouped @imports' with media="$media" specified in the <style> tag. So one <style> tag per one media type. That should avoid IE6 issues?

#13

Paratio - May 12, 2008 - 18:18

Hello Everyone,
I think a fundamental solution to managing CSS in drupal core would be fine.
There should be a way to use "css aggregation" when private filesystem is turned on because of performance reasons. My programming skills aren't good enough to make this possible. I'm working on my language improvement. On PHP and my english skills too :-)

I like the suggested solution of a grouped "style@import" because I found another big problem with IE5 with the "link-import" of D6. I solved this problem by changing the $styles var in page.tpl.php. Now I added the grouping function.

My workaround is placed in the template by changing the $styles in the page.tpl.php calling an own function ( <?php print _mychange_css($styles) ?> instead of <?php print $styles ?> ).

The following snippet (Drupal 6-Version) have to be placed in the used template.php:

<?php
function _mychange_css($oldstyles) {

global
$mycatch_media_all; // globals are needed to handle the preg-results outside the replacement-routine
global $mycatch_media_rest;
$mystyles = $mycatch_media_all = $mycatch_media_rest = ""; //reset of used vars

function _mycatch_css_f ($match) {  // the callback-function for the preg-replacement-routine
global $mycatch_not_rest;
global
$mycatch_media_all;
    if (
$match[1] == "all") { $mycatch_media_all .= '@import "'."$match[2]".'.css";'. "\n"; }
    else {
$mycatch_media_rest .= '<style type="text/css" media="'."$match[1]".'">@import "'."$match[2]".'.css";</style>' . "\n"; }
return
''; // return nothing for replacement
}

$oldstyles = preg_replace_callback('#<link type="text/css" rel="stylesheet" media="(.*?)" href="(.*?)\.css(.*?)/>\n#','_mycatch_css_f', $oldstyles ); // the preg-replacement-routine. "#" seems to be needed for cutting "<" an ">" (found on php.net).
$mystyles .= '<style type="text/css" media="all">' . "\n".$mycatch_media_all.'</style>'."\n".$mycatch_media_rest . $oldstyles;
return
$mystyles;
}
?>

Edit: The global-definition of $mycatch_not_rest was wrong (forgot to change after renaming) and I added . "\n" to "rest".

A Drupal 5 -Version needs a regex-pattern like: '#<style type="text/css" media="(.*?)"\>\@import "(.*?).css";\</style>#' (NOT tested).

At this time there is only one group for the most used media type "all". The "rest" gets a "style@import"-form too (not needed in D5). With elseif it's easy to add more groups. By using an array to collect the search results it would be easy to make a group for each media type. There could be other even unknown media types. Think of tools like CSS-Injector (http://drupal.org/project/css_injector) and additional css-files in .info-file.

I found no information about $query_string which is added to the css-import links between line 1717 an 1739 in the common.inc of d6. Can somebody of you explain this? In my solution I don't use this information. You can use this with "$match[3]".

Edit: In a german forum someone told me that $query_string is for cache-rebuilding in browser.

Yours sincerely
Carsten Logemann

#14

Paratio - May 12, 2008 - 21:58

Here the function (Drupal 6) with grouping all media-types in an array and adding the $query_string:

<?php
function _mychange_css($oldstyles) { // get the content of $styles-var from page.tpl.php

 
global $mycatch_media// globals are needed to handle the search results ...
 
global $myquery_string; // ... outside the preg_replace -routine
 
$mycatch_media = $mystyles = $myquery_string = ""; //reset of used vars

 
function _mycatch_css_f ($mymatch) {  // function for the preg-replacement
   
global $mycatch_media;
    global
$myquery_string;
   
$mycatch_media ["$mymatch[1]"] [] = $mymatch[2];  //add result with media-type as key
   
$myquery_string = $mymatch[3];  // get $query_string
   
return ''; // return nothing for replacement = deleting the <link />-tag in $oldstyles
 
}

 
$oldstyles = preg_replace_callback( // search regular expression in $oldstyles
   
'#<link type="text/css" rel="stylesheet" media="(.*?)" href="(.*?)\.css(.*?)" />\n#',
   
'_mycatch_css_f', // call of function with each preg-result with an array
   
$oldstyles ); // "#" seems to be needed for cutting "<" an ">" found on php.net

 
foreach ($mycatch_media as $mymedia => $mycss_links) // making a grouped <style>@import
 
{
 
$mystyles .= '<style type="text/css" media="'."$mymedia".'">' ."\n"; //group by media-type
   
foreach ($mycss_links as $mycss_link) // get each css-link
   
{
     
$mystyles .= '@import "'."$mycss_link".'.css'."$myquery_string" .'";'."\n";
    }
   
$mystyles .= '</style>' ."\n";
  }
 
$mystyles .= $oldstyles; // add the rest of $styles-var
 
return $mystyles; // return the changed content of $styles to page.tpl.php
}
?>

Edit: inline comment improvement

Successfull tested on my IE 5.

#15

dvessel - June 5, 2008 - 20:33

As Gábor mentions, if there is no issue with style tags grouped by media type, then it's the only solution.

From 5 to 6, the "style" tag was replaced with "link". Anyone recall why it was changed?

#16

hass - June 5, 2008 - 20:42

Well... as i remember - someone said @import was used in past to keep Netscape 4.7.x users outside of a Drupal website and that this is no more the case now ~15 years after Netscape 4.7.x :-). Group by media must keep the order intact or things will break in themes... this could cause more style tags as we expect now and might bring us back to the 30 style tag limit...

#17

dvessel - June 5, 2008 - 20:57

Right, I just found it. I didn't see that as a compelling reason for the change.

The only thing to consider when ordering is what added the style. Core should go in first, then contrib modules, possibly theme engines then themes and maybe sub-themes. The ordering within each group shouldn't be a big deal so at most, we would have 4-5 groups for the same media type.

#18

dvessel - June 5, 2008 - 21:06

btw hass, why did you roll a patch with the media type pushed into the @import? IE no likey. Your original post should do the trick.

#19

hass - June 5, 2008 - 21:14

Not that would break themes. you might end up with X medias per core, modules and themes. And if you must keep the original order intact you will easily exceed the 10 media switches per core, modules, themes. I know some core developers don't care about the order, but CSS order IS VERY important.

#20

dvessel - June 5, 2008 - 21:28

Who uses more than a handful of media types? The most I've encountered is 3-4.

CSS order is important but we shouldn't take it too far. Does Views need to worry about the order set by cck? I don't believe so, each module should only be concerned with the styles it adds itself. Set it's defaults and let the theme worry about the details.

There might be some cases where modules that depend on each other try and alter its style, in that case what's wrong with them working based on specificity instead of cascades? But most times, it shouldn't need to worry about anything but itself or it can get messy.

If there's an alternate method, then what is it? Please, lets keep it simple. :) I found this queue because I ran into this bs 30 style limit. It really needs to be fixed.

#21

hass - June 5, 2008 - 21:54

Well, between modules this overwrites should never happen... and I'm happy that YAML use media=all everywhere and inside the CSS files @media {} what would hopefully not cause anything to break in the order, but there seems only a workaround that reduces the problem, but is no final solution.

Good to have you on this issue... a better chance to get something in :-)

#22

dvessel - June 6, 2008 - 18:00

This is really tricky. Grouping by media types would also mean splitting aggregated files for modules and themes. Something I'd rather not see.

I've been out of it for a while now. Not sure I'll have much weight in this issue but I'm definitely thinking about a fix.

#23

brahms - July 2, 2008 - 16:24

Sorry to say but today I found out that grouped @imports' with media="$media" inside one style tag does not work in IE7 if there are more than 30 grouped CSS files. In my Drupal Site I have now 34 CSS files (after adding another module) inside they style tag for media="all". The first 30 CSS Stylesheets are loaded by IE7 the stylesheets after them are skipped.

So I tried another solution. I replaced the "style" tag with "link" like it is done in Drupal 6.x and this seems to work. I attach a patch file for the official common.inc in Drupal 5.7, please apply this patch from Drupals base directory.

AttachmentSize
common.inc-5.7-css_link.patch 2.01 KB
Testbed results
common.inc-5.7-css_link.patchfailedFailed: Failed to apply patch. Detailed results

#24

hass - July 2, 2008 - 16:41

This will not work in IE6 as i know. Cannot remember for sure, but I think I've tested this some time ago. IE doesn't make any difference between link as style and inline styles.

#25

brahms - July 3, 2008 - 14:05

If this does not work in IE6 I see primary one solution that may work: I will try to group the stylesheets per media into style tags again but this time I use a maximum of 30 stylesheets per style tag. Maybe this will work, I will post a patch if it succeeds.

Another solution could be this one: we make it possible to aggregate CSS stylesheets even if private download method is set. I have to use private download method becausee I am working on an intranet solution and there is a need for role based restrictions on content and uploaded documents.

By the way: if replacing style tags with link tags does not work in IE6 it means that many people will have troubles using Drupal 6.x and IE6 ?

#26

brahms - July 3, 2008 - 17:31

Here is now a patch for Drupal 5.7 (modifying include/common.inc and modules/system/system.module) for grouping a maximum number of CSS stylesheets inside media specific style tags. I call this feature "Combine CSS" and you find the settings for it at the bottom of the "performance" settings form. There it is possible to disable or enable the "combine CSS" feature and to specify the limit of CSS stylesheets that get grouped into one media specific style tag. It is best to set this limit to 30 to minimize the number of style tags. I have just made short tests with IE7 and disabled CSS aggregation, here it seems to work. I will do more testing with a higher number of stylesheet files in the next weeks.

AttachmentSize
common.inc-5.7-css_styles.patch 9.47 KB
Testbed results
common.inc-5.7-css_styles.patchfailedFailed: Failed to apply patch. Detailed results

#27

brahms - July 3, 2008 - 17:33

Here is an example of the grouped style tags for my site, with the conditional comment for IE7 I get 41 CSS stylesheet files grouped inside 8 style tags and this works in IE7 (and FireFox 2). I am using yaml-for-drupal 3.x and an additional yaml layout:

<style type="text/css" media="all">@import "/ak/intratest/sites/all/modules/admin_menu/admin_menu.css";
</style>
<style type="text/css" media="all">@import "/ak/intratest/sites/all/modules/devel/devel.css";
@import "/ak/intratest/modules/aggregator/aggregator.css";
@import "/ak/intratest/modules/book/book.css";
@import "/ak/intratest/modules/node/node.css";
@import "/ak/intratest/modules/poll/poll.css";
@import "/ak/intratest/modules/system/defaults.css";
@import "/ak/intratest/modules/system/system.css";
@import "/ak/intratest/modules/user/user.css";
@import "/ak/intratest/sites/all/modules/cck/content.css";
@import "/ak/intratest/sites/all/modules/date/date.css";
@import "/ak/intratest/sites/all/modules/date/date_popup/themes/white.calendar.css";
@import "/ak/intratest/sites/all/modules/date/date_popup/themes/timeentry.css";
@import "/ak/intratest/sites/all/modules/img_assist/img_assist.css";
@import "/ak/intratest/sites/all/modules/lightbox2/css/lightbox.css";
@import "/ak/intratest/sites/all/modules/tagadelic/tagadelic.css";
@import "/ak/intratest/sites/all/modules/cck/fieldgroup.css";
@import "/ak/intratest/sites/all/themes/yaml/layouts/yaml_akstmk5v3/advforum/advanced_forum-structure.css";
@import "/ak/intratest/sites/all/themes/yaml/layouts/yaml_akstmk5v3/advforum/advanced_forum.css";
@import "/ak/intratest/sites/all/modules/panels/css/panels.css";
@import "/ak/intratest/modules/forum/forum.css";
@import "/ak/intratest/sites/all/modules/nodequeue/nodequeue.css";
@import "/ak/intratest/modules/comment/comment.css";
@import "/ak/intratest/sites/all/themes/yaml/yaml/core/base.css";
@import "/ak/intratest/sites/all/themes/yaml/css/screen/basemod.css";
@import "/ak/intratest/sites/all/themes/yaml/css/screen/basemod_2col_13.css";
@import "/ak/intratest/sites/all/themes/yaml/css/screen/basemod_drupal.css";
@import "/ak/intratest/sites/all/themes/yaml/layouts/yaml_akstmk5v3/css/screen/basemod_akstmk5v3.css";
@import "/ak/intratest/sites/all/themes/yaml/layouts/yaml_akstmk5v3/css/screen/basemod_gfxborder_akstmk5v3.css";
@import "/ak/intratest/sites/all/themes/yaml/css/navigation/nav_shinybuttons.css";
@import "/ak/intratest/sites/all/themes/yaml/layouts/yaml_akstmk5v3/css/navigation/nav_shinybuttonsmod_akstmk5v3.css";
</style>
<style type="text/css" media="all">@import "/ak/intratest/sites/all/themes/yaml/css/navigation/nav_vlist_drupal.css";
@import "/ak/intratest/sites/all/themes/yaml/css/screen/content.css";
@import "/ak/intratest/sites/all/themes/yaml/layouts/yaml_akstmk5v3/css/screen/contentmod_akstmk5v3.css";
@import "/ak/intratest/sites/all/themes/yaml/yaml/core/print_base.css";
@import "/ak/intratest/sites/all/themes/yaml/css/print/print_003.css";
@import "/ak/intratest/sites/all/themes/yaml/css/print/print.css";
</style>
<style type="text/css">#page_margins { width: auto; min-width: 740px; max-width: 95em }</style>
<!--[if lte IE 7]>
<style type="text/css" media="all">@import "/ak/intratest/sites/all/themes/yaml/yaml/core/iehacks.css";</style>
<style type="text/css" media="all">@import "/ak/intratest/sites/all/themes/yaml/css/patches/patch_nav_vlist_drupal.css";</style>
<style type="text/css" media="all">@import "/ak/intratest/sites/all/themes/yaml/css/patches/patch_2col_13.css";</style>
<style type="text/css" media="all">@import "/ak/intratest/sites/all/themes/yaml/css/patches/patch_drupal.css";</style>
<![endif]-->

#28

hass - July 3, 2008 - 18:12

I totally don't understand the sense behind "Max. number of CSS files inside one style tag".

#29

brahms - July 4, 2008 - 00:32

I try to explain in detail:

After I modified the function drupal_get_css() in common.inc like I described in my comment#7 above I thought that everything worked fine. I got one style tag per media type and in the style tag for media="all" I had more than 30 imported stylesheet files and they all seemed to be processed by IE7. What I did not realize at this time was the fact that IE7 still did only process the first 30 imported files, because my site looked correctly formatted. After I added 2 modules with some additional CSS stylesheets I noticed that IE7 did not process all of those imported stylesheets inside the style tag for media="all". The font styles (color and font-family) of the navigation menu suddenly had changed in IE7. By comparing the html source of the web page to the html source of the same page before adding the 2 modules I found out that the affected stylesheet was now number 31 in the import sequence inside the style tag for media="all". Before I added the 2 additional modules the same stylesheet was number 29 in the import sequence inside the style tag for media="all". (As you are the developer of YAML for Drupal: it is a stylesheet I called contentmod_my-yaml-layout.css which overrides styles in yamls content.css). So I think you are mistaken in your comment#5 above where you say "Well, it counts the number of <style></style> and <link type="text/css" ... /> not the number of files added with @import"

So I tried what I describe in my comment#25 above: I still group stylesheets per media type with @import inside <style&gt tags but I limited the number of imports to 30 stylesheets. For the following stylesheet files 31, 32, and so on I use a second <style> tag for the same media type="all". Again I would limit the number of @import sequences to 30 stylesheets, if I had more than 60 stylesheets to import for thia media type (but I haven't, I have exactly 36 CSS stylesheets for this media type). You can see the result in my comment#27 above.

The sense behind "Max. number of CSS files inside one style tag" now is that you can specify this limit for testing purposes. If in my example from comment#27 I would set it to 5, I would get 6 <style> tags for media type="all" each with 5 @import lines inside and 1 more <style> tag for the last of the 36 CSS stylesheets for the media types.

If I set the limit to 30 I reach the optimum setting, because I minimize the number of generated <style> tags using IE7s (obvious) limit for @import lines inside one <style> tag.

If I set the limit to 31 oder any higher number, I will get layout errors in IE again because @import number 31 and above will not get processed.

I considered if I should hard code the setting for "Max. number of CSS files inside one style tag" to the optimum value of 30. I did not because the possibility to specify "Max. number of CSS files inside one style tag" in performance settings page makes it easier to test IEs errors without changing the PHP source.

#30

hass - July 4, 2008 - 07:41

IE does not have issues with loading only a maximum of 30 files! It have issues with more then 30 <style> or <link> tags - it doesn't matter if you add 100 or 1000 files between one style tag <style type="css/text" media="all">... [here you can add for e.g. 1000 @imports without any problems] ...</style>. 1000 @imports are nevertheless untested :-).

We need to fix this without adding new options to the UI what is not required at all.

#31

brahms - July 4, 2008 - 10:26

Are you sure that IE7 does not have issues importing and processing more than 30 files inside one single <style> tag? I had read your comment#5 and I know you have written there and in your new comment#30 that it doesn't matter if there are 100 or 1000 imported files inside one singel <style> tag.

But as I notized in my Development Site, in IE7 the file number 31 (contentmod_myyaml.css) did not get processed because the styles for the navigation menu elements in this file did not override the default yaml styles from a previous processed stylesheet (content.css) anymore. If I deactivated one drupal modul with 2 own CSS files, contentmod_myyaml.css got number 29 inside the <style> tag and got processed again, as I saw in the now correct styling of the navigation menu.

I can only reproduce this, I haven't found an microsft issue describing this effect.

I will test it again and maybe I can install a demonstration site to show you what I mean.

Nevertheless I'd like to thank you for your strong engagement in this issue. I think it is a very important issue which may lead to many side effects. If someone doesn't know about this IE bug he could hardly find the reason why sometimes after adding a module something suddenly does not work anymore or the freshly installed module does not work correct.

#32

brahms - July 4, 2008 - 13:34

Demo Site as answer to comment#30:

I provide a demo site under http://78.47.120.6/demo/intratest/
(maybe hass will be so kind to try the demonstartion, I prepared this site as an answer to his post in comment#30)

On this site anyone can reproduce the bug in Internet Explorer 7 as I descibed it in my comment#23 above.

Here are some instructions to reproduce the bug and test the work-around patch from my comment#26> above. (If someone wants to apply the patch on a site: please don't use the patch from comment#23, because this patch does not work. Use the patch in file common.inc-5.7-css_styles.patch from comment#26> instead!) I included those instructions in the front page of the demo site too.

I would be glad if someone tries to reproduce this bug in Internet Explorer 6 and post the results here.

Please login as user demo with password demo

You see the bug in different font colors and styles of the navigation menu left under following circumstances:

Navigate to Performance, go to the bottom of the page, enable Combine CSS files per media type and set Max. number of CSS files inside one style tag to 0. Then follow this link forum-issue/how-add-images-story to go to the forum issue How to add images to a story. You will notice that the colors of the title of the navigation menu and the color of the forum topic title are blue. They should be green like defined in the CSS stylesheet contentmod_akstmk5v3.css, which you can see in the html source but did not get processed in IE.

Now try a work-around for this bug

Navigate to Performance again, go to the bottom of the page, enable Combine CSS files per media type and set Max. number of CSS files inside one style tag to 30. Then follow this link forum-issue/how-add-images-story to go to the forum issue How to add images to a story again. You will notice that the color of both the title of the navigation menu and the color of the forum topic title are now dark green because now the stylesheet contentmod_akstmk5v3.css got processed!

#33

hass - July 4, 2008 - 15:50

Hm... this is really new to me and makes thinks more worse... :-(. I haven't tested IE7 much regarding this issue, but thought it should work the same way wrong as IE6 and as MS documented this :-(.

#34

brahms - July 4, 2008 - 17:31

hass, thank you for your quick reply. By the way, in the meantime I could test the demo site with IE6 and IE6 shows the same bug as IE7! So I think with the patch we may achieve a usable work around. We can have a maximum of 30 <style> tags over all and inside each of them a maximum of 30 imported CSS files. So we can use a limit of 900 CSS files... This is not really correct because Drupal's drupal_get_css (and my modified version) groups some no_preprocess CSS files in their own <style> tag and there are some more conditional CSS files from YAML. So we may end with a 880 file limit or so, but this should work for everyone.

#35

Damien Tournoud - July 4, 2008 - 19:03
Version:6.1» 7.x-dev

#245268: Notify about, or avoid, IE 31 stylesheet limit was a duplicate.

Bugs should be fixed first in the development version (Drupal 7.x currently), than backported.

#36

brahms - July 5, 2008 - 00:06

Damien, I agree to your comment#35. Here is my patch for the head revision of Drupal, which is Drupal 7. Please apply this patch from Drupal's base directory. This patch modifies the files include/common.inc and modules/system/system.admin.inc. It will add a fieldgroup named "Group CSS files" at the bottom of the performance settings page.

This fieldgroup provides 2 new fields:

Group CSS files per media type enables or disables the grouping of multiple CSS stylesheets (using @import) into one <style> tag per media type.

Limit for the number of CSS files inside each style tag gives the additional possibility to specify the maximum number of CSS files inside each <style> tag.

Best settings are:

Group CSS files per media type: Enabled
Limit for the number of CSS files inside each style tag: 30

AttachmentSize
drupal-head_css-styles.patch 9.57 KB
Testbed results
drupal-head_css-styles.patchfailedFailed: Failed to apply patch. Detailed results

#37

brahms - July 5, 2008 - 20:03

Here are the backported patches for:

Drupal 6 Branch (with cvs tag DRUPAL-6): drupal-6_css-styles.patch
Drupal 6.2 (with cvs tag DRUPAL-6-2): drupal-6-2_css-styles.patch
Drupal 5 Branch (with cvs tag DRUPAL-5): drupal-5_css-styles.patch
Drupal 5.7 (with cvs tag DRUPAL-5-7): drupal-5-7_css-styles.patch

AttachmentSize
drupal-6_css-styles.patch 9.67 KB
drupal-6-2_css-styles.patch 9.67 KB
drupal-5_css-styles.patch 9.45 KB
drupal-5-7_css-styles.patch 9.45 KB
Testbed results
drupal-6_css-styles.patchfailedFailed: Failed to apply patch. Detailed results
drupal-6-2_css-styles.patchfailedFailed: Failed to apply patch. Detailed results
drupal-5_css-styles.patchfailedFailed: Failed to apply patch. Detailed results
drupal-5-7_css-styles.patchfailedFailed: Failed to apply patch. Detailed results

#38

Crell - July 5, 2008 - 20:52

No no no! The use of @import was removed in Drupal 6 very deliberately, because it causes various problems. Let's not bring it back. (See #145218: Use href instead of @import for CSS)

#39

hass - July 5, 2008 - 21:33

@Crell: If we are not able to move back to @import we cannot solve this issue...

#40

Crell - July 6, 2008 - 00:35

@hass: You'll forgive me for not being in favor of making our code lower quality to cater to a dying browser that has already resulted in the waste of hundreds of thousands of hours. If you use @import, you cannot effectively Save-As a page because the imported stylesheets don't come with it. Frankly I've yet to hit the 30 stylesheet mark myself so I've not encountered this problem.

What about instead some sort of setting that forced the compressed CSS to be regenerated for every page request? That way if you get as far as 30 stylesheets AND you're doing your primary development in IE 6 (in which case you're doing it wrong anyway), you can enable the compressor to get down to one file and then get a new stylesheet built each time. It's development anyway so you just eat the recompilation cost.

#41

brahms - July 6, 2008 - 02:32

@Crell: from an idealistic point of view you may be right if you prefer other browsers than IE, I also do prefer Firefox over IE. But if you are developing web sites for the people out there and especially if you try to develop web sites for companies you have to accept that many of them will use IE as browser. For me that is fact number one.

Drupal offers a public and a private download method: with public download method everyone has the possibility to download every file from Drupal's file system path. If you want or need to control access to downloads you have to use the private download method. Therefore you don't have the possibility to compress the stylesheets. For me this is fact number two.

A limit of 30 stylesheets seems enough at first glance. But in Drupal core alone you will find 20 stylesheets (without a theme). If you build a feature rich site you will need to integrate contributed modules and many of them bring additional stylesheets into your pages. It won't take long to recognize that 30 stylesheets are not enough. This is fact number three for me.

And if you use a modular theme like Yaml for Drupal with a lot of stylesheets you soon will get about 50 stylesheets or more that need to be processed.

So the big and serious dilemma for me is: I do need private download method, I do have a customer who uses IE7, I am using a pretty bunch of modules (cck, views, panels, tinymce to name some of them) and I am using this awesome layout framework Yaml for Drupal is.

At the end I have to choose between 2 problems:

  • I apply the patch I provided to make my site work in IE and loose the possibilty to "save as a page"
  • I deliver a site with a broken layout to my customer for the sake of code quality. Maybe he will be pleased by the perfect code quality which gives him the possibility to save the broken layouts as a page. Or he won't get much impressed by the excellent code which sends such ugly pages to his IE and might think that the developer or the system offers only poor quality

Which of these 2 options would you select?

#42

Michelle - July 6, 2008 - 03:18

@Crell - I just stumbled on this issue, but have dealt with this problem on CRO for a while. I can't turn off aggregation on the live site, even if I just want to check something quick in firebug, because then the site is unusable for any IE users that happen by. I don't know enough about this to know what the best solution is, but I wanted to offer another voice saying yes this is a real world problem even for people who don't normally use IE.

Michelle

#43

pegleglax - July 6, 2008 - 04:50

This. Patch. Saved. My. Life! Brahms you're the shiznit!

Will this patch be in 6.3?

#44

hass - July 6, 2008 - 09:03

You'll forgive me for not being in favor of making our code lower quality to cater to a dying browser that has already resulted in the waste of hundreds of thousands of hours.

I don't know what your site is doing but this is more then unrealistic today and for the next ~2 years. I'm not a fan of IE and i hope it rest in peace, but it will *not* for the next 5 years and therefore we must also take a look to IE6 and IE7 as long as this versions going under 2% usage what may never be happen.

If you use @import, you cannot effectively Save-As a page because the imported stylesheets don't come with it.

I truly understand save-as this issue... but it is *much more* important to get a browser working that has a market share of ~45% (IE6) and ~35% IE7. However we all know how buggy this damn browsers are or not. I don't think 80% usage are a dying browser we don't need to support...

What about instead some sort of setting that forced the compressed CSS to be regenerated for every page request? That way if you get as far as 30 stylesheets AND you're doing your primary development in IE 6 (in which case you're doing it wrong anyway), you can enable the compressor to get down to one file and then get a new stylesheet built each time. It's development anyway so you just eat the recompilation cost.

This will not work for people with private files folder and speed will be awful...

Aside, I'm not developing "only for" or "primary with" IE... but the yaml theme provides compatibility to *all* major browsers we cannot ignore and solves uncountable issues that you do not need to care about as webdeveloper if you use the YAML CSS Framework. See http://www.yaml.de/en/documentation/introduction/browser-support.html about the supported browsers. There is no other theme out there that works reliable with so many and all this browsers. You can do this yourself different and develop only for standard compliant browsers what we are doing too, but we will not ignore IE and have a *fallback* for this browser we really need to test.

Have you tested IE8 with RTL? *argl* I don't know what this guys in Redmond are working on, but this is more broken then IE6 ever was.

#45

hass - July 6, 2008 - 09:23

Will this patch be in 6.3?

@pegleglax: I expect this is not going to get committed as is - today and we cannot say when 6.3 goes out.

@brahms: your patch cannot get in as is, while you are changing the UI that is additional not required. This is something we don't need as we know the hard technical limits (30 files). The settings are fine for someone how like to reproduce and test this IE bugs themself, but we cannot stress people to configure this as they do not understand the issue behind (newbies, non developers, etc).

#46

hass - July 6, 2008 - 09:37

@brahms: your patch produces a WSOD. See your site on performance link.

#47

brahms - July 6, 2008 - 11:44

your patch cannot get in as is, while you are changing the UI that is additional not required. This is something we don't need as we know the hard technical limits (30 files).

@hass: I agree with you. Before I'm going to modify the patch please tell me your opinion to following suggestions:

  • I remove the parameter Max. number of CSS files inside one style tag from the performance settings page. Instead of the UI setting I hard code the limit to 30 files. As it is now this limit is only relevant, if the other remaining UI parameter Combine CSS files per media type has been set to Enabled
  • I keep the other parameter Combine CSS files per media type in the performance setting page but rename it to Group CSS files for simplicity. If the parameter Group CSS files has been left or set to it's default state Disabled I'll change the code so that it will use <link> tags just like it is doing now in Drupal head revision and Drupal 6.x. I hope this way the patch is going to be accepted as it leaves the behaviour of the function drupal_get_css untouched if grouping of CSS files is disabled.

#48

brahms - July 6, 2008 - 11:48

your patch produces a WSOD. See your site on performance link

@hass: hm, I could not reproduce the WSOD with IE7 or Firefox. I also did not find any hint for the problem in apache logfiles or Drupal's watchdog logs. What browser did you use? Do you still get the WSOD?

#49

hass - July 6, 2008 - 14:28

IE7. "Max. number of CSS files inside one style tag" = 0 and the "Combine CSS files per media type" have been changed from enabled to disabled. Afterwards I've got a WSOD. After some clicks, wait and another click the performance page showed again.

Don't change the UI please. I'm not yet sure what your patch does in detail as I haven't found any time to test. If the combine CSS files change the order of CSS files you must remove this stuff, too. I'm not sure what this radio box change without taking a look to the code.

#50

brahms - July 6, 2008 - 16:36

IE7. "Max. number of CSS files inside one style tag" = 0 and the "Combine CSS files per media type" have been changed from enabled to disabled. Afterwards I've got a WSOD. After some clicks, wait and another click the performance page showed again.

@hass: I am not sure but this WSOD may be a side effect of the 30 stylesheet problem. With these settings some of the stylesheet are not processed and I get some kind of javascript error (variable 'Drupal' is undefined' in IE7).

But nevertheless: I will prepare a fresh demo site for Drupal 7 head revision. I won't install any contributed module there. To get the needed number of CSS files I will slightly modify Garland theme so that it loads some dummy stylesheets. In stylesheet number 31 I will place a significant style change (I think changing the text color of all headings to red) so it would be easy to see, if this stylesheet gets processed or not.

Don't change the UI please.

@hass: I'm not shure what you mean here? I intend to add only one new parameter Group CSS files to performance setting page so that everyone has the possibility to switch it on or off.

If the combine CSS files change the order of CSS files you must remove this stuff, too.

My code is written to leave the order of CSS files untouched. If he does not he would be buggy! And if you disable parameter Group CSS files or if you use public download method and enable Optimize CSS files everything should work as it did before applying my patch.

But please wait for the Drupal 7 demo site and the new cleaned up patch I announced and descibed in my comment#47. I hope it will be ready in a few hours.

#51

brahms - July 6, 2008 - 19:07

New patch and Drupal 7 demo site:

I attach a new and hopefully final patch file for Drupal 7 (head revision from today). This patch modifies the files include/common.inc and modules/system/system.admin.inc. To apply the patch please gunzip this file in your Drupal 7 base directory and start the patch from there with:

patch -p0 < drupal-head_css-styles.patch1

It will add the new parameter Group CSS files in the existing fieldgroup Bandwith optimization in the performance settings page:

If this parameter is disabled or if the existing parameter Optimize CSS files is enabled everything keeps working like it did before applying the patch.

Only if Group CSS files is enabled and Optimize CSS files is disabled all CSS files are grouped using the CSS @import command up to a limit of 30 files per media type into single <style> tags. The order of file processing is itended to be unchanged. This will provide a work around for IE's 30 stylesheets bug.

The new demo site where everone can reproduce the bug and see the patch working is: http://78.47.120.6/demo/drupal7head/

I really hope that this patch will be accepted for core as long as it will prove to be free of bugs. It does not change Drupal 7 behaviour if the new option Group CSS files is disabled.

AttachmentSize
drupal-head_css-styles.patch1_.gz 2.02 KB

#52

stripped your speech - July 6, 2008 - 19:12

Isn't this because IE6 doesn't recognize more than 30 CSS links? Turn on CSS compression.

But really, this seems to be a problem deeper in the Drupal core / community. Every module has its own CSS it seems like. There needs to be a more uniform approach to module layout to where its not so absurd with the CSS includes.

#53

brahms - July 6, 2008 - 19:27

Isn't this because IE6 doesn't recognize more than 30 CSS links? Turn on CSS compression.

@gh0st25: yes this is the reason (btw; IE7 shows the same bug). But you can't turn on CSS compression if you set Drupal's file download method to private...

#54

Jeff Burnz - July 6, 2008 - 21:53

Subscribing

#55

hass - July 6, 2008 - 21:58

@brahms: only as side info... there is a private/public handing patch on the road... #166759: Public/Private File Handling that was "planed" for D6 and hopefully go in for D7, but nevertheless will not solve our issue here.

#56

stripped your speech - July 6, 2008 - 22:55

Anything more than 5 CSS includes is absurd, there should be a more concerted effort to addressing that. Personally, I refuse to use @import. IE6 works fine with link rel=''.

People on slower internet connections are getting roadblocked by all those includes to download, not to mention images and site graphics.

Can't there be an algorithm where you put your CSS files into a directory, and in the admin say which files should be included in order, then, Drupal takes all of these files and creates one sitewide CSS file with all the definitions in it on demand or cron? If this is CSS Compression I apologize, but I am a stickler on the amount of includes for a website. This doesn't seem like a difficult issue, the issue is moreso on the side of the necessity (or perceived necessity) to having 25-50 CSS includes.

#57

brahms - July 7, 2008 - 00:23
Status:needs work» needs review

@hass: thank you for your info about issue #166759. In the last few days I thought by myself that there could be another solution if Drupal offered a way to compress CSS files even with private download method enabled. But as you say in your last post: it will not solve our issue here at the moment.

What I don't know is who is allowed or responsible for changing the status of an issue. Because I am thinking, that my patch file from comment#51 above offers an acceptable solution for the moment (until someone finds a better one) I change the status of this issue to patch (code needs review). I really hope that this patch or another solution will find it's way into core and will be backported down to Drupal 6.x and 5.x. Life would be much easier without core patches (and I always have to apply a second one bringing the missing proxy configuration into Drupal)...

#58

Crell - July 7, 2008 - 15:02

@gh0st25: Drupal already does that. It's called the CSS compressor. It compresses the CSS files down to one file per media target. The issue discussed here is that you can't really use it during development (as it doesn't pick up changes you make to your CSS files) and it doesn't work with private file handling.

My recommendation is 1) Have some flag (set in $conf?) to always regenerate the compressed CSS on every page load; 2) Honestly, I've considered Drupal's private file handling to be broken for a long time to start with. I know, not a great solution, but I don't use it as it hasn't really been useful since at least Drupal 5. (It always broke the CSS compressor as well as Garland's color picker.)

#59

catch - July 8, 2008 - 15:29
Status:needs review» needs work

The aggregated css file already has a query string appended to force browser cache refreshes, which is incremented by http://api.drupal.org/api/function/_drupal_flush_css_js/7

So ensure the file is rebuilt and that function called every page load, and you can develop with css aggregation switched on.

#51 isn't a proper patch file, please upload it as plain text, not an archive.

#60

brahms - July 8, 2008 - 16:59

@catch: I have attached the plain patch file with my patch for Drupal 7 head revision described in comment#51 renamed to the standardized naming convention for patch files.

AttachmentSize
drupal-228818-60.patch 8.61 KB
Testbed results
drupal-228818-60.patchfailedFailed: Failed to apply patch. Detailed results

#61

brahms - July 8, 2008 - 17:01
Status:needs work» needs review

@catch again: sorry, in comment#60 I had forgotten the change the issue's state to patch (code needs review) again.

#62

pegleglax - July 10, 2008 - 03:59

FOR DRUPAL 6.3

Guys, I just upgraded a site I'm working on from 6.2 to 6.3. The patch for 6.2 (found in comment #37) also applies for 6.3. Here is the link in case you don't wanna scroll up :p

http://drupal.org/files/issues/drupal-6-2_css-styles.patch

Btw, I'm not the best patcher in the world so I manually applied the patch.

Once again, THANK YOU brahms for this WONDERFUL patch! I personally hope it or something with similar functionality makes it into D7!

#63

brahms - July 10, 2008 - 18:02

@pegleglax: thank you for the flowers :-)

In the meantime I modified the first patch from comment#37. This new patch provided using standardized naming convention in comment#60 above and described in comment#51 does not change Drupal's behaviour if the one remaining option "Group CSS" is disabled. (The patch from comment#37 still works, but the last one is IMHO better and simpler). And as I needed this patch for myself in some Drupal 5.x und 6.x sites which I updated to the new releases 5.8 and 6.3 today I provide those backports of the patch here (the names don't follow the standardized naming convention for official patches):

AttachmentSize
drupal-5.8_css_styles.patch 8.46 KB
drupal-6.3_css_styles.patch 8.34 KB
Testbed results
drupal-5.8_css_styles.patchfailedFailed: Failed to apply patch. Detailed results
drupal-6.3_css_styles.patchfailedFailed: Failed to apply patch. Detailed results

#64

pegleglax - July 10, 2008 - 18:04

ahh... the beauty of open source!

#65

jfxberns - September 17, 2008 - 20:07
Version:7.x-dev» 5.7
Category:bug report» support request
Priority:critical» normal

I tried to apply patch drupal-5-7_css-styles.patch from response #37 to a Drupal 5.7 install. I confirmed the files to be patched were 5.7 files and they had not been previously modified in any way.

I used the command:

$ patch -p0 < ../patches/drupal-5-7_css-styles.patch

The output is:

(Stripping trailing CRs from patch.)
patching file includes/common.inc
patching file modules/system/system.module
Hunk #1 FAILED at 704.
1 out of 1 hunk FAILED -- saving rejects to file modules/system/system.module.rej

Any idea why the patch is failing?

#66

hass - September 17, 2008 - 20:18
Version:5.7» 7.x-dev
Category:support request» bug report
Priority:normal» critical

Please don't high jack this case!

#67

brahms - September 17, 2008 - 20:51

I don't know why, but my uploaded file drupal-5-7_css-styles.patch in comment#37 has the wrong line endings (CR LF instead of LF). I attach the same file but this time with LFs as line endings, this works for me as I just tested with a fresh Drupal 5.7 download.

AttachmentSize
drupal-5-7_css-styles.patch 9.28 KB
Testbed results
drupal-5-7_css-styles.patchfailedFailed: Failed to apply patch. Detailed results

#68

System Message - September 17, 2008 - 20:52
Status:needs review» needs work

Patch failed to apply. More information can be found at http://testing.drupal.org/node/14504. If you need help with creating patches please look at http://drupal.org/patch/create

#69

hass - September 18, 2008 - 06:44

Have someone already tested if attaching media type to @import (like #10) works with IE8B2 and if this 30 files limit is now gone? Nevertheless it wouldn't today - I'd like to know... :-)

#70

thelocaltourist - October 27, 2008 - 12:43

I added the patches in #63 and it didn't work. Also, I'm using Drupal 6.6 and there's no CSS Aggregation option in performance.

#71

brahms - October 27, 2008 - 13:01

@thelocaltourist

If you don't see the group of 2 radiobuttons below the label "Group CSS files:" in the fieldset "Bandwidth optimizations" of the performance settings page then the patch (for Drupal 6.3) failed to apply. Did you see any error messages from the patch command?

#72

thelocaltourist - October 27, 2008 - 15:40

No, no error messages.

(thanks for the quick response!)

#73

momendo - October 31, 2008 - 01:59

sub

#74

Garnerin - November 4, 2008 - 18:44

subscribing

#75

Owen Barton - November 7, 2008 - 01:05
Priority:critical» normal

This is not a critical issue AFAICS - there is no practical benefit to sites over aggregating CSS files and a bunch more code (that could quite happily live in a contrib module). For sites with private files enabled I think the solution is to fix that code so we have the ability to add public files at the same time, rather than do a big work around.

If convenience while developing a theme is the issue, simply add this to your template.php:

<?php
if (module_exists('devel')) { // Or some other way of checking you are really on your sandbox
 
file_scan_directory(file_create_path('css'), '.*', array('.', '..', 'CVS'), 'file_delete', TRUE);
}
?>

#76

c960657 - November 29, 2008 - 12:52

Enabling the CSS preprocessor when using private files is being worked on in #146611: Allow JS/CSS aggregation in private mode. There is a patch waiting for review.

#77

hass - November 29, 2008 - 13:18

#78

joachim - January 12, 2009 - 13:54
Priority:normal» critical

@Grugnog: on many sites it's not an option to turn on CSS aggregation. And like Michelle says, you always need to turn it off sometimes to fix theme issues, and that will break the site for IE6 and 7 users.

+1 and also can we change this back to critical? It's an IE bug but it breaks sites and it's a pig to track down too.

#79

pegleglax - January 14, 2009 - 06:12

Amen. +1

#80

tombigel - January 15, 2009 - 12:37

What about taking a mixed approach?

Core modules CSS files are not something that 99.9% of the users and developers ever change (and most developers don't use IE anyway), so why not add an option in "Site configuration -> Performance" to compress only the core CSS files, thus saving 10 - 20 "link" entries, and keeping our theme/third party modules CSS files intact while developing?

We can take it further and separate between Core CSS files, 3rd Party Modules CSS files and Theme CSS files in The "Compress" dialog, maybe using Checkboxes, and let the user choose to compress only the files in the part that he is not currently developing.

This will give us a bit more breathing space in finding a better way to deal with this problem, and you can deal with edge cases like the "private download" thing with approaches like the ones suggested in this thread without worrying about breaking Drupal for the majority of the users.

BTW -
Another approach that was used in RTL sites on Drupal 5 and below was to load only the RTL CSS files, but in order to keep it incremental add an @import for the original CSS at the begining of the file.
I hate using @import, and I didn't like this technique even when it was the only way, but it will save another bunch of "link" tags.

#81

Michelle - January 15, 2009 - 14:10

Oh, I really like #80. That sounds like a great solution if someone is willing to code it.

Michelle

#82

Crell - January 15, 2009 - 16:03

There is no more distinction between "core" and "module" CSS anymore, as of Drupal 5. All CSS is coming from a module or a theme, although some modules are in core and some are not. That's not something we can easily detect (although there have been changes in the CSS system for D7 already, so I'm not sure how those affect it). That said, allowing module and theme CSS to be compressed separately makes a lot of sense, since it's rare you'll be actively modifying both at the same time.

#83

hass - January 15, 2009 - 17:18

But we shouldn't forget that such a workaround would not fix the problem for users with private download method. I would suggest to solve this issue in general.

#84

tombigel - January 15, 2009 - 18:32

@hass:
You're right, but right now this affects everybody, especially the RTL developers community.
From where I stand (I stand on the platform that says "NOT A PHP DEVELOPER":-)) separating the module compression looks quite easy compared to the solutions discussed here.
So I think it will be a good practice to first solve the problem for the general case, and then concentrate on the exceptions.

BTW -
Can't we use the "package" string on each module's ".info" file to tell if it's a core module or not? does anyone but Drupal itself add modules to "Core-Required" and "Core-optional"?

#85

Crell - January 15, 2009 - 22:23

Only if we actually knew what module added the JS. We don't. We only know that it's added to the "from module stuff" or "from theme stuff" arrays. (Again, unless that's changed drastically in D7 already.)

#86

tombigel - January 19, 2009 - 16:24

@hess:
I take back what I said.

I just got stuck with a site that has problems with CSS compress, and it was really frustrating (Especially with the large amount of CSS files Tendu's new version takes when running with RTL on IE6: 1 style.css + 1 ie.css + 1 ie6.css for parent theme, same 3 for the sub-theme, plus RTL version of each... 12 altogether).

Anyway, I guess you are right, and with no other solution we are reduced to using @import.
Though it would be nice to see it as an option and not a must (is there a way to sniff IE before building the page?).

btw - do you know if this technique is compatible with the module "conditional stylesheets"? Zen is using it and I just committed a patch for RTL support and implemented it in Tendu.
It will be a shame to lose compatibility with it.

#87

hass - January 19, 2009 - 17:25

Sorry, you have issues if the files are compressed? Please validate your CSS files first... they seems to have bugs. You need to figure out what's going wrong, but it shouldn't be the compressor. Only 12 Files is no problem... the problems starts if you have more then 30 link/style tags of typically UNCOMPRESSED CSS files in the HEAD section of your HTML code. Your issue shouldn't be this issue we are discussing here... I hope I haven't misunderstood something.

#88

tombigel - January 19, 2009 - 17:49

@hass, you totally missed my point.
I have no problem with compression (Nor my CSS files), there are some permissions issues on the site I was working on, so I couldn't compress files.
This sub-theme of Tendu loads 12 CSS files with RTL on IE6 only for the theme, not including core and modules.
So my point was that I got to the 30 files limit pretty fast, and now I get the need to solve this problem for all cases (#83).

#89

hass - January 19, 2009 - 19:08

How about fixing the permission problem? :-)

#90

natrio - February 12, 2009 - 04:42

Any progress on the fix?

subscribing...

#91

decibel.places - February 14, 2009 - 22:16

I had broken css in IE using the QuickTabs module 6x-2x-dev in D6.9

Fortunately I eventually found the issue on that project about Optimizing CSS in Performance settings, which resolved the issue

http://drupal.org/node/358114#comment-1258343

However, as it is a production site in dev, it is annoying.

I do most of my dev with Firefox, so I can safely turn the css performance setting off (why does FFox work and not IE?)

#92

jonga - February 15, 2009 - 09:30

+

#93

JohnAlbin - February 20, 2009 - 19:17

/me contemplates… (and subscribes)

#94

s.Daniel - February 20, 2009 - 20:10

Having fun with IE6 debugging atm looking for a quick fix.
How about staying with the current solution and having the option to @import all css files in a contrib module/theme?

Zen issue: http://drupal.org/node/378508

#95

Reg - February 23, 2009 - 09:16

I just came across this issue myself after more hours than I care to admit tracking down what was going wrong.

When you are developing you typically only look at the last 5 or so CSS files but I see the need where you just might need to look at a core file or some other file that's not near the end. Perhaps we could select the files not to compress like selecting printer pages... compress all CSS files except 1,15,17-20, 30-. I see no issue with media types since virtually all files are "all" anyway and it's just the last couple files that might be screen, print or esoteric.

So in Development mode the pages are selected to not be compressed but when aggregate is on normal Drupal behavior applies.

I guess with this method you could theoretically have multiple aggregate files if you want to strictly maintain order but Drupal probably doesn't cater to that so you would probably aggregate all non-compressed files and then put the uncompressed files after it in their relative order.

I see no problem with @import for development if it solves this problem, development is development - production is production, no need to be a purist while developing, it takes enough effort to get a significant site into production, let's not make it harder than it needs to be just for the sake of ideals.

#96

Apfel007 - March 24, 2009 - 09:04
Version:7.x-dev» 6.9
Category:bug report» task

Hi, I steped in the same issu.... :-(
Hope someone can give me a hint - I'm using D 6.9 and private download.
Is there a working solution to solve the 30 ccs files problem, without patching corefiles...

Cheers

#97

wretched sinner... - March 24, 2009 - 10:27
Version:6.9» 7.x-dev
Category:task» bug report

Please don't change statuses. The fix will be applied to Drupal 7 first, and then (maybe) backported to Drupal 6.

#98

Apfel007 - March 24, 2009 - 11:21

sorry

#99

stripped your speech - March 24, 2009 - 12:58

"Sorry, you have issues if the files are compressed? Please validate your CSS files first... they seems to have bugs. You need to figure out what's going wrong, but it shouldn't be the compressor."

He's correct. If you turn on CSS compression in Performance, the backgrounds tend to disappear. You need to add a ./ in front of the image path for the background: declaration if you are experiencing problems. This fixed it for me.

Also, why not just use link tag instead of @import? And, why tease with a backport to Drupal 6? You should always support the most widely used version not just bleeding edge. Not all people will be able to swing into Drupal 7 right away.

#100

catch - March 24, 2009 - 15:23

gh0st25, Drupal 6 is supported, Drupal 7 is in active development. We fix bugs in the development version first because this avoids regressions - it's much easier to backport a bug fix than forward port it. Additionally Drupal 7 has automated testing - which means we can ensure bugs are fixed properly with a higher rate of success with less reliance on manual testing - so it's less work to do things that way too.

#101

antoniodemarco - April 14, 2009 - 13:16

Hi all,

I've adopted a solution that doesn't require any core patch, it just uses the hook_preprocess_page() in my template.php file, here it is:

function YOUR_THEME_preprocess_page(&$vars) {
 
  /**
   * Slove 30 CSS files limit in Internet Explorer
   */
  $preprocess_css = variable_get('preprocess_css', 0);
  if (!$preprocess_css) {
    $styles = '';
    foreach ($vars['css'] as $media => $types) {
      $import = '';
      $counter = 0;
      foreach ($types as $files) {
        foreach ($files as $css => $preprocess) {
          $import .= '@import "'. base_path() . $css .'";'."\n";
          $counter++;
          if ($counter == 15) {
            $styles .= "\n".'<style type="text/css" media="'. $media .'">'."\n". $import .'</style>';
            $import = '';
            $counter = 0;
          }
        }
      }
      if ($import) {
        $styles .= "\n".'<style type="text/css" media="'. $media .'">'."\n". $import .'</style>';
      }
    }
    if ($styles) {
      $vars['styles'] = $styles;
    }
  }
}

It's tested on Drupal 6.10 and works well with Drupal CSS compression. Any feedback it's appreciated.

#102

dvessel - April 14, 2009 - 19:13

I just ran into this article on the performance implication of using the @import declaration. Considering this is an IE only bug and would only effect it when developing for most sites. I would hate to see @import used.

The only other option I can see ATM is to give the option to only aggregate module styles freeing the theme to be inspected and worked on.

#103

antoniodemarco - April 15, 2009 - 10:26

Hi all,

as a result of this conversation I've contributed a module that changes the link tag in a combination of style tag and imports. here the project page:

http://drupal.org/project/unlimited_css

It's enough to enable the module to solve the IE problem. Any feedback is appreciated.

#104

antoniodemarco - April 15, 2009 - 10:30

In reply to #102: The code I've posted changes all the links in imports so, as stated in the article you've linked, they should be loaded at the same time. Do you see any other issues?

BTW, aggregating the core CSS files it's indeed a good idea.

#105

hass - April 15, 2009 - 10:33

This module may not work if a theme like YAML, Zen etc. alters the css array and regenerate + alters the styles output... (untested)

#106

antoniodemarco - April 15, 2009 - 12:26

Good point. Could you please open an issue in the project page about this potential problem:

http://drupal.org/project/issues/unlimited_css?status=All

I'll test it as soon as possible, thank you for your help!

#107

antoniodemarco - April 15, 2009 - 12:38

Hi,

I've successfully tested on Zen as from #434298: Test using Zen theme. I suggest to move the discussion about this module to its own issue page: http://drupal.org/project/issues/unlimited_css.

Thank you for the support!

#108

hass - April 15, 2009 - 15:08

I'm not sure how much or if and when Zen alters the CSS array... long time I haven't taken a look into the code, but the YAML theme (http://www.yaml-fuer-drupal.de/de/download) does - 100% for sure. I'd like to get core fixed...

#109

decibel.places - April 17, 2009 - 16:51

Thanks for the great work on the CSS issues! http://drupal.org/project/unlimited_css

I was having problems with the Four Seasons theme not displaying pages in admin (in Firefox):

http://drupal.org/node/285147
http://drupal.org/node/297192

I had to change admin theme and use themekey for some public pages too to use a different theme, and use Garland for the embedded Gallery2

I installed the IE Unlimited CSS Loader which also seems to resolve the display issues in Firefox and other browsers

I think the CSS loader module can also help with style issues for

QuickTabs http://drupal.org/node/346180 http://drupal.org/node/358114

and

MySite http://drupal.org/node/253522

#110

hass - April 17, 2009 - 17:43

Sounds like you found tons of duplicates...

#111

decibel.places - April 17, 2009 - 18:37

@110

no, not duplicates, these are similar issues I have reported, most involving problems with styles in IE (and some other browsers)

I think the IE Unlimited CSS Loader module fixes them, even in other browsers

thanks to antoniodemarco for taking the time to post the module to these other queues

#112

antoniodemarco - April 17, 2009 - 18:38

I'm happy to know it is also useful to solve other issues, thanks for reporting!

#113

tanc - April 20, 2009 - 08:05

F*#king M$cros#ft m*ther f@c*ers!!! Sorry... I can't believe I just wasted so long trying to work out what was going on. Microsoft I hate you!!!!!

On a more positive note, thanks antoniodemarco for making that module.

Also confirmed that a zen subtheme works with the module. But it has to be latest release of zen to avoid some preprocess functions they had going on.

#114

JohnAlbin - June 12, 2009 - 13:26

As much as I love to see new contributors getting excited about Drupal and making new modules (yay, Antonio!), I was disappointed to see that the unlimited_css module ignored the performance implications of @import pointed out in #102 and the partial-aggregation method from #80 that everyone seems to think is a good solution.

Since the idea in #80 is a new feature, it will never get back-ported to D6, so I’ve started to implement that solution for D6 as the IE CSS Optimizer module. Further discussion of that module should be over in that project's issue queue.

Now for D7, it seems pretty apparent to me that since most Drupal users won’t be developing CSS, that the default for the “Optimize CSS” performance option should be “enabled”. That would neatly solve this problem for nearly everybody. Of course, it would be critical that we also solve #166759: Public/Private File Handling

Now that easy fix still leaves CSS developers in the cold. Now we can leave that solution to contrib (which I think would be a bad idea for D7), or we can expand the “Optimize CSS” option to include these 2 new options:

#115

Jeff Burnz - June 12, 2009 - 13:54

Well this is brilliant, and very much what we need. I agree that the unlimited_css module is not really the method we want to move forward with. Its been a good stop gap solution for D6 while developing themes/modules but for live sites its not the best option.

This certainly goes a long way towards solving the problems, and combined with style stripper all bases should be covered.

Not sure about the labels, initially I got confused, which is easy I know... perhaps

- Optimize module stylesheets
- Optimize Theme stylesheets

Small details in any case, otherwise awesome.

#116

JohnAlbin - June 12, 2009 - 18:33

- Optimize module stylesheets
- Optimize Theme stylesheets

That's what I had originally. But if I'm a module developer and I need to work on my CSS, which option do I choose? Optimize module stylesheets? Why would I want to pick "Optimize theme stylesheets"?

But, I agree that even my wording in #114 needs improving. I wish we could have a descriptive help text below each radio button. Wait… did that help pop-up patch ever get in? Doesn't the admin/build/modules page have some now? Hmm…

Maybe… “Optimize stylesheets, but allow module CSS development”?

#117

Crell - June 12, 2009 - 19:08

"Compress [module|theme] CSS only" ?

Perhaps we should close this issue and continue in the optimizer module queue.

#118

hass - June 12, 2009 - 19:59

@John: The style sheet limit is 30 - not 31, see http://support.microsoft.com/kb/262161/en-us.

#119

s.Daniel - June 14, 2009 - 11:41

Would it be technically possible to have a field "exclude css files from compression[xyz.css,... ]" to manually insert filenames one is working on?

#120

DamienMcKenna - June 26, 2009 - 17:36

This works great in Zen v6.x-2.x-dev, thanks! All themes should use this code.

#121

Crell - June 26, 2009 - 18:37
Status:needs work» fixed

Since we now have a working solution in contrib, let's call this fixed and continue discussion over in the IE CSS Optimizer issue queue.

#122

hass - June 26, 2009 - 23:59
Status:fixed» needs work

I think we need a solution in core as core is still broken. Only having a solution for a core bug in contrib by altering core doesn't solve the issue.

There are over 152,128 Drupal D6 installations out there compared to ~20 css optimizer installs that should mostly be dev machines...

#123

decibel.places - June 27, 2009 - 12:24

core is not "broken" - a default install or one with only a few contrib modules will be fine

the optimization is properly a contrib module for "special" circumstances

it would not hurt to include it in core, but it's not fair to say core is "broken"

#124

hass - June 27, 2009 - 15:35

For sure - core is broken! Here is the list of the CSS files in a default D6 installation with all modules enabled and RTL theme:

\misc\print-rtl.css
\misc\print.css
\misc\farbtastic\farbtastic.css
\modules\aggregator\aggregator-rtl.css
\modules\aggregator\aggregator.css
\modules\block\block.css
\modules\book\book-rtl.css
\modules\book\book.css
\modules\color\color-rtl.css
\modules\color\color.css
\modules\comment\comment-rtl.css
\modules\comment\comment.css
\modules\dblog\dblog-rtl.css
\modules\dblog\dblog.css
\modules\forum\forum-rtl.css
\modules\forum\forum.css
\modules\help\help-rtl.css
\modules\help\help.css
\modules\locale\locale.css
\modules\node\node-rtl.css
\modules\node\node.css
\modules\openid\openid.css
\modules\poll\poll-rtl.css
\modules\poll\poll.css
\modules\profile\profile.css
\modules\search\search-rtl.css
\modules\search\search.css
\modules\system\admin-rtl.css
\modules\system\admin.css
\modules\system\defaults-rtl.css
\modules\system\defaults.css
\modules\system\maintenance.css
\modules\system\system-menus-rtl.css
\modules\system\system-menus.css
\modules\system\system-rtl.css
\modules\system\system.css
\modules\taxonomy\taxonomy.css
\modules\tracker\tracker.css
\modules\update\update-rtl.css
\modules\update\update.css
\modules\user\user-rtl.css
\modules\user\user.css

\themes\bluemarine\style-rtl.css
\themes\bluemarine\style.css
\themes\chameleon\common-rtl.css
\themes\chameleon\common.css
\themes\chameleon\style-rtl.css
\themes\chameleon\style.css
\themes\chameleon\marvin\style-rtl.css
\themes\chameleon\marvin\style.css
\themes\garland\fix-ie-rtl.css
\themes\garland\fix-ie.css
\themes\garland\print.css
\themes\garland\style-rtl.css
\themes\garland\style.css
\themes\garland\color\preview.css
\themes\garland\minnelli\minnelli.css
\themes\pushbutton\style-rtl.css
\themes\pushbutton\style.css

Sum up - 42 module files + 2-5 theme files - exceeding the maximum number of 30 files... you may subtract ~3 files as they are not always added... but overall - tooo many files.

#125

Michelle - June 27, 2009 - 15:47

Core isn't broken... It's IE that's broken. The question is whether to adjust core to adapt to IE's brokenness.

Michelle

#126

hass - June 27, 2009 - 18:49

This is really nitpicking. We have a web based CMS and therefore we need to implement it how the most browsers are working. IE has a market share of more than 70% in the world. If we like it or not - we need to keep this always in mind and develop for IE, too. It's not the intention of this case to discuss browser wars.

#127

Michelle - June 27, 2009 - 18:51

I'm not getting into browsers wars. Just clarfying that IE having a limit on stylesheets does not mean core is broken. If folks want to put time into accomodating IE in core, that's just peachy. But I don't consider IE's limitations to be a bug in core.

Anyway, I'm done with this. I'm not actually working on this issue so bowing out.

Michelle

#128

hass - June 28, 2009 - 00:08

All core themes are shown broken if I use IE with all core modules enabled. Therefore I say core is broken as the theme isn't showing the pages as intended. We always need to care about browsers and their abilities.

#129

decibel.places - June 28, 2009 - 01:28

@hass

core is not broken - IE is broken

the optimizer module fixes the issue

please contribute further energy to adding it to core if you feel strongly it should be there

#130

tombigel - June 29, 2009 - 07:35

Come on people! Are you really arguing about IE?

Common Logic:
IE is broken -> IE dominates the web -> Drupal lives on the web -> Drupal depends on IE -> Drupal is broken if it does not support IE.

All good now?

@hass:
I still think that a better sustainable solution for the amount of CSS files should be found.

As I wrote earlier in this thread, I believe that the CSS files should be devided to "groups" -
Core CSS files, 3rd party Themes CSS files, 3rd party Modules CSS files.
The "Compress CSS files" feature should have a way to know which is which and the developer should have the option to choose which one to compress.

Besides that, 42 CSS files for a basic RTL installation?
I don't understand why "core" can't have a unified CSS file, or at least the option to have one.
Most themes override a great amount of these CSS properties anyway, and many of these properties can be annoying as hell (".list-item" defaults for example)

#131

JohnAlbin - June 29, 2009 - 07:49
Status:needs work» needs review

IE is broken in a way that core is affected out-of-the-box for RTL sites. So core needs a work around for IE’s bug. Otherwise, core will appear to be broken to a casual user.

Imagine for a moment that #166759: Public/Private File Handling is fixed and that a site can have both public and private files in a single Drupal install. Then, given IE's bug, it seems pretty apparent that the easy fix for this problem is simply to enable CSS aggregation by default.

The question then becomes: why would we ever want to turn off CSS aggregation? o_O

The only answer I can come up with is for development purposes. If you are examining/developing the CSS on a site using a browser's handy css-inspection-related tools, having the CSS appear to come from a different file (the aggregated file) then where it actually comes from is a major PITA. Not to mention that you have to force the CSS to re-aggregate each time you tweak the CSS. And, if you're not a css develper, trust me, you have to tweak the CSS every 30 seconds (literally) to get things to look correctly cross-browser. :-p

So, our choices are these:

Solution #1 (the CSS-developers-are-on-their-own option):

  1. Fix #166759: Public/Private File Handling
  2. Enable CSS aggregation by default.
  3. (Optional) In fact, remove the CSS aggregation toggle entirely. Why not? Its not like the "disabled" option will help you to develop CSS in IE anyway.
  4. Leave CSS developers to figure out on their own that there's a contrib module to help them.

Solution #2 (the explain-why-IE-is-broken-and-not-Drupal option):

  1. Fix #166759: Public/Private File Handling
  2. Enable CSS aggregation by default.
  3. Expand the CSS aggregation toggle to include more useful options. Similar to the screenshot I included in #114 above, but with much better/simpler explanations of what each option does

As you can probably tell, I prefer solution #2. Also, note that the first two steps of both solutions are the same. So here's a patch that does step #2 of either solution.

AttachmentSize
optimize-css-default-228818-130.patch 1.76 KB
Testbed results
optimize-css-default-228818-130.patchfailedFailed: Failed to install HEAD. Detailed results

#132

System Message - June 29, 2009 - 08:25
Status:needs review» needs work

The last submitted patch failed testing.

#133

JohnAlbin - June 29, 2009 - 09:29
Status:needs work» needs review

Another try, bot?

#134

hass - June 29, 2009 - 19:31

The main reason why this is not enabled by default should be the private files folder... if private files are used - you cannot enable JS/CSS compression. About the compress module/theme css only - I cannot say how often I have analysed what core or module style is currently cluttering my theme css definition... I only hope this is not the only way... aside if I have 15 modules installed, all having LTR and RTL styles and I'm overriding them all in the theme... the latest approach will not solve the problem. 1 core + 1 print file + 1 module file + 30 theme module overrides + ~3-5 theme CSS + IE fix files... ~40 files. 15 modules seems not many for me...

It may be better to create 1-3 CSS files and link all core, module and theme styles via @imports... this would avoid the 30 style tag limit at all and load all files uncompressed.

I do not like to spoil the party, but the contrib module only deflects the problem...

However - we *NEED* to fix #166759: Public/Private File Handling or we leave all private files users out for the next ~3 years.

#135

JohnAlbin - June 29, 2009 - 19:51

Using @import to load all CSS is a no-go because of performance implication of using the @import declaration. BTW, the IE CSS Optimizer module tries to load as many stylesheets as a possible using <link> before it starts using @import (as a last resort) for the remaining stylesheets. Its not pretty, but its the best that is possible given the circumstances.

The main reason why this is not enabled by default should be the private files folder... if private files are used - you cannot enable JS/CSS compression

Which is why I mentioned fixing #166759: Public/Private File Handling three times in my comment. :-) Fortunately, the CSS aggregator is smart enough to turn itself off right now if the files directory is not set to the "public downloads" option. So enabling the option by default won't hurt the "private downloads" people. Of course, it won't help them either.

#136

hass - June 29, 2009 - 21:29

I'm aware about this "performance implication" you named "no-go", but I think it's more beneficial to have a reliable and working solution with a small performance implication - than a broken site... :-)

#137

System Message - June 30, 2009 - 02:40
Status:needs review» needs work

The last submitted patch failed testing.

#138

JohnAlbin - June 30, 2009 - 07:01
Status:needs work» needs review

Hass, so you want a solution that doesn't involve CSS aggregation at all? What are objections to the Solution #2 that I outlined above in comment #131? The "40 stylesheets in a theme" issue is not a problem; see comment #135. Any other objections?

#139

System Message - June 30, 2009 - 07:15
Status:needs review» needs work

The last submitted patch failed testing.

#140

hass - June 30, 2009 - 09:08

I wasn't aware about the "link" first than "@import" solution as I'm not using the contrib module. I only hope you count them correctly as sometimes the IE hacks are added manually to the themes page.tpl.php... sometimes... and sometimes they may be added via page_preprocess like I've done it in the YAML theme and maybe there are others doing other things or using your (?) IE hacks css module... I'm really interested to take a look to the code how this counting issues are solved.

#141

hass - June 30, 2009 - 09:23

Well, "IE CSS Optimizer" module looks very buggy if it comes to the count

1. It's not parsing page.tpl.php
2. It's not counting style tags
3. The Limit is 30 link/style elements not 31 as said above.
4. The module seems not the last... (no weight -> therefore alphabetic weight today) so other modules are able to add CSS files after the "IE CSS Optimizer" module.
5. CSS files added by the theme are also not counted.
6. No official release.

Untested, but this is all from a very short code review.

#142

decibel.places - June 30, 2009 - 14:09

please move discussion of the IE CSS Optimizer module to its issue queue where they belong!

http://drupal.org/project/issues/ie_css_optimizer

This thread was begun as a disussion of the problems using 31+ stylesheets in IE in general

Bug reports and support requests for the contrib module belong in its issues

FWIW I use the Unlimited CSS module to correct these problems

http://drupal.org/project/unlimited_css

@John Albin: do we really need another module that does the same thing? Perhaps you could co-maintain that module to introduce your ideas for refinement?

#143

hass - June 30, 2009 - 14:52

Code wise "IE CSS Optimizer" looks better to me and have more potential... I noted this here as I do not think it heals this case. John can take this over from here - I do not like to have 6 more cases in my issue queue

#144

Owen Barton - August 25, 2009 - 03:36
Status:needs work» fixed

This is fixed by the stream wrapper conversion (you can now do aggregation even with private files enabled). For folks having trouble developing with the aggregator enabled the above contrib modules should work, or else you can do what I do and add code in your theme (with a settings.php switch so it doesn't run on production sites) that deletes the aggregated files so they are recreated for each page load.

#145

JohnAlbin - August 25, 2009 - 12:41
Status:fixed» needs review

@Owen, this is not fixed.

To reiterate, we have 2 problems regarding this IE limit:

  1. IE users see a crippled site with more than 31 stylesheets unless the CSS optimization is enabled. And, by default, the CSS optimization is disabled. Which means, by default, IE users see many Drupal sites broken.
  2. Once we've enabled the CSS optimization, its difficult for CSS developers to actually develop CSS when its all compressed into one file

I fear that there isn't enough time to get a full fix for #2 before code freeze. Also, #2 doesn't make it impossible to develop CSS, just extremely tedious (you have to constantly rebuild the cache) and difficult (you can't view source to find out which CSS file has which ruleset).

However, the fix for #1 is really simple now that core has both public and private files (see #517814: File API Stream Wrapper Conversion). yay! We just need to make CSS optimization enabled by default. Which is what this patch does.

AttachmentSize
optimize-css-default-228818-145.patch 1.59 KB
Testbed results
optimize-css-default-228818-145.patchfailedFailed: 12426 passes, 6 fails, 0 exceptions Detailed results

#146

System Message - August 25, 2009 - 13:05
Status:needs review» needs work

The last submitted patch failed testing.

#147

Owen Barton - August 25, 2009 - 15:24

OK, that is a fair point. I have advocated having them on by default previously also, because having so much latency in page loads is a significant usability issue. Looks like we need to update some tests.

#148

JohnAlbin - August 27, 2009 - 05:13
Status:needs work» needs review

New patch fixes broken test.

AttachmentSize
optimize-css-default-228818-148.patch 2.4 KB
Testbed results
optimize-css-default-228818-148.patchfailedFailed: 12385 passes, 5 fails, 0 exceptions Detailed results

#149

System Message - August 27, 2009 - 05:30
Status:needs review» needs work

The last submitted patch failed testing.

#150

JohnAlbin - August 27, 2009 - 19:09
Status:needs work» needs review

Another test fix.

AttachmentSize
optimize-css-default-228818-150.patch 3.01 KB
Testbed results
optimize-css-default-228818-150.patchfailedFailed: 12789 passes, 1 fail, 1 exception Detailed results

#151

System Message - September 1, 2009 - 21:55
Status:needs review» needs work

The last submitted patch failed testing.

#152

JohnAlbin - October 3, 2009 - 18:06
Status:needs work» needs review

Added fix for color test.

AttachmentSize
optimize-css-default-228818-152.patch 2.84 KB
Testbed results
optimize-css-default-228818-152.patchfailedFailed: 13570 passes, 8 fails, 0 exceptions Detailed results

#153

System Message - October 3, 2009 - 18:20
Status:needs review» needs work

The last submitted patch failed testing.

#154

donquixote - October 16, 2009 - 22:26

subscribe

#155

seutje - October 20, 2009 - 09:45

Simply making CSS aggregation enabled by default doesn't really solve any problem imho, but I guess it prevents users from going all "omagawd, where are my styles :(" and I guess most developers/themers/whatever-you-want-to-call-them will notice right away when CSS is being aggregated

however, I think that enabling this by default would annoy the hell out of me in the long run, but then at least I will be less annoyed by the recurring questions like "why is my site unstyled in IE"

is there any chance of getting a proper solution into core aside from setting aggregation on by default? perhaps aggregate all CSS except theme CSS?

#156

JohnAlbin - November 6, 2009 - 12:42
Status:needs work» needs review

is there any chance of getting a proper solution into core aside from setting aggregation on by default? perhaps aggregate all CSS except theme CSS?

The proper solution exists in contrib. But trying to get the functionality of http://drupal.org/project/issues/ie_css_optimizer into core would be difficult to argue since we're past code freeze. At the very least, I'll make sure the module is available in D7 very soon (I'm going to need it).

But even if core already had a toggle to aggregate everything except theme CSS, Drupal would still have the problem of end users seeing a broken site in IE. So we definitely need to turn on aggregation by default.

I've re-rolled the patch with a fix for Hook menu tests.

AttachmentSize
optimize-css-default-228818-156.patch 3.21 KB
Testbed results
optimize-css-default-228818-156.patchfailedFailed: Failed to run tests. Detailed results

#157

System Message - November 6, 2009 - 13:00
Status:needs review» needs work

The last submitted patch failed testing.

#158

JohnAlbin - November 6, 2009 - 16:08
Status:needs work» needs review

“failed Failed: Failed to run tests.”? Huh? Wazzat mean? Trying again.

#159

System Message - November 6, 2009 - 16:30
Status:needs review» needs work

The last submitted patch failed testing.

 
 

Drupal is a registered trademark of Dries Buytaert.