Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Via drupal_add_js() add a flag for async (like defer) and when the script tag is generated place the attribute on the tag as needed. For more details on the attribute see https://developer.mozilla.org/En/HTML/Element/Script
Comment | File | Size | Author |
---|---|---|---|
#61 | d7-add_async_attr-1140356-61.patch | 6.21 KB | Gold |
#58 | d7-add_async_attr-1140356-58.patch | 4.71 KB | Gold |
#57 | d7-add_async_attr-1140356-57.patch | 4.71 KB | Gold |
#54 | d7-add_async_attr-1140356-54.patch | 4.73 KB | Gold |
#49 | 1140356_onload.patch | 3.26 KB | grendzy |
Comments
Comment #1
RobLoachWhat if we just don't babysit broken code that doesn't work asynchronously?
Comment #2
mfer CreditAttribution: mfer commentedIn several browsers the default case is true already. Sure, we can set it to true explicitly. I was more looking for a way to set it to false. Like if I have a script that decides to do a document.write().
Note: you really should never have a script do a document.write(). Really, if you are considering it... don't.
Comment #3
RobLoachI 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?
Comment #4
mfer CreditAttribution: mfer commentedasync = true is a contract with the browser not to use document.write(). If you are going to use document.write() (a terrible idea but a freedom I don't want to take away from people) you need to set async to false.
Comment #5
JacineTagging.
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
Comment #6
bleen CreditAttribution: bleen commentedGiven http://groups.drupal.org/node/210973, I think this can/should be backported
Comment #7
cosmicdreams CreditAttribution: cosmicdreams commentedso 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?
Comment #8
bleen CreditAttribution: bleen commentedre #7: sounds like a plan
Comment #9
medden CreditAttribution: medden commentedChris Coyier has written a great article on why Async is the way to go...
http://css-tricks.com/thinking-async/
Comment #10
cosmicdreams CreditAttribution: cosmicdreams commentedI tried to created the patch I described in #7 but it quickly became complex. I'll try again next week.
Comment #11
nod_#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.
Comment #12
Bevan CreditAttribution: Bevan commentedThe 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
Comment #13
nod_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.
Comment #14
bleen CreditAttribution: bleen commentedAccording 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...
Comment #15
nod_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.
Comment #16
bleen CreditAttribution: bleen commentedThis 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]
Comment #17
iamEAP CreditAttribution: iamEAP commentedbleen18 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.
Comment #18
iamEAP CreditAttribution: iamEAP commentedHere'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.)
Comment #19
arnested CreditAttribution: arnested commentedI 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.
Comment #20
iamEAP CreditAttribution: iamEAP commentedAgree 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.
Comment #21
arnested CreditAttribution: arnested commentedYes, much better approach in #20.
Reviewed and tested. Works like a charm.
Comment #22
bleen CreditAttribution: bleen commentedPlease be specific about which patch in #20 you are RTBCing
Comment #23
arnested CreditAttribution: arnested commentedSorry. I reviewed and testet both patches.
I prefer the second patch though since it incorporates the patch from #1503804.
Comment #24
iamEAP CreditAttribution: iamEAP commentedThis 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.
Comment #25
bleen CreditAttribution: bleen commentedI think #24 is ready to go.
Comment #26
catchI 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.
Comment #27
iamEAP CreditAttribution: iamEAP commentedA 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.
Comment #28
iamEAP CreditAttribution: iamEAP commentedThrowing this back to RTBC unless there are strong opinions otherwise.
Comment #29
JacineBumping the priority to major as this needs to be resolved for Drupal 8 as part of the HTML5 Initiative.
catch, is #1587536: JavaScript aggregation should account for "async" and "defer" attributes sufficient or are you looking for that to be solved here?
Comment #30
ericduran CreditAttribution: ericduran commentedI 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.
Comment #31
ericduran CreditAttribution: ericduran commentedTest 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.
Comment #32
iamEAP CreditAttribution: iamEAP commentedComment for testDeferAttribute should say "defer" rather than "async."
Comment #33
ericduran CreditAttribution: ericduran commentedI knew a comment would ruin it lol :)
Comment #34
iamEAP CreditAttribution: iamEAP commentedIf it works for testbot, it works for me.
Comment #35
sunLovely patch. Would it be wise to conditionally default 'async' to TRUE, if 'external' is TRUE? (not possible for D7, but considerable for D8)
Comment #36
iamEAP CreditAttribution: iamEAP commentedHmm...
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.
Comment #37
catchI'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.
Comment #38
mfer CreditAttribution: mfer commented@iamEAP The patch defaults async to FALSE. It just provides the option for TRUE. So, Drupal from a CDN by default wouldn't be a problem. Not unless someone deliberately set async to TRUE. This is not something to be worried about.
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.
Comment #39
iamEAP CreditAttribution: iamEAP commented@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.
Comment #40
arnested CreditAttribution: arnested commented#38:
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.
Comment #41
diggingrelic CreditAttribution: diggingrelic commentedComment #42
diggingrelic CreditAttribution: diggingrelic commentedPatch for D7 backport.
Comment #43
diggingrelic CreditAttribution: diggingrelic commentedComment #44
diggingrelic CreditAttribution: diggingrelic commentedSorry, learning how to do this.
Comment #45
iamEAP CreditAttribution: iamEAP commentedClose, 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.
Comment #46
diggingrelic CreditAttribution: diggingrelic commentedOkay. Thank you for your response.
Comment #47
cweagansFixing tags per http://drupal.org/node/1517250
Comment #48
grendzy CreditAttribution: grendzy commentedWith 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: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.Comment #49
grendzy CreditAttribution: grendzy commentedComment #51
nod_you should pull the new code, defer and async have been replaced by the usual attributes array.
Comment #52
bleen CreditAttribution: bleen commentednod_ does that mean this is fixed for D8? can we move this directly to backporting to D7?
Comment #53
grendzy CreditAttribution: grendzy commentedThanks, 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).Comment #54
GoldThis 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;
Comment #56
GoldHmm... 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.
Comment #57
GoldOkay, this is looking a bit more promising. Update to the patch at #54.
Comment #58
GoldHmm... 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.
Comment #59
GoldNuts... Both are queued for testing now. It is just the Status that matters. :)
On the upside, they passed the tests. :)
Comment #60
grendzy CreditAttribution: grendzy commentedThe patch works for
async
, but still needs support foronload
.Comment #61
GoldUpdate of #58. Added onload as requested in #60
This adds the following;
Comment #62
mikeytown2 CreditAttribution: mikeytown2 commentedThis 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.
Comment #63
grendzy CreditAttribution: grendzy commentedWorks as advertised, thanks!
Comment #64
nod_#61: d7-add_async_attr-1140356-61.patch queued for re-testing.
Comment #65
David_Rothstein CreditAttribution: David_Rothstein commentedSo 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.
Comment #66
mgiffordBased 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.
Comment #67
dooug CreditAttribution: dooug at Promet Source commentedAdding duplicate ticket as related for easier reference.