Allow behaviors to detach from AHAH/AJAX contents

Better title?

Thought about this. The benefit from having separate callback functions for attaching/detaching are:

1) Performance: Not every attached behavior needs to be detached. So we can limit the invocations to functions that really need to detach.

2) Logic: Besides not having a big, nasty switch/if statement in each and every behavior, the actual logic of detaching behaviors is probably almost completely different to the attaching behavior in most cases (besides the CSS class name). Wysiwyg API is probably the only existing example currently; if you look at the attaching behavior in Drupal.wysiwyg.attach.tinymce(), I wouldn't really want to add another level of complexity to this function.

#4 looks much cleaner, but then again, I'd ask where's the difference between

Drupal.behaviors.myThingy.attach
Drupal.behaviors.myThingy.detach

and

Drupal.behaviors.attach.myThingy
Drupal.behaviors.detach.myThingy

I didn't think of this variant in the first place, since I wanted to keep API changes minimal. So, if Drupal.(de|un|mis)behaviors or similar won't work out at all (which would not require changes in all modules), then the last variant would be my favorite, because we could simply iterate over all attaching and detaching behaviors separately (and fast). We should bear in mind that probably only a small minority of all behaviors needs to detach from dynamically loaded contents.

Oh nice. Indeed, that's probably the most clean way to make detaching methods completely optional.

Tested attached patch with a custom module and it works as expected:

Drupal.behaviors.attachCustom = function(context) {
  ...
}

...is invoked just like before, and

Drupal.behaviors.custom = {
  attach: function(context) {
    ...
  },
  detach: function(context) {
    ...
  }
}

is also invoked properly. :)

I'm not sure whether I agree with that. Sure, it's more consistent - but turning all behaviors unnecessarily into objects with just one method can also be considered as cruft.

Anyway, leaving this decision to core maintainers.

Attached patch changes all behaviors throughout core.

Can someone set this to RTBC, please?

OK. Hopefully, last re-roll now.

Attached are the 3 variants of this issue.

1) Backwards compatible API change, optionally introducing 'attach' and 'detach' methods.

2) API change, introducing 'attach' and 'detach' methods, by moving function contents of all existing behaviors into the 'attach' method.

3) API change, introducing 'attach' and 'detach' methods, by altering function/method declarations only (same as 2, but different syntax leading to less changes).

To test Drupal.detachBehaviors(), here's a simple test module, which invokes the function upon clicking on any link.

Patch still applies cleanly and works like a charm. This issue is on the list of killer feature enablers a.k.a. better WYSIWYG support: http://groups.drupal.org/node/6492/summary

I'd set it to RTBC if it wasn't my own patch. Already tested and triple-checked a few times now. I really cannot see any reason for holding this off.

Added section to module upgrade docs: http://drupal.org/node/224333#drupal-behaviors

Sorry, but I have to re-open this issue and bump this to critical for 2 reasons:

1) Many JS gurus in here.

2) The problem:

As of now, Drupal core does not implement detaching behaviors yet, so we actually have no real-world code to test Drupal.detachBehaviors(), neither in core, nor in contrib. As some of you might have noticed, I heavily worked on Dreditor recently, which implements a pure, jQuery-based JavaScript application that only works on drupal.org. It is re-using Drupal's JavaScript environment on drupal.org, and because of that, many architectural concepts of Drupal have been adapted. Dreditor builds new content on the fly, but also removes and thereby tries to properly destruct content on the page. It is not yet using detaching behaviors, but while working on it, I found a major issue with the current design of detaching behaviors we added here:

We are always passing a regular DOM element as context. Detaching behaviors are supposed to detach from all elements in the context.

However, you cannot tell: Detach only from elements 24 + 25 + 32 + 48 (replace those numbers with arbitrary DOM elements), because currently, the passed in "context" has to be a surrounding container, i.e. a container holding many, arbitrary elements. That means, each detaching behavior will run on all elements within the context. There is no way to tell that we only want to detach from X and/or Y only, because all a detaching behavior gets to know is the (global) context.

Admittedly, this problem only applies to edge-case situations and not the common situation, in which you have a "context" that contains a certain DOM element tree in the current document only (regular AHAH/AJAX interface actions that add and remove contextual content).

The problem arises when a behavior acts on multiple slices of a context.

Let's consider the following markup:

<ul class="menu">
	<li>one</li>
	<li>two</li>
	<li>three</li>
</ul>

Drupal.attachBehaviors(context) is invoked, where context == document:

<ul class="menu">
	<li class="behavior-processed">one</li>
	<li class="behavior-processed">two</li>
	<li class="behavior-processed">three</li>
</ul>

Now, some nifty JavaScript wants to do $('.menu li:first, .menu li:last').remove() to replace "one" and "three" with something completely different. It thereby should invoke Drupal.detachBehaviors(). But since we only have context == document, or possibly context == ul.menu, our detaching behavior runs:

Drupal.behaviors.behavior = {
  attach: function(context) {
    $('.menu li:not(.behavior-processed)', context).addClass('behavior-processed')...
  },
  detach: function(context) {
    $('.menu li.behavior-processed', context).each(...);
  }
};

The result: Our behavior is detached from all list items in the menu, although only the first list item is about to be removed.

The solution to this issue sounds quite simple, but is yet another major change to Drupal's behaviors: Always pass a jQuery object (which can contain certain elements only) as context (instead of one, plain, surrounding DOM element).

Apparently, for jQuery itself it is irrelevant whether context is a DOM element or a jQuery object.

And actually, a similar thread on the development mailing list discussed what "context" should be. To my knowledge, some contrib modules already get it wrong (passing a jQuery object instead of a DOM element).

However, there are two issues with a dirty "resolution" that just assumes anything:

a) The proper variable name for a context that is a jQuery object is $context.

b) If behaviors accept a $context instead of a context, then they can safely assume that the following code will work:

Drupal.behaviors.behavior = {
  attach: function($context) {
    $context.find('.menu li:not(.behavior-processed)').addClass('behavior-processed')...
  },
  detach: function($context) {
    $context.find('.behavior-processed').each(...);
  }
};

Note that this may only apply to detaching behaviors, because attaching behaviors can simply check for the class "behavior-processed" to decide whether they need to run. Detaching behaviors can't rely on that.

To summarize: What we added here (detaching behaviors) is nice and badly needed. However, we have a fundamental design flaw in the function signature of attaching/detaching behaviors.

ok, that actually means we are passing a jQuery 'context', which is not a jQuery object, but either a DOM element, an array of DOM elements, or a jQuery object, which are all properly handled by jQuery internally.

To detach from certain elements only, those elements need to be either passed in a jQuery object or a DOM element array (i.e. aforementioned jQuery context).

That works.

We could clarify this in the documentation, but I'm not sure whether that belongs into this issue. Hence, closing.