Jump to:

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:

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.

Comments

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

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.

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:

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

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.

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.

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

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.

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

@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 ?

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 :)

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 :)

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.

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]

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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?

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.

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.

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.

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?

This inserts it in head rather than html. head is where jQuery inserts scripts.

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.

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.

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.

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.

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

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.

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.

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.

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.

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.

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

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.

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.

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.

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

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.

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.

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

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!

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.

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

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?

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

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.

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.

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.

@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?

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

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.

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

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.

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 :)

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?

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.

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.

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);

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

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.

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

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.

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.

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.

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.

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

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.

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.

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.

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

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 :)

#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)

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:

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.

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.

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

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.

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.