ajax.js and tabledrag.js need to implement Drupal.detachBehaviors()

Here's a patch. This can be tested with the iframesrc module from http://drupal.org/node/561796#comment-2139256. Not exactly a "useful" module, but hopefully, at least useful as a test-case.

Here's what the patch does:

• Calls detach before old content is removed (for example, on the wrapper about to be "replaceWith"d).
• Calls detach with the entire form as the context in the beforeSerialize() handler. The reason is in the case of wysiwyg, which is what we're doing this for, data needs to be transferred from the editor to the textarea *before* the form values are collected.
• Calls attach on the entire form as the context for both success() and error() handlers, to undo above.

This means that in the use-case of "add another item" of the iframesrc field (and more importantly, a wysiwyg enabled textarea), detachBehaviors() is called twice (once before the form is serialized, and once before the wrapper is removed) and attachBehaviors() is called twice (once after the new content is added, and once in the success() handler). I'm assuming this isn't a problem, since behaviors use the convention of a BEHAVIOR-processed class name to avoid redundant attaching/detaching. I don't see an immediate way around this (non)problem, since we don't want to assume any correlation between the wrapper and the form.

The other issue I can foresee is that it might be annoying to "detach" everything within the form, especially if the AJAX will only affect a small part of the form (for example, here we detach behaviors for all elements within node-form, even though "add another item" only really affects that field). However, only by detaching everything within the form can we be assured that wysiwyg has a chance to have the form values populated correctly. By "annoying", I mean that if wysiwyg implements detach() as a reversion of the wysiwyg editor to a simple textarea, then this would result in a lot of flickering due to all wysiwyg editors on the form reverting to the textarea at the start of the AJAX call and then reverting back to the editor at the end of the AJAX call, even if the AJAX call doesn't affect that field.

What this points to is that we may want to have two modes of detach. Something like "fully detach", in which case the wysiwyg editor would fully revert to a simple textarea, and "partially detach", in which case the wysiwyg editor would save its content to the textarea, but not need to fully revert to the textarea. This partial detaching could be done fast, and would mean that there wouldn't be a situation of all wysiwyg editors visually disabling and re-enabling, every time "add another item" is performed on an unrelated field. What do you think, should we add a 3rd parameter to Drupal.detachBehaviors() to support the distinction? Or can we assume that "partial" is all we ever need, and that there's no use-case for "full", in which case, no new parameter needed?

Confirming that #3 still works for me. @sun: I'm still curious about what you think of my last paragraph in that comment.

While the text of comment #3 is still relevant, there's an updated tester module: http://drupal.org/node/561796#comment-2139256.

I updated comment #3 to have the correct link to the tester module.

Discussed with sun and DamZ on IRC, and will soon implement and summarize the thinking. Leaving as "needs review" because more reviewer feedback is still welcome.

Thanks sun! In addition to what you already said, there's a 3rd type: "row swap", or more generically "dom move". I think we should incorporate the change needed to tabledrag.js (#561796: tabledrag.js does not implement Drupal.detachBehaviors()) into this issue because of this. A "dom move" is different than a "dom remove" followed by a "dom insert", because it appears only iframes get messed up during a dom move, and non-iframe tied behaviors wouldn't need to do anything to deal with a row swap.

Here's a patch with both the ajax.js and tabledrag.js changes. Unfortunately, I am not familiar enough with the whole local settings thing to decide what to pass in for that parameter. To help search, I added null for that parameter. Can we get quicksketch or someone else who understands how local settings work to look at this and advise on what's needed for local settings in each line that invokes Drupal.attach() and Drupal.detach()?

Apologies for the "?" lines at the top of the previous patch. Here's the same one with those removed.

Oops. This one fixes a mistake in the @code JSDoc for Drupal.attachBehaviors. See #13 for my note about local settings param.

With #16, except:

1) I've had 0 experience with all the work that went into local settings. I think it's great, but I don't think I'm the right person for the job of figuring that out and figuring out how to test it. Settings data, element data, and AJAX data seems to be merged and massaged all over the place. It probably makes sense to someone, but not to me yet.

2) If TwoD can figure out a way around the need for 'dom_move' triggered detach/attach, great! The reason wysiwyg for D6 breaks when you row-swap (at least on tinymce on firefox does) is the IFRAME problem in the JSDoc above Drupal.attachBehaviors(). I had to do some nasty hacking to make D6 wysiwyg work within a cck multi-valued field: http://drupal.org/project/wysiwygcck, and I know from my experience on that module that this would fix it.

subscribing

The issue sun mentioned earlier is probably #595654: AJAX command 'settings' broken, without that patch, these fixes will have little effect.

In the patch below, I've just added the settings arguments to the last patch. I followed the (var settings = ... pattern in existing calls. Technically, we could skip the || Drupal.settings part but having all of it there might make things clearer.

After spending half the night testing this using both the iframe module and a modified version of Wysiwyg module I think this does a pretty good job fixing this issue. I think this concept is RTBC, but there are a few details I'd like to bring up before actually marking it as such.

If I understood the above discussion, the 'reason' argument was added to give behaviors a way to determine if they always need to do a full detach/attach, and Wysiwyg was the main target because of the editor iframes. As far as I can tell, iframe-fields always need to fully detach/attach on 'dom_move', no easy way around that. Other field types won't be hurt by the swapping routine as they keep all relevant states, so 'dom_move' could be dropped in favor of a full detach/attach.

I like the ideas about the pre form serialization step ('data'), anything submitting forms can rely on widgets 'dumping' their raw data on their own and in time. It seems to work pretty well too as is. We could even put this in its own 'serialize' behavior and there would be three distinct operations; attach, serialize and detach. In its current form, there could of course also be other reasons added by other modules, but they can't really control what responds to which reasons without further hacking. Just a thought...

Sorry for my delay [and ranting] on this guys, life seems to have its way to get in the way hehe.

Oh, there's an attach button too...

Thanks for the work on this and its dependency issue, TwoD!

I'm fine with keeping it as-is with respect to the "reason" parameter. However, if there's a concern that it creates unnecessary complexity for people writing behaviors to understand, then given what you said, I can envision your other thought as working: drop "reason" and add "flush" (I'm open to a better word choice, but that's what I've come up with so far), so we would have a Drupal.flushBehaviors(), and each behavior could implement an optional "flush" function. Two things would need to be true for this to work:

1) That developers writing behaviors that implement a "detach" function are ok with that being called before a dom move -- i.e., that we're not anticipating some kind of behavior that needs to detach before the dom element is fully removed, but that wouldn't want to do so before being row-swapped within tabledrag. I guess I'm just paraphrasing what sun said in #21.

2) That behaviors can implement a "flush" without needing an additional callback when the form is done submitting (i.e., we could drop the attach calls from success() and error()). This, I actually think should always be true. Even in the case of a hypothetical editor that can only save its data by fully unloading, I suppose there's no reason why it couldn't fully unload and fully reload within the same flush() call. Besides, I think both tinymce and fck support a way to flush data quickly and without flicker, and any editor that doesn't support that, perhaps we just shouldn't be using.

If we're ok with both of these conditions, I can definitely see the appeal of 3 methods instead of 2 methods that each take an extra parameter that can be 1 of 3 values.

Hmmm, that's right... If a behavior fully detaches before a swap it should clean up after itself, including all settings. Unless we temporarily store a deep copy of the settings during the swap it might not be possible to attach something again. The attach behaviors should not take harm from being fed data once 'deleted' by detach behaviors, but is that something we could get away with? It's probably more fair to let the developer decide what to do in this situation. The swap is a special case, so how special do we make the workaround?

I think I'm fine with keeping the 'dom_move' reason as a flag to partially or temporary detach/suspend a widget as storing a deep copy of the settings and making swapping a behind-the-scenes-operation might lock it in too much for the developer. We might encounter other situations where similar to tabledrag might need to swap/move elements, and then it would be pretty handle to be able to signal a partial detach.

Case 2 should indeed be true. I don't know of an editor not capable of flushing the contents or be forced to do so without fully detaching (editors that don't want to detach at all seems more common). Editors can't on the other hand detach and attach in the same call if they rely on iframes or other asynchronous operations, but that we should not have to consider since they are able to update the original field anyway.

@sun: I'm probably missing something, but I'm not following your point #1. What's wrong with the attached patch?

It also occurs to me that attach() doesn't need a trigger, because detach()'s trigger-specific implementation can setup the needed state variables to track the element state, and attach() can react to the element state.

OK. Do you agree, however, that only detach() needs the trigger, and attach doesn't? That's how this patch is structured.

Actually, this one restores the attach() done in success() and error(). My thought is that a behavior that implements the serialize trigger correctly won't need to do anything on attach in these places. However, a simple behavior that implements a detach without taking the trigger into account will need an opportunity to reattach.

Let me know if you want them back in, and I'll do it. I can't think of a use-case where the attach() function couldn't figure it out, given what the detach() function ended up doing, and it saves us from figuring out a name for the trigger that success() and error() would pass.

Basically, I don't think the attach() should care what triggered it, it should only care about what is the element's current condition, and what needs to happen for the behavior to be fully operational.

#29 looks good to me too.

Subscribing

Hm. I don't really get what this patch does, but all the smart JS people want it. :P Therefore, committed to HEAD. ;)

Wohoo! Thanks webchick! What it does is pretty simple. Our JS widgets can now 'jump out of the way' when the user [re]moves elements on the page and translate any changes made, temporarily stored in the widget's state, back to the original element's state.
It would not be so easy for the code doing the [re]moving to handle all kinds of widgets, so now it doesn't have to. =D

webchick: I suspect you get much more than you let on, but if you're interested in the non-techy translation of #38 to share with people you talk to about this, this fix means that unlike in D6, WYSIWYG editors in D7 won't break when used on "unlimited" values text fields.

FYI: there's a follow-up issue related to the code that was added in this one: #825318: Drupal.attachBehaviors() is called an extra time, unnecessarily and for document context, when there is no form.