I recently filed #781994: Load mollom.js in scope 'footer' for the Mollom module, but later realized that the same idea applies to Drupal core. Several JavaScript only defined Drupal behaviors and are thus not required to be loaded already in <head>.

We should move these to 'scope' => 'header', or use 'defer' => TRUE as suggested by sun in #781994-7: Load mollom.js in scope 'footer'.

If we choose to use 'defer' => TRUE, we should attach behaviors when the script eventually finishes loading. This may happen after the HTML page itself has finished loading.

We could change the default parameters in drupal_js_defaults(), though perhaps this is too late for D7. I don't think 'defer' should default to true, because that introduces non-deterministic behaviour (depending on how soon the file finishes loading) that may confuse users and is hard to debug. Defaulting 'scope' to 'footer' may be a good idea, though.

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Update the issue summary Instructions
CommentFileSizeAuthor
#188 interdiff.txt491 bytesManuel Garcia
#188 footer-js-784626-188.patch25 KBManuel Garcia
#180 interdiff.txt2.56 KBWim Leers
#180 footer-js-784626-180.patch26.25 KBWim Leers
#178 interdiff.txt2.11 KBWim Leers
#178 footer-js-784626-177.patch28.76 KBWim Leers
#175 interdiff.txt5.28 KBWim Leers
#175 footer-js-784626-175.patch29.21 KBWim Leers
#170 head-js.png271.96 KBjoelpittet
#170 bottom-js.png166.03 KBjoelpittet
#169 interdiff.txt5.29 KBWim Leers
#169 footer-js-784626-169.patch29.3 KBWim Leers
#166 interdiff.txt2.56 KBWim Leers
#166 footer-js-784626-166.patch29.22 KBWim Leers
#161 test_jsload.tgz648 bytesManuel Garcia
#157 interdiff.txt11.49 KBWim Leers
#157 footer-js-784626-157.patch26.72 KBWim Leers
#156 interdiff.txt885 bytesWim Leers
#156 footer-js-784626-155.patch25.09 KBWim Leers
#153 interdiff.txt447 bytesWim Leers
#153 footer-js-784626-153.patch24.44 KBWim Leers
#152 interdiff.txt459 bytesWim Leers
#152 footer-js-784626-152.patch24.41 KBWim Leers
#148 footer-js-784626-147.patch24.18 KBWim Leers
#139 784626-Change-JS-scope-to-footer-138.patch1.34 KBfcaspani
#124 js_scope_header.graffle.zip3.09 KBWim Leers
#124 js_scope_header.png16.48 KBWim Leers
#124 js_scope.graffle.zip3.02 KBWim Leers
#124 js_scope.png15.95 KBWim Leers
#122 784626-Change-JS-scope-to-footer-122.patch2.9 KBManuel Garcia
#114 784626-Change-JS-scope-to-footer-114.patch2.91 KBManuel Garcia
#112 784626-Change-JS-scope-to-footer-112.patch2.89 KBManuel Garcia
#111 test_jsload.tgz595 bytesManuel Garcia
#108 test_jsload.tar_.gz592 bytesManuel Garcia
#102 set_the_default_scope-784626-101.patch4.68 KBjoelpittet
#102 interdiff.txt2.59 KBjoelpittet
#90 784626-Change-JS-scope-to-footer-90.patch2.48 KBManuel Garcia
#83 784626-Change-JS-scope-to-footer-83.patch1.74 KBManuel Garcia
#79 784626-Change-JS-scope-to-footer-79.patch969 bytesManuel Garcia
#40 js-default-scope-784626-40.patch5.65 KBdcmouyard
#36 chrome-standard.png10.35 KBjcisio
#36 filmstrip-standard.png31.87 KBjcisio
#36 waterfall-standard.png23.28 KBjcisio
#36 chrome-bottom.png10.12 KBjcisio
#36 filmstrip-bottom.png30.62 KBjcisio
#36 waterfall-bottom.png22.94 KBjcisio
#36 chrome-labjs.png11.38 KBjcisio
#36 filmstrip-labjs.png51.01 KBjcisio
#36 waterfall-labjs.png23.62 KBjcisio
#22 js_footer.patch6.63 KBBevan
#17 javascript footer.patch6.63 KBBevan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Issue tags: +frontend performance

According to arbitrary docs and benchmarks I've read, the "defer" attribute can be used for all scripts that do not happen to change the DOM upon loading. None of Drupal's behaviors do that.

Whether changing the scope to footer is still valid with modern browsers is a different question though. Perhaps Wim Leers can chime in on that.

Overall, however, I agree that we could use better defaults here.

dmitrig01’s picture

None of Drupal's behaviors do that.

That's not strictly true. Take collapse.js as an example.

sun’s picture

You need to read the entire sentence, not just the last bit. :) All Drupal behaviors are invoked on $(document).ready(), which means the DOM is loaded.

Therefore, "defer" can be used. It can only not be used, if your JS is acting before $.ready() -- typical example is document.write() in an inline script.

Wim Leers’s picture

This will require too many changes to the D7 API to be done properly.

Essentially, the default should indeed be 'footer'. Setting the defer attribute was quite pointless 2 years ago, I'm not sure how that has evolved. I'm quite sure it's still not properly supported by all browsers. In any case, it doesn't allow for sufficient granularity in the first place.

To elaborate a bit more:

Put JS files at the bottom by default. However, when at least one JS file is added to the header, both jquery.js and drupal.js should be added to the header as well. JS files that alter the appearance of the site heavily (such as carousels) should always be added to the header, and guidelines for this should be written, and should be very clear. This is for rule 6.

source: http://wimleers.com/blog/battle-plan-drupal-7

So, the basic needs:
- JS in footer by default
- API to mark JS dependencies, so that if a file is marked as being required in the header, the JS it depends on is loaded in the header (and before it) as well. An obvious example is the requiring of jquery.js.
- Strict guidelines on when it is necessary to override from the default of 'footer' to 'header'. Two cases that can't be "footered". One: JS that provides absolutely essential styling that cannot be added later during the page load without annoying the user. E.g. a carousel.Two: JS that provides absolutely essential behaviors, that can't be loaded later during the page load because the user may want to and be able to use it immediately, while the page is still loading. E.g. an autocomplete on a search page (Google's search suggestions are a prime example).

And if we want to go crazy about optimizing Drupal's page loading performance:
- It'd be nice if we'd no longer have to rely on jQuery.extend to set the Drupal settings, or have the ability to move the Drupal settings also to the footer when no JS is required initially. This allows for the ability to make pages load blazingly fast (i.e. the time until the user sees useful content, not until the page is 100% done loading).
- A JS API to load additional JS files when it is needed. This is related to #561858: [Tests added] Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework, but not the same. That issue is about loading additional JS files when performing an AJAX callback to the server, which may return HTML that requires additional JS to be loaded. This is about when JS decides that it's time to load additional JS. E.g. the JS for a complex text editor, map editor, form element/widget could be loaded just-in-time.
- A JS API *and* a Drupal API that gives the ability to preloading JS files that are likely to be needed in the future, so that they're in the browser cache. E.g. preload that text editor JS. This is not limited to JS, it can just as well be applied to CSS, images, fonts …

I'm probably forgetting some things because it's so late and I've been programming C++/Qt all day.
I talked to John Resig about *the* best technique to prevent flicker some time ago. I'll look that up as well.

It should be clear that the full range of enhancements is out of scope for D7. It is fully doable though to make the simple API change for D7 that makes 'footer' the default. When modules find this is a problem, they only have to change a single line of code. That is a perfectly acceptable change at this point of time IMO.
However, I have the feeling I'm forgetting about something important, but then again I can't think of what that could be.

c960657’s picture

<script defer> is supported by IE5.5+ and Firefox 3.5+. Not sure about Webkit or Opera.

c960657’s picture

<script defer> is not supported by Opera, but Opera 10.5 does support the new HTML5 <script async> attribute.

<script async> is supported by Firefox 3.6. Note that Firefox have/has had some issues with defer ([1], [2], [3]). This includes a change to DOMContentLoaded being fired before deferred scripts have been loading - this may affect how behaviors work.

Neither <script defer> nor <script async> is supported by Webkit.

  • Using async would be optimal, but that is only supported by few browsers, and D7 does not use an HTML5 DTD.
  • We could use defer. The page load in IE and Firefox would be optimized, while status quo is preserved in Opera, Webkit and others. We would need to find out whether the bugs related to defer+DOMContentLoaded in Firefox affects behaviors.
  • Adding scripts to the footer would work across all browsers. The question is whether it actually improves performance. When inserted into the header, Firefox stops the rendering while the script is downloaded, but it does seem to download the script in parallel with other resources (images etc.) references further down in the HTML page. Moving the script to the footer would likely decrease the time until the page rendering starts and finishes but possibly increase the time until behaviors are attached.

To summarize: If we go with defer, we reduce the time until rendering starts for users of IE5.5+ and Firefox 3.5+ and preserve status quo for others, while the footer approach will reduce the time until rendering starts for everybody but possibly increase the time until behaviors are attached. Do you agree with this analysis?

ball.in.th’s picture

Subscribing. According to Yahoo, "Put Scripts at the Bottom" should be the best approach , http://developer.yahoo.com/performance/rules.html , and will greatly improve page load experience. And I believe scripts will block parallel image download.

mfer’s picture

Putting scripts at the bottom can be a good idea. But, this needs to happen on a case by case basis. It cannot be done for everything.

For example, if a script takes the html and alters it, like vertical tabs does, you want that to be in the header. If that script is moved to the footer the html will render as fieldsets for just a moment before the JS loads and changes it to vertical tabs. If the JS is in the header you won't see the page layout change.

Damien Tournoud’s picture

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

I'm not convinced it ever makes a sensible difference for the user. It's one of those cases where Yahoo! advices really do not cut it (another that doesn't quite always makes sense is the "use a CDN" advice).

Moving to Drupal 8, this is way to late for Drupal 7 anyway.

Damien Tournoud’s picture

My measurements, on the drupal.org frontpage, indicates that the javascript and CSS files (in the header) finish downloading way before the main page (on Webkit: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_4; en-US) AppleWebKit/534.5 (KHTML, like Gecko) Chrome/6.0.489.0 Safari/534.5). In this case, moving the javascript files to the footer is perfectly pointless.

mfer’s picture

Moving JS to the footer is about optimization not developing solutions. Drupal defaulting to the footer could fall into the category of premature optimization.

Moving scripts to the footer should happen per module/feature or happen using hook_js_alter() on a per site basis by the developers for the purpose of optimization.

I'm tempted to mark this as by design.

Bevan’s picture

Title: Insert JavaScript files in 'footer' scope or with 'defer' attribute » Make 'footer' the default scope for drupal_add_js() so that pages render fast
Category: task » feature
Priority: Normal » Major
Issue tags: +WPO, +web performance optimization

There is a tonne of evidence and statistics showing how dramatically page performance improves when scripts are loaded at the end of an HTML document. Steve Souders' High Performance Web Sites blog is a good and up to date source for such data.

Steve is working on the cutting edge of web performance optimization and is a respected leader in the front-end performance community as well as in multiple W3C working groups, where he contributes to standards working groups to help the standards become more performant and implementors (browser vendors) make more performant browsers. His recent post on the Evolution of Script Loading summarizes the best techniques to use today to get the best out of front end web browser performance with modern browsers (and without breaking older browsers). His biggest piece of advice is "Move Scripts to the Bottom". He goes into more detail on this technique in "Render first. JS second.".

I have not measured loading and rendering times for the javascript-heavy admin UI of Drupal 7 (though we should) but I expect that it is mostly impacted by javascript loading (40-90%). (Of course the ratio also depends heavily on the network layer as well as the server, but I assume in my estimates that both are performing normally.) In other words, I think we could cut page load times for overlays and other admin UIs down to as little as 10-60) of their current load times. These numbers are not based on any tests with Drupal, but front end performance improvements this radical are often and easily achieved just by moving scripts to the end of the document.

Further, more and more research is showing that page-load time directly impacts the usage of, and the perception of the quality of a website or service. Or in the case of Drupal's admin UIs, the Drupal software itself. I.e. How much people like using Drupal. Steve Souders' post on WPO – Web Performance Optimization summarizes the research well and links to the actual reports by Google, Yahoo Bing, Netflix, Mozilla and Shopzilla. Google search reveals more.

On a number of projects I have worked on, it has been a challenge simply move <?php print $scripts; ?> to the bottom of page.tpl.php because some scripts are written with an assumption that they will execute before the browser starts parsing anything after the opening <body> tag. There are some cases where scripts actually need to be in the <head>, but in my experience these cases are rare, and the scripts are simply not written correctly or do no fully support progressive enhancement. Often they were easily re-factored to function performantly.

Writing or re-writing scripts in this way also naturally encourages two other good web-performance practices;

  1. Progressive enhancement; The page should look and work just fine without the scripts. That applies to the case where javascript is disabled, and to the case where the scripts are still being loaded.
  2. Separation of code that must execute on page-load from code that can be lazily-loaded to run in response to events after page load. This requires a little more thought, but is not at all difficult. Steve Souders writes about this technique here, though it is more in reference to Control JS, a new project he has just started that is an API for loading scripts in a performance-optimized manner.

If Drupal makes the default scope for drupal_add_js() 'footer' there will be several major improvements for Drupal core and contrib;

  1. Pages will render faster! :)
  2. Users will like using Drupal more. (And become frustrated and impatient less often.)
  3. Scripts will become cleaner, more performant and better support progressive enhancement.
  4. Drupal implementors will not need to manually move <?php print $scripts; ?> to the bottom of html.tpl.php just to make pages render fast.
  5. Drupal implementors will not need to repeatedly fix scripts that do not execute correctly in the footer, just to make pages render fast.

If we leave the moving of scripts to the bottom, to be done "on a case by case basis" or as a "per module/feature", and thus do not make 'footer' the default scope;

  • Drupal will be perceived to be not as good as other systems that do perform well.
  • Making sites perform well will continue to be really difficult and require a lot of duplicate effort.
  • Scripts will continue to be written poorly, and authors will have the right to ignore the fact that their script only works in the header scope.

Responses to previous comments

  • defer="" and async="" are useless until they are implemented equivalently in webkit, gecko/firefox and IE (and opera ideally). Steve Souders points this out, though I can not find that article right now.
  • Wim Leer's comment #4 is an excellent guide for how to implement this. The starting point (make 'footer' the default scope for drupal_add_js()) is easy. The script dependency API is a harder, but can probably be a separate issue/feature/patch.

    The dependency API should not assume any script will require Drupal.settings, misc/drupal.js or misc/jquery.js. It should require that modules explicitly declare a dependency on any of these.

    Additionally, it would be nice to be able to declare which property of Drupal.settings is the dependency, since contrib modules tend to bloat Drupal.settings, and breaking out parts of it into the top (if required through a dependency) or the bottom will result in still further performance enhancements.

  • Strict guidelines on when it is necessary to override...

    If we are to avoid all module authors simply adding array('scope' => 'header') to their calls to drupal_add_js(), we will need to be strict about this and provide very clear documentation and guidelines that is easy to find. It should also help script authors understand how to write their scripts for progressive enhancement.

  • Two cases that can't be "footered". One: JS that provides absolutely essential styling that cannot be added later during the page load without annoying the user. E.g. a carousel.

    Why can't the style be provided with CSS for a carousel?

    Two: JS that provides absolutely essential behaviors, that can't be loaded later during the page load because the user may want to and be able to use it immediately, while the page is still loading. E.g. an autocomplete on a search page (Google's search suggestions are a prime example).

    I disagree that this would be a candidate for 'header' scope (assuming that 'footer' were the default). The autocomplete feature is not required for the page to render and on most Drupal sites the main purpose of a page is not the search form.

    To support progressive enhancement, the search form should function without any javascript at all. I expect most uses of search forms occur after page load anyway, except in exceptional cases of Users Who Always Search and a few sites like Google.com and other search engines.

  • if a script takes the html and alters it, like vertical tabs does, you want that to be in the header. If that script is moved to the footer the html will render as fieldsets for just a moment before the JS loads and changes it to vertical tabs. If the JS is in the header you won't see the page layout change.

    Vertical Tabs is a difficult one to fix without major page flicker. I have some untested ideas to maintain the same layout for vertical tabs in both JS and no-JS modes;

    • Show and hide drawers on :hover. This will require refactoring Vertical Tabs default no-JS markup so that drawers are child elements of tabs. It can be refactored back to a list of tabs followed by a list of drawers by jQuery when JS is enabled. I think this is roughly how Drupal 7's no-JS Vertical Tabs work already (fieldsets are transformed to a list of tabs and a list of drawers), but am not certain.
    • Make tab-links point to a page with a different default vertical tab, e.g. /my/path?vertical_tab=2. This will cause data to be lost when the vertical tab is part of a form. We could fix that if the tab were a form button. But that would require we make the tab name the button, and that the summary description is not clickable, since the summary can not be styled if it is inside a button label.

    These types of page-flicker issues should be addressed after the initial change to make 'footer' the default scope, so that we can see how severe they actually are. I suspect modern browsers may cause page-flicker to be quite infrequent, by pre-fetching scripts before they need to be executed and by caching them more intelligently.

  • I'm not convinced it ever makes a sensible difference for the user.

    Please read the articles I linked to.

  • My measurements, on the drupal.org frontpage, indicates that the javascript and CSS files (in the header) finish downloading way before the main page

    That is probably because the the browser stopped loading the page while it loaded and parsed the javascript, which is all the more reason to move them to the footer. I refer you again to Steve Souder's blog where he explains page-blocking quite well. Note also the point "when the page become usable" relative to all of those events.

    Also, we need to compare measurements, not just take them. We also need to note when the page become usable, as well as when the document.ready event fires, and when resources have finished loading.

  • Moving JS to the footer is about optimization not developing solutions. Drupal defaulting to the footer could fall into the category of premature optimization.

    I disagree. Loading scripts in the footer and footer only is becoming the convention. Any high-budget website that has spent time or money on WPO does this or would like to.

I firmly believe this is a critical feature for Drupal. Clearly it is too late for Drupal 7, but if we get this in early in the Drupal 8 cycle there will be time to re-factor scripts as necessary in both core and contrib, as well as address the worst page-flicker issues.

mfer’s picture

Up through Drupal 7 we have been thinking simply with our JavaScript. Most of our JS is uncompressed. We use a lot of anonymous functions (which can be bad for memory if they are executed too much). And, a lot of other things that are just not good for performance.

Maybe with Drupal 8 we can make it better. If we are going to move a lot of scripts to the footer, and there are a lot that can go there, we need to re-think and re-structure our JS in a much larger effort. If we start to pursue this we need to also separate stuff that can execute later from stuff that changes the layout of the page when it loads.

I proposed a BoF for DrupalCon Chicago (http://chicago2011.drupal.org/forum/future-drupal-js) where we should get together with a game plan and mobilize for some better JS, better JS patterns, etc.

Bevan’s picture

Thanks Matt! A BoF is a great idea. I hope that we can move on this earlier than that though, since D8 development will probably be underway by then. For the record, I do not think that "move a lot of scripts to the footer" will achieve much. Because most of our scripts are dependent on jquery.js and/or drupal.js, the bulk of the executable script and network-transfer will still be in the head. Lazy loading can drastically reduce that initial overhead, but that is an awfully large undertaking and a separate issue to this one.

Wim Leers’s picture

Bevan, thanks for this awesome, awesome comment. It's chock-full of excellent links, and breathes some new life in this unfortunately dusty issue. And of course, I wholeheartedly agree.
I too will try to work on this for Drupal 8, possibly/hopefully as part of my internship that I'll be doing at a Drupal company in October/November 2011. But of course, if somebody can work on it before that, that's even better :)

For those who aren't "convinced": read Steve Souders' articles and books. You'll become "convinced" of the fact that this makes a very sensible difference.

alanburke’s picture

Subscribe

Bevan’s picture

Status: Active » Needs review
FileSize
6.63 KB

The attached patch makes 'footer' the default scope for drupal_add_js(), instead of 'header'. I tested manually with a fair bit of point-click with overlay, filefield widgets and other goodies and could not break it. It seems pretty stable and like a good starting point.

Todos before this can be RTBC:

  • These items need further investigation. I have not yet investigated what problems these is intend to solve;
    • function overlay_deliver_empty_page(), and if the change there makes any improvement to performance.
    • The section about "If we're outputting the footer scope, then this might be the final...".
  • common.test needs running and probably updating.
  • More manual testing
  • Benchmarks that include realistic network latency (e.g. cross-continent). As expected, there is no noticeable difference when browsing a site served locally or on server that is very close in terms of the network.

Other todos, possibly for follow-up post-commit or other issues;

  • A documentation correction ("all JavaScript files of the same scope added with $options['preprocess']" instead of "all JavaScript files added with $options['preprocess']") needs to be handled in a separate issue, and back-ported to Drupal 7 (and maybe 6).
  • maintenance-page.tpl.php should either use html.tpl.php, or handle both drupal_get_js() and drupal_get_js('header'). This patch causes it to handle drupal_get_js() only ('footer').
  • An equivalent of simpletest but for JS would allow us to make changes like this much more thoroughly.
Bevan’s picture

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

I spoke to Dries (in person) about this and he suggested we should also consider this for Drupal 7. It needs a lot of manual testing because we still do not have a unit testing framework in place for javascript, so I am doubtful that it would be wise to commit this to D7. However I am interested to hear others' thoughts.

Status: Needs review » Needs work

The last submitted patch, javascript footer.patch, failed testing.

RobLoach’s picture

We need to keep jquery.js on the top as some nodes might use inline JavaScript (ugly, but possible). Anything in JS_SYSTEM on top? We should also consider moving Drupal.settings to the bottom of the page too since most of that stuff is run in behaviors anyway, which is run when $(document).ready is run.

For Drupal 7? Seems a bit late as some of contrib might expect drupal_add_js() to add things to the header. But I'm more then happy moving it :-) .

catch’s picture

I think LABjs might be a more robust solution for this, see #1033392: Script loader support in core (LABjs etc.) and http://drupal.org/project/labjs

Bevan’s picture

FileSize
6.63 KB

LABjs allows JS scripts to be transferred in parallel with little effort on the part of the developer, so that they can all get there faster. (Older browsers will load them one at a time. Newer browsers load them in parallel anyway, but still maintain execution order and block page rendering in order to support expectations of many scripts and websites which are poorly optimized on this point.)

This issue is about moving the scripts to the footer so they do not block page rendering.

Both techniques can yield front end performance techniques, but they are different techniques each with their own problems and advantages. They are however quite related, especially #1033392: Script loader support in core (LABjs etc.).

IMO LabJS is more likely to have more problems than this technique, because it is more complex and does not respect execution order by default. It would be good to continue development and testing of both techniques.

I have re-attached the patch to see if I can not-break the testbot. ;)

sun’s picture

We need to keep jquery.js, drupal.js, and - potentially - also all libraries using weight JS_LIBRARY at the top (unless a specific weight has been assigned).

For me, this is D8 material. However, we should make sure that contrib is able to perform this tweak in D7. This most likely means that we need an alter hook that gets the yet unprocessed original values (i.e., without defaults), so contrib is able to figure out whether a certain script uses the default scope or a specific one.

Additionally:

+++ includes/theme.inc	17 Jan 2011 09:49:14 -0000
@@ -2308,12 +2308,12 @@ function template_process_html(&$variabl
-  $variables['page_bottom'] .= drupal_get_js('footer');
+  $variables['page_bottom'] .= drupal_get_js();
...
-  $variables['scripts'] = drupal_get_js();
+  $variables['scripts'] = drupal_get_js('header');

Both of these should be explicit, not relying on function argument defaults.

+++ includes/theme.inc	17 Jan 2011 09:49:14 -0000
@@ -2481,6 +2481,7 @@ function template_process_maintenance_pa
+  // @todo maintenance-page.tpl.php should either use html.tpl.php, or handle both drupal_get_js() and drupal_get_js('header').
   $variables['scripts'] = drupal_get_js();

For now, let's simply append header and footer scopes into the 'scripts' variable.

+++ modules/overlay/overlay.module	17 Jan 2011 09:49:45 -0000
@@ -402,7 +402,7 @@ function overlay_page_delivery_callback_
-  $empty_page = '<html><head><title></title>' . drupal_get_css() . drupal_get_js() . '</head><body class="overlay"></body></html>';
+  $empty_page = '<html><head><title></title>' . drupal_get_css() . '</head><body class="overlay">' . drupal_get_js() . '</body></html>';

WTF? Overlay only outputs one scope? It should output both header and footer, like we do everywhere else.

Powered by Dreditor.

Bevan’s picture

We need to keep jquery.js, drupal.js, and - potentially - also all libraries using weight JS_LIBRARY at the top (unless a specific weight has been assigned).

That would be at the cost of most speed improvements, and thus defeats the purpose of this enhancement.

Bevan’s picture

Status: Needs work » Needs review

Hmm. How to get the test-bot to re-run...

Status: Needs review » Needs work

The last submitted patch, js_footer.patch, failed testing.

Bevan’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review

At a glance those failures appear to be because the tests expect all JS settings and files to be in . This needs to be checked though.

Moving this back to 8.x-dev to see if it might get any more feedback and reviews before committing too much time to this direction.

nod_’s picture

Bevan’s picture

nod_: I am not sure exactly what you are asking.

I still believe Drupal should, by default, load all scripts last, and that should be the default behaviour out of the box for all scripts, and developers who really need their scripts in the header must be responsible for calling drupal_add_js() with the appropriate parameters, and managing dependencies (if Drupal still does not manage JS-dependencies for them).

I think this is probably a more achievable goal for Drupal 8 than adding a script loader. I think script loaders will make Drupal more complex and even further out of reach for less experienced developers. However core should support the ability for a contrib module to take-over the rendering of script tags and use a script loader if the contrib module knows how to do so.

Dries mentioned (some time ago) that "loading scripts last" is a feature he would consider for Drupal 7.

If I had time to contribute to core this is the #1 feature/patch I would try to get in.

jcisio’s picture

Status: Needs work » Postponed

With a script loader, it's better to load scripts in header to take advantages of parallel loading. I think we should postpone this issue when we have not decided whether there will be a script loader in core.

If we don't have a script loader, we can still load *all* scripts in footer. When I tried to put $scripts (D6) in footer, the only problem that I had is inline scripts that calls Drupal or Drupal.settings. This problem was remedied with something like this patch.

nod_’s picture

Status: Needs review » Needs work

I don't think this one should be postponed just yet.

It's worth trying again, can someone reroll/fix the patch in #22 so we can see how well it's working and be able to make benchmarks?

jcisio’s picture

Status: Postponed » Needs work

Benchmarks will be favored for scripts at the bottom. However, the question is: when we have a script loader, will this patch be reverted? It's likely.

nod_’s picture

Benchmarks between this and a script loader.

jcisio’s picture

Assigned: Unassigned » jcisio

I'll make a benchmark later today.

nod_’s picture

Don't forget to include the current setup as well. To have something to compare against.

Thanks for working on this :)

jcisio’s picture

Disclaimer: I'm the author of LABjs module.

Here are the results. Three testcases for D7 (as I have something that is ready for D7):

  • Standard: D7 standard profile, anonymous with most of permissions, frontpage of 10 nodes and 4 of them have an image. 8 scripts and an inline script block.
  • Bottom: same as standard, but I modify modules/system/html.tpl.php to move $script to the bottom.
  • LABjs: same as standard, with LABjs 7.x-1.3.

Tested with webpagetest.org, with Chrome and IE8 (only with IE8 there is the filmstrip feature). I tested a few times with each testcases (webpagetest also for each request, it tests several times and take the median result).

There are 3 images Chrome timing, IE8 filmstrip, IE8 waterfall, each for 3 testcases (standard, bottom, LABjs).

Chome:

chrome-standard.png

chrome-bottom.png

chrome-labjs.png

IE8 filmstrip:

filmstrip-standard.png

filmstrip-bottom.png

filmstrip-labjs.png

IE8 waterfall:

waterfall-standard.png

waterfall-bottom.png

waterfall-labjs.png

nod_’s picture

That is awesome data, thank you very much for doing that.

I might take a crack at making some data as well, I'd like to try some variations.

JohnAlbin’s picture

Trimming some redundant tags.

JohnAlbin’s picture

Issue tags: +Performance

Blargh.

dcmouyard’s picture

Status: Needs work » Needs review
FileSize
5.65 KB

Here's a re-roll from the patch in #22.

It still needs the tests to be updated and maintenance-page.tpl.php needs a way to print $page_bottom to include scripts with the footer scope.

Status: Needs review » Needs work

The last submitted patch, js-default-scope-784626-40.patch, failed testing.

JohnAlbin’s picture

Issue summary still mentions D7. Needs an update.

rupl’s picture

What if we created a new scope called default?

Over in the #1563530: Add option to move JavaScripts to footer we've been discussing a contrib-based method to alter the default from 'header' to 'footer' and it occurred to me that either value still ties the hands of a module maintainer.

Right now the default scope is header. This means that any script which does not supply a scope will default to 'header.' It also means that scripts whose scope is explicitly set to 'header' are indistinguishable from ones with no scope. This causes issues when the default is changed, because there are many legitimate cases where one might need a script to be included in the <head>.

This overzealous scope change can be remedied by creating a whitelist that allows modules to opt out, but it seems like what we really need is a third option that says "put me wherever you want, I'm not picky" which is what scopeless drupal_add_js() calls imply anyway.

Wouldn't it be nice if there was a new scope called 'default' which could be aliased to either 'header' or 'footer' at the direction of a site admin? We could go ahead and make 'footer' be the default 'default' value. Then all scripts can live wherever they need to live without fear of being moved by a presumptive hook_js_alter().

nod_’s picture

That's a really nice idea. I'm not comfortable putting footer as the default so that'd be a cool way to let people choose for themselves.

JohnAlbin’s picture

Title: Make 'footer' the default scope for drupal_add_js() so that pages render fast » Add explicit "default" scope for drupal_add_js() which defaults to "footer" so that pages render fast

So… maybe this title works better?

Bevan’s picture

I agree, for the most part.

I disagree that it should be configurable to make the 'default' scope "footer". If a developer wants to do this through alters or other code, that's fine, but not through any sort of standard configuration UI. Drupal should Just Work—Fast—Out of the box with no configuration, hacking nor bugs. This issue is about making the scripts run after the page renders so that Drupal can render fast. This implies that poorly written scripts may need to be fixed.

JohnAlbin’s picture

If a developer wants to do this through alters or other code, that's fine, but not through any sort of standard configuration UI.

Right, rupl said “at the direction of a site admin?” which I interpreted as via code, but it could also mean UI. I agree with Bevan that it should not be a UI. If a UI is needed, that's a contrib project.

rupl’s picture

Agreed. I don't want to offer this as a configuration option in the core UI, but the additional 'default' scope would allow a contrib project to offer this functionality much more cleanly.

Bevan’s picture

Oh great! We all agree then; I just misunderstood! :)

RobLoach’s picture

Issue tags: +JavaScript

Tagging.

RobLoach’s picture

Issue tags: +sprint

.

sun’s picture

I still think this makes sense. Any chance to move forward here? Looks like we're bound to feature freeze with this.

Wim Leers’s picture

Are we really bound to feature freeze with this? I'd say it's an optimization that can happen after feature freeze, no?

sun’s picture

While this is performance-related, and performance-related stuff generally seems to be post-feature-freeze material, the concrete change proposal involves to add an entirely new notion of how developers should specify the JS they want to load, so I'd personally not bet on that.


That said, any idea how this maps to and will play with the foreseen removal and replacement of drupal_add_js() with the everything-is-a-library design?

Will this even be needed with that?

xjm’s picture

catch’s picture

Category: feature » task
Priority: Major » Normal
vrazz’s picture

make all js to footer only change theme.inc

function template_process_html(&$variables) {
// Render page_top and page_bottom into top level variables.
$variables['page_top'] = drupal_render($variables['page']['page_top']);
$variables['page_bottom'] = drupal_render($variables['page']['page_bottom']);
// Place the rendered HTML for the page body into a top level variable.
$variables['page'] = $variables['page']['#children'];
//$variables['page_bottom'] .= drupal_get_js('footer');
$variables['page_bottom'] .= drupal_get_js();
$variables['head'] = drupal_get_html_head();
$variables['css'] = drupal_add_css();
$variables['styles'] = drupal_get_css();
$variables['scripts'] = '';
//$variables['scripts'] = drupal_get_js();
}

mikeytown2’s picture

@vrazz
If you want to do this in D7, checkout AdvAgg. It can do it for you.

Wim Leers’s picture

We should still do this for Drupal 8. It's not too late yet. It does not require an API change, just a change of defaults.


  1. Drupal 8 has properly defined dependencies for each library.
    • Consequence 1: jQuery, drupal.js and drupalSettings can now be loaded only when necessary.
    • Consequence 2: We can make Drupal reason which JS *must* be in the header (i.e. ensure jQuery is in the header if anything that depends on it is in the header).
  2. For us to be able to decide whether a JS asset must be in the header or in the footer, we must use dependency-based ordering, not weight-based ordering. I.e. we must kill the remaining "weight" stuff.
  3. Then, we can implement dependency resolution with the following algorithm (devised together with @sdboyer at DrupalCon Portland):

    1. Create a virtual node for each scope (FOOTER or HEADER).
    2. Let any dependency within the scope depend on the scope's virtual node.
    3. Build dependency tree for each scope.
    4. Subtract the children of each earlier scope's dependency tree from a given scope's dependency tree. (i.e. FOOTER minus HEADER)

    Thoughts?

catch’s picture

That sound sensible enough to me, and agreed it should still be done for 8.x.

nod_’s picture

big +1, sounds good to me.

Bevan’s picture

yay!

mgifford’s picture

Ok, so what's happening with this? Can we proceed with Wim's ideas in #59?

sun’s picture

Actually, I think we have to remove the concept of 'scope' altogether for JS in D8.

A custom scope breaks the order of loaded dependencies between libraries.

→ Even though properly declared as dependency to be loaded upfront, a dependency will not be loaded upfront when e.g. using a custom scope 'footer'.

That is, unless (1) a custom scope would be applied to an entire library (instead of individual files) AND (2) every custom scope would be derived to all libraries that depend on that library.

And I don't think we want to get into that territory, since it requires a complex preprocessing and state tracking within drupal_add_library() already, so as to record and pass forward the scope of whichever dependency that declared one first.

Therefore, I think we need to remove 'scope' altogether, and instead, allow a themer to (manually) output the entire $scripts in the footer, if desired.

mgifford’s picture

Thanks for the explanation. So is this "won't fix" or D9?

Bevan’s picture

I agree we should remove the concept of script's "scope" entirely. I have never seen it used except to intentionally add the JavaScript to the footer, which should be the default behavior.

So;

  1. This ticket should not marked "won't fix".
  2. Instead, this ticket should be repurposed to render
    tags at the end of <body> (aka "footer") instead of in <head>.
  3. Scripts that really do need to be in the header could use a new a type of dependency (probably called "body") which indicates that the script must be loaded in the HTML <head>, before the <body> element is parsed. And any other scripts that it requires should follow it to <head>.
Support for a "body"-dependency would:
  • allow for scripts that really do need to run before body is parsed. Such scripts are definitely valid, but rather uncommon. Many scripts that do not run correctly in the footer are merely poorly implemented.
  • avoid the complex preprocessing that sun mentioned two comments earlier.
  • still reap the significant performance benefits of loading scripts last (the original purpose of this ticket).
[Edited 2014-11-17 to be more clear and easier to understand.]
sun’s picture

#66 makes sense to me.

For starters, I'd be interested to see/learn how much stuff breaks if the concept of scope is quickly hacked-away/commented-out and all scripts are force-output in the footer.

The result of that brutal test delivers clear data on (1) how much will break, (2) whether 'scope' can be removed immediately, and (3) how much work remains for the task of introducing a scope 'head' fake library dependency.

gmclelland’s picture

Issue summary: View changes

@sun the https://drupal.org/project/magic (D7) provides functionality to move the scripts to the footer and keep libraries in the header.

Bevan’s picture

For full optimisation libraries go to footer too. Most scripts that break when they run in the footer don't need to be in the header, but are just poorly implemented.

Manuel Garcia’s picture

I haven't had the time to read through the whole discussion, but wanted to chip in.

First, some information on the projects I've worked on optimizing frontend performance (D7 & D6). This is more or less what I've been doing:

  • Moved the $scripts variable in page.tpl.php to just above $closure
  • Printed any custom inline scripts depending on jQuery with scope 'footer', so that they print in $closure after jQuery
  • If necessary , used drupal_set_html_head() for any REALLY critical inline scripts needed in <head>.

Setting all scripts' scope to 'footer' will most likely break some js dependencies, and doing something more kamikaze like this will break inline scripts depending on jQuery, since it gets loaded async (on supporting browsers):

function THEME_process_html(&$variables) {
  // async all scripts
  // Drupal settings gets broken by this, jQuery.extend() is called when jQuery is not yet loaded...
  $pattern = '/(<script src)/';
  $variables['scripts'] = preg_replace($pattern, '<script async src', $variables['scripts']);
}

As a general idea, this is my opinion

  • Libraries should be printed before the closing </body> tag by default.
  • Initializing scripts depending on these libraries should be printed in another variable below that one (we could use $closure). So perhaps inline scripts should default to 'footer' in current terms.
  • We should have a way to add aggregated critical inline scripts to the , not sure if within the same system or not.

Printing the libraries in <head> would result in a not very different user experience when it comes to frontend performance. Sure, it might be a tad faster, but since all js in <head> are blocking, the more code to download and execute, the longer the wait. Also, we can't load them using async because then our inline scripts will break, even if all browsers supported it.

carlos@ukelele.la’s picture

Suscribe, im having issues with analitycs due to this...

catch’s picture

Title: Add explicit "default" scope for drupal_add_js() which defaults to "footer" so that pages render fast » Remove scope for scripts, move to bottom of the page, add opt-out for scripts that really need to be in the header
Priority: Normal » Critical

#66 works for me. This is a small API change but it's also a critical front end performance issue that we don't default like this, so bumping to critical.

attiks’s picture

#72 +1

hass’s picture

We cannot remove "scope" param. I need to add JS code to header.

catch’s picture

@hass if you read the issue title, you'll notice it says:

add opt-out for scripts that really need to be in the header

joelpittet’s picture

I agree with @hass, though it would be ideal to just swap the defaults. Default to footer, can choose header. Leave the scope alone in this issue.

Should achieve most of what is asked here and less likely to cause any issues, no?

Manuel Garcia’s picture

@Bevans on #66:

... Any dependencies of such scripts (usually none) would follow the script to .

I don't think it's necessary to print the dependencies right after such scripts, I mean as long as they are loaded after what they depend on, it's ok. We should default dependencies to the footer in all cases in my opinion.

Nevermind I saw the light in IRC, very true that @Bevans, dependencies would need to be printed before the offending script.

hass’s picture

The title is now highly confusing as it starts with - remove scope. The previous issue title was very clear. The default scope should be changed, nothing else.

Manuel Garcia’s picture

Well this was a bit simpler than expected...

The only thing left in head is the drupalSettings and modernizr.

I've been browsing the backend a bit nothing seems complaining.

Manuel Garcia’s picture

Status: Needs work » Needs review
catch’s picture

Issue tags: +Needs manual testing

If that doesn't break anything (needs manual testing), I'd commit as is, we can always look at removing scope in a follow-up.

Status: Needs review » Needs work

The last submitted patch, 79: 784626-Change-JS-scope-to-footer-79.patch, failed testing.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
1.74 KB

This patch expands on #79 and sets drupal_get_js's first parameter $scope to default to 'footer', since that's where most files will be. This also fixes the failing tests from Drupal\system\Tests\Common\JavaScriptTest.

The other failing tests will stil need addressing though.

Manuel Garcia’s picture

Title: Remove scope for scripts, move to bottom of the page, add opt-out for scripts that really need to be in the header » Set the default scope for scripts to 'footer'
Related issues: +#2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests

Status: Needs review » Needs work

The last submitted patch, 83: 784626-Change-JS-scope-to-footer-83.patch, failed testing.

mikeytown2’s picture

In advagg I have a special exception for modernizr. So yes you'll need some js in the head but it's very rare. Overall I've had good success with moving everything to the footer. So a big +1 for this from me.

I also have a wrapper for inline javascript that was added with script tags instead of drupal_add_js. The wrapper delays the execution of the code until jQuery and Drupal are defined. With a new version of Drupal the wrapper code most likely will not be needed; just something to be aware of; people will hardcode stuff.
See: advagg_mod_process_move_js() - Find script tags, advagg_mod_wrap_inline_js() - Wrap js code.

hass’s picture

@Catch: why do you like to remove scope? We need this! It cannot removed.

Manuel Garcia’s picture

@mikeytown2 thanks for your input! You make a very valid point, and indeed we may end up with a lot of support requests of jQuery is undefined etc, from users hard-coding script tags, and these users might hard-code their own jQuery in the same way, in order to fix the problem...

Would running such a preg_replace every time be detrimental to performance?

Another option could be to have the placement of scripts be configurable through the interface. This way these users would be able to print a certain library in the header if they need it. Not sure this would be worth the effort though.

catch’s picture

I think the hard coding of script tags probably happens because core lets you get away with this. Since advagg is in contrib it's then having to support that bad behaviour because otherwise it "breaks" previously working code.

I don't think we need to support that in core, people should just fix their code.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
2.48 KB

Updated patch #83, this one fixes the Drupal\system\Tests\Ajax\FrameworkTest->testLazyLoad() test. 3 more to go, Drupal\system\Tests\Ajax\FrameworkTest->testOrder() is still failing... if anyone wants to take a look, not sure how to fix those.

Status: Needs review » Needs work

The last submitted patch, 90: 784626-Change-JS-scope-to-footer-90.patch, failed testing.

hass’s picture

It would also be extremely useful if drupal_js_defaults() implements an alter function.

I do not need this myself, but it would had helped in #2309079: Respect original JS scope. In #23 of this case I have added the alter that we need. It is possible here that a module need to define 'header' for all by default, but do not like to break other modules that explicitly define footer. A backport to D7 of this alter function is also something we should really do.

Manuel Garcia’s picture

@hass I guess that makes sense, and it would be easy to do.

Users implementing that hook and changing the default scope should be aware that drupal_get_js() will default to getting the ones in 'footer' scope. I suppose if you are playing with this hook, you should expect some trouble anyway.

Fabianx’s picture

Yes and therefore (#2309079: Respect original JS scope) removing 'scope' parameter and adding 'force_header => 1' as an option is exactly what we need here :).

Thanks for the idea, hass! And given some contrib modules already support it, that is a good thing to have as the same option in core.

Because scope is implicit header as the defaults are that way in D7, so no one puts it as outlined several times in this issue.

So a new parameter is warranted for those scripts that want to be in the header - to avoid even more BC break.

An alter hook for the JS defaults is still a good thing to have and you could still set force_header => TRUE for everything as the default.

But yes the scope -> force_header change can indeed be follow-up.

hass’s picture

@Fabian: No, you completly misunderstood something. There is no need for a new param and I already explained this in the other case. The proper way to solve this requirement is the alter function for the js_defaults I described and not a force_header param nobrainer.

Manuel Garcia’s picture

OK, so just to recap to see if I understand where we stand.

For you @hass, the way to go about this is to keep scope, set it to footer by default, and call drupal_alter on the defaults.

As far I understand the case for force_header, it's because if we are just going to print everything in "footer", there really isn't much of a case for scope. Only if you want certain things in header you'd then use force_header. I can understand this approach, if there are only two possible scopes, so it really is just behaving like a boolean.

I believe the only valid case for keeping 'scope' would be if we are going to have scopes other than footer or header. Otherwise it makes sense to default to footer unless the boolean force_header is set to true.

In any case, and in my opinion, whether we keep scope or not I think calling drupal_alter on the defaults is a valid request.

joelpittet’s picture

@Manuel Garcia I agree with the intent behind force_header though I believe at this point in the release cycle it is potentially more disruptive of a change than necessary to achieve all of our goals.

Simply swapping of the scope defaults, seems to be getting us closer to the goal without the API disruption and could likely get in without too much friction and I propose we push hard to get that in first and postpone the 'force_header' to 8.1.x or 9.0.x. What do you think, is that reasonable?

Manuel Garcia’s picture

@joelpittet that makes total sense. Should we include the call to drupal_alter? I don't think that'd break anything, and it'd give users & contrib more flexibility.

We still have 3 failing tests, and I'm currently busy relocating to Wales UK, so I might not find the time necessary to sit down and wrap my head around these tests. Hopefully next week... busy times for me (starting a new job next week).

joelpittet’s picture

@Manuel Garcia the alter hook does sound like a great idea too. It *could* be construed as a feature or maybe a BC layer to the change... hard to know, may need a committer chime in here?

Is there likely to be a need to revert this change through that alter hook would be my main question and if so include it here, if not punt it to a followup.

I'll take a stab at the test fails.

hass’s picture

Let's say we have the alter hook in js defaults. We also have JS scopes header and footer only - no others. However you could define more, but my code still goes in header if I set header. Why do we still need force_header? I do not get the idea of this. If my contrib modules defines that code go into header the defaults will not override this. These only merge defaults, but contrib always wins. It is left as is and therefore I think there is absolutly no need for force header. Not now and not in 8.1 or 9.x

Fabianx’s picture

Yes, lets implement the alter hook. It is a needed BC layer, because by implementing the alter() hook the old D7 implementation can be enforced.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
2.59 KB
4.68 KB

Seems to me that AjaxFormsTestLazyLoadForm::submitForm attachment of /system.js overrides the test. Not sure the best way to solve the test breaking yet but that may help someone.

Also, addressing previous comments this patch puts jquery and drupal.js into the header, and domready was also needed at that point.

Status: Needs review » Needs work

The last submitted patch, 102: set_the_default_scope-784626-101.patch, failed testing.

joelpittet’s picture

Well that has more failures... hmm well the JS loads in the header for the jquery/drupal/domready so we'll likely need to fix those too.

hass’s picture

alter() in js defaults seems missing in the patch

Manuel Garcia’s picture

@hass I think we are no longer pushing the force_header idea. Defaulting to footer and using the drupal_alter should be enough for all cases.

@joelpittet do we really need jquery/domready on header? I cant seem to find the comment arguing for it. Imho it would make a huge difference getting these to the footer. Scripts depending on them would just be printed after the libraries, so no harm could be done. We should just make sure that if say some jcarousel script declares that it has to go on the header, then its dependencies should be pushed to the header as well.

Fabianx’s picture

Agree, on that, need to find dependency chain for what goes to which scope ...

Manuel Garcia’s picture

FileSize
592 bytes

OK, I've created a tiny module to test the dependencies when a module declares that its js should go in the header, find it attached.

Right now, we are not handling this at all, so you'll see an error on the browser's js console: ReferenceError: jQuery is not defined.

We can use this as a tool to debug and work on this.

joelpittet’s picture

@Manuel Garcia the reason for moving jquery and drupal to the head is because of the comment in #4

Put JS files at the bottom by default. However, when at least one JS file is added to the header, both jquery.js and drupal.js should be added to the header as well. JS files that alter the appearance of the site heavily (such as carousels) should always be added to the header, and guidelines for this should be written, and should be very clear. This is for rule 6.

And because of what you wrote in #88:

@mikeytown2 thanks for your input! You make a very valid point, and indeed we may end up with a lot of support requests of jQuery is undefined etc, from users hard-coding script tags, and these users might hard-code their own jQuery in the same way, in order to fix the problem...

But also because I feel some people will need to write jquery in the body, though I don't have a strong case for this... and I know this will be rare, hmm I'm torn here. Just trying to make this transition a softer blow in-case someone has this use-case and it's legit.

nod_’s picture

The zip doesn't work.

If we have to put stuff in the header it should be the scripts defining global variables: jquery, drupal.js, domready, drupalSettings, ckeditor, backbone, underscore, classlist, jqueryonce, jqueryform, jqueryUI, html5shiv. So let's try not to do that.

For the scopes I can't see the testing code so I don't know if you guys defined the dependencies appropriately or not, if yes we should probably check a script and it's dependencies scope to make sure we print deps before.

Manuel Garcia’s picture

FileSize
595 bytes

Humm ok. I think at least if we default the rest to footer it should make a difference, if small. We'll just have to alter libraries in d8 to get a great frontend performance I suppose. Perhaps we should sit face to face at an event and try to figure this out?

Sorry for the bad zip file @nod_, drupal.org doesnt like tar.gz files, and I keep forgetting. Let's try again.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
2.89 KB

This patch builds on #90, and ads a call to drupal_alter on the defaults.

The test module should be tested against this (or #90).

We should still have 3 failing tests though.

[edit] IGNORE THIS PATCH, SEE #114

joelpittet’s picture

+++ b/core/modules/system/src/Tests/Ajax/FrameworkTest.php
@@ -56,7 +56,7 @@ public function testOrder() {
-          $path . '/system.js' => array(),
+          $path . '/system.js' => ['scope' => 'header'],

@@ -67,7 +67,7 @@ public function testOrder() {
-          $path . '/system.modules.js' => array('scope' => 'footer'),
+          $path . '/system.modules.js' => [],

@@ -134,7 +134,7 @@ public function testLazyLoad() {
-    $expected_js_html = drupal_get_js('header', array($expected['js'] => drupal_js_defaults($expected['js'])), TRUE);
+    $expected_js_html = drupal_get_js('footer', array($expected['js'] => drupal_js_defaults($expected['js'])), TRUE);

I think these lines should help the failing tests but I not not quite sure if it gets all of them.

Manuel Garcia’s picture

FileSize
2.91 KB

Nevermind my previous patch, drupal_alter is gone for a long time, my bad. (see https://www.drupal.org/node/1894902)

Manuel Garcia’s picture

Tried those changes @joelpittet, ran the test localy, stil fails (plus another fail "Page now has the core/modules/system/system.js file." on testLazyLoad()). :S

The last submitted patch, 112: 784626-Change-JS-scope-to-footer-112.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 114: 784626-Change-JS-scope-to-footer-114.patch, failed testing.

joelpittet’s picture

@Manuel Garcia dang, I was hoping you'd have better luck. It looks like when running that and stepping through it that both JS files in that test were in 'footer' or 'header'(can't remember it was a couple days ago) but not split up as they are added in the test. I'm really, really positive that they changes for testOrder are correct, but there is something manipulating them elsewhere.

The 3 tests failing are on FrameworkTest.php and it returns 4 commands instead of 5 and the footer/header js are merged into one.

joelpittet’s picture

Oh and I'm not positive about the last change above though so maybe confirm the testLazyLoad is of any use.

Manuel Garcia’s picture

@joelpittet:

Yeah the changes you suggest to testOrder() make total sense. Not sure what's going on there.

Also, I don't think we should touch testLazyLoad(), that one is OK afik.

hass’s picture

Not sure, but the alter hook should be names js_defaults only if I'm not wrong. A module may define modulename_js_defaults() function.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
2.9 KB

@hass you're right, I see that pattern also on other alter hooks. The attached patch sets the alter hook to 'js_defaults' only - thanks!

Status: Needs review » Needs work

The last submitted patch, 122: 784626-Change-JS-scope-to-footer-122.patch, failed testing.

Wim Leers’s picture

The problem with HEAD and this patch is actually fairly simple.

HEAD forces a certain JS asset to be in the footer, but defaults to header. If a certain JS asset X is forced into the footer, then it is guaranteed to run after JS in the header. Hence there is no problem with regards to dependencies: X's dependencies either live in the header, and then the dependency condition is fulfilled, or they live before X in the footer, and then the dependency condition is fulfilled also.

This patch flips that around: it forces a certain JS asset to be in the header, but defaults to footer. If a certain JS asset Y is forced into the header, then it is going to run before JS in the footer. Hence there is a potential problem with regards to dependencies: Y's dependencies either live in the footer, and then the dependency condition is not fulfilled, or they live before Y in the header, and then the dependency condition is fulfilled.

In fact, this problem of "dependency loading too late" can easily manifest itself in HEAD if we play with weights. But since opting out of the default header scope is so rare, this problem was probably only ever seen/noticed by a handful of people in D7.


catch bumped this at #72, saying that #66 is fine with him. But this issue seems to have been derailed and gone in a completely different direction since.

The solution is simple: apply a similar, but slightly more advanced dependency resolution system as we have for libraries. I already wrote this more than a year ago in this issue, at #59:

  1. Drupal 8 has properly defined dependencies for each library.
    • Consequence 1: jQuery, drupal.js and drupalSettings can now be loaded only when necessary.
    • Consequence 2: We can make Drupal reason which JS *must* be in the header (i.e. ensure jQuery is in the header if anything that depends on it is in the header).
  2. For us to be able to decide whether a JS asset must be in the header or in the footer, we must use dependency-based ordering, not weight-based ordering. I.e. we must kill the remaining "weight" stuff.
  3. Then, we can implement dependency resolution with the following algorithm (devised together with @sdboyer at DrupalCon Portland):

    1. Create a virtual node for each scope (FOOTER or HEADER).
    2. Let any dependency within the scope depend on the scope's virtual node.
    3. Build dependency tree for each scope.
    4. Subtract the children of each earlier scope's dependency tree from a given scope's dependency tree. (i.e. FOOTER minus HEADER)

The tricky thing there is that we currently define the scope on a per-asset basis, and we resolve library dependencies on a per-library basis (and a single library can include many assets). I think that is also easily solvable. Let's look at an example.

  1. Library 1 contains A.js, B.js and C.js. Only B.js is declared to live in the header scope, the rest is assigned to the default scope (footer). This library is declared to depend on libraries 2 and 3.
  2. Library 2 contains D.js, which is declared to live in the default scope (footer).
  3. Library 3 contains E.js, which is declared to live in the header scope.

First, we build a dependency tree that doesn't care about scope. The dependency tree then looks like this:

(An arrow pointing to another asset means a dependency on that other asset.)

But then if we take into account that B.js is declared to live in the header scope, we simply enforce that scope on all the JS assets it depends on as well. All assets assigned to the header scope are marked in red here:

Now we have all the information we need. We assign all red JS assets in the header scope. Then we assign all remaining assets to the footer scope.


The advantage of the above (#59) is that it requires absolutely zero API changes, unlike #66.

#66 would introduce the concept of a "pseudo library", but:

  • We don't have any pseudo libraries yet.
  • It'd be less visible/explicit than what we have today.
  • It'd be set on an entire asset library, but only affect JS assets in it.
  • It'd force all JS assets to be put in the header, rather than only the ones that need to be. (Of course, you can fix this by splitting it up into multiple libraries).
  • That pseudo library is the equivalent of the "virtual node for each scope" I described in #59. The difference is that #66 proposes to expose that "virtual node" (virtual node ~= pseudo library) to all users, whereas #59 continues to allow users to opt in specific assets to the header scope.

Don't get me wrong, I like the simplicity of #66. For me the biggest reason to not do #66 is the fact that it'd be a rather strange DX: depending on a core/<head> (or something like it) amongst other, actual library dependencies feels weird.

I just wanted to show that it's possible to do #59 and achieve the same.

Bevan’s picture

Wim; Thank you for keeping up with this 4.5 year old overdue issue and trying to bring it back on track. It seems previous discussion and patches might have been ignored.

I agree that the solutions you and I proposed in #59 and #66 (respectively) are better than the solution in the most recent patches. But if that is all that we can do this late in Drupal 8 (because of restrictions on time, developers, API change or whatever else) I think that is probably better than leaving $scope = 'header' as is. WDYT?

hass’s picture

@Bevan: we still have the need to add js code to header. Google analytics is just one example for async code that must not added to the footer. Other tracking tools may require the same. We cannot remove header/footer regions for js. Please read the discussion first.

Bevan’s picture

@Hass; Thank you. I read the discussion again more carefully.

I agree that the ability to add JS to the header must be retained. I do not believe any of the suggested solutions remove that ability. Most solutions suggest removing the "scope" parameter and providing an alternate, smarter method to add JavaScript to <head> without a scope-parameter. E.g. a "body"-dependency, "force_header" option and/or using the library system's dependency-resolution tree with one enhancement.

So I think that we actually agree, but perhaps I misunderstood you. Could you clarify what you think I am missing? Thank you! :)

xjm’s picture

Issue tags: +Triaged D8 critical
hass’s picture

I see no benefit in changing scope to any other name. We all know what footer/header means for many years. force_header means nothing useful to me. Header is already more than clear.

Wim Leers’s picture

I'd be happy to take this on and push it to the finish line once #2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests is done. Once that is done, this issue will be much simpler also. (Plus, this would conflict with the work being done there.) So perhaps it's better to postpone it on that.

dawehner’s picture

Issue summary: View changes
Issue tags: +Novice

Seriously, let's summarize the discussion so far in the issue summary!

To be honest I don't get why we can't do the current patch, allow people to specify a different scope and take that into account when building up the dependency tree.

Bevan’s picture

The last patch does not solve the problem of managing dependencies both across scopes and within scopes. Without that, the API is somewhat broken. This is not a trivial problem. But the Libraries system solves half of it already. It is getting late (too late?) in the Drupal 8 cycle. Contributing is not easy. Given those circumstances, I would be overjoyed for the default scope to be fixed even if the API is broken/incomplete.

I think it would be nice to change "scope" to a boolean. It would be a nice and easy improvement to simplify the API. (We have not found any evidence that anyone uses scope for anything other than header and footer.)

I suggest we split this into three separate issues:

  1. Change the default scope to "footer"; E.g. first/last patches
  2. Manage dependencies both across scopes and within them; As per Wim's comment #124
  3. Make the scope parameter a boolean; Too late for D8?

This will help keep discussions more focussed, goals clearer and patches smaller and more specific.

Wim; You have my support for working on it! If we split this up as per my suggestion, I think you would be focussing on #2. Correct?

#1 could get in in the meantime. If not, #2 would address both #1 and #2. WDYT?

mikeytown2’s picture

Another heads up that some people do set the js scope to something other than header/footer #2102243: Inconsistency with core on adding JS to the page. We should document it if we decide to go the header/footer only route (boolean).

Bevan’s picture

Thanks!

andypost’s picture

Don't wanna derail the thread but +1 to leave scope as string (default to footer).
I'd really like to see follow-up that allow use scope as region name.
At least inline assets needs this.

mikeytown2’s picture

@andypost
Best to speak up in #2382533-5: Attach assets only via the asset library system about inline assets; inline css/js is on the chopping block. I've already given some suggestions like defaulting them to be at the bottom, as messing up aggregate creation was one of the reasons why they wanted to remove support for adding inline css/js.

Bevan’s picture

Saying what you prefer is nice. But explaining the reasons gives weight to it. I still have not seen a convincing reason for the parameter which determines if a javascript resources depends on <body> or not (currently known as "scope") to be anything more complicated than a boolean.

fcaspani’s picture

Assigned: Unassigned » fcaspani
fcaspani’s picture

Assigned: fcaspani » Unassigned
Status: Needs work » Needs review
FileSize
1.34 KB
hass’s picture

Status: Needs review » Needs work

The default js alter and some other stuff is missing. See patch in #122

The last submitted patch, 139: 784626-Change-JS-scope-to-footer-138.patch, failed testing.

gmclelland’s picture

Not sure if it is helpful, but there is also this discussion http://drupal.stackexchange.com/questions/46202/move-aggregated-js-file-...

Wim Leers’s picture

Bevan’s picture

Yay!

Manuel Garcia’s picture

AssetResolver::getJsAssets is where all the action takes place now, and now that _drupal_add_js() is gone, my I guess is we should also remove drupal_js_defaults(). As far as I can see this is only being used in locale's implementation of hook_js_alter.

I'll stay out of Wim Leer's way but this is so awesome I can hardly wait to help out where I can!!

hass’s picture

Where are the default values set now if not in drupal_js_defaults()

Manuel Garcia’s picture

As far as I can see, they are hardcoded for now in AssetResolver::getJsAssets, line 233 of core/lib/Drupal/Core/Asset/AssetResolver.php:

public function getJsAssets(AttachedAssetsInterface $assets, $optimize) {
    $javascript = [];

    foreach ($this->getLibrariesToLoad($assets) as $library) {
      list($extension, $name) = explode('/', $library, 2);
      $definition = $this->libraryDiscovery->getLibraryByName($extension, $name);
      if (isset($definition['js'])) {
        foreach ($definition['js'] as $options) {
          $options += array(
            'type' => 'file',
            'group' => JS_DEFAULT,
            'every_page' => FALSE,
            'weight' => 0,
            'scope' => 'header',
            'cache' => TRUE,
            'preprocess' => TRUE,
            'attributes' => array(),
            'version' => NULL,
            'browsers' => array(),
          );
          ...
Wim Leers’s picture

Title: Set the default scope for scripts to 'footer' » Default all JS to the footer, allow asset libraries to force their JS to the header
Status: Needs work » Needs review
Issue tags: -Novice
FileSize
24.18 KB

Implemented #59 / #64 / #66 / #67 / #124 / #127 / #132. The patch is rather simple.

It:

  1. implements a "virtual" core/<head> asset library for other asset libraries to depend on if they contain critical/crucial assets (for critical UI elements) that should block the page from being loaded
  2. updates core/drupal.dropbutton and core/modernizr to depend on core/<head>
  3. makes the necessary changes to AssetResolver::getJsAssets() to efficiently calculate which JS assets should be loaded in the HTML <head>, taking into account asset library dependencies
  4. removes the 'scope' option for JS assets (both all usages and the ability to use it)
  5. updates the test coverage

Everything works fine AFAICT.

hass’s picture

Status: Needs review » Needs work

There seems to be no way to alter header/footer for assets.

What about CSS assets? Keep https://docs.google.com/presentation/d/1Q44eWLI2qvZnmCF5oD2jCw-FFql9dYg3... in mind.

Wim Leers’s picture

Status: Needs work » Needs review

#149:

  • Sure there is a way to alter header/footer for assets; alter the dependencies of the asset library.
  • What about CSS assets? Drupal doesn't do loading of CSS assets in the footer. They're unaffected by all of this.
Wim Leers’s picture

Also, for the record, I've been working towards this in some shape or form for the last seven years: http://wimleers.com/article/improving-drupals-page-loading-performance#r...

Wim Leers’s picture

FileSize
24.41 KB
459 bytes

nod_ remarked in IRC that drupal.js should also be loaded in the HTML <head>, because it adds the js class on <html>, which can have a profound impact on the looks, since it affects CSS selectors.

Wim Leers’s picture

FileSize
24.44 KB
447 bytes

Manuel Garcia (in IRC) found a bug that's been lurking for a long, long time: drupal.js uses drupalSettings, but a corresponding library dependency is missing!

The last submitted patch, 148: footer-js-784626-147.patch, failed testing.

Manuel Garcia’s picture

FileSize
633 bytes

Find attached my updated test_jsload module to work with patch #153.

It declares as dependencies:

   - core/<head>
   - core/drupal
   - core/jquery.once
  • All dependencies are printed before the module's js on the header (w00t!)
  • drupalSettings is now printed above drupal.js, so that bug is gone (yay!)
  • jquery is pulled in because it is a dependency of jquery.once (nested dependencies do get pulled up to the header!)

This is as awesome as it gets.

As far as what should go by default in the header, let's not pull anything up just because the UI would look bad otherwise, if we can just fix the look on CSS when js is disabled.

Perhaps we should get some of the twig/UI guys to discuss this on a follow up issue. Now that we're finally getting rid of js on the header, let's be very selective as to what we leave there, and maximize the impact this has.

Wim Leers’s picture

FileSize
25.09 KB
885 bytes

nod_ also remarked that html5shiv.js should be in the header; and I realized that picturefill.js was another one that was initially overlooked.

Wim Leers’s picture

FileSize
26.72 KB
11.49 KB

nod_ remarked in IRC that he disliked abusing the library system for this. Turns out that one of the test failures is related to precisely that abuse. So, instead, introducing a critical: true flag on asset libraries. This indicates which asset libraries should block the page from being loaded, because they're critical assets for the page to function correctly. nod_ suggested header: true. Manual Garcia suggested render_blocking: true.

But I actually would prefer critical: true, meaning that we mark the "critical path" assets as such — this would be very valuable metadata when optimizing the critical rendering path (see https://docs.google.com/a/acquia.com/presentation/d/1IRHyU7_crIiCjl0Gvue..., https://developers.google.com/web/fundamentals/performance/, http://fourword.fourkitchens.com/article/optimizing-critical-rendering-path). But it would also be very valuable metadata when building alternative, smarter CSS and JS aggregators! (In D8 contrib or in future versions of D8 core.)

Tests should pass now.

The last submitted patch, 152: footer-js-784626-152.patch, failed testing.

The last submitted patch, 153: footer-js-784626-153.patch, failed testing.

catch’s picture

Agreed with Manuel Garcia on the js class and dropbuttons. It's always possible to either change those dependencies in a follow-up issue, or for contrib/custom code to make them more conservative. The more libraries in core have critical: true the more contrib will end up copying those examples. So without bug reports for those I'd limit this strictly to polyfills.

Manuel Garcia’s picture

FileSize
648 bytes

Here's the updated testing module, using critical: true. Everything seems to be in order (see #155).

I like what Wim Leers is suggesting syntactically (critical). It makes sense to me because it speaks as to what it actually means in terms of priority to the browser.

We'll see how contrib's ego reacts to the word critical, but we can always put big bold red letters on the documentation...

hass’s picture

What about CSS assets? Drupal doesn't do loading of CSS assets in the footer. They're unaffected by all of this.

Per https://docs.google.com/presentation/d/1Q44eWLI2qvZnmCF5oD2jCw-FFql9dYg3... it is important for remove the CSS blocking, too. It looks like the same logic used for JS should also be used for CSS.

I think header: true is best. It makes clear where this code need to stay.

The last submitted patch, 156: footer-js-784626-155.patch, failed testing.

cosmicdreams’s picture

Status: Needs review » Needs work

at #157
Then call it critical_path. Critical means too many things and is a little confusing (on the surface I didn't understand what you meant)

at #162
Or header. yea header is fine.

The last submitted patch, 157: footer-js-784626-157.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
29.22 KB
2.56 KB

Now it will actually be green.

larowlan’s picture

Issue tags: +Critical Office Hours

Tagging for manual review in office hours today

joelpittet’s picture

@Wim Leers nice work, green:) I do agree with @cosmicdreams above critical_path: true is a bit clearer, or header: true like @hass mentioned. 'critical' has a bit to vague a meaning in the context, save a few doc lookups.

Just curious, why did all those test need their offsets adjusted? Was that related to #153 fix?

Wim Leers’s picture

FileSize
29.3 KB
5.29 KB

#168: those offsets needed to be adjusted because in HEAD, all JS was in the header (== 1 AJAX command), then in most of the above patches all JS was in the footer (== 1 AJAX command), but then we realized drupal.js belongs in the header (== 2 AJAX commands: header JS + footer JS).

Updated from critical: true to critical_path: true.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
166.03 KB
271.96 KB

@Wim Leers thanks for explaining that, very informative:) And thanks for the variable change, I hope that is clear enough to it's purpose.

I tested it to make sure JS still work as the tests say they do.

How easy would it be for contrib to move the jquery library to head in their theme?

Wim Leers’s picture

Issue tags: -Needs manual testing

How easy would it be for contrib to move the jquery library to head in their theme?

Any library that has critical_path: true set and depends on jQuery — directly or indirectly — will cause jQuery to be loaded in the header.

Also created a CR: https://www.drupal.org/node/2412769

alexpott’s picture

Assigned: Wim Leers » catch

Assigning to @catch as I know this is a favourite.

catch’s picture

Would be good to get nod_'s opinion on critical_path vs. the other options.

I don't have a strong view on that, having a key instead of the extra library is good, 'critical_path' is better than 'critical'. I can understand the desire to not call it 'header' since there could be rendering strategies where it doesn't just mean that, but not sure that's enough of a reason to call it something else either. Should be relatively few libraries that need to set this so not that bothered.

nod_’s picture

I'm for header: true. It's the same thing as optimized/minified key in js file settings. In the end we went with minified because people can't make it mean something we don't want.

Same thing here, header can't be misunderstood whereas critical or critical_path will be easy to give it a meaning we didn't want to give.

Wim Leers’s picture

FileSize
29.21 KB
5.28 KB

header: true it is. CR also updated.

catch’s picture

Status: Reviewed & tested by the community » Needs work

That works for me.

Minor things from review. If you disagree with the first one, we could do the opposite but I still think it's worth discussing in its own right in a follow-up.

  1. +++ b/core/core.libraries.yml
    @@ -43,11 +43,14 @@ domready:
    +  header: true
    

    Let's take the drupal and dropbutton hunks out of the patch and open a follow-up to discuss.

  2. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
    @@ -214,8 +214,23 @@ protected function getJsSettingsAssets(AttachedAssetsInterface $assets) {
    +      if (isset($definition['js']) && isset($definition['header']) && $definition['header']) {
    

    Nit, could just be !empty().

  3. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
    @@ -233,6 +247,10 @@ public function getJsAssets(AttachedAssetsInterface $assets, $optimize) {
    +          // marked as 'blocking' (see above).
    

    Now could just say 'header'.

nod_’s picture

Drupal.js needs to be in header, there is a piece of code that needs to be executed ASAP to avoid flash of unstyled content, everything with the .js prefix.

Wim Leers’s picture

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

#178 is going to fail in DialogTest; rerolling.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
26.25 KB
2.56 KB

This will be green again.

The last submitted patch, 178: footer-js-784626-177.patch, failed testing.

attiks’s picture

Why did you move picture to head? There's no reason to do so.

Wim Leers’s picture

#182: Huh? How can Picturefill prevent the fallback behavior from happening instead of perform its duties of a polyfill if it's loaded *after* the fallback behavior has already been executed? That's what may happen if it were loaded from the footer.

attiks’s picture

#183 Picturefill does not prevent the fallback from happening, no matter what since it gets prefetched by the browser, on our site we load it at the bottom, see https://attiks.com/project/attiks-logo-design-and-huisstijl

Wim Leers’s picture

Aha! Good to know :)

Then there is still one reason to load picturefill in the header, but we should do it with async (as they recommend themselves), to ensure we load the right version of the image as soon as possible, because fetching that image can take some time too. If we only do it after the entire page has been fetched, that might cause some content to jump.

attiks’s picture

##18 Async is fine, but content reflow will roughly be the same no matter where it is loaded. Only solution for the reflow is using css the specify width and height, there used to be an issue to add it to picture/sources but it was not accepted.

catch’s picture

Sounds like we should probably remove that as well. I'm not able to commit at the moment, but I can commit a re-roll with it taken out, or just remove hunk from the patch on commit.

Manuel Garcia’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
25 KB
491 bytes

If we set async on picturefill it'll still block rendering and likely reflow the page, once it has been downloaded. The only difference is that it will not block anything while it is being downloaded for the first time. Having it on the footer on the other hand means that the page will be rendered before picturefill is loaded and executed, no matter if it has already been downloaded or not.

Here is a reroll of #180 with picturefill moved to the footer.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

It's a sufficiently small change to keep it at RTBC, also because a follow-up to decide on unclear cases already exists: #2412945: Determine which additional asset libraries should be in the critical path/loaded i/t header (core/drupal, core/dropbutton).

  • catch committed ea568f1 on 8.0.x
    Issue #784626 by Wim Leers, Manuel Garcia, jcisio, joelpittet, Bevan,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

Also second-oldest critical in the queue closed :)

Wim Leers’s picture

WOW!!! I can't quite believe this is finally done! :D

As I said in #151, I've been working towards this in some shape or form for the last seven years… so it feels incredibly satisfying to finally have this done :) I already couldn't wait to see Drupal 8 be used in the wild, now even more so! :)

joelpittet’s picture

Thanks everybody for their help on this! Congrats @Wim Leers and @Manuel Garcia.

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture