Problem/Motivation

Currently there is no way to pass #attributes to the drupal_add_js function. This is not a feature that is widely used, however, there are unique cases where the tag generated needs to have available an id="" or other unique attributes to that tag.

Proposed resolution

By initializing the #attributes key of the $js_element array we can easily pass specific #attribut elements in our drupal_add_js calls, and sense theme('html_tag', array('element' => $js_element)); handles #attributes passed to it, all additional support is inherited. Here is an example of the new attribute code working in the case 'external' type of drupal_add_js is invoked. The patch of course accounts for all cases of 'type':

<?php
case 'external':
 
$js_element = $element;
 
// Attach custom attributes to the element.
 
$js_element['#attributes'] = !empty($item['attributes']) ? $item['attributes'] : array();
 
// Preprocessing for external JavaScript files is ignored.
 
if ($item['defer']) {
   
$js_element['#attributes']['defer'] = 'defer';
  }
 
$js_element['#attributes']['src'] = $item['data'];
 
$processed[$index++] = theme('html_tag', array('element' => $js_element));
  break;
?>

Attached to this feature request is the patch containing this new fix.

Files: 
CommentFileSizeAuthor
#54 js_attributes_1664602-54.patch5.97 KBjaperry
FAILED: [[SimpleTest]]: [MySQL] 40,447 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
#44 interdiff-1664602-40-44.txt2.96 KBDavid_Rothstein
#40 js_attributes_1664602.patch5.71 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 39,656 pass(es).
[ View ]
#22 js_attributes-1664602.patch6.82 KBquicksketch
PASSED: [[SimpleTest]]: [MySQL] 46,316 pass(es).
[ View ]
#21 js_attributes-1664602.patch5.21 KBquicksketch
PASSED: [[SimpleTest]]: [MySQL] 46,304 pass(es).
[ View ]
#20 js_attributes-1664602.patch3.13 KBquicksketch
FAILED: [[SimpleTest]]: [MySQL] 46,312 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
1664602-1-d8.patch1.55 KBwebchick
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1664602-1-d8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#4 drupal8.script-tag-attributes.4.patch1.01 KBsun
PASSED: [[SimpleTest]]: [MySQL] 37,293 pass(es).
[ View ]
#1 1664602-1.patch1.38 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1664602-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
patch_commit_fe01acd1a960.patch2.08 KBnrussell
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch patch_commit_fe01acd1a960.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Version:7.14» 8.x-dev
Status:Active» Needs review
StatusFileSize
new1.38 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1664602-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Another example is using Aloha Editor, where you specify which plug-ins to load through a data-aloha-plugins attribute on the <script> tag: http://aloha-editor.org/guides/using_aloha.html#using-plugins.

Rerolled the patch for D7: now it actually applies and it has been cleaned up. Feature requests are against D8. Should also apply to D8.

Title:drupal_add_js #attributesdrupal_add_js() #attributes

Status:Needs review» Needs work

The last submitted patch, 1664602-1.patch, failed testing.

Title:drupal_add_js() #attributesAllow to specify SCRIPT HTML element attributes through drupal_add_js()
Component:javascript» base system
Status:Needs work» Needs review
Issue tags:+JavaScript
StatusFileSize
new1.01 KB
PASSED: [[SimpleTest]]: [MySQL] 37,293 pass(es).
[ View ]

Proper patch for D8. :)

Thanks, @sun! :)

Issue tags:+needs backport to D7

1) Does this require tests?

2) What happens if JS aggregation is enabled?

In light of 2), is this a good idea to do, or isn't the idea flawed in the first place? :-/

1) Probably.
2) That was my thinking as well… :/

Needs to be taken into consideration while working on #1490312: [META] Improving CSS and JS preprocessing.

I'd be ok with saying attributes are fine on single scripts and gets removed on aggregation, once we have aggregation groups (can't seem to find the issue talking about it though) we might be able to add attributes to them as well.

Though if aggregation groups get aggregated, we're boned.

Status:Needs review» Needs work

I applied the patch in #1 to a Drupal 7 install in order to get the Edit module and Aloha Editor working, as per #1702520: When using compiled version of Aloha Editor: "no method 'deferInit'". I found that this patch corrected the problem when js aggregation was switched off, but not when it was switched on. As far as I could tell, this wasn't simply a problem of my not clearing a cache or similar. Not sure if this is an issue with the approach taken in this patch, or just an interaction with the Aloha Editor bug, but post this here to ensure this aspect of the patch is scrutinized. :)

#10: thanks :) That's indeed important to also tackle.

Though one could of course argue that it makes no sense to keep these extra attributes when aggregated: imagine that 5 different JS files set the same attribute to different values. Which one should then be used on the aggregated script tag?

If the attributes are important, then I would expect different attribute values to result in different aggregates. Currently, drupal_group_js() adds several things to $group_keys to force new aggregates. Might make sense to add attributes to that list. Also note that #865536-204: drupal_add_js() is missing the 'browsers' option backports the drupal_group_js() architecture to D7, though there's still discussion on that issue as to whether there's enough justification for it: this issue may help provide that justification, but even if not, D7's drupal_get_js() has an equivalent $key = 'aggregate_' ... line to support this too.

#12: thanks, that's very valuable feedback!

Overall, I think we should just *not* have attributes on script tags, but it's not always up to us ("us" being Drupal) to decide that…

Though this patch is a bit strange, it's not unprecedented. The "defer" attribute we've allowed to be added to scripts for as long as we've had drupal_add_js(). When the JS aggregator went in, we made it so that the "defer" attribute only works when the individual file has aggregation turned off:

From drupal_get_js():

        if (!$item['preprocess'] || !$preprocess_js) {
          if ($item['defer']) {
            $js_element['#attributes']['defer'] = 'defer';
          }
          $query_string_separator = (strpos($item['data'], '?') !== FALSE) ? '&' : '?';
          $js_element['#attributes']['src'] = file_create_url($item['data']) . $query_string_separator . ($item['cache'] ? $query_string : REQUEST_TIME);
          $processed[$index++] = theme('html_tag', array('element' => $js_element));
        }

Adding arbitrary attributes is no different than what we're doing already for the defer attribute.

My example in #16 is from D7. In D8 the relevant code has been moved to drupal_pre_render_scripts().

        // The defer and async attributes must not be specified if the src
        // attribute is not present.
        if (!empty($element['#attributes']['src'])) {
          // Both may be specified for legacy browser fallback purposes.
          if (!empty($item['async'])) {
            $element['#attributes']['async'] = 'async';
          }
          if (!empty($item['defer'])) {
            $element['#attributes']['defer'] = 'defer';
          }
        }

So overall this suggestion makes no unusual changes. However it needs to be rerolled so that the attributes list is moved into this same IF statement so that has effect ONLY when aggregate is set to FALSE. We should update the documentation to reflect this requirement too. It wouldn't hurt to do the same thing for async and defer, since they have the same requirement already, but it's not mentioned anywhere.

@quicksketch: The entirety of that code is located in an else control structure, whereas the corresponding if to that is:

  foreach ($elements['#groups'] as $group) {
    // If a group of files has been aggregated into a single file,
    // $group['data'] contains the URI of the aggregate file. Add a single
    // script element for this file.
    if ($group['type'] == 'file' && isset($group['data'])) {
      $element = $element_defaults;
      $element['#attributes']['src'] = file_create_url($group['data']);
      $element['#browsers'] = $group['browsers'];
      $elements[] = $element;
    }
    // For non-file types, and non-aggregated files, add a script element per
    // item.
    else {

Additionally, I think this issue is semi-obsolete — AE folks have been notified of the problem and are working on a completely revamped library loading already:
https://github.com/alohaeditor/Aloha-Editor/issues/737

I don't think it is worth to pursue this issue, but I'll leave it to @Wim to won't fix it.

Title:Allow to specify SCRIPT HTML element attributes through drupal_add_js()drupal_add_js() defer and async options break when aggregation is enabled (Replace with universal attributes array)
Category:feature» bug
Status:Needs work» Needs review
StatusFileSize
new3.13 KB
FAILED: [[SimpleTest]]: [MySQL] 46,312 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Heh, looks like in D8 the "defer" and "async" options are entirely lost if you enable aggregation, so this moves to a bug report.

To fix this, I propose we kill two birds with one stone. We replace the "defer" and "async" options with a single "attributes" option. If you want defer or async, you use the attributes array. If you want custom attributes like Aloha requires, just add it. I think use of async and defer are rare enough that we can easily change this behavior without upsetting anyone.

Before:

drupal_add_js('myscript.js', array('defer' => TRUE));

After:

drupal_add_js('myscript.js', array('attributes' => array('defer' => 'defer')));

If you add any custom attributes, aggregation is forced to be disabled for that file, as it would if you set $options['cache'] = FALSE.

StatusFileSize
new5.21 KB
PASSED: [[SimpleTest]]: [MySQL] 46,304 pass(es).
[ View ]

Updating tests.

StatusFileSize
new6.82 KB
PASSED: [[SimpleTest]]: [MySQL] 46,316 pass(es).
[ View ]

This version includes an additional test to ensure that attributes are kept when JS aggregation is enabled. As mentioned above, the script is not aggregated with other files when attributes are added.

@sun: Sorry I didn't see your responses up above. Honestly I'm not sure what the requirement is for Aloha currently (I'm just trying to get set up to test it). I think this approach makes sense from an implementation standpoint all-around, plus it fixes a bug. But if it turns out that Aloha fixes this on their side then that's great. The new approach allows additional flexibility and increases consistency with normal Drupal paradigms, but I would consider it an unnecessary change on the whole if Aloha isn't going to need it. Then again, you never know when you'll need something like this and it's got its upsides. In the grand scheme of things I'd say this would be insignificant in affecting the D7->D8 porting and learning curve.

It is common to add ids and custom types to script tags when you're using js libraires to do the templating client-side. See http://handlebarsjs.com/ for example.

@quicksketch:
I didn't expect substantial changes here, but I've to say, I really like your API function argument simplification here, a lot.

First things first, resolving this issue will "resolve the bug", on the cost of performance (in case aggregation is enabled). That is OK-ish and could indeed be expected. Specifying custom attributes is edge-case either way, so 0.001% will be affected.

Replacing two custom array-of-doom options parameters with a single, standard 'attributes' parameter makes a lot of sense. (definitely not backportable though)

I'm not up to speed with regard to cross-HTML4 and HTML5 browser engine interpretations of the 'defer' and 'async' attributes though -- do they need any values, and if so, do the values have to be exactly "defer" and "async" respectively?

If the answer to both is no/anything, then we're good to go with replacing array('defer' => TRUE) with array('attributes' => array('defer' => TRUE)).

Otherwise, we could still technically decide that "developers should know that they need specify certain values" and do no special-casing. Alternatively, we could as well ensure proper values built-in.

Aside from that, this patch looks pretty good.

Thanks @barraponto, that's an absolutely excellent example! Well seems like we should push forward with this then.

I tried the exact example that HandlebarsJS would require and found that it worked perfectly with the patch in #22. I had thought that changing the "type" attribute would have been a problem, but since D8 doesn't output a type attribute by default, setting type to something entirely custom like "text/x-handlebars-template" worked fine.

drupal_add_js('example.js', array('attributes' => array('id' => 'entry-template', 'type' => 'text/x-handlebars-template')));

Otherwise, we could still technically decide that "developers should know that they need specify certain values" and do no special-casing. Alternatively, we could as well ensure proper values built-in.

I think that's probably the best way to go. Let's not get in the way of developers. If they want something like array('defer' => 'mypants') then we should let them without any kind of correction.

@sun: P.S. @jenlampton *really* wants to give you a free plane ticket to BADCamp. It's *next weekend*! :)

1) Aloha currently still needs it, but I'm working with them to get it removed. Their loading mechanism makes sense given Aloha's history, but if they're aiming to be the best, most widespread, most integrable WYSIWYG editor, then they should get rid of their "aloha-defer-init" attribute. They acknowledge this, and they're working on improving loading/building.
2) Aloha currently needs it, and it may only be post-feature freeze that they'll get rid of it. So, from that POV, it's not ideal to not fix this issue.
3) @barraponto pointed out valid use cases. It's also better for DX if developers can integrate random JS libs without having to curse core.
4) @quicksketch clearly argumented this is in fact related to an hitherto unreported bug.
5) @quicksketch's patch cleans up the API.

I think a follow-up issue could/should be an update to the aggregation logic so that if there's >1 script with the defer and/or async attributes (or more generally, any time there's >=2 JS files with the same set of attributes), they can be aggregated, while maintaining their attributes. However, it doesn't make sense to implement this right away IMO, it's more sane to defer (pun intended) the implementation of that to the grouper/bundler that is yet to be implemented over at #352951: Make JS & CSS Preprocessing Pluggable.

I'm very much in favor of the patch in #22 being committed. It looks RTBC to me. I pinged @nod_ and @Rob Loach for reviewing this issue: https://twitter.com/wimleers/status/262908598810730497. Hopefully one of them can set it to RTBC.

Indirect benefit of this is that it might work with Drupal\Core\Template\Attribute too. Haven't tested though...

<?php
use Drupal\Core\Template\Attribute;
$attributes = new Attribute(array(
 
'defer',
 
'async',
));
drupal_add_js('example.js', array('attributes' => $attributes));
?>

Status:Needs review» Reviewed & tested by the community

Looks good to me.

Status:Reviewed & tested by the community» Needs review

So this is disabling aggregation when there's attributes. Are we sure we don't want to make an aggregate group for async and/or defer instead?

So this is disabling aggregation when there's attributes. Are we sure we don't want to make an aggregate group for async and/or defer instead?

This patch just restores the same behavior that we have in Drupal 7. We weren't grouping together defer/async files then either. I don't think we want to get into custom aggregation groups for a couple reasons. It's uncommon enough that we have *any* files that use defer or async to begin with and we don't have any idea what attributes people are going to add to their scripts since they can be things other than async/defer. In some situations (similar to the Handlebars.js example), grouping files together by common attributes could actually break the functionality of the script.

Status:Needs review» Reviewed & tested by the community

I agree with #33.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

#33 makes loads of sense. Committed/pushed to 8.x.

#32: that's what I said in #29 :) It's a new feature, and it can be a follow-up. Glad to see this has been committed :)

Status:Patch (to be ported)» Fixed

As an API change, I don't think this needs backporting. Aloha has solved this problem through a hook_preprocess_html() (though it's not pretty) in D7.

Status:Fixed» Closed (fixed)

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

Status:Closed (fixed)» Patch (to be ported)

I just ran into this trying to use require.js in Drupal 7, there's no way to add arbitrary attributes with drupal_add_js() and while it does introduce a feature here there's no API breakage, fixes a valid bug, so feels backportable.

Moving back to 'to be ported'.

Status:Patch (to be ported)» Needs review
StatusFileSize
new5.71 KB
PASSED: [[SimpleTest]]: [MySQL] 39,656 pass(es).
[ View ]

Here's a backport. Leaves the 'defer' option, adds 'attributes'.

I checked what aloha does and it's using drupal_add_html_head(). That puts the JavaScript before stylesheets get rendered, which is poor for front end performance since it blocks rendering of the page and therefore delays the stylesheets getting downloaded.

The two new test methods passed locally.

Status:Needs review» Patch (to be ported)

This can be backported without #865536: drupal_add_js() is missing the 'browsers' option, but if we want to make it easier to keep this flow consistent between D7 and D8, then I suggest getting that issue in too.

Status:Patch (to be ported)» Needs review

x-post.

Status:Needs review» Reviewed & tested by the community

I've tried the patch in #40 and it works great. I'm able to add the right script tag for require.js.

<?php
drupal_add_js
($theme_path . '/path/to/require.js', array(
     
'type' => 'file',
     
'scope' => 'footer',
     
'weight' => 5,
     
'attributes' => array(
       
'data-main' => $theme_path . '/path/to/main.js',
      ),
    )
  );
?>

Produces:

<script data-main="/path/to/main.js" src="/path/to/require.js"></script>

It would be really nice to get this into core.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new2.96 KB
new5.84 KB
FAILED: [[SimpleTest]]: [MySQL] 40,262 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

So this is the part that looks like an API change:

-        if (!$item['preprocess'] || !$preprocess_js) {
+        if (!$item['preprocess'] || !$preprocess_js || $item['defer'] || $item['attributes']) {

(In particular the check for $item['defer'], since that's an existing property rather than new one - this will turn off JS aggregation for files that were previously getting it.)

That change seems to make sense and fix a bug; however in #16 @quicksketch sort of suggested that the current behavior (where you have to specify 'preprocess' FALSE manually on the file if you want your 'defer' to take effect) was intentional? Does that mean that although it's extra work for the developer, it should be left alone for Drupal 7?

Either way, if we are going to change that behavior it should be documented, so I've added it to the attached patch. And also fixed a minor issue in the tests (it was adding a library that doesn't exist) while I was at it.

That change seems to make sense and fix a bug; however in #16 @quicksketch sort of suggested that the current behavior (where you have to specify 'preprocess' FALSE manually on the file if you want your 'defer' to take effect) was intentional?

I don't think this behavior was meant to be intentional, my guess it was overlooked when we added the JS aggregation. My preference (and the way it now works in D8) is that if there any attributes then aggregation is disabled for that file.

Does that mean that although it's extra work for the developer, it should be left alone for Drupal 7?

Its rare that anyone uses defer anyway, but in those situations, their code has to already be working with the defer attribute when JS aggregation is turned off. Unless they explicitly only have their code working when the JS aggregation is turned on (thus removing the defer attribute to make it work), this isn't going to affect anything negatively.

So I think the current patch is fine. It only changes the behavior of the JS aggregator for consistency. It may end up removing files from aggregation (thus causing more HTTP requests), but it shouldn't be able to break script functionality because those scripts should already be working fine when JS aggregation isn't enabled.

FYI: AdvAgg 7.x-2.x supports defer, async, onload when aggregation is enabled & has an option to defer all scripts in the advagg mod sub module.

Just wanted to throw in a note that I've also tested the patch in #44 with drupal-7.x-dev, and it worked well.

So #1140356: Add async, onload property to script tags was marked RTBC, and it's basically trying to enable some of the exact same things as this patch, but in a different way.

As I commented there, we can only really commit one of them, so I think the people working on each issue should combine efforts and figure out which approach is best...

(For what it's worth, the patch in that issue does not make any effort to force the file to not be aggregated when one of these attributes is used.)

I'm for the attribute array: It's what we're doing in D8 and it'll help people who wants to add data attributes to script tags (you'd need that for require JS, maybe templating).

Status:Needs review» Reviewed & tested by the community

Thanks for the patch! Works very well.

I reviewed the code and since there are no possible security Issues applied the patch to a project where I needed the contentflow library with AddOns. The AddOns needs to be declared in the 'load' Attribute of the script tag.
So, with the patch #44 I was able to do things the Drupal Way instead of hacking it in another place, thx

Any chance of this being rolled into 7.x core?

put #44 into core ;)

44: js_attributes_1664602-44.patch queued for re-testing.

Issue summary:View changes
StatusFileSize
new5.97 KB
FAILED: [[SimpleTest]]: [MySQL] 40,447 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

Rerolled #44 against 7.23.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 44: js_attributes_1664602-44.patch, failed testing.

Depending on which gets in first, this patch will need to be rerolled against #865536: drupal_add_js() is missing the 'browsers' option As these two patches conflict with each other.