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 bydrupal_add_js/css
which could not be addressed here.
Comment | File | Size | Author |
---|---|---|---|
#225 | drupal8.ajax-lazy-load-tests.225.patch | 9.56 KB | sun |
#215 | user_modal.tar_.gz | 1.56 KB | amitaibu |
#211 | drupal.ajax_lazy_load_tests-211.patch | 10.19 KB | effulgentsia |
#209 | drupal.ajax_lazy_load_tests.patch | 10.19 KB | effulgentsia |
#207 | drupal.ajax_lazy_load_561858_207.patch | 2.83 KB | effulgentsia |
Comments
Comment #1
RobLoachWould 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().
Comment #2
andrewlevine CreditAttribution: andrewlevine commentedThis 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.
Comment #3
haydeniv CreditAttribution: haydeniv commentedIsn't this already being accomplished with the Ajax Load module?
http://drupal.org/project/ajax_load
Could be wrong but seems to be similar.
Comment #4
RobLoachAlso note that the jQuery 1.4 Roadmap speaks of jQuery.require(), which might assist with asynchronous loading of JavaScript libraries.
Comment #5
sunAll 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.
Comment #7
seutje CreditAttribution: seutje commentedThis 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:
for CSS this would be
also, having the option to re-load a cached script can be really handy in some cases
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
Comment #8
merlinofchaos CreditAttribution: merlinofchaos commentedWell, 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?
Comment #9
RobLoachWe'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.
Comment #10
seutje CreditAttribution: seutje commentedyea 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
Comment #11
sun.core CreditAttribution: sun.core commentedWe need a solution for this. 90% of AJAX stuff is more or less useless without lazy-loading.
Comment #12
q0rban CreditAttribution: q0rban commentedsubscribe
Comment #13
Pasquallesubscribe
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedsubscribe
Comment #15
effulgentsia CreditAttribution: effulgentsia commentedRe #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?]
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedThere are a bunch of AJAX-related issues in the queue, and no AJAX component, so tagging to try to get a handle on them.
Comment #17
casey CreditAttribution: casey commentedSubscribing.
Comment #18
effulgentsia CreditAttribution: effulgentsia commented#653580: Upgrade to jQuery 1.4 landed. Does that let us move forward without xLazyLoader, or do we still need xLazyLoader (#9)?
Comment #19
sun$.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.
Comment #20
effulgentsia CreditAttribution: effulgentsia commentedPlease 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.
Comment #21
cosmicdreams CreditAttribution: cosmicdreams commented@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.
Comment #22
RobLoach@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.
Comment #23
casey CreditAttribution: casey commentedNot sure if xlazyloader is jquery 1.4 ready.
Comment #24
merlinofchaos CreditAttribution: merlinofchaos commentedI 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.
Comment #25
effulgentsia CreditAttribution: effulgentsia commented@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.
Comment #26
sun@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 ?
Comment #27
agentrickardI'm sorry. Why is this critical? Seems like a nice-to-have, not a release blocker.
Comment #28
effulgentsia CreditAttribution: effulgentsia commentedI 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 :)
Comment #29
effulgentsia CreditAttribution: effulgentsia commentedI 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 :)
Comment #30
merlinofchaos CreditAttribution: merlinofchaos commentedSorry, I haven't had a chance to work on this in awhile.
Comment #31
sunThis 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.
Comment #32
sunDiscussing 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.
Comment #33
effulgentsia CreditAttribution: effulgentsia commentedI 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.
$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]
Comment #34
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #35
rfaysubscribing
Comment #36
Wim LeersI 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:
I fully support this feature. This will also allow for improved page loading performance.
Comment #37
Wim LeersMaking this issue also show up in the performance issues. Page loading performance is a specific kind of performance.
Comment #38
yhager CreditAttribution: yhager commentedMarking as a bug. We also need this for #768128: ajax callbacks can't apply #attached
Comment #39
Scott Reynolds CreditAttribution: Scott Reynolds commentedseems 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.
But this doesn't have to do with the form at all. Doesn't it belong in just the cache bin?
Comment #40
Scott Reynolds CreditAttribution: Scott Reynolds commentedAlso, this is using the ajax settings command to add scripts and styles. Seems like we need two new commands as well.
Comment #41
katbailey CreditAttribution: katbailey commentedThis 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...?
Comment #42
merlinofchaos CreditAttribution: merlinofchaos commentedAnyone 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.
Comment #43
sunI 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.
Comment #44
Scott Reynolds CreditAttribution: Scott Reynolds commentedNot 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.
Comment #45
sunwow. effulgentsia, you totally rock.
We need to add the following to the non-form part of Drupal.ajax() though:
With that, ajax-delivered HTML returned via ajaxified links are lazy-loading JS/CSS as well. :)
Comment #46
sunAnd, 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.
Comment #47
pwolanin CreditAttribution: pwolanin commentednice to have
Comment #48
merlinofchaos CreditAttribution: merlinofchaos commentedI'm not sure this should be downgraded to normal. See #38
Comment #49
sunI 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.
Comment #50
merlinofchaos CreditAttribution: merlinofchaos commentedThis 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.
Comment #51
Wim LeersI 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.
Comment #52
effulgentsia CreditAttribution: effulgentsia commentedRe #49:
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.
Comment #53
fagosubscribing
Comment #54
effulgentsia CreditAttribution: effulgentsia commented.
Comment #55
chweetraju CreditAttribution: chweetraju commentedsubscribing
Comment #56
mfer CreditAttribution: mfer commentedA couple things:
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.
Comment #57
catchI 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.
Comment #58
rfaySo 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.
Comment #59
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #60
yched CreditAttribution: yched commentedsubscribe.
/me wants #states in ajaxed forms
Comment #61
yched CreditAttribution: yched commentedUnrelated yet shameless plug while all the right #ajax people are subscribed to this thread :
#818660: Prevent duplicate code by adding an AJAX command to invoke simple jQuery methods
#736066: ajax.js insert command sometimes wraps content in a div, potentially producing invalid HTML and other bugs
pending reviews :-)
Comment #62
Anonymous (not verified) CreditAttribution: Anonymous commentedsubscribing
Comment #63
rfayThis 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.
Comment #64
mikeytown2 CreditAttribution: mikeytown2 commentedThese functions need to be documented. I need to know what they do and how to use them.
Powered by Dreditor.
Comment #65
mikeytown2 CreditAttribution: mikeytown2 commentedComment #66
merlinofchaos CreditAttribution: merlinofchaos commentedThis 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.
Comment #67
rfay@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.
Comment #68
merlinofchaos CreditAttribution: merlinofchaos commentedOk. 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.
Comment #69
merlinofchaos CreditAttribution: merlinofchaos commentedFor 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.
Comment #70
merlinofchaos CreditAttribution: merlinofchaos commentedOh 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.
Comment #71
rfayWorked 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.
Comment #72
rfayNot sure why it went to needs work.
Comment #74
rfayThe 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!
Comment #75
rfayAh the weirdness. Why does it always do the wrong thing?
Comment #76
merlinofchaos CreditAttribution: merlinofchaos commentedArgh. 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?
Comment #77
rfayOne of us can do it, I'm sure. You made the big push! If I can get to it later today I will.
Comment #78
rfayIt 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.
Comment #79
rfayOK, 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.
Comment #80
DamienMcKennaHow is the failover for CSS loading if JS is disabled in the browser?
Comment #81
mfer CreditAttribution: mfer commentedThe $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.
There should be a space before ||
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?
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.
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!
Comment #82
mfer CreditAttribution: mfer commentedIf we do go with out own loader I wonder if we should do something similar to what jQuery is doing internally? Something like:
Three big differences here.
FYI, insertBefore is what jQuery.prepend uses.
40 critical left. Go review some!
Comment #83
merlinofchaos CreditAttribution: merlinofchaos commentedThe 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.
Comment #84
Owen Barton CreditAttribution: Owen Barton commentedIn 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.
Comment #85
katbailey CreditAttribution: katbailey commentedHere'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.
Comment #86
fagoI gave #85 a short test with the rules UI which needs js lazy-loading several times - works fine!
Comment #87
rfayI also took #85 for a spin using the test module in #74, and it worked great.
Comment #88
katbailey CreditAttribution: katbailey commentedIt 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.
Comment #89
mfer CreditAttribution: mfer commented@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.
Comment #90
rfayOK, 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
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.
Comment #91
merlinofchaos CreditAttribution: merlinofchaos commentedLet's pass on this based on katbailey's experiments.
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.
Comment #92
effulgentsia CreditAttribution: effulgentsia commentedThanks 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.
Comment #93
mfer CreditAttribution: mfer commented@merlinofchaos Have you looked into recording the CSS paths in _drupal_build_css_path?
Comment #94
ksenzeeChanging to the new major priority as an excuse to subscribe.
Comment #95
chweetraju CreditAttribution: chweetraju commented#28: drupal-ajax-lazy-css-js-561858-28.patch queued for re-testing.
Comment #96
effulgentsia CreditAttribution: effulgentsia commentedRe #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.
Comment #97
BenK CreditAttribution: BenK commentedNeed to track this...
Comment #98
dawehnerRerole the patch
This patch fixes a bug for contrib module which uses a tabledrag form on a page loaded with the ajax framework.
Comment #99
webchickSubscribing as an excuse to subscribe. ;)
Comment #100
yched CreditAttribution: yched commentedWhile ajax folks (and webchick) are subscribed here, shameless bump for #818660: Prevent duplicate code by adding an AJAX command to invoke simple jQuery methods
Comment #101
rfayThis 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.
Comment #102
katbailey CreditAttribution: katbailey commentedTo 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.
Comment #103
rfayTo 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.
Comment #104
effulgentsia CreditAttribution: effulgentsia commented#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.
Comment #105
sunThanks! 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.
Comment #106
sunFor full BC, the settings command needs to be responded first.
Comment #107
rfayMarked #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.
Comment #108
rfayI 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.
Comment #109
webchickI'm confused in this issue why we're passing around tarballed modules and not expanding test coverage?
Comment #110
rfay@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!
Comment #111
webchickOh, right. :\ Sorry, carry on!
Comment #112
merlinofchaos CreditAttribution: merlinofchaos commentedHere 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.
Comment #113
rfayThanks, merlinofchaos. I confirm that #112 works great.
Comment #114
merlinofchaos CreditAttribution: merlinofchaos commentedOK 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.
Comment #116
effulgentsia CreditAttribution: effulgentsia commentedReally 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):
Comment #117
Owen Barton CreditAttribution: Owen Barton commentedSomeone 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.
Comment #118
fago>Changing to the new major priority as an excuse to subscribe.
!? This is definitely critical - without that the ajax system cannot be properly used.
Comment #119
Nick_vhBeing 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.
Comment #120
rfay@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
Comment #121
rfay#114: 561858-ajax-lazy-load.patch queued for re-testing.
Comment #122
rfayInitiated 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.
Comment #124
rfayAt the sprint I'm working on fixing the tests, and making progress on that.
Comment #125
Nick_vhDrupalcon 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
Comment #126
Nick_vhComment #128
Nick_vhJust 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!
Comment #129
effulgentsia CreditAttribution: effulgentsia commentedI'm putting some more hours into this today.
Comment #130
effulgentsia CreditAttribution: effulgentsia commentedBecause 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.
Comment #132
effulgentsia CreditAttribution: effulgentsia commentedMinor fix.
Comment #133
Nick_vheffulgentsia 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 :-)
Comment #134
effulgentsia CreditAttribution: effulgentsia commentedHi Nick. There's a few differences between #125 and #132:
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.
Comment #135
sun#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.
Comment #137
mparker17Subscribe
Comment #138
bojanz CreditAttribution: bojanz commented#135: drupal.ajax-lazy-load.135.patch queued for re-testing.
Comment #140
rfayThis 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":
3. The change to ajax_render in #135 didn't actually work, so I put it back mostly the way it was in #132.
Comment #142
rfayTwo 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.
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.
Comment #143
Nick_vhPerfect! 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?
Comment #144
webchick#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.
Comment #145
duellj CreditAttribution: duellj commentedI'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?
Comment #146
duellj CreditAttribution: duellj commentedCross post, sorry.
Comment #147
rfay@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:
Comment #148
rfayReviewer'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.
Comment #149
merlinofchaos CreditAttribution: merlinofchaos commentedI 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.
Comment #150
Nick_vh+1 seems to be working well, and i second the opinion of merlinofchaos.
marking as RTBC, it is about time!
Comment #151
Nick_vhAfter some considerations I'm reverting this back to needs review because, as mentioned earlier, the approval of sun & effulgentsia is critical.
Comment #152
merlinofchaos CreditAttribution: merlinofchaos commentedWell, Nick, this patch is effulgentsia's approach. I think we can safely assume approval.
This quote from sun:
Sounds like approval to me. Admittedly it's 2 tweaks ago, but the tweaks since have just been test fixes.
Comment #153
sunPatch went stale. Re-rolled against HEAD. Will review now.
Comment #154
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #155
sunOdd. 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?
Comment #156
rfayShucks, testing with the test module in #122 #fails.
Comment #157
sunAlright. 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)
Comment #158
rfayThis one was broken by #756762: AJAX should follow same rules for whether to call drupal_rebuild_form() as non-AJAX submissions and is fixed by the current #115 over there.
Comment #159
sunWe 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.
Comment #160
Nick_vh@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?
Comment #161
rfaySetting 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.
Comment #163
katbailey CreditAttribution: katbailey commentedI 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.
Comment #164
effulgentsia CreditAttribution: effulgentsia commentedI'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.
Comment #165
effulgentsia CreditAttribution: effulgentsia commented#159: drupal.ajax-lazy-load.158.patch queued for re-testing.
Comment #167
katbailey CreditAttribution: katbailey commented@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.
Comment #168
Nick_vhA 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!
Comment #169
Nick_vhSorry, wrong patch format. Correct one now
Comment #170
Nick_vhReview bot, review!
Comment #171
Anonymous (not verified) CreditAttribution: Anonymous commentedsubscribe. 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.
Comment #172
Nick_vhAnd 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!
Comment #173
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #174
rfayUsing 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.
Comment #175
effulgentsia CreditAttribution: effulgentsia commentedI 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 :)
Comment #176
Nick_vhI 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
Comment #177
effulgentsia CreditAttribution: effulgentsia commentedThanks, 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.
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.
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.
Comment #178
rfayAgreed. Time for this one to go in.
Comment #179
Dries CreditAttribution: Dries commentedTRUE 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.
It would be good to explain why that is preferred.
Comment #180
rfayThis patch changes #173 only in comments as requested by @Dries as follows:
and
If it passes muster we should go right back to RTBC.
Comment #182
rfayPutting back to RTBC. The first bot stumbled for unknown reasons. I'll have it do a retest.
Comment #183
egorovanton CreditAttribution: egorovanton commented#29: drupal-ajax-lazy-css-js-561858-28.patch queued for re-testing.
Comment #184
tstoeckler..., 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.
Comment #186
tstoeckler#184: drupal.ajax_lazy_load_561858_184.patch queued for re-testing.
Comment #187
effulgentsia CreditAttribution: effulgentsia commentedBot 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 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.
Comment #188
effulgentsia CreditAttribution: effulgentsia commentedJust tweaked the docs for ajax_base_page_theme() as per #179, to:
No code change, so leaving RTBC.
Comment #189
Dries CreditAttribution: Dries commentedAlright. Thanks for making the documentation improvements. Committed to CVS HEAD! Yay.
Comment #190
merlinofchaos CreditAttribution: merlinofchaos commentedThere 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.
Comment #191
sunI've a ready solution for that in #647228: Links are needlessly unable to fully participate in D7 AJAX framework features
Comment #192
effulgentsia CreditAttribution: effulgentsia commented@merlinofchaos: being dealt with in #647228: Links are needlessly unable to fully participate in D7 AJAX framework features I think.
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.Comment #193
effulgentsia CreditAttribution: effulgentsia commentedx-post.
Comment #194
merlinofchaos CreditAttribution: merlinofchaos commentedUm. 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.
Comment #195
sun@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.
Comment #196
merlinofchaos CreditAttribution: merlinofchaos commentedWith 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:
You can see that it's getting jquery.js which it absolutely should NOT be getting a second time.
Comment #197
merlinofchaos CreditAttribution: merlinofchaos commentedOk, 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.
Comment #198
merlinofchaos CreditAttribution: merlinofchaos commentedOk, the problem is here:
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:
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.
Comment #199
Anonymous (not verified) CreditAttribution: Anonymous commentedjust updating the status.
Comment #200
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #201
effulgentsia CreditAttribution: effulgentsia commentedSorry 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.
Comment #202
sunThanks! 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.
Comment #203
Dries CreditAttribution: Dries commentedWow, that is a long sentence. It has 3 nested because's ... :)
That said, it seems to make sense. Will commit later.
Comment #204
merlinofchaos CreditAttribution: merlinofchaos commentedYes, this fix is working for me now, though the description I wrote of it was somewhat shorter. =)
Comment #205
webchickCommitted to HEAD. Re-working that paragraph-masked-as-a-sentence can be the job of a non-critical follow-up patch, if we want.
Comment #206
yched CreditAttribution: yched commentedI 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 :-).
Comment #207
effulgentsia CreditAttribution: effulgentsia commentedApologies 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.
Comment #208
sunThe 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.
Comment #209
effulgentsia CreditAttribution: effulgentsia commentedRe #201:
And only 3.5 months later, here they are :) Docs shortening from #203 as well.
Comment #210
sunAwesome, thanks!
Typo in "Verify".
Stray (inner) trailing comma.
Aside from that, this looks RTBC to me.
Powered by Dreditor.
Comment #211
effulgentsia CreditAttribution: effulgentsia commentedComment #212
effulgentsia CreditAttribution: effulgentsia commentedSun 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.
Comment #213
rfayMust go into D8 first.
Comment #214
effulgentsia CreditAttribution: effulgentsia commentedFYI: #1149770: Consider using RequireJS for loading JS files needed after an AJAX request.
Comment #215
amitaibuThere 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:
Refresh and it will of course work.
Comment #216
sun@Amitaibu: Please create a separate issue for that.
Comment #217
sun#211: drupal.ajax_lazy_load_tests-211.patch queued for re-testing.
Comment #218
amitaibu@sun,
Sure, wasn't sure if it deserved a new issue or not.
Comment #220
RobLoach@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'].
Comment #221
amitaibu@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
Comment #222
moonray CreditAttribution: moonray commentedsubscribing (need this for dialog module...)
Comment #223
casperbiering CreditAttribution: casperbiering commentedsubscribe
Comment #224
Anonymous (not verified) CreditAttribution: Anonymous commentedPlease, add this to a D7.x version soon, so it can be used also on QuickTabs to load css from Panels inside Quicktabs.
Comment #225
sunA 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.
Comment #226
WilliamB CreditAttribution: WilliamB commentedIs there a way to find the backport for Drupal 7.2 anywhere?
Comment #227
katbailey CreditAttribution: katbailey commented@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.
Comment #228
xjmTagging issues not yet using summary template.
Comment #229
Jacine#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.
Comment #230
andypostsubscribe
Comment #231
catch#225: drupal8.ajax-lazy-load-tests.225.patch queued for re-testing.
Comment #232
catchThis 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.
Comment #233
xjmIf I understand #232, the patch just needs to pass testbot for 7.x, correct? Requeuing. We might need a reroll.
Comment #234
xjm#225: drupal8.ajax-lazy-load-tests.225.patch queued for re-testing.
Comment #235
nlambert CreditAttribution: nlambert commentedHi,
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
Comment #236
xjmPatch passes for D7; marking RTBC.
Comment #237
webchickCommitted and pushed to 7.x. Thanks!
Comment #238
nlambert CreditAttribution: nlambert commentedFor 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.
Comment #240
lex0r CreditAttribution: lex0r commentedSorry 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
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).
Comment #241
nod_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 :)
Comment #242
lex0r CreditAttribution: lex0r commentedThis 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 :)
Comment #243
nod_Hello, you can find the issue here #1461322: Fix AJAX add_css – insert the needed CSS assets after the already-inserted ones.
Comment #244
attiks CreditAttribution: attiks commentedjs 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
Comment #245
yched CreditAttribution: yched commented#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)
Comment #246
yched CreditAttribution: yched commentedRe: 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)Comment #247
lmeurs CreditAttribution: lmeurs commented@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!
Comment #248
yched CreditAttribution: yched commented@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)
Comment #249
lmeurs CreditAttribution: lmeurs commented@yched: Thanks for the link!
Comment #250
rjacobs CreditAttribution: rjacobs commentedSo 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:
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.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).
Comment #251
yched CreditAttribution: yched commented@rjacobs #250:
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.
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.Comment #252
rjacobs CreditAttribution: rjacobs commentedThanks for the response yched,
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.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.
Comment #253
effulgentsia CreditAttribution: effulgentsia commentedDo 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?Comment #254
Wim LeersWe'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.