Just had an idea for how we could easily implement just-in-time loading of javascript files using the cool new AJAX command framework. By adding a file attribute to an AJAX command object, we could check for the existence of the returned command in javascript, and if it does not exist, use jQuery.getJSON() to load the required file, then run the command in the callback.

This would allow contrib and custom modules to load scripts JIT rather than needing to load these scripts before the ajax call is made. An example command callback would look like this:


function custom_ajax_command() {
  return array(
    'command' => 'my_custom_command',
    'file' => drupal_get_path('module', 'my_custom_module') .'/custom.js',
  );
}

Thoughts?

Post-commit notes:

  • The solution committed is not compatible with inline CSS and JS items, only files. CSS ans JS added with the 'inline' parameter will not be lazy-loaded during an AJAX response. This appears to be a limitation with the way inline code is indexed by drupal_add_js/css which could not be addressed here.
CommentFileSizeAuthor
#225 drupal8.ajax-lazy-load-tests.225.patch9.56 KBsun
#215 user_modal.tar_.gz1.56 KBamitaibu
#211 drupal.ajax_lazy_load_tests-211.patch10.19 KBeffulgentsia
#209 drupal.ajax_lazy_load_tests.patch10.19 KBeffulgentsia
#207 drupal.ajax_lazy_load_561858_207.patch2.83 KBeffulgentsia
#207 field_edit_in_place.zip3.3 KBeffulgentsia
#201 drupal.ajax_lazy_load_561858_201.patch1.11 KBeffulgentsia
#188 drupal.ajax_lazy_load_561858_188.patch34.76 KBeffulgentsia
#184 drupal.ajax_lazy_load_561858_184.patch32.89 KBtstoeckler
#180 drupal.ajax_lazy_load_561858_180.patch32.9 KBrfay
#173 drupal.ajax-lazy-load.173.patch34.14 KBeffulgentsia
#172 drupal.ajax-lazy-load_561858_171.patch34.2 KBNick_vh
#169 drupal.ajax-lazy-load_561858_168.patch33.57 KBNick_vh
#168 drupal.ajax-lazy-load_561858_168.patch25.49 KBNick_vh
#159 drupal.ajax-lazy-load.158.patch30.69 KBsun
#153 drupal.ajax-lazy-load.153.patch29.64 KBsun
#142 drupal.ajax_lazy_load_561858_142.patch29.14 KBrfay
#140 drupal.ajax_lazy_load_561858_140.patch29.27 KBrfay
#135 drupal.ajax-lazy-load.135.patch12.48 KBsun
#132 drupal.ajax_lazy_load_561858_132.patch12.18 KBeffulgentsia
#130 drupal.ajax_lazy_load_561858_130.patch12.01 KBeffulgentsia
#125 drupal.ajax_lazy_load_561858_125.patch37.26 KBNick_vh
#122 lazy_load_demo_122.tgz10.06 KBrfay
#114 561858-ajax-lazy-load.patch26.15 KBmerlinofchaos
#114 lazy_load_temo.tgz10 KBmerlinofchaos
#112 561858-ajax-lazy-load.patch18.34 KBmerlinofchaos
#108 lazy_load_demo_108.tar_.gz1.45 KBrfay
#106 drupal.ajax-lazy.106.patch17.46 KBsun
#105 drupal.ajax-lazy.105.patch18.45 KBsun
#104 drupal.ajax_lazy_load_561858_104.patch18.44 KBeffulgentsia
#98 drupal.ajax_lazy_load_561858_85.patch11.27 KBdawehner
#85 drupal.ajax_lazy_load_561858_85.patch13.33 KBkatbailey
#78 drupal.ajax_lazy_load_561858_78.patch11.24 KBrfay
#74 lazy_load_demo.tgz1.4 KBrfay
#68 561858-load-js-files-via-ajax.patch7.4 KBmerlinofchaos
#68 examples-module.patch1.25 KBmerlinofchaos
#67 lazy_load_demo.tgz1.08 KBrfay
#63 drupal.ajax_lazy_load_561858_63.patch6.04 KBrfay
#29 drupal-ajax-lazy-css-js-561858-28.patch6.83 KBeffulgentsia
#28 drupal-ajax-lazy-css-js-561858-28.patch6.83 KBeffulgentsia
#5 drupal.ajax-lazy.5.patch5.83 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

Would be great to apply this to not only JavaScript, but also CSS. Now that #attached_css, #attached_js and #attached_library are part of the Form API, seems reasonable to apply them here. During the JSON return, we'd have to designate some return that would in turn call jQuery.get*(). Maybe attached_css/attached_js through the JSON callback? Great idea, something that is definitely needed. Would this support the other types of JavaScript/CSS we have now, like inline or external scripts?

The Popups API just avoided it by adding the additional JavaScript straight to the page rather then loading it with jQuery.getScript().

andrewlevine’s picture

This is a very cool idea. My only warning is that it might not make sense to load many types of scripts JIT. My guess is that cached and aggregated blocks of javascript will end up performing better and/or *appearing* to perform better than loading specific javascript separately when a user is performing an action and then waiting for something to happen.

One case where I can see this being most useful is when a user is opening a modal which is essentially a whole separate client-side application. If the user isn't opening that modal on every page request there is no reason to load that javascript every time.

A feature that also may be useful for this would be to aggregate .js files for all commands being run into one getJSON request to avoid the HTTP request overhead of loading multiple scripts. So if you are running multiple commands, Drupal would load all the JS with one getJSON request before running the commands.

haydeniv’s picture

Isn't this already being accomplished with the Ajax Load module?
http://drupal.org/project/ajax_load
Could be wrong but seems to be similar.

RobLoach’s picture

Also note that the jQuery 1.4 Roadmap speaks of jQuery.require(), which might assist with asynchronous loading of JavaScript libraries.

sun’s picture

Title: Just-in-time loading of javascript files in new AJAX framework » Lazy-load JavaScript and CSS via AJAX framework
Category: feature » task
Priority: Normal » Critical
Status: Active » Needs review
FileSize
5.83 KB

All credits for this patch belong to merlinofchaos. I merely took his pastebin diff and applied it to core.

Possibly be broken due to my D6 thinking, but let's get this QUICKLY into the wild + fix this.

Status: Needs review » Needs work

The last submitted patch, drupal.ajax-lazy.5.patch, failed testing.

seutje’s picture

This is awesome, I've been using various projects to accomplish pretty much the same and what worries me with the current approach is that there doesn't seem to be any talk about throwing an event when the JS/CSS is done loading or allow some sort of callback after the JS/CSS has loaded (or if it was already loaded in the first place)

for instance, the dominoes project requires a syntax like the following:

dominoes('path/to/script.js', function() {
  callFunctionFromLoadedScript();
});

for CSS this would be

dominoes('$css(path/to/stylesheet.css)', function() {
  // do some manipulation that would require the loaded css
});

also, having the option to re-load a cached script can be really handy in some cases

dominoes({
  url: 'path/to/script.js',
  cache: false
}, function() {
  // do work
});

using an approach like dominoes also allows u to easily use it as a dependency system

not saying we should drop the dominoes framework in drupal and introduce another 3rd party library to core, but it might be interesting to look at just to get some inspiration on how to handle various use-cases

-> http://github.com/jaubourg/dominoes

will have a more in-dept look at sun's patch when I get back, rly gotta run now

merlinofchaos’s picture

Well, the way this is set up, CSS and .JS will be included before any behaviors are run. Behaviors should be able to handle what you need?

RobLoach’s picture

We'll have more control over this if we use something like xLazyLoader. Parts of it were also planned to go into jQuery 1.4 during the roadmap with jQuery.require, but they decided to hold off on it. So, the plugin is pretty future proof, and it means we'll know it works on all browsers.

seutje’s picture

yea and that's a much saner syntax, and since we can opt-out on core js files a module can easily drop in an updated jQuery version and u prolly wouldn't even have to change the syntax

it is kind of late though

sun.core’s picture

We need a solution for this. 90% of AJAX stuff is more or less useless without lazy-loading.

q0rban’s picture

subscribe

Pasqualle’s picture

subscribe

effulgentsia’s picture

subscribe

effulgentsia’s picture

Re #9: I have no objections to using xLazyLoader if that's Rob's recommendation. Does anyone else? If we can agree on that, who's up for rolling the patch?

[Edit: @seutje: sorry, I read #10 initially as a question, but now I see it's a statement. Care to elaborate on why it's too late?]

effulgentsia’s picture

Issue tags: +Ajax

There are a bunch of AJAX-related issues in the queue, and no AJAX component, so tagging to try to get a handle on them.

casey’s picture

Subscribing.

effulgentsia’s picture

#653580: Upgrade to jQuery 1.4 landed. Does that let us move forward without xLazyLoader, or do we still need xLazyLoader (#9)?

sun’s picture

$.require() is not part of jQuery 1.4 (at least there are no docs). So this most probably means xLazyLoader library.

The biggest challenge for this issue is to support our JS/CSS aggregation.

Technically, we need to know whether we need to load a file or not, based on the original/parent page. Suggestion for attack:

- Make drupal_add/get_js/css() generate a list of files and add that list into Drupal.settings

- Make it possible to opt-in into this lazy-loading support, so we don't add that potentially large list of files needlessly on all pages.

- Implement the actual lazy-loading.

effulgentsia’s picture

Please also see #384992: drupal_html_id does not work correctly in AJAX context with multiple forms on page. Something I explored in a D6 contrib module is a page build id and storage of information based on it: http://drupal.org/project/ahah_page_storage. I'm not too happy with that implementation, but I think it can be improved. If it can be made to work well, then it can help with this issue, as well as that other one. Biggest issue is efficiency: how to not pollute a database table with way too many page build storage records.

cosmicdreams’s picture

@sun: It seems that the jQuery team is planning to eventually provide a .require API during 1.4.x series: http://docs.jquery.com/JQuery_1.4_Roadmap .

That roadmap is open for a little interpretation due to their lack of description or clear explanation of their goals. I know that information doesn't change the proposed plan of attack, I just wanted to add that to the knowledge pool here.

RobLoach’s picture

@cosmicdreams: In an interview with John Resig somewhere on 14 Days of jQuery, they talk about how they were thinking of adding it into jQuery 1.4, but ended up deciding against it because they wanted to make sure to do it right and not fit it in such a small timeline. Most likely it'll be in jQuery 1.5.

casey’s picture

Not sure if xlazyloader is jquery 1.4 ready.

merlinofchaos’s picture

I took a quick look at xLazyLoader and I don't think it will really work for us here.

1) It only checks for dups with itself. If you load a file traditionally with a <script> tag and with xLazyLoader it will be loaded twice.
2) It won't help us with aggregation at all, which is the one place the method I am working with doesn't actually work. I'm still working out how to deal with this, but I think I've got something.

effulgentsia’s picture

@merlinofchaos: I'm very interested to see what you come up with. In case you haven't seen #384992: drupal_html_id does not work correctly in AJAX context with multiple forms on page, please take a look at it. If what you've come up with is something along the lines of a persisted $page_state, so that AJAX requests can submit a page_build_id, and from that single id, Drupal can retrieve the $page_state info from the prior request, then that would be useful on that issue too. Seems like a natural extension of the form_build_id / $form_state pattern we use for multi-step forms, except here, it's more like multi-step page requests that all correspond to the same page. Or maybe, you're thinking of something totally different that only applies to CSS and JS tracking.

sun’s picture

@merlinofchaos: Any updates? Skeleton template pseudo code? We'll eat it. We badly need some progress here. As of now, I guess that everyone's on hold for the first draft you mentioned, so as to prevent wasting time with a different approach. This issue will be the #1 show-stopper for anything remotely related to advanced AJAX operations in D7.

@all: Any other suggestions/approaches based on the rough edges outlined in #5 and #19 ?

agentrickard’s picture

I'm sorry. Why is this critical? Seems like a nice-to-have, not a release blocker.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
6.83 KB

I remain interested in whatever merlinofchaos comes up with, as I'm sure it will totally rock. In the meantime, here's a proof of concept for the "persist page state" idea. This patch does not refactor drupal_html_id() to use it, but that would be the logical next step once the rest of the patch is polished. This even "works", in that if you add a multi-valued field to the Basic Page type and then try to add a node of that type, and click "add another item", you get garland's css files loaded :)

effulgentsia’s picture

I remain interested in whatever merlinofchaos comes up with, as I'm sure it will totally rock. In the meantime, here's a proof of concept for the "persist page state" idea. This patch does not refactor drupal_html_id() to use it, but that would be the logical next step once the rest of the patch is polished. This even "works", in that if you add a multi-valued field to the Basic Page type and then try to add a node of that type, and click "add another item", you get garland's css files loaded :)

merlinofchaos’s picture

Sorry, I haven't had a chance to work on this in awhile.

sun’s picture

This looks quite impressive. However, I'm not sure whether it is valid to presume that the server should always know upfront which page requisites already exist. As of now, similar to the drupal_html_id() issue, it would look more natural to me that the client tells the server of what the client already knows. Contrary to form builds, AJAX requests can demand for anything, not even remotely related to the contents that are already on the base page. We also have to take page caching into account.

sun’s picture

Discussing this issue with effulgentsia, I think the major decision we need to take is whether the actual processing and lazy-loading should be handled on the server-side or on the client-side.

In addition to that, I'm a bit concerned about our various caches, which may unintentionally break (shortcut) the server-side processing.

effulgentsia’s picture

I think the major decision we need to take is whether the actual processing and lazy-loading should be handled on the server-side or on the client-side.

I agree. #29 takes the server-side approach which has some advantages (like knowing which source files are "new" even when aggregation is enabled and being able to aggregate all of those new ones), but also some possible pitfalls, especially if client-side code loads css/js files that don't come from drupal_get_(css|js)(). A client-side solution could be more robust in handling those "rogue" files, but encounters challenges with aggregation.

I'm a bit concerned about our various caches, which may unintentionally break (shortcut) the server-side processing.

$page_state persisting in cache_form with a 6 hour life time means that AJAX won't work right (in regards to the lazy loading) for pages that have been cached in the page cache for more than 6 hours. But this problem already exists for AJAX form submissions, since those require $form to be pulled from the same cache bin with the same life time. So I don't think the server-side approach adds a new problem, just more motivation to solve (or accept) the existing one.

[EDIT: Presumably, we could make pages that have AJAX-enabled elements (i.e., run through ajax_process_form()) not be eligible for page caching or expire from the page cache in less than 6 hours as a way of solving the above problem]

effulgentsia’s picture

I don't have time to work on this today, but having slept on it, I'm increasingly confident that the server-side approach makes sense. The render cache isn't a problem, because all the page state related logic is happening in ajax_process_form() (a #process function that runs regardless of render cache), ajax_render() (which always runs for returning an AJAX response), and template_process_html(), which is unlikely to be render-cached, as at that level, you're working with the page cache. As per #33, the page cache is a problem, but it *already* is breaking AJAX, and if that gets fixed (for example, by making pages with AJAX-enabled elements expire from the cache before the corresponding $form expires from the cache), then the problem is fixed for $page_state too.

So that leaves "rogue" CSS/JS inclusions (ones that weren't driven by drupal_add_(css|js)()) as the only problem with a server-side solution. But that can be solved in contrib or in the custom module/theme (we may need to add hook_page_state_alter() in addition to hook_page_state_update() in order to give themes a way in) responsible for the rogue inclusion.

I'm still open to further debate on this. Just sharing my latest thoughts on it.

rfay’s picture

subscribing

Wim Leers’s picture

I haven't read the entire issue, but I did a quick review the code. I was going to say that there is a problem for cached pages with anonymous users; that there would be issues with cached pages.

Fortunately, I was wrong: that problem has been addressed very elegantly:

+  // There can be many users viewing pages that need their page state persisted.
+  // We don't want each one having a randomly generated id, as that would flood
+  // the cache bin with many records. By basing the id on the page state
+  // content, we only need as many records as there are unique states.
+  $page_state_id = md5(serialize($page_state));

I fully support this feature. This will also allow for improved page loading performance.

Wim Leers’s picture

Issue tags: +Performance

Making this issue also show up in the performance issues. Page loading performance is a specific kind of performance.

yhager’s picture

Category: task » bug

Marking as a bug. We also need this for #768128: ajax callbacks can't apply #attached

Scott Reynolds’s picture

   // Automatically extract any 'settings' added via drupal_add_js() and make
   // them the first command.
   $scripts = drupal_add_js(NULL, NULL);
   if (!empty($scripts['settings'])) {
     array_unshift($commands, ajax_command_settings(call_user_func_array('array_merge_recursive', $scripts['settings']['data'])));
+    $commands[0]['styles'] = isset($new_css) ? drupal_get_css($new_css) : '';
+    $commands[0]['scripts'] = isset($new_js) ? drupal_get_js('header', $new_js) : '';
   }
 
   // Allow modules to alter any AJAX response.

seems like the opening comment needs an update. This also means that if there are no 'settings' it won't add new CSS or JS scripts. CSS obviously being the bigger issue.

+  // $page_state serves a similar purpose to $form_state. Therefore, persist it
+  // in the same cache bin with the same 6 hour life time.
+  cache_set('page_state_' . $page_state_id, $page_state, 'cache_form', REQUEST_TIME + 21600);

But this doesn't have to do with the form at all. Doesn't it belong in just the cache bin?

Scott Reynolds’s picture

Also, this is using the ajax settings command to add scripts and styles. Seems like we need two new commands as well.

katbailey’s picture

Status: Needs review » Needs work

This solution would only work for #ajax form elements. We need more people thinking about non-form-related ajax-loaded content. Currently the only way to use the ajax framework in a context other than forms is with the 'use-ajax' class on a link and it seems like this is considered totally peripheral to what the ajax framework is all about. However, the ctools version of the framework uses it for loading content in modal dialogs for example (a pretty common use case and also very similar to another common use case, tabs) - these require lazy-loaded js & css and a solution for this has already gone in for D6. We need an equivalent solution for D7 core ajax.

Adding a link to #647228: Links are needlessly unable to fully participate in D7 AJAX framework features which may not be necessary and is probably too big a change for this late in the game anyway, but might be food for thought...?

merlinofchaos’s picture

Anyone who wants should consider checking what recently went into CTools -dev. We added a page ID that is automatically consulted when calculating what additional .css and .js files to add on ajax response. the ajax renderer in CTools automatically adds this stuff on every ajax render, which is how it should work in core as well using the delivery callback.

sun’s picture

I had a look at the code that went into CTools, and if I didn't overlook an important part, then effulgentsia's existing proof-of-concept patch here achieves more or less the same, but 1) with less page state ids and 2) without the need for jquery_update's js replacements hook.

Scott Reynolds’s picture

Not sure it gets around 2.), but yes it does indeed produce less page state ids.

Although, I currently couldn't get this to work as drupal_page_state_enabled always returned false. Only time it was enabled is on ajax_render. So the page state for the current non-ajax page isn't cached away.

sun’s picture

wow. effulgentsia, you totally rock.

We need to add the following to the non-form part of Drupal.ajax() though:

    else {
      // Give the server information about the page state.
      options.data['_page_state_id'] = Drupal.settings.pageStateId;

      // Prevent duplicate HTML ids in the returned markup.
      // @see drupal_html_id()
      options.data['ajax_html_ids'] = [];
      $('[id]').each(function () {
        options.data.ajax_html_ids.push(this.id);
      });

      $.ajax(options);
    }

With that, ajax-delivered HTML returned via ajaxified links are lazy-loading JS/CSS as well. :)

sun’s picture

And, for the record, either themes need to use more important CSS selectors, or we could/should think about ways to inject lazy-loaded CSS before theme stylesheets. Debatable. Shouldn't hold off this patch.

pwolanin’s picture

Priority: Critical » Normal

nice to have

merlinofchaos’s picture

I'm not sure this should be downgraded to normal. See #38

sun’s picture

Priority: Normal » Critical

I agree. Not only due to technical constraints (#38), but also due to logical reasons: you almost can't do anything useful with the AJAX framework currently.

If eff or someone doesn't beat me to it, I'll re-roll and complete some parts of this patch shortly.

merlinofchaos’s picture

This is critical because:

If you have AJAX that loads further AJAX, and that AJAX needs javascript that is not on the page, it won't work. This is happening right now in http://drupal.org/node/768128

I have also run into this problem trying to add additional javascript widgets. Some examples that take work to get around that should not:

1) Adding an autocomplete widget to a form when autocomplete.js is not already on the page.
2) Adding a farbtastic color picker to a form when farbtastic.js (and the associated thousand (exaggerating) lines of .js to configure it) are not on the page.
3) Adding a collapsible fieldset widget to a form when collapsible.js is not already on the page.
4) Adding a wysiwyg editor widget to a form (or the resizable textarea) when the .js necessary for that wysiwyg item is not already on the page.

I don't know what the #attached feature mentioned above actually is or does, so I can't speak to it, beyond knowing that it's one of Many Examples that don't work right now without workarounds.

Wim Leers’s picture

I fully agree with sun and merlinofchaos that this is critical.
Without this, anything AJAX that's even slightly advanced (or optimized in the sense that css/js/images are loaded only when necessary), it will become a nightmare. As it was in Drupal 5. As it still was in Drupal 6. Even if only for the sake of making it easier for Views — that alone justifies it IMHO. But please also think of the dozens of developers who will bang their heads against the wall trying to figure out work-arounds while they shouldn't need to do that. This is one of the things that can set Drupal apart in AJAX DX.

effulgentsia’s picture

Re #49:

If eff or someone doesn't beat me to it, I'll re-roll and complete some parts of this patch shortly.

Realistically, it could be about a week or so before I'm able to pick this up again. If you have time between now and then, by all means, please move it along. Otherwise, I'll pick it up again as soon as I can carve out the time.

fago’s picture

subscribing

effulgentsia’s picture

Component: javascript » ajax system

.

chweetraju’s picture

subscribing

mfer’s picture

A couple things:

  1. Instead of md5() use drupal_hash_base64() or hash('sha256', $data).
  2. Caching is nice to have but if the cached data isn't there it shouldn't break anything. Clearing caches on the fly should be safe and just something to be rebuilt. What about a separate 'registry' type of thing for the page ids. We could add a separate way to clear it. Unlike caches, clearing this could cause page problems.

Otherwise we should be able to pull forward what ctools is doing.

I'm not sure this is critical (certainly annoying) but I would not file it as normal either.

catch’s picture

I think that's the definition of what the major status is for, so tagging. Also removing a load of other tags which don't look very helpful.

rfay’s picture

So is #29 the last contribution here? Now we're at #58. It seems to me like there has been great feedback on effulgentsia's approach. There's lots of moaning about this over on #768128: ajax callbacks can't apply #attached, which is blocked by this one.

effulgentsia’s picture

I marked #768128: ajax callbacks can't apply #attached a duplicate of this one. Apologies that I haven't made any further progress here. I will resume it as soon as I'm able to, but if someone else wants to step in and drive, go for it.

yched’s picture

subscribe.
/me wants #states in ajaxed forms

yched’s picture

Anonymous’s picture

subscribing

rfay’s picture

Status: Needs work » Needs review
FileSize
6.04 KB

This is a straight re-roll of #29 against HEAD with md5() replaced with drupal_hash_base64().

This issue is so important to the future of D7.

mikeytown2’s picture

+++ includes/common.inc
@@ -6794,3 +6794,79 @@ function drupal_get_updaters() {
+/**
+ *
+ */
+function drupal_page_state_id($new_id = NULL) {
...
+/**
+ *
+ */
+function drupal_get_page_state($id = NULL) {
...
+/**
+ *
+ */
+function drupal_page_state_enabled($enable = NULL) {
...
+/**
+ *
+ */
+function drupal_update_page_state(&$page_state = NULL) {

These functions need to be documented. I need to know what they do and how to use them.

Powered by Dreditor.

mikeytown2’s picture

Status: Needs review » Needs work
merlinofchaos’s picture

This patch to CTools implements a version where we do not use a relatively fragile server side cache: http://drupal.org/node/815164

I *highly* recommend going this route instead.

rfay’s picture

FileSize
1.08 KB

@merlinofchaos: Definitely looking forward to your approach.

Attached is a simple module which I think demonstrates why we need lazy loading and can be used to affirm that a patch is working for us. (It does NOT work with #63).

The simple module here simply adds #attached on a form element when rebuilding in an AJAX call. In a happy world it would cause the appropriate javascript (named in the #attached) to be loaded. It does not.

Remember katbailey's admonition in #41, though, that is not just about FAPI.

If this demo module is *not* a potential demonstration, just tell me why and I'll try to make a better one.

merlinofchaos’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
7.4 KB

Ok. I took a couple hours tonight and I have put together knowledge gleaned from CTools to make this happen.

This patch goes back to Sun's original patch. The page state idea, IMO, is the wrong direction, and for this effort I am abandoning it.

Instead, we go back to what I'm doing in CTools: There is a command to send a list of scripts to the client. Simultaneously, during aggregation, the system tells the client what scripts are in the aggregated file. As a result, we are able to cleanly and safely add .js files during AJAX operations, which makes a whole host of goodness via AJAX requests possible.

In addition, I've included a simple test that will show very simply what happens in Drupal right now, and how things will improve with it. Simply apply the examples module patch to the examples module and enable the ajax example module. The 'Ajax link' example contains a small link that adds content to the page by selecting a link. In this case, it adds a form which contains a textarea.

In normal Drupal, the javascript to make the textarea resizable is not present. Therefore, using just this without my patch, the newly added textarea will not be resizable.

With the patch, the client is told what javascript files were requested during the AJAX callback. It compares that to the list of files currently used, and loads the difference. In the simple case, it should only be textarea.js. Then, behaviors trigger as per normal and the newly added textarea is resizable. This works properly with javascript aggregation on and off.

My belief is that other javascript goodies such as #attach should automatically work as well. Adding that behavior to the example form will be a good test, for those who know how to do it.

One final note: In CTools and in the original patch here, CSS files were also transmitted. However, I had trouble getting this to work. In 7, a different method of adding CSS files (one that I assume helps get around the IE 30 stylesheet limit, in fact) is used, and I'm not sure how to detect the files. In any case, it is probably just a matter of doing precisely what we're doing now with javascript, only in CSS. Unfortunately, my time to work on this patch is limited, so while that piece of it is important, I am going to have to leave that one for someone else.

merlinofchaos’s picture

For the record, I tested this in IE8, Chrome and Firefox.

Firefox is the easiest browser for testing, since firebug makes it easy to see precisely what files are requested. IE8's debugger should theoretically show me that, but I couldn't figure out how to get it. Chrome probably has something.

merlinofchaos’s picture

Oh and one further note: Clicking the link again will add another textarea. One good thing to note is that it will not add textarea.js a second time.

rfay’s picture

Status: Needs review » Needs work

Worked right out of the box with the example I posted in #67.
merlinofchaos++

The only thing that I didn't anticipate is that when the #attached in my example is omitted, the js behavior on the page continues. So the textfield stays red, even when the field has been rebuilt in AJAX with no #attached.

rfay’s picture

Status: Needs work » Needs review

Not sure why it went to needs work.

Status: Needs review » Needs work

The last submitted patch, examples-module.patch, failed testing.

rfay’s picture

Status: Needs work » Needs review
FileSize
1.4 KB

The attached is a version of the demo module from #67 that also does #states (set within an AJAX load).

IT WORKS GREAT! #states turns on and off just fine. Yippee!

rfay’s picture

Status: Needs review » Needs work

Ah the weirdness. Why does it always do the wrong thing?

merlinofchaos’s picture

Argh. It looks like all of the tests that fail are ones that are expecting ajax return packets that do not include the newly added 'scripts'. Sigh. I am not going to have time to fix the tests. Can someone else take this? Pretty please?

rfay’s picture

One of us can do it, I'm sure. You made the big push! If I can get to it later today I will.

rfay’s picture

Status: Needs work » Needs review
FileSize
11.24 KB

It looks to me like this wasn't as hard as I thought it would be. The results of an ajax_render just got pushed back from the front by the new script item, so the tests had to be adjusted accordingly. We'll see what the bot thinks.

rfay’s picture

OK, the bot likes it. Now this needs reviews from all the JS meisters who wanted this :-).

#74 has a simple module (that can be expanded) that demonstrates lazy loading with #attached and #states. And #68 has a patch to show the ajax link example from Examples project's AJAX example.

DamienMcKenna’s picture

How is the failover for CSS loading if JS is disabled in the browser?

mfer’s picture

+++ includes/common.inc
@@ -3799,6 +3800,8 @@ function drupal_get_js($scope = 'header', $javascript = NULL) {
+          $reported_files[] = file_create_url($item['data']) . $query_string_separator . ($item['cache'] ? $query_string : REQUEST_TIME);
+

The $reported_files is 'registering' the files that end up in the preprocessed files. In drupal_get_js() none of the other js files are registered. Instead, it happens in the browser is Drupal.ajax.prototype.commands.scripts.

Can you add a comment to both of these pointing out that part of the registration happens in each of them. If they were not in the same patch file I would have spent a lot of time tracking this down.

+++ misc/ajax.js
@@ -13,7 +13,13 @@
+Drupal.ajaxFiles = Drupal.ajaxFiles|| { scripts: {}, css: {} };

There should be a space before ||

+++ misc/ajax.js
@@ -427,6 +433,31 @@ Drupal.ajax.prototype.commands = {
   /**
+   * Command to add scripts to the page.
+   *
+   * We keep a record of what CSS files are already on the page and attempt
+   * to avoid adding more.
+   */
+  scripts: function (ajax, response, status) {

Here is says that we are keeping a record of the CSS files. But, instead we are keeping a record of the JavaScript files. Typo?

+++ misc/ajax.js
@@ -427,6 +433,31 @@ Drupal.ajax.prototype.commands = {
+    $('script').each(function () {
+      Drupal.ajaxFiles.scripts[this.src] = this.src;
+    });
+
+    var html = '';
+    for (var i in response.files) {
+      if (!Drupal.ajaxFiles.scripts[response.files[i]]) {
+        Drupal.ajaxFiles.scripts[response.files[i]] = response.files[i];

Each time this runs detecting all the scripts and adding the new one seems a little redundant. But, we do not control everything that may add scripts. So, it seems good to make sure the list is up to date each time.

+++ misc/ajax.js
@@ -427,6 +433,31 @@ Drupal.ajax.prototype.commands = {
+        html += '<script type="text/javascript" src="' + response.files[i] + '"></script>';
+      }
+    }
+    if (html) {
+      $('html').prepend(html);
+    }

Is there a reason we are not using jQuery.getScript? (http://api.jquery.com/jQuery.getScript/)

The big addition I would like to see added to this is CSS support. JS and CSS so often go together, especially in Drupal. One without the other will be annoying and feel half implemented.

I really do like the direction of NOT storing the JS and CSS files in the cache. There is potential for a lot of problems going that route.

40 critical left. Go review some!

mfer’s picture

+++ misc/ajax.js
@@ -427,6 +433,31 @@ Drupal.ajax.prototype.commands = {
+    var html = '';
+    for (var i in response.files) {
+      if (!Drupal.ajaxFiles.scripts[response.files[i]]) {
+        Drupal.ajaxFiles.scripts[response.files[i]] = response.files[i];
+        html += '<script type="text/javascript" src="' + response.files[i] + '"></script>';
+      }
+    }
+    if (html) {
+      $('html').prepend(html);
+    }

If we do go with out own loader I wonder if we should do something similar to what jQuery is doing internally? Something like:

var head = document.getElementsByTagName("head")[0] || document.documentElement;
for (var i in response.files) {
  if (!Drupal.ajaxFiles.scripts[response.files[i]]) {
    Drupal.ajaxFiles.scripts[response.files[i]] = response.files[i];
    var script = document.createElement("script");
    script.src = response.files[i];
    head.insertBefore( script, head.firstChild );
  }
}

Three big differences here.

  1. Each script is added individually rather than as a group.
  2. The browser creates the script element rather than us assuming what should be in it. For example, type="text/javascript" is not needed for html5. That is needed under xhtml and, also, works under html5. Should we assume what the script tag should look like?
  3. This inserts it in head rather than html. head is where jQuery inserts scripts.

FYI, insertBefore is what jQuery.prepend uses.

40 critical left. Go review some!

merlinofchaos’s picture

The reason the CSS got removed is that the method I used in CTools to detect the existing scripts changed; I couldn't quickly figure out how to detect the use of @import scripts and manage them, and I'm still not sure how to do it. I agree that the CSS management is important and we really need that too.

The only reason I didn't use getScript() is that I did not know it existed. Using a jQuery function for this makes total sense, and is probably more robust than our own version. That's an easy change.

Owen Barton’s picture

Issue tags: +frontend performance

In terms of the patches script tag "document.write" approach v.s. the jQuery DOM element approach http://www.stevesouders.com/blog/2009/04/27/loading-scripts-without-bloc... is a good reference. Given that we are only doing this in response to AJAX events right now (rather than as part of an effort to improve the initial page display performance) I don't think that parallel loading is too critical, so the script tag approach looks like a reasonable and safe option.

katbailey’s picture

Here's a reroll taking most of mfer's comments in #81 into account, with the exception of the $.getScript idea because for some reason I couldn't get this to work :-( I'll try again tomorrow if I have time.

fago’s picture

I gave #85 a short test with the rules UI which needs js lazy-loading several times - works fine!

rfay’s picture

I also took #85 for a spin using the test module in #74, and it worked great.

katbailey’s picture

It doesn't look like $.getScript will work for us here. Testing this with merlinofchaos's patch to ajax_example module in #68, what happens is when the textarea is delivered via ajax it is NOT resizable, however further clicks on the ajax link deliver new textareas which ARE resizable. So $.getScript is adding the script to the page, but by the time Drupal.attachBehaviors is called on the new textarea, Drupal.behaviors.textarea has not yet been registered, though for further new ajax content it is there.

I can't say much more beyond that, but I don't think there'd be a huge advantage to using $.getScript here even if it did work so I say we just leave as is.

mfer’s picture

@katbailey ah yes. $.getScript will not work as a straight drop in. Not all the JS happens in series. So, which $.getScript is executing the lazy loading JavaScript continues on. There would need to be some structure changes to handle this.

rfay’s picture

OK, all of you (us) who are convinced this is critical for D7, pony up. Is this good enough as it is?

We could make it better by

  • Using $.getScript
  • Handling CSS the same way

Or we could just say "this is going to work for D7, and it solves the big criticalness of this issue".

Please say what you think, RTBC it if you think it is. Let's not let it sit too long.

merlinofchaos’s picture

Using $.getScript

Let's pass on this based on katbailey's experiments.

Handling CSS the same way

I agree but CSS does not need to hold this up. We can do CSS with the same technique, except for the part where, as far as I can tell, @import directives cannot be detected so we will need to always make a list, aggregated or not. Which is no big deal.

effulgentsia’s picture

Thanks so much, merlinofchaos, and everyone else who breathed new life into this issue since I abandoned it. I plan on reviewing it Wednesday, and unless someone else does it first, adding the CSS portion to it as well. If #85 or a later patch lands before then, I'm happy to do the CSS portion as a follow-up.

mfer’s picture

@merlinofchaos Have you looked into recording the CSS paths in _drupal_build_css_path?

ksenzee’s picture

Priority: Critical » Major

Changing to the new major priority as an excuse to subscribe.

chweetraju’s picture

effulgentsia’s picture

Re #92: Sigh. What is that expression about the best laid plans? I haven't been able to carve out enough core time lately. I'll keep trying though. Once I'm able to, this is my highest priority issue. No need to wait for me though: if people feel good about #85, keep pushing it through the process.

BenK’s picture

Need to track this...

dawehner’s picture

Rerole the patch

This patch fixes a bug for contrib module which uses a tabledrag form on a page loaded with the ajax framework.

webchick’s picture

Subscribing as an excuse to subscribe. ;)

yched’s picture

While ajax folks (and webchick) are subscribed here, shameless bump for #818660: Prevent duplicate code by adding an AJAX command to invoke simple jQuery methods

rfay’s picture

This one should go forward without CSS unless somebody is ready to come in and save the day on that.

Anybody ready to RTBC?

If it's RTBC, we need an issue summary and a restatement of why this is so critical for D7.

katbailey’s picture

Title: Lazy-load JavaScript and CSS via AJAX framework » Lazy-load JavaScript via AJAX framework

To summarise...

Without this patch, any ajax-loaded content that has some JavaScript behaviour will not work unless the required JavaScript files have already been loaded on the page before the ajax request was made. A couple of examples:
- An ajax-enabled form loads a textarea dynamically upon click of a button (there were no textareas on the form when it was first loaded). The textarea will not be resizable because textarea.js was not part of the original page load.
- A draggable table gets loaded dynamically into a div when you click on a particular link. Except it's not draggable, because tabledrag.js was not part of the original page load.

In order to get around the above problems currently, you'd have to anticipate all content that could possibly get loaded dynamically into the current page, find out what JavaScript files are needed for all that content, and load every one of them onto the page. This is both bad for performance and messy.

rfay’s picture

To add to katbailey's issue summary in #102, some of the simple real-world examples where we completely #fail without this patch:

1. You add #states to a form element when changing the element in an AJAX form. It won't work.
2. You do anything else when processing an AJAX form that changes the javascript that's required to make the form work. #fail.

effulgentsia’s picture

#102 and #103 are excellent summaries. Another way to think of it is quite simply that drupal_add_js() is currently broken. The developer expectation is that if you call drupal_add_js(), then your javascript file will get loaded. But that does not happen for AJAX responses, for no good reason, other than we simply haven't gotten around to fixing it. This issue finally fixes it. Not critical only from the perspective that D6 AHAH has the same thing broken. But, IMO, critical by any other standard, at least to the extent that we consider AJAX important, and I think we do. But leaving the priority as "major", since we're (understandably) adopting an increasingly strict standard of what makes something critical.

Overall, I think the #98 patch is great (thanks merlinofchaos and others!). Here's a relatively minor cleanup (sadly, I guess that means I'm no longer eligible to RTBC it).

As noted in earlier comments (and evident in the issue title prior to #102), drupal_add_css() is similarly broken, but given we're already past 100 comments on this issue, I agree that we should fix the (lack of) CSS loading in a separate issue.

sun’s picture

FileSize
18.45 KB

Thanks! Tweaked code comments + aggregate files handling.

Added a relatively major @todo to Drupal.ajax.commands.scripts, which we need to resolve.

Speaking of todos, I'd highly prefer to resolve CSS within this issue and patch. That is, because I suspect we could slightly change the added functions to support both JS and CSS at once, and we need to do it either way, so deferring to a separate issue just takes more time.

sun’s picture

FileSize
17.46 KB

For full BC, the settings command needs to be responded first.

rfay’s picture

Marked #885532: Collapsible behaviour does not work on fieldset updated via AJAX as a duplicate. Yet another example of why we have to have this in.

rfay’s picture

Status: Needs review » Needs work
FileSize
1.45 KB

I tested #106 and it doesn't actually make the test module in #74 (or here) work, which earlier patches did. #104 works, #106 doesn't.

Attached is a (lazy_load_demo_108.tar.gz) re-roll of the test module from #74.

It shows the lazy load behavior with #states and also a custom bit of js. It's crude, but demonstrates the issue here quite well.

webchick’s picture

I'm confused in this issue why we're passing around tarballed modules and not expanding test coverage?

rfay’s picture

@webchick, this is in the class of "can't test it yet" due to the fact that it's all javascripty. At least I don't know how to write a test for it. Open to suggestions!

webchick’s picture

Oh, right. :\ Sorry, carry on!

merlinofchaos’s picture

Status: Needs work » Needs review
FileSize
18.34 KB

Here is a straight reroll of #106 that works. Attaching for testbot review.

There are two problems:

1) The CSS *is* necessary. I'm going to work on that today. We can do this by *always* listing our CSS files in the closure if javascript is in use, regardless of aggregation and ignoring theme javascript because we can't safely lazy load those. (Themes can change during AJAX operations).

2) Knowledge gleaned from doing this in CTools has taught me that we have issues with the query string. Scenario: You load a page. Some indeterminate amount of time later, (could be 1 second, could be 5 days) you click an AJAX link on that page and it performs an AJAX operation. You have a list of javascript files, but all our javascript includes 'query_string' which we use to keep browsers from caching files if we think they've changed. If that query string has changed, you reload files that you've already loaded. Bad things happen. It doesn't matter if that file has changed or not.

I'm going to work on both things here and try to get this in. I will say that in several weeks of using this in the wild in CTools, it is a fantastic thing to have.

rfay’s picture

Thanks, merlinofchaos. I confirm that #112 works great.

merlinofchaos’s picture

OK here is a reroll that takes into account query strings!

1) I've moved the reporting of javascript files into the footer rather than the header so that we can be sure it happens after all basic javascript has run.
2) I've modified ajax_get_js() to be able to return either a command syntax OR a list of files depending upon the need. Because of this, I needed to change how it fetches the actual javascript files so that drupal_alter() changes would stick (otherwise we run the risk that drupal_alter would get called during drupal_get_js() and we would not reflect those changes during a later call to ajax_get_js()).
3) I've added ajax_get_css() similar to ajax_get_js().
4) I've added a css_files command similar to scripts. NOTE: It is called 'css_files' and not css because in CTools I have a 'css' command that purely adds inline CSS. I would like to keep that command named simply 'css'.
5) I've modified the lazy load module just slightly. It now #attaches its CSS according to the checkbox, which means that Randy's original intent, that the checkbox change the color of the textfield consistently with each click now works, because the CSS file is removed when the checkbox is clicked off. THis actually works.

Caveats:
1) My Drupal.getPath() function is pretty lame. Could probably be abstracted into something more useful but that's out of scope for this.
2) ONE thing I did not do is to respect the 'browsers' setting. That probably needs to be added but I'm not familiar enough with how this works to take it on. It should be relatively easy I would think. It's also entirely possible that non-theme CSS files will never care about browser.

Status: Needs review » Needs work

The last submitted patch, 561858-ajax-lazy-load.patch, failed testing.

effulgentsia’s picture

Title: Lazy-load JavaScript via AJAX framework » Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework

Really great to see this progress! I looked it over a bit and would like to spend more time with it before posting a more in-depth review and/or another patch. But I ran out of time today. So here's a quick hit (merlinofchaos and I discussed this in IRC):

  • I wonder about removing the query strings. I agree with having ajax.js be insensitive to query string for indexing what's already on the page, and sun added a @todo in #105 to that effect that's still in #114. But if ajax.js needs to load the file, then I think it should load it with the query string, since otherwise, I imagine at least on some browsers, a stale browser cached file will be used inappropriately.
  • I also wonder about the IE-workaround of each AJAX iteration removing the prior one's CSS files. While a lot of in-the-wild ctools experience is use-cases where this is ok, because it's things like modal dialogs where each AJAX callback replaces the prior one's content, I'm also familiar with use-cases like flexifield, where each AJAX callback adds new content without removing prior content, and each step can involve new CSS/JS files for the current step only. Perhaps we can use the 31 @import statements per STYLE tag technique instead and have the first AJAX callback create a STYLE tag, and then have successive ones add @import statements to this one tag, and make a new tag after the 31st file? Basically, the same thing we already do in D7 HEAD for normal pages when aggregation is disabled.
Owen Barton’s picture

Someone let me know about the diffable project today, and while it is solving a somewhat different problem (patching cached aggregate files to avoid redownload) than we have here I think there are some ideas that may be useful here. Essentially it has a JS loader script that requests the contents of files (via http-request, not a script tag), and then evals them to bring them into life. On subsequent requests it then requests the same file but without the querystring (i.e. the same technique discussed above) to force the browser to return whatever it has cached and then, if needed, downloads and applies a patch to that code before evaling it. Not suggesting a major rewrite at this stage, but it might help us thing about some of the versioning issues.

fago’s picture

Priority: Major » Critical

>Changing to the new major priority as an excuse to subscribe.

!? This is definitely critical - without that the ajax system cannot be properly used.

Nick_vh’s picture

Being new to the issue and being at Drupalcon I'm a bit confused about the steps we should be taking now.

I think the patch of merlinofchaos is a good approach but it still fails testing so I suppose we continue to fix this and are then about ready to get some reviews?

On the other hand, the comment of Owen about diffable might be a good approach to have as a contributed module. Actually most of these can be handled in a contributed module. But for now, I'd go for the approach we have in this thread and go more narrow towards a solution so we can get drupal 7 out!

I'd like to ask somebody with a bit more experience to give his/her opinion about what to do now and I'm more then happy to help out. In the meantime I'll start debugging the patch of merlin and see if I can get it run without testing failures.

rfay’s picture

@Nick_vh - The test fails look to me like they shouldn't be hard to fix. I hope to work on that at the sprint on Friday - happy to look at it with you.
-Randy

rfay’s picture

Status: Needs work » Needs review

#114: 561858-ajax-lazy-load.patch queued for re-testing.

rfay’s picture

FileSize
10.06 KB

Initiated a re-test of #114 because I'm not seeing all the #fail on my local.

Attaching merlinofchaos's test module from 114 again, because it wasn't gzipped and had a typo in the name and caused some people to stumble.

Status: Needs review » Needs work

The last submitted patch, 561858-ajax-lazy-load.patch, failed testing.

rfay’s picture

At the sprint I'm working on fixing the tests, and making progress on that.

Nick_vh’s picture

Drupalcon codesprint efforts helped to make this patch for correcting the simpletests. Added was a function that verifies key/values from the commands array or any array available.
Please review this one and let's get it RTBC!!

Thanks also goes to rfay for aiding me with this

Nick_vh’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal.ajax_lazy_load_561858_125.patch, failed testing.

Nick_vh’s picture

Just posting a todo for anybody that is interested

The remaining test errors are quite easy to handle. I forgot to test the whole system so I only fixed the AJAX errors. There is a utility function in ajax.test that allows you to handle/solve 90% of the errors

I have a little shortage on time now so if you want to move this forward, please help out!

effulgentsia’s picture

Assigned: Unassigned » effulgentsia

I'm putting some more hours into this today.

effulgentsia’s picture

Assigned: effulgentsia » Unassigned
Status: Needs work » Needs review
FileSize
12.01 KB

Because of my comments in #116, I'm trying a new approach here. It goes back to my page state idea from #63, but without the complication of introducing a page build id, and also taking into account some of the details that have been figured out on this issue since then. Let's see what bot thinks.

Status: Needs review » Needs work

The last submitted patch, drupal.ajax_lazy_load_561858_130.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
12.18 KB

Minor fix.

Nick_vh’s picture

effulgentsia can you confirm the module uploaded above still works?
I don't really understand what you did but did you somehow change the order of the variables in the $settings var?

I would be very happy if you could explain me what you've done :-)

effulgentsia’s picture

Hi Nick. There's a few differences between #125 and #132:

  • #132 does more of the heavy lifting on the server-sde. Whereas the logic in #125 is to have the server-side code send back 'scripts' and 'css_files' commands containing all the files that are needed, and to have ajax.js then figure out what's new in order to just load that, the logic in #132 is to have ajax.js tell the server-side code what it already has when it issues the AJAX submission, and for the server-side code to then determine what's new and only send that back.
  • What the above allows is for the PHP function ajax_render() to reuse the existing drupal_get_css() and drupal_get_js() functions, rather than needing to introduce ajax variants of these functions on the server-side PLUS ajax.js code to generate SCRIPT, LINK, and STYLE tags. Instead, #132 reuses the existing Drupal functions for generating these tags, with all of their intricacies like aggregation, working around the IE 31 tag limit, dealing with the media attribute and browser-conditional comments, etc. So, #132 doesn't add new ajax commands at all: it just re-uses the existing 'prepend' and 'append' commands. The result is identical, in that #125's 'scripts' and 'css_files' commands ended up getting implemented by ajax.js as 'prepend' and 'append' commands anyway.
  • #125 introduced some divergence in the URLs output for a particular resource relative to the logic in drupal_get_*(). I commented on this in #116.1. By reusing the drupal_get_*() functions, #132 removes that (IMO, unnecessary) divergence, while retaining the desired behavior of deciding what's new vs. what's already existing on the page.
  • #125 introduced an interesting workaround to the IE 31 tag limit, by having each AJAX response remove the CSS files that were added by the previous one, instead of letting the number of CSS tags pile up past IE's limit. But as I commented in #116.2, while this approach makes sense for modal wizards, it's not what's desired for other AJAX use-cases. #132 doesn't do this, because by resusing the drupal_get_*() functions, it benefits from aggregation and tag consolidation implemented by those functions, making it much less likely to encroach on IE's limit.
  • #125 implemented some logic to prevent theme css files from being returned in an AJAX response to get around the issues that could come up if the AJAX request is rendered with a different theme than the base page (common, for example, if system/ajax uses the default theme, but the base page uses the admin theme), since then the page would be simultaneously applying rules from 2 different themes. #132 implements a different solution to this problem, by making AJAX responses render with the same theme as the base page.
  • #125 implemented some changes to the way the drupal_get_*() functions invoke drupal_alter() to prevent undesired multiple alters. #132 adds a parameter to these functions to achieve the same thing, but with less heavy-handedness inside the drupal_get_*() functions, and more room for control by the calling functions.

In summary, the end results are very similar. The #122 use-case works with both. But I think #132 addresses my concerns from #116, without losing the important things that were figured out in getting to #125.

Note 1: in theory, it would be good to add tests to #132, but I'd really rather have #789186: Improve drupalPostAJAX() to be used more easily, leading to better AJAX test coverage land first, to re-use its better code structure for AJAX testing. I don't think that issue is necessarily a blocker for this one, but I do think it's a blocker for adding tests to this one.

Note 2: neither #125 nor #132 address a couple edge-case scenarios where the returned CSS needs to be interleaved in the correct order relative to what's already on the page. But there's work being done in #769226: Optimize JS/CSS aggregation for front-end performance and DX to make CSS grouping and ordering more sane and efficient, and I think once that's in place, then these edge-case scenarios can be addressed in follow-up issues, or even in contrib, if we miss the window in which to do them in D7 core.

sun’s picture

#132 works excellently. I did some minor tweaks to the phpDoc in attached patch.

After this patch and #647228: Links are needlessly unable to fully participate in D7 AJAX framework features landed, we will have to revisit Drupal.ajax.prototype.beforeSubmit(), because we currently do not POST any HTML ID and ajaxPageState data for links (and other non-form AJAX requests).

I'd recommend to commit this patch as is. Unless this is in, we won't see much manual testing.

Status: Needs review » Needs work

The last submitted patch, drupal.ajax-lazy-load.135.patch, failed testing.

mparker17’s picture

Subscribe

bojanz’s picture

Status: Needs work » Needs review
Issue tags: -frontend performance

#135: drupal.ajax-lazy-load.135.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +frontend performance

The last submitted patch, drupal.ajax-lazy-load.135.patch, failed testing.

rfay’s picture

Status: Needs work » Needs review
FileSize
29.27 KB

This patch does a couple of things:

1. @nick_vh's test improvements from #125 (making the ajax tests less fragile) got lost somewhere, and that was some of the breakage here.

2. There was a missing "else if":

@@ -28,7 +28,7 @@ class AJAXTestCase extends DrupalWebTestCase {
     if ($commands[0]['command'] == 'settings') {
       array_shift($commands);
     }
-    else if ($commands[0]['command'] == 'scripts') {
+    if ($commands[0]['command'] == 'scripts') {

3. The change to ajax_render in #135 didn't actually work, so I put it back mostly the way it was in #132.

Status: Needs review » Needs work

The last submitted patch, drupal.ajax_lazy_load_561858_140.patch, failed testing.

rfay’s picture

Status: Needs work » Needs review
FileSize
29.14 KB

Two more to go! Maybe this one will go green.
I did two things here:

1. There was an invalid test (confirmed with @effulgentsia) in testAJAXRender(); it was looking for a scripts command but there is none any more.

@@ -84,11 +88,6 @@ class AJAXFrameworkTestCase extends AJAXTestCase {
     $result = $this->findJSONKey(array('settings' => array('basePath' => base_path(), 'ajax' => 'test')), $commands);
     $this->assertTrue($result, t('The %setting setting is included.', array('%setting' => 'basePath')));
 
-    // Verify that there is a command to load script files added with
-    // drupal_add_js().
-    $result = $this->findJSONKey(array('command' => 'scripts'), $commands);
-    $this->assertTrue($result, t('ajax_render() added a scripts command to load files added with drupal_add_js().'));

2. Added a fairly hacky removal of the unpredictable 'ajaxPageState' key from the settings in findJSONKey(), which again is just a test utility used in this class.

@@ -44,6 +44,10 @@ class AJAXTestCase extends DrupalWebTestCase {
   function findJSONKey($conditions, $json) {
     $found = FALSE;
     foreach ($json as $item) {
+      // Can't anticipate ajaxPageState.
+      if (isset($item['settings']['ajaxPageState'])) {
+        unset($item['settings']['ajaxPageState']);
+      }

Nick_vh’s picture

Perfect! A green one!

The changes of effulgentsia went way above my head but I'm happy to see this working.
Is there still something trivial I can do for this to become RTBC?

It looks like we finally made it to a stable version of this patch and it should be ready to go in no?

webchick’s picture

Status: Needs review » Needs work

#616240: Make Field UI screens extensible from contrib - part II was just committed (sorry, it was ready first) which includes some @todo code in template_preprocess_field_ui_table() which this patch will need to take into consideration.

duellj’s picture

Status: Needs work » Needs review

I've tested the patch in #142 against several scenarios (inspired by merlinofchaos in #50) when the js/css hasn't already been loaded:

1) Adding AJAX farbtastic elements (via ['#attached']['library']): WORKS

2) Adding AJAX autocomplete elements: WORKS

3) Adding AJAX collapsible fieldset elements: WORKS

Not sure if it's a views-only problem, but the previous scenarios fail when inside a view style options_form. To replicate create a collapsible fieldset within a view style options_form and watch it fail (fieldset doesn't collapse, loads JSON to screen when saving form). Like I said, though, might be that views needs to update to reflect the changes in the patch. Any views developers want to comment on this?

duellj’s picture

Status: Needs review » Needs work

Cross post, sorry.

rfay’s picture

Status: Needs work » Needs review

@webchick: I'd be happy to roll a patch including the @todos from #616240: Make Field UI screens extensible from contrib - part II, but I'd rather do it as a followup in that issue. Seems that it would keep things cleaner. Setting this back to needs review, but will be happy to roll a patch including it here if that's what you'd prefer.

All: Here's the todo webchick referred to:

@@ -115,12 +154,35 @@ function template_preprocess_field_ui_table(&$variables) {
           $cell = current(element_children($row));
           $row[$cell]['#prefix'] = theme('indentation', array('size' => $depth)) . (isset($row[$cell]['#prefix']) ? $row[$cell]['#prefix'] : '');
         }
...
+  drupal_add_js(array('fieldUIRowsData' => $js_settings), 'setting');
+  // @todo : use #attached instead when http://drupal.org/node/561858 is fixed.
+//  $elements['#attached']['js'][] = array(
+//    'type' => 'setting',
+//    'data' => array('fieldRowsData' => $js_settings),
+//  );
+
+  return $elements;
 }
rfay’s picture

Reviewer's Guide:

The easiest way to try this out is by adding CSS or JS in a #attached. , or by adding on #states in a form in AJAX.

There is a module that does just this in #122 (#561858-122: [Tests added] Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework). You can experiment with it to test.

There is an issue summary in #102-#104, but it doesn't talk implementation details. #134 goes into some implementation details. @sun, @effulgentsia, if you could address that I think it will help us to get this committed.

If you think this is RTBC, set it RTBC. We probably need thumbs-up from @effulgentsia, @merlinofchaos, and @sun.

merlinofchaos’s picture

I haven't had a chance lately to get to checking out effulgentsia's different approach on this issue.

My general feeling is that "Hey if it works in all the demo cases then I'm for it," because while I'm fond of the approach that I've been using in the wild with CTools, what matters to me is that we get a working solution. IMO if this passes some testing (and we know automated testing doesn't help with this, so it has to be manual testing with demo modules which should later be committed to the example modules) then I say +1.

Nick_vh’s picture

Status: Needs review » Reviewed & tested by the community

+1 seems to be working well, and i second the opinion of merlinofchaos.

marking as RTBC, it is about time!

Nick_vh’s picture

Status: Reviewed & tested by the community » Needs review

After some considerations I'm reverting this back to needs review because, as mentioned earlier, the approval of sun & effulgentsia is critical.

merlinofchaos’s picture

Well, Nick, this patch is effulgentsia's approach. I think we can safely assume approval.

This quote from sun:

I'd recommend to commit this patch as is. Unless this is in, we won't see much manual testing.

Sounds like approval to me. Admittedly it's 2 tweaks ago, but the tweaks since have just been test fixes.

sun’s picture

Patch went stale. Re-rolled against HEAD. Will review now.

effulgentsia’s picture

I haven't read latest, but I trust sun's review and assessment of tweaks since my last post, so feel free to RTBC without me. I'm away this weekend, but I'll check in on this on Monday.

sun’s picture

Odd. While that patch passed tests, I'm not able to make it work with http://drupalcode.org/viewvc/drupal/contributions/sandbox/sun/core_tabs/ - any clues?

rfay’s picture

Status: Needs review » Needs work

Shucks, testing with the test module in #122 #fails.

sun’s picture

Alright. rfay (+ me) confirms that also his testing module in #122 is broken. Not sure what changed in the meantime. To my knowledge, the only AJAX-related patch seems to have been #850612: setting class 'use-ajax-submit' for form submission causes js error(s)

rfay’s picture

sun’s picture

We figured out that #756762: AJAX should follow same rules for whether to call drupal_rebuild_form() as non-AJAX submissions is actually guilty. Also, #647228: Links are needlessly unable to fully participate in D7 AJAX framework features needs to be changed to account for ajax_page_state.

OK, I've really tried hard to make any sense of the changed ajax.test. The improvements made in there are basically irrelevant for this patch, so I'm not sure why that has been done. Anyway, I'd agree that the new assertions are more solid, but they still need work. Apparently, testAjaxRender() will fail when doing a type-agnostic comparison, and that is, because the AJAX commands are communicated via $_GET to the page callback, but $_GET does not understand Booleans, so a value of 'merge' => FALSE turns into 'merge' => ''. Needs work.

Nick_vh’s picture

@sun the improvements on the test are relevant enough because without these changes the patch of merlinofchaos would never be succeeded by the test bots (see #125 and continued)

This is because the old test was based on a static place in the array while this can be variable and it should exist just somewhere in the array. So to my humble opinion they are valid for this patch? If you prefer to put them in a separate thread to make this core patch more light I'm very open to do that if that could progress the release of drupal 7?

On the other hand, I checked the function testAJAXRender() and I don't see any type-agnostic comparison? I will test this when I have time and as far as I understand, an empty value should reflect in a 'FALSE' attribute then?

rfay’s picture

Status: Needs work » Needs review

Setting to needs review to test #159. I think that #153 was close to committable. The test finesse looks OK to me. The stuff Nick re-did was because the previous version had horrendously fragile static array references. Improvements are good.

I just took #159 for a spin and it seems to be OK to me. So nice that #756762: AJAX should follow same rules for whether to call drupal_rebuild_form() as non-AJAX submissions got committed so things work again.

Status: Needs review » Needs work

The last submitted patch, drupal.ajax-lazy-load.158.patch, failed testing.

katbailey’s picture

I had promised to have a look at this days ago in relation to the #647228: Links are needlessly unable to fully participate in D7 AJAX framework features patch, but I've been snowed under lately. However I did just look through this briefly to get an understanding of how the new approach works and I am dismayed at how form-centric it is :-(
I wish I could say more than that, like come up with a solution, but for now all I can say is that this really needs to work for ajax links as well. The whole raison d'etre of the ajaxifiable link patch was to allow links to get in on the ajax_render action, but now ajax_render is going to ignore them anyway, if I'm understanding it correctly, because they don't have $_POST going on. Sad panda is sad.

effulgentsia’s picture

I'm about to try to see if I can make #159 pass.

Re #163: See #135. Our intention is to make this work for #647228: Links are needlessly unable to fully participate in D7 AJAX framework features too. If this one lands first, then we'll incorporate that into the other issue. If the other issue lands first, then we'll need to re-roll this one to take it into account. But I think we need to keep the two parallel for now, rather than bogging either one down with stuff that isn't officially part of Drupal yet.

effulgentsia’s picture

Status: Needs work » Needs review
Issue tags: -frontend performance

#159: drupal.ajax-lazy-load.158.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +frontend performance

The last submitted patch, drupal.ajax-lazy-load.158.patch, failed testing.

katbailey’s picture

@effulgentsia - thanks for the explanation, I had misunderstood where things were at. It doesn't seem like there's much point in trying to 'fix' the ajaxifiable link patch until this lands then.

Nick_vh’s picture

A throw at fixing some of the tests.
The checking of the data in the simpletest algorithm needed some work.

I just can't figure out why the multiform doesn't work!

Nick_vh’s picture

Sorry, wrong patch format. Correct one now

Nick_vh’s picture

Status: Needs work » Needs review

Review bot, review!

Anonymous’s picture

subscribe. helped Nick_vh out in #drupal. the fails come from using the testing profile, which doesn't have some of the requisite modules and setup.

simplest fix is to use the standard profile, right fix is to enable necessary modules and possibly some other content-type setup to make the test work but still be fast.

Nick_vh’s picture

And here we are.. With results from the whole AJAX Suite working

Only issue left is that I had to enable the standard profile for testing instead of the testing profile.
With some help of beejeebus we nailed it down to that particular case and if you'd like to test it with testing profile and fix that 1 little bug left in there please do!

The bug is in the multiwebform test case. Somehow we have permission denied if we are in the testing install profile.

I think we should see a green one here!

effulgentsia’s picture

I just spent the last couple hours working on this without knowing Nick was too. I guess I should have assigned the issue to myself in #164. Anyway, here's a cleanup of ajax.test that I came up with. Not sure how we want to proceed with reconciling this and #172.

rfay’s picture

Using the simple module in #122, #172 and #173 both work (javascript is loaded to change the color of the upper textfield; #states is successfully activated by #ajax). There are no differences between them except in the approach to the tests.

effulgentsia’s picture

I just want to comment on something here before it becomes an issue. Today, webchick posted an excellent commentary on what is and isn't critical at this point in the release process: #927792-13: Please explain what is still considered for D7, and getting community clarity and alignment with it is very important. In it, she says Awful problems that also existed in previous releases. If they were not bad enough to block the release of the previous version of Drupal, they will not block the next release, either. are not to be considered critical any more. And to some extent I agree with this, which is why I downgraded an issue I care very much about, #823428: Impossible to alter node URLs efficiently, from critical to major.

But we still have critical issues around fixing awful problems in overlay.module, even though lack of a functioning overlay.module didn't stop D6 from being released. And I'm not just picking on Overlay here. We have criticals on dashboard module, field ui module, and other parts of Drupal that we didn't have in D6. Well, I'd like to make the case that the D7 AJAX framework is effectively new. Yeah, it's somewhat based on D6 AHAH, but it's hardly recognizable as such any more. And we have all of the major AJAX framework developers, including merlinofchaos, sun, rfay, me, and many others, who have already unanimously argued in this issue's comments for why this issue is critical, because without it, the AJAX framework is severely crippled. I hope that counts for something. End of rant :)

Nick_vh’s picture

I vote for the patch of effulgentsia (#173)

It's a really good cleanup of what I've started and I'm all fine with commiting that one to core even though mine was also perfectly valid. After comparing the 2 I've seen no critical changes, only the way on how to control the checks changed.

Also the multiform was handled by removing the protected profile as far as I understand so the basic profile is in use? If effulgentsia could elaborate on just this I would be happy because I do think that we can ship this test with a testing profile?

+1 for #173

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community
It's a really good cleanup of what I've started and I'm all fine with commiting that one to core even though mine was also perfectly valid. After comparing the 2 I've seen no critical changes, only the way on how to control the checks changed.

Thanks, Nick. Very gracious of you. To be honest, I haven't had time to read #172, but if you think there's good stuff in there that we'll want, please feel free to post a follow-up after this one lands. No sense in throwing away useful work. And if all it touches is tests and makes them better, then that's still eligible for D7.

Also the multiform was handled by removing the protected profile as far as I understand so the basic profile is in use? If effulgentsia could elaborate on just this I would be happy because I do think that we can ship this test with a testing profile?

To be honest, I didn't even know we could have a protected $profile variable in test classes. Sounds intriguing. But let's deal with that in a follow-up. #173 makes no changes with respect to HEAD related to this, so no need to have it delay this issue.

+1 for #173

Thanks. In that case, RTBC. This has been approved by sun (in actual review, #135), by merlinofchaos (in principle, #149), by rfay (in manual testing, #174), and by Nick and me. I think that qualifies for RTBC.

It may not be perfect, it may have bugs, but I echo merlinofchaos's and sun's sentiments that it's time for the broader community to start testing this in the wild, and that can only happen if it's committed. No better time than for beta.

rfay’s picture

Agreed. Time for this one to go in.

Dries’s picture

+++ includes/ajax.inc	1 Oct 2010 00:17:25 -0000
@@ -213,11 +213,66 @@
+  // Render the HTML to load these files, and add AJAX commands to insert this
+  // HTML in the page. We pass TRUE as second argument to prevent the data from
+  // being altered again, as we already altered it above.
+  $styles = drupal_get_css($items['css'], TRUE);
+  $scripts_footer = drupal_get_js('footer', $items['js'], TRUE);
+  $scripts_header = drupal_get_js('header', $items['js'], TRUE);

TRUE is the third argument.

Personally, I think the $skip_alter parameter is a bit of an ugly hack. Can't immediately see a way around it though.

+++ includes/ajax.inc	1 Oct 2010 00:17:25 -0000
@@ -307,6 +362,31 @@ function ajax_form_callback() {
+ * Many different pages can invoke an AJAX request to system/ajax or another
+ * generic AJAX path. It is almost always desired for an AJAX response to be
+ * rendered using the same theme as the base page, and it is therefore
+ * recommended for all AJAX paths to list this function for its 'theme
+ * callback', as is done in system_menu() for the 'system/ajax' path.

It would be good to explain why that is preferred.

rfay’s picture

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

This patch changes #173 only in comments as requested by @Dries as follows:

 * Many different pages can invoke an AJAX request to system/ajax or another
 * generic AJAX path. It is almost always desired for an AJAX response to be
 * rendered using the same theme as the base page as different themes may have
 * different AJAX properties, so it is therefore recommended that all AJAX paths
 * list this function for its 'theme callback', as is done in system_menu() for
 * the 'system/ajax' path.

and

  // Render the HTML to load these files, and add AJAX commands to insert this
  // HTML in the page. We pass TRUE as the $skip_alter argument to prevent the
  // data from being altered again, as we already altered it above.
  $styles = drupal_get_css($items['css'], TRUE);
  $scripts_footer = drupal_get_js('footer', $items['js'], TRUE);
  $scripts_header = drupal_get_js('header', $items['js'], TRUE);

If it passes muster we should go right back to RTBC.

Status: Needs review » Needs work

The last submitted patch, drupal.ajax_lazy_load_561858_180.patch, failed testing.

rfay’s picture

Status: Needs work » Reviewed & tested by the community

Putting back to RTBC. The first bot stumbled for unknown reasons. I'll have it do a retest.

egorovanton’s picture

tstoeckler’s picture

..., so it is therefore recommended...
"so" and "therefore" are redundant. Rerolled with the following:
..., so it is recommended...

Also:
The answer may very well be no, but should the following array key not be 'css' instead of 'js' in drupal_get_css():
+ $styles['#attached']['js'][] = array('type' => 'setting', 'data' => $setting);

Otherwise the code looks very good. I read through the whole patch intesively (not including the tests, though), and I can say that the code is well documented, adheres to coding standards and makes sense to someone, who has no particular knowledge of AJAX or AJAX in Drupal. Hence, +1 on RTBC. Obviously, no comment on the actual implementation.

Status: Reviewed & tested by the community » Needs work
Issue tags: -frontend performance

The last submitted patch, drupal.ajax_lazy_load_561858_184.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
Issue tags: +frontend performance
effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Bot passes again. The docs improvements in #180 and #184 are good. The docs for ajax_base_page_theme() can be a little better still at explaining why it's usually desired for a given page to only have a single theme, and I'll work on that, but as I've said on other issues, minor docs tweaking can happen in a non-critical follow-up.

The answer may very well be no, but should the following array key not be 'css' instead of 'js' in drupal_get_css():

The answer is no, the patch is correct. We're adding a JavaScript variable to the output. The JavaScript variable contains information about CSS, but it's a JavaScript variable, so 'js' is the correct key.

effulgentsia’s picture

Just tweaked the docs for ajax_base_page_theme() as per #179, to:

/**
 * Theme callback for AJAX requests.
 *
 * Many different pages can invoke an AJAX request to system/ajax or another
 * generic AJAX path. It is almost always desired for an AJAX response to be
 * rendered using the same theme as the base page, because most themes are built
 * with the assumption that they control the entire page, so if the CSS for two
 * themes are both loaded for a given page, they may conflict with each other.
 * For example, Bartik is Drupal's default theme, and Seven is Drupal's default
 * administration theme. Depending on whether the "Use the administration theme
 * when editing or creating content" checkbox is checked, the node edit form may
 * be displayed in either theme, but the AJAX response to the Field module's
 * "Add another item" button should be rendered using the same theme as the rest
 * of the page. Therefore, system_menu() sets the 'theme callback' for
 * 'system/ajax' to this function, and it is recommended that modules
 * implementing other generic AJAX paths do the same.
 */

No code change, so leaving RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Alright. Thanks for making the documentation improvements. Committed to CVS HEAD! Yay.

merlinofchaos’s picture

Status: Fixed » Active

There is a problem with this patch: beforeSubmit is only used when $.ajaxSubmit() is called, but not when $.ajax is called. This is creating havoc with Panels/CTools because I use a lot of $.ajax and in that case, .js files are being reloaded.

At this time I'm not precisely sure what the best solution is, but we can't be using beforeSubmit to pass values that are not form-related.

sun’s picture

Status: Active » Fixed
effulgentsia’s picture

Priority: Critical » Normal

@merlinofchaos: being dealt with in #647228: Links are needlessly unable to fully participate in D7 AJAX framework features I think.

.js files are being reloaded

That's weird. ajax_render() is specifically checking for empty($_POST['ajax_page_state'][$type]) and shouldn't return ANY css or js in this case, though again, #647228: Links are needlessly unable to fully participate in D7 AJAX framework features is trying to improve on that.

effulgentsia’s picture

Priority: Normal » Critical

x-post.

merlinofchaos’s picture

That's weird. ajax_render() is specifically checking for empty($_POST['ajax_page_state'][$type]) and shouldn't return ANY css or js in this case,

Um. Why? That makes no sense. Why would the method (form or not form) make it arbitrarily decide it's ok to add js one way and not the other? That's totally broken for my uses.

sun’s picture

@merlinofchaos: Did you verify that http://drupal.org/node/647228 does not fix your issue/use-case? I'm pretty sure it should. If it really does not, then let's open a new issue and link to it here.

merlinofchaos’s picture

With that patch in place, I'm now getting duplicates of files on every ajax request. This is, in a way, an improvement, in that now it's broken all the time as opposed to just sometimes (i.e, when I submit a form) so it's a step in the right direction.

When playing with Panels I am seeing in firebug:

Firebug's log limit has been reached. 0 entries not shown.		Preferences	 
POST http://d7p3.dev.logrus.com/panels/ajax/editor/layout/panels_mini%3Atest/add/2
POST http://d7p3.dev.logrus.com/panels/ajax/editor/layout/panels_mini%3Atest/add/2
	
200 OK
		464ms	
jquery.js?v=1.4.2 (line 132)
GET http://d7p3.dev.logrus.com/misc/ui/jquery.ui.core.min.js?v=1.8
GET http://d7p3.dev.logrus.com/misc/ui/jquery.ui.core.min.js?v=1.8
	
200 OK
		175ms	
jquery.js?v=1.4.2 (line 132)
GET http://d7p3.dev.logrus.com/misc/jquery.ba-bbq.js?v=1.2.1
GET http://d7p3.dev.logrus.com/misc/jquery.ba-bbq.js?v=1.2.1
	
200 OK
		287ms	
jquery.js?v=1.4.2 (line 132)
GET http://d7p3.dev.logrus.com/modules/overlay/overlay-parent.js?v=1.0
GET http://d7p3.dev.logrus.com/modules/overlay/overlay-parent.js?v=1.0
	
200 OK
		34ms	
jquery.js?v=1.4.2 (line 132)
POST http://d7p3.dev.logrus.com/panels/ajax/editor/layout/panels_mini%3Atest/add/2
POST http://d7p3.dev.logrus.com/panels/ajax/editor/layout/panels_mini%3Atest/add/2
	
200 OK
		308ms	
jquery.js?v=1.4.2 (line 132)
GET http://d7p3.dev.logrus.com/misc/jquery.js?v=1.4.2
GET http://d7p3.dev.logrus.com/misc/jquery.js?v=1.4.2
	
200 OK
		88ms	
jquery.js?v=1.4.2 (line 132)
GET http://d7p3.dev.logrus.com/misc/jquery.once.js?v=1.2
GET http://d7p3.dev.logrus.com/misc/jquery.once.js?v=1.2
	
200 OK
		35ms	
jquery.js?v=1.4.2 (line 132)
GET http://d7p3.dev.logrus.com/misc/ui/jquery.ui.core.min.js?v=1.8
GET http://d7p3.dev.logrus.com/misc/ui/jquery.ui.core.min.js?v=1.8
	
200 OK
		34ms	
jquery.js?v=1.4.2 (line 132)
GET http://d7p3.dev.logrus.com/misc/drupal.js?l9u3i0
GET http://d7p3.dev.logrus.com/misc/drupal.js?l9u3i0
	
200 OK
		24ms	
jquery.js?v=1.4.2 (line 132)
GET http://d7p3.dev.logrus.com/misc/jquery.ba-bbq.js?v=1.2.1
GET http://d7p3.dev.logrus.com/misc/jquery.ba-bbq.js?v=1.2.1
	
200 OK
		38ms	
jquery.js?v=1.4.2 (line 132)
GET http://d7p3.dev.logrus.com/modules/overlay/overlay-parent.js?v=1.0
GET http://d7p3.dev.logrus.com/modules/overlay/overlay-parent.js?v=1.0
	
200 OK
		31ms	
jquery.js?v=1.4.2 (line 132)
GET http://d7p3.dev.logrus.com/sites/all/modules/panels/js/panels.js?l9u3i0
GET http://d7p3.dev.logrus.com/sites/all/modules/panels/js/panels.js?l9u3i0
	
200 OK
		24ms	
jquery.js?v=1.4.2 (line 132)
GET http://d7p3.dev.logrus.com/sites/all/modules/ctools/js/dropdown.js?l9u3i0
GET http://d7p3.dev.logrus.com/sites/all/modules/ctools/js/dropdown.js?l9u3i0
	
200 OK
		12ms	
jquery.js?v=1.4.2 (line 132)
Drupal.Panels.Draggable is undefined
[Break on this error] 

You can see that it's getting jquery.js which it absolutely should NOT be getting a second time.

merlinofchaos’s picture

Ok, I see. The problem is that on the 2nd and successive AJAX requests, the ajax_page_state has been blown away by previous responses. Perhaps this should be a followup.

merlinofchaos’s picture

Ok, the problem is here:

+  $styles = drupal_get_css($items['css'], TRUE);
+  $scripts_footer = drupal_get_js('footer', $items['js'], TRUE);
+  $scripts_header = drupal_get_js('header', $items['js'], TRUE);

Using drupal_get_js on $items like that causes the settings to be encoded in the $scripts_header *even though* they are being sent separately; when encoded into the header, they overwrite what was previously there, which is wrong.

So far it seems like the easy thing to do is this:

+ unset($items['js']['settings']);

There's another bug with the links patch which I'll post there.

At the moment I can't post a patch very easily, I've got 3 or 4 patches running and I'm in kind of a difficult state.

Anonymous’s picture

Status: Fixed » Active

just updating the status.

effulgentsia’s picture

Status: Active » Postponed

I think it makes sense to focus efforts on the points raised above by merlinofchaos in #647228: Links are needlessly unable to fully participate in D7 AJAX framework features, so postponing this issue for now. If for whatever reason that issue gets stalled, we can then set this back to active and just deal with the bugs like #198 that would need to be fixed regardless. But hopefully, we can get #647228: Links are needlessly unable to fully participate in D7 AJAX framework features resolved and all the above bugs fixed in the process.

effulgentsia’s picture

Status: Postponed » Needs review
FileSize
1.11 KB

Sorry for flip-flopping on this, but while I'm in total support of #647228: Links are needlessly unable to fully participate in D7 AJAX framework features, and believe that contains the right solution to #190, (and I look forward to some focus time soon so I can review that issue in depth), #198 is pretty bad, easily fixed, and ideally, should make it in before the beta release (I'm not giving it the beta blocker tag, as it shouldn't block beta, but would be much nicer to have it fixed for the release that we expect a lot of people to do more serious pounding on).

Note, this patch's code existed in #132 and #135, but I think mistakenly got dropped in subsequent patches.

Also note, I'm well aware that we need some test coverage of the functionality introduced by this issue, including a test for this small patch. I promise to work on that when I get a chance. But I won't get that chance before beta, and I'd really like to see this patch land before then. As said in earlier comments, any test coverage added for this won't be ideal, since this touches on JavaScript code that isn't testable via simpletest, but I think we can still get some test coverage in that's better than nothing.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! I agree that this minimal fix should go in before beta. I also promise to help effulgentsia with writing proper tests for this entire functionality.

Dries’s picture

+++ includes/ajax.inc	6 Oct 2010 14:46:16 -0000
@@ -246,6 +246,16 @@ function ajax_render($commands = array()
+  // Settings are handled separately, later in this function, so that changes to
+  // the ajaxPageState setting that occur during drupal_get_css() and
+  // drupal_get_js() get included, and because the jQuery.extend() code produced
+  // by drupal_get_js() for adding settings isn't appropriate during an AJAX
+  // response, because it does not pass TRUE for the "deep" parameter, and
+  // therefore, can clobber existing settings on the page.

Wow, that is a long sentence. It has 3 nested because's ... :)

That said, it seems to make sense. Will commit later.

merlinofchaos’s picture

Yes, this fix is working for me now, though the description I wrote of it was somewhat shorter. =)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Re-working that paragraph-masked-as-a-sentence can be the job of a non-critical follow-up patch, if we want.

yched’s picture

I cannot really assign blame on this very patch, but #939860: on 2nd ajax call, behaviors are called with empty settings describes a (critical) bug that was not present a couple weeks ago.
At least, all the right people are subscribed here, so just pinging :-).

effulgentsia’s picture

Status: Fixed » Needs review
FileSize
3.3 KB
2.83 KB

Apologies to katbailey, sun, and merlinofchaos. You all warned that #188 was too form-centric and didn't adequately handle the lazy load for AJAX triggered by links. But I wouldn't listen, because in #164, I thought #647228: Links are needlessly unable to fully participate in D7 AJAX framework features was a blocker to making the AJAX framework work at all for links. But when I recently reviewed that issue and discussed it with merlinofchaos in IRC, I realized that's not true. That issue continues to be useful to make customized #ajax settings available to links, and for that reason alone, deserves to still be "major", but because the AJAX framework in HEAD already supports links that trigger AJAX via a "use-ajax" class, getting lazy load to work in that context is "critical", and therefore, needs to be pulled out of that issue, and added as a follow-up here. Attached is the patch that does that. It's pretty simple really. Thanks go to sun for figuring out how to do it.

For Dries and webchick, who will naturally ask why is this follow-up critical: We have lots of contrib use-cases for AJAX triggered from links. Views and Panels do that throughout, hence, merlinofchaos immediately noticing the problem in #190. katbailey pointed this out in #163, because of her experience with Quick Tabs. On DrupalGardens, we also run into this, because we have Video Galleries, where the gallery page opens a modal dialog in a colorbox, and the AJAX-returned contents of the colorbox needs to play a YouTube video using JavaScript (for systems that don't have Flash, like iPads). All of these examples involve AJAX content returned from a link, and the AJAX content requires JavaScript code that isn't originally present on the page.

Finally, I'm attaching another example: an early prototype of an "Edit in place" module for fields. If you create an Article node, populate body and tags, and disable comments on it, then with this module, but without the attached lazy load patch, when you view the node, you get contextual links for each field to edit it in place. Clicking the link replaces the view of that field with its edit interface. I didn't implement a Submit button yet, so you can't finish the editing process, but just this much gives you an idea. Anyway, without the attached lazy load patch, all works, except that the JavaScript needed by the edit interface isn't available, so editing the Body field gives you the no-js version of that interface (separate Summary and Body fields, no resize control), and editing the Tags field gives you a textfield, but without the autocomplete working. However, with the lazy load patch, these become fully functional, because their JS dependencies get correctly loaded.

No tests included with this patch, because the only code being changed is ajax.js, and we don't have a test framework for testing JS code. That's why I'm including the "Edit in place" example module.

sun’s picture

Status: Needs review » Closed (fixed)

The patch is missing a corresponding change for 'theme_token' elsewhere. I also don't like the idea of ripping out partial changes from the other issue/patch. Let's mark the other issue critical instead and fix all remaining issues related to links over there.

effulgentsia’s picture

Title: Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework » [Tests added] Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework
Priority: Critical » Normal
Status: Closed (fixed) » Needs review
FileSize
10.19 KB

Re #201:

Also note, I'm well aware that we need some test coverage of the functionality introduced by this issue, including a test for this small patch. I promise to work on that when I get a chance.

And only 3.5 months later, here they are :) Docs shortening from #203 as well.

sun’s picture

Awesome, thanks!

+++ modules/simpletest/tests/ajax.test	27 Jan 2011 02:53:21 -0000
@@ -117,6 +117,64 @@ class AJAXFrameworkTestCase extends AJAX
+    // Veryify the expected CSS file was added, both to Drupal.settings, and as
...
+    // Veryify the expected JS file was added, both to Drupal.settings, and as

Typo in "Verify".

+++ modules/simpletest/tests/ajax.test	27 Jan 2011 02:53:21 -0000
@@ -117,6 +117,64 @@ class AJAXFrameworkTestCase extends AJAX
+      'browsers' => array('IE' => TRUE, '!IE' => TRUE,),

Stray (inner) trailing comma.

Aside from that, this looks RTBC to me.

Powered by Dreditor.

effulgentsia’s picture

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Sun gave it the thumbs up in #210, except for the typos that are fixed in #211. This is just tests and docs, so I don't think it needs more review than that, so RTBC.

rfay’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs backport to D7

Must go into D8 first.

effulgentsia’s picture

amitaibu’s picture

FileSize
1.56 KB

There is an issue with lazy loading #attached library for IE (7-9).

I've attached an example module, which is a strip down from the user-modal (currently in my sandbox).

Enable and try on chrome/ firefox -- tabs are ok.
Try on IE -- it fails.

Then you can uncomment the following lines:

function user_modal_init() {
  // FIXME: IE bug, library isn't loaded.
  // Uncomment the following link to see it working.
  // drupal_add_library('system', 'ui.tabs');

Refresh and it will of course work.

sun’s picture

Status: Needs review » Reviewed & tested by the community

@Amitaibu: Please create a separate issue for that.

sun’s picture

amitaibu’s picture

@sun,
Sure, wasn't sure if it deserved a new issue or not.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal.ajax_lazy_load_tests-211.patch, failed testing.

RobLoach’s picture

@Amitaibu: Instead of using hook_init to add a library on every page, it might be better to use hook_page_alter(), along with ['#attached']['library'].

amitaibu’s picture

@Rob Loach,
Yeah, hook_init() is the wrong way, it just to show the example. Anyway, opened a new issue about it -- #1153938: #attached for Library doesn't work with AJAX on IE

moonray’s picture

subscribing (need this for dialog module...)

casperbiering’s picture

subscribe

Anonymous’s picture

Please, add this to a D7.x version soon, so it can be used also on QuickTabs to load css from Panels inside Quicktabs.

sun’s picture

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

A completely useless patch was committed that changed each and every appearance of AJAX into Ajax in code comments. Not only utterly wrong (IMHO), but also breaking every other patch in the queue that happens to touch affected or nearby lines.

Re-rolled #211 against HEAD. Back to RTBC.

WilliamB’s picture

Is there a way to find the backport for Drupal 7.2 anywhere?

katbailey’s picture

@javier the lazy-loading solution is already in D7 - this issue is now about adding tests for it. Since Quicktabs 7.x-3.x uses the ajax framework I suggest you try that for your use case.

xjm’s picture

Tagging issues not yet using summary template.

Jacine’s picture

#967166: Content rendered via AJAX does not respect stylesheets removed in .info files depends on this issue going in.

Apparently these tests make it possible to write tests for that issue, and it's stuck on "needs work" pending this commit.

andypost’s picture

subscribe

catch’s picture

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs review

This is just docs and tests and both look great, so committed to 8.x

Moving back to D7, but leaving CNR since I'm not if the exact same patch is applicable at this point.

xjm’s picture

If I understand #232, the patch just needs to pass testbot for 7.x, correct? Requeuing. We might need a reroll.

xjm’s picture

nlambert’s picture

Hi,

Would this work for drupal_add_js inline?

For instance, to execute a small piece of code could my ajax callback contain following ?

drupal_add_js('jQuery('#somediv').fadeIn();','inline')

For now I'm just returning

$output .= '<script>jQuery('#somediv').fadeIn();</script>';

Having the former would be nice!

Cheers

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Patch passes for D7; marking RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

nlambert’s picture

For anyone looking, the answer to my question (#235), is in the following presentation (slide 12 in particular).

http://www.slideshare.net/merlinofchaos/drupal-7-advanced-ajax

A custom ajax command should be written.

Status: Fixed » Closed (fixed)

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

lex0r’s picture

Status: Closed (fixed) » Active

Sorry to re-open this. I've got an issue with the way ajax commands add CSS as part of Ajax response.
This is the code that works differently than needed in my case:

includes/ajax.inc, line 278

  if (!empty($styles)) {
    $extra_commands[] = ajax_command_prepend('head', $styles);
  }

This code makes it possible to prepend extra CSS files to other files inside head. However, I need it to be appended in order to re-define other styles, and I want it to be done dynamically when a form is returned via Ajax, in case if certain condition happens (i.e I can't include it by default).

I will probably look for a workaround and post back (I was looking for workaround and found this thread in fact).

nod_’s picture

Status: Active » Closed (fixed)

Please don't reopen several months old issue for a support request.

Your question has already been raised in the issue queue. It shouldn't be too hard to find it :)

lex0r’s picture

This is not a "support request". Anyone will experience the issues described if he/she tries to add CSS during an Ajax request. Weight/group parameters of drupal_add_css are not respected in this case.

I tried to find my question in the queue - I found a lot of duplicates of this issue merely. I would be glad to get the link :)

nod_’s picture

attiks’s picture

+++ b/modules/simpletest/tests/ajax.testundefined
@@ -116,6 +116,64 @@ class AJAXFrameworkTestCase extends AJAXTestCase {
+    // Verify the expected JS file was added, both to Drupal.settings, and as
+    // an AJAX command for inclusion into the HTML. By testing for an exact HTML
+    // string containing the SCRIPT tag, we also ensure that unexpected
+    // JavaScript code, such as a jQuery.extend() that would potentially clobber
+    // rather than properly merge settings, didn't accidentally get added.
+    $expected_js_html = drupal_get_js('header', array($expected['js'] => drupal_js_defaults($expected['js'])), TRUE);
+    $this->assertEqual($new_js, $original_js + array($expected['js'] => 1), t('Page state now has the %js file.', array('%js' => $expected['js'])));

js isn't really inserted, only exists inside the request, see new issue #1962236: Scripts added using drupal_add_js() are never added to the page after an AJAX callback

yched’s picture

Issue summary: View changes

#768128: ajax callbacks can't apply #attached was closed as a dupe of this, but ['#attached']['js'] or drupal_add_js() still fail to include the new scripts to the page when executed in an ajax callback - at least on D7.
(js settings are correctly merged though)

yched’s picture

Re: myself in previous comment:
Never mind. #1962236-4: Scripts added using drupal_add_js() are never added to the page after an AJAX callback from @Wim Leers explains that this is a weirdness in jQuery <=1.9 that the scripts do not appear to be added to the DOM, but they are still correctly loaded.
Bumping jQ to 1.10 with latest release of jquery_update fixes the weirdness (the additional <script> tags do appear in the DOM)

lmeurs’s picture

@yched: Could you please supply a working link to the page about the jQuery <=1.9 weirdness? I tried both 1.7 and 1.10 so far with success but am curious what I probably am missing. Thanks in advance!

yched’s picture

@lmeurs : didn't look too much into it, that's what I understood from #1962236-4: Scripts added using drupal_add_js() are never added to the page after an AJAX callback
(sorry, wrong link in my comment above, fixed it)

lmeurs’s picture

@yched: Thanks for the link!

rjacobs’s picture

So I realize that this is a very old, and very closed issue, and I do not wish to raise any new points. My reason for commenting is to bring some small clarity the solution that was implemented. Even though this issue is many years old I think it may still serve as a reference that sets some potentially confusing expectations about how drupal_add_js/css and #attached could be depended-on within AJAX requests. Many different pathways through the queues keep leading back to this issue so it seems like the appropriate place to capture a few key notes about assumptions and limitations.

I've been confused about why certain assets do not get included via an AJAX request, even though this issue implies they should when standard practices are used to declare them. What I've learned, and am hopeful that someone can confirm, is that the solution implemented makes some assumptions:

  1. The AJAX request must POST ajax_page_state data about what assets already exist on the page for things to work. I assume this would always happen for AJAX requests initiated via an #ajax element (forms, etc.) but it seems like other applications of the AJAX framework may not do this. The result is that something like a humble display formatter, that has no awareness of if/how it's involved in an AJAX request, cannot always count on #attached to load it's assets.
  2. Only asset files can participate. There are comments in ajax_render() stating that inline javascript is purposely skipped as it can't effectively be diffed to the existing page data.
  3. The solution is incompatible with some versions of jQuery, as noted in recent comments and #1962236: Scripts added using drupal_add_js() are never added to the page after an AJAX callback

Bits and pieces of the ~250 comments above hint at these points, but not necessarily in a clean way that speaks to the final committed fix. If someone can confirm that these points are accurate I'd be happy to add notes to the issue summary that could provide a shortcut for others who end up here.

I'd also be curious to know if there are any open follow-up issues that continue to address point #2 (inline includes don't work).

yched’s picture

@rjacobs #250:

1. The AJAX request must POST ajax_page_state data about what assets already exist on the page for things to work. I assume this would always happen for AJAX requests initiated via an #ajax element (forms, etc.) but it seems like other applications of the AJAX framework may not do this

All requests done using *Drupal*'s ajax framework include the 'ajax_page_state' in the request, this is done in the beforeSerialize callback set on all Drupal.ajax calls.

3. The solution is incompatible with some versions of jQuery, as noted in recent comments and #1962236: Scripts added using drupal_add_js() are never added to the page after an AJAX callback

From what I have observed, older versions of jQuery (< 1.9) do not append the newly added scripts to the DOM <head>, but the scripts *are* correctly loaded. So things work, your JS files are here, it's just surprising because you don't see them in the DOM.

rjacobs’s picture

Issue summary: View changes

Thanks for the response yched,

All requests done using *Drupal*'s ajax framework include the 'ajax_page_state' in the request

Yep, that makes sense. In theory I suppose the framework could be used for a response without being used for the request. The only example I can cite that seems to do this is views exposed filters (I'm not too sure what it's doing to construct the request, but it's not populating ajax_page_state). Anyway, that's getting contrib case-specific, so certainly not relevant to address here.

older versions of jQuery (< 1.9) do not append the newly added scripts to the DOM , but the scripts *are* correctly loaded

Got it, thanks. The fact that there was not a functional issue involved just didn't come though clearly in #1962236.

So the only point remaining is the fact that inline includes aren't compatible. The reason for that is clear (and well commented in ajax_render()). I went ahead and added a note about it in the summary for reference though, as I found this detail hard to distill from the queues alone. From what I can see in AjaxResponse::ajaxRender() this is probably still an issue for D8 too.

effulgentsia’s picture

So the only point remaining is the fact that inline includes aren't compatible.

Do we still support 'type' => 'inline' CSS/JS in D8, now that #2378095: Convert all remaining attached individual CSS/JS assets to attached asset libraries is done?

Wim Leers’s picture

We're talking about removing it in #2382533-5: Attach assets only via the asset library system, point 3 and approved by a core committer (catch) at #2382533-8: Attach assets only via the asset library system.