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 KBbleen18
Passed on all environments.
[ View ]
#193 css_overflow.patch6.28 KBbleen18
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 KBbleen18
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

effulgentsia (#295)

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

hass (#296)

I don't like this counting stuff very much as it must be incompatible somedays somewhere.

There are two (unusual) use cases for the counting idea:
1) We have aggregation disabled and still have a relatively low number of stylesheets. This could happen with a theme that disables a lot of the default stylesheets, or on pages where a lot of the usual stylesheets do not apply.
2) We have aggregation enabled and nevertheless have a high number of stylesheets (more than 31). The reason could be if many modules declare that their stylesheets should not be aggregated, or if we have a high number of explicit stylesheet inclusions that Drupal doesn't know about.

These cases might be theoretical, but they show that a magic number is a more robust thing than relying on the aggregation setting. In the average case, both will have the same effect, if the magic number is sufficiently low.

Thanks for the review, John. Copying over your comment from #277:

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.

This problem exists in HEAD. drupal_get_css() currently outputs all LINK tags. If a theme then outputs STYLE tags for conditional IE CSS, that's the theme's problem, and should be addressed in a separate issue. garland_get_ie_styles() currently outputs one of each, which is just plain stupid. So, let's assume it's the theme's responsibility to stick to LINKs if it cares about performance, and if core themes aren't currently doing that, a new issue needs to be filed to fix them.

Now, if a theme wants to be intelligent and output LINK tags if drupal_get_css() outputs LINK tags, and STYLE tags if drupal_get_css() outputs STYLE tags, then it can call drupal_get_css() and pass it an array of $css items instead of rendering the tags itself. This would also let it benefit from aggregating multiple IE-specific stylesheets.

Also, please see #295. If we have to use STYLE tags at all (and we do when aggregation is disabled), do we have any evidence that mixing sets of LINK tags with STYLE tags is any slower than using multiple STYLE tags. http://www.stevesouders.com/blog/2009/04/09/dont-use-import/ says nothing about this. I suspect that having aggregation disabled at all has a page load time impact that FAR overshadows any impact created by mixing LINK and STYLE tags in the way that's done in #299. If you have evidence to the contrary, please post it.

Also, this patch seems to have a lot more +'s in it then -'s. What's the performance hit?

Virtually none. drupal_get_css() is called only once per page request (at most 2 or 3 times if a module/theme wants to use it for adding conditional CSS or CSS within the BODY). And it already has to either call md5() because of file aggregation or theme() a whole bunch of times to render each css item individually. The function call overhead from 3 extra functions and a couple extra walks of $css/$css_groups is nothing compared to this.

There are simpler ways to do the same thing.

Yes, but not in a way that supports extensibility well (see #291). And this issue has generated a lot of opinions and disagreements on what the "right" way is. We have actual use-cases for wanting to override grouping logic, override aggregation logic, and override render logic. But maybe there are improvements that could be made to #299. By all means, please offer suggestions.

Status:Needs work» Needs review

From #301:

These cases might be theoretical, but they show that a magic number is a more robust thing than relying on the aggregation setting. In the average case, both will have the same effect, if the magic number is sufficiently low.

The purpose of core is to implement a default solution that's reasonable, stable, maintainable, and bug-free except in rare edge cases, and to allow for override so the edge cases can be dealt with or use-case-specific optimizations can be employed.

The case of a site that wants to run without aggregation and has a few stylesheets receives 1 STYLE tag with #299, and would receive a bunch of LINK tags with a magic number based solution. Even according to http://www.stevesouders.com/blog/2009/04/09/dont-use-import/, this makes no difference performance-wise.

The case of aggregation enabled and still exceeding 31 total files is extremely rare, and we can't predict ahead of time what the needs for such a custom and complex website might be. Anyone creating that kind of website would be perfectly able to implement a function that overrides the _drupal_css_render() default to achieve what they want.

On the flip side, a magic number based solution would require hard-coding the magic number into core code (and depending on edge case, any hard-coded number might cause undesired behavior) or exposing at as a setting, ideally with a UI for it. I'm totally fine with a magic number based solution living in a contrib module, and #299 would make that easy. I doubt that all of us participating in this issue will reach consensus on the details of a magic-number based solution for D7 core, but if opinion swings that way, I'll be happy to go along with it.

StatusFileSize
new554 bytes
Unable to apply patch aggregate-css-with-filemtime.patch
[ View ]

I haven't had a chance to read through this thread yet, but based on the thread title, I believe this patch should solve the problem of developing for IE or a site that needs aggregated stylesheets.

The patch adds an array item called 'modified' with the filemtime value of the CSS file to the options array when building the list of CSS files in drupal_add_css. When the CSS aggregation under performance is turned on, this allows all CSS files to be compiled into a single file whose name is hashed by the serialized options array (and query string). Now that the modified time is in this array, you can always have aggregation turned on, but still be able to modify your stylesheets without needing to clear caches to see the updated aggregated file.
The aggregated stylesheet updates when any of its component sheets become modified.

tl;dr: You never have to turn CSS aggregation off.

I will still turn it off, to see the correct line numbers in Firebug.
The 'modified' timestamp is an interesting approach, which could be useful, but it doesn't really solve the problem discussed in this issue.

Status:Needs review» Needs work

The last submitted patch, aggregate-css-with-filemtime.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new23.6 KB
Passed on all environments.
[ View ]

@JonathanRoberts: I think #304 is quite interesting and would make for an excellent contrib module (re-implemented as a hook_css_alter() implementation). I don't think it's good for core, because it adds disk IO requests to every page request. And there remain reasons why people would want aggregation off anyway (as per #305).

Re-uploading #299 as the current patch needing review, which fixes what this issue needs fixed. @JohnAlbin, I'm still very interested in your thoughts on this regarding #302.

Subscribed.

@JonathanRoberts - making aggregation smarter is not what this issue is about - some people will still want separate files for finding code from Firebug.

I already posted a patch along the lines of what you were proposing at #678292: Make CSS and JS aggregation sensitive to file changes and be enabled by default - review there would be very welcome :)

I love the direction taken in #678292: Make CSS and JS aggregation sensitive to file changes and be enabled by default. Aggregation on by default with this method would put this issue to bed and please everyone IMO. (Tempted to mark duplicate)

@q0rban:

I too love the direction taken in #678292: Make CSS and JS aggregation sensitive to file changes and be enabled by default ... I think it's a great idea, however I believe firmly that it does not solve this issue. There several legitimate circumstances (many of them described throughout this thread) in which people will want to turn off aggregation - Drupal still needs to work in those circumstances - even in IE.

StatusFileSize
new29.53 KB
Passed on all environments.
[ View ]

Following up on #300 and #302, this patch changes garland and seven to add IE conditional CSS correctly: by calling drupal_add_css(). For a contrib module that decides to override the _drupal_css_render() function with a custom handler in order to implement a magic number based solution, this will allow that module to have an accurate count of how many tags are being added.

I added 'html_prefix' and 'html_suffix' optional properties to css items, to support the above.

Just to summarize, here's what this patch solves:

  • In all but the extremely unlikely situation that even under aggregation we cross the 31 limit, this results in a fix to the 31 tag problem. As per #303, in whatever complex website has more than 31 files even with aggregation turned on (perhaps because of adding many external CSS files that can't be aggregated), the developer can override _drupal_css_render() with a custom handler to solve their particular use-case.
  • When aggregation is on, this results in all LINK tags, as currently in HEAD, including IE conditional CSS added as per above.
  • When aggregation is off, but all CSS files are eligible for aggregation (no external ones or ones added with an explicit 'preprocess'=>FALSE option), this results in as few STYLE tags as possible (new STYLE tags only for new media types or different IE conditional comments or when needed to not have >31 @imports per STYLE tag). This includes IE conditional CSS added as per above.
  • When aggregation is off and there is a mix of files eligible for aggregation and ones that aren't, then there is a mix of STYLE tags (one for each set that would be aggregated if aggregation were enabled) and LINK tags (for ones that are ineligible for aggregation). This lets the developer know by viewing source which files will be aggregated together when aggregation is enabled. It also decouples the LINK/STYLE decision from the global aggregation setting, making it more compatible with modules that may want to implement alternate aggregation logic. While this might not always result in the absolute most optimal page load time in IE, I think it's close, and if someone believes there's a use-case where it's not sufficiently performant, please post the exact use-case you're concerned about and benchmarks, and consider that anywhere where every last drop of performance really matters, aggregation will be enabled.
  • This patch introduces a lot of extensibility to how grouping, aggregation, and LINK/STYLE rendering is done. Contrib modules wanting alternate behavior can implement that alternate behavior much more easily than currently in HEAD.
  • Beyond the strict scope of this issue, this patch also fixes an inconsistency bug between having aggregation enabled and not. In HEAD, enabling aggregation can result in different CSS order than when it's disabled. This patch results in the same CSS order regardless of whether aggregation is enabled.

StatusFileSize
new29.58 KB
Passed on all environments.
[ View ]

Tiny change to #312. Only difference is in this one, the garland and seven IE conditional CSS are added with the 'preprocess'=>FALSE option. There is no reason to add an expensive md5() call needed for aggregation to every page hit, just to aggregate 1 file (or, if RTL, 2 files). Note that these are files not aggregated in HEAD, so with this patch, no change with respect to that decision is introduced. It also allows better benchmark comparisons between HEAD and the patch, because both have the same number of md5() calls.

I'm confirming what I said in #302 in answer to JohnAlbin's question about impact of patch on server performance: with this patch, none. With aggregation enabled, #312 was very slightly slower than HEAD, but that was entirely due to the extra md5() call which this patch removes.

It does result, however, in a situation where if aggregation is disabled, you have STYLE tags followed by LINK tags (for the IE conditional CSS). But we still have no evidence that this is any slower on IE than STYLE tags followed by STYLE tags.

subscribe

Are we asking the wrong question?

I'm sorry if something like this has been suggested, but I could only bring myself to read about the last 100 or so comments. In any case, I think I see a workable option. Consider:

  1. we already have a way to aggregate stylesheets in core, and
  2. aggregating stylesheets neatly solves this problem at a single stroke, but
  3. aggregation interferes with site/theme development (i.e. because stylesheet edits are not reflected in aggregated stylesheets until after caches are cleared)

But consider also that stylesheets provided by modules should never be edited (i.e. since upgrades would destroy such edits). Thus if we could optionally aggregate only theme and/or module stylesheets into single stylesheets (I'd suggest module stylesheets be aggregated by default and the aggregated version updated as modules are enabled, disabled or uninstalled), we should be able to solve the issue without seriously impeding development (I gather there are issues with private downloads and aggregation but I'm not sure of the details...hopefully it's not an insurmountable problem...)

If I might editorialize a bit, I think this problem points to a problem with how Drupal modules are just able to add stylesheets willy-nilly whether the site developers/themers/administrators want them or not. To the best of my knowledge, module-supplied stylesheets can be overridden in the .info file, but they can't simply be deactivated. I'd like to see one (or both) of the following:

  • A way of excluding module stylesheets in the theme info file, and/or
  • A checkbox in the module administration page of every module that provides a stylesheet to allow that stylesheet to be disabled (obviously we couldn't force this, but it could be added to the various module development tutorials and documentation).

Comments? Errors in my thinking or assumptions?

-- b

But consider also that stylesheets provided by modules should never be edited (i.e. since upgrades would destroy such edits).

you're forgetting module developpers here ;)

Not at all. That's why I said "optionally aggregate." As far as I can tell, doing this would solve virtually all the problems listed and doesn't seem like an insurmountable problem. Have I missed something obvious?

@bedlam: I think your idea is good and already exists as a contrib module: http://drupal.org/project/ie_css_optimizer. And regardless of how this issue is solved, you're correct that there are use-cases for more sophisticated aggregation logic than what comes in core, and this too exists as a contrib module: http://drupal.org/project/sf_cache. I also think your idea of stopping module-included CSS files from being included is interesting, and can be implemented in a contrib module, using D7's new hook_css_alter() hook. I suspect it's too late to implement that for D7 core, though if the idea is flushed out well in a D7 contrib module, no reason not to try getting it into D8 core.

But as you say, "optionally aggregate", which means there remain use-cases for not aggregating (some developers want access to CSS source file and line number when debugging in Firebug, and this includes both theme developers and module developers). Meanwhile, patch #313 (summary in #312) solves the IE limit issue without relying on aggregation being enabled (when aggregation is off, it combines links to files that would have been otherwise aggregated, into one or a few STYLE tags instead of using a LINK tag for each one). That patch still needs a code review, and I would love to get feedback from John Albin as to whether he has any remaining concerns with it. Or, if anyone else following this issue has concerns with it, please share them.

+function garland_init() {
+  // Add conditional CSS for IE6.
+  drupal_add_css(path_to_theme() . '/fix-ie.css', array('weight' => CSS_THEME, 'html_prefix' => "\n<!--[if lt IE 7]>\n", 'html_suffix' => "<![endif]-->\n", 'preprocess' => FALSE));
+}

+// The theme system does not automatically call hook_init() for themes, so it
+// must be called when this file is loaded.
+garland_init();

+function seven_init() {
+  // Add conditional CSS for IE6.
+  drupal_add_css(path_to_theme() . '/ie6.css', array('weight' => CSS_THEME, 'html_prefix' => "\n<!--[if lt IE 7]>\n", 'html_suffix' => "<![endif]-->\n", 'preprocess' => FALSE));
}

+// The theme system does not automatically call hook_init() for themes, so it
+// must be called when this file is loaded.
+seven_init();

Could not these be in hook_preprocess_page() ? It seems to work fine, but maybe I am missing something.

I'm adding all theme css files in hook_pre_page

StatusFileSize
new32.88 KB
PASSED: [[SimpleTest]]: [MySQL] 17,533 pass(es).
[ View ]

Could not these be in hook_preprocess_page() ?

Thanks! That's much better. Even better, they can be in hook_preprocess_html().

HANDLERS for adding CSS files?

As much as I LOVE YOU, effulgentsia - but this stretches our input and return on investment calcs a bit too much.

ROI? WTF?

HANDLERS for adding CSS files?

:-D

AFAIK, that CSS file limit is still apparent in IE7, and Drupal 7 loads many more CSS files than Drupal 6, so this overall bug is still somewhat critical.

Quick fix: Enable CSS aggregation by default.

@322, please don't enable CSS aggregation by default. Themers will rage.

I very much appreciate your attention on this, sun. Yes, handlers. Here's why, and in the process, let me make the case for the ROI on this.

Simply enabling aggregation by default was rejected by webchick in #260. Even if you enable it by default, you would still have it be an option to disable, to make development easier. But consider a developer who wants it disabled in order to, for example, view source file and line numbers from within Firebug, but let's say that developer also wants to spot-check to make sure things look right in IE. Having to toggle the aggregation setting every time the developer switches from using Firebug to spot-checking in IE sucks.

Meanwhile, there is a way to simply fix the issue which is to use STYLE tags with @import statements instead of LINK tags. But that has down sides (#206), so many comments in this issue were devoted to opinions on when to use which one, but without any consensus. Seems like there's no single answer that will make everyone happy in all circumstances. What do we do in that situation? We try to pick a good enough answer, and make it overridable. How to make it overridable? I suggested a theme function in #227, but mfer correctly pointed out in #238 that a handler would make much more sense than a theme function.

So that's the origin of the 'render' handler. But if we need to change drupal_get_css() to be less hard-coded by using handlers, let's fix the other logic in drupal_get_css() that's annoyingly hard-coded. If you have the following:

drupal_add_css('a.css');
drupal_add_css('X.css', array('preprocess' => FALSE));
drupal_add_css('b.css');

With aggregation turned on, a single aggregate file will be made with a.css and b.css and the LINK tag for it included before the LINK tag for X.css, resulting in a different CSS rule processing order than what was asked for. Is that a bug (IMO, yes) or a feature (hey, it's more optimal to have fewer css files, and does the rule order really matter that much)? If we consider it a feature, we should provide some mechanism for a module to override this "feature". That's what the "group" handler does. Conveniently, it also allows modules like http://drupal.org/project/sf_cache to have very straightforward implementations.

Then there's the aggregation and compression logic itself. On several sites I've worked on, I wanted the ability to add comments within the aggregate file identifying the source file before that file's contents. But, sorry, no way to do that without hacking core.

Since the "group" and "aggregate" handlers are beyond the scope of this issue, I can re-roll the patch with just the "render" handler. But I'm not sure the "I" in the ROI calculation is significantly more with 3 handlers than with 1 (I suspect the refactoring of drupal_get_css() at all is your main concern).

The "R" in the ROI calc is a fix to a critical bug in a way that doesn't require developers to use aggregation. IMO, that justifies the time needed to review and polish the patch.

@322, please don't enable CSS aggregation by default. Themers will rage.

Themers won't rage if we can figure out how to verify/fix any slowness issues in #678292: Make CSS and JS aggregation sensitive to file changes and be enabled by default

Please, stop it!

I am on of those who will rage.
For reasons that have been repeated over and over in this thread.
Yes, I want to inspect all CSS files (including core modules) with Firebug, to see which file I need to override. And at the same time I want to be able to test the site in IE.

Sniffers are not a nice solution either, not bullet-proof afaik (has been discussed probably).

The one reasonable way to do it is to use @import if the number of CSS files is larger than.. let's say 20. This means, 11 stylesheets that can still be defined explicitly, circumventing this mechanism. Yes this is a magic number and as such it stinks, but 31 is a magic number too. The limit for explicit stylesheets (those that circumvent this mechanism) will always be something less than 31, so we always have a magic number limit - but now we make this number more predictable.

The condition, "more than 20 stylesheets" does typically trigger only then if CSS aggregation is off. If it is on, we will hardly have more than 20 stylesheets. And if we still have that many, it is only reasonable to use @import.

The same idea has been discussed before with higher numbers than 20, but:
- Smaller is safer.
- We don't win much by choosing a higher number. With CSS aggregation on, the number will always be smaller than 20, or otherwise there is something seriously wrong.

----

The unlimited_css module gets very close to it, but unfortunately it has some problems with stylesheet overrides and stylesheet file order.
#693180-2: Overriding module CSS files does not work with unlimited_css
The post contains a new version of unlimited_css, that attempts to solve these problems (this is "needs review", and has already received a complaint). The same algorithm could be used in core, to replace drupal_get_css().

----

I support the theme hook idea, and I would probably support the handler idea if I had any idea what handlers mean in Drupal. That said, we still need a good default, and the @import > 20 could be that good default.

I am one of those who will rage. I want to inspect all CSS files (including core modules) with Firebug, to see which file I need to override. And at the same time I want to be able to test the site in IE.

So you are going to rage about something that is not possible currently? ;) You sound like what is proposed is a step backward.

It is possible with the unlimited_css module (or with the patched version, if it works as advertised).
I think this should be in core, because people should not have to install a module just to fix something that is broken.

After rereading alot of posts once more I think agregation by default and the possibility to disable agregation of singe files (or maybe core/theme/contrib modules in groups as well) ist the best sulution we can get.

* Makes Drupal fast by default.
* Results in Drupal working with every browser by default.
* Provides an easy way for themers to "check out" files they are working on.

It has been well settled in several posts throughout this thread that turning on aggregation by default is not a solution to this issue. Please see http://drupal.org/node/228818#comment-2446150 for details and references to the posts that explain why.

A spin-off issue has been created at #678160: Turn CSS aggregation on by default if you want to discuss turning css aggregation on by default for performance reasons, but PLEASE PLEASE PLEASE do not set this issue back by suggesting that this is a solution to the IE 31 link/style tag limit.

I am one of those who will rage. I want to inspect all CSS files (including core modules) with Firebug, to see which file I need to override. And at the same time I want to be able to test the site in IE.

me too.

Aggregration on by default? Sure!
Combining multiple @import commands in style tags when Aggregration is turned off? Sure!
Unable to sense with firebug? NO WAI!
Breaking Design in IE without Aggregration? NO WAI!
Switching Aggregration on and off each time i'm switching between firebug and IE? NO WAI!

We have already determined that we can add a maximum total of 961 stylesheets when we add 31 style tags with 31 @import commands in each of them (optimal usecase scenario).

Sure, it isn't pretty but it works.
And besides, when Aggregration is turned on again after development, it is all pretty again.
It doesn't matter how it is impemented, as long as i can see each CSS file seperate during development.

Really, a bit slower during development isn't a problem, as long as it doesn't influence production.

This issue is not about giving themers a great experience by allowing complete control over theming (firebug inspection, etc.) at the same time as supporting IE. This issue is *only* about supporting IE when there are more than 31 stylesheets. Therefore, having aggregation on by default alongside the patch in #678292: Make CSS and JS aggregation sensitive to file changes and be enabled by default *is* a solution, and a very good one IMO.

Is it the silver bullet solution for all Drupal theming wtfs? No. But it's not supposed to be. We can open separate feature requests for making the theming experience even better for drupal, but if we keep trying to turn this issue into some sort of silver bullet approach, I feel pretty confident that no solution will be committed for the actual issue at hand!

@#332: Even if aggregation is sensitive to changes, many (most?) themers will still disable it first thing because development without inspecting filenames in firebug can be a pain. Therefore, I don't think it's really a good solution at all. In fact, if it's enabled by default, then IMO it's a step backwards (because it will require me to disable it for development which is just an extra step).

@332, giving themers a great experience by allowing firebug inspection, etc. at the same time as supporting IE. is one of the best usecases of supporting IE when there are more than 31 stylesheets.

having aggregration enabled by default together with filechanges sensitivity and not being able to have seperate CSS files is one step forward on the IE front and two steps backwards on the overall front.

Remember, we have already determined that we can add a maximum total of 961 stylesheets when we add 31 style tags with 31 @import commands in each of them (optimal usecase scenario), so why aren't we (ab)using this?

That may be true, but making themers' lives easier doesn't make this a critical bug report. The fact that you end up with stylesheets missing in IE, on live sites, because they installed a few modules and didn't enable aggregation, is. So if themer experience is what makes this patch complex with over 300 replies, then we should commit one of the simpler solutions, then solve this at a less urgent priority afterwards. If it's not, then why hasn't someone marked the current patch RTBC?

I like the idea in #332, too. It would simply work, the only side effect is - we loose the original file and line number... Better than a broken page in IE and without compression the code is still readable. If there is no other reliable solution it may be the best workaround.

Why is this going in circles all the time?
Themer experience is not an issue, so we choose the inferior of two equally simple solutions? Wtf??

And, aside themer experience:
- We enable agg. by default.
- Joe themer disables agg.
- Joe themer forgets to re-enable aggregation.
- Site is shown to the client. Poof!

so we choose the inferior of two equally simple solutions?

Can you refresh my memory about the other simple solution? If you're referring to using import instead of link, I don't think that's a good option without lots of testing. We really don't know the implications of having 31+ stylesheets loaded with @import: http://www.stevesouders.com/blog/2009/04/09/dont-use-import/

So, yes, there are other options, but they are options that require lots of testing.

themers will still disable it first thing because development without inspecting filenames in firebug can be a pain.

I agree that this is a pain. Here we need to choose the lesser of two evils. Is the greater evil for themers to have to disable aggregation, or for themers to have no idea why their site looks like spit in IE and spend hours and hours troubleshooting the problem?

- We enable agg. by default. - Joe themer disables agg. - Joe themer forgets to re-enable aggregation. - Site is shown to the client. Poof!

This is true, however this solution is trying to fix situations like this:

- Joe themer knows nothing about aggregation or the 31 stylesheet bug.
- Joe themer shows the site to the client. Poof!

or

- Joe noob knows nothing about Drupal and enables all 4000+ contrib modules on his site
- Joe noob wonders why Drupal is so ugly
- Joe noob googles "Joomla"

or

- Joe dev enables a contrib theme and carefully selected list of modules
- Joe dev knows nothing about the 31 stylesheet bug and forgets to enable aggregation
- Joe dev shows the site to the client. Poof-bang!

That may be true, but making themers' lives easier doesn't make this a critical bug report. The fact that you end up with stylesheets missing in IE, on live sites, because they installed a few modules and didn't enable aggregation, is.

I'd agree, but enabling aggregation by default isn't a solution (even if aggregation was sensitive to file changes). It would have to be disabled for development (for anyone who uses Firebug), and when that happens, the original problem is still there.

The whole problem that we're trying to solve is that themes break in IE during development. A solution that only works AFTER development (i.e., turning aggregation on by default and committing #678292: Make CSS and JS aggregation sensitive to file changes and be enabled by default) isn't a solution at all (except for the minority of DIY'ers who download Drupal, use a default/contrib theme, and don't know about aggregation).

mcrittenden is exactly correct. We are trying to solve this problem as it occurs during development for IE.

IMO aggregation should be on by default, and I've said so at #678160: Turn CSS aggregation on by default but this issue would not be solved in that case because any experienced themer would tell you that the first thing he/she would do when when developing is to turn that off in order to use the basic tools available to themers out there (ex. Firebug, IE Developer Toolbar, etc...).

These tools are indispensable to themers and any solution we come up with here *MUST* keep that in mind.

Imagine we told module developers that in order for their module to work correctly in IE they had to turn on a setting which would prevent them from using any IDE/TextEditor except notepad. It would be an unreasonable expectation and no one would advocate for it.

I dont know enough about handlers vs. hooks vs. API to comment on the patch in #321 except to say that I have tried to apply it and it seems to work great in all the use cases that have been outlined in this thread. It ensures that production sites with lots of stylesheets dont break; aggregation still works as intended; it does not hinder the work or themers who (by necessity) turn off aggregation. Can we please focus on the virtues (or lack thereof) of that patch instead of having the same argument that was pretty well settled over 100 comments ago?

If you're referring to using import instead of link, I don't think that's a good option without lots of testing. We really don't know the implications of having 31+ stylesheets loaded with @import: http://www.stevesouders.com/blog/2009/04/09/dont-use-import/

A bunch of people already do it, using the unlimited_css module.

unlimited_css has a few bugs, but they are not caused by @import - they are caused by:

  • unlimited_css uses hook_preprocess_page to override the $styles string. Other modules or themes can override the $styles string again, with a new call to drupal_get_css(), so in this case unlimited_css will have no effect. We are less likely to get this issue, because we would modify drupal_get_css() directly.
  • unlimited_css uses a flawed algorithm to build the array of CSS files. The problem is fixed in the dev snapshot.

I am using unlimited_css (modified version) at my projects, and it works.

The linked article really only talks about performance. Nothing of that is relevant for us.

----------

If we really want to be safe, we could put a checkbox on the performance page, saying "use @import if there are more than 20 stylesheets, to circumvent the 31 stylesheet bug in Internet Explorer."

The main use case for this checkbox would be custom themes and modules that use regex to extract stylesheet information from the $styles string - not a good idea, but who knows how many people have already done that and now don't want their sites to break.

We could add another checkbox "only use @import for IE6." I think this would be too much, and should rather be done in contrib, or in a second step. I would not do server-side browser sniffing by default, because then every cache module (boost, core, ..) would need to cache by user agent (if it doesn't already do). See #652690: Only process CSS if the browser is IE.

------

I can't say much about #321, because I have not made myself familiar with Drupal+handlers.

But: If we really want a flex point, it would be nice if it could also serve other use cases such as modules that want to cache and gzip CSS files, or add parameterized CSS etc (as much as makes sense).

We could add another checkbox "only use @import for IE6."

I definitely wouldn't recommend this, not only because cache modules would have problems but any site using a CDN (ex. akamai) would have a whole new level of pain introduced.

I can't say much about #321, because I have not made myself familiar with Drupal+handlers.

It sounds like our current problem is that no one who is familiar (and invested) in this issue has much experience with Drupal+handlers except effulgentsia ... hmmm

Can we instead have this handled by hook_css_alter()? This is becoming a very ugly solution to a very ugly problem that shouldn't exist in the first place... Turn on CSS Aggregation.

@catch:

...then why hasn't someone marked the current patch RTBC?

It seems like the problem here is that themers aren't core-savvy enough to do a patch review on patches like #321, and core devs (i.e., the ones who can do a proper review) aren't themers so "turn on CSS aggregation" seems like a good solution to them.

By the way, I don't know if we need to do anything here; Drupal has been just fine with solutions like these living in contrib until now. But I do know that enabling aggregation won't solve anybody's problems.

We could add another checkbox "only use @import for IE6."

I definitely wouldn't recommend this, not only because cache modules would have problems but any site using a CDN (ex. akamai) would have a whole new level of pain introduced.

I'm curious - how exactly would this interfere with a CDN ?

EDIT:
Me understands.
I was thinking of a CDN for js and css files. But I think you are talking about a CDN for html. This makes a lot of sense now.
Of course, if you use a CDN, you will probably enable aggregation. But that's no excuse to let it break if you don't.

Can we instead have this handled by hook_css_alter()?

This hook does a very different thing. It acts while we are still dealing with an array of CSS files. We want to change the way it is printed to html.

----

It sounds like our current problem is that no one who is familiar (and invested) in this issue has much experience with Drupal+handlers except effulgentsia ... hmmm

Yes..
A little explanation by effulgentsia would be nice, with a few links where I can learn about Drupal+handlers. I'm not totally against reviewing, I just don't have the time to search for this information or extract it from issue comments.

And to those who don't like another flex point: I would be happy without that, if we could agree on the solution outlined in #326. The handlers can come in another step.

devs aren't themers so "turn on CSS aggregation" seems like a good solution to them.

For the record, I am a themer and a developer. I already run into this problem, but as any themer does, you just find your own way to work around the issues.

I already run into this problem, but as any themer does, you just find your own way to work around the issues.

That's my point. If we're going to leave it like it is, then fine, themers will get by with fixes in contrib and whatnot. But if we're going to "fix it" in core, then it actually needs to be fixed, and aggregation by default isn't a fix.

@Rob Loach:

Can we instead have this handled by hook_css_alter()?

Rob, you're one of the people who can give an in-depth review of #321, and I would so appreciate it if you did. In fact, hook_css_alter() plays a role in that patch. It's extended with an additional parameter to allow the necessary overrides of code that is currently hard-coded into drupal_get_css(). If you mean, can it be handled by a contrib module without any core changes, then only with some kind of aggregation logic similar to what is done by http://drupal.org/project/ie_css_optimizer. The only way with current HEAD for a contrib module to replace LINK tags with STYLE tags in order to allow >31 CSS files is to implement hook_process_html() and override $variables['styles']. #321 fixes this situation by allowing hook_css_alter() to specify handlers to use for overriding what is currently hard-coded and broken logic in drupal_get_css().

@catch:

...then why hasn't someone marked the current patch RTBC?

I agree with #344. The people in this issue who care most about its solution are theme developers without the in-depth Drupal expertise to code review #321. The people who can do a code review are ok with the "enable aggregation by default" answer, but even webchick is not ok with that answer (#260). And to be fair, people are busy with other issues. I take as much responsibility for that, since I've also been too busy with other issues, and haven't gotten into IRC to aggressively solicit reviewers.

@catch:

...then we should commit one of the simpler solutions, then solve this at a less urgent priority afterwards.

It's not like the issue being "critical" has really resulted in it being treated as urgent. The issue was opened two years ago, and #313 has been sitting there for over 5 weeks. After posting #313, I got on IRC and talked with webchick about getting some reviewer attention on it before alpha1, and she said "Why is this on your alpha1 hit list? It can be done after alpha1, and there's more pressing issues to resolve for alpha1". I'm not saying this was the wrong answer, just pointing out that at no point in this issue's history have I gotten a sense that anyone has considered it urgent. However, some theme developers are quite passionate about wanting a solution that doesn't hinder their development process.

@donquixote:

A little explanation by effulgentsia would be nice, with a few links where I can learn about Drupal+handlers.

To be honest, I'm a little surprised that handlers are the sticking point. I'd like an explanation as to why that is. They're really not that complicated. Here's a quick summary of what they are. Throughout Drupal, we try to make core as overridable as possible. The primary mechanism we use to achieve this is hooks. When code in core calls module_invoke_all($hook, ...), every module has a chance to implement the hook do something at that point in the processing. There's a special case of this, which is hooks that end with "_alter", and these get invoked by calling drupal_alter(), but they basically work the same way. The key point being that there can be >1 modules implementing the hook, and they all get to run. Another way we provide overridability is through theme functions (or templates, but for this discussion, I'll assume functions). Here, there's only 1 function that will ultimately be used (either the theme's implementation if there is one, or a base theme's implementation, or the default theme_*() function, but only one of these). But, as q0rban points out in #224, we reserve theme functions for things that output "presentation", and one could argue that the use of a STYLE tag vs. a LINK tag is not a "presentation" decision (though I believe the line is blurry here). So, that leaves the missing piece: what do we do when we want to provide a mechanism for a single function that doesn't deal with presentation decisions to be overridable? Don't want to use a theme function since those are designed for presentation. Don't want to use a hook, since those are designed to potentially have multiple implementations that all get to run. The answer: handlers. A handler is a single, swappable, function that runs at a particular point in the process. An example is the 'page callback' property of a hook_menu() implementation. Modules set this to a function to run for a particular Drupal path. Modules can change this via hook_menu_alter(), but for any given menu item, there is a single 'page callback' function. When menu_execute_active_handler() runs, it calls that single function.

So, that's basically all that #321 does. It extends hook_css_alter() with a second parameter that can be used to specify overrides of default handlers: one handler for the grouping logic for determining which files should be grouped together (if aggregation is enabled, these files get aggregated into one file, and if aggregation is disabled, these files get combined into as few STYLE tags as possible), one handler for the aggregtion step itself (the code that does the generation of an aggregate file, strips comments and whitespace, etc.), and one handler for the rendering of the LINK or STYLE tags, since someone may want to override the default logic that determines when to use which one. It basically takes the stuff that makes drupal_get_css() hard-coded, and makes it pluggable. And in the process, uses STYLE tags when aggregation is disabled to fix this issue.

For anyone joining late, #312 has additional information that's still valid for #321.

Just to summarize, I think we have 3 ways we can proceed, but it would be great to find a way to pick one that we can agree on and pursue it as a unified community. There's a time for constructive debate, but I think we're nearing a time where we need to unify if we want to see any solution implemented.

1) Implement the "quick fix" of making aggregation on by default. This would in fact fix this issue, and a new non-critical issue could be opened titled something like "Theme development experience on sites using >31 CSS files really sucks". However, let's think about how "quick" this "quick fix" really is. We'd need to add some text for the "aggregation" checkbox, saying something like "if you disable this, your site might not work in IE". And given webchick's comment in #260, we'd need to resolve #678292: Make CSS and JS aggregation sensitive to file changes and be enabled by default, which as I said on that issue, would at the very least require introducing yet another performance setting (or, the "aggregate" checkbox could become a 3-option radio choice: "disable", "enable with file change auto-checking", "enable without file change auto-checking").

2) Get code review and polish for #321, which I believe addresses every concern that has been raised in this issue, except #322: that it would take too much time to get the necessary code review. Regarding #338:

If you're referring to using import instead of link, I don't think that's a good option without lots of testing. We really don't know the implications of having 31+ stylesheets loaded with @import.

Even the article you reference says there are no implications to doing it for any non-IE browser. Also, prior to Drupal 6, @import is what core used, so it has been subject to lots of testing, including all the sites currently running Drupal 5. And John Albin has tested this thoroughly: http://john.albin.net/css/ie-stylesheets-not-loading. Furthermore, #321 only does this when aggregation is disabled (patch introduces no change at all to markup when aggregation is enabled), and even Crell who was the driving force for #145218: Use href instead of @import for CSS is ok with @import being used when aggregation is disabled (#185). Finally, we DO know what the implication is of >31 LINK tags: site fail. How is changing something that's known to be broken to something that may have theoretical implications that have not surfaced in all of the above a step backwards?

3) If handlers are the sticking point in #321, then we could pursue something along the lines of #326: a hard-coded solution that's easier to get code review for.

Any other ideas?

Thanks for the explanation of handlers (#348). I thought there is more about it :)
I had a look at the patch, and I'm happy with the handler concept.

Some comments:

1) I would use a magic number solution for _drupal_css_render(), but I have no strong opinion about it. Some weird handlers for group and aggregate could result in a bunch of files that are all aggregated, and would need to be done with @import.

2) In _drupal_css_render(), two groups can be joined into one style+@import tag, if they have the same media type. And two groups can be swapped if they have two media types that never occur at once (I think that is all media except "all", is it?). This could result in better packaging into style+@import sections, especially if there are many groups.

3) Someone argued that mixing @import and <link> would be worse than using only @import. What is the status here?

4) drupal_render vs performance: What's the benefit of passing a renderable array as return value, if all that happens to it will be an inevitable drupal_render() ? Where is the benefit?

Btw, the renderable tree would be fun if used like this:

<?php
class DrupalRenderable {
  public
$tree = array();
  public function
__toString() {
    return
drupal_render($this->tree);
  }
}
?>

This thing could be returned and passed around instead of strings, and would allow subsequent theme calls to play with the raw data ..

Thanks, donquixote.

Thanks for the explanation of handlers (#348). I thought there is more about it :)

Well, there can be. Crell goes into a more sophisticated analysis here: http://www.garfieldtech.com/blog/drupal-handler-rfc. If someone who's up on these kinds of jargon details thinks I'm using the word "handler" inappropriately in the context of the patch, please let me know, and I'll change the word to "callback" (or whatever other word you recommend) instead.

Re #350.1 and #350.2:
I think both of those ideas make sense to explore in contrib as overrides of the default _drupal_css_render() function, but I'm nervous about adding any unnecessary complication to the default function. My goal with the default function was to implement as simple a decision as possible, which I think is "if when aggregation is enabled, files would be combined into 1 file, then when aggregation is disabled, group them into 1 STYLE tag instead (or >1 STYLE tag if necessary to stay within the 31 @imports per STYLE tag limit)". I didn't want the default render function to change any grouping decisions made by the "group" function, or any decision of which groups to aggregate vs. which not to aggregate made by the "aggregate" function. I simply want it to take unaggregated groups, and use STYLE tags for them in order to avoid the too many LINK tags problem. But I don't want this to be a hold up. I'm open to any implementation of _drupal_css_render() that solves this bug and that the community can agree to.

Re #350.3:
Please see #295 and #302. The issue is strictly IE-performance when aggregation is disabled, there's no evidence for there being functional problems in any browser or performance problems in any non-IE browser, and with aggregation enabled, you get all LINK tags. Unfortunately, no one has yet posted test results for different combination of multiple STYLE tags vs. STYLE tag followed by multiple LINK tags or LINK tags followed by a STYLE tag. There's no option that results in fully parallel downloading in IE, but which of these results in the most parallelism is unconfirmed. Barring clear data that shows one approach clearly superior to another, I don't think we should worry about it. If we get clear data, we can always open a new issue to performance-optimize _drupal_css_render(). If that data comes after D7 is released, someone will write a contrib module to implement whatever is most performant in IE.

Re #350.4:
See #313. While it's true that a drupal_render() call has some overhead, it's tiny when only done a very few times: in this case, once per call to drupal_get_css() plus once per LINK/STYLE tag (since drupal_render() calls drupal_render() on each child in the array), so maybe as much as 0.1ms could be saved by just returning a string that got built by calling theme('html_tag') directly for each tag rather than by returning a render array. The benefit, however, of using render arrays is it's consistent with the D7 approach to rendering: keep things structured as render arrays as long as possible and render to HTML string as late as possible. For example, this allows someone to create an override of _drupal_css_render() that calls _drupal_css_render() and then alters the render array before returning it back to drupal_get_css().

Re #351:
Render arrays as they are are pretty entrenched in D7 at this point. But it's an idea worth exploring for D8. I suspect performance would be an issue as PHP objects tend to be slower than PHP arrays, but it would be interesting to try it and see what the impact is.

I've decided that "handler" is too-overloaded a word. Actually so is "plugin". For the moment the evolved version of that RFC is called "components" unless I can come to an agreement with merlinofchaos about the name "plugin". :-)

Absent a broader acceptance of the terminology for many-to-one extensibility and a standardized mechanism, I'd recommend "callback" for a function-based approach.

I have no comment on the rest of the code or approach at this time. I'm trying to ignore this thread given how needlessly long it's gotten. :-(

@effulgentsia (#352):

Re Re #350.4:
I think the overhead, if any, is not in drupal_render() but in theme('html_tag'), as opposed to plain hardcoded string concatenation. I think theme('html_tag') is far too generic for targetted overrides. But you are right, it does make sense in combination with drupal_render().

Re Re #351:
"PHP objects tend to be slower than PHP arrays"
I think "Some operations on PHP objects are slower than similar operations on PHP arrays." would be more accurate. On the other hand, I would guess that polymorphic method calls are faster than building a string and finding the function with that name (that's not what we do here, it was just an example).
In #351 only one object is involved, and most of the operations will deal with nested array elements. So I guess the OOP penalty would be less severe.
On the other hand, we get a lazy evaluation benefit in case the render tree is discarded. Of course, we already did spend the time to build that tree.
Anyway, that's another issue.

"OO is slower" is a gross over-simplification. Basic operations are virtually identical. Object instantiation varies widely with the object. See:

http://www.garfieldtech.com/blog/magic-benchmarks
http://blog.samboyer.org/blog/microbenchmarking-php-performant-code-now-...

I don't quite get why a class is even entering this discussion, though. We're not changing render API at this point for D7, and we really need to avoid talking about D8 in this issue lest this issue not get fixed until D8.

Status:Needs review» Reviewed & tested by the community

FOR FUCKS SAKE (sorry about that...)

1)
http://www.stevesouders.com/blog/2009/04/09/dont-use-import/ is a red herring

Using @import in IE causes the download order to be altered. This may cause stylesheets to take longer to download, which hinders progress rendering making the page feel slower.

While http://support.microsoft.com/kb/262161

All style tags after the first 30 31 style tags on an HTML page are not applied.

well, which one do you prefer, a page that "feels" slower in IE, or the page plainly not working in IE?
http://john.albin.net/css/ie-stylesheets-not-loading

Also remember, multiple @imports within a single style tag download in parallel in all browsers, it is just that multiple @imports in different style tags do not download in parallel in IE.

2)

+++ includes/common.inc 2 Feb 2010 19:22:03 -0000
@@ -3265,75 +3269,368 @@ function drupal_get_css($css = NULL) {
+  // Regardless of the order of the items in the $css array, we order all the
+  // 'file' types ahead of the 'external' types, and the 'external' types ahead
+  // of the 'inline' types.
+  // @todo Why? We need a comment explaining why we do this. It's not at all
+  //   clear what the benefit is.

This is so we can override core and module stylesheets (which can't be changed)

Powered by Dreditor.

3)
RTBC #321, with the following notes:
- point 1 has already been documented and solved in the patch there.
- documentation in point 2 should be fixed.

StatusFileSize
new32.89 KB
PASSED: [[SimpleTest]]: [MySQL] 17,907 pass(es).
[ View ]

Same as #321, but with 'handler' replaced with 'callback' as per #353.

Re #356.2: No. There's already a weight system within drupal_add_css() for what you're talking about. The @todo is about why rules in css files external to the website should always take priority over rules in css files internal to the website, regardless of weights applied when calling drupal_add_css(). But the behavior is a pre-existing condition and isn't being changed by this patch, so it shouldn't hold it up. But eventually, we should document why this is done, or fix it to not do it, and that's what the @todo is for.

I still haven't properly looked at the patch, every time I look here there's another 20 followups to read. But making it pluggable, and calling it a 'callback' as opposed to a 'handler' seems fine. If the @import stuff is just FUD that's a nice bonus, but if people care at all about performance they should have aggregation on anyway. It's also nice that this helps some of the advanced css aggregation modules in contrib, you shouldn't have to _alter() runtime things to make them faster ideally.

@webchick: assuming this will now come across your eyes, summary of patch is in #312 and that summary is still valid for #357. The discussion from #312 on may be of interest to you as well.

Status:Reviewed & tested by the community» Needs review

Regarding antoniodemarco's solution (#101), I've made a small change to your code as it didn't include conditional stylesheets. Stick this in template.php in YOURTHEME_preprocess_page:

<?php
  $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 . $vars['conditional_styles'];
    }
  }
?>

Tested in 6.15.

Status:Needs review» Reviewed & tested by the community

Please don't change the status of this issue unless you have a problem with the latest patch. There's a Drupal 6 module which covers this, the fix here won't be backported because it depends on completely different APIs. The last thing this issue needs, at over 330 replies, is random status changes.

Uh, not sure how that happened, I didn't touch any of the issue status settings. Sorry about that.

+++ includes/common.inc 17 Feb 2010 01:46:58 -0000
@@ -2896,6 +2896,12 @@ function drupal_add_html_head_link($attr
+ *   - 'html_prefix': Markup to add before the XHTML tag that includes this
+ *     stylesheet. This is primarily used for adding conditional stylesheets
+ *     for IE.
+ *   - 'html_suffix': Markup to add after the XHTML tag that includes this
+ *     stylesheet. This is primarily used for adding conditional stylesheets
+ *     for IE.

What are other uses cases? If it is only used for conditional stylesheets for IE, we don't need to provide that level of abstraction. If we don't have another use case, a simpler, more intuitive API might be feasible.

+++ includes/common.inc 17 Feb 2010 01:46:58 -0000
@@ -2967,23 +2975,20 @@ function drupal_add_css($data = NULL, $o
+  // Allow modules to alter the css items. Also allow them to override the

'css' should be 'CSS' in documentation.

+++ includes/common.inc 17 Feb 2010 01:46:58 -0000
@@ -3001,75 +3006,368 @@ function drupal_get_css($css = NULL) {
+ * Once drupal_get_css() has an array of css items, the items then need to be

See above.

+++ includes/common.inc 17 Feb 2010 01:46:58 -0000
@@ -3001,75 +3006,368 @@ function drupal_get_css($css = NULL) {
+ *   An array of css items, as returned by drupal_add_css(), but after

css -> CSS

+++ includes/common.inc 17 Feb 2010 01:46:58 -0000
@@ -3001,75 +3006,368 @@ function drupal_get_css($css = NULL) {
+  // @todo Why? We need a comment explaining why we do this. It's not at all
+  //   clear what the benefit is.

We need to resolve this @todo because I had the exact same question reviewing this patch. It is hard to review it properly if I don't understand the reason behind the grouping logic.

+++ includes/common.inc 17 Feb 2010 01:46:58 -0000
@@ -2896,6 +2896,12 @@ function drupal_add_html_head_link($attr
+ *   - 'html_prefix': Markup to add before the XHTML tag that includes this
+ *     stylesheet. This is primarily used for adding conditional stylesheets
+ *     for IE.
+ *   - 'html_suffix': Markup to add after the XHTML tag that includes this
+ *     stylesheet. This is primarily used for adding conditional stylesheets
+ *     for IE.

What are other uses cases? If it is only used for conditional stylesheets for IE, we don't need to provide that level of abstraction. If we don't have another use case, a simpler, more intuitive API might be feasible.

+++ includes/common.inc 17 Feb 2010 01:46:58 -0000
@@ -2967,23 +2975,20 @@ function drupal_add_css($data = NULL, $o
+  // Allow modules to alter the css items. Also allow them to override the

'css' should be 'CSS' in documentation.

+++ includes/common.inc 17 Feb 2010 01:46:58 -0000
@@ -3001,75 +3006,368 @@ function drupal_get_css($css = NULL) {
+ * Once drupal_get_css() has an array of css items, the items then need to be

See above.

+++ includes/common.inc 17 Feb 2010 01:46:58 -0000
@@ -3001,75 +3006,368 @@ function drupal_get_css($css = NULL) {
+ *   An array of css items, as returned by drupal_add_css(), but after

css -> CSS

+++ includes/common.inc 17 Feb 2010 01:46:58 -0000
@@ -3001,75 +3006,368 @@ function drupal_get_css($css = NULL) {
+  // @todo Why? We need a comment explaining why we do this. It's not at all
+  //   clear what the benefit is.

We need to resolve this @todo because I had the exact same question reviewing this patch. It is hard to review it properly if I don't understand the reason behind the grouping logic.

+++ includes/common.inc 17 Feb 2010 01:46:58 -0000
@@ -3001,75 +3006,368 @@ function drupal_get_css($css = NULL) {
+ * The grouping logic implemented by this function is to group all files that
+ * are eligible for aggregation and are for the same media type into a single
+ * group. This results in the fewest possible aggregate files when aggregation
+ * is enabled. However, in some cases, it can result in a change to the

We should add a sentence explaining why we group by media type.

+++ includes/common.inc 17 Feb 2010 01:46:58 -0000
@@ -3001,75 +3006,368 @@ function drupal_get_css($css = NULL) {
+ * relative order of css items from what is specified in the $css array (for
+ * example, when a file that is ineligible for aggregation is between two files
+ * that are). A module wanting to always preserve relative order or wanting more
+ * control of which files get aggregated together can register an alternate
+ * group callback in hook_css_alter().

This solution sounds pretty painful to me, and looking at the implementation of the grouping function, a nightmare to override. To be honest, to me this makes it sound as if we inadequately addressed the problem and that our grouping approach is broken by design.

Status:Reviewed & tested by the community» Needs work

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new33.63 KB
FAILED: [[SimpleTest]]: [MySQL] 17,894 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

With #364. Straight to RTBC for Dries to review.

We need to resolve this @todo because I had the exact same question reviewing this patch. It is hard to review it properly if I don't understand the reason behind the grouping logic.

This solution sounds pretty painful to me, and looking at the implementation of the grouping function, a nightmare to override. To be honest, to me this makes it sound as if we inadequately addressed the problem and that our grouping approach is broken by design.

I agree with both of the above. It was hacky code to continue emulating HEAD's broken behavior. This one is both cleaner in implementation and makes more sense by design. With this code, the default grouping function preserves the order of items in $css, so that we finally fix HEAD's "aggregation changes CSS rule order" bug explained in #324.

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new33.11 KB
PASSED: [[SimpleTest]]: [MySQL] 17,900 pass(es).
[ View ]

Fixed a broken test, allowing remaining @todos to be removed from the patch. Also improved some comments.

Status:Reviewed & tested by the community» Needs review

Rule of thumb. Never RTBC your own patches.

Even when it's a minor change to an already RTBC patch, where the change is in direct response to Dries's review (#364) on an issue that's already proven extremely challenging to get any core developer reviewers for?

Status:Needs review» Reviewed & tested by the community

+++ includes/common.inc 24 Feb 2010 19:41:55 -0000
@@ -2651,6 +2651,17 @@ function drupal_add_html_head_link($attr
+ *   - 'conditional_comment_expression': An expression to control which versions
+ *     of IE should load the CSS. For example, setting this to 'lt IE 7' makes
+ *     the CSS load in IE versions less than 7 only; this is the standard way
+ *     of adding an extra CSS file that compensates for the poor standards
+ *     support in IE6. See http://en.wikipedia.org/wiki/Conditional_comment.
+ *   - 'conditional_comment_downlevel_revealed': If TRUE, non-IE browsers will
+ *     load the CSS regardless of the conditional comment expression. Otherwise,
+ *     if there's a conditional comment expression, regardless what it is,
+ *     non-IE browsers will not load the CSS. For example, to add a CSS file for
+ *     IE8+ and non-IE browsers, set 'conditional_comment_expression' to
+ *     'gte IE 8' and 'conditional_comment_downlevel_revealed' to TRUE.

Based on the documentation (haven't read the code yet), I wonder why we need 'conditional_comment_downlevel_revealed'. Given that 'conditional_comment_expression' is set, it feels like'conditional_comment_downlevel_revealed' could be redundant.

+++ includes/common.inc 24 Feb 2010 19:41:55 -0000
@@ -2756,75 +2764,369 @@ function drupal_get_css($css = NULL) {
+  // Merge markup needed for conditional comments into generic 'html_prefix' and
+  // 'html_suffix' keys. These keys are not documented in drupal_add_css(),
+  // because there's no identified use-case for them independent of conditional
+  // comments, but if these are passed in when drupal_add_css() is called or set
+  // by hook_css_alter(), don't override them, but wrap them with the
+  // conditional comment markup.

I think this mapping is weird and unnecessary. If we want to keep html_prefix and hmtl_suffic for internal purposes, I recommend rolling back the 'conditional_comment_downlevel_revealed' and 'conditional_comment_expression' changes.

+++ includes/common.inc 24 Feb 2010 19:41:55 -0000
@@ -2756,75 +2764,369 @@ function drupal_get_css($css = NULL) {
+ * A hook_css_alter() implementation may override this function with a custom
+ * 'group' callback. For example, a module may want module CSS files and theme
+ * CSS files in different groups.

This looks good but we don't explain how the default grouping works.

+++ includes/common.inc 24 Feb 2010 19:41:55 -0000
@@ -2756,75 +2764,369 @@ function drupal_get_css($css = NULL) {
+ * @return
+ *   An array of CSS groups. Each group contains the same properties as a CSS
+ *   item, where the property value applies to the group as a whole. Each group
+ *   also contains an 'items' property which is an array of CSS items, like
+ *   $css, but only with the items in the group.

What are the 'properties of a CSS item'? From reading the documentation, I can't tell what the return value will be.

+++ includes/common.inc 24 Feb 2010 19:41:55 -0000
@@ -2756,75 +2764,369 @@ function drupal_get_css($css = NULL) {
+function _drupal_css_group($css) {

This function feels a lot cleaner now. Good job.

Overall this feels like a nice improvement over the previous patches, but there are still a number of complex functions. I need to revisit them to see if they can be simplified. Everyone is welcome to help, of course.

Thanks, Dries, for taking the time to thoroughly review this patch. Questions about your feedback:

Based on the documentation (haven't read the code yet), I wonder why we need 'conditional_comment_downlevel_revealed'. Given that 'conditional_comment_expression' is set, it feels like 'conditional_comment_downlevel_revealed' could be redundant.

http://en.wikipedia.org/wiki/Conditional_comment explains it. Basically, only IE evaluates conditional comments. It doesn't matter what the expression is, non-IE browsers will skip right over it, so a choice needs to be made when outputting the markup whether you want non-IE browsers to load the CSS file or not. I suspect it's rare for conditional comments to be used for targeting non-IE browsers: I think it's mostly used for targeting either just IE or just some version of IE, but to be complete, this "downlevel revealed" cruft exists. I guess we could have drupal_get/add_css() try to auto-determine it by parsing the 'conditional_comment_expression', but that seems frought with peril. Or, we could make 'conditional_comment_expression' not an expression, but an array with syntax like ('IE' => TRUE | FALSE | '<N' | '<=N' | '>N' | '>=N' , 'non-IE' => TRUE | FALSE), but then we'd lose out on all the other kinds of expressions one could have that IE supports (like checking on WindowsEdition). I guess it's a question of how much we want to hide vs. expose HTML complexities from the code that calls drupal_add_css(). 'html_prefix' and 'html_suffix' is the most transparent, but requires the most ugliness in the calling code. 'conditional_comment_expression' and an optional 'conditional_comment_downlevel_revealed' creates one layer of indirection while retaining concepts and syntax that people who are used to dealing with conditional comments are used to. Something like 'browser_condition' => array('IE' => '<7') would create the most friendly API to people not already used to dealing with conditional comment cruft, but is that our target audience for this?

I think this mapping is weird and unnecessary. If we want to keep html_prefix and html_suffix for internal purposes, I recommend rolling back the 'conditional_comment_downlevel_revealed' and 'conditional_comment_expression' changes.

Really? I thought your goal in #364 was to create a friendlier API. So that, for example, we could have themes call:

drupal_add_css(path_to_theme() . '/ie6.css', array('weight' => CSS_THEME, 'conditional_comment_expression' => 'lt IE 7', 'preprocess' => FALSE));

instead of
drupal_add_css(path_to_theme() . '/ie6.css', array('weight' => CSS_THEME, 'html_prefix' => "\n<!--[if lt IE 7]>\n", 'html_suffix' => "<![endif]-->\n", 'preprocess' => FALSE));

But since grouping and rendering code needs to be aware of some kind of notion of 'html_prefix' and 'html_suffix', whether they calculate it, or whether it's prepared for them before they're called, I think it's nice to centralize their preparation, which is why I added that into drupal_get_css(). I'm fine with rolling back as you suggest, especially if I mis-interpreted your motivation for wanting those keys removed.

Here's a suggestion for replacing 'conditional_comment_expression' and 'conditional_comment_downlevel_revealed': We could have something like:

'browser_condition' => array('IE' => TRUE | FALSE | expression, 'non-IE' => TRUE | FALSE)

Examples:
To load something for IE only (version independent):

drupal_add_css(path_to_theme() . '/ie.css', array('browser_condition' => array('IE' => TRUE, 'non-IE' => FALSE));

To load something for IE6 only:

drupal_add_css(path_to_theme() . '/ie6.css', array('browser_condition' => array('IE' => 'lt IE 7', 'non-IE' => FALSE));

To load something for IE8+ and non-IE:

drupal_add_css(path_to_theme() . '/advanced-styles.css', array('browser_condition' => array('IE' => 'gte IE 8', 'non-IE' => TRUE));

This retains the full power of expressions which can be anything IE supports, while still being somewhat approachable, especially for the most common use-cases.

I read through this patch and overall think it's really great! - truly amazing effort to figure this all out and put it all together.

In terms of Dries's comment in #371 that the functions seem complex, my only thought was that some of the apparent complexity comes from the fact that there is (inherently) some data munging and foreach() loops needed to pass the data through three different defined callback functions. I wondered a bit if there was a simpler, more flexible way to make this stuff alterable that didn't involve a defined list of three callbacks. For example, I could imagine building up a single big array and passing it through drupal_render(), something like:

<?php
array(
 
'#type' => 'stylesheets',
 
'group1' => array(
   
'#type' => 'css_group',
   
'items' => array(
     
$css_item_1,
     
$css_item_2,
      ...
    ),
  ),
 
'group2' => array(
   
'#type' => 'css_group',
   
'items' => array(
     
$css_item_3,
     
$css_item_4,
      ...
    ),
  ),
);
?>

Then a #pre_render function could be used to do the aggregation and add the appropriate '#theme' = 'html_tag' data in the right places, and anyone who wants to modify any part of it could do so on as granular a level as they wanted to, e.g. via #pre_render functions of their own.

I hesitate to suggest that at all, since (a) this issue is already at 370 comments, (b) I don't even know if it would work, and (c) I think the patch is basically committable as it is, without needing any kind of major rewrite :) But it might be worth at least thinking about.

Other comments on the patch:

+ *   - 'conditional_comment_expression':
+ *   - 'conditional_comment_downlevel_revealed':

I agree that this seems a bit too "jargon"-y. The suggestion in #373 would be a great improvement. Before reading that, I was thinking something along the lines of 'ie_only' => TRUE/FALSE, 'ie_condition' => 'lt IE 7'/etc, which is pretty similar. Either would be good and make it nice and easy to use.

+ *   - 'conditional_comment_expression': An expression to control which versions
+ *     of IE should load the CSS.

Here and a million other places - I wonder, should we really be saying "IE" here, or is it better to write out "Internet Explorer"? I don't have a very strong opinion.

+  $callbacks = array();
+  drupal_alter('css', $css, $callbacks);
+  $callbacks += array(
+    'group' => '_drupal_css_group',
+    'aggregate' => '_drupal_css_aggregate',
+    'render' => '_drupal_css_render',
+  );

Why not just add the defaults first, before passing in to drupal_alter()? Seems like it would be a bit cleaner.

  // Merge markup needed for conditional comments into generic 'html_prefix' and
  // 'html_suffix' keys. These keys are not documented in drupal_add_css(),
  // because there's no identified use-case for them independent of conditional
  // comments, but if these are passed in when drupal_add_css() is called or set
  // by hook_css_alter(), don't override them, but wrap them with the
  // conditional comment markup.

I agree with Dries that it does seem at least a little odd to introduce these here after going through the trouble to have separate keys for conditional comments. The reason being that anyone who is replacing the group or render callbacks now needs to think in terms of 'html_prefix' and 'html_suffix', whereas they'd otherwise be thinking in terms of the 'conditional_comment' keys whenever they use drupal_add_css(). Is there any reason that the 'conditional_comment' stuff can't be passed all the way through, and leave it up to the render callback to turn it into HTML? It feels like it belongs at that stage a bit more (especially since the linked-to Wikipedia article even suggests that there are a couple different strategies for exactly how to render these conditional comments).

  foreach ($css as $key => $item) {
    $item += array(
      'html_prefix' => '',
      'html_suffix' => '',
    );
    ...
    $css[$key] = $item;

Here and in several other places in the patch, instead of including the $css[$key] stuff at the end, it might be simpler to just delete that line and then use foreach ($css as &item) at the top instead.

        // http://en.wikipedia.org/wiki/Conditional_comment#Downlevel-revealed_conditional_comment

Rather than just a URL, this should get some kind of explanation out in front also.

  // For non-aggregated files, a dummy query string is added to the URL (@see
  // _drupal_css_render()). For aggregated files, it is instead added to the

I think @see isn't supposed to be used inline like that; just use "see" instead.

-      // Prefix filename to prevent blocking by firewalls which reject files
-      // starting with "ad*".

It looks like we lost this code comment in the patch. Seems like a good one to keep around.

+ * The variables generated here is a mirror of template_process_html().
+ * This processor will run it's course when theme_maintenance_page() is

Grammar issue #1: Should maybe say "The variables array generated here..."?
Grammar issue #2: Should be "its" rather than "it's".

-    if (preg_match_all('/href="' . preg_quote($GLOBALS['base_url'] . '/', '/') . '([^?]+)\?/', $styles, $matches)) {
-      $result = $matches[1];
+    // Stylesheet URL may be the href of a LINK tag or in an @import statement
+    // of a STYLE tag.
+    if (preg_match_all('/(href="|url\(")' . preg_quote($GLOBALS['base_url'] . '/', '/') . '([^?]+)\?/', $styles, $matches)) {

I wondered a bit why "@import" isn't actually being searched for in the regular expression, but instead "url" only... on the other hand, I guess I wonder the same thing about "LINK" and "href" :) Not a huge deal either way.

+  // This site will definitely have few enough CSS files to not encroach on IE's
+  // limit, but for some reason needs to run with aggregation disabled, and we
+  // want all CSS files included with LINK tags instead @import statements, so
+  // override _drupal_css_render().
+  $callbacks['render'] = 'mymodule_css_render';

I found this a little confusing to read and follow, since a lot of the description out in front was more about rationale for the site rather than explaining what the code itself does. Maybe something like this would be better?

Use a custom render callback that outputs all CSS files in LINK tags rather than @import statements. (This might be used on a site with a small number of non-aggregrated CSS files that is not in danger of running into IE's 31 stylesheet limit.)

Thanks for the review David. It seems like we're still making progress on this issue so I'm going to set this back to 'needs review' -- some of David comments should be incorporated. Thanks for your hard work and persistence, effulgentsia -- I'm committed to help get this committed to D7 as soon as possible. Let's iterate a bit more on it -- it feels like we're really close!

Status:Reviewed & tested by the community» Needs review

...so I'm going to set this back to 'needs review'

glad I could help. ;)

StatusFileSize
new35.08 KB
PASSED: [[SimpleTest]]: [MySQL] 17,903 pass(es).
[ View ]

I really appreciate the thorough review and commitment to this issue. I think this incorporates all the feedback from #371 and #374 except the tentative question about a regular expression used for an existing test.

I really like the suggestion in #374 to integrate this whole system better with the render system, especially using #pre_render rather than extending hook_css_alter(). That's incorporated in this patch.

Status:Needs review» Fixed

I received the code again, and decided to commit it to CVS HEAD. Great work all, especially effulgentsia. Let's make sure we update the upgrade instructions about this. Great work.

Cool beans!!!

Congrats!

Thanks and congratulations to everyone, for all the work on this. This was very much a community effort. A lot of important things were figured out before I started rolling patches, the debates helped air out important differences in goals and perspectives, and the reviews and attention at the end were critical to the code becoming commit-worthy.

At almost 400 comments, I'm hoping to let the issue stay "fixed" and automatically close in 2 weeks. So, if there's follow-up work needed, let's try to do those in follow-up issues. To that end:

#727068: drupal_get_css() needs more tests
#727072: Warn admin when IE 31 CSS file limit exceeds on D6

Status:Fixed» Closed (fixed)

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

I wrote a script that produces the import format as

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

Heres the code in template.php preprocess_page:

<?php
$styles_document
= new DOMDocument();
 
$styles_document->loadHTML($vars['styles']);
 
$style_elements = $styles_document->getElementsByTagName('style');
 
$i = 0;
  foreach (
$style_elements as $item) {
   
$stylesheets .= $item->nodeValue . "\n";
   
$i++;
  }
 
$vars['styles'] = '<style type="text/css" media="all">' . $stylesheets . '</style>';
?>

However, IE still doesn't seems to be reading all the css. Whats the next step??

Just found out @imports is also limited to 31 in a single style tag (source: http://john.albin.net/ie-css-limits/single-style-test.html)
Rewrote the script to generate style tags with only 30 @imports as below. Limited to 60 imports. Not a perfect solution but should suffice for most users.

<?php
// Consolidate all styles import into 1 style tag. Solves IE 31 style tag limit
 
$styles_document = new DOMDocument();
 
$styles_document->loadHTML($vars['styles']);
 
$style_elements = $styles_document->getElementsByTagName('style');
 
$stylesheets = '';
 
$stylesheets2 = '';
  for (
$i=0;$i<$style_elements->length;$i++) {
   
$stylesheets .= $style_elements->item($i)->nodeValue . "\n";
    if(
$i == 30) {break;}
  }
 
$styles = '<style type="text/css" media="all">' . $stylesheets . "</style>\n";
  for (
$i=31;$i<$style_elements->length + 1;$i++) {
   
$stylesheets2 .= $style_elements->item($i)->nodeValue . "\n";
    if(
$i == 60) {break;}
  }
  if(
$stylesheets2 != '') {
   
$styles .= '<style type="text/css" media="all">' . $stylesheets2 . '</style>';
  }
 
$vars['styles'] = $styles;
?>

Thank you @mfer for posting that link (post #297) and thank John for developing that module. Of all my years of design/ dev experience I had never come across this Internet Explorer bug. Impressive, given how many there are to choose from, that this one may be the nastiest yet. And that's really saying something.

Are there instructions for applying this patch for the average user that does not have GIT? Can't someone just create updated versions of the files in question so the average person can just upload them and overwrite the existing files? Why is the process of applying updates made to be so complex, time consuming, and directed towards advanced users beyond the average user?

I'm looking at the patch now and may have figured out what to extract to do it that way, but if anyone already has the files in this form, it would be a time-saver.

And, of course, thanks to everyone who worked on this. This one looks like a lot of work.

Does anyone know if or when this will be included in Drupal core?

@sparkweb: this was committed in February.

As for the process. If the "average user" doesn't know how to use patches then they either learn or wait until they are committed. It's really not a good idea to be patching core if you don't know what you're doing anyway.

Michelle

Thanks, Michelle.

Committed to 7, or to 6? We are still running Drupal 6. It looks like this is only for Drupal 7.

If it's in 6, I just need to do a core upgrade, which is what I was in the process of doing, but ran across this problem in IE and came here to check it out.

I'm still learning the Druapl way and Drupal-speak and mindset: I was taking a patch to mean something that was fully approved and ready to go. That's the way it looked if you read the thread as someone who doesn't know the difference --but since I made the post I've been reading up, and along with your comments I'm seeing how things are done here: a patch is a patch which, if you're properly geared up, you can use GIT and test it out, otherwise, if you want it in a simple upgarde, wait for the final "commit."

It seems to me that should be marked on the page (http://qa.drupal.org/pifr/test/34433) where you get the patch that it has been committed to core and one can get it just by upgrading. I would say it should be in this thread, but now it is...

Also, one can know what one is doing in terms of how to work with appplying changes to core code while not knowing how to use GIT. I hack core code in other systems all the time. It's not too far fetched to take the patch and apply it to the core files yourself (just compare the files and apply the differences). Of course it's easier if you use GIT since that is how Drupal is set up -- but you have to go through the motions of setting it up and learning how to use it.

Another way to do it, which is used in other projects, is just provide new files one can upload and overwrite the old files with rather than requiring a user to have GIT to do it. That's what I meant by "average user," since unlesss you're deep into Drupal or use GIT for other purposes--in which case you're probably more of a programmer--you're not going to be able to use or try out the patch. But it seems Drupal is trying to encourage more user involvement for testing out patches, which you don't have to be a programmer, but then puts up this kind of roadblock and more Drupal learning curve, thereby turning off and leaving another level of Web developer out who might otherwise be very capable of using and testing the patch and contributing to the project.

I went out and got hooked up with GIT anyway, and was in the process of setting it up....we'll see, since I have a job to get done and a client waiting...

Thanks :)

It does say it was committed here in this issue: http://drupal.org/node/228818#comment-2647054

You don't need to use git to apply patches but you shouldn't be patching core with uncommitted patches if you don't have a good idea of what you're doing. Until a patch is committed, there's no way to know for sure it will get in or if it's secure.

Yes, this is for Drupal 7. I have no idea if a backport to Drupal 6 is feasible. I haven't been following this issue... Just clicked on it to see why it popped up in my tracker again months after it had been closed.

Michelle

You may not know that there is a simple way to fix this issue : enable CSS aggregation so that there is only one stylesheet.

@jide: please read the full discussion, enabling aggregation does not solve the problem for the use-case when you need to be able to load each CSS file individually.

There are two modules around to solve this in D6. You can pick one.
http://drupal.org/project/unlimited_css -> have my fingers in this one
http://drupal.org/project/ie_css_optimizer -> this is the one i recommend for added control.

@DamienMcKenna : I'm perfectly aware of this, I'm trying to help a user who may not be aware of the aggregation mechanism and could have been lost in here (I may be wrong though). Hopefully issue summaries will help with this kind of situations.

Thanks everyone for the extra comments. Seems like I tore the scab off this one... LOL.

Anyways, as far as finding the comment in this thread that states this change was committed, I did follow through the discussion fairly closely to get an idea what the problem was and the solutions were -- and I have to say I am very impressed with how the community here solved this issue -- but there are close to 400 comments--and are comments where that notice should be?

Hard-core Drupalers please try to understand the point of view of someone who has not spent a zillion hours of their life to become a hard-core Drupaler: I don't know the jargon and where to find things on this site and how this whole system works like you do, and if you come here from other projects which are, let's just say, organized differently, this site and the entire Drupal edifice can be a very obtuse thing to find your way around in and absorb. It seems to me a lot of Drupalers, once indoctrinated in the Drupal way and Drupal-speak, have lost their "beginnner's mind" and forget that what is obvious to them is not necessarily obvious to the rest of the world and otherwise intelligent people who are trying to understand the ways of the Drupal universe--and that these ways are not primary truths of existence or even the rest of the programming and Web development world, and there's a very big world out there....

Then, there are those who do get that and truly try to help with solid comments and pointers and references. Thanks Don Quixote for the link to those modules--I will check them out.

Yes, I realize that I can just use the built-in CSS aggregator, which I have on the live site, except that in development, when you're working on the site CSS, this is a major PITA for obvious reasons.

So, if I don't need GIT to apply patches, can someone point me to where I am instructed as to how to do that? From what I've seen of patch files (like the one on this issue), it has changes and additions for multiple files in it and appears to be designed to be used with GIT. The only way I can see to use that file manually would be to extract the changes for each file, compare them to the existing file, and apply the newer changes. Of course I realize that if this patch was tested for 7 I can't just go around dumping the changes into 6. Well, I could try, but it would be largely experimental and hazardous and probably a waste of time.

Regards,

:-)

There used to be a manual somewhere, "how to apply patches".
http://drupal.org/node/132745
http://drupal.org/patch
(and i only found this by googling)
well, I don't find the one with "apply a patch without git, cvs and friends". Does it no longer exist?

Anyway, posting these links here is quite pointless anyway, since after a while it will get lost in comments again.
What you could or should do is create a new documentation issue, asking for a guide to apply patches without git.

And the other thing, you can follow the issue about
#1036132: Provide a mechanism for issue summaries

So, knowing how quickly stuff gets lost, I post again the links to the two
modules for D6, from #393.
Unlimited CSS module
CSS Optimizer module -> more powerful.

and to say it again,
this has been committed and fixed in D7.

Now, I hope this will still stick out after a ton of new posts have been added.

Pages