The main issue with calling ctools_ajax_command_scripts() in ctools_ajax_render() is that when using jquery_update (which replaces the jquery script tag in a preprocess_page), Drupal's original jquery.js gets loaded even though jquery_update's jquery.js was already loaded. This causes everything that was extended onto jquery's $ object to be wiped out. If we leave this in, there should probably at least be a big warning on the project page since jquery_update has 34k users.
This also could potentially cause other problems: In ctools_ajax_render(), because drupal_add_js(NULL,NULL) is being called, it assumes it is outputting a page and returns all scripts it thinks it needs for that (non-existent) page. This happened to include a theme javascript file for a theme I wasn't using (but ostensibly would have been used if the modal was actually a true page).
I don't understand why ctools_ajax_command_scripts was added in ctools_ajax_render but I assume it was because scripts needed to be added for different functionality in the modal. To prevent unpredictable behavior, we should probably have a ctools_modal_add_js to add scripts one by one instead of adding everything from drupal_add_js(NULL, NULL) minus what was already loaded.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | ctools_js_replacements3.patch | 2.75 KB | andrewlevine |
| #13 | ctools_745468-13.patch | 8.69 KB | Scott Reynolds |
| #12 | ctools_js_replacements2.patch | 3.23 KB | andrewlevine |
| #10 | ctools_js_replacements.patch | 2.68 KB | andrewlevine |
| #4 | ctools_script_add_tmpfix.patch | 1020 bytes | andrewlevine |
Comments
Comment #1
merlinofchaos commentedWell, the idea was to catch everything even if the page does not realize it's part of the modal. One of the problems we have with the modal is that things like wysiwyg and img_assist don't really work very well because they add their own javascript to forms, and unless they're *aware* of the modal, they can't do it.
In order to make the stylizer work, I discovered I needed to send a lot of javascript and settings and while I can make this stuff modal aware, it adds a significant burden of work.
I should think that there would be a way to test for what jquery update is doing and get the right jquery, for example. Not sure about the theme javascript. I'm not sure that should matter, since wouldn't that already have been loaded on the primary page?
Comment #2
andrewlevine commentedHmm, OK I understand the problem better now. A couple possible options I can think of:
1. Render the page as if it was a regular page and save it in a variable , and then pick out the scripts from that (this would fix the jquery_update problem because preprocess_page would swap out the jquery version)
2. Module_invoke_all a hook that returns an "exclude script" array which would give a list of scripts not to add to the modal. At least we would have a way of tweaking the scripts that load this way.
Regarding the theme problem: The primary page was a panel page edit page which was using the admin theme. When the modal was called, it was using the user-facing theme (I guess because the URL was not in the admin path) and it caused the theme javascript to be loaded. I don't think this did anything harmful, but I guess it potentially could if you are loading the javascript for the wrong theme. Now that I think about it, we can fix this problem by breaking on the 'theme' type like we do with 'inline'. Like you said, any js from the current theme should already be loaded.
Comment #3
zroger commentedCan we assume that we don't ever need to load jquery since it will have been required to do the ajax call anyway?
Comment #4
andrewlevine commentedI think Roger should be right...here's a patch that would fix the jquery_update+theme JS problems. It wouldn't fix the problems from scripts being removed via the theme layer, but nobody should really be doing that anyway.
Comment #5
andrewlevine commented#4 actually won't completely fix the problem because it double loads jquery.form.js, ahah.js etc etc
Just noticed this happening while I had firebug on and was clicking around panels
Comment #6
merlinofchaos commentedOk, looking at how jquery_update does its thing, it is in fact not possible to detect what it does cleanly. We could put a 'hook' in try to replicate what it does, but calling through to the page process won't help because extracting the data out of
isn't really going to be viable.Comment #7
merlinofchaos commentedWe could attempt to alleviate the problem by checking raw script names and chucking the directory? This will, of course, cause scripts with actually duplicated names to possibly not be loaded during ajax calls, but it's a low percentage occurence I think.
Comment #8
andrewlevine commentedUnfortunately, #7 wouldn't work for jquery_update because it uses the minified version (I think by default) with the filename jquery.min.js.
Not sure what we can do about this besides sending an alterable list of scripts to exclude to the "scripts" function through Drupal.settings.
Comment #9
merlinofchaos commentedI think the better choice is to build upon what jquery_update does with its little replacement functions. If we ask jquery_update to provide that as a hook we could call that hook and perform those updates. That'll reduce the actual interdependence by creating a sort of "agreed upon" hook that non-CTools things could also use.
Then we could hack our own copy of the hook in private modules until jquery_update is updated (which is, what, a 3 line hook implementation?) to do this.
Comment #10
andrewlevine commentedHere is a patch for what you proposed in #9. If this looks good, I'll put together a patch for jquery_update and we can (hopefully) convince them to put it in the next release
Comment #11
merlinofchaos commentedWow, tiny patch!
I learned last night that along with theme .js we also should not be sending theme .css -- I gave Michelle a temporary patch here http://merlinofchaos.pastebin.com/92bLW6bg and we may as well integrate it into this one.
Comment #12
andrewlevine commentedAdded in the CSS patch as well.
Comment #13
Scott Reynolds commentedThis patch adds a page id that is used to cache the JS and CSS into a cache table. This page id is then used in the AJAX request to prevent all those scripts and CSS from being added to the AJAX response.
It also ignores theme js and css because as noted, with admin themes this isn't desired and they are automatically added already. This patch will pave the way for this to work with JS aggregation. #751252: ctools_ajax_render doesn't respect js aggregation
Comment #14
merlinofchaos commentedI'm not sure if there's a way to port this to 7.x, but marking it for porting anyway.
I had to make acouple of slight changes (it was possible to have horrible javascript crashes if pageId wasn't in the settings) but otherwise committed as is. Let's see how this affects people.
Comment #15
andrewlevine commentedthe committed patch didn't include the js_replacements hook and therefore jquery_update still breaks ctools. Working on updating my patch to fix this.
Comment #16
andrewlevine commentedHere is the updated patch that builds on Scott Reynolds' patch that works around the jquery_update/js theme layer problem. Once this gets committed I will file an issue to jquery_update so hopefully we won't have to have the code in ctools.module anymore.
Comment #17
merlinofchaos commentedCommitted the followup (I admit Id idn't even look at it, since I thought it was already *in* the patch =)
Comment #18
Scott Reynolds commentedFor porting, probably want to start with the actual patches that were committed
http://drupal.org/cvs?commit=352712
http://drupal.org/cvs?commit=352912
Comment #19
merlinofchaos commentedAll of the 'to be ported' patches got merged in during the porting sprint in September.