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>
Files: 
CommentFileSizeAuthor
#377 drupal_css_workaround_for_ie_31_limit-228818-377.patch35.08 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 17,903 pass(es). View
#368 drupal_css_workaround_for_ie_31_limit-228818-368.patch33.11 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 17,900 pass(es). View
#366 drupal_css_workaround_for_ie_31_limit-228818-366.patch33.63 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 17,894 pass(es), 1 fail(s), and 0 exception(es). View
#357 drupal_css_workaround_for_ie_31_limit-228818-357.patch32.89 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 17,907 pass(es). View
#321 drupal_css_workaround_for_ie_31_limit-228818-321.patch32.88 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 17,533 pass(es). View
#312 drupal_css_workaround_for_ie_31_limit-228818-312.patch29.53 KBeffulgentsia
Passed on all environments. View
#313 drupal_css_workaround_for_ie_31_limit-228818-313.patch29.58 KBeffulgentsia
Passed on all environments. View
#307 drupal_css_workaround_for_ie_31_limit-228818-299.patch23.6 KBeffulgentsia
Passed on all environments. View
#304 aggregate-css-with-filemtime.patch554 bytesJonathanRoberts
Unable to apply patch aggregate-css-with-filemtime.patch View
#299 drupal_css_workaround_for_ie_31_limit-228818-299.patch23.6 KBeffulgentsia
Passed on all environments. View
#288 drupal_css_workaround_for_ie_31_limit-228818-287.patch18.66 KBeffulgentsia
Passed on all environments. View
#283 index.php_.txt1.62 KBdonquixote
#281 drupal_css_workaround_for_ie_31_limit-228818-281.patch13.08 KBeffulgentsia
Failed on MySQL 5.0 InnoDB, with: 16,686 pass(es), 0 fail(s), and 1 exception(es). View
#258 optimize-css-default-228818-258.patch1.53 KBmfer
Failed on MySQL 5.0 InnoDB, with: 16,643 pass(es), 10 fail(s), and 1 exception(es). View
#257 optimize-css-default-228818-257.patch411 bytesmfer
Failed on MySQL 5.0 InnoDB, with: 16,644 pass(es), 15 fail(s), and 1 exception(es). View
#227 drupal_css_workaround_for_ie_31_limit-228818-227.patch11.64 KBeffulgentsia
Passed on all environments. View
#226 drupal_css_workaround_for_ie_31_limit-228818-226.patch11.64 KBeffulgentsia
Passed on all environments. View
#225 drupal_css_workaround_for_ie_31_limit-228818-225.patch11.31 KBeffulgentsia
Failed on MySQL 5.0 InnoDB, with: 16,517 pass(es), 2 fail(s), and 0 exception(es). View
#222 drupal_css_workaround_for_ie_31_limit-228818-222.patch11.31 KBeffulgentsia
Passed on all environments. View
#198 drupal_css_workaround_for_ie_31_limit-228818-197.patch10.28 KBeffulgentsia
Passed on all environments. View
#195 css_overflow.patch6.28 KBbleen
Passed on all environments. View
#193 css_overflow.patch6.28 KBbleen
Failed on MySQL 5.0 InnoDB, with: 16,522 pass(es), 1 fail(s), and 0 exception(es). View
#189 drupal_css_workaround_for_ie_31_limit-228818-184.patch7.45 KBeffulgentsia
Failed on MySQL 5.0 InnoDB, with: 16,517 pass(es), 1 fail(s), and 0 exception(es). View
#167 css_overflow.patch7.37 KBbleen
Passed on all environments. View
#156 optimize-css-default-228818-156.patch3.21 KBJohnAlbin
Failed: Failed to run tests. View
#152 optimize-css-default-228818-152.patch2.84 KBJohnAlbin
Failed: 13570 passes, 8 fails, 0 exceptions View
#150 optimize-css-default-228818-150.patch3.01 KBJohnAlbin
Failed: 12789 passes, 1 fail, 1 exception View
#148 optimize-css-default-228818-148.patch2.4 KBJohnAlbin
Failed: 12385 passes, 5 fails, 0 exceptions View
#145 optimize-css-default-228818-145.patch1.59 KBJohnAlbin
Failed: 12426 passes, 6 fails, 0 exceptions View
#131 optimize-css-default-228818-130.patch1.76 KBJohnAlbin
Failed: Failed to install HEAD. View
#67 drupal-5-7_css-styles.patch9.28 KBbrahms
Failed: Failed to apply patch. View
#63 drupal-5.8_css_styles.patch8.46 KBbrahms
Failed: Failed to apply patch. View
#63 drupal-6.3_css_styles.patch8.34 KBbrahms
Failed: Failed to apply patch. View
#60 drupal-228818-60.patch8.61 KBbrahms
Failed: Failed to apply patch. View
#51 drupal-head_css-styles.patch1_.gz2.02 KBbrahms
#37 drupal-6_css-styles.patch9.67 KBbrahms
Failed: Failed to apply patch. View
#37 drupal-6-2_css-styles.patch9.67 KBbrahms
Failed: Failed to apply patch. View
#37 drupal-5_css-styles.patch9.45 KBbrahms
Failed: Failed to apply patch. View
#37 drupal-5-7_css-styles.patch9.45 KBbrahms
Failed: Failed to apply patch. View
#36 drupal-head_css-styles.patch9.57 KBbrahms
Failed: Failed to apply patch. View
#26 common.inc-5.7-css_styles.patch9.47 KBbrahms
Failed: Failed to apply patch. View
#23 common.inc-5.7-css_link.patch2.01 KBbrahms
Failed: Failed to apply patch. View
#10 common_style_limit_D5_2008050501.patch2.19 KBhass
Failed: Failed to apply patch. View
#10 common_style_limit_D6_2008050501.patch2.32 KBhass
Failed: Failed to apply patch. View
#10 common_style_limit_HEAD_2008050501.patch2.35 KBhass
Failed: Failed to apply patch. View

Comments

Trendy’s picture

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

dman’s picture

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.

hass’s picture

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

dman’s picture

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)!

hass’s picture

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...

brahms’s picture

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.

brahms’s picture

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:

$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  issue on drupal.org. By enabling this option all import links for css files are combined into one <style> 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.

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 .= '@import "'. base_path() . $file .'";' ."\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 .= '@import "'. base_path() . $file .'";' ."\n";
            }
          }
          else {
            if ($combine_css) {
              $output_media .= '@import "'. base_path() . $file .'";' ."\n";
            } else {
              $output .= '@import "'. base_path() . $file .'";' ."\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 .= '@import "'. base_path() . $preprocess_file .'";'. "\n";
      }
    }

    if ($combine_css && $no_module_preprocess_media) {
      $no_module_preprocess_media = ''. $no_module_preprocess_media .''. "\n";
      $no_module_preprocess .= $no_module_preprocess_media;
    }
    if ($combine_css && $no_theme_preprocess_media) {
      $no_theme_preprocess_media = ''. $no_theme_preprocess_media .''. "\n";
      $no_theme_preprocess .= $no_theme_preprocess_media;
    }
    if ($combine_css && $output_media) {
      $output_media = ''. $output_media .''. "\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!

te-brian’s picture

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

hass’s picture

@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.

hass’s picture

Status: Active » Needs review
FileSize
2.35 KB
Failed: Failed to apply patch. View
2.32 KB
Failed: Failed to apply patch. View
2.19 KB
Failed: Failed to apply patch. View

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 :-)

hass’s picture

Status: Needs review » Needs work

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

Gábor Hojtsy’s picture

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?

C_Logemann’s picture

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

C_Logemann’s picture

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.

dvessel’s picture

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?

hass’s picture

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...

dvessel’s picture

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.

dvessel’s picture

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.

hass’s picture

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.

dvessel’s picture

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.

hass’s picture

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 :-)

dvessel’s picture

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.

brahms’s picture

FileSize
2.01 KB
Failed: Failed to apply patch. View

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.

hass’s picture

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.

brahms’s picture

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 ?

brahms’s picture

FileSize
9.47 KB
Failed: Failed to apply patch. View

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.

brahms’s picture

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]-->
hass’s picture

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

brahms’s picture

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.

hass’s picture

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.

brahms’s picture

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.

brahms’s picture

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!

hass’s picture

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 :-(.

brahms’s picture

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.

Damien Tournoud’s picture

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.

brahms’s picture

FileSize
9.57 KB
Failed: Failed to apply patch. View

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

brahms’s picture

FileSize
9.45 KB
Failed: Failed to apply patch. View
9.45 KB
Failed: Failed to apply patch. View
9.67 KB
Failed: Failed to apply patch. View
9.67 KB
Failed: Failed to apply patch. View

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

Crell’s picture

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)

hass’s picture

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

Crell’s picture

@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.

brahms’s picture

@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?

Michelle’s picture

@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

bensnyder’s picture

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

Will this patch be in 6.3?

hass’s picture

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.

hass’s picture

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).

hass’s picture

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

brahms’s picture

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.
brahms’s picture

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?

hass’s picture

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.

brahms’s picture

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.

brahms’s picture

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.

kevinquillen’s picture

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.

brahms’s picture

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...

Jeff Burnz’s picture

Subscribing

hass’s picture

@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.

kevinquillen’s picture

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.

brahms’s picture

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)...

Crell’s picture

@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.)

catch’s picture

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.

brahms’s picture

FileSize
8.61 KB
Failed: Failed to apply patch. View

@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.

brahms’s picture

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.

bensnyder’s picture

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!

brahms’s picture

FileSize
8.34 KB
Failed: Failed to apply patch. View
8.46 KB
Failed: Failed to apply patch. View

@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):

bensnyder’s picture

ahh... the beauty of open source!

jfxberns’s picture

Version: 7.x-dev » 5.7
Category: bug » support
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?

hass’s picture

Version: 5.7 » 7.x-dev
Category: support » bug
Priority: Normal » Critical

Please don't high jack this case!

brahms’s picture

FileSize
9.28 KB
Failed: Failed to apply patch. View

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.

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

hass’s picture

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... :-)

thelocaltourist’s picture

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.

brahms’s picture

@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?

thelocaltourist’s picture

No, no error messages.

(thanks for the quick response!)

momendo’s picture

sub

Garnerin’s picture

subscribing

Owen Barton’s picture

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:

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);
}
c960657’s picture

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.

hass’s picture

joachim’s picture

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.

bensnyder’s picture

Amen. +1

tombigel’s picture

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.

Michelle’s picture

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

Michelle

Crell’s picture

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.

hass’s picture

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.

tombigel’s picture

@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"?

Crell’s picture

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.)

tombigel’s picture

@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.

hass’s picture

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.

tombigel’s picture

@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).

hass’s picture

How about fixing the permission problem? :-)

natrio’s picture

Any progress on the fix?

subscribing...

decibel.places’s picture

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?)

chandrabhan’s picture

+

JohnAlbin’s picture

/me contemplates… (and subscribes)

s.Daniel’s picture

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

Reg’s picture

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.

Apfel007’s picture

Version: 7.x-dev » 6.9
Category: bug » 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

wretched sinner - saved by grace’s picture

Version: 6.9 » 7.x-dev
Category: task » bug

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

Apfel007’s picture

sorry

kevinquillen’s picture

"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.

catch’s picture

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.

ademarco’s picture

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.

dvessel’s picture

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.

ademarco’s picture

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.

ademarco’s picture

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.

hass’s picture

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

ademarco’s picture

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!

ademarco’s picture

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!

hass’s picture

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...

decibel.places’s picture

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

hass’s picture

Sounds like you found tons of duplicates...

decibel.places’s picture

@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

ademarco’s picture

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

tanc’s picture

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.

JohnAlbin’s picture

FileSize
33.99 KB

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:

Jeff Burnz’s picture

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.

JohnAlbin’s picture

- 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”?

Crell’s picture

"Compress [module|theme] CSS only" ?

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

hass’s picture

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

s.Daniel’s picture

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

DamienMcKenna’s picture

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

Crell’s picture

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.

hass’s picture

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...

decibel.places’s picture

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"

hass’s picture

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.

Michelle’s picture

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

Michelle

hass’s picture

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.

Michelle’s picture

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

hass’s picture

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.

decibel.places’s picture

@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

tombigel’s picture

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)

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
1.76 KB
Failed: Failed to install HEAD. View

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

Status: Needs work » Needs review

Another try, bot?

hass’s picture

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.

JohnAlbin’s picture

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.

hass’s picture

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... :-)

Status: Needs review » Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

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?

Status: Needs review » Needs work

The last submitted patch failed testing.

hass’s picture

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.

hass’s picture

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.

decibel.places’s picture

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?

hass’s picture

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

Owen Barton’s picture

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.

JohnAlbin’s picture

Status: Fixed » Needs review
FileSize
1.59 KB
Failed: 12426 passes, 6 fails, 0 exceptions View

@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.

Status: Needs review » Needs work

The last submitted patch failed testing.

Owen Barton’s picture

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.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
2.4 KB
Failed: 12385 passes, 5 fails, 0 exceptions View

New patch fixes broken test.

Status: Needs review » Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
3.01 KB
Failed: 12789 passes, 1 fail, 1 exception View

Another test fix.

Status: Needs review » Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
2.84 KB
Failed: 13570 passes, 8 fails, 0 exceptions View

Added fix for color test.

Status: Needs review » Needs work

The last submitted patch failed testing.

donquixote’s picture

subscribe

seutje’s picture

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?

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
3.21 KB
Failed: Failed to run tests. View

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work

The last submitted patch failed testing.

bleen’s picture

superscribe

donquixote’s picture

If @import has such bad performance implications, why not do the following:
- If the number is lower than 30, use
- If it's more than 30, use @import for every stylesheet above the limit.

And for CSS aggregation, provide an extra option "for everyone except the admin".

I think this stuff should go in core, or shipped with D6 as an optional module, or anything to make it available on first time install. It's rightfully marked as "bug report", so why is code freeze an issue?

----

There could be an extra option to enable it based on IP address or something, to test site layout for not logged-in users - if anyone feels like writing such a module.

alexanderpas’s picture

Radical idea: How about always serving IE (-versions that have this problem) with the compressed stylesheets output when this issue occurs. (altrough, this probaly has some influence on other IE Conditional Comments)

Idea in code:
When compression is turned off, but more then 30 stylesheets are availble

<!--[if IE]>
[... compressed CSS ...]
<![endif]-->
<!--[if !IE]<!-->
[... uncompressed CSS ...]
<!--<![endif]-->
alexanderpas’s picture

Radical idea: How about always serving IE (-versions that have this problem) with the compressed stylesheets output when this issue occurs. (altrough, this probaly has some influence on other IE Conditional Comments)

Idea in code:
When compression is turned off, but more then 30 stylesheets are availble

<!--[if IE]>
[... compressed CSS ...]
<![endif]-->
<!--[if !IE]<!-->
[... uncompressed CSS ...]
<!--<![endif]-->
alexanderpas’s picture

Radical idea: How about always serving IE (-versions that have this problem) with the compressed stylesheets output when this issue occurs. (altrough, this probaly has some influence on other IE Conditional Comments)

Idea in code:
When compression is turned off, but more then 30 stylesheets are availble

<!--[if IE]>
[... compressed CSS ...]
<![endif]-->
<!--[if !IE]<!-->
[... uncompressed CSS ...]
<!--<![endif]-->
treksler’s picture

Hi,

Without a fix drupal_add_css() is simply useless, because it doesn't adequately abstract away the task of reliably adding CSS to the document in a cross-browser compatable way. Simple as that. Imagine if jQuery's $.show(); function had some arbitrary undocumented limitation that made it only show the element the first 30 times it was called in IE7 .. Ridiculous!

The obvious solution is:
1) when CSS aggregation is OFF, use one

tag (one per media) with multiple @import statements in each. (or up to 29 tags, and beyond that use one tag with multiple @import statements) 2) when CSS aggregation is ON, use one or tag per css file Remember, CSS aggregation is a performance optimization, so when it is off, you don't care about performance anyway so it is safe to use @import when aggregation is off. Also, now that the private file issue is fixed, there is no reason not to use aggregation in production. So, if saving web archives doesn't work while developing themes or modules (aggregation off) then no big deal.
mrfelton’s picture

IMO this should definitely be fixed in Core. Every single Drupal site I have ever built has hit this problem. The first time it took me several hours to diagnose it. Other users may simply abandon Drupal at this point as it can be really, really frustrating.

#161, #162 and #165 all have good ideas.

bleen’s picture

FileSize
7.37 KB
Passed on all environments. View

This patch fixes this issue and implements the following:

  • if there are more than 30 stylesheets, display the first 30 using <link ...> and then display the overflow in a single <style> tag using @import.
  • adds a checkbox to the config/development/performance page that allows users to turn on/off this workaround (its "on" by default) but I'm not certain this is the correct place for it. I am certain it is necessary though
  • I did NOT implement any logic to check if the request is coming from IE because in my experience there are too many issues with caching and CDNs to make this work

I love the idea of giving users the ability to turn CSS compression on/off for non-admins only, but I think that is a completely seperate issue and should be dealt with separately.

Thoughts?

q0rban’s picture

Status: Needs work » Needs review

I think the best solution would be to just add a hook_requirements error if you go over 30 stylesheets that explains the problem and points to some solutions (like turning on preprocessing or this).

Dave Reid’s picture

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

The problem with a hook_requirements is that how do we know if we go over 30? It may happen on a node page, but not the front page. Maybe an admin page. Maybe you disable a module and it brings the number below 30. This is too edge-case a condition to fix in core.

We need to make sure that:
1. We have this documented thoroughly in some kind of 'Handling IE stupidness' handbook section.
2. We direct people to file issues to ensure modules only include their CSS when absolutely necessary.
3. We not add this hack to core. Support contrib solutions like http://drupal.org/project/ie_css_optimizer and http://drupal.org/project/unlimited_css

Jacine’s picture

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

If core itself wasn't responsible for having loaded 20+ files, pointing to contrib would be a more reasonable argument. Just clicking around in D7, I found that editing a field (in the overlay) brought the count up to 22 stylesheets. Point is, all you have to do is enable a few modules and you are at the limit. We all know every site ends up with at least a few modules.

Despite knowing full well about this problem, I still manage to get burnt by it every now and then. It's very frustrating. Also, trying to develop themes is a nightmare with CSS compression on. Personally, I would be fine with using compression, IF there was an option to do it by type, i.e:

* Aggregate and compress ALL CSS files into one file.
* Aggregate and compress MODULE CSS files into one file.
* Aggregate and compress THEME CSS files into one file.

Is that an option here?

hass’s picture

D7 core have only in misc and modules 75 CSS files and a theme adds additional files... ~5 per theme (e.g. Garland) - so only enable a few core modules and you will reach the limit - very easily... is it acceptable that D7 is broken out of the box?

Dave Reid’s picture

D7 core with *all* core modules enabled has 15 CSS on the frontpage and 17 CSS on a node page. We don't include all freakin 75 at once, you know better than that.

DamienMcKenna’s picture

q0rban: just simply yelling at developers that they shouldn't use so many CSS files is not a constructive suggestion, especially when this is otherwise outside of their control. The whole point of this discussion is to work out a way to have this out of the box have a behavior that will work for what is still the most widely used browser in the world.

q0rban’s picture

@DamienMcKenna: Did you intend that for hass? I only suggested putting a warning on the status page if you went over 30 CSS sheets.

bleen’s picture

Dave Reid and I spoke briefly on IRC and he pointed out that when you do a clean install of D7 and enable every module you get 15 stylesheets () and 17 (node/123) ... I took it one step further and installed just Views, Admin_menu & Zen (without creating a sub theme and those numbers jumped to 20 () and 22 (node/123)

I see both arguments here. On one hand, this really *should* be handled in contrib. On the other hand, it doesn't take much to get to the magic number.

Anyone else want to weigh in?

DamienMcKenna’s picture

q0rban: Yes, displaying a message doesn't fix the problem which is needing to work around a bug in what is still the most popular browser series.

q0rban’s picture

@DamienMcKenna Ok, that's fine, but I wasn't yelling at any developers. I was just suggesting a status message so the site owner would at least know why things were acting funky. I'm not talking about anything rude, just something like: "Page xyz loaded more than 30 style sheets, which may cause problems in Internet Explorer. You might consider enabling CSS Preprocessing."

mrfelton’s picture

Could print a message in the watchdog for that perhaps? Though I do think a message is a good idea, a fix would be better! :)

q0rban’s picture

> Could print a message in the watchdog for that perhaps

That would be simpler to implement than using hook_requirements for sure. Don't get me wrong, I'm all about a solution as long as it's not hacky. I was getting the impression that all the patches in this thread were pretty hacky, so just trying to think of another hack-free solution.

bleen’s picture

@q0rban: do you feel that #167 is too hacky?

q0rban’s picture

@bleen18: It is yet another UI element, and yet another workaround for a Microsoft bug. I'll leave that up to others to determine if that's hacky or not. ;)

mcrittenden’s picture

Sub.

bleen’s picture

Was talking with webchick about this in IRC and she suggested it may be a good time to summarize:

PROBLEM:
IE (all versions, even 8) hates and will not render more then 30 style tags+links to css files (total). It doesn't take much to hit this limit on a Drupal install (fresh install of D7 with all core modules ~ 15-20 CSS files).

POSSIBLE SOLUTIONS

  1. Force css aggregation (#40) - This solves the root issue, but causes problems when developing. Mainly that developers cannot easily figure out what files to edit. (#42 also makes some good points about turning off compression for one reason or another will break a production site for large numbers of users).
  2. Force css aggregation for IE only (when +30 css) - Same issue as the previous solution plus there are caching problems when detecting what browser is making the request (in my experience).(#163)
  3. Create a style tag for each media type and use @import for all CSS includes(#51) - solves the issue but with performance hit. Also, Crell points out in #145218: Use href instead of @import for CSS (#3) that @import also creates issues when trying to save a whole webpage and all its dependent files.
  4. No core fix (contrib fixes only), just heavily document the problem (#169)
  5. Include the first 30 CSS files using link tags (as normal) and then all overflow is put in a style tag using @import (#167) - this fixes the issue with a performance hit. The patch in #167 lets users turn this on/off which is yet another UI element.

ME THINKS

  • this needs to be fixed in core because it is too easy to hit the magic number
  • admins should be able to turn this workaround on/off
  • the patch in #167 has the least performance impact because the first 30 (it may need to be changed to 29) files are linked appropriately and only the overflow is included using @import

did I miss anything?

webchick’s picture

The performance hits don't bother me too direly, since CSS aggregation should always be turned on for production for a performance boost (though few of the high-profile Drupal sites I've seen recently seem to know this trick...).

I agree that a fix of some kind belongs in core. I don't understand enough about the underlying browser render patterns / CSS itself to make a judgment call on which way is optimal, though. For that I'd defer to front-end developers who've hit this issue and been forced to work around it.

But my gut instinct is:

#1: -1. We split up all those CSS files for a reason, so that it'd be easier to find and isolate the styles that are affecting a certain part of the page.
#2: -1. Browser detection algorithms are inaccurate, in my experience. Unless jQuery helps us here.
#4: -1. The pattern that Drupal core establishes with its CSS organization leads to this issue, therefore core ought to provide a fix/workaround.

I can't really differentiate between #3 and #5.

I don't really understand exposing a setting for this though.

Crell’s picture

I am loathe to suggest this option but...

if (compression is enabled) {
use link tags (one per media type, of course)
}
else {
use style tags with a metric ton of @import statements
}

The assumption here being that if you gave a damn about performance or page save-ability, you'd turn the compressor on. If you don't have the compressor on, then you clearly don't care if your site is slow or if it's impossible to save pages from.

Themers: Would that increase or decrease the pain during theme development?

donquixote’s picture

I don't really understand exposing a setting for this though.

Sometimes you have more CSS files than Drupal knows about, because they are explicitly included in page.tpl.php, instead of using drupal_add_css(). I have this situation with a PHP-generated CSS file that gets the base url as a $_GET parameter (for IE6 transparent png support).

In this case it would be nice to be able to adjust the number of CSS files that go with
, before @import is used. To be safe, the default number could be 25 instead of 30.

hass’s picture

@Dave Reid: re #172 - enable a RTL language, please. You may hit the limit very quickly... 17*2 = 34 CSS files...

Additionally, it really sounds like a pretty useless discussion if core have enough files out of the box or not. I'm not aware about any site that doesn't run contrib modules. And there are themes out like YAML that include *many* more than 5 CSS files like garland - I would guess an average of 10-15 without compression.

@import for uncompressed situations seems fine for me as it should work and we don't care in DEV about speed here. Aside compression may be better enabled by default. I mostly see uncompressed CSS files on Drupal sites... many may not aware about this setting and performance boost.

q0rban’s picture

I suppose it's not an option to have preprocess CSS on by default? It's really a bad idea for it to be off on a production site anyways, due to the additional number of HTTP requests.

effulgentsia’s picture

FileSize
7.45 KB
Failed on MySQL 5.0 InnoDB, with: 16,517 pass(es), 1 fail(s), and 0 exception(es). View

I agree that this should be fixed in core if possible. We give people an option to run without CSS aggregation. It's very easy to go over the 31 limit. And much as we would like to, we can't just say that IE isn't a supported browser.

Here's another attempt at fixing it in a way that's less hacky (IMO). I don't think there needs to be an option to turn off this logic. The theme_stylesheets() function can be overridden if someone really wants to run a site that doesn't work in IE.

effulgentsia’s picture

oops, cross posted without reading 184-188. I still think #189 is good, and given #186, theme_stylesheets() can be overridden to accomodate more sophisticated needs, or we can work on making theme_stylesheets() more flexible with respect to the magic number.

Status: Needs review » Needs work

The last submitted patch, drupal_css_workaround_for_ie_31_limit-228818-184.patch, failed testing.

jwilson3’s picture

Status: Needs work » Needs review

+1 for overall patch ideas implemented in #167 (visual review)
-1 for new UI elements and making the performance settings screen more complicated.

(Provided that the files directory is writable), this patch would make the default performance settings on a fresh install look like this:

Bandwidth Optimizations
External resources can be optimized automatically, which can reduce both the size and number of requests made to your website.

[ ] Aggregate and compress CSS files into one file.
[x] Employ workaround for this (IE stylesheet limit bug)[link].
    All stylesheets after the 30th will be included using the slower @import method.
[ ] Aggregate JavaScript files into one file.

There's a couple problems with this.

1) Visually, this looks confusing, as it separates the two pre-existing checkboxes that are already very similar in form and function by putting the IE workaround in between them, which is totally unrelated in form and function.
2) Its counterintuitive to show this configuration (again, enabled by default) as a Bandwidth Optimization when the description text seems to describe that its somehow slower. The cost of employing the workaround is felt in a slower browser rendering, not bandwidth. See Yahoo Performance Rules.
3) Forsaking #1 and #2 above, the terminology seems unnecessarily complex: 'CSS file' and 'stylesheet', in this case both imply the same thing.

Finally, I'd argue that there is actually no need for the preference to be exposed in the admin UI. The logic should be as follows: If a page has more than 30 stylesheets, then Drupal should support it so it renders correctly in Internet Explorer.

I read this entire thread and I cannot find any reference to real stats on real or perceived performance degradation across the various popular browsers from using (for example) 30 <link>s and 30 @imports versus just 60 <link>ed stylesheets. If you're already at 30 stylesheets, and haven't considered (or simply cannot) turn on CSS Aggregation, then you're probably less worried about performance than you are about supporting IE. This is true for developers that need to test css without aggregation.

So, in the real-world cases when a site has 30+ css files, is there anyone more concerned with specifically NOT supporting IE versus a presumably minimal performance optimization gain?

Unless I missed something (entirely possible;), it looks like all the other issues and limiting factors noted above (eg, private file system) have been resolved. There just doesn't seem to me to be much of case against this being always enabled.

bleen’s picture

FileSize
6.28 KB
Failed on MySQL 5.0 InnoDB, with: 16,522 pass(es), 1 fail(s), and 0 exception(es). View

It appears no one else thinks this should be an option for users to turn on/off ... so this patch is the same as #167 but w/o the UI element.

For the record, I agree with all the comments in #192, but *if* we were going to have this checkbox, I dont know where else it would go ... hmmm

@Crelll (#185) I'm warming to your idea. If enough people chime in Ill take a look at creating a patch going in that direction, but for now, lets see how well this patch is received.

Status: Needs review » Needs work

The last submitted patch, css_overflow.patch, failed testing.

bleen’s picture

Status: Needs work » Needs review
FileSize
6.28 KB
Passed on all environments. View

oops ... I left the "magic number" at 3 (for testing) and never put it back to 30.

This patch is the same as #167 w/o the config checkbox

donquixote’s picture

see #186,
I strongly suggest to have a "magic number" less than 30, to allow explicit stylesheet inclusions in page.tpl.php.

wretched sinner - saved by grace’s picture

Thinking about it, and how the CSS aggregation works, why not combine the CSS files using @import statements, one per media type (screen, print, all etc). That way, there are the same number of link tags between aggregated and not, the difference is the performance.

That way we completely avoid the issue of not leaving enough room for manual stylesheets in a theme.

effulgentsia’s picture

FileSize
10.28 KB
Passed on all environments. View

And here's my recommendation, so we can compare/contrast with #195. This one's a continuation of #189. Here's what I prefer about this one vs. #195:

  • IE7 does not support media types in the @import statement. Unless we're willing to drop IE7 as a supported browser, we need to deal with this. This patch addresses it (most of the ugliness within theme_stylesheets() is to address this).
  • On IE7 (I haven't tested IE8), there is a limit of 31 @import statements per STYLE tag. This patch addresses it. While this only matters for a site that has magic number + 31 CSS files (~60 files), if we're dealing with this issue in core at all, let's cover that.
  • On IE7 (I haven't tested IE8), there is a 31 limit on total LINK (rel=stylesheet) plus STYLE tags. Even the STYLE tag for inline styles counts against this limit. This patch addresses it.
  • This patch does not add any new configuration settings to core. Instead a preprocess function can set $variables['max_tags'] to something smaller than 31 to accommodate #186. And I think it should be the responsibility of a module/theme to implement a preprocess function to do this if the module/theme is adding stylesheets through some mechanism other than drupal_add_css().

This patch also includes a @todo for needing a comment as to why external CSS files should be after non-external ones. I don't know why we need this, but it's how HEAD works. If we don't need this, this patch can be rerolled to not do it.

Jacine’s picture

I like #198. Clean and flexible.

effulgentsia’s picture

why not combine the CSS files using @import statements?

If you mean why not ONLY do this, then because for reasons already discussed, we would prefer to use LINK tags over STYLE tags where possible, and we should not be forced into STYLE tags just because IE sucks. But #198 does do this combining, just in a way that is compatible with IE while preserving the use of LINK tags as much as possible. Sites using aggregation get to use LINK tags. Sites not using aggregation but with <31 CSS files get to use LINK tags. The patch allows us to only do the combining into STYLE tags when we absolutely have to.

And with this, a theme can override theme_stylesheets() to do something different, like just output LINK tags and put a message on the website for the user to download a web browser that doesn't suck.

jwilson3’s picture

@effulgentsia, does your script actually handle >60 stylesheets correctly in IE6, et al?

In other words: 61 stylesheets would create: 30 link tags plus 1 style tag with 31 imports.

I was under the impression based on comments #25, #27, and #34 way up above that only 30 @import statements will be rendered per <style> tag.

Update: oops, this comment was for a review of patch in #184, it looks like your subsequent version in #198 does support max number of @imports per style tag. This thread is moving right along!

I like where #198 is going from a visual overview. Havent installed or tested it.

For the record, @Crell (#185) and @bleen18 (#193):

"... else { use style tags with a metric ton of @import statements }"

You cant just use all style + import tags because that too has a separate rendering bug in IE: FOUC (flash of unstyled content). The fix... at least one <link rel='stylesheet'>. Hence, the beauty of the combined solution in #198 of links with @imports.

q0rban’s picture

Status: Needs review » Needs work

I don't like the idea of using a theme function for this. The whole premise of theme functions is to separate logic from presentation. This seems even more hacky than previous solutions to me.

bleen’s picture

+++ includes/theme.inc	6 Jan 2010 04:28:24 -0000
@@ -2033,8 +2033,150 @@ function theme_html_tag($variables) {
+  // Order all the external CSS files to be after the non-external ones.
+  // @todo Add a comment explaining why.
+  $internal = array();
+  $external = array();
+  foreach ($variables['files'] as $file) {
+    if ($file['external']) {
+      $external[] = $file;
+    }
+    else {
+      $internal[] = $file;
+    }
+  }
+  $files = array_merge($internal, $external);
+
+  // Default max_tags to 31, if it isn't specified. Then decrement if there is
+  // inline CSS since that will use up one.
+  $max_tags = !empty($variables['max_tags']) ? $variables['max_tags'] : 31;
+  if (!empty($variables['inline'])) {
+    $max_tags--;
+  }
+
+  // Without changing their order, split $files into groups, where each group
+  // contains files of the same media type and each group is restricted to 31
+  // items. This enables the use of STYLE tags on a per-group basis if needed
+  // to stay within the $max_tags limit. We must ensure that each file within a
+  // group is for the same media, because IE7 does not support media declaration
+  // in the @import statement, so the media declaration must be on the STYLE
+  // tag.
+  $file_groups = array();
+  $file_group = array();
+  $file_group_media = NULL;
+  $file_group_counts = array();
+  foreach ($files as $file) {
+    if (count($file_group) == 31 || ($file_group_media && $file['media'] != $file_group_media)) {
+      if (!empty($file_group)) {
+        $file_groups[] = $file_group;
+        $file_group_counts[] = count($file_group);
+        $file_group = array();
+      }
+      $file_group_media = $file['media'];
+    }
+    $file_group[] = $file;
+  }
+  if (!empty($file_group)) {
+    $file_groups[] = $file_group;
+    $file_group_counts[] = count($file_group);
+  }

To @q0rban's point ... I think most of the code above could be handled in drupal_get_css and we could send $file_groups into the theme function. That way the theme function is truly handling only display logic and it would still be flexible enough to let a programmer rejigger those groups for his/her own purposes

Dave Reid’s picture

Maybe add a 'can_group' or 'can_import' parameter to drupal_add_css()?

DamienMcKenna’s picture

Title: IE: Stylesheets ignored after 31 link/style tags » IE: Stylesheets ignored after 30 link/style tags

I'm going to make the heretical point that the default action by drupal_get_css(), when aggregation is disabled, should be to output all CSS with @import (wrapped in a STYLE tag with the correct MEDIA attribute), given that:

  1. CSS files can be manually added through page.tpl.php, preprocessor functions, etc,
  2. There are no reasons forthcoming stating why using @import would be bad,
  3. It would be simpler code: if aggregate just use LINK otherwise use @import,
  4. We would save ourselves from support issues being opened because of people running into problems because of #1.
JohnAlbin’s picture

Title: IE: Stylesheets ignored after 30 link/style tags » IE: Stylesheets ignored after 31 link/style tags

So that we don't bikeshed the issue title, I created a comprehensive test suite to determine once and for all that MS's KB article is wrong. It really is a 31 stylesheet limit. See Stylesheets Not Loading? 31 Reasons to Hate Internet Explorer.

The #5 solution breen and effulgentsia are suggesting (using a mix of link and @import) sounds like a good solution at first, but according to Don’t use @import, we will have even worse performance for IE users. This is because IE (all versions) will wait until it has downloaded all <link> styles and all JavaScripts before even starting to download @import stylesheets.

Again, looking at the above article, if we use all link stylesheets or all @import stylesheets we won’t have this idiotic IE performance hit. So what bleen said about solution #3 was incorrect. Using only @import for stylesheets doesn't cause performance problems.

I think Larry's suggestion in comment #185 is the best solution. When CSS aggregation is enabled, use a link tag. When CSS aggregation is disabled, use @import. Also make sure that each style tag has a max of 31 @import styles. This solves the issue and only leaves a slight possibility of javascript race-conditions; which without any concrete examples we can safely ignore, IMO.

There is additionally the question of do we enable CSS aggregation by default or not. IMO, we could and should enable it by default. It will increase performance out of the box for all users. And enabling it only creates issues for developers, but a developer will quickly look at the source code, see <link type="text/css" rel="stylesheet" media="all" href="/sites/default/files/css/css_09986c26e7411208bc6d2574c7872939.css" /> and realize something is going on. But that feels more like a separate issue if we go with Larry's suggestion.

Dave Reid’s picture

Yes, I'd much prefer an approach of using all @imports per-media type when CSS aggregation is disabled and use <link> when aggregation is enabled. Let's not do any kind of mix.

mrfelton’s picture

When CSS aggregation is enabled, use a link tag. When CSS aggregation is disabled, use @import.

If this is to be the solution, could it be made such that @import is only used for IE and link for all proper browsers?

DamienMcKenna’s picture

mrfelton: As webchick has mentioned above, browser detection routines are not reliable enough to be worth using for this case, given that CSS aggregation will usually only be disabled during development it should be a-ok to do for all visitors.

q0rban’s picture

Title: IE: Stylesheets ignored after 30 link/style tags » IE: Stylesheets ignored after 31 link/style tags
Status: Needs work » Needs review

I'm in agreement with #206-7, and also support enabling CSS aggregation by default, although I suppose that's a separate issue now.

Dave Reid’s picture

Status: Needs review » Needs work

Its going to be impossible to do browser detection because of caching, proxies, etc.

Crazy idea...would we be able to do something like the following? Does IE not count <link> tags inside conditional comments?

<!--[if !IE]>
  <link ... />
  <link ... />
<![endif]>
<!--[if IE]>
  <style> @import ... </style>
  <style> @import ... </style>
<![endif]>
mrfelton’s picture

@Dave Reid: that is what I was getting at... I have never heard of <!--[if !IE]> being misinterpreted.

DamienMcKenna’s picture

davereid: Is it worth bothering with that extra logic given we're hoping to push this to effectively a "site is in development" option rather than the default? Less code = less problems down the road.

Dave Reid’s picture

Yeah I'm really starting to flip back to my position that this should be solved in contrib.

hass’s picture

1. IE *does* count link elements in conditional comments... 100% save :-(
2. We cannot merge all @imports into one style tag as IE does not support media after @import. Therefore we need to go back to the way we used in D5. Add every file with it's own @import.

hass’s picture

This is not a development issue only. We don't/cannot/shouldn't force people to use compression... maybe they use a buggy CSS file and this breaks all if compressed... then they use an uncompressed site until their bugs are fixed (if they are going to be fixed). Or the compressor is broken...

DamienMcKenna’s picture

hass: If the standard practice becomes to use aggregation then developers will become more aware of bugs in their CSS and be more likely to fix them. I don't believe we should have to cow-tow developers that much.

mrfelton’s picture

If, as it is looking, it is agreed that nothing should be done about this except enable css aggregation by default (which I honestly don't think is an adequate solution) then at least lets have a watchdog message (#178) would help people who are hitting the problem to identify it more easily.

Jacine’s picture

@JohnAlbin Thanks for the links in #206. I wasn't aware that the combination was the problem. Based on that, I agree #185 seems like a better approach.

effulgentsia’s picture

Assigned: Unassigned » effulgentsia

On it.

bleen’s picture

+1 for #185, #205, #206, #207:

CASE 1: Aggregation is ON
We won't get anywhere near the Magic Number, so use <link> tags as it is generally the better option

Case 2: Aggregation is OFF
Use <style> tags with @import's adhering to the following rules:

  • the media attribute must be applied to the <style> tag (IE needs this) so @imports must be grouped by "media"
  • no single <style> tag should have more than 31 @import statements. So if, there are 40 "screen" stylesheets to include, thre will be 2 <style media="screen"> tags.
effulgentsia’s picture

Status: Needs work » Needs review
FileSize
11.31 KB
Passed on all environments. View

Ok, this implements the consensus reached above.

I maintain that this logic should live in a theme function, which it does in this patch. While our consensus makes sense as a core default, there is no reason to assume that it will be universally desired. A front-end developer may have any number of reasons for wanting to customize this logic, and that's what theme functions are for.

effulgentsia’s picture

Assigned: effulgentsia » Unassigned

Back to a community issue.

q0rban’s picture

> A front-end developer may have any number of reasons for wanting to customize this logic, and that's what theme functions are for.

No, theme functions are for customizing presentation, not logic.

effulgentsia’s picture

FileSize
11.31 KB
Failed on MySQL 5.0 InnoDB, with: 16,517 pass(es), 2 fail(s), and 0 exception(es). View
+++ includes/theme.inc	6 Jan 2010 18:19:58 -0000
@@ -2033,6 +2033,143 @@ function theme_html_tag($variables) {
+      if (count($groups) == 31 || (!empty($group) && $file['media'] != $media)) {
+        $groups[] = $group;
+        $group = array();
+      }

change count($groups) to count($group)

No, theme functions are for customizing presentation, not logic.

A blurry line sometimes, because sometimes logic is needed to customize the presentation. And considering that themes can implement alter hooks, I'd be ok with moving this to a drupal_alter() instead of a theme() if that's what folks want. But we are talking about a function that outputs HTML markup, only implements logic needed to achieve the desired markup from some input data and options, and can benefit from preprocess functions making adjustments to the input data and options first. Seems to me to be a pretty clear-cut case for a theme function.

effulgentsia’s picture

FileSize
11.64 KB
Passed on all environments. View

This one splits the logic portion into a template_process_stylesheets() function, leaving only presentation decisions in the theme_stylesheets() function.

effulgentsia’s picture

FileSize
11.64 KB
Passed on all environments. View

How did that typo get back in there? This one fixes it again.

donquixote’s picture

This patch does not add any new configuration settings to core. Instead a preprocess function can set $variables['max_tags'] to something smaller than 31 to accommodate #186. And I think it should be the responsibility of a module/theme to implement a preprocess function to do this if the module/theme is adding stylesheets through some mechanism other than drupal_add_css().

I still think the magic number should be 25-28 rather than 30 or 31. A custom theme is usually the first thing you do as a new Drupal developer, and the chance is low that you know about that wicked stylesheet limit and the preprocess hook. I think it won't make a big difference in performance.

I also think it is important to have a good documentation page explaining CSS aggregation, the IE stylesheet limit, and some performance statistics of @import vs link vs mixed. This page can be linked to from the settings page. With good explanations we can also afford more options than "Disabled" and "Enabled".

Lastly, there should be a warning shown with a summary of all settings that are not recommended on production environments.

mfer’s picture

Priority: Critical » Normal
Status: Needs review » Needs work

Why is this set to critical. We have had this problem in previous versions of Drupal the the problem was not critical in those releases? The original CSS Preprocessor went into D5 about 3 years ago. I fail to see why this is critical for core to fix now. It may be annoying in some cases but is it critical?

A few notes stated a bunch of time but still relevant:

  • http://drupal.org/project/issues/ie_css_optimizer is a working solution in contrib
  • Private files and CSS Preprocessing work together
  • It is a bad bad bad idea to have a production site without CSS Preprocessing enabled. Performance is reason enough and in D7 there is no excuse.

The option presented here of using @import presents the issue of JavaScript race conditions. D7 makes heavy use of JavaScript and in the places we do that there could be issues. Most of these issues we don't see in dev setups because of the high network speeds and low latency of the typical setup. The race conditions will show up when sites go live in intermittent ways many developers will have a hard time figuring out.

bleen’s picture

Status: Needs work » Needs review

@mfer: To my knowledge, there is no solution to this problem that does not involve using @import.

Back to "needs review"

I would also argue that just because this has been a problem for a long time does not mean its not a critical problem. I'll let others chime in though before changing back

hass’s picture

Status: Needs review » Needs work

This enables each group to be output within a single STYLE tag.

You missed that this is not possible. You MUST keep the order of the CSS files. You cannot merge them together by media type or you will loose the order of CSS and therefore cascading styles order and this WILL break themes.

mfer’s picture

@bleen18 There is a race condition between JavaScript and CSS that can occur using @import in the way suggested and that the patch is written to make happen. This is a deal breaker for the patch going into core.

The details are at http://www.stevesouders.com/blog/2009/04/09/dont-use-import/.

hass’s picture

@mfer: we are aware about the performance degradation... but missing files are more critical than slow loading in only one browser.

effulgentsia’s picture

Priority: Normal » Critical

My preference would be to go back to #198 with #211 added to make the n>$max_tags ones use conditional comments to be LINKs for real browsers and @import for IE.

I'm not persuaded by the performance concern in #206, as with this approach, the mix-and-match of LINK and STYLE only occurs when you're loading >31 files, so you've already decided that you don't care about performance.

I don't see any documentation that the race condition exists. The link in #232 says that scripts are LOADED before styles, but not that they are RUN before styles. Can someone find any evidence that they actually RUN before styles earlier in the DOM are fully applied?

Finally, with the approach I'm suggesting here, there would be no change relative to HEAD with respect to non-IE browsers, no change when not crossing over the IE limit, and it would replace the one condition remaining (>31 files on IE) from having styles not applied AT ALL to MAYBE having them applied later than scripts run. If we have proof that the race condition is real, then maybe that's not a good change (since it would result in intermittent bugs rather than reproducible bugs), but in the absence of such proof, I think the change makes sense and would be a net positive.

effulgentsia’s picture

Status: Needs work » Needs review

Priority change was not intentional.

mfer’s picture

Priority: Critical » Normal
Status: Needs review » Needs work

@hass my issue is not slow loading. The issue is the race condition I pointed to in #232. This is not about performance but JavaScript/CSS based functionality breaking in a site.

It's the kind of problem that will most likely not show up for developers with fast connections or even during the development of sites. It would show up for site viewers and be hard to diagnose. The kind of thing that slips through most QA testing.

DamienMcKenna’s picture

mfer: so it's either:

  • Make developers fix their CSS so aggregation can be enabled (preference)
  • Use this fix bug have a possible race condition for the (hopefully) small number of sites that don't run with aggregation on their production sites
  • Don't fix it and let sites be broken in IE during development or on production sites that don't have aggregation enabled and/or broken CSS.

Are you aware of any other options here?

mfer’s picture

I did a little digging into this for more detail and the race condition may not be an issue. If the style tag is before the script it should be ok. Though, I suggest someone test it.

My second reason for not liking this is the approach....

The solution for this should use a plugin style system to handle different possibilities. There is the way core does it now. There is the way this patch provides. There are still other ways people want to do this (see http://drupal.org/project/sf_cache). It is the kind of thing that should use a plugin (think handlers or components). Not the kind of thing that uses a preprocess function. There is a difference between having one thing that does something and it can be swapped out for a different way and using an event based system where everyone can act.

If we put this solution in now a good solution may get overlooked in D8. A good solution will change the APIs significantly so we can't do it now.

The initial code for this solution has existed for about a year so it will be trivial to get working when the D8 cycle opens up. I'll even code it.

So, what for now. With hook_css_alter and a preprocess function for theme_html_tag the solution we are debating here could be implemented in contrib.

q0rban’s picture

Can't we just enable css preprocessing by default and be done with it? We can add some documentation below the preprocess CSS checkbox that says, "Hey, turning this off may mess up styling in certain browsers."

mfer’s picture

@DamienMcKenna I am unaware of a reason for someone to not use aggregation. If people want to cache in groupings something like sf_cache should be used. I expect this to be ported to D7 since it's being used by nowpublic and they are committed to D7.

If someone is developing in IE and they want preprocessing turned off an existing module to help can be used or hook_css_alter/theme_html_tag can be used to create the solution we are talking about here.

I don't see why core should have a hack solution because of a browser issue.

DamienMcKenna’s picture

mfer: for sites that don't have broken CSS and can have aggregation on production, the issue is the development-time need to be able to run with aggregation turned off (so you don't have to clear the CSS cache every time you modify a file).

effulgentsia’s picture

What about this idea (some of this might have been suggested in earlier comments)?

1) We change the "Aggregate and compress CSS files into one file." setting into a radio group:
a) "Aggregate CSS files into fewer files while preserving the order of CSS rules" (default)
b) "No aggregation of CSS files" (useful while a theme is being developed, warning: if site has too many CSS files, IE will fail to load some of them)
c) "Aggregate CSS files for IE only" (useful while debugging a theme in non-IE browsers, but also needing for the site to be operational in IE and running into IE's limit)
d) "Aggressively aggregate CSS files into as few files as possible and compress them" (best performance for production site, warning: may change order of CSS rules)

2) In above, d) is what the current checkbox is and b) is what currently happens when it's off. I'm suggesting adding implementation for a and c. Implementation for c) would be to do the aggregation as in a) but then spit out conditional comments with lots of LINK tags for not IE and less LINK tags for IE.

3) This means no @imports. Only LINKs.

This would address the two use-cases for why people don't always run with aggregation on: because it's annoying for development/debugging, and because it changes order of CSS rules. a) would then be a sensible default.

effulgentsia’s picture

By the way, #242 could be done entirely within drupal_get_css() (except the UI change of checkbox -> radio): no need for a theme function or plugin/handler, so as per #238, wouldn't preclude a more proper architecture for D8.

mfer’s picture

@effulgentsia I like the idea of multiple options. But, why just these?

drupal_get_css() shouldn't be overloaded with if statements or cases. This should be done with plugins. Think of Views. The plugins let you choose from a list. People can write and add more options if they choose. This is the type of thing that needs to go here as well.

To do that properly it should be done in D8.

Jeff Burnz’s picture

Right now it seems like we need a practical solution, to my mind its this:

1. CSS aggregation ON by default, warning message explaining the issue and pointing to a book page which documents it with solutions etc.

2. Leave the fix in contrib for Drupal 7, there is IE Unlimited CSS and Style Stripper, maybe others.

I think leaving this in contrib gives time for the uber-elegant solution to flourish in contrib, ripe for D8 in a couple of years time.

For me this is most certainly an edge case, I can only think of one project in the past 6 months where this was an issue and we used Unlimited CSS module which was just fine for our development purposes.

If we really must have it, then #206 is the voice of reason here and gets my +1.

effulgentsia’s picture

Thinking about this some more:

1) The goal is to make Drupal as usable and error-free as possible for novice users who just install good modules and themes. It is unacceptable that someone who installs Drupal, leaves it in its default settings (aggregation off), and installs a bunch of modules that all work properly, but each one adds a css file, ends up having a site that doesn't work in the browser that the majority of people use. That's why I think this issue should be considered critical, but that's just my opinion, so I'm not changing the priority flag.

2) There are currently several reasons for why some people like aggregation off. This probably isn't exhaustive, but here's some of them:
a) In addition to aggregation, the setting also compresses (removes css comments and formatting).
b) In addition to aggregation, the setting also caches (a change to a css file doesn't "take" until you clear the Drupal cache).
c) The code in drupal_get_css() that does the aggregation sometimes results in changes to the order of CSS rules: all files without the 'preprocess' attribute explicitly FALSE are inserted in the DOM where the first of these files exists within the $css array, and all eligible files for the same media type are aggregated together, so the relative order of a.css (for media 'all') and b.css (for media 'screen') might not be preserved.
d) Not sure if this has been fixed in D7, but at least in D6, CSS files with different encodings (for example, one saved on Windows and one saved on Mac), when aggregated together, would sometimes cause problems. Possibly other kinds of "broken" CSS had/has similar problems.

3) We want to encourage people to turn aggregation on on production websites, because performance is horrible without it. Therefore, I think #2c and #2d aren't legitimate things for core to worry about, other than to make aggregation more robust within reason. I think it's totally reasonable for core to take the position that CSS files coded in a way where #2c or #2d cause problems are buggy files and should be fixed, not coddled. And better to catch the bug earlier in the process than only after turning on aggregation as part of site launch. If those were the only issues, I would recommend not having aggregation be an option, but always be on. If someone wants to maintain a contrib module that allows sensitive/buggy CSS files to avoid being aggregated, great. If right now, we removed the setting from core and always had it on, contrib modules would be able to do this, because they can implement hook_css_alter() and set each item's 'preprocess' to FALSE. This also allows a contrib module to implement its own custom aggregation logic to replace core's.

4) With regards to #2a and #2b, we could take the position that those needs should be addressed by the devel module or another contrib module. But, I also think it makes sense to allow efficient core development to not be so dependant on a contrib module. So, I suggest changing the current checkbox "Aggregate and compress CSS files into one file." to "Compress and cache aggregated CSS files", and have it be enabled by default, but can be disabled by someone who needs to do efficient development/debugging of CSS files. And change drupal_get_css() to always aggregate, but only compress and cache if that setting is enabled. Also, the uncompressed aggregation can insert comments into the aggregated files identifying which source file is inserted at that location, so someone debugging the aggregated CSS file within Firebug can easily find the source file that needs to be changed.

5) The benefits of #4 are:
a) IE problem fixed.
b) Default setting is the one that's appropriate to non developers. Developers can change the setting to sacrifice performance in order to make development more efficient.
c) Changing the performance setting on/off doesn't change CSS order, because aggregation would happen either way. The fewer differences between development and production environments, the better.
d) Without support for non-aggregation in core, people are more likely to not code CSS files that break when aggregation is turned on.
e) People really against aggregation can look to contrib for solutions.

Thoughts?

JohnAlbin’s picture

Priority: Normal » Critical

Yes, this is critical. By default, CSS aggregation is disabled. Which means if you have an RTL site (which nearly doubles the number of CSS files) or more then a handful of modules enabled, then you hit the 31 stylesheet limit pretty quickly. And, since theme CSS is loaded last, your site will appear mostly unstyled in all versions of IE.

And, unlike, Jeff Burnz, all (yes, ALL) of the sites I have developed in the past year have hit this 31 stylesheet limit.

I agree with the points that effulgentsia just posted. Actually, if we want to use the just-enable-CSS-aggregation-by-default patch I posted way back in #156 (which would need work now), the only people “harmed” by this would be developers/Firebug users. But if they look at the source, they'll see the aggregated giant-hex-named stylesheet. Why not add an inline html comment explaining what's going on? Something like: <-- CSS file optimization is enabled. See the handbook for details. drupal.org/node/whatev -->

One nitpick on:

Also, the uncompressed aggregation can insert comments into the aggregated files identifying which source file is inserted at that location, so someone debugging the aggregated CSS file within Firebug can easily find the source file that needs to be changed.

I'm constantly telling people to actually open up the CSS file and stop using Firebug exclusively, because Firebug doesn't display any CSS comments at all. That's why I was suggesting an inline html comment next to the link tag for the aggregated CSS file. Firebug would still need to turn on the "show comments" option on the HTML tab, but what are gonna do? I still use "View Source" quite a lot.

effulgentsia’s picture

So, just to summarize, I think we have 2 current options on the table:

1) #246-4 (change to have aggregation always on, change the performance checkbox to only control compression and caching)
2) leave HEAD functionality pretty much alone, change CSS aggregation to be on by default, add HTML comment as per #247, and documentation/warnings about all the horrors that will befall you if you turn aggregation off

With either option, D7 contrib and D8 can attempt more.

Any other options?

Jeff Burnz’s picture

If 1 then please please please have the option to disable aggregation (like IE CSS Optimizer). I do use Firebug along with the other 2.5 million active daily users!

Taking away the ability to see the stylesheet name and line numbers is a little crazy. Sorry, but Drupal can have 100 stylesheets no problem - Firebug is a Drupal themers godsend for sifting through this jungle of files.

If we have aggregation on by default there must be a way to disable it, via the UI.

donquixote’s picture

Priority: Critical » Normal

It's a pity we don't have a wiki to summarize the different options. This thread is becoming a pain to read through.
(manual pages are by default only editable by the author and some people with special rights, afaik.)

Some more thoughts.

Core vs contrib:
I think the purpose of contributed modules should be to extend core, not to fix foreseeable problems.
Having a hook for contrib module to alter the stylesheets inclusion sounds like a good idea, but I think we need a few reasonable and robust default options in core.

Priority: Critical or normal?
I think it's critical. We can expect a lot of sites to be affected, if they use a lot of modules.

Options?
I would suggest the following options:

Aggregate css?
* aggregate all CSS.
* don't aggregate any CSS.
* advanced settings (contrib?)

Advanced CSS aggregation options (contrib?):
(some of it should be checkboxes instead of radios, not sure)
- preserve the order of CSS rules (checkbox, on by default).
- aggregate for IE 6 and 7 only, not for other browsers.
- do not aggregate for the admin user.
- do not aggregate for the following subdomain. (contrib!)
- do not aggregate for the following roles (contrib!)
- do not aggregate theme CSS (contrib!)
- do not aggregate custom module and custom theme CSS (contrib. needs extra settings to define what is "custom")
- do not aggregate CSS from the following modules / themes (contrib!).
- do not aggregate for the following client IPs (contrib!).
- allow to change aggregation settings for a browser session (contrib!).

Note: To avoid problems with the _31_stylesheet_limit_in_Internet_Explorer_ (link to manual page), all CSS files above 27 will be included using @import instead of a link tag.

If we have more than 27 stylesheets (contrib!):
* use @import for stylesheets above 27.
* use @import for all stylesheets, if their total number is greater than 27.
* use link tags no matter how many. Warning: This will break in IE 6 and 7.
(and a note saying which of these is the recommended way to do it)

and a checkbox:
- only use @import for IE 6 and 7.

How many stylesheets before using @import? (contrib. default is 27.)

One combination of the "advanced" options could be provided as a third default option, that can be

Preserve order?
I'm not sure about this, but I think we need a robust behavior by default, and don't want to clutter the basic interface with a new option for this. Therefore I think we should make sure that the order is not changed by default. And if we need an option, put it in contrib or "advanced settings".

Problems with file encoding?
I think it's a matter of the compression algorithm to work with different encodings and preserve the most common CSS hacks. Ideally, the compressed CSS should behave in the same way as the uncompressed one. We should not outsource this responsibility to the theme developer.

Extensibility?
I think the best would be to have very simple hooks in core that let contrib modules replace the entire CSS aggregation thing, and then let contrib come up with more fine-grained API solutions.

Firebug vs "view source" ?
I much prefer using Firebug and look at the line numbers, than doing any "view source". The only problem I have with Firebug is the browser performance penalty - but that's not related to CSS. If we can handle the problems caused by @import settings, we should

Browser-specific (IE) settings?
I am not a big friend of this, because they make it harder to compare the behavior in different browsers.

donquixote’s picture

Priority change was not intentional.
I wrote this message before John turned it back to critical.

Jacine’s picture

Priority: Normal » Critical

Re #248: #1 would totally wreck the way I develop sites. I would be vehemently against that, so #2 wins for me hands down.

I have no problem at all with CSS aggregation being on by default. It's not a big deal to turn it off. It likely will not trip themers up for more than a couple of minutes the first time they come across it. Anyone who doesn't know what it is will learn and that is a good thing.

donquixote’s picture

I am not strictly opposed to turning CSS aggregation on by default, but please, don't sell that as a solution to the 31 stylesheets problem!

I will turn aggregation off for sure (for demo sites), and I want it turned off for all browsers including IE 6 / 7, to immediately see the results of CSS changes. And I tend to have tons of stylesheets in my own themes.

kevinquillen’s picture

I can't read all these posts, but how about a core addition to Drupal that takes all active stylesheets and puts them into one uncompressed file if aggregation is off, then loads that in the theme? Then you could have a simple GUI that lets you drag and drop the sheet ordering so rules are loaded correctly.

To me the only way you can solve this (easily) is to reduce the amount of stylesheets included -without- having to turn on aggregation.

Most people should/would want this on in production mode, but when its off it certainly can break IE and has caught me off guard more than enough times. So I wouldn't mind one large CSS file, so long as Drupal could keep track of that and aggregate it properly after changes have been made.

You could even have it smart enough to break it down to something like core.css, contrib.css, yourtheme.css. The first two alone would keep 20 some sheets from loading seperatly.

bleen’s picture

@stripped ... that is basically what aggregation does already (minus the GUI).

If you do read the posts you'll find several problems with this

DamienMcKenna’s picture

Just to mention it, (for my $0.02) I'm vehemently opposed to leaving a message for the admin that says (paraphrase) "sorry your site is broken in IE" without providing a fix when we know it will happen. The suggestion in #250 to provide a default option to load all CSS via @import when aggregation is disabled that could then be modified by contrib is a very sound concept - provide a default solution and ways to modify if needed.

mfer’s picture

Status: Needs work » Needs review
FileSize
411 bytes
Failed on MySQL 5.0 InnoDB, with: 16,644 pass(es), 15 fail(s), and 1 exception(es). View

@JohnAlbin because of the RTL issues the critical status makes sense.

I like the idea of having preprocessing on by default in the standard install profile. The global default should still be off. The attached patch enables preprocessing in the standard profile so expert/custom profiles that may not want it on by default do not have to turn it off.

mfer’s picture

FileSize
1.53 KB
Failed on MySQL 5.0 InnoDB, with: 16,643 pass(es), 10 fail(s), and 1 exception(es). View

The previous patch should fail tests. It seems the CSS tests assume preprocessing is disabled when run. This patch fixes that as well.

Status: Needs review » Needs work

The last submitted patch, optimize-css-default-228818-258.patch, failed testing.

webchick’s picture

Status: Needs work » Needs review

While it's the simplest implementation-wise, this is just throwing new WTFs in the way for everyone new to Drupal, and I'm not sure that's a good trade-off...

Something we worked very hard on this release was making it possible to create beautiful themes in Drupal with just CSS, without diving into template.php. We even added the Stark core theme to facilitate this.

New users are the ones most likely to install Drupal using the Standard profile. They'll immediately see that Garland is ugly, and then look to see what they can do about that. "Oh! Neat! A base theme I can start modifying myself! Let me go get Dreamweaver. WTF? Did I forget to Save? No... Did I forget to FTP? No... WHY DOES DRUPAL HATE ME?!?!"

Ick. :\

webchick’s picture

Status: Needs review » Needs work

Cross-posted with bot.

mfer’s picture

When preprocessing is disabled we could go with using @import. Using @import when preprocessing is disabled will cause worse performance than the current setup but will let us scale to many more css files in IE.

The problem may be compounded if a conditional stylesheet uses link and the other stylesheets are using @import.

donquixote’s picture

When preprocessing is disabled we could go with using @import. Using @import when preprocessing is disabled will cause worse performance than the current setup but will let us scale to many more css files in IE.

That's what others have been saying all the time. Almost.
Instead of "when preprocessing is disabled", the condition should be a magic number (27 for my taste). With aggregation enabled we will likely never hit the magic number, but in case we would, the mechanic should use @import.

The race condition (if it exists - so far I have only seen it mentioned, not explained) needs to be dealt with by theme developers, who need to make sure that scripts go after styles (or was it the other way round?). This can be encouraged by a PHP comment in the default page.tpl.php

Any "on by default" vs "off by default" is not a solution to the problem.

q0rban’s picture

> Any "on by default" vs "off by default" is not a solution to the problem.

I disagree. On by default means that when Joe User downloads Drupal, downloads a theme, downloads some modules he will never run into this problem. If we use @import when preprocessing is disabled he will never run into this problem. Granted it may cause even more performance issues, but all we need is some text underneath the CSS aggregation that says disabling it will impact performance.

DamienMcKenna’s picture

webchick: your use case could be improved by the suggestion from someone above to add a comment indicating the CSS has been aggregated; maybe there could be a system-wide setting that would optionally embed comments in the output for beginners to understand what they're seeing, e.g. "beginner_inline_help", that would default to TRUE for new installs?

kevinquillen’s picture

'On by default' made our designers b*tch to kingdom come. It's not something that jumps right out at people and says 'hey, changing CSS? turn off aggregation'.

donquixote’s picture

If we use @import when preprocessing is disabled he will never run into this problem.

Aside of the point that I prefer a number condition, this is the actual solution. No matter for the default for CSS aggregation, the @import solution is necessary in every case where we risk to have more than 31 stylesheets.

With a working (condition-based) @import solution in place, the question of aggregation on or off will still have performance implications, but will be irrelevant for the 31-stylesheets problem. It will be a matter of taste: Performance vs ease of development.

So: If we need the @import solution anyway (with a reasonable condition), why don't we focus on that?

bleen’s picture

@q0rban

> I disagree

I disagree with your disagreement ... 99% of Joe Users (that you are describing) will inevitably want to make *some* change to the CSS and he will (after some confusion, two google searches and maybe a hint from someone on IRC) turn off aggregation so it will be easier to find where to change that link from blue to green. I agree that aggregation should be on by default but:

a) it does not - in any way - constitute a solution to this issue
b) should be addressed in another thread

We have gone around and around in circles on this particular point. It's time to accept that there are enough compelling reasons to suggest that even if we turn on css aggregation by default, many many users will still face this issue and it will still need to be fixed.

#155, #156, #187, #206, #210, #218, #222, #242, #253, #263
UPDATE: #267, #268, #269

UPDATE: see spin off issue: #678160: Turn CSS aggregation on by default

mfer’s picture

On by default is not an option. This was discussed in IRC. If someone downloads drupal, sees garland, goes this sucks, starts creating their own theme, and sees problems from the cached css we have a problem.

Many designers/front end developers use tools like firebug. These tools don't show CSS comments so where is the benefit of them?

@donquixote The race condition is not an issue. Further discovery found that. Looking at the number of CSS files would be a good way to balance the @import usage.

effulgentsia’s picture

Assigned: Unassigned » effulgentsia

I'm giving this another stab. Will try to post a patch in the next couple hours.

Owen Barton’s picture

Assigned: effulgentsia » Unassigned

I think we are going around in circles. Even though I personally don't have an issue with always on aggregation, I agree it will really frustrate newbies who don't know about tricks like #77. I think #227 is the only patch that properly addresses all the issues discussed so far. I am not sure what outstanding criticisms there are of this:

(a) #229 by mfer that it might cause JS race conditions - this was addressed by mfer in #238
(b) #238 by mfer that it is not pluggable (or does not operate as a plugin) - however as it is it would not prevent sf_cache working the way it has been, and is in addition themable - of course if someone has a way to abstract it #227 more cleanly that is welcome.
(b) #228 by donquixote that the limit should be 25 or 28 - this I agree with. Not only to we have to worry about themers adding CSS tags in page.tpl.php, but also 3rd party embedded code in nodes or blocks, which I have sometimes seen include CSS tags (although more often this is pulled in through JS). Either way, it would be unwise to run so close to the limit and cause someone some major headaches when the cost or pulling back a little is pretty small - even better just wrap it in a variable_get().

Unless there is some blocker criticism that I have missed I think #228 looks like the best way to proceed.

Owen Barton’s picture

Unassignment was accidental - just a comment race (and obviously I can't fix myself!)

effulgentsia’s picture

Assigned: Unassigned » effulgentsia

:)

bleen’s picture

Assigned: effulgentsia » Unassigned

@Owen Barton: I agree with b) (well, b2) that we should probably wrap the magic_number variable in a variable_get and make it trivial for a developer or themer to change. That said, can we all agree on 28 as our default magic number (for safeties sake)?

@effulgentsia: since you wrote #227 and seem to be making changes.... what issues are you currently trying to address?

bleen’s picture

cross cross cross post ... sorry eff

bleen’s picture

For everyone who wants to weigh in on whether or not css aggregation should be on by default (or not) - having nothing to do with how it effects this IE 31 stylsheets issue, just generally - please see this spin-off issue: #678160: Turn CSS aggregation on by default, and please lets keep all the discussion about that there.

JohnAlbin’s picture

I was the first to suggest "css aggregation on by default" and I'm fine with not using it as a fix for this.

I think Crell's solution is best. Leave CSS aggregation off by default. If CSS aggregation is disabled, use <style> tags with a max of 31 @import tags per <style tag. If CSS aggregation is enabled, use <link>.

The problem with counting stylesheets in order to determine <link> vs. @import usage is the added complexity. And what would that solution buy us? As long as Drupal uses all < link> or all @import on any particular page load, the performance is the same.

But I just thought of a new wrinkle with Drupal switching between <link> and @import. What happens when a themer adds IE conditional comment styles to the html.tpl? See seven_process_html() and garland_process_html() We can't mix @import and <link> or we're going to screw up CSS loading performance in IE.

bleen’s picture

@JohnAlbin:

We can't mix @import and
or we're going to screw up CSS loading performance in IE.

I'm trying my absolute best to say this without getting back into the argument about css aggregation being on by default ... with that in mind:

The reality is that the 2 use cases we are worried about are:

aggregation = on

in this case, everything will be <link> so there are no worries

aggregation = off

In this case the site-admin is either developing/themeing (e.g. non-production site) and so performance is a non-issue (sucks, but not a problem) OR the site-admin doesnt care too much about performace or they would have turned on CSS aggregation.

right?

donquixote’s picture

If CSS aggregation is disabled, use

tags with a max of 31 @import tags per .
Now we want to limit the number of @import statements, instead of the <link> statements?? I'm puzzled.
The problem with counting stylesheets in order to determine vs. @import usage is the added complexity.
Nah..
And what would that solution buy us? As long as Drupal uses all < link> or all @import on any particular page load, the performance is the same.
The alternative would be: Always use @import. This would only work if all modules and themes use @import for explicit CSS inclusion. I doubt it.
What happens when a themer adds IE conditional comment styles to the html.tpl? See seven_process_html() and garland_process_html() We can't mix @import and or we're going to screw up CSS loading performance in IE.
This penalty would only happen with CSS aggregation disabled. With CSS aggregation enabled, we will hardly hit the magic number or any of the conditions that would trigger @import. We don't care about IE performance of development sites, but we want that it works. We can show a warning to please enable CSS aggregation - similar to the "rebuild theme registry on every page". Ideally, summed up in one single warning, to limit the annoyance factor. All said before. If a theme uses @import inside conditional comments, well, then we can't help it, it's the themer's fault. Themes should do <link>, not @import, for explicit CSS inclusion.
donquixote’s picture

@bleen18 (#278):
that's how I see it, yes.

effulgentsia’s picture

Assigned: Unassigned » effulgentsia
FileSize
13.08 KB
Failed on MySQL 5.0 InnoDB, with: 16,686 pass(es), 0 fail(s), and 1 exception(es). View

Here's a work in progress. It doesn't yet pass tests. I need to take a break, but will resume in a little bit with polishing it and then posting a comment here explaining what it does at a high level and why I think it addresses all (or almost all) needs. For anyone interested in reviewing the code in the meantime, please feel free.

bleen’s picture

Assigned: effulgentsia » Unassigned

@donquixote:

Now we want to limit the number of @import statements, instead of the
statements?? I'm puzzled.

** everything below this line is unconfirmed, but needs to be checked **

it appears that, in addition to the limit of 31 (32?) stylesheets, there may also a limit on the number of @imports in a given <style> or on a given page. Incidentally, there may also be a max filesize of 288k/stylesheet. BLAAAAAAARRRRGGG!!

http://acidmartin.wordpress.com/2008/11/25/the-32-external-css-files-lim...
http://social.msdn.microsoft.com/Forums/en/iewebdevelopment/thread/ad1b6...
http://stackoverflow.com/questions/1082849/ie-question-how-many-css-incl...
http://joshua.perina.com/africa/gambia/fajara/post/internet-explorer-css...

donquixote’s picture

FileSize
1.62 KB

@bleen18 (#282)
That's odd. wtf.

I made a little script to play with. It shows that all the different possibilities have a limit at 30 or 31, but you can have 30 <style> tags each filled with 30 @import statements.

I wonder what exactly the "unlimited CSS" module does to circumvent the problem. The same trick? Will it break at 31x31+1 stylesheets?

I think we could live with a solution that allows 30x30 or 31x31 stylesheets, but the clean way would be to totally get rid of any such limits and allow truly unlimited CSS (until memory melts).

donquixote’s picture

Ok, great, this is unlimited_css.module:
- make packages of 25 @import statements each
- wrap each of them into a separate <style> tag.

All of this happens in unlimited_css_preprocess_page().

We could go further than that and allow second-level inclusions, but that's really not worth the trouble I think. If we can get a mechanic equivalent to unlimited_css in core, with reasonable conditions to decide when to start using @import statements, we are done.

donquixote’s picture

Status: Needs review » Needs work

I had a look at the issue queue of unlimited_css.module.

The most common problem of this module is conflicts with other modules that use hook_preprocess_page. This will be less likely to happen if we put the thing in core, where it does not depend on hook_preprocess_page.

Another request was to only do this for IE browsers. I think this could be done with "advanced conditions" in contrib.

Another one: "Does not preserve order of CSS" - hrm. Needs to be investigated.

----------

Now, about the conditions, and all @import vs mixed @import + <link>.

The unlimited_css.module has a strict all-import policy, as I understand. Even if the total number is less than 25.

We could have a magic number condition (25-28), and we could have a mixed solution if we go beyond the magic number.

For $n stylesheets:
floor($n/25) style blocks with 25 import statements each.
$n%25 stylesheets included with link tags.

If that mixed solution is good for anything. Which has been questioned somewhere above.

EDIT:
Just to get the math right:
$m = $n-3 (substitution)
26 - floor($m/24) link tags.
floor($m/24) - 1 style tags with 25 stylesheets each.
One more style tag with $m%24 + 2 stylesheets.
(if the magic number is 25, we allow a mix, and we want to keep the number of link tags maximized.)

jwilson3’s picture

For $n stylesheets:
floor($n/25) style blocks with 25 import statements each.
$n%25 stylesheets included with link tags.

If that mixed solution is good for anything. Which has been questioned somewhere above.

The mixed solution is desirable and link tags should be used at all costs because @import statments inside style tags is the equivalent of putting link tags at bottom of the page. Since CSS is required to style the page, this will cause the dreaded FOUC error (mentioned waaaaay above).

It is claimed that at least one link tag will prevent FOUC in Internet Explorer, but I think the more the better. The pseudo code quoted above barely covers this for the case of $n = 26.

I think ideally 25 is a great number of link tags (to allow room for additional link tags in template),

for $stylesheets :
  split $stylesheets array at index 25 into $link_tags and the rest put in $import_statements
  split $import_statements into two dimensional array "$style_tags"  every 30th index
  foreach $link_tag in $link_tags:  append a link tag to $output
  foreach $style_tag  in $style_tags:  append a style tag to $output
return $output

thoughts?

Jeff Burnz’s picture

#286 see #206, there are performance implications when mixing link and @import. An empty script tag has long been the standard fix for the FOUC.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
18.66 KB
Passed on all environments. View

Getting this in bot's queue. Will follow up with a comment summarizing it.

Crell’s picture

Status: Needs review » Needs work

Can anyone who is still talking about performance please *read* this thread before replying? Performance does not matter here. At all. When aggregation is enabled, we use link and have one file per media type (typically). If you care about performance in the slightest, you'll have aggregation enabled and this entire thread is a non-issue.

This is an issue *only* when developing, for which counting milliseconds of HTTP requests is *completely irrelevant*. I'd even argue that FOUC is probably a non-issue there as well during development. Bug-free behavior is the only concern here.

I repeat: There is no performance question here. None. It is irrelevant. If you think there is, read the thread again until you realize why there is not.

Crell’s picture

Status: Needs work » Needs review

Sigh.

effulgentsia’s picture

As the many comments in this issue attest to, it may well be impossible to find a universally acceptable solution, so we need to have a good default solution, and hooks for modules to customize what they don't like. #288 attempts to accomplish that.

1) The basic summary of this issue is that the drupal_get_css() function in HEAD is broken. When aggregation is disabled, it results in a high likelihood of a site not working in IE. Even worse, the only ways for a contrib module to fix this are to either implement hook_css_alter() and remove items from the $css array (possibly aggregating them in the process ignoring the site-wide setting that has it disabled: LAME) or to implement hook_preprocess_html() and set $variables['styles'] = SOME_OTHER_FUNCTION_BECAUSE_DRUPAL_GET_CSS_IS_BROKEN();: also LAME. I don't think anyone here would consider either of these as "solutions" that we should be proud of.

2) Additionally, drupal_get_css() returns output that potentially is in violation of the order specified in the $css array. But only when aggregation is enabled. So it returns a different order depending on if aggregation is enabled. Which means, people can develop something that works when aggregation is disabled, then enable aggregation and have it not work, so they then keep aggregation off and their site runs horribly slow. And this is not overridable except via the hook_preprocess_html() hack above.

3) So, this patch refactors drupal_get_css() to support more extensibility. Conceptually, drupal_get_css() in HEAD consists of these steps: get and finalize the $css array, arrange the items into groups to support aggregation, perform the aggregation, then generate markup. This patch makes those steps clear.

4) This patch does the group arrangement (resulting in the potential re-ordering of rules as in #2) regardless of whether aggregation is enabled or not. Enabling aggregation should result in performance improvement only, not cause different functionality.

5) Because the way core implements group arrangement may not be the way that is universally desired, this patch adds a hook_css_groups_alter() so modules can do something different, like the radical concept of actually preserving the rule order in $css. Or, implement more sophisticated grouping ala http://drupal.org/project/sf_cache.

6) hook_css_groups_alter() can also specify a "preprocess handler" and "markup handler" to override what core does next.

7) This patch adds a default "preprocess handler" to do the aggregation of the groups prepared in the previous step. An alternate handler may want to aggregate differently (for example, not compress, or cache differently).

8) This patch adds a default "markup handler" to generate the LINK and STYLE tags. LINK tags for anything that wouldn't be aggregated even if aggregation is enabled. LINK tags for aggregate files. STYLE tags for groups of files that can be aggregated, but aren't because the setting is disabled. If a module doesn't like this strategy, it can override the handler.

9) The logic in #8 results in equivalent markup as HEAD when aggregation is on (all LINK tags). It results in mix-and-match LINK/STYLE when aggregation is off. As per #289, if this is sub-optimal performance, who cares? The alleged race condition possibility brought up in earlier comments has been debunked. As far as FOUC, according to http://bluerobot.com/web/css/fouc.asp/, it doesn't happen as long as there is a single SCRIPT tag in the HEAD, and in D7, there always is.

So: any feedback?

Jeff Burnz’s picture

Crell, for me its not the microsecond HTTP request issue, its the parallel vrs sequential loading issue + loss of progressive loading. If the page takes just a moment longer to load this is pita for me as a themer when I am doing thousands of reloads week in week out. I am not talking about this as server performance issue or and end user issue, I am talking about this from a theme developers perspective, well, mine at least.

donquixote’s picture

Status: Needs work » Needs review

@jrguitar21 (#286):
I fixed the math :)

@Crell, Jeff Burnz:
Performance has a different priority on production sites than it has on development sites.
This doesn't mean that performance on dev environments is irrelevant. Time is money! It's a tradeoff with different weights, but it is a tradeoff.

I still don't feel smarter from the different statements about @import vs link vs mixed. Could someone summarize?

@effulgentsia (#291):
Point 4) This patch does the group arrangement (resulting in the potential re-ordering of rules as in #2) regardless of whether aggregation is enabled or not.
This makes a lot of sense to me!

effulgentsia’s picture

@Jeff, unless there's documentation to the contrary, all browsers except IE load in parallel regardless of tag used. Using certain combinations of LINK and STYLE result in sequential loading for IE only. So, this would only be an issue if you're a developer, AND doing lots of reloads in IE, AND developing on a remote server instead of your local machine. Furthermore, I suspect (though http://www.stevesouders.com/blog/2009/04/09/dont-use-import/ doesn't make this completely clear) that all @imports within a single STYLE tag are loaded in parallel even in IE, and this is the most common situation with #288, since the vast majority of CSS files are eligible for aggregation. Yes, on a site with some files ineligible for aggregation, you would have LINK tags mixed in, and these would be done sequentially, but I doubt this would really cause that much of a problem, and if it does, I'm sure we'll see a contrib module that implements an alternate "markup handler".

effulgentsia’s picture

I still don't feel smarter from the different statements about @import vs link vs mixed. Could someone summarize?

Here's the issue: Only IE uses sequential instead of parallel loading in some situations. There's no evidence yet posted that other browsers base their loading logic on the tag used. For IE, all LINKs is best, because it results in the most parallel loading. But it's not an option when aggregation is off (unless we compare how many CSS files are needed with some magic number that needs to take into account what else is on the page that drupal_get_css() doesn't know about -- a perfectly ok thing for a contrib module, but kind of ugly for core). According to http://www.stevesouders.com/blog/2009/04/09/dont-use-import/, all @imports within as few STYLE tags as possible would likely be next best (so, for example, if we have 40, put 31 in one and 9 in the other). But that page doesn't actually post metrics for that, so it's only a guess. But, we can't mix and match @imports of different media types (cause IE7 doesn't support that), so we would need at least one STYLE tag per media type. We don't know, but suspect that everything in one STYLE tag would need to be downloaded before IE starts downloading the ones in the next STYLE tag. The article claims (and has supporting evidence) that @imports in a STYLE tag won't load until LINK tags above it are loaded, but doesn't say what happens if you have multiple LINK tags above it (presumably based on other evidence, all LINK tags near each other would load in parallel). So, my assumption is that all consecutive LINK tags load in parallel, then when a STYLE tag is encountered, all prior loading must finish and then all @imports within the same STYLE tag are loaded in parallel, but must all finish before the next STYLE tag or set of LINK tags can start. Assuming this is true, #288 probably isn't the most performant, but is pretty close, because all files eligible for aggregation (even if aggregation is disabled) are combined into 1 or several neighboring STYLE tags, and the files ineligible for aggregation (because they were explicitly added with 'preprocess' set to FALSE when drupal_add_css() was called or are for external URLs) are likely to be near each other in neighboring LINK tags which get loaded in parallel. Further performance testing and tweaking would require time, and given what I said in #294, I think it makes more sense to let contrib figure that out, since really, we're kind of shooting in the dark here.

hass’s picture

I don't like this counting stuff very much as it must be incompatible somedays somewhere. Have someone tried to create one css added with link to the page that links inside this css file with @import? Not sure if the same limitations apply than... Maybe it's possible to have 1000 @imports inside this one master include file.

This may solve the flashing effect and allow shift+reload in dev... Never tested myself yet... Only an idea!?

mfer’s picture

aspilicious’s picture

mfer nice link! Is the latest patch doing this?
Then i'm definitly in to it.

effulgentsia’s picture

FileSize
23.6 KB
Passed on all environments. View

This is a cleaned up version of #288. High-level summary on #291 (only slightly obsolete now). Biggest change relative to #288 is removing hook_css_groups_alter() and just extending hook_css_alter(). Comments in patch should be clear: if not, please provide feedback.

Personally, I think this is ready to fly. First person to provide code review gets to be comment #300. Seriously though, alpha is coming fast: this needs to get to RTBC status quickly. Timely code review would be much appreciated. Thanks!

JohnAlbin’s picture

Status: Needs review » Needs work

This doesn't address the problems with our core themes I mentioned in #277. Both Garland and Seven will end up with mixed @import and <link> tags by default.

Also, this patch seems to have a lot more +'s in it then -'s. What's the performance hit? There are simpler ways to do the same thing.

Pages