Add async, onload property to script tags

What if we just don't babysit broken code that doesn't work asynchronously?

I guess in some cases, you would want async to be false. With tracking scripts, like Chartbeat, where it times how long the page takes to load, you'd want the script to load inline. So, TRUE by default, but allow setting to FALSE?

Tagging.

Here are some resources on the async attribute for reference:

http://peter.sh/experiments/asynchronous-and-deferred-javascript-executi...
http://www.nczonline.net/blog/2010/08/10/what-is-a-non-blocking-script/
http://www.w3.org/TR/html5/scripting-1.html#the-script-element

Given http://groups.drupal.org/node/210973, I think this can/should be backported

so it kind of sounds like a good solution for this is to add an argument to the drupal_add_js to govern whether it should be asyc or not and default that argument to true.

Does that sound like a good plan?

re #7: sounds like a plan

Chris Coyier has written a great article on why Async is the way to go...

http://css-tricks.com/thinking-async/

I tried to created the patch I described in #7 but it quickly became complex. I'll try again next week.

#784626: Default all JS to the footer, allow asset libraries to force their JS to the header
#1033392: Script loader support in core (LABjs etc.)

We only need one direction, we should choose.

The HTML5 async feature does not yet have very wide browser support. On the other hand, loading scripts last is a well-tested, simple and commonly used technique that does not require any special browser support: #784626: Default all JS to the footer, allow asset libraries to force their JS to the header

I agree that this one should be postponed until browser support is sufficient in the latest stable versions of the main browsers (IE/FF/Chrome/safari/Opera).

Unless comicdreams or someone else can make a patch which shows it doesn't impact negatively browsers not supporting it while still working properly if the browser supports it I'm postponing this one.

According to my testing using this test: http://test.getify.com/test-async/ there is support for async in all major browsers (except IE which I didnt have available): FF 7+, Chrome12+, Safari 5+, Opera11+

Further, there has never been (to my knowledge) a case where including an unsupported attribute has a negative effect on older browsers. They are simply ignored. As evidence (non-conclusive) of this, note that Google has implemented async for all its analytics and advertising (Doubleclick) products:
http://googlecode.blogspot.com/2009/12/google-analytics-launches-asynchr...
http://support.google.com/dfp_premium/bin/answer.py?hl=en&answer=1638622
I dont think Google would be suggesting that publishers include this on their ad tags if it had a detrimental effect on older browsers given the blowback they would get from their high-paying customers in this realm.

One more post on this (as of 7/11):
http://stackoverflow.com/questions/1834077/which-browsers-support-script...

Well this attribute is not like any other HTML attribute. Async, ok but does it preserve the order? If all the major browsers don't give the same answer or have half-working implementations we won't be able to put that in core.

The first link is not working for me in Opera 11.61 or 12. Google is different, they have only 1 script to load not the 20+ we usually see on Drupal websites.

Again, talking about this is not very useful unless we have a patch to review and see what's going on for Drupal. #1 looks good but needs a reroll.

Google is different, they have only 1 script to load not the 20+ we usually see on Drupal websites

This issue is simply about adding support for async, not about implementing it for all the js files included on a Drupal site. I dont think you can consider the async property for any js file where order is important. But this would be very helpful for any site that uses the DART module, or the GoogleAnalytics module, or [insert third-party integration module that includes a js file here]

bleen18 is correct. Async does not preserve order; if that's the desired behavior, you use defer, which is already supported by Core.

If Drupal 8 is to be HTML5 ready out of the box (as I've been hearing), the async attribute should have support in drupal_add_js, as it's in the current draft HTML5 spec.

I think the agreement is to default to FALSE (current behavior), and allow the user to set TRUE.

Implementation would be almost identical to how the defer attribute is set. I bet a significant amount of existing code could be borrowed to achieve this.

Here's a preliminary patch that adds the async attribute. As per the spec, it only adds it to script tags that contain an "src" attribute.

What I'm not sure of his how to handle aggregation of async/defer scripts. Maybe it's a separate issue; I'm not sure how core handles defer scripts and aggregation.

(While patching, I found that defer is implemented incorrectly. Separate issue created here: #1503804: Defer attribute should not be added to inline scripts or settings.)

I agree with iamEAP (#17) that async should default to FALSE and that you should be allowed to set it to TRUE.

His patch in #18 looks sound and works fine although I would add the code regarding the async attribute next to the code about the defer attribute, see attached patch.

Agree with arnested in #19.

Echoing comments from nod_ in the aforementioned defer bug thread, here's a patch that cuts down on some of the redundancy.

I'm also attaching a second patch that takes care of both this and the defer bug in a nice, neat way.

Yes, much better approach in #20.

Reviewed and tested. Works like a charm.

Please be specific about which patch in #20 you are RTBCing

Please be specific about which patch in #20 you are RTBCing

Sorry. I reviewed and testet both patches.

I prefer the second patch though since it incorporates the patch from #1503804.

This patch landed: #1503804: Defer attribute should not be added to inline scripts or settings, which makes our patching lives a little easier. Here's a re-roll of #20 (the second one).

Could use a couple more opinions in addition to arnested, since this is an API addition.

I think #24 is ready to go.

What I'm not sure of his how to handle aggregation of async/defer scripts. Maybe it's a separate issue; I'm not sure how core handles defer scripts and aggregation.

I don't think this is a separate issue, although we may have a pre-existing bug if defer isn't handled already in the aggregation logic. Would be good to at least check what the current situation is and open a related bug report before commit, or add support here.

A quick look at drupal_group_js() shows defer is not currently handled in any special way: a pre-existing bug.

I created a new bug report that takes both async and defer into consideration. See: #1587536: JavaScript aggregation should account for "async" and "defer" attributes

With that said, I think it's worth committing this in the meantime. The bundling only affects half of the async functionality anyway, as it can be applied to external JS resources in addition to files.

Throwing this back to RTBC unless there are strong opinions otherwise.

Bumping the priority to major as this needs to be resolved for Drupal 8 as part of the HTML5 Initiative.

I don't think this is a separate issue, although we may have a pre-existing bug if defer isn't handled already in the aggregation logic. Would be good to at least check what the current situation is and open a related bug report before commit, or add support here.

catch, is #1587536: JavaScript aggregation should account for "async" and "defer" attributes sufficient or are you looking for that to be solved here?

I also think this is a major, but this should be tested.

I see two test that should be added.
- Text async on external scripts. Should be as simple as testAddExternal
- Test async on internal scripts. Practically the same as the previous test except adding a call to misc/collapse.js or some other internal js file.

Test attached. Testing async and defer since we modified them both here.

I'm ok RTBC-ing this. But I'll leave it to someone else. This should be good to go.

+  /**
+   * Tests adding external JavaScript Files with the async attribute.
+   */
+  function testDeferAttribute() {

Comment for testDeferAttribute should say "defer" rather than "async."

I knew a comment would ruin it lol :)

If it works for testbot, it works for me.

Lovely patch. Would it be wise to conditionally default 'async' to TRUE, if 'external' is TRUE? (not possible for D7, but considerable for D8)

Hmm...

Suppose someone used Google or another CDN for jQuery. The first time they loaded the page, they might get a slew of JS errors because some jQuery-dependent code would have fired before jQ was loaded, but afterward, because it was in his/her cache, they wouldn't get any errors. They might spend some unneeded time debugging, or looking through the API docs, or not realize there was a problem to begin with.

Or perhaps a third party service that requires the inclusion of multiple external scripts with interdependencies...

This isn't really my area of expertise, though.

I'm OK with the follow-up, just bumped that to major though.

Committed/pushed this one to 8.x, since we'll only hit the bug if using the property, and we already have that with defer.

I don't think the backport should be committed until the other issue is fixed, but that's up to David/webchick.

@mfer My comment was in reference to sun's proposal to change the existing patch to conditionally default async to TRUE; I was just supporting keeping the existing patch in place.

I agree that backporting this is not a major issue; that being said, I think catch meant to mark #1587536: JavaScript aggregation should account for "async" and "defer" attributes as major in #37, but didn't.

#38:

I wouldn't consider this high priority either. The script tag in Opera and IE9- doesn't support async. If you actually want async across browsers you need to do it by hand. This is just a nice to have for now.

Sure older browsers don't know how to interpret the async attribute. But by specifying in drupal_add_js() that a JavaScript is supposed to be loaded asynchronously we can have Drupal 8 create clean HTML5 output and let a contrib module pre_render the scripts to output some old-browser-compatible workaround instead.

I created a proof-of-concept module for exactly that: http://drupal.org/sandbox/arnested/1630800.

This approach will also make it a lot easier to change our "old fashion asynchronous script loading technique" down the road.

Patch for D7 backport.

Sorry, learning how to do this.

Close, diggingrelic. Your patch is missing code from drupal_pre_render_scripts() that actually adds the property to script tags.

Additionally, as catch pointed out in #37, we may want to hold off on the backport until #1587536: JavaScript aggregation should account for "async" and "defer" attributes is resolved, depending on how the D7 maintainers feel. Once that's resolved, we may just roll all of it up into a single patch here.

Okay. Thank you for your response.

Fixing tags per http://drupal.org/node/1517250

With async scripts, the onload attribute is important as well (especially since the developer can't predict when or in what order an async script will execute). It looks like:

  <script async src="myAsyncScript.js" onload="myInit()"></script>
  <script defer src="myDeferScript.js" onload="myInit()"></script>

In the HTML 5 spec, onload must be supported by all HTML elements other than body and frameset. The WebKit blog also has some useful details.

Passing through the generic '#attributes' option might be the way to go, it would make add_js/css more flexible for future needs.

you should pull the new code, defer and async have been replaced by the usual attributes array.

nod_ does that mean this is fixed for D8? can we move this directly to backporting to D7?

Thanks, I tested D8, and array('attributes' => array('async' => TRUE, 'onload' => 'init()')) works now. The commit was #1664602: Allow attributes to be passed to drupal_add_[css|js] (SRI).

This is a clean attempt at the d7 port of the d8 patch in #33. This is working on my system here at work.

This adds the following;

• async attribute to <script> tags with an external source added using drupal_add_js()
• async attribute to <script> tags with an internal source added using drupal_add_js()
• simpleTest test for async being added to <script> tags with an external source
• simpleTest test for async being added to <script> tags with an internal source
• fixes simpleTest test for defer being added to <script> tags with an external source
• fixes simpleTest test for defer being added to <script> tags with an internal source

Hmm... I can see why it failed, but I'm wondering why it didn't fail locally. :/ Checking into it again.

If I'm updating my own patch, what is the protocol? Can I edit the original comment, remove the fail and attach the, hopefully, workinng one?

update: and having tried editing the comment I see we can't update the attachment. I guess that answers that.

Okay, this is looking a bit more promising. Update to the patch at #54.

Hmm... Does "Status: needs review" trigger the automated testing? And just in case it's related to the status and the attachment I'm attaching it again. This one and the one at #57 are the same.

Nuts... Both are queued for testing now. It is just the Status that matters. :)

On the upside, they passed the tests. :)

The patch works for async, but still needs support for onload.

Update of #58. Added onload as requested in #60

This adds the following;

• async attribute to <script> tags with an external source added using drupal_add_js()
• async attribute to <script> tags with an internal source added using drupal_add_js()
• onload attribute to <script> tags with an external source added using drupal_add_js()
• onload attribute to <script> tags with an internal source added using drupal_add_js()
• simpleTest test for async being added to <script> tags with an external source
• simpleTest test for async being added to <script> tags with an internal source
• simpleTest test for onload being added to <script> tags with an external source
• simpleTest test for onload being added to <script> tags with an internal source
• fixes simpleTest test for defer being added to <script> tags with an external source
• fixes simpleTest test for defer being added to <script> tags with an internal source

This feature has been integrated into AdvAgg. Patching of core is no longer required if one wishes to use async, onload or defer. I actually have an option (in advagg_mod) to turn on defer for all scripts and it will use onload to ensure that the js settings loads after drupal.js loads.

Works as advertised, thanks!

#61: d7-add_async_attr-1140356-61.patch queued for re-testing.

So it would be great to get something committed for this, but this patch and #1664602: Allow attributes to be passed to drupal_add_[css|js] (SRI) (which was RTBC once before) are basically trying to do the same thing but in a different way.

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

I'll leave a comment there too.

Based on @David_Rothstein's comment above, there's more activity in the other issue, so I'm going to mark this as a duplicate only for that reason.

Feel free to re-open.

Adding duplicate ticket as related for easier reference.