Enable AJAX by making all behaviours reattachable

Definite +1 on this idea.

However, I think we need to go further in this. All autoAttach handlers should have an argument that specifies which content they should process (note that this does not negate the use of consistent 'foo-processed' classes). Also, all autoAttach handlers should be registered in a central array, and we should provide a function to invoke them.

Then, any piece of code that inserts new mark-up in the page can simply call this centralized handler. By only processing the new content (through the extra argument), we gain a speed bonus too. In drupal.js, we would set a single $(document).ready() event that invokes the centralized handler on the entire document.

That way, adding a new behaviour is as simple as e.g. Drupal.autoAttach(function (content) { ... });.

I'm not a big fan of extending the jQuery namespace with generic names like "behaviour". Unless this is a generic, separate jQuery plug-in, it deliberately misuses the namespaces we have.

Great work!

Some suggestions:

I don't like the need of adding a class to every processed element, instead of this, we can pass the context (the content we have just added to the page, or its parent element) as an argument to the attach methods, so them can pass it back when calling $() function. Currently the attach methods can use this since attachBehaviors change the context to the element object when calling this.apply(elem). But would not make sense to pass this explict as an argument to the attach functions, e.g. using this.call(elem) instead?

I agree with Steven that this should extend the Drupal namespace, this way we can remove the if (Drupal.jsEnabled) checks and add a single check to the attachBehaviors function.

Also, I wonder if we could remove the $.registerBehavior() calls and build the behaviors array from php, in a similar way we do with Drupal.settings.

Also, I wonder if we could remove the $.registerBehavior() calls and build the behaviors array from php, in a similar way we do with Drupal.settings.

I thought a bit more about this and come up with another approach. We can just instantiate attach methods extending Drupal.behaviors object, so instead of

Drupal.mycoolBehaviorAttach = function() {
  $('.mybehavior').doSomeStuff();
}

it would be

Drupal.behaviors.mycoolBehavior = function() {
  $('.mybehavior').doSomeStuff();
}

then attachBehaviors would loop through Drupal.behaviors object.

Also I forgot to mention that you patch has a typo in batch.js, a space on the attach function Drupal.batch Attach.

I've changed the patch with my suggestions in #7 and #8.

I did some basic testing. Autocomplete, textarea, tableHeader, collapse, teaser. color and upload seems to be working fine.

If Steven has the time it takes, I'll let Steven carry this patch home. He started reviewing it already, and would be the best person for it anyway. :)

I did some changes in system.js to remove the 'processed' class stuff from dateTime behavior (tested and works). Also I'm wondering if we could make the clean url check reatachable too, think in a full AJAX administration interface.

The patch is big mainly due to changes in color.js indentation.

As far as I can tell, the color.js code isn't actually re-attachable in the last patch. It inserts the color picker without looking at existing marker classes.

However, I think the idea of making everything a behavior (even things like color.js, which only work on one particular element) makes sense in the long run. This gives everyone a simple 'no thought required' mechanism for making inserted ajax html 'just work', and forces consistent usage of 'processed' marker classes. This is a good thing, especially because your behaviour code becomes completely disconnected from specific events, like "onready".

Since you still need to call drupal_add_js() directly or indirectly for every behavior you actually use, there is no real performance hit either.

The only tricky situation would be where an ajax request loads HTML which uses behaviours that weren't part of the original page. In that case, the JS for them will not be loaded. However, I think such cases will be very rare and those problems would easily be solved by adding the right drupal_add_js() calls to the original page, using form_alter or something.

I've worked on another approach to make behaviors re-attachable, instead of looking for 'processed' marker classes, my approach pass the 'context' argument to the behaviors functions, then these functions can pass this back when calling $().

In other words, instead of writing things like:

Drupal.behaviors.myCoolBehavior() {
  $('.mycoolbehavior:not(.mycoolbehavior-processed)').doSomething().addClass('.mycoolbehavior-processed');
}

We simply pass the 'context' to jQuery:

Drupal.behaviors.myCoolBehavior(context) {
  $('.mycoolbehavior', context).doSomething();
}

This approach is a lot cleaner than using the 'processed' marker classes IMO.

I forgot to add = function to the examples above, the correct examples are:

In other words, instead of writing things like:

Drupal.behaviors.myCoolBehavior = function() {
  $('.mycoolbehavior:not(.mycoolbehavior-processed)').doSomething().addClass('.mycoolbehavior-processed');
}

We simply pass the 'context' to jQuery:

Drupal.behaviors.myCoolBehavior = function(context) {
  $('.mycoolbehavior', context).doSomething();
}

The default context is 'document'. Here is an example (untested) of how re-attaching would work.

/**
 * Implementation of hook_menu().
 */
function myajaxmodule_menu() {
  $items['myajaxmodule'] = array(
    'page callback' => 'myajaxmodule_js',
    'type' => MENU_CALLBACK,
  );
}

/**
 * My AJAX callback.
 */
function myajaxmodule_js() {
  $form = drupal_get_form('some_form');
  $files = drupal_add_js(NULL, NULL, 'header');
  print drupal_to_js(array('files' => $files, 'data' => $form));
  exit;
}

/**
 * My AJAX callback.
 */
Drupal.myajaxmodule.callback = function(data) {
  // Add new javascript files.
  jQuery.each(data.files, function() {
    Drupal.myajaxmodule.addJs(this);
  });
  // Append the loaded content.
  var context = jQuery('.page').html(data.form);
  // Attach behaviors to the loaded content.
  Drupal.attachBehaviors(context);
}

/**
 * Load a js file.
 */
Drupal.myajaxmodule.addJs = function(path) {
  if (!Drupal.myajaxmodule.checkJs(path)) {
    $('head').append('<script src="'+ path +'" type="text/javascript"></script>');
  }
}

/**
 * Check is a js file is already loaded.
 */
Drupal.myajaxmodule.checkJs = function(path) {
  var ret = false;
  $('head script').each(function() {
    if (this.src == path) {
      ret = true;
    }
  });
  return ret;
}

I think addJs and checkJs would be good additions to drupal.js, however this approach will not work with the js aggregation patch, so perhaps we can set and array, e.g. Drupal.loadedBehaviors, from php so we know what js files has been loaded with the current page?

The code above wouldn't work I think... you'd need to wait until the script has loaded before attaching behaviours.

However, I think marker classes should stay. For one, it lets you easily detect other behaviours' presence reliably. For another, it makes it easier to work around certain annoying browser bugs that require repeated attachment (e.g. with the collapsed fieldsets).

+1 to the general idea

The code above wouldn't work I think... you'd need to wait until the script has loaded before attaching behaviours.

I just discovered jQuery ships with a method for including js files via xmlHttpRequest. The method is $.getScript(url, callback).

Do you think it's worth adding some functions to drupal.js for automating the process of loading .js and .css files, as well as merging 'settings' loaded via Ajax to Drupal.settings object?

However, I think marker classes should stay. For one, it lets you easily detect other behaviours' presence reliably. For another, it makes it easier to work around certain annoying browser bugs that require repeated attachment (e.g. with the collapsed fieldsets).

I don't see how the 'processed' class and the 'context' argument approaches can coexist. Sounds somewhat redundant for me. Can you elaborate on the bugs you mentioned?

I'll re-roll the patch as soon as possible. Assigning to myself.

Comments are welcome :)

Ok, I've re-rolled the patch with the following changes:

Did some basic testing on every 'behavior'.

Syncing with HEAD.

Needs a re-roll after the js aggregator patch.

Here is the re-rolled patch.

As per Steven's and Nedjo's request, I've added back the 'processed' classes.

The color.module behaviour is not really re-attachable since it can only be used once (it uses id's). Better would be just to check if the known target element has been processed at the start, and then do nothing (return;) if so.

Subscribing.

While the last couple of weeks, I spent a fair amount of time writing/practicing jQuery code, and also read up on AHAH/behaviours.

In general, this patch looks good, but I'm thinking it would be useful to formulate some guidelines here. For example, the way you postfix IDs with '-processed' to indicate that they are processed, the behaviors-namespace and the way Drupal picks up these behaviours. That's probably something we'll want to document.

Some of this should probably go into a handbook page, but I'd also like to see us improve the inline documentation. Specifically, I suggest we describe the overall design in the PHPdoc comments of Drupal.attachBehaviors.

I spent 30 minutes looking at the code trying to figure out how Drupal picks up behaviors and to discover some of the patterns in how behaviors are written. Some inline documentation would have saved me 20 minutes so let's make sure that other people can pick this up a little easier.

Thus, expand the documentation of attachBehaviors and explain briefly (i) what we mean by 'behaviors', (ii) why we are using behaviors, (iii) how Drupal picks up behaviors so people can figure out how to tab into the system and (iii) what to be careful about. It can be brief and to the point but at least, it gives people a starting point. I can probably be done in 5 sentences or so. We can then use the Drupal handbook for more verbose explanation with examples.

I'd move this function up in the .js file so it's easy to spot.

Other than that, the patch looks RTBC. Great work, nedjo.

I'd like to commit this patch too: http://drupal.org/node/154398. Might be useful to see if there any conflicts.

The patch and the new JSDOC ;) looks great! It's breaking the JS teaser splitter, though. I'm looking into it.

Hm. Or maybe it doesn't. Weird, it seems to work now after reapplying the patch.

Ok, did one round of reviewing this.
I tested pretty much all of Drupal's JavaScript behaviours (fieldsets, farbtastic, autocomplete, file attachments, openID login, teaser splitter, ..) and did not found one single problem. The new comment for Drupal.attachBehaviors is really nice and extensive now. This is ready to go. Great work!

Committed to CVS HEAD. Thanks nedjo.