Conditional Styles in .info files, since drupal_add_css has it

That would be awesome!

subscribe. :-)

Oh neat idea. What about conditional RTL styles?

Steph, the latest version of Conditional Styles has rtl support. http://drupal.org/node/472284 It wouldn’t be a core-worthy idea without rtl support, IMO. :-)

Just so there's a bit more to discuss in this issue, here's the current syntax for adding conditional stylesheets:

Allow themes to specify conditional stylesheets in their .info file so that the conditional comments will be automatically included at the end of the standard $styles variable.

; Set the conditional stylesheets that are processed by IE.
conditional-stylesheets[if lt IE 7][all][] = ie6-and-below.css
conditional-stylesheets[if IE 7][all][] = ie7.css

The themer can place any conditional syntax in the first bracket; the full Conditional Comments syntax is available on Microsoft’s website. And placing the above text in your theme's .info file will automatically append the following HTML to your $styles variable in the page.tpl.php template:

<!--[if lt IE 7]>
  <link type="text/css" rel="stylesheet" media="all" href="ie6-and-below.css" />
<![endif]-->
<!--[if IE 7]>
  <link type="text/css" rel="stylesheet" media="all" href="ie7.css" />
<![endif]-->

The contrib module has to use conditional-stylesheets instead of core’s stylesheets key since it would cause conflicts otherwise. But if we moved this functionality into core, we could re-think the above syntax for conditional stylesheets in .info. What if we just nix the conditional- part of the key? Then, in the backend, we'd just need to check if the first-level array key started with "if " to know if its a conditional stylesheet or a standard stylesheet.

Lastly, we'd also need to modify drupal_add_css() to have a conditional param.

I would love to see this get committed, it would make things a lot easier for themers getting started to Drupal, especially when porting an html/wp/joomla template to Drupal.

I support that.

I don't support this. Proper markup doesn't require any special browser hacks. We should fix the markup (and stylesheets accordingly) instead.

@sun
Conditional styles are for IE and IE6 mostly. "Proper" and "IE6" are not synonyms. So there are 3 options.
1. You don't support IE6 so you don't need conditional styles, or not as many as if you do support it. (ideal scenario)
2. You support IE6 and let themers add their own conditional styles. (current state)
3. You support IE6 and give an easy way for themers to add conditional styles. (better than current state IMO)

What would be immensely useful would to be to support both Microsoft proprietary syntax and the upcoming CSS3 media queries (already supported by Safari and the iPhone) [1].

[1] http://msdn.microsoft.com/en-us/library/ms537512.aspx
[2] http://www.w3.org/TR/css3-mediaqueries/

@tic2000 I am not against conditional styles. However, I agree with Sun. With proper coding, I can create styling for IE6, IE7 and Firefox and others without ever using a single conditional style sheet. And I can create complex layouts as well.

So conditional styles are not a requirement when dealing with IE6.

But if .info files support conditional styles in one way or the other, I don't mind. I will just continue not using them.

That being said, I am not coding for IE6 since January. :-)

subscribe

+1. Most of the time it's possible to rewrite some CSS in a way that works for IE6, IE7, and the various more standards compliant browsers, but every once in a while I've run into bugs that simply aren't fixable any other way (other than changing the design). Sometimes the hackery necessary to get something working in IE using the same CSS as Fx, Safari, etc. is worse than simplifying the base CSS and providing a one rule override for IE.

I am in favor of this. I personally prefer to use the separate stylesheet over other forms of hacks. I have figured out how to do this in the current theme system (I am sure it is not the most optimized solution). HAving it in core would definitely make my life easier.

My only question is why the decision to hyphenate "conditional-stylesheets" when "base theme" is not. I would opt for consistency in naming convention.

A first try for a patch.
Now you can use "conditional stylesheets[if lt IE 7][all][] = fix-ie.css"
Also you can use drupal_add_css() in a module with 'condition' => 'if lt IE 7' as an option or the #attached_css version to add conditional styles.

Would it be possible to test this somehow?

This one has a small bug fix and adds a test for drupal_add_css($file, array('condition' => $condition)).
There are no test for themes yet I'm told. Maybe I'll try to write some, but it shouldn't keep this issue from moving forward.
Please test and review :)

Question. Is that 'lt' some kind of CSS standard? If not, could we use '<' and '>' signs like we do in .module info files to indicate version dependencies? That would be more consistent (presuming it's not a WTF for themers, but since most themers deal with PHP I don't imagine it would be).

It's all in the link [1] in Damien's comment in #9. It's just what IE expects.
Of course I could change the code to use '<' and then convert, but I think it would be to much and themers should be familiar with IE's conditional comments syntax anyway.

@webchick: please see Damien comment in #9. At least for the Microsoft conditional tagging documentation. Conditional styles is a Microsoft beast. They are not using '<' or '>' in their spec.

As for CSS 3 media queries, I would not mix this with this patch. CSS 3 media queries can load a style sheet file designed for a type of output, or, using a single CSS file, part of the file can separated in various media.

Furthermore, CSS3 media queries are not made to support various version numbers.

@xmacinfo
The CSS3 media queries are already supported, and are supported by this patch too. Whatever you put inside the first [] (for regular stylesheets) or second [] (for conditional stylesheets) is added to the media attribute.

Ok cool. Figured there was a reason. :) Carry on, then!

The CSS3 media queries are already supported, and are supported by this patch too. Whatever you put inside the first [] (for regular stylesheets) or second [] (for conditional stylesheets) is added to the media attribute.

I know this. I wanted to say that we cannot merge CSS 3 Media Queries and Microsoft Conditional Styles.

Yes, you are right we can't and we won't because we don't have to.

+1

I noticed some people had the idea that good coding practice makes conditional styles obsolete but this is not true;

-Older versions of internet explorer work with the hasLayout system. In order to style and position HTML elements properly you sometimes needs to give elements hasLayout and the safest way to do this is by giving it the zoom:1; property, and you dont want that mess in your main stylesheet.
-internet explorer renders some elements inconsistently with other browsers. In a complex graphical layout you might need browser specific styles for styling fieldsets and other form controls, and for internet explorer 6 you may need to control for its incorrect implementation of absolute positioning.
-You may need to correct for stacking order context bugs
-etc. etc.

re-roll

My bad!

+1, this would bring consistency to something that currently is far from consistent and just make it easier for all themers; one less thing to worry about.

@RobLoach
I don't know either how useful inline conditional styles are or how often are they used, but the code is there. If themers say they don't needed it can be removed. I just wrote that code for completeness.

Just a reroll so back to RTBC

Looks good from my end. Subscribe.

tagging

HEAD is broken.

HEAD may be broken, but the patch doesn't apply anymore.

Re-rolled.

This patch is particularly important to making http://drupal.org/node/469242 as valuable as it wants to be. Arguably the syntax:

; Set the conditional stylesheets that are processed by IE.
conditional-stylesheets[if lt IE 7][all][] = ie6-and-below.css
conditional-stylesheets[if IE 7][all][] = ie7.css

is even more elegant the the standard way, while essentially preserving the conventions that no one remembers anyway.

This patch currently only supports Downlevel-hidden Conditional Comments and not Downlevel-revealed Conditional Comments, so currently, we can't hide stuff from (certain versions of) IE (e.g. some advanced stylesheet,) while keeping it visible for the (latest IE versions and) other browsers.

-- for the full syntax see: http://en.wikipedia.org/wiki/Conditional_comment#Downlevel-revealed_cond... --

I would think the best way to implement this, is to say positive assertion is IE only, negative assertion is not IE. (AFAIK can be safely done "thanks" the the extensive IECC statements)

also, a followup issue would be to add this ability also to the scripts section ;)

Anyone keen to implement @alexanderpas's suggestion?

It's not that trivial, because you need to write the opening and closing "tag" so it's like adding a new level to the array, or some separator in the first one so you do something like

conditional-stylesheets[<![if !IE]>|<![endif]>][all][] = non-ie.css
conditional-stylesheets[<!--[if lt IE 7]>|<![endif]-->][all][] = ie6-and-below.css

actually, i was thinking of a more simpler approach:
if IE => <!--[if IE]> & <![endif]-->
if !IE => <!--[if !IE]><!--> & <!--<![endif]-->
similar:
if lt IE 7 => <!--[if IE]> & <![endif]-->
if !(lt IE 7) => <!--[if !(lt IE 7)]><!--> & <!--<![endif]-->
so, if the condition starts with if ! we use Downlevel-revealed Conditional Comments else we use Downlevel-hidden Conditional Comments

this keeps in my opinion, the best possibillities, as in a worst case scenario, we can always use if !(IE)|(IE 7) as a condition for example.

conditional-stylesheets[if IE][all][] = fix-ie.css
conditional-stylesheets[if IE][if lt IE 7][all][] = ie6-and-below.css
conditional-stylesheets[if IE][if IE 7][all][] = ie7.css

@sun: i was more thinking about:

conditional-stylesheets[if lt IE 7][all][] = ie6-and-below.css ; IE6 & below
conditional-stylesheets[if !(lt IE 7)][all][] = not-for-ie6-and-below.css ; IE7+ & other browsers.
conditional-stylesheets[if !IE][all][] = way-too-advanced-for-ie.css ; only other browsers.

Hm. Sorry, folks. I don't think this is going to make D7. It seems we're still discussing conceptual stage stuff (syntax) and we're well past code freeze at this point. :\

So if I understand correctly, the fact that this is pushed to 8.x means that html.tpl.php will HAVE to be overridden in order to add conditional styles to a theme in D7? This concerns me a bit since we put html.tpl.php in (at #469242: Move <head> outside page.tpl.php for anybody who wasn't following) in hopes that it wouldn't have to be overridden very often, but this means that it will basically always have to be overridden.

Am I correct here? If so, it seems like we need to get this in no matter how past code freeze we are.

Yes. Like all tpl.php files, html.tpl.php will need to be overriden. So we do not need to have conditional styles in core.

I don't see any problem here.

Re: #57, the problem I'm talking about is webchick's quote here: http://drupal.org/node/469242#comment-2022608

So while themers can edit html.tpl.php, we'd really rather they didn't.

I'm ok with overriding html.tpl.php personally, but if we're going to be emphasizing that it's sort of a bad practice, then this is a contradiction.

Well, this was RTBC since 24 of August, but tough luck it was overlooked so it didn't got in. You can't get everything in.

But in #469242: Move <head> outside page.tpl.php we also changed Garland theme so it doesn't need to override html.tpl.php and instead we add those conditional styles in a process_html hook.

To be honest, I don't know how on demand this feature will be in D8 with IE6/7 having a smaller market share by that time.

That quote was mostly me talking about people monkeying around with the order of the tags. But I've been talking to members of the security team and we might have another way around that, or, worst-case, we'll just use the same fix in D7 as D6 has.

This was *marked* RTBC 24 of August, but the most recent discussion leads me to believe that we don't have consensus here on the exact approach. Which means it's not *yet* RTBC, and didn't meet the deadline. :(

@webchick: gotcha, thanks for the clarification.

The last discussion was about something that wasn't in the scope of the initial patch.
@alexanderpas's suggestion came late, too late (and I think after the code freeze already).

But as I said, tough luck.
Anyway you don't need to "play" with html.tpl.php just to add conditional styles as Garland theme already proves.
You can use that approach to add a lot of things to the head tag without the need of editing html.tpl.php so there is no real issue.

and in the other news, I have created #580440: Add support for Downlevel-revealed Conditional Comments for http://drupal.org/project/conditional_styles so we can point those who want this functionality to that module.

Subscribe

Ok!

An API change was made to drupal_add_css; it now supports the use of a browsers option which is an array of conditions to use inside conditional stylesheets. So we've now lost parity between .info files and drupal_add_css().

The latest patch for this issue adds a lot of features. I think many of them are still D8 material. But if we focus on just meeting parity with drupal_add_css(), I think this issue is still very much needed for D7.

If we have a patch that just does this:

; Set the conditional stylesheets that are processed by IE.
conditional-stylesheets[if lt IE 7][all][] = fix-ie.css

I'd consider that D7 material. Are there any other things we need in order to match drupal_add_css()'s browsers option?

I agree with JohnAlbin.

So we've now lost parity between .info files and drupal_add_css().

This is a big problem IMO, and renders .info files pretty much useless where CSS files are concerned until it's resolved. If this isn't fixed it's going to be a major WTF and good luck to anyone that needs to explain adding files to Drupal themes to people...

- Well you add files via .info in the order you want them: stylesheets[all][] = styles.css
- But oh, if you are overriding a module file or you need to remove one you need to hook_css_alter()
- And oh yeah, if you want to add a conditional css file, you need to do so using drupal_add_css() in template_preprocess_html().

Good lord.

Even if you do understand all this, it still makes for a very messy template.php: http://bit.ly/958i3r

OK, here's a first try. Since I didn't follow the D7 development much have changed. Let see how bad the bot think it is.

OK, It passes. Now I need to know if this is what you desired from the patch or not :)

+++ modules/system/system.module	10 Apr 2010 14:19:05 -0000
@@ -2369,6 +2369,18 @@
+    $pathed_conditional_stylesheets = array();

"pathed"? What's that? :)

+++ modules/simpletest/tests/common.test	10 Apr 2010 14:19:02 -0000
@@ -698,6 +698,21 @@
+  ¶
+   /**
+   * Test conditional styles.
...
+  ¶

Trailing white-space and wrong indentation here.

+++ themes/garland/garland.info	10 Apr 2010 14:19:05 -0000
@@ -7,4 +7,5 @@
+conditional stylesheets[lt IE 7][all][] = fix-ie.css

+++ themes/seven/seven.info	10 Apr 2010 14:19:06 -0000
@@ -7,6 +7,8 @@
+conditional stylesheets[lte IE 8][all][] = ie.css
+conditional stylesheets[lt IE 7][all][] = ie.css

All of our other .info file properties do not use spaces.

+++ includes/theme.inc	10 Apr 2010 14:19:01 -0000
@@ -160,6 +170,17 @@
+  ¶

@@ -167,6 +188,15 @@
+  ¶

More trailing white-space.

Powered by Dreditor.

I don't know what "pathed" is. It's the term used for regular stylesheets, I just added "conditional" to the variable :)

All the other problems solved... I hope.

PS. Note that the syntax now it's conditional-stylesheets[lt IE 7][all][] = fix-ie.css without the "if" exactly like the "browsers" option in drupal_add_css.

In seven I add ie.css for both conditions. Fixed that.

I went through this patch carefully and the only thing I see is the "conditional stylesheets" vs. "conditional-stylesheets" discussion.

Sun said:

All of our other .info file properties do not use spaces.

Actually, that's not true. All of our .info properties (but one) are single words. The "base theme" is the only multi-word property and it uses a space. See: http://drupal.org/node/171205 Also, module .info files only have single-word properties.

I personally prefer conditional-stylesheets, but I recognize that it is not consistent with "base theme".

I'm marking this RTBC, unless people think "conditional stylesheets" is better.

Why not change "base theme" to "base-theme"?

Yes, we would do this in a new issue. Shall I open one? :-)

ok, but could we at least try to make those .info properties belonging to stylesheets use a consistent pattern?

Especially when considering #575298: Provide non-PHP way to reliably override CSS, I think it would be good if all those properties would share the same prefix, i.e.

stylesheets[] = ...
stylesheets-override[] = ...
stylesheets-conditional[] = ...

Thoughts?

Here is an updated patch to follow head (changes in seven theme) and one that incorporates sun's suggestion in #76

That makes sense, thanks!

Now we only need a consensus on JohnAlbin's finding that we may not use hyphens but spaces elsewhere. I'm also fine with using a space.

Though I'm not sure whether it will look wonky, given that we are dealing with an array syntax:

base theme = ...
stylesheets[] = ...
stylesheets-conditional[...] = ...

base theme = ...
stylesheets[] = ...
stylesheets conditional[...] = ...

Perhaps it's better to stay consistent and use a space. Thoughts?

I prefer with a hyphen because it's an array.
But to achieve consistency then we would need a new issue to make "base theme" become "base-theme". Or kill a kitten in this patch which I don't like.

Don't kill a kitten and deal with base-theme later in a new issue. :-)

So are we RTBC on the 2nd patch in #77 then?

+++ modules/simpletest/tests/common_test.module	23 Apr 2010 08:32:23 -0000
@@ -198,3 +204,15 @@
+function common_test_conditional_styles() {
+ $build['common_test_conditional_styles'] = array(
+    '#sorted' => TRUE,
+    '#markup' => 'Add conditional style',
+  );
...
+  return $build;

1) Wrong indentation here.

2) What is this #sorted for?

3) Can't we simply return a string here?

+++ modules/simpletest/tests/common.test	23 Apr 2010 08:32:23 -0000
@@ -699,6 +699,21 @@
+  function testConditionalStyles() {
...
+    $this->drupalGet("conditional-styles");
+    $css = '<!--[if lt IE 5]>';
+    $this->assertRaw($css, t('Conditional stylesheets - condition exists.'));
+    $css = 'simpletest.css';
+    $this->assertRaw($css, t('Conditional stylesheets - stylesheets added.'));

We can keep this test, if there was no test for conditional stylesheets via drupal_add_css() yet.

However, this test does not test the functionality that is added here. We either need to enable one of the testing themes (there are some, right?), or we need to implement common_test_system_info_alter() to fake a conditional stylesheet declaration for a regular theme. (The former would be preferred of course)

+++ includes/theme.inc	23 Apr 2010 08:32:22 -0000
@@ -168,6 +189,15 @@
+        drupal_add_css($stylesheet, array('weight' => CSS_THEME, 'media' => $media, 'browsers' => array('IE' => $condition, '!IE' => FALSE), 'preprocess' => FALSE));

Why is IE hardcoded here?

Powered by Dreditor.

Yes, we can just return a string.

When I first wrote the patch I made the test for "condition", now we have browsers in css and it's not tested so I kept the test.

Conditional stylesheets are only working for IE and i used the same options as garland and seven used to add the css in template.php.

Yeah, given the array syntax and the fact that there's an additional issue for stylesheet overrides, I prefer the stylesheets-conditional version. Using a space with the array syntax just looks weird. For example, if we had to use array syntax for base theme, base theme[] = zen would look weird.

OK, a new patch

I changed the test description in common tests to reflect that we test the 'browsers' option.
I return just a string.
I use the test theme to test 'stylesheets-conditional'. I had to add a ie.css file to have something to add. (I think that hook_system_info_alter() did not test all the process so it's safer this way)

IE is still hard coded for the reason I gave in my previous comment. To be honest I have no idea how I could do it otherwise in an "easy" manner.

+++ modules/simpletest/tests/common.test	24 Apr 2010 10:08:34 -0000
@@ -699,6 +699,21 @@
+    $css = '<!--[if lt IE 5]>';
+    $this->assertRaw($css, t('Conditional stylesheets - condition exists.'));
+    $css = 'simpletest.css';
+    $this->assertRaw($css, t('Conditional stylesheets - drupal_add_css "browsers" option.'));

Same assertion issue as below.

+++ modules/simpletest/tests/theme.test	24 Apr 2010 10:08:34 -0000
@@ -66,6 +66,17 @@
+  ¶
+  /**

Trailing white-space here.

+++ modules/simpletest/tests/theme.test	24 Apr 2010 10:08:34 -0000
@@ -66,6 +66,17 @@
+    $css = '<!--[if lte IE 8]>';
+    $this->assertRaw($css, t('Conditional stylesheets in theme.info - condition exists'));
+    $css = 'ie.css';
+    $this->assertRaw($css, t('Conditional stylesheets in theme.info - file added'));

Can we use assertPattern() to match the opening conditional comment, the filename, and the closing conditional comment in the output? These two assertions don't verify that ie.css is actually contained within the conditional comment.

+++ themes/tests/test_theme/ie.css	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,12 @@
+ * Test theme
+ * ¶
+ * Conditional stylesheet for IE browsers

1) Trailing white-space.

2) We can move the description into the summary; "Conditional stylesheet for IE for Test theme."

Powered by Dreditor.

Patch that implements sun's comments.

We're almost done! :)

+++ modules/simpletest/tests/common.test	24 Apr 2010 17:18:08 -0000
@@ -699,6 +699,19 @@
+    $this->drupalGet("conditional-styles");

Use double-quotes only when necessary.

+++ modules/simpletest/tests/common.test	24 Apr 2010 17:18:08 -0000
@@ -699,6 +699,19 @@
+    $this->assertPattern($pattern, t('Conditional stylesheets - drupal_add_css() "browsers" option.'));

The assertion message is quite cryptic, not sure what it tries to tell. Usual messages are like "Condition stylesheet added by drupal_add_css() was found."

+++ modules/simpletest/tests/common.test	24 Apr 2010 17:18:08 -0000
@@ -699,6 +699,19 @@
+  }
+
 }

If I'm not mistaken, then there should not be a blank line here.

+++ modules/simpletest/tests/theme.test	24 Apr 2010 17:18:08 -0000
@@ -66,6 +66,15 @@
+    $this->assertPattern($pattern, t('Conditional stylesheets in theme.info'));

Same as above; assertion message could be more precise: "Conditional stylesheet added via theme .info file was found."

+++ includes/theme.inc	24 Apr 2010 17:18:07 -0000
@@ -168,6 +189,15 @@
+  // And now add the conditional stylesheets properly

Missing trailing period.

Note: Please roll patches with diff -up.

Powered by Dreditor.

I'm on Windows and using Aptana/Eclipse, I can't do diff -up and if I try in command line it doesn't add the new file even if I use -N

After some reading I found out how to add the file.
Same patch as in #89 just that it's done using diff -up

I find #90 RTBC.

However, while I don't find downlevel-revealed conditional comments particularly useful, drupal_add_css() has them as well. And it turns out its ridiculously easy to add them to the patch. Just two additional lines of code (plus brackets and comments). Here's the patch, what do people think?

It's not that easy.

1. You forgot that you have to remove the "!" from $condition

2. You set IE to false, meaning that the style will be used by every browser except IE. But what if I want the style to be used by every browser AND IE >= 8? Using the broswers option you do this with browsers = array('IE' => 'gte IE 8'). The easy way would be to use something else for this case. "!" for IE false and "~" when we don't want IE to be false.

So we need more lines of code. <10

Looks good.

Which one is RTBC? #90 or #91?

+++ themes/tests/test_theme/test_theme.info	25 Apr 2010 11:16:22 -0000
+++ themes/tests/test_theme/test_theme.info	25 Apr 2010 11:16:22 -0000
@@ -3,4 +3,5 @@ name = Test theme

@@ -3,4 +3,5 @@ name = Test theme
 description = Theme for testing the theme system
 core = 7.x
 engine = phptemplate
-hidden = TRUE
\ No newline at end of file
+hidden = TRUE
+stylesheets-conditional[lte IE 8][all][] = ie.css

Is there a newline at the end of file?

Cross-posted with tic2000

New patch

I did not find an easy way for downlevel-revealed comments, but John did.

Changes since #91.

1. "!" becomes "+" because we may have a condition like "!(IE 7)" and have a false positive.
2. I remove the "+" from the condition. (now when I write this comment I wonder if ltrim($condition, "+") wasn't a better way to do it).
3. When "+" is found if the condition is "!IE" it means we want the style for everything, but IE and I set IE to false. For any other condition it means we want the style for every browser and IE browsers that meet the condition so I just set 'IE' => $condition as per drupal_pre_render_conditional_comments documentation example.
4. Added 2 more tests for the 2 additional cases.

line endings?

#98: stylesheets-conditional-522006-98.patch queued for re-testing.

I have no failure on my test server.

#98: stylesheets-conditional-522006-98.patch queued for re-testing.

Finally the bot behaves :)

Now we need a review especially on the text additions and I think the patch is ready to go.

There were three end-of-line spaces that needed to be removed. Otherwise, this patch is identical to the last one.

I'm not crazy about the "+" sign being the flag that determines that stylesheet is going to be included in a "downlevel revealed conditional comment" instead of the usual "downlevel hidden conditional comment". However, I can't think of anything better. Magic text flags are obscure, but horribly verbose nested array patterns are much worse. I considered stylesheets-conditional['revealed']['!IE']['all'][] = 'not-ie.css'; stylesheets-conditional['hidden']['IE']['all'][] = 'ie.css'; but that's worse then what is in this patch (as explained by tic2000 in #96).

Also, plus signs are not used in any of the MS-specified conditions, so there's no chance of a conflict between a user wanting a hidden-style comment that just happens to match our "magic" plus sign flag.

So I think we should either:

1. Go with this patch with its "if first character is +, then do downlevel revealed" syntax.
2. Nix the whole idea of having downlevel revealed conditional syntax in .info files and go with the patch in #90.

I prefer #1 since its more feature complete. And we'll ensure the d7 theme .info docs are clear on the syntax.

I never used downlevel-revealed conditional comments, actually didn't know they exist, but since they are even documented on Wikipedia, and since we have a patch that seems to work and support them via .info files, I think we're good to go.

Then someone needs to RTBC this. I can't do it.

I really dislike this solution. info-files are not meant to be for programming. We can revisit this in Drupal 8, but for Drupal 7 this is a no go.

@Dries: Could you elaborate a bit more on which part you do not like? Is it the recent addition of downlevel-revealed conditional comments (the stuff with + prefix), perhaps? We could surely skip that. But the rest of what this patch changes is very similar to existing declarations in theme .info files and merely allows themers to not have to program PHP to add conditional stylesheets. The "if..." condition in those registered stylesheets may read like programming, but it's really a simple registry only (that "condition" is output as is).

Resetting status to get some more input/discussion from Dries.

Why do we need more input? Both core maintainers have said no to this. We have 100s of other issues to resolve to get D7 out the door, and this is not one of them.

Conditional stylesheets are used by all of the current and proposed D7 core themes, are used by core themes in D6 and D5, are recommended by Microsoft since "CSS Hacks" for IE might break when a simple bugfix is made, and are used in development best practices to keep the CSS free of unreadable hacks.

So, Dries and webchick, you'd prefer that all designers use the following PHP code to add their conditional stylesheets:

function garland_preprocess_html(&$vars) {
  // Add conditional CSS for IE6.
  drupal_add_css(
    path_to_theme() . '/fix-ie.css',
    array(
      'weight' => CSS_THEME,
      'browsers' => array(
        'IE' => 'lt IE 7',
        '!IE' => FALSE
      ),
      'preprocess' => FALSE
    )
  );
}

Instead of putting this in the their .info file:

stylesheets-conditional[lt IE 7][all][] = fix-ie.css

?

Note: the above PHP code snippet is what we currently have in core (go look in seven/template.php or garland/template.php) and the .info snippet is what we would have in core if the patch in #104 was committed. 203 characters of PHP versus 53 characters of .info-style text.

Sad… sad pandas.

+1 on JohnAlbin #112. In this age of iPhones, Android, iPad and the hard claws of lingering IE 6 and 7, having conditional stylesheets declarable in .info would be an extremely designer/front-end developer-friendly move.

The more preprocess/process code required for common and obvious front-end development use cases, the more Drupal 7 becomes the system that is not designer friendly (as perceived by the outside world).

+1 on JohnAlbin and laura s.
HTML comments shouldn't have logic or presentational code either. But conditional comments are the tool that Microsoft has provided for fixing its broken browsers. The only other alternatives are worse: javascript, css hacks, change your design. Conditional comments are the industry standard way to fix IE css bugs.

By the time Drupal-8 goes out the door they'll be far less relevant (I hope), but right now we need them. I'm putting this back to 7.x, if it's not going in then please mark it 'wont-fix', it's more honest and more apt. None of the arguments for will get any stronger in the future and the relevance will wane.

We can either provide this nice designer/themer friendly feature or have almost every theme duplicate similar code, as is already much the case for Drupal-6 base themes. The info file syntax is even cleaner and easier than the original conditional comment code I'm used to pasting into <head> of every theme I make.

If this doesn't get in, then the average themer will either:

A) Do it the old fashioned way (i.e., by pasting the code straight into <head> in html.tpl.php), or
B) Think "Hmm, I bet Drupal has a better way to do this", then research and find code like JohnAlbin posted in #112, and say "WTF this is way harder than just doing it the old fashioned way" which will leave them with a bad taste in their mouth.

On the other hand, .info integration could be a KILLER feature to themers. And I agree with #114 - it's not going to be any different for 8.x, so just mark won't fix if it's still a no.

As JohnAlbin said in #64, this is the real problem:

we've now lost parity between .info files and drupal_add_css()

Here's what's going on now:

Seven

Current Situation

Seven is adding 2 files in .info and then calling template_preprocess_html() for the sole purpose of adding 2 conditional stylesheets via drupal_add_css() because you cannot do this in the .info file.

stylesheets[screen][] = reset.css
stylesheets[screen][] = style.css

 /**
 * Override or insert variables into the html template.
 */
function seven_preprocess_html(&$vars) {
  // Add conditional CSS for IE8 and below.
  drupal_add_css(path_to_theme() . '/ie.css', array('weight' => CSS_THEME, 'browsers' => array('IE' => 'lte IE 8', '!IE' => FALSE), 'preprocess' => FALSE));
  // Add conditional CSS for IE6.
  drupal_add_css(path_to_theme() . '/ie6.css', array('weight' => CSS_THEME, 'browsers' => array('IE' => 'lt IE 7', '!IE' => FALSE), 'preprocess' => FALSE));
}

Could be

stylesheets[screen][] = reset.css
stylesheets[screen][] = style.css
stylesheets-conditional[lte IE 8][all][] = ie.css
stylesheets-conditional[lt IE 7][all][] = ie6.css

Bartik

Current Situation

stylesheets[all][] = css/layout.css
stylesheets[all][] = css/style.css
stylesheets[all][] = css/colors.css
stylesheets[print][] = css/print.css

 function bartik_preprocess_html(&$variables) {
  <snip>

  // Add conditional stylesheets for IE
  drupal_add_css(path_to_theme() . '/css/ie.css', array('weight' => CSS_THEME, 'browsers' => array('IE' => 'lte IE 7', '!IE' => FALSE), 'preprocess' => FALSE));
  drupal_add_css(path_to_theme() . '/css/ie6.css', array('weight' => CSS_THEME, 'browsers' => array('IE' => 'IE 6', '!IE' => FALSE), 'preprocess' => FALSE));
} 

Could be

stylesheets[all][] = css/layout.css
stylesheets[all][] = css/style.css
stylesheets[all][] = css/colors.css
stylesheets[print][] = css/print.css
stylesheets-conditional[lte IE 7][all][] = ie.css
stylesheets-conditional[IE 6][all][] = ie6.css

The same is true for Garland (and any other theme I've done/seen), as JohnAlbin noted above, but you get the idea here.

I consider this a bug, and think that since we can fix it we should. With all the improvements in Drupal 7, and how much easier things are in general, it would really be sad to leave this in its current state. It's definitely going to confuse a lot of people. There are 3 different ways themers need to deal with CSS files right now (.info, template_preprocess_html/X() and hook_css_alter()), and this would at least eliminate one of them in most cases.

Not fixing this also so means that modules, like Skinr, will not be able to take advantage of the awesome improvements to drupal_add_css() because it reads from .info files.

Reverting status; there's a working patch.

I agree with all arguments, and this should land for D7. I'm still a bit baffled by the counter-arguments, as 1) there is no programming logic added to .info files here, and 2) the earlier counter-argument merely was that there was no consensus on the .info file syntax reached yet.

Should this be RTBC again then? The community is obviously in support of it.

I don't usually chime in that much... (I'm a bit slow on the uptake ;-), however as all themers I've ran into the Drupal conditional style sheet issue. As with some I've found ways to implement this in the theme layer to my own liking and I do agree with the need for conditionals. I find that style declaration separation is always a good thing when it adds to legibility and elegance in code.
However... I also agree that sometimes conditional style sheets are used more as crutches instead of real implementation needs.

I feel that it would be good to get conditional style sheets into D7, but would like to present a concept for future thought to the Drupal community.

Drupal CSS namespaces:
Drupal has set structures for html implementation and could provide default class namespaces to allow front end devs to alter namespace style declarations for specific html structures that are output. The only missing component is the browser specific conditional style sheet loading and a consistent namespace that themers and front-end developers can leverage for consistent declaration assignment implementation.

Alas... I am not a back-end dev and am much more familiar with design implementation then I am with the internal workings of... what's faster then what... lol

In my experience browser conditionals tend to be leveraged in conjunction with style sheets already present in the page load. From a front end implementation perspective. Conditional style sheets "could" almost always be thought of as replication of current style sheets (however this doesn't have to be the case), which have some different declarations. So style.css could also load style-ie6.css or style-ie7.css programatically without needing to statically define what browser style sheets are provided.

The main issues I see with this, would be...
1. Defining our browser type via code. We could leverage something like:
(taken from: http://php.net/manual/en/function.get-browser.php)

function browser_info($agent=null) {
  // Declare known browsers to look for
  $known = array('msie', 'firefox', 'safari', 'webkit', 'opera', 'netscape',
    'konqueror', 'gecko');

  // Clean up agent and build regex that matches phrases for known browsers
  // (e.g. "Firefox/2.0" or "MSIE 6.0" (This only matches the major and minor
  // version numbers.  E.g. "2.0.0.6" is parsed as simply "2.0"
  $agent = strtolower($agent ? $agent : $_SERVER['HTTP_USER_AGENT']);
  $pattern = '#(?<browser>' . join('|', $known) .
    ')[/ ]+(?<version>[0-9]+(?:\.[0-9]+)?)#';

  // Find all phrases (or return empty array if none found)
  if (!preg_match_all($pattern, $agent, $matches)) return array();

  // Since some UAs have more than one phrase (e.g Firefox has a Gecko phrase,
  // Opera 7,8 have a MSIE phrase), use the last one found (the right-most one
  // in the UA).  That's usually the most correct.
  $i = count($matches['browser'])-1;
  return array($matches['browser'][$i] => $matches['version'][$i]);
}

This returns an array with the detected browser as the key, and the version as
the value, and also sets 'browser' and 'version' keys. For example on Firefox 3.5:

$ua = browser_info();
print_r($ua);
/* Yields ...
Array
(
    [firefox] => 3.5
    [browser] => firefox
    [version] => 3.5
)
*/

// Various browser tests you can do with the returned array ...
if ($ua['firefox']) ... // true
if ($ua['firefox'] > 3) ... // true
if ($ua['firefox'] > 4) ... // false
if ($ua['browser'] == 'firefox') ... // true
if ($ua['version'] > 3.5) ... // true
if ($ua['msie']) ... // false ('msie' key not defined)
if ($ua['opera'] > 3) ... // false ('opera' key not defined)
if ($ua['safari'] < 3) ... // false also ('safari' key not defined)

However I'm sure that we could find a better way to implement hookable browser types in order to provide a solid upgrade path as browsers change and evolve. ;-)

2. The other major issue I see with this is page cache.
One of the best things in Drupal is the system page caching and the css aggregation.
I have no complaint providing both a globally aggregated style sheet and a browser specific aggregated style sheet, however without a consistent CSS namespace structure, class declaration hierarchy will be difficult to identify/implement for most themers and front-end developers and would probably result in an even more confusing theme layer.
The other big issue with this is that url ".com" gets cached with all the styles associated with it. This would mean that if the page cached with style sheets for IE6 then the next user on IE7 may run into issues. So page cache would need to be thought through and probably updated in order to provide browser specific page caching. This area of Drupal is a bit foggy for me, but I know the issue exists.

Please note, these are just thoughts and there are many people smarter then me within this community, but I felt that this was in a vein I've been thinking about personally for a while now and wanted to throw in my two cents.
I hope this is helpful in some way,
:-)

The main reason behind declaring stylesheets in info file was to abstract logic away so that themers don't have to deal with PHP functions.

Unfortunately the opposite is happening - the things are getting overcomplicated - because of the limitation of info files the average themer will have to deal with both concepts: info files and preprocess functions.

If it's not possible to add conditional stylesheets, disable core stylesheets or assign weights, then I would prefer the whole concept of defining stylesheets in info files to be dropped.

I agree with what you're saying jarek.

If my reading is correct, then this discussion has already ran its course and was too late.
If, by chance it is brought back to the table and gets in.. then that's cool, but personally... I've found a way that works for me, so I'll stand behind the code freeze.

I do think that we should keep the conversations going for D8 to see what we can eliminate from the headaches of theming that currently exists. I personally need to be better at this, but its good to see the conversations going.

I removed my example to prevent confusion
:-)

@katris Your first solution: Browser sniffing went out of fashion a decade ago, mostly for the reason that user-agent strings are utterly unreliable. You solution won't work 100% in the real world. Your second solution: in D7 head is no longer output in page,tpl.php.

@katris (comment #119): Browser sniffing the USER_AGENT is buggy because browsers can (and do) impersonate other browsers. That fact completely breaks your idea of browser namespaces and CSS caching. As a side note, jQuery doesn't rely on browser detection either; instead it uses "feature detection". This is a similar idea to the CSS community's "progressive enhancement". Its only IE's horribly broken box model that makes using conditional stylesheets the simplest solution. Otherwise, progressive enhancement is the way to go. Any further discussion is out-of-scope for this issue.

@katris (comment #121): Your solution works okay with D6, but it could easily suffer from the IE 31 stylesheet limit depending on modules/languages used on a site and could also suffer from performance problems with mixing @import and <link /> stylesheet loading on one page. That's why the current D7 method of adding conditional stylesheets is what it is (as described in comment #112 above).

@jarek (and Dries/webchick): From a themer's perspective, drupal_add_css() is a horribly complicated mess and its default parameter values are set to ease module developers and require themers to override the defaults. I'm not advocating that .info files should be able to do anything that drupal_add_css() can do. Instead I'm advocating that any basic, frequently-used "I want to add this CSS file" use case should be able to avoid using drupal_add_css() by supporting a simple syntax in .info files. The fact that all our core themes use them means that conditional stylesheets are a basic use case.

When a themer wants to add a conditional stylesheet, they will look to how core themes do it:

• In D6, garland just hard-coded these in page.tpl; that was hacky, but it was obvious and easy to modify for your own usage.
• At code freeze in D7, core themes used a hook_preprocess_html function to tack-on a hard-coded stylesheet to the end of $vars['styles']; this was non-obvious to find since it was buried in template.php, but easy to modify once you found it. I was… "ok" with postponing for D8 at that point in time.
• But now we have a worst-case situation: a difficult-to-find, difficult-to-modify PHP code snippet for a simple use case. So in D7, we will have themers trying to come up with solutions like what described in comment #121 above; core should be guiding themers, not allowing them to flounder to find easier (but broken) solutions. That's why I re-opened this issue after #228818: IE: Stylesheets ignored after 31 link/style tags landed.

@adrinux
@JohnAlbin

Correct, correct and correct.

My bad with the code...lol. I removed my example to prevent the confusion noted in John's last post.

1. Yes... out o fashion and one of the big issues with being able to do this on the backend is the reliability. I agree.
2. Agreed... I was really just bringing it up. If its not something the Drupal community feels is a worthy effort, then no biggie.
3. Correct... as noted. If you need to load in a lot of style sheets... you're not in a better boat, but a different boat.

Thanks for the feedback.

For argument's sake, can you explain what exactly is so horrible about just doing what D6 Garland does for conditional stylesheets, and embedding them directly in the template file? I get that they're not included in CSS compression then, but then I notice that sites who get a lot of traffic, such as http://www.whitehouse.gov/, seem to take this approach and somehow stay online.

Another approach is removing the ability to add conditional stylesheets in drupal_add_css() to begin with. This would then remove this inconsistency between the two, and not introduce programming logic into .info files. I find this preferable to the solution outlined here.

@webchick - from my pov its because its a maintenance overhead, lets say you have a complex setup of subthemes, we would have to include and maintain html.tpl.php for each subtheme, assuming each subtheme will have unique conditional stylesheets (not improbable). I think for more simple use cases its not that bad, but it still means having to add a template that otherwise you would not.

I would rather live with the inconsistency than have it removed from drupal_add_css, that at least is good for those who know PHP.

Please excuse my ignorance but I still don't get what is meant by programming logic in info files, and that being the case why this is so bad?

@webchick the way how stylesheets are currently handled is not very consistent from themers point of view. Just to give you an example: lets say I want to build basic CSS-only theme that consist from reset.css, page.css, ie7.css and disables system-menus.css from core. A different approach must be taken for each of those stylesheets.

1. To enable reset.css I have to use drupal_add_css() with proper weight option:

function mytheme_preprocess_html(&$variables) {
  drupal_add_css($data = path_to_theme() . '/reset.css', $options['type'] = 'file', $options['weight'] = CSS_SYSTEM - 1);
}

2. To enable page.css I have to use mytheme.info:

stylesheets[all][]   = page.css

3. If you remove the ability to add conditional stylesheets via drupal_add_css(), I would have to use preprocess function to add ie7.css:

function mytheme_process_html(&$variables) {
  $variables['styles'] .= "\n<!--[if lte IE 7]>\n" . '<link type="text/css" rel="stylesheet" media="all" href="' . file_create_url(path_to_theme() . '/ie7.css') . '" />' . "\n" . "<![endif]-->\n";

4. And finally in order to disable core stylesheet I have to use hook_css_alter():

function mytheme_css_alter(&$css) {
  unset($css[drupal_get_path('module', 'system') . '/system-menus.css']);
}

It would simplify things a lot if themers could do all those steps in one place and using one consistent syntax:

; stylesheets[media][weight][condition]
stylesheets[all][-1][]             = reset.css
stylesheets[all][][]               = page.css
stylesheets[all][][lte IE7]        = ie7.css
stylesheets[disabled][][]          = system-menus.css

I get that they're not included in CSS compression then

Actually, conditional stylesheets aren't going to be included in the CSS aggregation anyway since they're included using different HTML syntax then normal CSS files.

not introduce programming logic into .info files

Again, as sun was saying earlier: there is no programming or logic at all in the proposed syntax for these .info file lines. none. nada. zip. Its a simple registry.

For example, if you were to type this in your .info file:

stylesheets-conditional[parsnip picadillo][cherry coke][] = wassup.css

Then Drupal would always output this HTML after the normal stylesheets:

<!--[if parsnip picadillo]>
<link type="text/css" rel="stylesheet" media="cherry coke" href="/themes/stark/wassup.css" />
<![endif]-->

The "condition" in conditional stylesheets for this context is a simple label. Just like the second bracket's label is "media", the first bracket's label is "condition". The values for the "condition" and "media" are both text snippets stored in the .info registry.

@jarek: that discussion is way out of scope for this issue. Theme .info syntax simply can't have all the options that drupal_add_css() has. Period. See my original response to you in comment #123 above.

The current situation is not designer friendly, or consistent at all. While weighting and altering CSS files isn't going to be the easiest thing for people to grok, it's not as common a task, nor is it the focus of what we are trying to do here, so let's try to stay on topic.

The goal is to prevent every theme from having to use to template_preprocess_html() just to add conditional stylesheets in order to make what should be a basic process, easier, more consistent, and less of a WTF for the average designer/themer.

Like many in this issue, I don't understand the resistance here. It seems like a complete 360 from the way things have been going:

• When it came to the new $classes_array/$classes for D7, the reason we couldn't have ALL attributes handled in this fashion (even though it would be much cleaner) was because people wanted to make it easy for a themer to come in and easily add an ID or class to a template file, without having to go into template.php and use preprocess functions.
• The lengths that were gone to in order to allow themers to render/hide/show fields in template files were epic.

Yet here, we guarantee most designers/themers will need to go into template.php and use a preprocess function just to add a stylesheet, very early on. I just don't get it. :(

@webchick
Even Garland in D7 does not embed them in a template file. Nor Seven does it.
This just allows themers a very easy way to add conditional stylesheets the exact same way it allows to add regular stylesheets, without the need for them to find out that html.tpl.php exists and where it is or to learn what a preprocess hook is and how to use it.
The condition in the first bracket the themer has to know it no matter the way he uses to add the style to the page (directly in the template file, using the preprocess_html hook or using the info file).

Here's a different way of looking at it:

There are a load of features that have been incorporated into Drupal core because it was believed that many, if not most, people will use them.

Being able to tag stylesheets according to their appropriate media is something many, if not most, themers can and should use. Isn't that a strong argument in favor of making this easy, so that Drupal themers can focus on the important stuff, and not finding arcane snippets of code they don't understand in order to do what seems patently obvious to them?

And I agree with @JohnAlbin: It's not conditional logic, that's just a phrase. We're talking about a form of registry or tagging, aren't we?

I think this should go in, at least some version of it, but I do see where Dries and webchick are coming from... .info files are so simple to use so everyone wants to stick functionality in them, but this kind of "pseudo-programming" is bad for Drupal's future. We need to encourage code development in PHP (i.e. a real programming language), not some made-up language that no one else knows about :) And the line needs to be drawn somewhere.

That said, I think this is mostly on the right side of the line. As others have said more eloquently above, this is a common case and no different from the normal stylesheets that you can already specify in an .info file (just with different HTML surrounding them when they are printed).

So maybe we can come up with a compromise that still gets the job done but in the simplest way possible? Reviewing the patch, I would say:

1. We should get rid of the "downlevel-revealed" stuff. That part actually is programming logic, and it's too complex for an .info file; plus, it isn't that common of a use case, so requiring the drupal_add_css() method for that seems fine.
2. We should make the syntax as clean as possible. Currently the patch does this:

    stylesheets[all][] = style.css
    stylesheets-conditional[lt IE 7][all][] = fix-ie.css
   

   which I find difficult to read - it looks like programming logic even if it isn't :) For example, why isn't the media type the first array key in both, in order to be consistent (and to make it simpler to read from left to right)?

   Second (and JohnAlbin suggested something similar in #4), wouldn't it be cleaner to merge this with the existing stylesheets key?

   Something like this:

    stylesheets[all][] = style.css
    stylesheets[all][][lt IE 7] = fix-ie.css
   

   would be nice, simple and readable. It might involve some slightly more complicated logic in the theme system itself (to properly parse the different posibilities), but it shouldn't be that bad, and I think it's worth it to correctly express the fact that this is not really "conditional logic" and therefore doesn't need to be labeled as such.

Personally I never ever use conditional style sheets. I do my best to have Internet Explorer 7 and up display everything as best as possible using a single set of CSS, even to the extent of sacrificing some display features, like rounded-corners. I just need to be extra careful, but it's entirely manageable.

However I agree with most people here, we should have conditional stylesheets in D7, which are not programs, but tags that only Internet Explorer is made to look at and use. Having conditional stylesheets makes sense to a lot of people and do really help out.

But the pain point of the current patch, I believe, is like David_Rothstein mentions:

We should get rid of the "downlevel-revealed" stuff. That part actually is programming logic, and it's too complex for an .info file; plus, it isn't that common of a use case, so requiring the drupal_add_css() method for that seems fine.

So, would removing the "downlevel-revealed" components from the patch be enough for Dries and Webchick to reconsider the inclusion of the not so conditional stylesheets in D7?

I just wanted to add my support for this issue.
As a themer, I think David_Rothstein made valid points in #132 and perhaps, Dries/webchick could rethink their position on the issue, after the patch is revised.

I read most of this issue, everything was going great, then the "downlevel-revealed" stuff appeared, and only after that, Dries stopped by on #108. At this point, the patch was trying to accomplish to much.

Also, as David_Rothstein pointed:

stylesheets[all][] = style.css
stylesheets[all][][lt IE 7] = fix-ie.css

Is cleaner and makes much more sense when reading.
But if changing this now is a deal breaker, I'm happy having this committed like it is now, even if it's hard to read.

So... Dries/webchick... without the "downlevel-revealed" stuff would you give this issue another chance of landing in D7? pretty please? with kittens on top?

subscribe

I also want to add my support on this and remention an important point for us which is rtl conditional styles (last seen in #3 and #4) that are not automatically called when the call is embedded in the template. for the same reason, removing the ability to add conditional stylesheets in drupal_add_css() (#125) will be very bad.

would be great!

+1 for this in D7.

Subscribing

I really hope for the sake of theming in Drupal 7, that this is reconsidered for inclusion, especially in light of this recent development #769226: Optimize JS/CSS aggregation for front-end performance and DX. If we can make a change like allowing modules to add stylesheets .info files, we should at least be able to fix this. Working with CSS files in Drupal 7 is currently a horror show, unless you are really comfortable using preprocess functions and alter hooks and even if you are comfortable it still feels dirty. I feel strongly that it is irresponsible to leave this the way it is.

Rerolled for bartik and #769226: Optimize JS/CSS aggregation for front-end performance and DX. Huge DrupalWTF for themers without this.

#141: drupal-conditional-stylesheets-522006-141.patch queued for re-testing.

We need to get rid of the "downlevel-revealed" stuff (based on the above discussion). Hopefully then this can go in.

I'll see if I can find some time to work on this, but not sure if/when I will be able to...

Trying this again.

Silly mistakes.

I'm a huge +45 for putting Conditional Stylesheet support into D7. CSS files are listed in the .info file. That's how Drupal works. It's a big exception to instead print the conditional calls for IE styles in the page.tpl file. Or, er, now, in the html.tpl.php?? Which doesn't even exist even in the core themes. It was hard enough when page.tpl.php was right there. Now it's worse. Or, as jacine talks about above, expecting front-end developers to write template preprocess functions to do something that is so deadly simple in other CMSes. Do we really want to make Drupal even harder and weirder for designers?

I really don't understand some of the comments above:

Proper markup doesn't require any special browser hacks. We should fix the markup (and stylesheets accordingly) instead.

What? We just should never write conditional IE stylesheets and instead just write our CSS 'correctly'?? That's just wrong. And insulting. Using IE-specific stylesheets, and calling them to load conditionally from the html document head IS the best practice. A best practice that was developed after years of star-hacks and other attempts to get IE to behave. Anyone who thinks writing specific styles for IE is a sign of weakness or a lack of ability to write better code just doesn't write CSS professionally all day every day and doesn't know what they are talking about. We have to write CSS for IE — even for IE8 sometimes. (Why do you think we complain about IE so much? Yes, we hate this reality too — but we are stuck with it.) And these IE styles should be called conditionally. Allowing people to list those stylesheets in the .info file with all the others just makes sense.

And then both our core maintainers think that putting the IE stylesheets into the .info file is somehow putting logic into the .info file. I couldn't disagree more strongly.

This is not about programming logic. CSS isn't a programming language. And this technique is a way to avoid browser sniffing and other bad logic-based techniques. The conditional is done by the user agent. Client-side. This is not like PHP. This isn't "programming in the .info file". This is a way to put all the calls for CSS files together. Some are printed straight up, "load this css"; others are printed "if this then load this css" — but the logic itself is not in the .info file. The "logic" is sent forward for the user agent to handle.

NOT doing this leaves us with a giant WTF. One that is much worse in D7 than in D6.

And this issue is an example of what frustrates me about the Drupal community being so dominated by PHP programmers. It's clear to the front-end developers that doing it this way is best. Yet we get overridden by people who don't do front end development (or do it very often) because of a misunderstanding of what this really is. It especially troublesome when a total designer/themer WTF is introduced because PHP coders think it's more elegant that way. It's not more elegant. We have a mess without this patch.

@jensimmons AMEN! I agree with every single word.

subscribe

subscribe

hmz, any reason why conditional js isn't being addressed as well in this issue? feels like most of the logic is exactly the same...

@jensimmons just nailed it. It's the reality that front end developers need and should use conditional stylsheets, and conditional js for that matter. Adding them directly to the .tpl.php is much LESS elegant and "correct" than declaring them in the same place that all other CSS is declared.

Subscribe.

Question - with this patch will RTL styles will be called automatically when in RTL mode ?

Woot! I think the clinching argument is that the "logic" of the conditional comment is interpreted in the user-agent. As far as the info file is concerned, it's saving a string of text. Conditional comments, in this way, are no different from "print" or other media declarations that determine that logic of when stylesheets should be applied, again, in the user-agent.

It's important to remember that the earlier patches in this issue which Dries and webchick rejected did contain programming logic in .info files (they had symbols like "+" being used as magical indicators within the .info file telling Drupal what to do).

In the more recent patches, those have being removed, though.

As for conditional JS, no, that is way out of scope for this issue; even drupal_add_js() does not support that currently! In order to have a chance of getting this patch committed to Drupal 7, we need to keep it focused on the most common use case only.

I reviewed #148 and rerolled it with the following changes:

• Got rid of the last remnants of the "downlevel revealed" stuff (tim.plunkett's patch got rid of almost all of it, but there was a tiny bit left in one of the tests).
• Added PHPDoc for the new property to list_themes(), since all the other properties are already documented there.
• Improved and simplified some of the tests, and made one of them a bit more robust.
• The previous patch introduced a subtle change in the conditional comment used in Bartik - I removed that so we are now using the same exact conditional comment expressions before and after the patch ("lte IE 7" and "IE 6").

I'm still somewhat interested in exploring the cleaner .info file syntax I suggested in #132 (where this would be part of the existing "stylesheets" key rather than a new key). That would be a simpler patch (less code), as well as a simpler syntax for themers. However, it also might introduce some weirdness in the internal array structure, and a tiny-pseudo-API change in the structure of .info files, so I decided to hold off for now and not try that just yet. The current approach is still probably good enough as is.

#157 is an excellent patch, and I believe addresses all concerns that have been raised, especially the ones by webchick and Dries about wanting to keep .info files simple and not embed programming logic. Others have eloquently explained that these more recent patches, unlike the earlier ones webchick and Dries objected to, do not add programming logic within .info files, but rather, add easy-to-grok, easy-to-use, hard-to-get-wrong support for something nearly every theme needs to do (correct for IE's noncompliance with web standards), in a way that is conceptually and syntactically similar to the already existing support for media-targeted stylesheets. The translation from a .info array index to a drupal_add_css() option parameter to HTML markup for a browser to interpret is highly parallel between the media index and the condition index, as implemented by #157.

From webchick in #125:

For argument's sake, can you explain what exactly is so horrible about just doing what D6 Garland does for conditional stylesheets, and embedding them directly in the template file?

1. We have API functions for a reason. drupal_add_css() is the API function for adding a CSS file. It's just bad programming practice to do something an API function is there to do without that API function being invoked. Wrappers around API functions, like a nice easy-to-use .info file are great, because they still end up calling the API function, but adding a CSS file without having drupal_add_css() called is bad. It takes it out of the hook_css_alter() step, out of the automatic discovery of an RTL version of the file if you're using an RTL language, makes it impossible for a #pre_render override of drupal_pre_render_styles() to know how many CSS files are being added to the page should that override choose to implement an alternate workaround or a warning message related to IE's 31 limit, and short-circuits everything else that the API function is there to handle.
2. All 3 of our core themes, garland, bartik, and seven use at least one IE-conditional CSS file, and none of them need to override html.tpl.php for any other reason. Why require people to override html.tpl.php if they don't need to otherwise?
3. It helps developer workflow, code maintenance, and debugging, for a developer to see related things together. All the other theme css files are right there in the .info file. It's less friendly to a theme developer to have some of their files listed there, and others listed in html.tpl.php.
4. Why the distinction between a print.css file listed in the .info file and a ie.css file listed in html.tpl.php (or as is now in HEAD, in template.php)? ~70% of most site's users use IE. Probably less than that print the web page they're viewing. Why is ie.css a second class citizen compared to print.css?

From David in #157:

I'm still somewhat interested in exploring the cleaner .info file syntax I suggested in #132 (where this would be part of the existing "stylesheets" key rather than a new key).

For D8 maybe, but I disagree with any BC break of the D7 .info file syntax. #157 is perfect. I don't think there's any theme developer who would look at the new .info lines in this patch and not know exactly what they mean.

+++ includes/theme.inc	27 Aug 2010 04:29:56 -0000
@@ -168,6 +189,15 @@ function _drupal_theme_initialize($theme
+  // And now add the conditional stylesheets properly.
+  foreach ($final_stylesheets_conditional as $condition => $stylesheets_conditional) {
+    foreach ($stylesheets_conditional as $media => $stylesheets) {
+      foreach ($stylesheets as $name => $stylesheet) {
+        drupal_add_css($stylesheet, array('weight' => CSS_THEME, 'media' => $media, 'browsers' => array('IE' => $condition, '!IE' => FALSE)));
+      }
+    }
+  }

My only nit with this patch is that I think the above hunk needs a comment for why we're setting '!IE' to FALSE. The attached patch adds that code comment. Since this is a code comment addition only, I'm considering myself still eligible to RTBC this patch. So, RTBC!

Powered by Dreditor.

Removing the "API Change" tag. There is no BC break whatsoever introduced here.

#158: stylesheets-conditional-522006-158.patch queued for re-testing.

Re-roll of patch in #158 after core’s inclusion of _system_info_add_path() function and removal of Bartik’s search.js.

#149 - I absolutely agree with every single letter of your post, and I am more of a PHP developer than I am frontend designer.

Completely agreed with jensimmons in #149 - I could do it in template overrides if necessary, but why make it so awkward?

I looked over the most recent patch - the new .info syntax is elegant and familiar from D6. The old syntax from template.php...the less said the better.

This is not going to happen, based on this #575298: Provide non-PHP way to reliably override CSS & #901062-59: Regression: Themes can no longer remove stylesheets by overriding them in .info files, so moving to 8.x.

oops.

Subscribe

Updated patch with tests and module .info support.

I want the testbot to have a whack at this patch. It took over 3 hours to run tests on my system last time I tried. :-p Bumping down to 7.x so the tests are run.

Added missing theme stylesheet and fixed periods in regex patterns.

Add proper .info file defaults to fix numerous PHP warnings and notices.

Subscribe

Looking forward to this. In the meantime, #1125220: Subthemes of core themes trigger an undesired 404 and fail to inherit their parent theme's IE styling is a bug in the old-school way of adding IE CSS files.

...subscribing.

PS/BTW: I thought that comments after #150 wrapped in a second page. Am I missing something?

@klonos: They wrap at 300.

Cross-posting #1209958: Allow themes to load stylesheets conditionally if a certain CSS file exists, which may require similar logic.

#77: conditional-stylesheets-522006-77.patch queued for re-testing.

This issue is not going to happen for D8. I can already see the arguments against it getting into D8. “Why add all the code weight just for IE8? We'd all be stuck with the choice to keep the extra IE8 baggage, which is incredibly annoying and inefficient."

As a patch for Drupal 7, it was possible. We had to support IE6 and IE7 as well. So there was a much bigger need for an easier implementation. Webchick told me in person once that she hoped that issue would just die. IMO, this is a dead issue in D8.

We can simplify our codebase by ripping out the conditional stylesheets from drupal_add_css() and opting for putting IE8-specific CSS support into our front-end instead. See #1471382: Add IE-conditional classes to html.tpl.php

epitaph

To add a single conditional stylesheet via drupal_add_css(), we are forever stuck with using these 37 lines of code:

/**
 * Implements hook_preprocess_html().
 */
function MYTHEME_preprocess_html(&$variables) {
  // Add conditional stylesheets for IE.
  drupal_add_css(
    drupal_get_path('theme', 'mytheme') . '/css/ie.css',
    array(
      'group' => CSS_THEME,
      'browsers' => array(
        'IE' => 'lte IE 7',
        '!IE' => FALSE,
      ),
      'weight' => 999,
      'every_page' => TRUE,
    )
  );
}

/**
 * Implements hook_preprocess_maintenance_page().
 */
function MYTHEME_preprocess_maintenance_page(&$variables) {
  // Add conditional stylesheets for IE.
  drupal_add_css(
    drupal_get_path('theme', 'mytheme') . '/css/ie.css',
    array(
      'group' => CSS_THEME,
      'browsers' => array(
        'IE' => 'lte IE 7',
        '!IE' => FALSE,
      ),
      'weight' => 999,
      'every_page' => TRUE,
    )
  );
}

And, in case anyone thinks I'm being dramatic, I'd like to point out this issue was "needs review" for 14 months. Let's call a dead horse a dead horse.

Half the lines in #180 go away if #1472542: Make theme('maintenance_page') consistent with theme('page') lands.

I'd like to point out this issue was "needs review" for 14 months

People got burnt out on it not making it into D7. And D8 release is still sufficiently far away that this hasn't bubbled up as a priority for people. I don't think that's a useful measure by which to kill this issue.

We can simplify our codebase by ripping out the conditional stylesheets from drupal_add_css() and opting for putting IE8-specific CSS support into our front-end instead. See #1471382.

That is a good reason though, so I agree with "won't fix" on this as long as that issue looks promising to people.