Auto-load needed JS and CSS files (and settings)

Wow.
Your last two patch were cool, but this one takes the cake. I am giddy.
I think I will have to close out the 1.1 series before integrating this one :)

Totally understandable. I've been using popups in a secret project for months now, and finally got to the point of understanding where I felt good about the code that we'd put together. Now that it's working correctly it's time to put back. I was just glad to see that popups was still being supported! Anyway, no worries on a timeline, I just wanted to put it out there and save someone else the work.

Cool, so this isn't just prototype code, you have actually been using it? I have been eagerly waiting to see what, other than SimpleViews, will peculate out of your secret project. Let me know if you release an modules that use Popup API - I am trying to create a list on the project page.

> I was just glad to see that popups was still being supported!
Yeah, I had to take a little time off after the Drupal 7 debacle. I really thought we were super close, and then getting marked as postponed for a issue that in turn was marked as postponed for another issue that hasn't been touched for about 6 months. Demoralizing. But after a bit I backported as many of the good idea from D7 back to the D6 version as I could, and have been building it up from there. Lots of people seem to be using it, and I have been able to make it much easier to use based on all the feedback I have been getting. Your patch will be another huge step in that direction.

I still don't know what to do about D7. Every time I hear Dries give a talk, he always emphasizes that D7 will be able to speak json, but I haven't seen any visible progress at all. I might take a shot at pulling out the part of the patch that adds a hook in the form submission process, so you can request other behaviors than just redirection. People have had a chance to see how sexy Views2, and I could pitch the patch as allowing views2-like effects without having to write your own version of drupal_get_form, like Earl ended up doing.

Committed to the 6--2 branch. I am amazed by how well this works. Now I just have to understand it better. Right now I am still too excited :)

I can't think of any reason to keep the 'additionalJavascript' and 'additionCSS' functionality in popups_add_popups(), can you?

Excellent! Regarding "additionalJavascript" and "additionalCSS", I can't think of any reason to keep additional CSS. However I've found that a *very few* number of JS files don't work if they're loaded dynamically. The only one I've actually experienced with this is tiny_mce.js, because it does two silly things: it uses document.write() to add more JS files to the page, but since the page is already loaded it doesn't work, and second tiny_mce.js actually scans the current page to find path for tiny_mce.js.

However, there are so few exceptions like this that it might be best to get rid of additionalJavascript and handle such exceptions in a different way.

Were you able to get popups and tinymce working together?

Yep! Just loaded tiny_mce.js first on the main page, then everything worked fine.

Note I was using WYSIWYG API, not tinyMCE module. Apparently tinyMCE 2 cannot be added to dynamically added textareas after the page is loaded, so it requires tinyMCE 3, which the tinyMCE module is not yet updated to support. Does the teaser splitter work for you? It really seems like it should work, since I can simply rename the behavior and everything works fine. It's a very peculiar problem.

Nice work. However, this is functionality that really belongs in an abstracted contrib module, since it's needed for most or all AJAX modules.

I wrote http://drupal.org/project/ajax_load for this purpose, as part of a patch adding AJAX to views.

Please consider rolling back this patch and instead building on (and improving where necessary) ajax load.

quicksketch and starbow, I've assigned you both CVS access to ajax load. If you do wish to improve it, please open issues.

Thanks.

@quicksketch - Your issue with the might be line 419 of popup.js
> delete Drupal.behaviors.teaser; // Work-around for bug in teaser.js (sigh).
In my testing, without this line, the teaser button kept being replicated every time Drupal.attachBehaviors was called. I think teaser.js doesn't respect the context. I don't remember if I ever got around to opening an issue.

@nedjo & @quicksketch - do either of you have a feeling for how much work it would be to use ajax load vs the popup specific code?

@nedjo - I downloaded ajax_load and took a look at it, but I don't understand how to use it. I do support the idea of having this functionality in a separate module (and hopefully getting it into Drupal 7).. If you provide me with a patch against the 6--1 branch, I will test it out. In the meantime, I am going to continue to work with Nate's current approach.

@quicksketch: more on the teaser. The problem is line 70 of teaser.js is:
var include = $('#'+ this.id.substring(0, this.id.length - 2) +'include');
and it should be:
var include = $('#'+ this.id.substring(0, this.id.length - 2) +'include', context);

Hard to call this one a bug, since it only fails when the page is not valid, with the same id appearing multiple times. But anything in a behavior should select within the context just for this reason.

Here is an patch on the 6--1 branch. I haven't tested but this should work.

From here the work would be: test, bugfix if necessary, add in improvements from quicksketch's work that weren't covered in ajax_load (which is very basic).

I think this is worth doing right, i.e., in an abstracted way, and I'm happy to lend a hand as needed.

And, yes, we should try to get this into Drupal core for 7. In which case, we need to abstract.

I've updated the documentation at http://drupal.org/project/ajax_load to try to be clearer about how to implement ajax_load.

I agree we need to get something working universally in Drupal core, but I'm not feeling entirely convinced that we should make popups depend on ajax_load.module, which might not provide the same ability that building directly into popups does.

I haven't looked enough into ajax_load, but there was a problem that is solved in this popups implementation that I don't believe could be used universally. This patch provides nearly 100% functionality in popups because it pulls in not only CSS and JS files, but also JS settings. Unfortunately I can't think of a way that we can safely pull in JS settings in a universal manner. Popups are a convenient use-case because they effectively "lock" the rest of the page until the popup is closed. This patch uses the approach that assumes the background page doesn't need its settings while the popup is open, so it actually replaces all the JS settings on the page with those from the popup. When the popup is closed, the popup's JS settings are then discarded and the original settings are restored to the page.

This approach certainly would not work in a "universal" AJAX solution, however it works exceptionally well in this popup scenario. Again, I haven't looked into AJAX load enough to know if it provides an equally suitable solution. And at the same time, the solution we've implemented for popups isn't directly portable to it. So my feeling is that since our solution works well, and only works in popups, we shouldn't add an unnecessary dependency that doesn't provide the same functionality. If we can find a universal solution (such as segregating JS settings into different sections of functionality) then introducing this dependency would be fine. However, I don't know if this will be possible in Drupal 6 due to the current structure and use of the JS settings array.

Popups indeed has some unusual and interesting needs for settings handling, and you've solved them nicely. But that is only one small detail. It definitely doesn't mean that we need to replicate the whole solution--any more than a single small feature justifies a fork of a module. We just need to add a bit of flexibility.

I've just applied #338271: Allow other modules to opt out of ajax_load settings handling, which should provide the flexibility needed. To use this (with the 6.x dev version of ajax_load), add a __customSettings = TRUE flag to the object you return by JSON. Then do your own handling of this one piece, the settings data.

Loading data needed for AJAX seems to me a clear cut case for an API module. Please try to use and improve on Ajax load rather than repeating so much nearly identical code.

Setting to code needs work. Again, I'd like to see this patch rolled back and done in a way that uses and improves on AJax load as an abstracted solution. Yes, the solution that was applied "works", but since appears to be a clear cut case of code duplication it wasn't the right approach.

One thing to consider is that Ajax load support is already built into Views. Any site using views and making extensive use of Ajax loading should use Ajax load. So Ajax load will provide benefits to most sites beyond the Popups use.

I produced a patch on 6--1 as requested. I also provided upgrades to Ajax load to meet the specific needs of Popups. Are there other outstanding issues that would prevent doing this right?

Oops, I'm beginning to sound strident in this issue ;)

quicksketch, starbow, if you feel like refactoring along the lines I suggested, pls open a new issue and I'd be happy to help. Meantime we can call this one fixed/closed.