Problem:
In at least the User Add/Edit forms, if I add an ajax triggering checkbox to the form, it's "changed" state will not be reflected when the form rebuilds.

Test for Yourself:
Add the following code and see for yourself:

/**
 * add an ajax_test checkbox to every form
 */
function MY_MODULE_form_alter(&$form, &$form_state, $form_id){
  $form['ajax_test'] = array(
    '#type' => 'checkbox',
    '#weight' => -100,
    '#title' => t('Ajax Test'),
    '#description' => 'This checkbox was inserted by "hook_form_alter" to test the #ajax property. Checking it will trigger an ajax form rebuild, after which the checkbox state should be preserved.',
    '#ajax' => array(
      'callback' => 'form_alter_ajax_test_update',
      'wrapper' => 'ajax-test',
		),
    '#prefix' => '<div id="ajax-test">',
    '#suffix' => '</div>',
  );
}

/**
 * callback function for ajax_test
 */
function form_alter_ajax_test_update($form, $form_state){
	return $form['ajax_test'];
}

Now visit the User Add page or any User Edit page, and you will see that the checkbox does not stay checked. If you use dpm($form_state) you can see that $form_state['input']['ajax_test'] does not reflect the change.
Now visit any other form (as far as I know) and it will work properly.

I found this bug while trying to alter the $form['account']['status'] checkbox (on user_register_form and user_profile_form) to trigger adding and removing the other account info form elements.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rfay’s picture

Category: bug » support

It's a bit unusual for a form element to be replacing itself. Can you give me an idea why you're doing this? Normally you would use a #ajax element to replace something else. There are a number of examples in the Examples project's AJAX example.

manimejia’s picture

Category: support » bug

I really do think this is a bug ... so please bear with me if my example was a little wierd.
Here's a better one, and still $form_state['values']['ajax_test'] does not reflect the checkbox on form rebuild of the User add or edit pages.
ALL OTHER forms I've tested (the code bellow) show the proper value being sent to $form_state.

I've set the "ajax replaced" element to be just markup reporting on the value of $form_state['values']['ajax_test'].
Please run this code, and see for yourself:

/**
 * add an ajax_test checkbox to every form
 */
function MY_MODULE_form_alter(&$form, &$form_state, $form_id){
 	$form['ajax_test'] = array(
    '#type' => 'checkbox',
    '#weight' => -100,
    '#title' => t('Ajax Test'),
    '#description' => 'This checkbox was inserted by "hook_form_alter" to test the #ajax property. Checking it will trigger an ajax form rebuild, after which the $form_state should store it\'s value.',
    '#ajax' => array(
      'callback' => 'form_alter_ajax_test_update',
      'wrapper' => 'ajax-test-value',
    ),
  );
  $form['ajax_test_value'] = array(
    '#weight' => -100,
    '#markup' => '$form_state[values][ajax_test] => '.$form_state['values']['ajax_test'],
    '#prefix' => '<div id="ajax-test-value">',
    '#suffix' => '</div>',		
  );
}

/**
 * callback function for ajax_test
 */
function form_alter_ajax_test_update($form, $form_state){
	return $form['ajax_test_value'];
}
manimejia’s picture

P.S.
This only happens when the #ajax enabled checkbox has been added via hook_form_alter() or hook_form_FORM_ID_alter().

rfay’s picture

I'll try to take a look at this tomorrow.

Note that altering the node form with #ajax is a little tricky at best (and always has been). See http://www.drupaler.co.uk/blog/ahah-node-forms-and-select-lists/434.

rfay’s picture

Your code in #2 works fine for me on firefox/Apache/Linux.

What browser are you using?

I do see some *significant* issues at http://example.com/node/add/article though when using IE7. IE thinks the AJAX response is a file and offers to save it.

Note that #557284: AHAH/AJAX bindings do not work on checkbox or radio in IE6/7/8 (and now IE9) just went in the other day and I think we should be suspicious if there is some change of behavior.

manimejia’s picture

Thanks for working with me on this.
I'm using Chrome on a MacBook.
But the problem is not with my browser.

The problem (as I see it) is that the stored state of the Ajax Test checkbox(in $form_state['values']['ajax_test']) is sometimes not acurate in representing the checkbox. This is on a PER FORM basis. On User Add, and User Edit forms it ends up being NULL value EVEN IF THE CHECK BOX LOOKS CHECKED. On other forms the stored state properly represents the Ajax Test checkbox. This difference is clearly visible in the markup output of $form['ajax_test_value']. You can also check this peculiarity by simply running dsm($form_state) in the $form_alter function.

rfay’s picture

I wasn't suggesting that something was wrong with your browser - I was suggesting that this is a browser-dependent problem.

So I put your code on http://d7.randyfay.com and sent you credentials via email. Please provide an exact "how-to" to make the problem happen there.

Thanks,
-Randy

manimejia’s picture

Thanks for your help.
Yes I know you were suggesting it might be a browser based problem.
With this test site, at least we can communicate clearly to reproduce the problem.
Thanks for setting this up.

I noticed that the markup in $form['ajax_test_value'] form element (as it is in #2 comment) does not show up along with the checkbox. It is essential for debugging that this markup, or output from dsm($form_state), be displayed on the same page, and reflect the latest form state.

Let's try it exactly as it appears in #2.
(or am I missing something?)

M.M.

manimejia’s picture

Randy,
I just followed your lead and tested the above code from comment #2 (as it's own module called ajax_test) in a clean install of Drupal 7.0-rc2 on my own machine. Indeed the problem persists. AND what's more (just added weirdness) is that on some forms the $form['ajax_test_value'] element renders above the checkbox and some forms it renders bellow the checkbox. (I don't see a patern to this yet.) ALSO I discovered that the SAME problem will show up on node add/edit forms.

AGAIN, the problem is that when the $form['ajax_test'] checkbox is checked (or unchecked) the $form_state does not store it's changed value. This is demonstrated by the markup rendered in $form['ajax_test_value'].

Here's a list of forms that I know present this problem (on a clean D7 install):
- admin/people/create
- user/1/edit
- node/add/article (along with a warning for file_managed_file_validate() ... )

Here are some forms (to use as reference) that do NOT present the problem
- admin/people
- admin/content

I hope you have duplicated my test successfully, and found something interesting.

manimejia’s picture

FileSize
1.87 KB

Following Randy's lead as in #557284: AHAH/AJAX bindings do not work on checkbox or radio in IE6/7/8 (and now IE9) ...
I've attached the module I made to test this problem.
Simply download, and enable to add an "Ajax Test" checkbox to every form on a D7 site.

rfay’s picture

I can't go any farther with this unless you give me a detailed explanation of how to demonstrate the perceived problem on d7.randyfay.com.

manimejia’s picture

Step By Step.
Here's how to recreate the problem, and what to look for.
(with screen shots too!)

a) Download and enable the UPDATED module I created, called "ajax_test". (attached to this comment) It has the same code found in #2 comment, and adds two elements to every form. $form['ajax_test'] is a checkbox that triggers an #ajax reload of the form. $form['ajax_test_value'] is the "ajax replaced" element, and renders (as markup) the $form_state value of the checkbox.

b) Visit /admin/people in your test site. The form should have these new elements rendered. The second element, $form['ajax_test_value'], (which sometimes renders above the first, for some strange reason) is what we look at to witness the problem. It outputs the value of $form_state['values']['ajax_test'] checkbox, which SHOULD match the actual checkbox. On this form you will see that it does. Check and uncheck the checkbox a number of times until you are convinced that the reported value and the checkbox are in sync. (see screen shots a and b)

c) Visit admin/people/create in your test site. This form as well should have the new elements rendered. Check the checkbox a few times here, and see that the $form_state, as reported by $form['ajax_test_value'], does not store the checked state of the checkbox.(see screenshot c)

The issue at hand is that any module is not (in a practical sense) able to use hook_form_alter() to add #ajax enabled checkboxes or radios to certain forms. (listed in comment #9) This is because the $form_state on these forms does not store the value of the checkbox on form rebuild. THIS is what the ajax_test module proves. So whatever the checkbox is supposed to do (ie: load additional form elements) will not get done.

I have my own reasons for needing to alter the user add/edit form. I would love to get into them some time, but I think this is a separate issue that stands on its own. It's an API breaker and should be resolved.

I hope this clears up any confusion, and you are able to successfully duplicate my test.
M.M.

rfay’s picture

FileSize
836 bytes

Thanks for the excellent description.

I've attached an updated ajax_test test module.

It seems that on the user forms $_POST on the ajax submission does not even include the ajax_test value (and it does on all the other forms). So I'm thinking this is a problem on the javascript end, but have no idea how that can be.

manimejia’s picture

A successful hunt ...
So there are three forms that I see the problem on, in a clean d7 install: admin/people/create, user/1/edit and node/add/article; but node/add/page does not seem to present the problem.

In looking further, and just commenting out sections of code till I see a difference, I found the culprit in both cases to be a 'file' form element. In the User forms it is the $form['picture']['picture_upload'] element. Even if one does not have "access" to the file element (ie: if user pictures are not enabled) the element (somehow) still presents a problem. In the node form case the culprit is the file upload element on the image field, which is only present on the article node type.

I don't know yet anything further, but if I comment out these 'file' elements the problem disappears.

rfay’s picture

I've confirmed that on admin/people/create, regardless of form altering, a checkbox triggers a form submission, but that checkbox's value does not get sent in the $_POST. I edited user.module to just add an element with #ajax into it, and it does not behave correctly.

This remains true even if tabledrag.js, user.js, progress.js, and jquery.cookie.js are excluded (commented out so they don't load).

rfay’s picture

I also confirm that select elements don't POST on admin/people/create.

rfay’s picture

Nice work, @manimejia (#15 and #16 were crossposts).

rfay’s picture

Notes from merlinofchaos in IRC:

rfay: A file element changes the mime type of the form. I remember CTools AJAX has some special code to deal with that.
I wonder if we failed to get that part into core.
merlinofchaos, hmm. The mime type of the whole post?
Yes.
merlinofchaos, hmm. Well, at first blush it looks like #ajax doesn't work on any form that has an element of #type = 'file'
This is in ctools_ajax_render
if (!empty($_REQUEST['ctools_multipart'])) {
// We don't use drupal_json here because the header is not true. We're not really
// returning JSON, strictly-speaking, but rather JSON content wrapped in a
// as per the "file uploads" example here: http://malsup.com/jquery/form/#code-samples
echo '' . drupal_to_js($commands) . '';
}
else {
drupal_json($commands);
}
rfay: So if this is missing in core, the symptom would be...all ajax responses would be ignored.
That explains precisely what's going on when uploading files via a form and how the ajax response needs to change.
* merlinofchaos actually looks at core now, if he can figure out where ajax rendering actually got moved to.
rfay: Yeah, core does not have this capability.

Correct malsup link is http://malsup.com/jquery/form/#file-upload

merlinofchaos’s picture

Short version: file uploads use a hidden iframe, so we have to return data in the textarea for the form plugin to read it.

It's possible that we might be able to detect this condition automatically by checking this:

    $xhr = $_SERVER['HTTP_X_REQUESTED_WITH'] == 'XMLHttpRequest'; 

(That's from the malsup site). The text is unclear, however, if that is reliable. If it is reliable, then checking that could tell us whether to wrap the JSON in a textarea or not.

i.e, in ajax_render(), replace

  return drupal_json_encode($commands);

with:

  $output = drupal_json_encode($commands);
  if ($_SERVER['HTTP_X_REQUESTED_WITH'] != 'XMLHttpRequest') {
    $output = '<textarea>' . $output . '</textarea>';
  }

  return $output;
eojthebrave’s picture

Subscribe. Curious about this one but don't have time to jump in right at the moment. At least now I can find it later ...

rfay’s picture

Title: $form_state value is not preserved on #ajax update in (at least) user_register_form and user_profile_form » #ajax doesn't work at all if a file field is included in the form

This really appears to be a pretty major problem now. @manimejia, can you roll a patch based on merlinofchaos's suggestions?

manimejia’s picture

You know I tried the "return replacement" for ajax_render() suggested in #19, and I got a big sloppy ajax error, but only on the problem forms. I think we know the parameters we're dealing with, so I won't post the error here.

With further investigation I found that simply checking if ($_SERVER['HTTP_X_REQUESTED_WITH'] != 'XMLHttpRequest') ... is not enough.On forms WITH a file element, the ajax_test checkbox reports $_SERVER['HTTP_X_REQUESTED_WITH'] == NULL as does the file element. But on the other forms our ajax_test checkbox reports$_SERVER['HTTP_X_REQUESTED_WITH'] == 'XMLHttpRequest'. So I think we might need a different kind of fix.

Seems that (this is just a shot in the dark) the code that does iframe wrapping (cause I'm not familiar with this part yet) might also be affecting the ajax_test checkbox, causing it to not use 'XMLHttpRequest'.

rfay’s picture

It turns out you don't need to have a file field or anything else on the form, you just have to set

$form['#attributes'] = array('enctype' => "multipart/form-data");

Apparently whenever the enctype is set, jquery.form does some magic, and that's where things go to hell.

rfay: So far it seems like the problem is that we have no way of reliably detecting that.
One solution might be to detect the enctype on the form and set a $_POST variable
Which is more or less what ctools does now, since it looks for ctools_multipart in $_POSt
merlinofchaos, is it the enctype itself that breaks it?
jquery form plugin detects the enctype I think and if it's there it automatically uses the hidden iframe.

rfay’s picture

Title: #ajax doesn't work at all if a file field is included in the form » #ajax doesn't work at all if a file element (or enctype => "multipart/form-data") is included in the form
sun’s picture

sun’s picture

https://github.com/malsup/form/blob/master/jquery.form.js

// are there files to upload? var fileInputs = $('input:file', this).length > 0;
var fileInputs = $('input:file', this).length > 0;
...
if (options.iframe !== false && (fileInputs || options.iframe || multipart)) {

so the question is, in which situation jquery.form should actually handle file uploads?

in all other situations, we default options.iframe = false to resolve this issue

merlinofchaos’s picture

Just as an aside, I'm completely off base. ajax_deliver() should be handling the textarea for us, so that's a red herring.

rfay’s picture

FileSize
1.05 KB

Attached is a module that demonstrates the problem.

Here's what I understand so far:

* You don't need a file to make this happen. Just enable $form['#attributes'] = array('enctype' => "multipart/form-data");
* if multipart/form_data is enabled, the *triggering_element* does not get POSTed, although the other elements in the form do.

To see the POST as it ought to be (with the attached module), uncomment the line that puts in the multipart attribute.

sun’s picture

So as far as I understand, the problem is that jquery.form always/automatically inspects the entire form in order to figure out whether a file needs to be processed, and based on that, dynamically switches to the multipart behavior. Since our usage of AJAX may actually handle/submit certain form sections through #ajax only, that auto-detection fails and the wrong headers are being sent.

But we're in luck, jquery.form supports options.iframe = true/false as global all-or-nothing controller, so we merely need to figure out when we actually _need_ that jquery.form behavior.

So in ajax_deliver(), we basically need to do

if (#header == '*/multipart') #ajax[iframe] = true

whereas, it would be really helpful, if Drupal.ajax would be extended with

$.extend(ajax.options, element_settings.options);

(or similar) so that options defined in PHP like #ajax['options']['iframe'] = TRUE are actually taken into account.

sun’s picture

Status: Active » Needs review
FileSize
2.35 KB

In other words, something along of these lines. Entirely untested.

Damien Tournoud’s picture

Status: Needs review » Needs work
FileSize
3.38 KB

If you look at rfay module, it seems clear that the problem is not the detection of multipart from jquery.form.js, but that the POST data is wrong when jquery.form.js submits the iframe (see attached trace of the same state: not checkboxes are clicked, click on just the first one).

It seems that jquery.form.js copies the checkbox to the iframe before the value has been changed. This looks like a bug in that javascript for me.

Damien Tournoud’s picture

Ok, the actual issue is that we are disabling elements before calling ajaxSubmit():

  // Disable the element that received the change.
  $(this.element).addClass('progress-disabled').attr('disabled', true);

Disabled elements are apparently submitted by jquery.form when in XHR mode (is that a bug?), but not (and it actually makes sense to me) when in iframe mode.

Damien Tournoud’s picture

Oh, and the multipart detection is *also* broken in ajax_deliver(), but it works all the same (at least on Firefox). As far as I understand it, the wrapper <textarea/> is only required for compliance to HTML standards.

Damien Tournoud’s picture

Version: 7.0-rc2 » 7.x-dev
Status: Needs work » Needs review
FileSize
2.49 KB

It seems that this wrapper is actually necessary to workaround automatic MIME detection in IE.

As far as I understand all the issues described in here, this should be what we need.

rfay’s picture

Version: 7.x-dev » 7.0-rc2
Status: Needs review » Needs work

This was a regression (at least for us) in jquery.form.

v2.43 works fine (1f9901ad56ba458c8db0bffb759dbb65921891ae)
v2.44 fails in the way described in this issue. (8e1056f4030da355d1164e86ab51b1a1a7fb0cfe)

So do we revert to 2.43? (we're currently at 2.48; the latest version is 2.52)

rfay’s picture

Status: Needs work » Needs review
Damien Tournoud’s picture

Version: 7.0-rc2 » 7.x-dev
rfay’s picture

Version: 7.x-dev » 7.0-rc2
Status: Needs review » Needs work

#34 doesn't seem to work at all for me. Blows chunks when I hit one of the checkboxes in the demo module:

An AJAX HTTP request terminated abnormally.
Debugging information follows.
Path: /system/ajax
StatusText: n/a
ResponseText: &lt;textarea&gt;[{"command":"settings","settings":{"basePath":"\/","ajaxPageState":{"theme":"bartik","theme_token":"b_xe61HZK-Elz08J-pXl_6xTPtZHOAfmiVsuQ_JGMrU","css":[]},"admin_menu":{"destination":"destination=system\/ajax","hash":"a2f889d11660034fa37800f0ce1c8311","basePath":"\/admin_menu","replacements":{".admin-menu-users a":"0 \/ 1"},"margin_top":1}},"merge":true},{"command":"insert","method":null,"selector":null,"data":"\u003cdiv id=\"ajax_checkbox_replace\"\u003eajax_checkbox_1 = 0\u003cbr\/\u003eajax_checkbox_2 = 0\u003cbr\/\u003eajax_checkbox_3 = 1\u003cbr\/\u003eajax_checkbox_4 = 0\u003cbr\/\u003eajax_select_1 = one\u003cbr\/\u003e\u003c\/div\u003e","settings":null},
Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
5.76 KB

Indeed. Well, #header has no reason for being then. Let's just kill it.

rfay’s picture

#39 does work in this context at least. Yay!

Is it worth seeing what the change was in jquery.form that blew us up? (#35)

sun’s picture

This patch makes sense to me, and I think it's RTBC. However, effulgentsia should sign it off.

manimejia’s picture

Makes sense to me too ... and it seems to be a fix.
Thanks for the good investigative work, and many smart brains!

Status: Needs review » Needs work

The last submitted patch, 995854-iframe-ajax-submission.patch, failed testing.

effulgentsia’s picture

This makes sense to me, and I'm happy to see #header removed. The tests are likely failing because drupalPostAJAX() doesn't set the XMLHttpRequest header. We can certainly fix that, but are we concerned about supporting other AJAX clients, other than jQuery, that might not set that header?

Other than that, I'd like one more thumbs up from merlinofchaos, since I didn't write the original TEXTAREA code, but just moved it from elsewhere into ajax_deliver(). I think merlin has the most on-the-ground experience with this from ctools. It wouldn't be bad to get quicksketch's eyes on this too, given his file field experience, but he might be too swamped with other work right now.

effulgentsia’s picture

+++ misc/ajax.js
@@ -309,8 +309,9 @@ Drupal.ajax.prototype.beforeSubmit = function (form_values, element, options) {
-  // Disable the element that received the change.
-  $(this.element).addClass('progress-disabled').attr('disabled', true);
+  // Mark the element that received the change as disabled. We cannot actually
+  // disable it or it will not appear in the form submission anymore.
+  $(this.element).addClass('progress-disabled');

Also, I wonder what the consequences of this will be. Why were we disabling to begin with? Is it to prevent problems if someone double-clicks a button? Does this re-introduce those risks?

rfay’s picture

Assigned: Unassigned » rfay

I'm working on the test failures. If somebody's already on this, let me know.

rfay’s picture

Assigned: rfay » Unassigned

I guess I've run out of time on this tonight.

It's not just that the tests are failing, it's that the tests are failing because the AJAX commands are broken - none of them work. You can enable the AJAX Example and use the AJAX commands, or just enable the core (test) ajax_forms_test module. None of the commands work. Looking at it in firebug, they don't return any data at all. However, stepping through it on the PHP side, the $output in ajax_deliver() is fine. Hmm.

merlinofchaos’s picture

There is double click prevention now that was not in the earlier versions of the AJAX library, so disabling the element is likely not necessary at all. I see no reason we can't remove that disable.

bblake’s picture

subscribe

sun’s picture

Version: 7.0-rc2 » 7.x-dev

I agree with @merlinofchaos that the disabling of elements is no longer necessary. It is internally prevented via ajax.ajaxing already. Actually, that disabling (as well as progress bar) code dates back to ahah.js in D6.

Like @effulgentsia I'm also not sure whether entirely removing the #header feature is a good idea -- perhaps we should rather change its value from TRUE/FALSE into a string that denotes a custom Content-Type header to send; if none is set, the default behavior is used.

effulgentsia’s picture

It's not just that the tests are failing, it's that the tests are failing because the AJAX commands are broken - none of them work.

My #44 comment might have been incomplete. Since drupalPostAJAX() doesn't send the XMLHttpRequest header, with #39, the JSON is returned in a TEXTAREA, which drupalPostAJAX() is then unable to successfully decode. This can be fixed by either having drupalPostAJAX() send the header, or by having it strip out the TEXTAREA. But the basic question remains as to whether coupling a TEXTAREA wrapper to the lack of a XMLHttpRequest header is desirable. It's what is recommended in http://malsup.com/jquery/form/#file-upload, but that's very browser-centric. It doesn't make sense for non-browser clients like simpletest or Flash. Then again, we can decide that browsers are our primary concern, and that non-browser clients like simpletest and Flash should be the ones responsible for emulating browsers as needed.

However:

+++ includes/ajax.inc
@@ -470,21 +468,23 @@ function ajax_deliver($page_callback_result) {
+  if ($iframe_upload) {
     $output = '<textarea>' . $output . '</textarea>';
   }

Do we actually need the TEXTAREA wrapper at all? Note that in HEAD core, we NEVER use it. It's only used when #header='multipart', and even file_ajax_upload() does not do that. Is it possible that drupal_json_encode() removes the need for a TEXTAREA wrapper in all cases? In which case, let's get rid of it, and then our tests should pass. merlinofchaos?

Like @effulgentsia I'm also not sure whether entirely removing the #header feature is a good idea -- perhaps we should rather change its value from TRUE/FALSE into a string that denotes a custom Content-Type header to send; if none is set, the default behavior is used.

If we get rid of the TEXTAREA wrapper, then instead of a #header property, use-cases that need a different header could implement an alternate 'delivery callback' that prints a header and then calls ajax_deliver().

Damien Tournoud’s picture

Do we actually need the TEXTAREA wrapper at all?

The wrapper is actually only necessary for Internet Explorer, as documented in the patch. The lack of proper support for this in HEAD explains #999704: Image field add one more button doesn't work in IE789.

Tidying up the wrapping with the absence of an XHR header is actually the only thing we can possibly do. Non-browser clients will have to emulate this header if they want proper JSON.

effulgentsia’s picture

The wrapper is actually only necessary for Internet Explorer, as documented in the patch.

I don't have time to test this right now, but are we sure about that? If TEXTAREA were necessary, then the "Upload" button shouldn't be working in IE right now either, but AFAIK, it does. #39 is correct in that the response header must not be sent when "Add another item" is clicked for a multipart form, and that part of the patch I agree with. But as I understand it, the TEXTAREA is only needed to deal with HTML entities, but drupal_json_encode() already deals with HTML entities.

Damien Tournoud’s picture

If TEXTAREA were necessary, then the "Upload" button shouldn't be working in IE right now either, but AFAIK, it does.

The TEXTAREA is definitely necessary. Of course maybe not for all versions of IE and on every conditions, but it is definitely necessary. It's *not* actually a question of entity encoding, which we (you are right) we don't have to worry about because you are producing HTML-safe JSON. The problem is the ill-designed MIME-detection engine of IE that detect that the AJAX page content doesn't contain HTML and fallbacks to download mode, as described in the OP of #999704: Image field add one more button doesn't work in IE789:

In add/edit node form, I added unlimited textfield (or other fields), when I click the add one more button, the form doesn't add more fields, the browser show the download file

effulgentsia’s picture

But how do we know that the MIME-detection engine of IE decides that the lack of markup is what makes it not HTML? Maybe it's just the HTTP header of Content-Type: text/javascript; charset=utf-8 that makes it switch to download mode? That would explain why "Upload" works, since that routes to file_ajax_upload() which turns off that header, whereas "Add another item" doesn't work, since it doesn't route to file_ajax_upload(), and therefore, doesn't turn off the header. And the fix in #39 to turn off the header based on $_SERVER['HTTP_X_REQUESTED_WITH'] rather than based on which AJAX callback is invoked makes a lot of sense.

Sorry if I'm being dense, but I see nothing in #999704: Image field add one more button doesn't work in IE789 or in this issue, or in http://malsup.com/jquery/form/#file-upload that convinces me that IE's mime detection engine decides what mime type something is by its content. It's some stupid combination of file extension and HTTP headers, and it's not W3C standard, but that it actually scans content? Where is the evidence of that? Furthermore, if it is based on scanning content, then can we solve it by setting a Content-Type: text/html; charset=utf-8 header? Even IE listens to explicit headers ahead of implicit assumptions, does it not?

effulgentsia’s picture

A couple more considerations:

1) For normal XHR, we should be using the application/json mime type rather than text/javascript: http://www.ietf.org/rfc/rfc4627.txt?number=4627

2) For IFRAME usage, do we need to worry about this security vulnerability? That post is old. Maybe browsers have been fixed accordingly?

effulgentsia’s picture

Priority: Major » Critical

Also, if I'm understanding this issue correctly, then currently, a multi-valued image field is broken in IE. That should block a 7.0 release, right? So, escalating to critical.

[EDIT: er, maybe a multi-valued image field isn't broken in IE, but a multi-valued text field added to an Article is (#54). IMO, also a release blocker.]

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
8.01 KB

Just for clarity, this is my recommendation. It needs cross-browser testing. And, as per #56.2, a security review. And perhaps #54 is right and #55 is wrong, but if so, please provide evidence of that.

rfay’s picture

Status: Needs review » Needs work

Nice work and progress.

I took a run through some of the AJAX examples, and most #ajax forms seem to work, but An AJAX-enabled submit button will always submit the page, with both IE and firefox. So you can't even get the alert AJAX command to "work" on firefox, probably because the page has submitted before the alert happens.

AJAX commands work now with IE, although I can't confirm they work with Firefox, probably because of the submit problem.

I imagine we're missing a "return FALSE" after the submit.

rfay’s picture

FileSize
6.83 KB

Before we take the risk of destabilizing AJAX at a point near release, I want to point out that reverting jquery.form to v2.21 (the version we were using two updates ago) also fixes this bug. Poking around with IE and Firefox on various AJAX examples, multivalued imagefields and the original report in this issue (#12, the ajax_test.module by @manimejia) shows no side-effects. Of course, reverting jquery.form might also destabilize something, and that's a risk that we would also have to understand.

I'm attaching the unminified v2.21 in case anybody wants to experiment with it. (It's hard to find on github. You have to search the repo to find it).

rfay’s picture

@effulgentsia, you promoted this to critical in #57 saying

[EDIT: er, maybe a multi-valued image field isn't broken in IE, but a multi-valued text field added to an Article is (#54). IMO, also a release blocker.]

I'm unable to recreate your results (multivalued textfield on an article, co-existing with an imagefield is broken). It seems to work for me in both IE and FF. I actually have no idea how those can work. It actually kind of looks like they're working by backing out to the non-javascript behavior on IE.

We might be able to demote this issue to major, but I'll let you make that decision.

Edit: The quote in #57 referred to a quote in #54, which was referring to the initial problem statement in #999704: Image field add one more button doesn't work in IE789, which has been marked a dup of this one. I just can't recreate it.

rfay’s picture

The problem mentioned in #59 (page submits on an ajax-type submit even though it shouldn't) could very well be related to #634616: Various problems due to AJAX binding to mousedown instead of click, which we've never solved, but which is demonstrated perfectly with this patch... every time. However, if I pause the execution using the tactic below, the behavior is resolved (with the exception of having a lousy alert :-)

@@ -253,6 +253,7 @@ Drupal.ajax.prototype.eventResponse = function (element, eve
     return true;
   }
   else {
+    alert('returning false');
     return false;
   }
alexanderpas’s picture

Can we replace that alert with a delay and some documentation?

sun’s picture

FileSize
11.02 KB

Tried to debug what rfay reported (and can be reasily reproduced), but it's weird/tough.

With this patch, an uncaught exception is thrown within the Drupal.ajaxError() handler in drupal.js, upon trying to access xmlhttp.statusText.

Further debugging revealed that we're not canceling a form submission upon an AJAX error/exception. Even the code in this patch doesn't properly catch the error.

Lastly, all of that broken error/exception handling is a separate issue. However, for some unknown reason, I wasn't able to get back any response/result from server-side AJAX callbacks.

Leaving at needs work.

rfay’s picture

FileSize
5.17 KB

Here's the diff of sun's #64 from #58

rfay’s picture

@alexanderpas, I experimented with a delay and got no joy. Besides, it would be so incredibly ugly.

effulgentsia’s picture

Status: Needs review » Needs work
Issue tags: -Needs security review
I'm unable to recreate your results (multivalued textfield on an article, co-existing with an imagefield is broken). It seems to work for me in both IE and FF. I actually have no idea how those can work. It actually kind of looks like they're working by backing out to the non-javascript behavior on IE.

I just tested with a clean install of IE8, with Drupal HEAD, and with Overlay disabled (you might not need to disable Overlay, I just do when trying to isolate JS issues):
When you add a multi-valued textfield to an Article node type, and then go to node/add/article and click "Add another item", you get a security warning because IE is trying to download the AJAX response instead of letting jQuery read it. See https://skitch.com/effulgentsia/rf7x9/w7hp64-running. This is consistent with what was reported in #999704: Image field add one more button doesn't work in IE789 and #21/#24.

With the patch in #58, the above is fixed.

Before we take the risk of destabilizing AJAX at a point near release, I want to point out that reverting jquery.form to v2.21 (the version we were using two updates ago) also fixes this bug.

Also, see #35. The relevant change occurred from 2.43 to 2.44: https://github.com/malsup/form/commit/6da6cb04bbe2102dc83ee051b6b7e1b604.... Specifically:

-  var multipart = false;
+  var multipart = ($form.attr('enctype') == mp || $form.attr('encoding') == mp);
-   if ((files.length && options.iframe !== false) || options.iframe || found || multipart) {
+   if (options.iframe !== false && (fileInputs || options.iframe || multipart)) {

In other words, in 2.43, iframe upload was used only if a file needed to be uploaded, but in 2.44, this was changed to using iframe upload if the form is a multipart form (even if there is no file to upload). So, yes, reverting to 2.43 or earlier would fix this particular bug. However, even with 2.43 or 2.21, a similar bug can be triggered by choosing a file for the Article image, not clicking Upload, and instead clicking "Add another item" of the textfield. Which a user is less likely to do, but is still a bug.

I don't know what the reason was for malsup changing from only using iframe upload when it was truly needed to using it whenever there's a multipart form, but considering our AJAX system is broken when iframe upload is used, we should probably fix it regardless of whether iframe upload is used sparingly or liberally. And if we do fix it, then there's no reason to revert to the older version and take ourselves out of future upgrade paths.

As a side note, I'm not sure where we got 2.48 from. https://github.com/malsup/form/commit/a603b6ed966660694188609bf49ba8977d... went from 2.47 to 2.49. Maybe we should upgrade to the latest though (2.52)? [EDIT: never mind about the 2.48 question. HEAD has 2.49].

I haven't investigated #62 and below yet. Will do so next.

rfay’s picture

I think we should fix the dangerous "touch xmlhttp.responseText and it throws an exception" in this issue, since it's nearly impossible to work on this without it. I know it perhaps should be done in another issue.

Note that this is one demonstration of an "HTTP Error 0" or rather "HTTP status = 0" which we have so many troubles with throughout Drupal AJAX-land, so it's worth paying attention to why we're getting a status 0 (and without responseText populated) in this particular case.

effulgentsia’s picture

Something interesting about the race condition, or why with #58, a non-AJAX submit is performed:

Try this alone on HEAD (without #58):

Index: misc/ajax.js
===================================================================
RCS file: /cvs/drupal/drupal/misc/ajax.js,v
retrieving revision 1.33
diff -u -p -r1.33 ajax.js
--- misc/ajax.js	17 Dec 2010 01:03:58 -0000	1.33
+++ misc/ajax.js	20 Dec 2010 18:20:19 -0000
@@ -176,4 +176,6 @@ Drupal.ajax = function (base, element, e
 
   // Bind the ajaxSubmit function to the element event.
   $(ajax.element).bind(element_settings.event, function (event) {
-    return ajax.eventResponse(this, event);
+    return false;
+    //return ajax.eventResponse(this, event);
   });

Expected: clicking "Add another item" should do nothing. No AJAX submit. No non-AJAX submit.
Actual: A non-AJAX submit is performed.

Reason: Because we're binding to "mousedown", and not to "click". So we're preventing the browser default action for mousedown, but not for click. And click is what triggers default browser form submission.

Reason why HEAD only has the race condition occur sometimes, whereas #58 has it occur always: because in HEAD, during if the AJAX is successful, we disable the triggering element, so that stops the "click" event. #58 removes the disabling of the triggering element.

Next steps:
1) Why do we bind to mousedown and keypress, instead of binding to click?
2) If we want to keep binding to mousedown and keypress, then we either need to also bind to click in order to prevent its default, or we need to restore the .attr('disabled', true) at least for buttons.
3) Once we're able to stop the non-AJAX submit, it will be easier to test and debug the AJAX errors.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
12.4 KB

This is #58 plus the handling of 'click' termination from the 'mousedown' as per #69. There's a comment in ajax_pre_render_element() explaining why we bind to mousedown instead of click, but I don't understand it, so I added a @todo asking for more clarification (can happen after this issue).

With this, I tested a multi-valued textfield on the Article type (where iframe upload is used due to the image field) and on the Page type (where XHR submission is used due to no file field), on Mac FF 3.6 and Windows 7 IE 8, and it seems to work. If there's steps to reproduce the exception in #64, please share.

@manimejia/@rfay: please also confirm that the initial problem that started this issue is also fixed.

rfay’s picture

Wow, is that fantastic from a manual test point of view.

I tested with FF 3.6 (linux), IE8 (win), Chrome 8 (win) and did these things:

  • Check the original report (an ajax checkbox form-altering admin/people/create or node/add/article)
  • Checked with the @manimejia's ajax_test module from #12 and #13.
  • Tested every example in AJAX Example
  • Tested every AJAX command in the AJAX commands in AJAX Example

And this also completely resolves #634616: Various problems due to AJAX binding to mousedown instead of click, which has been a problem with every version of Drupal.

I have not thoroughly tested #999704: Image field add one more button doesn't work in IE789 yet. With the patch, I cannot make #999704: Image field add one more button doesn't work in IE789 happen. Without the patch, I can't make anything work at all because IE throws a security warning about downloading a file.

I'll take a look at the code now and see if there's anything useful I can add.

effulgentsia’s picture

Issue tags: +Needs security review

Thanks rfay! Additional x-browser testing on this would be useful. Particularly IE6, IE7, IE9. Remember to clear browser caches between tests (i.e., applying patch or reverting patch, but not clearing browser cache, results in an invalid test).

Also, tagging for a security review, as per #56.2. I really don't understand the attack vector described in that article. Seems like some kind of XSS vector that's possible with JSON that isn't possible with HTML? I don't get it, but I'm hoping that either the article is wrong, or that browsers have since been fixed, or that the use of text/plain (rather than text/html) combined with only doing it for POST closes whatever hole that article describes.

rfay’s picture

Status: Needs work » Needs review
Issue tags: +Needs security review
effulgentsia’s picture

Issue Summary

Browsers don't allow JavaScript to read a user's local files. Therefore, jquery.form.js has to use two different techniques for submitting an AJAX request. When a file doesn't need to be uploaded as part of the submission, it can use the browser's XMLHttpRequest object (XHR). When a file needs to be uploaded, it submits to a hidden IFRAME.

In #969456: Update to jQuery Form 2.52, we updated jquery.form.js from 2.36 to 2.49. One of the things that changed as part of this upgrade was that previously, jquery.form.js only used the IFRAME technique when a file needed to be uploaded, whereas in the newer version, it uses the IFRAME technique whenever the form is a multipart form (see #67). jquery.form.js is now on version 2.52, and as a separate issue, we may want to upgrade to this before releasing D7, but changes from 2.49 to 2.52 are unrelated to this issue. 2.52 still uses the IFRAME technique for all multipart forms.

Because of that change, two bugs got discovered:

  1. A multivalued textfield added to the Article type became broken in IE (see #999704: Image field add one more button doesn't work in IE789).
  2. An AJAX-enabled checkbox added to the user add/edit forms fails to submit its value.

Both of these bugs demonstrated that some of our AJAX framework code is buggy when the IFRAME technique is used. These bugs existed prior to the jquery.form.js upgrade, but were harder to trigger, because to trigger them previously, you had to browse for a file in the file field that appears on these forms, not upload it, and then click the "Add another item" button or the checkbox. Since it's not common to browse for a file, and then do something else on the form instead of clicking Upload, it's not surprising that both of these bugs were discovered after Nov. 15.

The first bug is caused by the text/javascript header being sent back to the IFRAME. IE doesn't like that (additional comments in patch). In HEAD, this problem is dealt with by the AJAX callback for the Upload button having code to prevent this header. But that is faulty logic, because that means the header is only prevented when the Upload button is clicked. What we really need is to prevent the header whenever the IFRAME technique is used. #70 does this.

Related to above, HEAD also has code to support wrapping the JSON output in a TEXTAREA element, as per the recommendation in http://malsup.com/jquery/form/#file-upload. However, core never makes use of this feature. #70 removes it, because:

  • The stated purpose on http://malsup.com/jquery/form/#file-upload for wrapping in a textarea is to support HTML entities within the JSON string without having the browser choke on parsing it. But our JSON strings are output using drupal_json_encode() which provides an alternate solution to this problem.
  • The secondary utility of the textarea is mentioned by Damien in #34 and #54: to ensure that in the absence of a content-type header, that IE's MIME detection engine doesn't auto-detect the JSON string as JSON, getting us back to the original problem. But this can be solved by sending an explicit text/plain header, which #70 does.
  • The problem of outputting a textarea, as #39 does, is that since the way we detect an iframe upload is by the absence of detecting an XHR submission, it means that we would output a textarea to all non-browser clients like SimpleTest, Flash, etc. (which is why #39 has all those simpletest failures). And that just makes no sense. So, if there's no reason to output it, then let's not output it.

Finally, while I was working on this, I discovered that text/javascript is not the standards-compliant MIME type for a JSON string, so #70 fixes this to application/json (when XHR is used and we don't have to work around IE's limitations).

The second bug (failure of AJAX-enabled checkbox to submit their value) is caused by code in ajax.js that disables the triggering element. In an XHR submission, this code runs after field data has already been collected, but in an IFRAME submission, it runs before the POST, so disabling the element means removing it from the POST data. #70 fixes this by removing the code that disables the triggering element. See #48 and #50 for why this is ok.

However, removing the disabling code uncovered the root cause of yet another bug: #634616: Various problems due to AJAX binding to mousedown instead of click, which is that for buttons, we prevent the browser default action of the mousedown, but not the click. #70 fixes that. This problem was masked when the button was being disabled before the click was registered, but happened as a race condition otherwise. By removing the disabling code, it's no longer a race condition, but something that happens every time, making it easier (and necessary) to fix, which is why the fix is included in this patch.

So, this patch fixes the two discovered bugs with AJAX IFRAME uploads, fixes the root cause of the race condition that's been bothering us, and fixes the JSON header to be standards compliant. Its biggest risk is outputting a text/plain header when XHR isn't used, which needs to be tested across browsers to ensure it doesn't cause problems, and checked for security as per #72.

As far as what to test:

  • Add a multivalued textfield to the Article and Basic Page types. Ensure "Add another item" works in both (on the Article type, IFRAME upload is being used, and on the Page type, XHR is being used). Also, test the image field Upload/Remove buttons on the Article type.
  • Additional test scenarios are summarized in #71.
chx’s picture

Issue tags: -Needs security review

This no longer needs a security review as we do not serve text/html.

aspilicious’s picture

Add a multivalued textfield to the Article and Basic Page types. Ensure "Add another item" works in both (on the Article type, IFRAME upload is being used, and on the Page type, XHR is being used). Also, test the image field Upload/Remove buttons on the Article type.

Tested this on IE6/IE7:

Before patch: errors
After patch: no errors :)

sun’s picture

+++ misc/ajax.js	20 Dec 2010 22:43:58 -0000
@@ -101,6 +101,7 @@ Drupal.ajax = function (base, element, e
     event: 'mousedown',
     keypress: true,
+    secondary_event: null,

I'm concerned about the added hard-coded assumption of a secondary event. I'd like to think some more about this, and also find out why we are using mousedown instead of click in the first place.

Powered by Dreditor.

sun’s picture

@merlinofchaos dug out the origin of the mousedown event: #216059-6: AHAH triggered by text input enter key press, breaks e.g. autocomplete

merlinofchaos’s picture

Ok, I propose we modify the patch to change mousedown to click, and test http://drupal.org/node/216059#comment-721125 to see if it is relevant. jquery's event handling has been completely overhauled since that issue and this may not be something we have to worry about anymore, but we won't know until we test.

effulgentsia’s picture

With this patch, we can defer the mousedown to click change to #634616: Various problems due to AJAX binding to mousedown instead of click, a non-critical issue, which I will now reset to active, as this patch no longer solves it.

Because, this patch leave the .attr('disabled', true) alone. In testing, I realized that we don't want to remove the disabling, because while ajax.ajaxing prevents AJAX request collision, it doesn't prevent a checkbox from being checked/unchecked, or a select element from having a different option chosen. And this can leave an ajax-enabled element in a weird state. For example, enable the book module for the Article or Page type (depending on if you want to test IFRAME or XHR), and especially if you add a sleep(5) to book_form_update(), then with #70, you can change the Book while the AJAX response is running, and then be left with a mismatch between the Book value and the Parent Item drop-down returned by AJAX.

This patch fixes that by leaving the disabling logic alone, but solving the problem of getting the value of the triggering element into the POST submission.

webchick’s picture

Broke out testing changing mousedown to click over here: #1004742: Change #ajax event 'mousedown' into 'click'

rfay’s picture

FileSize
841 bytes

OK, I've tested #80 on a LOT of platforms. Getting IE9 working was the hardest thing I've done this week.

Manual testing results for #80

Platforms: IE 6, 7, 8, 9 (Windows), Firefox 3.6 (Linux), Chromium 8 (Linux), Google Chrome 10 (Linux)

Procedure:

  1. Add a multivalued textfield to the standard "article" node type
  2. Install ajax_test (new version attached to this comment to enable testing with IE9)
  3. Visit admin/people/create and see that the form-altered ajax checkbox and select work OK
  4. Visit node/add/article and see that the form-altered ajax checkbox and select work OK
  5. On the article add page, add several text fields by clicking "Add another item" by the multivalued textfield
  6. Install the ajax_example module from Examples project
  7. Visit and test these basic items in examples/ajax_example/*: simplest, autocheckboxes, autotextfields, submit_driven_ajax, dependent_dropdown, dynamic_sections, wizard, add_more, ajax_link
  8. Visit examples/ajax_example/advanced_commands and test each of the AJAX commands represented there.

Results
Everything works as intended on all browsers. There are two minor exceptions that are by design or not part of this issue:

  1. In IE9, checkboxes don't work as AJAX triggering elements (again). This is true for HEAD and also for #70. As a result of that I had to enhance the ajax_test module (attached) to trigger on something besides checkbox. This is noted in #557284-38: AHAH/AJAX bindings do not work on checkbox or radio in IE6/7/8 (and now IE9), which was reopened by effulgentsia anyway.
  2. As by design by effulgentsia, the race condition of #634616: Various problems due to AJAX binding to mousedown instead of click is back, which is sad for me, but we'll get it fixed in that issue.
sun’s picture

FileSize
8.07 KB
+++ misc/ajax.js	21 Dec 2010 22:27:42 -0000
@@ -152,9 +152,9 @@ Drupal.ajax = function (base, element, e
-    beforeSend: function (xmlhttprequest) {
+    beforeSend: function (xmlhttprequest, options) {
...
-      return ajax.beforeSend(xmlhttprequest, ajax.options);
+      return ajax.beforeSend(xmlhttprequest, options);

I feel more comfortable with this approach/workaround, and especially that options argument fix looks like a big oversight and really nice fix to me.

Attached patch merely re-phrases and shortens the code comments a bit. Otherwise, this looks RTBC to me.

Powered by Dreditor.

merlinofchaos’s picture

According to http://api.jquery.com/jQuery.ajax/ beforeSend does not receive a second argument. This is not an oversight, I checked the documentation before I did that.

merlinofchaos’s picture

We've really got to be careful about sneaking changes like that into these patches. If sun hadn't point that out, I may not have noticed that beforeSend() was changed to not match the documentation.

sun’s picture

Looks like the documentation is outdated, since .beforeSend() actually gets the options:

		// Allow custom headers/mimetypes and early abort
		if ( s.beforeSend && s.beforeSend.call(s.context, xhr, s) === false ) {

However, upon reading http://api.jquery.com/jQuery.ajax/ I wondered whether this "custom" HTTP header handling might break JSONP requests?

rfay’s picture

Here is the difference between #80 and #83 for reviewers. It is comments only, as sun said.

Are we good with the conversation in #84-86?

effulgentsia’s picture

sun's comments improvements in #83 are fantastic.

Otherwise, this looks RTBC to me.

I agree, but given #84-#86, merlinofchaos should have a chance to confirm or deny.

I wondered whether this "custom" HTTP header handling might break JSONP requests?

I'm not sure ajax.js or ajax.inc are really setup to handle JSONP requests. I'm not sure what would be needed to make them do that. I don't think the header is a problem though, since as per code comments in the patch, a page callback or alternate delivery callback can send a different header.

+++ includes/ajax.inc	22 Dec 2010 03:21:21 -0000
@@ -407,19 +407,49 @@ function ajax_base_page_theme() {
+    $iframe_upload = !isset($_SERVER['HTTP_X_REQUESTED_WITH']) || $_SERVER['HTTP_X_REQUESTED_WITH'] != 'XMLHttpRequest';

Upon more thinking about this, I wonder if this line is the best implementation. Maybe instead of checking for XHR, we should check for 'application/json' within $_SERVER['HTTP_ACCEPT']? Can be deferred to a separate issue though. What we have here works, so changing would be more for secondary goals, like non-browser clients, and better compatibility with web standards.

+++ misc/ajax.js	22 Dec 2010 03:34:50 -0000
@@ -318,7 +318,20 @@ Drupal.ajax.prototype.beforeSubmit = fun
+  var v = $.fieldValue(this.element);
+  if (v !== null) {

I haven't tested this thoroughly for links. $.fieldValue() returns null if the element doesn't have a name attribute, but links can have name attributes, and I'm not sure if $.fieldValue() handles that well. Not sure how big of a concern that is for this patch, vs. dealing with that edge case separately.

rfay’s picture

Status: Needs review » Reviewed & tested by the community

We would prefer for merlinofchaos to have another round with this, but he shouldn' t be looking at trivial things like this when super-important things have happened, so let's go ahead and RTBC.

Reviewed carefully by 3 great contributors.
Tested within an inch of its life.

Should be OK.

Issue summary is #74
Testing summary is #82

merlinofchaos’s picture

Unfortunately I can only speak for the documentation. It is possible jQuery's documentation is out of date but I dunno. It may be that this is only true for the beforeSend on $.ajaxSubmit and not on $.ajax?

sun’s picture

Status: Reviewed & tested by the community » Needs review
+++ misc/ajax.js	22 Dec 2010 03:34:50 -0000
@@ -318,7 +318,20 @@ Drupal.ajax.prototype.beforeSubmit = fun
 Drupal.ajax.prototype.beforeSend = function (xmlhttprequest, options) {
-  // Disable the element that received the change.
+  // Disable the element that received the change to prevent user interface
+  // interaction while the AJAX request is in progress. ajax.ajaxing prevents
+  // the element from triggering a new request, but does not prevent the user
+  // from changing its value.
+  // Forms without file inputs are already serialized before this function is
+  // called. Forms with file inputs use an IFRAME to perform a POST request
+  // similar to a browser, so disabled elements are not contained in the
+  // submitted values. Therefore, we manually add the element's value to
+  // options.extraData.
+  var v = $.fieldValue(this.element);
+  if (v !== null) {
+    options.extraData = options.extraData || {};
+    options.extraData[this.element.name] = v;
+  }
   $(this.element).addClass('progress-disabled').attr('disabled', true);

And now... the best question of all: Do we still need this code? We no longer remove the 'disabled' attribute, so... why?

Powered by Dreditor.

effulgentsia’s picture

Don't the code comments make that clear? We want to disable the element to prevent UI interaction while the AJAX request is in progress, as we do in HEAD. But, we want the element's value submitted in the POST. Which happens automatically when XHR is used, as per the code comment. But doesn't happen when IFRAME is used, unless we add it to extraData.

dmitrig01’s picture

As an innocent bystander, the code comment makes perfect sense and seems necessary

sun’s picture

Status: Needs review » Reviewed & tested by the community

ok, let's do this. There's a small chance that we're going to revise the JS part of this patch given some other IE/AJAX issues in the queue right now, but I don't think that the PHP part is going to change.

effulgentsia’s picture

Unfortunately I can only speak for the documentation. It is possible jQuery's documentation is out of date but I dunno. It may be that this is only true for the beforeSend on $.ajaxSubmit and not on $.ajax?

FYI: Options was added as a parameter to beforeSend() invoked from $.ajax when jQuery was upgraded from 1.2.4 to 1.2.5. It has been a parameter in the beforeSend() invoked by the IFRAME upload of $.ajaxSubmit() at least as far back as jQuery Form 2.18. I added a comment to http://api.jquery.com/jQuery.ajax/#comment-117461525 asking for that doc page to be updated accordingly.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for all the great discussion and especially the thorough summary in this issue. Sounds like we're fixing bugs in upstream projects too as a part of this. Very nice. ;)

This has +1s from all the right folks, lots of manual testing, and better to commit it pre-RC than post-RC to give ample time for testing

Committed to HEAD.

rfay’s picture

@manimejia, I want to thank you for spotting this, reporting it, and sticking with it when I didn't understand it. I am *really *glad we got this one killed. And 2 more longstanding ajax bugs are going with it. Thanks.

effulgentsia’s picture

effulgentsia’s picture

as a separate issue, we may want to upgrade to this before releasing D7

#969456-16: Update to jQuery Form 2.52

I added a comment to http://api.jquery.com/jQuery.ajax/#comment-117461525 asking for that doc page to be updated accordingly.

And they responded. That doc page now shows beforeSend() as taking an xhr and settings param, so the code that was committed in #96 is not in violation of any jquery documenatation.

rbayliss’s picture

Looks like using $.fieldValue() added a dependency from ajax.js to jquery.form.js. Is this intended?

effulgentsia’s picture

Looks like using $.fieldValue() added a dependency from ajax.js to jquery.form.js. Is this intended?

Thanks for catching that. It's already the case that ajax.js depends on jquery.form.js for form elements, due to this code in Drupal.ajax.prototype.eventResponse():

if (ajax.form) {
  ...
  ajax.form.ajaxSubmit(ajax.options);
}

However, in the case of a link triggering AJAX behavior, there was no prior required dependency before #96. A fix for this is included in #1018714-4: Image Upload Widget Not Working in IE8 (it puts $.fieldValue() inside an if (this.form) block).

Csabbencs’s picture

I'm a bit lost in this thread.. could you guys please confirm that the patch from comment #80 should solve the issue of changing checkboxes via ajax is not reflected in $form_state?
I still have the issue and my code is patched.

effulgentsia’s picture

@Csabbencs: #83 was committed to CVS in #96, so if you're running Drupal 7.0 or CVS HEAD, then you should have the fix with no patches from this issue needed. However, there is still the problem of IE failures for multipart forms in #1018714: Image Upload Widget Not Working in IE8 and IE9 failures in #557284: AHAH/AJAX bindings do not work on checkbox or radio in IE6/7/8 (and now IE9). If you're having a problem that doesn't appear to be one of those, please open a new issue describing the steps to reproduce, and link to it from here. Thanks.

Status: Fixed » Closed (fixed)

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

whendrik@gmail.com’s picture

I still encounter problems with AJAX property's in Form, with internet explorer (IE).
In FF/Chrome/Safari, it works perfect.

A Form, with AJAX elements, doesnt work in IE 6/7/8 when:
The form contains a (plain) 'file' element.

( I tried different AJAX methods, also example code doesnt work anymore when adding a form in IE. Again, all works fine in FF/Chrome/Safari )

When i remove the 'file' element, the form works fine in IE.

rfay’s picture

Making a comment on a closed issue won't get it fixed.

And you say "I encounter problems" and "doesn't work any more", but we need far better descriptions than these.

Please open a new issue with an explicit description. Also, please make sure you've tried this on a clean D7 install.

effulgentsia’s picture

@whendrik: your description sounds like you're encountering #1018714: Image Upload Widget Not Working in IE8 (patch in comment #4, summary in comment #19). That issue is also closed, because that patch has been committed to Drupal 7.x. But it was done so after 7.0 was released. When 7.1 is released, the fix will be included. In the meantime, if you're running 7.0, you can either update to 7.x-dev, or apply that issue's #4 patch.

whendrik@gmail.com’s picture

Thank you, i already created a new issue...

I will test the patch, and re-status my issue. -> Done (by droplet already)...

I fixed this, instead of going to DEV, just use the

misc/ajac.js
include/ajax.inc

From the DEV. Worked in FF, Chrome, and.... IE! :)

Renee S’s picture

This problem exists in D6 as well - any chance of a backport?