Not only for CSS, but if the patch from #865536: drupal_add_js() is missing the 'browsers' option is applied, then JS browser conditional settings get ignored as well.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeytown2’s picture

I can't repo this for css; advagg does work with browser conditionals. Will test with the JS patch soon.

  drupal_add_css('test1.css', array('browsers' => array('IE' => 'lte IE 7', '!IE' => FALSE)));
  drupal_add_css('test2.css', array('browsers' => array('IE' => 'lte IE 7', '!IE' => FALSE)));

Gives me

<link type="text/css" rel="stylesheet" href="http://127.0.0.1/drupal7/sites/default/files/advagg_css/css__bJS7XBx-TarMsMux90wnkxP4FHrSHgcnFwtQSC866mQ__gJPDiKTMbrNjdeXqe0hFL5kssMtW-79_ql75zu61e34__1CNZ_aPmB1mUYP7PnynpPn1Otpa9ea4eEAedil_DG1Y.css" media="all" />

<!--[if lte IE 7]>
<link type="text/css" rel="stylesheet" href="http://127.0.0.1/drupal7/sites/default/files/advagg_css/css__iW7PfWrsD5rKo4eoE8is-_eVeqrkMm_6aT2XhIPE6rQ__uiZpwaXE860bOhSoGhYQ9YOGcFv2B1_SsiSo6K3Fwhw__1CNZ_aPmB1mUYP7PnynpPn1Otpa9ea4eEAedil_DG1Y.css" media="all" />
<![endif]-->
<link type="text/css" rel="stylesheet" href="http://127.0.0.1/drupal7/sites/default/files/advagg_css/css__8y0f00RY-jYpIeBZaOef1vE2X-eT2A3XJP3SRWSM1R8__e1XiZxlH2K1N90Zp2rHqi7YWBr2i0LqTi17ljkeYaBs__1CNZ_aPmB1mUYP7PnynpPn1Otpa9ea4eEAedil_DG1Y.css" media="all" />
<link type="text/css" rel="stylesheet" href="http://127.0.0.1/drupal7/sites/default/files/advagg_css/css__W3w7D04ecFtH1vdn_yV_l_LCecOkhOZpWZOq4T8oSiY__BcQLPEoS_jFLL-iTHvRZ3YdCeB2smEwKYOL4TIPfOiw__1CNZ_aPmB1mUYP7PnynpPn1Otpa9ea4eEAedil_DG1Y.css" media="screen" />

<!--[if lte IE 8]>
<link type="text/css" rel="stylesheet" href="http://127.0.0.1/drupal7/themes/seven/ie.css?mke2ke" media="all" />
<![endif]-->

<!--[if lte IE 7]>
<link type="text/css" rel="stylesheet" href="http://127.0.0.1/drupal7/themes/seven/ie7.css?mke2ke" media="all" />
<![endif]-->

<!--[if lte IE 6]>
<link type="text/css" rel="stylesheet" href="http://127.0.0.1/drupal7/themes/seven/ie6.css?mke2ke" media="all" />
<![endif]-->

This

  drupal_add_css('test1.css', array('group' => CSS_THEME, 'browsers' => array('IE' => 'lte IE 7', '!IE' => FALSE)));
  drupal_add_css('test2.css', array('group' => CSS_THEME, 'browsers' => array('IE' => 'lte IE 7', '!IE' => FALSE)));

gives me

  <link type="text/css" rel="stylesheet" href="http://127.0.0.1/drupal7/sites/default/files/advagg_css/css__AbK9AsGzcg7Ubb_go8_kQy2b-D5UDi6aS0NOpdrvrYQ__3MHTRHH75m4tGu_Gxp8ico5qM9693u86V7gZ7_CacNM__1CNZ_aPmB1mUYP7PnynpPn1Otpa9ea4eEAedil_DG1Y.css" media="all" />
<link type="text/css" rel="stylesheet" href="http://127.0.0.1/drupal7/sites/default/files/advagg_css/css__W3w7D04ecFtH1vdn_yV_l_LCecOkhOZpWZOq4T8oSiY__BcQLPEoS_jFLL-iTHvRZ3YdCeB2smEwKYOL4TIPfOiw__1CNZ_aPmB1mUYP7PnynpPn1Otpa9ea4eEAedil_DG1Y.css" media="screen" />

<!--[if lte IE 7]>
<link type="text/css" rel="stylesheet" href="http://127.0.0.1/drupal7/sites/default/files/advagg_css/css__iW7PfWrsD5rKo4eoE8is-_eVeqrkMm_6aT2XhIPE6rQ__uiZpwaXE860bOhSoGhYQ9YOGcFv2B1_SsiSo6K3Fwhw__1CNZ_aPmB1mUYP7PnynpPn1Otpa9ea4eEAedil_DG1Y.css" media="all" />
<![endif]-->

<!--[if lte IE 8]>
<link type="text/css" rel="stylesheet" href="http://127.0.0.1/drupal7/themes/seven/ie.css?mke2ke" media="all" />
<![endif]-->

<!--[if lte IE 7]>
<link type="text/css" rel="stylesheet" href="http://127.0.0.1/drupal7/themes/seven/ie7.css?mke2ke" media="all" />
<![endif]-->

<!--[if lte IE 6]>
<link type="text/css" rel="stylesheet" href="http://127.0.0.1/drupal7/themes/seven/ie6.css?mke2ke" media="all" />
<![endif]-->

As for why ie7.css is not included in the aggregate has to do with the preprocess being set to FALSE. Using hook_css_alter, if you change "themes/seven/ie7.css" to have preprocess = TRUE & change the weight of test1/2.css to be the same as ie7.css then things work as expected.

I'll work on a patch for the weight so it only compares the int value as I was having trouble getting my test style sheets to merge in with ie7.css

Putting this inside of a hook_css_alter call made things work as expected

  $css['themes/seven/ie7.css']['preprocess'] = TRUE;
  $weight = $css['themes/seven/ie7.css']['weight'];
  $css['test1.css']['weight'] = $weight;
  $css['test2.css']['weight'] = $weight;
mikeytown2’s picture

further investigation into the css issue hasn't turned up anything out of the ordinary. The weight issue was a red herring. changing the bundler to 1 caused all files in the ie7 group to be in one aggregate.

mikeytown2’s picture

Title: Bundler doesn't respect CSS or JS browser conditionals. » Bundler doesn't respect JS browser conditionals (via core patch).
mikeytown2’s picture

Status: Active » Fixed
FileSize
29.74 KB

did find a bug in regards to array_diff($a, $b);. Correct usage in my case is array_merge(array_diff($a, $b), array_diff($b, $a));.

The patch below has been committed. Long story short is you no longer have to apply the patch in #865536-204: drupal_add_js() is missing the 'browsers' option in order for browser conditionals to work.

markhalliwell’s picture

From #2

changing the bundler to 1 caused all files in the ie7 group to be in one aggregate.

Hmm, maybe this is what was throwing me. Yes, I want everything in one bundle if possible. In my mind though, I would think that browser conditionals are the exception to this? Shouldn't they be aggregated down to one bundle for each type of condition outside of the main bundle?

That's what I was mostly talking about. Perhaps you fixed that in the patch, I haven't quite read all the way through it.

mikeytown2’s picture

In my mind though, I would think that browser conditionals are the exception to this?

ie7 group = browser conditional

markhalliwell’s picture

Well yes, I understand that... but that wasn't happening for me. I was only getting one aggregated CSS file and one JS file. The conditionals were never output as a separate aggregate, perhaps they were thrown in the main aggregated file, dunno... didn't look too hard but it shouldn't do that right? TBH, I probably should have said that this was more of a documented note to check (and have others check) this issue. I'll need to re-test with the dev.

mikeytown2’s picture

The 2 examples in #1 show that on my dev environment I was getting browser conditionals. ctrl-f for <!--[if lte IE 7]> to see where they land in the output. Core could have an issue and thus it's output could be wrong #1034208: drupal_sort_css_js() ignores media and browser keys. If AdvAgg still has an issue I would need reproducible code; given this I expect ___ but in reality I got ___.

I do appreciate the feedback and testing; the array_diff() issue would have taken a long time to find on its own.

I started a list of core issues that advagg should look into #1956142: Intergrate D7 patches. Input on this list is greatly appreciated.

Status: Fixed » Closed (fixed)

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