Split out from #934234: AJAX requests during drag-and-drop on the "Manage display" tabs in the Field UI breaks the page, which got a little confused.

In short : on some pages with ajax interactions, the second ajax call breaks all behaviors.
Can be seen with poll.module's "More choices", or the ajax interactions on Field UI's "Manage display", but *not* for Field's "Add another item" button - more on that below.

Let's consider the poll case, which is a bit simpler to follow than field UI's "Manage fields" - steps to reproduce :
- enable poll.module
- go to node/add/poll, fill in some text in the existing two 'Choice' textfields
- Press "More choices" once : OK, one more textfield shows up, tabledrag handlers are present
- Press "More choices" once more : one more textfield shows up, but tabledrag handlers are gone

What happens is :

1 - On initial page load, ajax.js initializes an ajax object associated to the "More choices" button.
It starts without an ajax.settings property (i.e ajax.settings == undefined)

2 - 1st ajax call : ajax.success() is called, performs its job fine, and then does :

  (end of Drupal.ajax.prototype.success :)
  // Remove any response-specific settings so they don't get used on the next
  // call by mistake.
  this.settings = { };

3 - No new ajax object is created, *the same ajax object is still associated to the "More choices" button*.

4 - 2nd ajax call : ajax.success() is called :
Drupal.ajax.prototype.commands.settings() merges incoming settings into Drupal.settings - OK
Drupal.ajax.prototype.commands.insert() calls behaviors on the newly inserted HTML, with the following settings :

var settings = response.settings || ajax.settings || Drupal.settings;

For the 1st ajax call, response.settings and ajax.settings were null or undefined, so this evaluates to Drupal.settings - OK
For this second ajax call, however, ajax.settings is now { } (empty object), and the above evaluates to { } : behaviors are called with empty settings instead of Drupal.settings, and tabledrag behavior is not reattached.

The bug does not affect the "Add another item" button on regular, multivalued Fields because, in this case, a new ajax object is created for the "Add" button after each ajax request, and thus, ajax.settings is always undefined and response.settings || ajax.settings || Drupal.settings always evaluates to the expected Drupal.settings.

That's all I have so far. I didn't search yet why in some cases a new ajax object is created for the triggering element, and in some other cases the same object is reused (which causes the bug). I also have no idea either why the bug appears now - I know for sure that the ajax interactions on "Manage display" worked OK a couple weeks ago - however, I cannot really pinpoint a new behavior introduced in, for instance, #561858: [Tests added] Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework.

The naive patch below fixes the bug for me, but it's very possible that this is not the correct fix at all.
I'm also attaching a patch that adds the debug messages I used to trace the behavior above.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

The bug involves js code only, so AFAICS, it's not testable.
The PHP side is fine and the ajax request performs normally, it's just how the results are processed on the js side that seems buggy.

Status: Needs review » Needs work

The last submitted patch, ajax_2nd_call.patch, failed testing.

rfay’s picture

subscribe

yhager’s picture

sub

bleen’s picture

patch is failing because of your file paths ... a/ and b/

rfay’s picture

Status: Needs work » Needs review
FileSize
346 bytes

Here's yched's #0 just rerolled with git diff --no-prefix so it should pass the bot.

swentel’s picture

subscribe

agentrickard’s picture

Wow. Can it be that simple? /me runs to test.

agentrickard’s picture

Status: Needs review » Reviewed & tested by the community

Fixes the described problem. Nice.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

yched’s picture

Priority: Critical » Major
Status: Fixed » Active

Thanks for being so quick, folks, but as I wrote in the OP :
"The naive patch below fixes the bug for me, but it's very possible that this is not the correct fix at all."

It would be good to hear from the ajax gurus, at least to get an "a posteriori" approval ?
Setting back to active, and bumping out of the critical queue.

effulgentsia’s picture

Status: Active » Fixed

I'm not really a guru with respect to all the intricacies of local, global, per-response, per-ajax-object settings management within ajax.js, but #6 definitely seems like a correct fix to me.

I know for sure that the ajax interactions on "Manage display" worked OK a couple weeks ago - however, I cannot really pinpoint a new behavior introduced in, for instance, #561858: [Tests added] Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework

The reason that issue surfaced this bug, is because it changed the "merge" flag on the returned settings, such that this ajax.js code:

settings: function (ajax, response, status) {
    if (response.merge) {
      $.extend(true, Drupal.settings, response.settings);
    }
    else {
      ajax.settings = response.settings;
    }
  }

is now running the "if" portion, whereas before, it was running the "else" portion, and masking the bug at the end of Drupal.ajax.prototype.success().

Anyway, thanks for the fix!

yched’s picture

Thks eff, makes sense. Setting back to critical, for history's sake.

What is still not clear to me is why, after an ajax request, in some cases ajax.js creates a new ajax object for the triggering element (Field "Add more"), and in some other cases the same object is reused (Poll "Add more", Field UI "Manage display" - which raised the bug).

effulgentsia’s picture

Priority: Major » Critical

Because in the case of Poll module, the triggering button remains on the page, and only the choices are returned by the AJAX response, but in Field "Add more", the whole field, including a new "Add more" button, is returned and replaces the old field content.

jurgenhaas’s picture

I've had the same issue, thanks for fixing it.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.