Problem

  • CSS can be conditionally loaded for limited 'browsers' (e.g., IE) only, but JS cannot.
  • #1077878: Add HTML5shiv to core cannot be done without conditional comment support for JS.
  • Modules like No IE6 cannot load JavaScript files for specific browsers.

Proposed solution

  • Make it consistent with drupal_add_css
  • Hacking a #browsers condition into the current code of drupal_get_js() doesn't really make sense.
  • Instead, further align drupal_get_js() with drupal_get_css(), as originally proposed in #330082-20: Refactor drupal_get_js() to use render layer similar to drupal_get_css() already.
  • Introduce #type 'scripts' -- equivalent to existing #type 'styles'.
  • As already the case for #type 'styles' / drupal_get_css(), properly separate the current processing of JS into a #group_callback and #aggregate_callback.

    Side-benefit: Default behavior can be changed and improved in contrib.

  • Only do the necessary to align #type 'scripts' with 'styles', and to introduce support for 'browsers'.

    A major long-term benefit of doing so is that we're finally able to abstract and merge further parts of the #group_callback and #aggregate_callback logic into common helper functions -- paving the way to get to the long-standing idea of having a single generalized system for "support files"/"page requisites" in Drupal core.

Current State

The last patch submitted #136 has been RTBC.
This patch does everything proposed. It also passed all the JS test, and a couple of new test were added.

Seems good to go.

CommentFileSizeAuthor
#263 Drupal-core--865536-263--brower-key-for-js-do-not-test.patch13.4 KBnatew
#262 Drupal-core--865536-262--brower-key-for-js.patch14.3 KBkhu
#252 drupal_add_js_is-865536-252--229-pass.patch14.21 KBjoelpittet
#252 drupal_add_js_is-865536-252--229-fail.patch7.32 KBjoelpittet
#229 865536-D7-229-PASS.patch14.75 KBmarkhalliwell
#229 865536-D7-229-FAIL.patch7.82 KBmarkhalliwell
#229 interdiff.txt23.63 KBmarkhalliwell
#204 drupal-865536-204.patch22.45 KBeffulgentsia
#204 interdiff.txt1.75 KBeffulgentsia
#202 drupal-865536-202.patch22.22 KBeffulgentsia
#202 interdiff.txt1.56 KBeffulgentsia
#188 drupal-add-js-browsers-865536-188-do-not-test.patch6.23 KBDavid_Rothstein
#178 drupal-865536-178-tests.patch4.39 KBtim.plunkett
#178 drupal-865536-178-combined.patch22.32 KBtim.plunkett
#176 drupal-865536-176-tests.patch17.93 KBtim.plunkett
#176 drupal-865536-176-combined.patch22.45 KBtim.plunkett
#157 865536-149_157.patch22.32 KBscor
#152 865536-152.patch22.52 KBeffulgentsia
#149 865536-149.patch22.32 KBaspilicious
#147 865536-147.patch22.4 KBJacine
#136 865536-136.patch22.33 KBericduran
#129 865536-129.patch22.33 KBeffulgentsia
#129 865536-129.patch22.33 KBeffulgentsia
#128 865536-128.patch22.5 KBeffulgentsia
#126 865536-126.patch18.11 KBeffulgentsia
#125 865536-125.patch18.04 KBeffulgentsia
#120 865536_2.patch20.13 KBcosmicdreams
#119 865536_1.patch18.67 KBericduran
#97 0001-Issue-865536-by-ericduran-Added-drupal_add_js-is-mis.patch19.1 KBericduran
#76 0001-Issue-865536-by-ericduran-Added-drupal_add_js-is-mis.patch18.92 KBericduran
#71 865536_71.patch18.92 KBcosmicdreams
#59 865536.patch17.54 KBericduran
#51 865536-drupal_add_js_2.patch17.32 KBericduran
#49 865536-drupal_add_js.patch17.93 KBericduran
#45 865536-ready-for-regression-testing.patch13.99 KBericduran
#43 865536_closer_2.patch11.64 KBericduran
#42 865536_closer.patch11.21 KBericduran
#41 865536.patch9.23 KBericduran
#40 865536-failed-attempt.patch6.98 KBericduran
#35 handle_browsers_in_drupal_get_js-865536-35.patch1.88 KBjessebeach
#26 865536-drupal_add_js-add-browser-options.patch1.88 KBericduran
#20 handle_browsers_in_drupal_get_js-865536-20.patch2.89 KBsreynen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

arianek’s picture

subscribe

rfay’s picture

subscribe

katbailey’s picture

I had a quick look at this and it seems that the root of the problem (or discrepancy between how it works for css and for js) is that the css is treated as a renderable array and so has pre_render functions added, whereas the js is not and is just output directly.
In drupal_get_css, we have:

  // Render the HTML needed to load the CSS.
  $styles = array(
    '#type' => 'styles',
    '#items' => $css,
  );
  return drupal_render($styles);

Then in system_element_info(), which is where pre_render functions for the various renderable element types are defined:

  $types['styles'] = array(
    '#items' => array(),
    '#pre_render' => array('drupal_pre_render_styles'),
    '#group_callback' => 'drupal_group_css',
    '#aggregate_callback' => 'drupal_aggregate_css',
  );

And the drupal_pre_render_styles function in turn does some crazy processing to deal with IE's 31 css files limitation, including changing the element's type to 'html_tag', which then will get another pre_render function as per system_element_info():

  $types['html_tag'] = array(
    '#theme' => 'html_tag',
    '#pre_render' => array('drupal_pre_render_conditional_comments'),
    '#attributes' => array(),
    '#value' => NULL,
  );

So at least this explains how drupal_pre_render_conditional_comments gets called (I think! :-P).

And there's no room for adding a pre-render function for js because it is not passed through drupal_render, so we'd need to restructure drupal_get_js entirely :-/

At least that's how it seems to me.

altrugon’s picture

Upsss... thank you so much katbailey, I never thought the problem was going to big that big. Now I see why I couldn't find the relation between drupal_pre_render_conditional_comments() and drupal_add_css(), this goes quiet far out of my knowledge.

I'm happy to help but as I said this is quiet a bit for me and I don't think I can solve it alone.

altrugon’s picture

Priority: Major » Critical

I'm going to bump this to critical because after katbailey's comment this looks like a release issue.

dmitrig01’s picture

Assigned: Unassigned » dmitrig01
RobLoach’s picture

Assigned: dmitrig01 » Unassigned

I'm not sure this is the right way around it. jQuery provides jQuery.support and jQuery.browser, which might be a better way aroudn this problem. It makes complete sense to have this for CSS, but for JavaScript, using $.browser might be a better option. Thoughts?

mfer’s picture

Category: bug » feature
Priority: Critical » Normal

This is neither a bug nor is it critical. It would be a nice feature to have. But, it is a feature. And, js support has worked for years without it.

The $scripts variable is built in template_process_html. That means moduleName_process_html and/or themeName_process_html both have the opportunity to alter the scripts before rendering. This is very similar to how you could alter it with preprocess previously.

dmitrig01’s picture

hm, i'd have to side with mfer. Though, I disagree with robloach

altrugon’s picture

Category: feature » bug
Priority: Normal » Critical

Thank you guys,

The solution proposed by mfer is what I was looking for because only IE browsers will read the scripts, but the solutions that Rob Loach offer is not bad either however all browsers will read the script and only IE will be able to render the code.

rfay’s picture

Category: bug » feature
Priority: Critical » Normal

Looks to me like there was an x-post and altrugon didn't mean to set the category or priority back.

altrugon’s picture

upsss... sorry, the page refresh didn't update the settings fieldset.

Thank you rfay.

masagin’s picture

I was quite surprised that the 'browser' option is missing in 'drupal_add_js'. I was pretty sure it's going to work same way as 'drupal_add_css' does. Browser sniffing with $.browser is not an option. Not only its usage is discouraged by jQuery itself but forcing other browsers to load IE fixing scripts (or whole libraries) is a bit harsh.

It would be nice to have the 'browser' option in both add_css and add_js...at least because themers will probably expect it that way.

I will try mfer's solution for now but I'm sad. :(

mfer’s picture

Version: 7.x-dev » 8.x-dev

@bganicky I understand your desire for a 'browser' option. Truthfully, the JS and CSS systems are going to get a bit of rework in D8 and this could be good to be part of that.

But, D7 is feature frozen. If these were found some time ago I'm sure it would have gone in. Now it's just too late. We need to wait for the D8 thaw to add it.

Jeff Burnz’s picture

Subscribe.

Mark Trapp’s picture

Subscribe.

JohnAlbin’s picture

Title: The drupal_add_js() function is missing the 'browsers' option » drupal_add_js() is missing the 'browsers' option
Jacine’s picture

sub

sun’s picture

On jQuery.support/.require, I guess that HTML5shiv should be loaded before the DOM initializes? (or rather, before jQuery hits the DOM and might potentially evaluate/calculate widths/heights of elements)

Thanks @katbailey for the analysis. Looks like we need to add a corresponding #type scripts that implements the identical logic. -- That said, I can only hope that we've implemented sufficient tests for the crazy and extremely complex AJAX lazy-loading code concerning scripts, so these changes won't break existing functionality (which is a pain to debug).

sreynen’s picture

Status: Active » Needs work
FileSize
2.89 KB

Attached is a first stab at restructuring drupal_get_js() to run output through drupal_render() rather than theme(). With aggregation off it works as expected for drupal_add_js() calls like this, the same format drupal_add_css() uses:

<?php

drupal_add_js(drupal_get_path('module', 'test'). '/test.js', array('browsers' => array('IE' => 'lte IE 7', '!IE' => FALSE)));

?>

It doesn't do any conditional comments with aggregation on yet, so that still needs work.

alexanderpas’s picture

Priority: Normal » Major

Major, since #1077878: Add HTML5shiv to core depends on this, which is important for HTML5 in D8

jessebeach’s picture

subscribe

ericduran’s picture

sub.

ericduran’s picture

Status: Needs work » Needs review

switching to needs-review so the testbot can run with the patch.

ericduran’s picture

Status: Needs review » Needs work

never mind, this patch is not really complete.

(sorry :( )

ericduran’s picture

Status: Needs work » Needs review
FileSize
1.88 KB

This is probably the most hack-y patch I've ever written. Anyways here it goes.

This patch separates out any browser specific files from the regular flows and renders those at the end. It also doesn't change the theme('html_tag") everywhere for a drupal_render it just calls render when it's ready to render the specific browser js files.

This works as your would expect it. You pass in

<?php
drupal_add_js(drupal_get_path('module', 'custom') . '/test.js', array('browsers' => array('IE' => 'lte IE 7', '!IE' => FALSE)));

You get

<!--[if lte IE 7]>
<script type="text/javascript" src="http://localhost/drupal.org/8/drupal/sites/all/modules/custom/test.js?lnjdon"></script>
<![endif]--> 

I don't in anyway think this is complete but at the same time I don't think we need to change all the theme('html_tag') calls to render just because we need the drupal_pre_render_conditional_comments function to run for those specific scripts.

sreynen’s picture

What's the down side to changing everything to render? There's already a check for browsers in the pre render function, so checking that again to selectively use render introduces redundancy we should avoid. Are we trying to avoid render for some reason?

This also doesn't seem to do any aggregation on browser-specific JS. Is that intentional?

ericduran’s picture

@sreynen, The pre_render doesn't need to run in any of the js except for the browser specific ones, so is just extra work for no reason. Same applies for switching everything to render, I'm going to safely guess that calling render is going to be a bit slower then the theme function which is pointless because we don't need any of the render functionality anywhere except the browser specific js files.

I didn't aggregate browser-specific js on purpose. I'm not sure on the best practice but browser specific js are usually not aggregated, maybe minified. But yes that was done on purpose.

sreynen’s picture

If there's a significant performance problem with drupal_render(), that makes sense to avoid it where we can. But then why do we use it universally for CSS? Should drupal_get_css() also be updated (in a separate issue) to selectively use drupal_render() like this or is there something different there?

ericduran’s picture

It was just the way I decided to write the patch :-/ I guess we'll see what other people think.

Jacine’s picture

Issue tags: +html5, +HTML5 Sprint: July 2011 - 1

Tagging.

sun’s picture

Status: Needs review » Needs work

Hacking a #browsers condition into the current code doesn't really make sense. We need to align drupal_get_js() with drupal_get_css(). That's quite a major challenge and change.

  1. Add #type 'scripts' to system_element_info(), using a #group_callback like #type 'styles'.
  2. Make drupal_get_js() invoke that #group_callback (which means to move most of the function into the #group_callback function)
  3. Replace custom theme() calls with array('#type' => 'scripts', '#items' => $elements) passed to drupal_render().
  4. ...

In the end, the flow and processing of drupal_get_css() and drupal_get_js() will look fairly similar, and we'll (finally!) likely be able to abstract and merge most of the processing into common helper functions for both. However, that does not have to be part of this issue.

ericduran’s picture

@sun, makes sense. /me stops hacking crap.

iflista’s picture

Status: Needs work » Needs review
jessebeach’s picture

JS filed added with a "browser" option loads before core JS files:
https://skitch.com/jesse.beach/f89qr/google-chrome

Without the "browser" option, the theme JS file loads after the core files:
https://skitch.com/jesse.beach/f89qk/developer-tools-http-drupal.dev

The $browser_output was being prepended to the scripts output. I updated the patch and appended the browser output instead. Otherwise, the patch is unchanged.

aspilicious’s picture

Status: Needs review » Needs work

This needs work, see #32.

ericduran’s picture

Assigned: Unassigned » ericduran

#32 is not an easy task.

Patch to come in a day or two. Right now is not working lol.

ericduran’s picture

ugg, this issue is getting me mad lol. Still not quiet there.

jessebeach’s picture

Feel free to post a partial patch if you want to collaborate. I can't promise that I can add much, but maybe I can help a little.

ericduran’s picture

FileSize
6.98 KB

Ok, here's an un-complete patch.

This is as stable as I can get it, I'm still messing with the groups and thats wayyyy broken, so I didn't want to put that one up.

On this patch the order is completely out of whack, and the settings are loaded 1st.

This is still missing the group and aggregate function which is where most of the real logic will be moved too. /me is struggling but I'll keep trying.

ericduran’s picture

FileSize
9.23 KB

This is a better patch with what I believe to be a proper working group function and also maybe proper aggregation of the groups

ericduran’s picture

FileSize
11.21 KB

here's another patch for anyone interested in helping.

This is by no ways finish, also make sure you have devel install, I'm sure I have some dsm's in there.

This patch sets up the proper groups, the aggration function is currently not working correct.

Also I print out what the new output is going to be. Is getting there, but still not there yet.

ericduran’s picture

FileSize
11.64 KB

Another patch, getting there, there's still something wrong with the grouping, I'm loosing the key somewhere which is causing each file to be render either individually or empty.

#Needs-more-eyes ;)

sun’s picture

Wow, amazing progress! Thanks, @ericduran!!

That's exactly the direction we need to go. Probably still some heavy lifting required, but I'm confident we'll get there. I'll try to loop in @effulgentsia, who is the person most familiar with this code to my knowledge.

ericduran’s picture

Thanks @sun.

Here's an actual working patch. This one is actually ready for testing. Still needs tons of clean-up and there's some to-dos in there. But all the old functionality should be working now.

I tested some ajax, but not a lot. Everything seems to work as expected.

I guess I just needed sleep.

ericduran’s picture

Status: Needs work » Needs review
ericduran’s picture

This is for functionality review only. I need to get some actual work done now, so we'll see what test are failing so we can get those fixed and then we can move on from there.

TODO:
- better variable names (I got lazy, sorry)
- Test for browsers options, - I haven't even touch this part yet
- There's some functionality in the aggrate_js that should be in the group_js.
- Comments!!

ericduran’s picture

Lol, this actually works, is cleanup and testing time.

ericduran’s picture

FileSize
17.93 KB

This is another patch, that is ready for testing.

I'll love to see @effulgentsia thoughts on this patch ;)

This should handle the browser option properly now too.

What this patch does:

- Add #type 'scripts' to system_element_info(), using a #group_callback like #type 'styles'.
- Make drupal_get_js() invoke that #group_callback
- Replace custom theme() calls with array('#type' => 'scripts', '#items' => $elements) passed to drupal_render().
- added drupal_aggregate_js, drupal_group_js to group the js and the aggregation
- Add the browser option into drupal_js_defaults, and also added it into the standard javascript array for misc/drupal.js, and settings.

sun’s picture

Thanks!

+++ b/includes/common.inc
@@ -4089,12 +4095,72 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
   $processed = array();
...
   $preprocess_js = (variable_get('preprocess_js', FALSE) && (!defined('MAINTENANCE_MODE') || MAINTENANCE_MODE != 'update'));

These two seem to be obsolete in the #pre_render function.

+++ b/includes/common.inc
@@ -4105,120 +4171,207 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+  // Since JavaScript may look for arguments in the URL and act on them, some
+  // third-party code might require the use of a different query string.
+  $js_version_string = variable_get('drupal_js_version_query_string', 'v=');
...
-  // Since JavaScript may look for arguments in the URL and act on them, some
-  // third-party code might require the use of a different query string.
-  $js_version_string = variable_get('drupal_js_version_query_string', 'v=');

This could be reverted to save some bytes ;)

+++ b/includes/common.inc
@@ -4105,120 +4171,207 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
     '#tag' => 'script',
+    '#type' => 'html_tag',

#type should come first.

+++ b/includes/common.inc
@@ -4105,120 +4171,207 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+      case 'file':
...
         else {
-          // By increasing the index for each aggregated file, we maintain
-          // the relative ordering of JS by weight. We also set the key such
-          // that groups are split by items sharing the same 'group' value and
-          // 'every_page' flag. While this potentially results in more aggregate
-          // files, it helps make each one more reusable across a site visit,
-          // leading to better front-end performance of a website as a whole.
-          // See drupal_add_js() for details.
-          $key = 'aggregate_' . $item['group'] . '_' . $item['every_page'] . '_' . $index;
-          $processed[$key] = '';
-          $files[$key][$item['data']] = $item;
...
-  // Aggregate any remaining JS files that haven't already been output.
-  if ($preprocess_js && count($files) > 0) {
-    foreach ($files as $key => $file_set) {
-      $uri = drupal_build_js_cache($file_set);
-      // Only include the file if was written successfully. Errors are logged
-      // using watchdog.
-      if ($uri) {
-        $preprocess_file = file_create_url($uri);
-        $js_element = $element;
-        $js_element['#attributes']['src'] = $preprocess_file;
-        $processed[$key] = theme('html_tag', array('element' => $js_element));
-      }

I've the impression that these two hunks got lost -- perhaps I'm missing/overlooking something though.

+++ b/includes/common.inc
@@ -4105,120 +4171,207 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+ *   An array of JS groups. Each group contains the same keys (e.g., ¶
+ *   'data', etc.) as a JS item from the $javascripts parameter, with the value of
+ *   each key applying to the group as a whole. Each group also contains an
+ *   'items' key, which is the subset of items from $javascripts that are in the group.

Trailing white-space and partially exceeding 80 chars (elsewhere, too).

16 days to next Drupal core point release.

ericduran’s picture

FileSize
17.32 KB

Changes:

- Removed - $processed since its pointless now
- Reverted the $js_version_string to its previous position to avoid a change.
- Move #type to be the 1st key in the array.
- Removed trailing whitespace, and anything over 80 chars

- The file groups got moved to to drupal_group_js
- The drupal_build_js_cache was moved to drupal_aggregate_js but there is no check like there was before for case when the file isn't created.

Still needs work, but this one is a bit closer.

altrugon’s picture

Thank you so much for all the work you are putting on this ericduran.

effulgentsia’s picture

I haven't read the full discussion here, but I totally support the direction of this patch. Consistency++. A year ago, after the corresponding refactoring of drupal_get_css() went in, I asked in #330082-20: Refactor drupal_get_js() to use render layer similar to drupal_get_css() if there was sufficient incentive to refactor drupal_get_js() along the same lines, but it wasn't clear there was a strong enough use-case to justify the investment of time. Great to see the 'browsers' use-case creating that justification, and thank you, @ericduran, for running with it.

#51 looks really close to me. Here's some feedback, but at this point, this is just about fine tuning. Additionally, I'm a little concerned we don't have adequate automated tests for all the combinations of options and how they affect aggregation and tag order. I don't know if we want to make creating those tests a requirement of this patch.

+++ b/includes/common.inc
@@ -4089,13 +4095,71 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+  // stripped off settings, potentially in order to override how settings get

Although this is just a copy/paste from elsewhere, might as well fix the typo: s/off/of/

+++ b/includes/common.inc
@@ -4089,13 +4095,71 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+  // Loop through the JavaScript to construct the rendered output.
+  $elements = array(
+    '#type' => 'scripts',
+    '#items' => $items,
+  );
+
+  return drupal_render($elements);

The comment doesn't make sense for this code. Perhaps something like "Render the HTML needed to load the JavaScript."

+++ b/includes/common.inc
@@ -4089,13 +4095,71 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
   // The index counter is used to keep aggregated and non-aggregated files in
   // order by weight.
   $index = 1;
-  $processed = array();
   $files = array();
-  $preprocess_js = (variable_get('preprocess_js', FALSE) && (!defined('MAINTENANCE_MODE') || MAINTENANCE_MODE != 'update'));
+  $new_elements = array();

Neither $index nor $files are used in this function, so can be removed. It's not good practice for a #pre_render callback to return an entirely new array (i.e., $new_element). Instead, add each new element to $elements, and return $elements.

+++ b/includes/common.inc
@@ -4115,110 +4179,198 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
-        $js_element['#value'] = 'jQuery.extend(Drupal.settings, ' . drupal_json_encode(drupal_array_merge_deep_array($item['data'])) . ");";
+        $js_element['#value'] = 'jQuery.extend(Drupal.settings, ' . drupal_json_encode(drupal_array_merge_deep_array($group['items'][0]['data'])) . ");";

This is making an assumption that there's only a single setting item within $group, and it's at index 0. While this is a reasonable assumption, given that drupal_add_js() creates just one setting item period, and the 'setting' type isn't grouped within drupal_group_js(), I think it would be clearer to loop this the same way that you do for 'inline' and 'external'. In fact, I wonder if the code could be shortened by doing the common things for setting, inline, and external just once.

+++ b/includes/common.inc
@@ -4115,110 +4179,198 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+        // The group has been aggregated into a single file: output a LINK tag
+        // for the aggregate file.

Copy/paste artifact? We're not outputting LINK tags here.

+++ b/includes/common.inc
@@ -4115,110 +4179,198 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+          $js_element['#browsers'] = $group['browsers'];

I like the addition of the 'browsers' option, but why only for files? Seems like 'inline' and 'external' should benefit from it too. Given the way drupal_add_js() squashes 'setting' items, that one may need further discussion to support a 'browsers' option.

+++ b/includes/common.inc
@@ -4115,110 +4179,198 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+            if ($item['defer']) {
+              $js_element['#attributes']['defer'] = 'defer';
+            }

Legacy issue, but we're ignoring 'defer' on aggregated files. Not sure if we want to fix that as part of this issue (for example, by making drupal_add_js() set 'preprocess' to FALSE when 'defer' is TRUE, similarly to what it already does for 'cache'), or defer (sorry, couldn't resist) it to another issue.

+++ b/includes/common.inc
@@ -4115,110 +4179,198 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+          // Preprocessing for external JavaScript files is ignored.

This comment seems out of place here.

+++ b/includes/common.inc
@@ -4115,110 +4179,198 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+ * This function arranges the JS items that are in the #items property of the
+ * script element into groups. Arranging the JS items into groups serves two
+ * purposes. When aggregation is enabled, files within a group are aggregated
+ * into a single file, significantly improving page loading performance by
+ * minimizing network traffic overhead.

s/script/scripts/
Remove the sentence about "two purposes", since here there's only one purpose.

+++ b/includes/common.inc
@@ -4115,110 +4179,198 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+ * and if they are for the same 'browsers'. Items of the 'file' type
+ * are groupable if their 'preprocess' flag is TRUE, items of the 'inline' type
+ * are never groupable, items of the 'settings' type are never groupable and
+ * items of the 'external' type are never groupable.

Seems this can be shortened to just 'file' and not 'file'.

+++ b/includes/common.inc
@@ -4115,110 +4179,198 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+ * @param $javascripts

In other parts of core, we refer to this variable as $javascript (singular), so for consistency, let's do the same here. Yes, it's an array with usually more than one item, so if we want to globally change it to plural throughout core, let's open a new issue to discuss that.

+++ b/includes/common.inc
@@ -4115,110 +4179,198 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    ksort($item['browsers']);

Needs a code comment explaining why.

+++ b/includes/common.inc
@@ -4115,110 +4179,198 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    switch ($item['type']) {
+      case 'file':
+        // Group file items if their 'preprocess' flag is TRUE.
+        // Help ensure maximum reuse of aggregate files by only grouping
+        // together items that share the same 'group' value and 'every_page'
+        // flag. See drupal_add_js() for details about that.
+        $group_keys = $item['preprocess'] ? array($item['type'], $item['group'], $item['every_page'], $item['browsers']) : FALSE;
+        break;
+      case 'external':
+      case 'setting':
+      case 'inline':
+        // Do not group external, settings, and inline items.
+        $group_keys = FALSE;
+        break;
     }

Minor nit, but would a construct more like if ($item['type'] == 'file' && $item['preprocess']) {...} else {...} be easier to read?

+++ b/includes/common.inc
@@ -4115,110 +4179,198 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+ * other unnecessary content. Additionally, this functions aggregates inline
+ * content together, regardless of the site-wide aggregation setting.

It does not do the last sentence.

+++ b/includes/common.inc
@@ -4115,110 +4179,198 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    switch ($group['type']) {
+      // If a file group can be aggregated into a single file, do so, and set
+      // the group's data property to the file path of the aggregate file.
+      case 'file':
+        if ($group['preprocess'] & $preprocess_js) {

Since there's only one 'case', does it make sense to remove the switch construct and add the 'type' condition to the if expression?

+++ b/includes/common.inc
@@ -4115,110 +4179,198 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+          // TODO: The key needs to be the data attribute of the item, in order
+          // to aggragate them properly. This should be properly set up in drupal_group_js
+          // as we don't want to loop the files yet again here.
+          foreach ($group['items'] as $js_keys => $file) {
+            unset($group['items'][$js_keys]);
+            $group['items'][$file['data']] = $file;
+          }

Alternatively, how about changing drupal_build_js_cache() to use $item['data'] instead of the array index? That would make it consistent with drupal_build_css_cache().

Powered by Dreditor.

omercioglu’s picture

Sub

ericduran’s picture

@effulgentsia, wow, thanks for that. I'll go through an implement those recommendation, I'll also clean up and add proper comments.

sun’s picture

FYI: I've updated the issue summary.

I like the addition of the 'browsers' option, but why only for files? [...] Given the way drupal_add_js() squashes 'setting' items, that one may need further discussion to support a 'browsers' option.

I agree that we should support the 'browsers' option for 'inline' and 'external', too. However, I'd leave out support for 'setting', as that deserves a separate discussion and possibly requires larger changes.

catch’s picture

Bringing these two into line would be great.

I'm crossposting #1014086-47: Stampedes and cold cache performance issues with css/js aggregation which already has some code to move some of the css/js rendering stuff to helper functions. As others have said that can be done separately.

Jacine’s picture

Issue tags: +HTML5 Sprint: August 2011 - 1

Tagging.

ericduran’s picture

FileSize
17.54 KB

Here's a patch witch takes into account all of @effulgentsia comments,

I'll post up a review in a sec.

ericduran’s picture

Ok so Here's what the latest patch does #59

  • Adds support for the 'browsers' option to be pass into drupal_add_js
  • Adds an element of type "scripts"
  • Adds a drupal_group_js to properly group the JS as appropriate
  • Adds a drupal_pre_render_scripts for rendering the appropriate tags
  • Puts drupal_get_js in line with all the same functionality as drupal_get_css which makes it possible for a follow up issue about abstracting and mergin out common functionality.

I'll try to do another check tomorrow to make sure I didn't miss anything.

Jacine’s picture

Issue tags: -HTML5 Sprint: August 2011 - 1 +HTML5 Sprint: August 2011 - 2

Still needs to be reviewed and tested. Updating to current sprint.

effulgentsia’s picture

Status: Needs review » Needs work

Getting very close.

+++ b/includes/common.inc
@@ -4115,110 +4173,185 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
-      case 'setting':
-        $js_element = $element;
-        $js_element['#value_prefix'] = $embed_prefix;
-        $js_element['#value'] = 'jQuery.extend(Drupal.settings, ' . drupal_json_encode(drupal_array_merge_deep_array($item['data'])) . ");";
-        $js_element['#value_suffix'] = $embed_suffix;
-        $output .= theme('html_tag', array('element' => $js_element));
...
-   return implode('', $processed) . $output;

By removing this, and replacing with the logic in this patch, Drupal.settings is no longer rendered after all other JavaScript. I believe HEAD is wrong for forcing settings to be last in this way, and that the patch is correct. If it's important for Drupal.settings to be last, then within drupal_add_js(), the settings object should be given a JS_SETTINGS group that is defined as a larger integer than JS_THEME.

+++ b/includes/common.inc
@@ -4115,110 +4173,185 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    $query_string =  empty($group['version']) ? $default_query_string : $js_version_string . $group['version'];

This shouldn't be evaluated at the group level, since each item can have its own version, and when site-wide aggregation is disabled, it's the item version that matters.

+++ b/includes/common.inc
@@ -4089,13 +4095,65 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+ *   - '#group_callback': A function to call to group #items to enable the use
+ *     of fewer tags by aggregating files statements within a single tag.

The whole fewer tags business was to deal with an IE limitation specifically surrounding LINK and STYLE tags. For JS, the only reason for grouping is to aggregate. So, how about "A function to call to group #items. Following this function, #aggregate_callback is called to aggregate items within the same group into a single file."

+++ b/includes/common.inc
@@ -4115,110 +4173,185 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+        // The group has been aggregated into a single file: output a LINK tag
+        // for the aggregate file.

Remove the "output a LINK tag ..." bit. "The group has been aggregated into a single file." is sufficient.

+++ b/includes/common.inc
@@ -4115,110 +4173,185 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
         else {

Might be nice to add a comment above this: "The group has not been aggregated into a single file. For example, aggregation might be disabled for a site during development."

+++ b/includes/common.inc
@@ -4115,110 +4173,185 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+ * scripts element into groups. When aggregation is enabled, files within ¶

Remove trailing whitespace.

+++ b/includes/common.inc
@@ -4115,110 +4173,185 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+ * items of the 'external' type are never groupable.
+ * This function also ensures that the process of grouping items does not change

Either remove the line break between these lines, or insert another one.

+++ b/includes/common.inc
@@ -4115,110 +4173,185 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+ * @return
+ *   An array of JS groups. Each group contains the same keys (e.g.,
+ *   'data', etc.) as a JS item from the $javascripts parameter, with the value
+ *   of each key applying to the group as a whole. Each group also contains an
+ *   'items' key, which is the subset of items from $javascripts that are in the
+ *   group.
+ *
+ * @see drupal_pre_render_scripts()
+ */
+function drupal_group_js($javascripts) {

s/$javascripts/$javascript/

aspilicious’s picture

+++ b/includes/common.incundefined
@@ -4115,110 +4173,185 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+ * scripts element into groups. When aggregation is enabled, files within ¶

Tiny issue, trailing whitespace

6 days to next Drupal core point release.

stevetweeddale’s picture

Tagging for the current HTML5 sprint

dynamicdan’s picture

Is this going to miss the boat for D7?
Seems like it's already done but not 100% approved :(

FYI, this is like 2 years over due. I'm been supporting IE7 using conditional js for ages.... how did this miss the original function design? Some dev thought it was bad coding design but ok in drupal_add_css ??

Also, my use case is for excanvas/FLOT support if interested.

klonos’s picture

cosmicdreams’s picture

Looks like what this patch needs is the implementation changes that #62 and #63 call for. I'll get to work on that.

KrisBulman’s picture

subscribing

cosmicdreams’s picture

Ended up not having time to work on this yesterday. Look like I will have a few hours near the end of the work day to touch on this.

cosmicdreams’s picture

working on this issue finding additional grammatical issues. Should I care?

cosmicdreams’s picture

FileSize
18.92 KB

Here is a patch with all of the changes requested to comments and grammar the top two items that effulgentsia mentions have not been done. I'll actually have to read an understand what this code is doing in order to do that.

cosmicdreams’s picture

Status: Needs work » Needs review
ericduran’s picture

Status: Needs review » Needs work

Lets wait till the actual code changes are made to mark it for review.

That way effulgentsia won't review the code till it's ready.

cosmicdreams’s picture

Since I only marked this issue as need review in order to see if I had created the patch properly and have the testbot validated it, that should be good.

If anyone else has working knowledge about this patch and know how to implement the suggestions listed above feel free to jump in.

Jacine’s picture

Issue tags: -HTML5 Sprint: July 2011 - 1, -HTML5 Sprint: August 2011 - 2 +D8H5

Tagging. Maybe this sprint is the lucky one?

Quick update: Per @ericduran This issue needs 2 small changes and will take him a couple of hours.

ericduran’s picture

Status: Needs work » Needs review
FileSize
18.92 KB

This patch fixes :
- Drupal.settings is now render last. I added a new JS_SETTINGS constant with a which gives the settings group a weight of 200.
- The $query_string added at the end of the file is now on a per file or per group basis. Which makes it work correctly even when aggregation is turned off.
- And all the comments/white spaces/line breaks are fixed.

This is now ready for some serious review.

There's one small thing I'll like to change about it, but honestly that doesn't have to go in with this issue. The one thing is forcing the browser group to render last before the settings. This should actually be happening now. But I need to review it a little more to see why thats not happening.

For anyone that wants the test, here are some things you might want to test for:
- Make sure all the expected JS is on the page ;-)
- Add some JS in a module with the browser option
- Add some JS in a theme with the browser option

RobLoach’s picture

I like where this is going, but I still would prefer using #1033392: Script loader support in core (LABjs etc.), with checks to jQuery.browser.

cosmicdreams’s picture

@Rob Loach: why? I don't mean to be cheeky here. I'm interesting in your opinion.

RobLoach’s picture

@cosmicdreams: Oh, just because it seems cleaner than adding a bunch more HTML and code ;-) . I'm happy either way though.

ericduran’s picture

@Rob Loach, This really doesn't block the other issue.

This is honestly an improvement to drupal_get_js which just happens to allow the browser option but it also makes it easier to to add other improvements on top of it.

After we get this one in, we can open another issue to combine drupal_render_style, drupal_render_scripts, and drupal_group_js, drupal_group_css.

Either way, getting this one in doesn't mean we need to use the browser option for the html5 shiv. It just means drupal_get_js is better than it was before ;)

Jacine’s picture

Yeah, I don't really understand why the script loading issue keeps getting referenced to this. One has nothing to do with the other. Also, every single article and book I have every read, recommends using conditional conditional comments for the shiv. Why on earth would be use a script loading solution for that? anyway, this issue is absolutely not meant to replace any true solution for dealing with polyfills. Like Eric said, it's to make it better and more consistent with drupal_add_js more consistent with drupal_add_css.

cosmicdreams’s picture

I'm in the process of testing this now. I just installed the patch and setup my drupal 8 site and I see that all of the js from the system and Bartik theme are present.

I'm about to test adding a new javascript into the mix by creating a module for that job. What is the proper syntax I should use?

ericduran’s picture

cosmicdreams’s picture

cool, I created a module named test_patch, and put the proper test_patch.module and test_patch.info files in place. Here's the contents of my test_patch.module:

function test_patch_page_alter(&$page) {
  drupal_add_js('//html5shiv.googlecode.com/svn/trunk/html5.js', array('group' => JS_DEFAULT, 'browsers' => array('IE' => 'lte IE 7', '!IE' => FALSE)));
}

now i'm going to install that module and inspect the result.

cosmicdreams’s picture

There must be something wrong with my module because the text is simply being spat out at the top. I'll test adding it to a theme.

Added the new line of code to the theme...
Oh, I think I figured out what I did wrong with adding that line to the module. I'll test both.

ericduran’s picture

You don't need the group, also you didn't specified it as en external js file so that won't work like that @see http://api.drupal.org/api/drupal/includes--common.inc/function/drupal_ad...

cosmicdreams’s picture

ok, with your corrections I turn my attention to testing this within a theme. Here's the code I added to bartik's bartik_preprocess_html

drupal_add_js('//html5shiv.googlecode.com/svn/trunk/html5.js', array('type' => external, 'browsers' => array('IE' => 'lte IE 7', '!IE' => FALSE)));

testing...
result:
* javascript shows up in Drupal.setting's CDATA:

<!--//--><![CDATA[//><!--
jQuery.extend(Drupal.settings, {"basePath":"\/","pathPrefix":"","ajaxPageState":{"theme":"bartik","theme_token":"-8h1aGdZMI8VZHFprRcbMDoG8JNFwmSGweA4T_bu4uA","js":{"misc\/jquery.js":1,"misc\/jquery.once.js":1,"misc\/drupal.js":1,"misc\/ui\/jquery.ui.core.min.js":1,"misc\/jquery.ba-bbq.js":1,"modules\/overlay\/overlay-parent.js":1,"modules\/contextual\/contextual.js":1,"misc\/jquery.cookie.js":1,

"\/\/html5shiv.googlecode.com\/svn\/trunk\/html5.js":1

,"modules\/toolbar\/toolbar.js":1},"css":{"modules\/system\/system.base.css":1,"modules\/system\/system.menus.css":1,"modules\/system\/system.messages.css":1,"modules\/system\/system.theme.css":1,"misc\/ui\/jquery.ui.core.css":1,"misc\/ui\/jquery.ui.theme.css":1,"modules\/overlay\/overlay-parent.css":1,"modules\/contextual\/contextual.css":1,"modules\/comment\/comment.css":1,"modules\/field\/theme\/field.css":1,"modules\/node\/node.css":1,"modules\/search\/search.css":1,"modules\/user\/user.css":1,"modules\/shortcut\/shortcut.css":1,"modules\/toolbar\/toolbar.css":1,"themes\/bartik\/css\/layout.css":1,"themes\/bartik\/css\/style.css":1,"themes\/bartik\/css\/colors.css":1,"themes\/bartik\/css\/print.css":1,"themes\/bartik\/css\/ie.css":1}},"overlay":{"paths":{"admin":"node\/*\/edit\nnode\/*\/delete\nnode\/*\/revisions\nnode\/*\/revisions\/*\/revert\nnode\/*\/revisions\/*\/delete\nnode\/add\nnode\/add\/*\noverlay\/dismiss-message\nuser\/*\/shortcuts\nadmin\nadmin\/*\nbatch\ntaxonomy\/term\/*\/edit\nuser\/*\/cancel\nuser\/*\/edit\nuser\/*\/edit\/*","non_admin":"admin\/structure\/block\/demo\/*\nadmin\/reports\/status\/php"},"ajaxCallback":"overlay-ajax"},"tableHeaderOffset":"Drupal.toolbar.height"});
//--><!]]>

Is that the intended result?

ericduran’s picture

I would test whats currently supported as thats all thats expected to work, so I wouldn't do //... instead pick a protocol for an external file or your a local file.

cosmicdreams’s picture

will do, but just so I know. What's the result I should be expecting? Can you post code I can compare to?

for example: should I expect that the browser specific code is wrapped in a conditional comment that only impacts a version of IE like I would expect to see for conditional css?

cosmicdreams’s picture

corrected code to :

drupal_add_js('http://html5shiv.googlecode.com/svn/trunk/html5.js', array('type' => 'external', 'browsers' => array('IE' => 'lte IE 7', '!IE' => FALSE)));

do we all agree that the code is syntactically correct?
testing...
OK, I've got a good result this time. You see, apparently, I hadn't applied the patch yet.... sorry.

So I've got a good perspective of before patch and after patch behavior.

Before patch the line of code above would simply place the new script in the list of other included scripts without additional comments.
After the patch I get this:

<!--[if lte IE 7]>
<script type="text/javascript" src="http://html5shiv.googlecode.com/svn/trunk/html5.js">
<!--//--><![CDATA[//>

Now I'm off to test this in a module...

cosmicdreams’s picture

OK, previously mentioned module, I change my hook_page_alter to this:

function test_patch_page_alter(&$page) {
  drupal_add_js('//html5shiv.googlecode.com/svn/trunk/html5.js',
    array('group' => JS_DEFAULT,
          'type' => 'external',
          'browsers' => array('IE' => 'lte IE 7', '!IE' => FALSE)
         )
  );
}

Syntactically it looks right although excessive
testing...
works great! Here's the output:

<!--[if lte IE 7]>
<script type="text/javascript" src="//html5shiv.googlecode.com/svn/trunk/html5.js">
<!--//--><![CDATA[//><!--

//--><!]]>

Yea, in both cases there seems to be an extra, blank conditional comment included. Is that intentional?

cosmicdreams’s picture

So in summary, works for me. I recommend RBTC status.

ericduran’s picture

@cosmicdreams try without the external flag. I'm not sure why it would be adding the extra cdata there I'll take a look.

ericduran’s picture

Actually the cdata is added to all inline, settings, and external scripts.

That shouldn't be the case, good catch.

cosmicdreams’s picture

standing by for another 15 minutes or so if you want me to retest.

cosmicdreams’s picture

If there a specific lines of code you want me to test please paste them to the issue.

ericduran’s picture

And this should fix that extra cdata tag.

cosmicdreams’s picture

testing...

Works great!

from module

<!--[if lte IE 7]>
<script type="text/javascript" src="//html5shiv.googlecode.com/svn/trunk/html5.js"></script>
<![endif]-->

from theme

<!--[if lte IE 7]>
<script type="text/javascript" src="http://html5shiv.googlecode.com/svn/trunk/html5.js"></script>
<![endif]-->
cosmicdreams’s picture

Is it worth the time to test the patch if the javascript's type is 'inline' or 'file'?

ericduran’s picture

We can test file, but that should just work like external.

I haven't tried inline but we should probably test that too. I think it'll work fine, but we should definitely testing.

cosmicdreams’s picture

cool, I ran out of steam last night. I'll try to test this evening. I'll use drupal_add_js's examples as a basis for testing.

cosmicdreams’s picture

OK, I have an hour before I pass out. LET'S DO THIS!

Going to attempt to test with a modified set of examples that are shown in the api doc for drupal_add_js.
Testing...
Looks like I have some interesting results. First, for my test case I added the following to Bartik's bartik_preprocess_html:

drupal_add_js('misc/collapse.js');
  drupal_add_js('misc/collapse.js', 'file');
  drupal_add_js('misc/collapse.js', array('type' => 'file', 'browsers' => array('IE' => 'lte IE 7', '!IE' => FALSE)));
  drupal_add_js('jQuery(document).ready(function () { alert("Hello!"); });', 'inline');
  drupal_add_js('jQuery(document).ready(function () { alert("Hello!"); });',
    array('type' => 'inline', 'scope' => 'footer', 'weight' => 5)
  );
  drupal_add_js('jQuery(document).ready(function () { alert("Hello!"); });',
    array('type' => 'inline', 'scope' => 'footer', 'browsers' => array('IE' => 'lte IE 7', '!IE' => FALSE), 'weight' => 5)
  );
  drupal_add_js('http://example.com/example.js', 'external');
  drupal_add_js('http://example.com/example.js', array('type' => 'external', 'browsers' => array('IE' => 'lte IE 7', '!IE' => FALSE)));
  drupal_add_js(array('myModule' => array('key' => 'value')), array('type' => 'setting', 'browsers' => array('IE' => 'lte IE 7', '!IE' => FALSE)));

Here's what resulted in the header

<style type="text/css" media="print">@import url("http://localhost/themes/bartik/css/print.css?lsksft");</style>

<!--[if lte IE 7]>
<link type="text/css" rel="stylesheet" href="http://localhost/themes/bartik/css/ie.css?lsksft" media="all" />
<![endif]-->
  <script type="text/javascript" src="http://localhost/misc/jquery.js?v=1.4.4"></script>
<script type="text/javascript" src="http://localhost/misc/jquery.once.js?v=1.2"></script>
<script type="text/javascript" src="http://localhost/misc/drupal.js?lsksft"></script>
<script type="text/javascript" src="http://localhost/misc/ui/jquery.ui.core.min.js?v=1.8.7"></script>
<script type="text/javascript" src="http://localhost/misc/jquery.ba-bbq.js?v=1.2.1"></script>
<script type="text/javascript" src="http://localhost/modules/overlay/overlay-parent.js?v=1.0"></script>
<script type="text/javascript" src="http://localhost/modules/contextual/contextual.js?v=1.0"></script>
<script type="text/javascript" src="http://localhost/misc/jquery.cookie.js?v=1.0"></script>

<!--[if lte IE 7]>
<script type="text/javascript" src="//html5shiv.googlecode.com/svn/trunk/html5.js"></script>
<![endif]-->
<script type="text/javascript">
<!--//--><![CDATA[//><!--
jQuery(document).ready(function () { alert("Hello!"); });
//--><!]]>
</script>

<!--[if lte IE 7]>
<script type="text/javascript" src="http://localhost/misc/collapse.js?lsksft"></script>
<![endif]-->

<!--[if lte IE 7]>
<script type="text/javascript" src="http://example.com/example.js"></script>
<![endif]-->
<script type="text/javascript" src="http://localhost/modules/toolbar/toolbar.js?lsksft"></script>
<script type="text/javascript">
<!--//--><![CDATA[//><!--
jQuery.extend(Drupal.settings, {"basePath":"\/","pathPrefix":"","ajaxPageState":{"theme":"bartik","theme_token":"-8h1aGdZMI8VZHFprRcbMDoG8JNFwmSGweA4T_bu4uA","js":{"0":1,"1":1,"misc\/jquery.js":1,"misc\/jquery.once.js":1,"misc\/drupal.js":1,"misc\/ui\/jquery.ui.core.min.js":1,"misc\/jquery.ba-bbq.js":1,"modules\/overlay\/overlay-parent.js":1,"modules\/contextual\/contextual.js":1,"misc\/jquery.cookie.js":1,"\/\/html5shiv.googlecode.com\/svn\/trunk\/html5.js":1,"2":1,"misc\/collapse.js":1,"http:\/\/example.com\/example.js":1,"modules\/toolbar\/toolbar.js":1},"css":{"modules\/system\/system.base.css":1,"modules\/system\/system.menus.css":1,"modules\/system\/system.messages.css":1,"modules\/system\/system.theme.css":1,"misc\/ui\/jquery.ui.core.css":1,"misc\/ui\/jquery.ui.theme.css":1,"modules\/overlay\/overlay-parent.css":1,"modules\/contextual\/contextual.css":1,"modules\/comment\/comment.css":1,"modules\/field\/theme\/field.css":1,"modules\/node\/node.css":1,"modules\/search\/search.css":1,"modules\/user\/user.css":1,"modules\/shortcut\/shortcut.css":1,"modules\/toolbar\/toolbar.css":1,"themes\/bartik\/css\/layout.css":1,"themes\/bartik\/css\/style.css":1,"themes\/bartik\/css\/colors.css":1,"themes\/bartik\/css\/print.css":1,"themes\/bartik\/css\/ie.css":1}},"overlay":{"paths":{"admin":"node\/*\/edit\nnode\/*\/delete\nnode\/*\/revisions\nnode\/*\/revisions\/*\/revert\nnode\/*\/revisions\/*\/delete\nnode\/add\nnode\/add\/*\noverlay\/dismiss-message\nuser\/*\/shortcuts\nadmin\nadmin\/*\nbatch\ntaxonomy\/term\/*\/edit\nuser\/*\/cancel\nuser\/*\/edit\nuser\/*\/edit\/*","non_admin":"admin\/structure\/block\/demo\/*\nadmin\/reports\/status\/php"},"ajaxCallback":"overlay-ajax"},"myModule":{"key":"value"},"tableHeaderOffset":"Drupal.toolbar.height"});
//--><!]]>
</script>
</head>

and the footer

</div></div> <!-- /#page, /#page-wrapper -->
  <script type="text/javascript">
<!--//--><![CDATA[//><!--
jQuery(document).ready(function () { alert("Hello!"); });
//--><!]]>
</script>

<!--[if lte IE 7]>
<script type="text/javascript">
<!--//--><![CDATA[//><!--
jQuery(document).ready(function () { alert("Hello!"); });
//--><!]]>
</script>
<![endif]-->
</body>
</html>
cosmicdreams’s picture

the

<!--[if lte IE 7]>
<script type="text/javascript" src="//html5shiv.googlecode.com/svn/trunk/html5.js"></script>
<![endif]-->

...is in there because of the module I previously made that puts that code there.

Does everything meet expectations?

cosmicdreams’s picture

<!--[if lte IE 7]>
<script type="text/javascript" src="http://localhost/misc/collapse.js?lsksft"></script>
<![endif]-->

This kind of looks fishy, are those random characters at the end expected?

Also, what's putting lose empty lines in?

ericduran’s picture

All that looks correct. Those random characters are appending on purpose.

Can we get a couple of more people review this. As this should pretty much be wrapped up or at least extremely close.

Thanks.

cosmicdreams’s picture

Even the extra lines of whitespace?

sun’s picture

Issue tags: +Needs tests

And now (you figured it): Each and every detail and expectation you manually tested and talked about in those last 5,000,000 comments needs to live in an automated test.

ericduran’s picture

Yea, I kinda saw it coming. Currently there isn't any test for any of this. So I guess we should add it here.

mikeytown2’s picture

Just saw this issue; for those looking for something like this in D6 you can put a prefix or a suffix around JS tags with the Advanced CSS/JS Aggregation module. hook_advagg_js_extra_alter() is the hook used to set the prefix or suffix for JS (add in conditional comments).

cosmicdreams’s picture

@sun, I assume you mean me. I'll look into what I need to know in order to add test cases.

klonos’s picture

...here you go: SimpleTest project, Getting Started with SimpleTest, Simpletest Tutorial (Drupal 6), SimpleTest Tutorial (Drupal 7)

...and here's QUnit (a JavaScript test suite).

ericduran’s picture

Issue tags: -Needs tests

@sun, We don't need to add Test core already has all this being tested in the JavaScriptTestCase in the common.inc file.

I'm going to run a re-test but this is looking correct to me. @sun, @effulgentsia wanna give it a pass?

ericduran’s picture

Jacine’s picture

Jacine’s picture

Issue summary: View changes

Revamped issue summary.

ericduran’s picture

Issue summary: View changes

Updated issue summary.

ericduran’s picture

Issue summary: View changes

Updated issue summary.

ericduran’s picture

Issue summary: View changes

Updated issue summary.

ericduran’s picture

I'm going to assigned this issue to @effulgentsia as he's been my main reviewer on this issue.

effulgentsia, feel free to assign it back to me if you can't review it.

Thanks.

ericduran’s picture

Oh, I can't do that lol. Never mind.

effulgentsia’s picture

Assigned: ericduran » effulgentsia

I can :)

I'm trying to clear my schedule to be able to review this tomorrow. If I can't for some reason, I'll unassign myself. If anyone else wants to review though, please don't let this issue being assigned to me stop you.

sun’s picture

Status: Needs review » Needs work
+++ b/includes/common.inc
@@ -3933,6 +3938,9 @@ function drupal_region_class($region) {
+ *   - 'browsers': An array containing information specifying which browsers
+ *     should load the JS item. See drupal_pre_render_conditional_comments()

1) [and elsewhere] No quotes around 'browsers'.

2) [and elsewhere] In phpDoc, "JS" is always phrased out as "JavaScript". (also note the capitalization)

3) Not sure whether the "item", or rather full sentence, ultimately makes sense.

+++ b/includes/common.inc
@@ -3978,9 +3986,10 @@ function drupal_add_js($data = NULL, $options = NULL) {
           'type' => 'setting',
...
+          'group' => JS_SETTINGS,

In my entire lifetime of Drupal work, I tried to specify 'settings' as type for drupal_add_js(), only to find out that it's 'setting'.

As of now, these seem to be singular, so the constant should be JS_SETTING (singular), too.

+++ b/includes/common.inc
@@ -3978,9 +3986,10 @@ function drupal_add_js($data = NULL, $options = NULL) {
+          'browsers' => array(),

@@ -3992,6 +4001,7 @@ function drupal_add_js($data = NULL, $options = NULL) {
+          'browsers' => array(),

@@ -4039,6 +4049,7 @@ function drupal_js_defaults($data = NULL) {
+    'browsers' => array(),

The empty array should be applied or assumed by default.

+++ b/includes/common.inc
@@ -4094,13 +4105,66 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+  drupal_add_js($setting, 'setting');

drupal_get_js() calls into drupal_add_js()...?

That's a mind-blowing recursion.

Looking further down, it looks like this code was merely moved, so separate issue.

+++ b/includes/common.inc
@@ -4120,110 +4184,193 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
         break;
-
       case 'file':
...
+        break;
       case 'external':

There should be a blank line between switch cases, unless a case falls through to the next.

-17 days to next Drupal core point release.

ericduran’s picture

Status: Needs work » Needs review
FileSize
18.67 KB

A couple of replies to @sun's comments:

The 'browsers' => array(), option is applied by default. What you see me adding here is is for the edge case ''misc/drupal.js' being that the way that JS file is placed on the page is kinda hacky. The array is hard-coded in there without actually running through drupal_add_js which would actually populate all the correct settings for every other JS file. If we decided to change that I think it'll have to be on a separate issue.

Same goes for the the drupal_add_js($setting, 'setting'). This is just a move of whats there, so we can follow it up on another issue.

The other comment related stuff, and the JS_SETTING is now fixed.

Also I left the comment as "should load the JavaScript item" as thats what it says when using css, also Items is not the most descriptive word, but I think it beats "should load the Javascript file, settings array, inline JS, etc.."

cosmicdreams’s picture

FileSize
20.13 KB

This patch builds on top of ericduran's last patch. This one also includes the requested change to comments where JS should now be Javascript

ericduran’s picture

@cosmicdreams, I avoided changing anything extra on purpose ;) Those other changes seem tiny but any unnecessary changes really slow down a patch.

Instead all other changes that aren't related to this patch can be made in a separate issue.

cosmicdreams’s picture

I'm not so sure that the jQuery approach to solving this problem is actually better. A cursory review of available internet-based information on jQuery.support shows that it's not really meant to apply scripts per browser type. It's meant for feature detection. That's not the aim of this patch.

The other approach that jQuery offers is to use $.browsers. This option is not ideal either since it is likely that a future release will cut $.browsers out of jQuery altogether.

Using conditional comments for browser specific scripts is a valid and decent approach. It allows us to group together legacy browser specific scripts so that support for those browsers can be removed once those older browsers become marginalized.

I'm sure that there will be a strong use case for using feature detection to conditional enable specific behaviors. But that really is a different patch.

@ericduran, sorry about that. I recommend the patch in #119 as the final patch. How can we move this issue forward?

Strange, I don't see the comment that I replied to anymore. where did 122 and 123 go?

effulgentsia’s picture

FileSize
18.04 KB

This takes #119 and makes some changes to drupal_pre_render_scripts() that I hope makes it more grokable. I'm gonna get some lunch now, but still want to do some manual testing with this, and think about whether any automated tests need to be added.

effulgentsia’s picture

FileSize
18.11 KB

This one just cleans up some phpDoc, mostly changing JS to JavaScript as per #118. Also, this passes my manual testing. I'm still working on adding some automated tests.

The manual tests I ran was to compare D8 HEAD to this patch, for each of these 4 pages:

  • Front page in Bartik, JS aggregation off
  • Front page in Bartik, JS aggregation on
  • node/add/article in Seven in Overlay, JS aggregation off
  • node/add/article in Seven in Overlay, JS aggregation on

For each of these, I saved the html source output with HEAD, and with this patch, and diffed the two to make sure there's no difference between the order or contents of any SCRIPT tag.

ericduran’s picture

This is definitely more grokable :)

effulgentsia’s picture

Assigned: effulgentsia » Unassigned
FileSize
22.5 KB

With tests. The tests could use review, but otherwise, I think this is RTBC.

effulgentsia’s picture

FileSize
22.33 KB
22.33 KB
+++ b/includes/common.inc
@@ -4120,110 +4184,184 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+      if ($group['preprocess'] & $preprocess_js) {

This shouldn't be a bitwise operator. While fixing that, I refactored the new function this is in, drupal_aggregate_js(), a bit.

Below is just a single patch. d.o. duplicated it for some reason on upload.

ericduran’s picture

Sweet, I guess we did need more test ;-)

ericduran’s picture

Status: Needs review » Reviewed & tested by the community

I took the last patch for a spin,

- Tested as much Ajax/JS interaction in core as I could find. Also tested with the few JS devel has.
- Tested adding various JS each with different weight and group number. The order shows correctly.
- Also Tested Groups with Aggregation turn on.
- JS Settings show up last (unless I use a group weight higher than JS_SETTING, This was not possible before, but is a great feature addition)
- I can successfully pass in the 'browsers' options and have drupal_pre_render_conditional_comments render them properly,

Even thought I worked on a big chunk of this, I'm going to mark this RTBC as per @effulgentsia comment on #128 and my Review.

I'm sure this could use another pass from @sun and a couple of other people just to be sure.

cosmicdreams’s picture

I concur with RTBC status based on previous tests. Will test it again tonight.

cosmicdreams’s picture

@effulgentsia

Maybe I'm not understand the AggregationTest well enough but it doesn't seem that it is testing the new 'browsers' argument in that test. Should it?

effulgentsia’s picture

@cosmicdreams: I don't think it needs to. Of the three new tests in #129, only testBrowserConditionalComments() tests the new feature added in this issue. The testVersionQueryString() test tests a regression that had occurred in earlier versions of this patch (see #62.2), so that flagged that this test is needed. The testAggregation() test is probably something that should have been added in #769226: Optimize JS/CSS aggregation for front-end performance and DX, but since it wasn't there, here seems like a good place to do it, considering that the complete refactoring of drupal_get_js() that's in this patch could have caused a regression here, though I don't think such a regression was actually introduced in any of the submitted patches on this issue.

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/includes/common.inc
@@ -4094,13 +4105,66 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+ *   - '#items': The JavaScript items as returned by drupal_add_js() and
+ *     altered by drupal_get_js().
+ *   - '#group_callback': A function to call to group #items. Following
+ *     this function, #aggregate_callback is called to aggregate items within
+ *     the same group into a single file.
+ *   - '#aggregate_callback': A function to call to aggregate the items within
+ *     the groups arranged by the #group_callback function.

There should be no quotes around the keys/property names.

+++ b/includes/common.inc
@@ -4120,110 +4184,182 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+  // Defaults for each SCRIPT element.
+  $element_defaults = array(
+    '#type' => 'html_tag',
     '#tag' => 'script',
     '#value' => '',
     '#attributes' => array(
       'type' => 'text/javascript',
     ),
   );

Looks like these defaults should live in system_element_info() instead?

-19 days to next Drupal core point release.

ericduran’s picture

Status: Needs work » Needs review
FileSize
22.33 KB

This fixes the quotes around the keys/property names.

@sun, These defaults aren't the default for the actual "script" element. If just the default settings we're using for generating the <script> tag.

effulgentsia’s picture

These defaults aren't the default for the actual "script" element. If just the default settings we're using for generating the <script> tag.

Right. This patch adds a 'scripts' element to system_element_info(), but we have no singular 'script' element type that we can add defaults for.

ericduran’s picture

I'm tempted to mark it RTBC again :-p lol

effulgentsia’s picture

I am too. But let's let sun do it when he's satisfied.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Excellent!

Jacine’s picture

Hooray!!! :D Thanks so much everyone. You guys rock. This makes me so happy. :)

Jacine’s picture

Issue summary: View changes

Updated issue summary.

ericduran’s picture

Yay!, I edited the summary to reflect its RTBC status.

catch’s picture

Issue tags: -html5, -sprint

#136: 865536-136.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +html5, +sprint

The last submitted patch, 865536-136.patch, failed testing.

ericduran’s picture

re-rolling now.

ericduran’s picture

ugh, I didn't realized the move to /core happen already :-/

Jacine’s picture

Status: Needs work » Needs review
FileSize
22.4 KB

Finally something I can actually help with for this patch! Here's a re-roll for ya.

Status: Needs review » Needs work

The last submitted patch, 865536-147.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
22.32 KB

Lets try this one

ericduran’s picture

The last patch seems, good. The diff looks different to me, but it might just be too early for me.

We'll wait to see what the test bot says.

cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

bot likes it. Back to RTBC

effulgentsia’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: +html5, +sprint
FileSize
22.52 KB

Confirming RTBC.

The diff looks different to me

Yeah. Doing a diff between the #136 patch and the #149 one, there are a bunch of differences that aren't real, but an artifact of the diff algorithm used to generate #149. Attached patch is just a git apply of #149 followed by a git diff from my computer, which seems to generate a cleaner cross-diff with #136. Leaving as RTBC, because there is no difference between #149 and this one other than the diff algorithm used.

Status: Reviewed & tested by the community » Needs work
Issue tags: -html5, -sprint

The last submitted patch, 865536-152.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review

#152: 865536-152.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 865536-152.patch, failed testing.

scor’s picture

Status: Reviewed & tested by the community » Needs review

#149: 865536-149.patch queued for re-testing.

#152 is just a reroll of #149. Let's keep the patch #149 since it passed the tests earlier (let's see if it passes the test again).

scor’s picture

FileSize
22.32 KB

reuploading #149 which is passing tests...

Jacine’s picture

Status: Needs review » Reviewed & tested by the community

Hey, so can we just commit this one? It looks fine.

cosmicdreams’s picture

@Jacine, isn't that your call as the HTML5 initiative owner?

Jacine’s picture

@cosmicdreams Hehe, well actually I don't have commit access. I think the patch in #157 is fine, so I marked it RTBC. Just trying to figure out what @effulgentsia is seeing and I am not, if anything. :)

effulgentsia’s picture

#157 is great. I was responding to #150 and for the benefit of anyone else who likes to double check incremental patches by diffing them against an earlier patch, but considering there's some mysterious technical difficulties with #152, we should just go with #157. Thanks for everyone's dedication and work on this issue!

Jacine’s picture

Great! Thank you @effulgentsia. :D

ericduran’s picture

Cool.

Yay, this is a wrap :-p

catch’s picture

Title: drupal_add_js() is missing the 'browsers' option » Change notification for: drupal_add_js() is missing the 'browsers' option
Category: feature » task
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

Committed/pushed to 8.x, thanks!

This needs a change notification.

ericduran’s picture

Awesome! :)

ericduran’s picture

Title: drupal_add_js() is missing the 'browsers' option » Change notification for: drupal_add_js() is missing the 'browsers' option
Status: Fixed » Active

Change notice has been created at http://drupal.org/node/1330682

I'll leave the title as is and leave it active, until I get a better description in there.

Jacine’s picture

Title: Change notification for: drupal_add_js() is missing the 'browsers' option » drupal_add_js() is missing the 'browsers' option
Status: Active » Fixed

Thanks so much @ericduran. I improved up the change notification, so marking this fixed.

Title: Change notification for: drupal_add_js() is missing the 'browsers' option » drupal_add_js() is missing the 'browsers' option
Status: Active » Closed (fixed)

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

hass’s picture

Version: 8.x-dev » 7.x-dev
Status: Closed (fixed) » Patch (to be ported)

As suggested in #1471382: Add IE-conditional classes to html.tpl.php, need a backport for adding htm5shim in D7. drupal_add_html_head() places places html5shim too early.

effulgentsia’s picture

As discussed in http://groups.drupal.org/node/210973, backporting new features/APIs to D7 is okay as long as we're not breaking backwards compatibility of existing APIs / data structures. I don't yet know what that means for this patch, but I suspect it's doable, if someone wants to give it a shot.

webchick’s picture

Priority: Critical » Normal

This isn't critical.

ericduran’s picture

Assigned: Unassigned » ericduran
Issue tags: -sprint

Ok, I'll try a re-rolled later today.

Also removed the sprint tag since this isn't related to the html5 sprints.

fubhy’s picture

Assigned: ericduran » Unassigned

I hope you didn't stay up for 30 days :P (j/k). Are you still going to do this? Otherwise I can take this over :).

ericduran’s picture

Lol, I forgot about re-rolling this one. I'm going to get back into this one because I rather this gets in before #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page that way drupal_add_js and drupal_get_js can both be inline in D7 and D8.

ericduran’s picture

Oh but don't let me stop you :) I think this should be a pretty clean re-rolled.

tim.plunkett’s picture

Status: Patch (to be ported) » Needs review
Issue tags: +Needs backport to D7
FileSize
22.45 KB
17.93 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, drupal-865536-176-combined.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
22.32 KB
4.39 KB

Er, did that wrong. The tests patch was just the fix instead, and there were still 'core/' bits left over.
Take two!

ericduran’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me.

A couple of nice extra wins that we get from this patch is now having the ability to change aggregate callback in D7 ;-)

fubhy’s picture

+1 Review

Looking good! Lets get this commited :P

AaronELBorg’s picture

Looks good. I wish it was committed already! Now I gotta wait........

Nuts.

hass’s picture

Great guys. It just works. RTBC.

I guess some others may need this, too. Copy and Paste :-)

function mytheme_preprocess_html(&$variables) {
  // html5shim:
  // HTML5 is supported by all modern browsers. To enable Internet Explorer 6-8
  // to render HTML5 elements correctly, JavaScript is mandatory to register
  // the new elements so that the browser allows them to be styled with CSS.
  drupal_add_js('//html5shim.googlecode.com/svn/trunk/html5.js', array('type' => 'external', 'browsers' => array('IE' => 'lt IE 9', '!IE' => FALSE)));
}
Mark Trapp’s picture

#182: off-topic, but for future visitors who land on this, you really shouldn't link directly to the shim.

Jacine’s picture

I'd like to see this committed to D7 as well.

perforator’s picture

When will it be commited? Who is in charge? Many people would like to deploy YAML on D7 (http://www.yaml-fuer-drupal.de/de/changelog/4.0.1.16) without patching D7.

nod_’s picture

It's RTBC and will get committed once a D7 core committer have the time to. We are over thresholds meaning that this kind of patch will not get committed until a few major and critical are resolved. Usually the oldest RTBC are committed first, you bumping it just made him move down this list, delaying some more it's commit.

The easiest way to know if a patch will get committed is to ask on IRC: #drupal-contribute and leave the issue alone unless you have something to contribute, if you found a RTBC patch introduced a bug for example.

You can see who is core commiter on the core project page. They are busy people, they may not be available in IRC very often.

David_Rothstein’s picture

Yup, I've had this patch on my radar for a little while, and although I'm not sure if it actually falls under the "don't-commit-if-we're-over-thresolds rule" (since I think it is properly classified as a task, i.e. missing API, rather than a pure feature request), it's certainly pretty close, so it has to be lower priority than other issues.

Anyway, I'm definitely supportive of adding the 'browsers' option to D7 but what concerns me a bit is that the patch is doing a lot of other things too (JS_SETTING, group and aggregate callbacks for JavaScript, etc). All of that totally made sense for Drupal 8, but it's a lot of code churn to commit to a stable branch, especially when it touches a pretty fundamental part of Drupal. I appreciate from the above comments that the patch has been very well-tested, but also appreciate that if we release a change this big to 400,000 sites it will get tested in ways we haven't thought of also :)

So I'm wondering if we can get some discussion about whether for Drupal 7, it would be possible/better to go back to some of the earlier, more limited patches in this issue, that just added the 'browsers' option and didn't try to do other refactoring? Certainly that would make this patch a lot easier (and quicker) to commit confidently to Drupal 7.

David_Rothstein’s picture

Just to be clear, what I'm roughly talking about would look something like the attached (with the tests added in too, of course!).

hass’s picture

Status: Needs work » Reviewed & tested by the community

I understood above patch was just a easy re-role of a D8 patch that has been tested and discussed very well. I belive it's just waste of time reinventing any other wheel and maintaining different code logics. This asks for more troubles. We should commit this now as is and don't waste more valuable developers time. I have not seen any issues at all, but can think of others if we start implementing things different. There should be enough tests in D7 that make sure nothing breaks.

attiks’s picture

Status: Reviewed & tested by the community » Needs work

I share @David_Rothstein concern, but I'm all in favor of the patch in 178, it is an improvement for D7 and will make sure that upgrading from D7 to D8 will be possible in the future. Another remark is that according to 170, this kind of backporting is allowed, and that remark was made 2 months ago. (OT, #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page will be even a bigger change code wise)

Do we need more testing of real life scenario's: absolutely, feel free to have a look at testswarm, it might make testing easier

Only concern: Doesn't this mean that settings are added after the javascript from the theme? Anf if so, can it break existing themes?

+++ b/includes/common.incundefined
@@ -4047,9 +4056,10 @@ function drupal_add_js($data = NULL, $options = NULL) {
-          'group' => JS_LIBRARY,
+          'group' => JS_SETTING,
attiks’s picture

Didn't mean to change status

ericduran’s picture

Yea, I also assumed this patch wasn't going to be backport-able at the time because of all the changes.

We could roll back to a simpler patch, but that would make a bunch of other bug fixes that are happening on D8 harder to backport. Off the top of my head I know at least 2 other issues that would be simpler to fix in D7 if this patch gets it. That being said I'm not saying they can't be fixed without this patch, is just nicer to have D7 and D8 be in line so the backports are a lot simpler and cleaner.

It should be pretty easy to add the browser option and the test without all the other changes, but these changes are probably welcome changes for anyone on D7 that this would cause problems for (being that it would probably be cleaner to do what ever they were doing before). Also I imaging this *might* break some contrib such as the speedy module (But that being said it'll now be easier for a module like speedy to accomplish is task).

If we want a lighter patch then lets go that route, but in the long run it's going to be more work. :-/

David_Rothstein’s picture

Only concern: Doesn't this mean that settings are added after the javascript from the theme? Anf if so, can it break existing themes?

This is the behavior before the patch also (the settings are hardcoded to be added last, after the theme). So the default ordering will remain the same.

As for the general issue, let me lay out for a second how I'm seeing this:

  1. The backport policy says we generally only change data structures and APIs to fix critical issues.
  2. The patch, as written, would change data structures (as seen by hook_js_alter(), and as interpreted by the code that runs after that hook).
  3. This isn't a critical issue.
  4. On top of that, we seem to have a way to fix the issue without making those kinds of changes.

All those put together seem like good reasons not to take the risk.

I don't, off the top of my head, have a great example of how this particular change might break something... probably in most cases it will be OK. But it does change data structures, and I don't think it's worth spending lots of time trying to think about the edge cases that might break as a result of that if we don't have to. (One example, perhaps: If someone is using hook_js_alter() to do heavy alteration of the JavaScript groups, then after this patch those alterations suddenly start affecting JavaScript settings when they didn't before, you could wind up with the order messed up... e.g., with JavaScript settings added to the page before jQuery, which will break everything.)

Happy to hear more opinions, but I think we really should follow the policy here unless there's a really good reason to break it, and that means we shouldn't change existing structures especially if there's no need to.

David_Rothstein’s picture

@attiks (#190):

but I'm all in favor of the patch in 178, it is an improvement for D7 and will make sure that upgrading from D7 to D8 will be possible in the future

Can you clarify this concern? How will this issue affect the D7 to D8 upgrade path?

@ericduran (#192):

Off the top of my head I know at least 2 other issues that would be simpler to fix in D7 if this patch gets it.

Out of curiosity, what are those issues? I agree that keeping the D7 and D8 codebases in sync is good when possible, but by definition of course we can't always keep them in sync; if we did there would never be a Drupal 8 :)

sherpadawan’s picture

webchick’s picture

Status: Reviewed & tested by the community » Needs review

That sounds like "needs review."

hass’s picture

Status: Needs review » Reviewed & tested by the community

No, its ready, you just need to commit it.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

#193 and #194 haven't been answered yet. Furthermore, until David gets the help he's requested on resolving 7.15 release blockers, I'm not planning on committing any features to 7.x.

hass’s picture

Issue tags: +7.15 release blocker
webchick’s picture

Category: task » feature
Issue tags: -7.15 release blocker

hass, please knock it off.

attiks’s picture

@David_Rothstein sorry for the late reaction, my concern is mainly about contrib modules needing to chance a lot of code, and the difficulties of backporting D8 patches to D7.

The biggest problem AFAIK is that we don't have any proper javascript testing, so I understand why you're hesitant to commit this, but the current handling of the javascript in D7 isn't 'optimal'.

I would love to see either patch backported to D7 because no browsers option is a PITA.

effulgentsia’s picture

FileSize
1.56 KB
22.22 KB

The patch, as written, would change data structures (as seen by hook_js_alter(), and as interpreted by the code that runs after that hook).

Indeed. Here's #178 updated to remove the JS_SETTING part.

what concerns me a bit is that the patch is doing a lot of other things too (JS_SETTING, group and aggregate callbacks for JavaScript, etc). All of that totally made sense for Drupal 8, but it's a lot of code churn to commit to a stable branch

As per above, changing setting elements from JS_LIBRARY to JS_SETTING is indeed an API change with respect to hook_js_alter(). I don't think the addition of a 'scripts' element with group and aggregate callbacks are though: that's just an API addition / implementation change.

effulgentsia’s picture

+++ b/includes/common.inc
@@ -4056,15 +4051,15 @@
-          'browsers' => array(),
         ),
         'misc/drupal.js' => array(
           'group' => JS_LIBRARY,
           'every_page' => TRUE,
           'weight' => 0,
+          'browsers' => array(),

I'm not sure why the interdiff is showing the above. I don't see any change between #178 and #202 with respect to that. Seems like a bug with my interdiff utility.

effulgentsia’s picture

This one's better. Interdiff is relative to #178. Same extraneous lines noted in #203 still appearing in the interdiff for some mysterious reasons.

To allay David's concerns, it would be nice if someone (or several people) could confirm the manual testing done in #126, both for D7 core, and for a site with various popular contrib modules enabled, especially any contrib modules that implement hook_js_alter() or do something related to JS aggregation, such as http://drupal.org/project/advagg.

mikeytown2’s picture

currently downloading all modules and will check for usage of hook_js_alter()
http://drupal.org/sandbox/greggles/1481160
Will get back by tomorrow for the details.

David_Rothstein’s picture

Removing the JS_SETTING stuff definitely does make me feel better (since it addresses the direct API change issue), but as I said also, this patch still seems like a pretty big code refactoring for a stable release....

Maybe someone can help me out: Is there any previous example where we committed a patch to a stable version of Drupal core that did a similar, major refactoring of an important core subsystem, for any purpose other than fixing a serious bug?

In general, I think we refrain from doing that in stable releases. For example, #330082: Refactor drupal_get_js() to use render layer similar to drupal_get_css() (the issue that originally proposed this refactoring) was bumped to Drupal 8 in January 2011 with a comment that said "Drupal 8 territory at this point" and no one disagreed there. And now we're 18 months past that.... I'm trying to follow how things have been done in the past as much as possible, and in my experience this is not how we tend to do things, especially for a feature/task (the 'browsers' option) which can be introduced in a simpler way.

If my assumptions above are wrong, I'm happy to be shown an example of that, though.

And I do understand that people want to keep the D7->D8 API changes for module authors (and D8->D7 backport process for core contributors) as simple as possible, but this is overall a losing battle :) There are already major differences between D7 and D8 in a number of key areas, and they will continue to grow as time goes on. This issue is really a small drop in the bucket compared to all that.

mikeytown2’s picture

output from grep -l -r -i "_js_alter(" ./ in the dir containing all the 7.x modules.

./advagg/advagg.module
./advagg/advagg_js_compress/advagg_js_compress.module
./ais/ais.module
./availability_calendars/availability_calendar.module
./availability_calendars/availability_calendar_handler_filter_availability.inc
./blackwhite/blackwhite.module
./clientside_validation/clientside_validation.module
./closure/closure.module
./coder/coder_review/includes/coder_review_7x.inc
./coder/coder_upgrade/conversions/call.inc
./conditional_fields/conditional_fields.module
./core_library/core_library.module
./core_library/modules/core_library_ui/core_library_ui.module
./core_library/modules/resource/lib/Aggregation/Resource/Js.php
./domain/domain_conf/domain_conf.module
./drupal_ruble/commands/hooks.rb
./elfinder/elfinder.module
./equalheights/equalheights.module
./excluded/excluded.module
./fieldset_helper/fieldset_helper.module
./ftools/features_tools.module
./google_webfont_loader_api/google_webfont_loader_api.module
./headjs/CHANGELOG.txt
./headjs/headjs.module
./jqmulti/jqmulti.module
./jquery_dollar/jquery_dollar.module
./labjs/labjs.module
./lc_connector/lc_connector.module
./logintoboggan/logintoboggan.module
./prepro/prepro.module
./scriptjs/scriptjs.inc
./scriptjs/scriptjs.module
./speedy/speedy.module
./vimrc/drupal7.tags
./vimrc/drupal8.tags
attiks’s picture

FYI ./clientside_validation/clientside_validation.module isn't going to be a problem

mikeytown2’s picture

Someone (not me) needs to check each modules usage of hook_js_alter() to see what the side effects might be if this patch goes into D7. If it does effect that module we need agreement from said module maintainers that the new change is ok with them. Does this sound right?

hass’s picture

Can we get this commited now after 7.15 is out, please? The patch was already RTBC.

windmaomao’s picture

subscrib +1

bleen’s picture

windmaomao: there is no longer a need to leave "subscribe" comments. Please use the shiney green "Follow" button at the top of the issue instead.

hass’s picture

Commit?

Dave Reid’s picture

The patch in #204 is not currently RTBC? Why would you needlessly ping an issue when it isn't? See #209 on how you can help get this moving towards RTBC.

hass’s picture

Just commit #178. It is RTBC. I'm using it since months, zero issues.

Dave Reid’s picture

And you're using every single module that implements hook_js_alter() listed in #207 and have no issues? Because if you are not, this is not valuable feedback, and you are just wasting your and my time, so I won't waste anymore here.

mikeytown2’s picture

That list is fairly old; fresh list from January 31, 2013

./advagg/advagg_js_compress/advagg_js_compress.module
./advagg/includes/file.inc
./advagg/includes/js.inc
./ais/ais.module
./availability_calendars/availability_calendar.module
./availability_calendars/availability_calendar_handler_filter_availability.inc
./blackwhite/blackwhite.module
./bootstrap_api/bootstrap_api.module
./bundle_aggregation/bundle_aggregation.module
./closure/closure.module
./cookiebar/cookiebar.module
./cookiecontrol/modules/cookie_googleanalytics/cookie_googleanalytics.module
./core_library/core_library.module
./core_library/modules/core_library_ui/core_library_ui.module
./core_library/modules/resource/lib/Aggregation/Resource/Js.php
./dojo_toolkit/dojo_toolkit.module
./domain/domain_conf/domain_conf.module
./drupalforkomodo/Abbreviations/PHP/Drupal 7/hooks/d7_js_alter.komodotool
./drupal_ruble/commands/hooks.rb
./easy_module/data/easy_module_hooks.data.php
./elfinder/elfinder.module
./equalheights/equalheights.module
./excluded/excluded.module
./ftools/ftools.module
./google_webfont_loader_api/google_webfont_loader_api.module
./headjs/CHANGELOG.txt
./headjs/headjs.module
./jqmulti/jqmulti.module
./jquery_dollar/jquery_dollar.module
./jstool/jstool.module
./labjs/labjs.module
./lc_connector/lc_connector.module
./librejs/librejs.module
./navigation_timing/navigation_timing.module
./clientside_validation/clientside_validation.module
./coder/coder_review/includes/coder_review_7x.inc
./coder/coder_upgrade/conversions/call.inc
./scriptjs/scriptjs.inc
./scriptjs/scriptjs.module
./simpletest/simpletest.module
./speedy/speedy.module
./tabledragextra/tabledragextra.module
./textmate/Support/commands/generated/hooks/7/hook_js_alter.7.php
hass’s picture

The only waste of time is that the patch that works well has not committed after 1year. And you are the only who put a veto in. This wastes 1000th of theme developers time.

mikeytown2’s picture

Not tested but based off of the code in question these modules should be OK:
http://drupal.org/project/availability_calendars/ - availability_calendar.module, availability_calendar_handler_filter_availability.inc
http://drupal.org/project/blackwhite/ - blackwhite.module
http://drupal.org/project/bootstrap_api/ - bootstrap_api.module
http://drupal.org/project/closure/ - closure.module
http://drupal.org/project/cookiebar - cookiebar.module
http://drupal.org/project/cookiecontrol/ - cookie_googleanalytics.module
http://drupal.org/project/domain - domain_conf/domain_conf.module
http://drupal.org/project/drupalforkomodo
http://drupal.org/project/drupal_ruble - commands/hooks.rb
http://drupal.org/project/easy_module - data/easy_module_hooks.data.php
http://drupal.org/project/elfinder - elfinder.module
http://drupal.org/project/equalheights
http://drupal.org/project/excluded/ - excluded.module
http://drupal.org/project/ftools/ - ftools.module
http://drupal.org/project/google_webfont_loader_api - google_webfont_loader_api.module (might be able to switch to drupal_add_js with type 'setting')
http://drupal.org/project/jquery_dollar - jquery_dollar.module (might be able to switch to drupal_add_js if setting the weight)
http://drupal.org/project/jstool - jstool.module (module seems like a bad idea)
http://drupal.org/project/navigation_timing/ - navigation_timing.module
http://drupal.org/project/clientside_validation/ - clientside_validation.module (looks like hook_init would be better then hook_js_alter in this case)
http://drupal.org/project/simpletest - simpletest.module
http://drupal.org/project/speedy - speedy.module
http://drupal.org/project/tabledragextra - tabledragextra.module
http://drupal.org/project/textmate - hook_js_alter.7.php

Modules that would need testing:
http://drupal.org/project/advagg/ - I'll make sure this works with the patch before a beta version is released.
http://drupal.org/project/ais/ - ais.module (might be able to switch to drupal_add_js if setting the weight)
http://drupal.org/project/bundle_aggregation/ - bundle_aggregation.module
http://drupal.org/project/core_library - core_library.module, modules/core_library_ui/core_library_ui.module
http://drupal.org/project/dojo_toolkit - dojo_toolkit.module
http://drupal.org/project/headjs - headjs.module
http://drupal.org/project/jqmulti - jqmulti.module
http://drupal.org/project/labjs - labjs.module
http://drupal.org/project/lc_connector - lc_connector.module, litecommerce DrupalConnector
http://drupal.org/project/librejs/ - librejs.module
http://drupal.org/project/coder - coder_review_7x.inc, coder_upgrade/conversions/call.inc
http://drupal.org/project/scriptjs - scriptjs.module

mikeytown2’s picture

markhalliwell’s picture

Just putting my .02 in, been using the latest patch (#204) for a while and haven't seen any real issues. Not going to get into the whole debate of whether it should be committed, but in my eyes this really should be committed. I was trying to figure out for the longest time why drupal_add_js() wasn't respecting the browser conditions. It doesn't make sense why drupal_add_css() supports this but not drupal_add_js().

jbrown’s picture

#204: drupal-865536-204.patch queued for re-testing.

mikeytown2’s picture

After getting this working 100% in AdvAgg (no core patch required if using AdvAgg) I would say these are the modules that would need to be tested. The issue with these modules is they rewrite the aggregates by grouping them together in new ways or by doing some tricks with in-lining various scripts. They would most likely ignore any browser tags. Thus it wouldn't break badly, just the browser tags would most likely go missing.

http://drupal.org/project/bundle_aggregation
http://drupal.org/project/core_library
http://drupal.org/project/headjs
http://drupal.org/project/labjs - Will be ok if using AdvAgg, AdvAgg patch was accepted.
http://drupal.org/project/librejs
http://drupal.org/project/scriptjs

http://drupal.org/project/dojo_toolkit - This might be ok.

RedRat’s picture

No progress for three years? :-(

altrugon’s picture

Shouldn't this issue be ported into D8 before it is too late?

mikeytown2’s picture

@altrugon
See #164

altrugon’s picture

yeah! missed that one sorry ;)

altrugon’s picture

Issue summary: View changes

Updated issue summary.

japerry’s picture

Depending on which gets in first, this patch for d7 will need to be rerolled against #1664602: Allow attributes to be passed to drupal_add_[css|js] (SRI) As these two patches conflict with each other.

markhalliwell’s picture

FileSize
14.75 KB
7.82 KB
23.63 KB

Given @David_Rothstein's concerns with modifying so much underlying architecture in how JS is handled, I decided to take the approach of the patch in #20. I agree with @sun's comments in #32 100%, but this was also for HEAD (8.x). This is a backport to 7.x and I agree with @David_Rothstein here. The approach here to very minimally, yet thoroughly, support browser conditionals in JS (mainly by simply calling drupal_render() instead of theme().

I've included the tests from #204 and also fixed a few issues and added some additional scripts to test with aggregation.

Let me know if this is a more appropriate approach for the backport and/or if I may overlooked something.

PS... I'm really tempted to mark this as a "bug" (not going to, but tempted). There is a lot of javascript that should only be loaded for IE. Take for example font icons. Modern browsers can handle CSS3 content properties, IE7 cannot. Many services include a JS script that will dynamically add the character for IE7 browsers, if this script is added (without the conditional comment wrappers) then you see double icons because the script is run on a page that supports the CSS3 content property.

edit: interdiff is from patch in #204

markhalliwell’s picture

Changing order of files? Pass should be last I think.

The last submitted patch, 229: 865536-D7-229-FAIL.patch, failed testing.

markhalliwell’s picture

Assigned: Unassigned » David_Rothstein

Assigning for review

David_Rothstein’s picture

Assigned: David_Rothstein » Unassigned

Thanks! Looks like almost the exact same approach I used in #188 so I don't really have any complaints myself :)

Main difference with that patch seems to be in how the key is calculated in the 'file' case of drupal_get_js()... I am not sure off the top of my head which approach is better. But overall, this looks like a good direction to me.

markhalliwell’s picture

Heh :) I honestly didn't scroll as far as #188 for reviewing possible options (decided to start at the beginning and see the progressions of 8.x). But it's a good sign that two people came to the same conclusion. The idea with the file bit in my patch was to help "group" similar conditionals when it's not aggregated. Granted, it's rather trivial and ultimately should only include one conditional comment instead of multiple. What really matters is that the aggregated scripts are grouped properly (as evident by the tests). I could go either way.

mgifford’s picture

@mark Carver - your patch applies nicely. You & @David_Rothstein had the same approach to this problem (with a year's space in between). It was committed to D8 3 years ago. What's the hesitation to having this marked as RTBC?

markhalliwell’s picture

229: 865536-D7-229-PASS.patch queued for re-testing.

markhalliwell’s picture

I guess both @David_Rothstein and I were just waiting to hear back from someone other than ourselves to RTBC it (since we both provided a patch). There is a little difference in how the key is handled between ours. So whichever one you deem appropriate, please feel free to RTBC it.

mgifford’s picture

Testing #229 With LibreJS (and labjs I think) against mikeytown2's list of modules in #223, most worked fine, but LibreJS gave me this nasty error:
Fatal error: Unsupported operand types in /home/sc431931bb6ed749/www/includes/common.inc on line 4347

It works with #188 but for some reason but drupal-add-js-browsers-865536-188-do-not-test.patch was built as just a prototype I think...

Bundle aggregation, Core Library, HeadJS, ScriptJS, Dojo Toolkit, AdvAgg worked fine.

Need more testing against #219 & #220 though. Not sure if we want to make advagg mandatory for those themes.

These modules would still need to be tested: Coder, lc_connector, jQuery Multi, Adaptive Image Styles (ais)
Along with these themes: AdaptiveTheme, Arctica, mothership & Simple Clean.

Do we need to reach out to these modules/themes?

Along with testing raw:

drupal_add_js('jQuery(document).ready(function () { alert("Hello!"); });',
    array('type' => 'inline', 'scope' => 'footer', 'weight' => 5, 'browsers' => array('IE' => 'lte IE 7', '!IE' => FALSE), 'preprocess' => FALSE)
  );

So, sadly I can't mark it as RTBC yet.

Oh ya, then we'd need to alter:
https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
https://api.drupal.org/api/drupal/includes!common.inc/function/drupal_ad...

markhalliwell’s picture

Assigned: Unassigned » markhalliwell
Status: Needs review » Needs work

I'll add the inline test in the next few days (unless someone else beats me to it).

Do we need to reach out to these modules/themes?

I'm inclined to say no, however it does/will impact quite a few contrib project. Perhaps an issue should be created in each queue (@mgifford, could you do this?) asking them to test/review this patch. It should not, however, hold up this issue (as some projects may be stale/minimally maintained). If we get feedback, great, we'll address it.

Oh ya, then we'd need to alter:
https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
https://api.drupal.org/api/drupal/includes!common.inc/function/drupal_ad...

hook_js_alter() doesn't mention anything about the properties of a JS item. The patch already contains documentation updates for drupal_add_js.

mgifford’s picture

@Mark Carver - Yes, I'll make an issue in each queue (for impacted themes & modules) and tie them back here.

mfb’s picture

The fix for LibreJS module would be a trivial one-line patch, see #2295203: Drupal 7 Core Change: drupal_add_js() is missing the 'browsers' option (librejs)

Jeff Burnz’s picture

Honestly I never saw this as a contrib issue but rather a "whats out there in the wild that we don't know about" issue, as in all the site owners who are not programmers who can't easily fix a breakage without hiring someone to do it. I'm not sure that was part of the deal, aka the whole stable api thing.

markhalliwell’s picture

Assigned: markhalliwell » Unassigned

Unfortunately, I have not be able to get to this issue to address #238/#239.

Honestly I never saw this as a contrib issue but rather a "whats out there in the wild that we don't know about" issue, as in all the site owners who are not programmers who can't easily fix a breakage without hiring someone to do it. I'm not sure that was part of the deal, aka the whole stable api thing.

This is simply an API addition, a backport... not really a change. It's just introducing a new associative key in the JS array; an array that already existed. It's very unlikely that any existing site (or contrib module) will "break" with this patch (aka: WSOD).

The only reason anything would appear to "break" is someone previously introduced their own "browser" key in the JS array and did some sort of custom rendering to account for this. I can assure you (as a themer), that this is very highly unlikely. Most developers wouldn't even attempt to customize to that extent (or care enough to know how) and simply add this conditional JS to their page.tpl.php template.

Technically speaking, the only real "breakage" that would appear to happen is when people start using said "browser" key and realized that some contrib module wasn't aggregating it properly and that is simply a new feature request in the contrib module.

It's a little sad that only librejs has responded to the issues @mgifford created. Enough time was given to warn these contrib projects, I say we go for it.

mikeytown2’s picture

I've had ZERO complaints with the browser key being supported in AdvAgg for the last 2+ years, so I agree with Mark that the risk here is very very low.

sonicthoughts’s picture

+1 no issues. please commit so themers can do this properly.

cilefen’s picture

In order for this issue to be fixed it must be moved by a community member to "Needs review" status. In comments #239 and forward, there is a discussion about an API break that seems to have inclined toward "we don't need to worry about it".

Next, a community member must offer a review of the patch. Thorough reviews are preferred. I hope this helps!

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 229: 865536-D7-229-PASS.patch, failed testing.

markhalliwell’s picture

Issue tags: +Needs reroll
joseph.olstad’s picture

+1 no reported issues with #204 (on D7.39), we've been using it for quite some time in production sites with over 200 contrib modules

ejanus’s picture

I need this functionality in a current project. Any updates if this will be integrated into core?

joelpittet’s picture

Re-uploading #229 rerolled (it actually just applied without git, just line fuzzed).

I've not addressed any concerns from #243/ #238/ #239.

The last submitted patch, 252: drupal_add_js_is-865536-252--229-fail.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 252: drupal_add_js_is-865536-252--229-pass.patch, failed testing.

  • catch committed 5f46023 on 8.3.x
    Issue #865536 by ericduran, effulgentsia, cosmicdreams, sreynen,...

  • catch committed 5f46023 on 8.3.x
    Issue #865536 by ericduran, effulgentsia, cosmicdreams, sreynen,...

  • catch committed 5f46023 on 8.4.x
    Issue #865536 by ericduran, effulgentsia, cosmicdreams, sreynen,...

  • catch committed 5f46023 on 8.4.x
    Issue #865536 by ericduran, effulgentsia, cosmicdreams, sreynen,...
joseph.olstad’s picture

Cool, looks like this could soon make its' way into 7.x now that 8.x has it.
We're still using patch #204.

dsutter’s picture

RTBC+ patch #204

gdaw’s picture

RTBC +1 for patch #204 , D7 needs this committed

natew’s picture

This patch does not apply to drupal core 7.69. It appears due to changes introduced in https://www.drupal.org/node/3051370 (in 7.68). I took a stab at re-rolling it and read through all this issue. However, I am not sure how to aggregate files with the same browser tag. I thought I would contribute my try, please feel free to ignore if this is wholly wrong, or provide direction on how I can help?

It is this section of code which now resides in drupal_pre_render_scripts (not drupal_get_js)...

It was

          // If item renders for specific browsers, group by browsers key and
          // append each item so they are grouped properly.
          else {
            $processed[$browsers_key] = (!empty($processed[$browsers_key]) ? $processed[$browsers_key] : '') . drupal_render($js_element);
          }

and now I think should be something like the following? or maybe pass it js_element through drupal_render?

          // If item renders for specific browsers, group by browsers key and
          // append each item so they are grouped properly.
          else {
            $scripts[$browsers_key][] = $js_element;
          }

  • catch committed 5f46023 on 9.1.x
    Issue #865536 by ericduran, effulgentsia, cosmicdreams, sreynen,...