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.
Comment | File | Size | Author |
---|---|---|---|
#6 | ajax_second_call_939860_06.patch | 346 bytes | rfay |
ajax.js_.debug_.txt | 1.56 KB | yched | |
ajax_2nd_call.patch | 354 bytes | yched | |
Comments
Comment #1
yched CreditAttribution: yched commentedThe 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.
Comment #3
rfaysubscribe
Comment #4
yhager CreditAttribution: yhager commentedsub
Comment #5
bleen CreditAttribution: bleen commentedpatch is failing because of your file paths ... a/ and b/
Comment #6
rfayHere's yched's #0 just rerolled with git diff --no-prefix so it should pass the bot.
Comment #7
swentel CreditAttribution: swentel commentedsubscribe
Comment #8
agentrickardWow. Can it be that simple? /me runs to test.
Comment #9
agentrickardFixes the described problem. Nice.
Comment #10
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD.
Comment #11
yched CreditAttribution: yched commentedThanks 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.
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedI'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.
The reason that issue surfaced this bug, is because it changed the "merge" flag on the returned settings, such that this ajax.js code:
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!
Comment #13
yched CreditAttribution: yched commentedThks 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).
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedBecause 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.
Comment #15
jurgenhaasI've had the same issue, thanks for fixing it.