I was working with a file upload field (<input type ="file" />) in a Webform form today, and noticed that clicking on the "Upload" submit input before I had selected a file didn't prevent the AJAX upload code from running. While this didn't break anything, it was confusing to see the throbber there and I wanted a way to prevent the AJAX submit from happening in the first place and display a "Please select a file" message to the user.

Upon investigating core's ajax.js I found Drupal.ajax.prototype.beforeSubmit(), left empty for easy overriding. I also saw in jquery.form.js that there's a check for the beforeSubmit function returning false, which is expressly for the purpose of preventing the form submission from going through.

So I implemented a custom beforeSubmit function for the submit button which returns false if the file input is empty. On first run, it worked as expected, but thereafter, clicking on the submit button appeared to do nothing whatsoever - not even run the beforeSubmit function again.

I figured out that the following sequence had happened (where "ajax" is the Drupal.ajax object for my HTML submit element):

  1. Leaving file field blank, I click the upload button.
  2. ajax.eventResponse() runs, calling ajaxSubmit() (in jquery.form.js) on the form.
  3. Within ajaxSubmit(), ajax.options.beforeSubmit() sets ajax.ajaxing = true, then calls ajax.beforeSubmit().
  4. My beforeSubmit() function returns false.
  5. Back in ajaxSubmit(), the returned false value is detected and ajaxSubmit() ends right then.
  6. I select a file (or even don't; irrelevant) and click the upload button again.
  7. Again, ajax.eventResponse() runs. But now, ajax.ajaxing is still true from the first attempt, and eventResponse() immediately returns false.

This if (ajax.ajaxing) check is intended to prevent multiple ajax requests from overlapping each other. But in this sequence, since ajaxSubmit() never finished the first time, ajax.ajaxing doesn't get reset to false, and no further attempts to submit will go through.

My rough fix would be to adjust the default ajax.options.beforeSubmit() around line 150 of ajax.js to something like:

beforeSubmit: function (form_values, element_settings, options) {
  ajax.ajaxing = true;
  // If beforeSubmit() aborts the submit, reset ajaxing flag.
  var continue = ajax.beforeSubmit(form_values, element_settings, options);
  if (!continue) {
    ajax.ajaxing = false;
  }
  return continue;

In the meantime I was able to work around this simply by setting ajaxing back to false if my beforeSubmit() function was going to return false.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

perelman.yuval’s picture

Hi tea.time
Thanks a lot for writing this comment, you save my day...

Strae’s picture

Thank you very much tea.time, I had the same problem and is about an hour that I dont understand where the problem is!

Heine’s picture

Status: Active » Needs review
FileSize
683 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,564 pass(es). View

#0 in patch form.

SebasL’s picture

Version: 7.19 » 7.42
Status: Needs review » Reviewed & tested by the community
C:\wamp\www\drupal>git apply -v do-1907932-ajax-reset-ajaxing-beforesubmit.patch
Checking patch misc/ajax.js...
Hunk #1 succeeded at 156 (offset 7 lines).
Applied patch misc/ajax.js cleanly.

The patch still applies cleanly in 7.42.

I had the same problem and now I'm able to for example use the beforeSubmit function to validate the fields and when the information entered is valid, the form is submitted.

lolandese’s picture

Version: 7.42 » 7.x-dev

Applies cleanly on dev as well.

martin@martin-UX305FA:~/www/drupal$ git apply -v do-1907932-ajax-reset-ajaxing-beforesubmit.patch
Checking patch misc/ajax.js...
Hunk #1 succeeded at 156 (offset 7 lines).
Applied patch misc/ajax.js cleanly.
W.M.’s picture

Patch at #3 solves the issue, kindly commit to master.

Fabianx’s picture

We don't have JS tests in 7 and this looks right.

However:

We need to make sure this bug does no longer exist in Drupal 8.

xangy’s picture

Assigned: Unassigned » xangy
Status: Reviewed & tested by the community » Needs review
FileSize
2 KB

did a plain reroll with the latest pull of 7.x branch.

xangy’s picture

Assigned: xangy » Unassigned
Fabianx’s picture

The re-roll is cool, we still need to check that the bug no longer exists in 8.1.x.

lolandese’s picture

Status: Needs review » Reviewed & tested by the community

Re-roll hasn't changed the code itself compared with the previous patch (was RTBC) and solves the issue for Drupal 7 (version indicated in the issue metadata).

Fabianx’s picture

stefan.r’s picture

Status: Reviewed & tested by the community » Needs review

Is Drupal 8 affected as well?

gskharmujai’s picture

i just faced this same issue today while working on a form validation coupled with an ajax callback on Drupal 8. So it looks like D8 is still affected too.

Applied the Patch on #3 and now my validation code worked perfectly.

xangy’s picture

I had this issue encountered in Drupal 7 because of a use-case on my project. I wasn't able to check if this issue still existed in Drupal 8. If this is yet not fixed then using the logic used for 7.x, adding patches for 8.3.x and 8.4.x. Please review with your use-case, if these patches fix it or not.

xangy’s picture

Assigned: xangy » Unassigned
gskharmujai’s picture

Excellent! This patch fixes the issue with my form validation code.

Also these patches applies cleanly on 8.3.4 and 8.4.x-dev

george@box-nwdz-net:/www/drupal-8.4$ git apply -v /home/george/Downloads/if_a_custom-1907932-15-1.patch
Checking patch core/misc/ajax.js...
Applied patch core/misc/ajax.js cleanly.
george@box-nwdz-net:/www/drupal-8.3$ git apply -v /home/george/Downloads/if_a_custom-1907932-15-2.patch
Checking patch core/misc/ajax.js...
Applied patch core/misc/ajax.js cleanly.
Fabianx’s picture

Version: 7.x-dev » 8.5.x-dev
Issue tags: -Needs check if D8 is affected +Needs reroll

Over to 8.x as it is affected as well

Fabianx’s picture

Issue tags: +needs backport to D7
Jo Fitzgerald’s picture

Test 8.4.x patch against 8.5.x (because it at least applies cleanly so...).

Jo Fitzgerald’s picture

Issue tags: -Needs reroll

Successfully passes tests for 8.4.x & 8.5.x so removing Needs Reroll tag.

gnuget’s picture

Exactly the same problem happens with beforeSend,

Should we use this same issue to address both? or should we create a separated one?

gnuget’s picture

This needed a reroll because of #2927230: 3/3 JS codestyle: camelcase, so I did it.

This is a reroll of #15 if_a_custom-1907932-15-1.patch

gnuget’s picture

Ok. Accord with: https://www.drupal.org/node/2815083 the changes needs to be done on ajax.es6.js and after that needs to be transpiled to ajax.js.

So that is what I did in this new patch, Also I fixed some coding standard issues, and added the change for beforeSend.

This should be almost RTBC

- David.

gnuget’s picture

Title: If a custom beforeSubmit() function on a Drupal.ajax object returns false, 'ajaxing' gets stuck as true » If a custom beforeSubmit() or beforeSend() function on a Drupal.ajax object returns false, 'ajaxing' gets stuck as true
jibran’s picture

Issue tags: +JavaScript
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

This looks correct, thanks @gnuget for the attention to detail and all who worked on this issue before. May be worth 8.4.x as well, let the committers decide.

drpal’s picture

Status: Reviewed & tested by the community » Needs work

Sorry to bump this back to Needs work. Just some minor coding standards issues.

  1. +++ b/core/misc/ajax.es6.js
    @@ -484,11 +484,19 @@
    +        var continueSubmit = ajax.beforeSubmit(formValues, elementSettings, options);
    

    Should be const not var.

  2. +++ b/core/misc/ajax.es6.js
    @@ -484,11 +484,19 @@
    +        if (continueSubmit === false) {
    +          ajax.ajaxing = false;
    +        }
    

    You can just check for the inverse of continueSubmit here.

gnuget’s picture

Status: Needs work » Needs review
FileSize
2.03 KB
1.92 KB

Hi drpal

Thank you for your review.

I just added your suggestions in this new patch.

Thanks!

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC, thanks for the review and quick fixes

Heine’s picture

Status: Reviewed & tested by the community » Needs work

Is the second part of #29 correct?

beforeSubmit and beforeSend only abort the request if they return false. When a beforeSubmit handler returns undefined, ajax.ajaxing will be set to false, but the request will continue.

gnuget’s picture

Hi Heine.

I just trusted what #28 said about to just inverse the value of the object but now that you mentioned you are right it NEEDS to be false to abort the request.

I'm going to undo that change. :-/

https://github.com/jquery/jquery/blob/master/src/ajax.js#L652

Is the second part of #29 correct?

Do you mean that we should use let instead of const? or do you mean that in the transpiled version is still using var? if the later yes is correct because that code is generated I didn't touch it. So conts is es6.

gnuget’s picture

Status: Needs work » Needs review
FileSize
1.76 KB
2.06 KB

I really hope that this is the good one :-)

Thank you all for your reviews.

droplet’s picture

Status: Needs review » Needs work

beforeSubmit isn't a jQuery function but jQuery.form. We have wrong doc.

+++ b/core/misc/ajax.es6.js
@@ -480,11 +480,19 @@
         ajax.ajaxing = true;
...
+        const continueSubmit = ajax.beforeSubmit(formValues, elementSettings, options);
+        if (continueSubmit === false) {

In another word, it should only set to TRUE when the condition is met.

In very edge case, if `ajax.beforeSubmit` takes more time than another script to detect `ajax.ajaxing = true` (e.g observable objects?), the bug will happen? How about replacing it with an early return?

Also,

"admin/structure/types/manage/article/form-display"

select a widget dropdown, the progress throbber still left there when the `beforeSubmit === false` after patched.

gnuget’s picture

We are adding this change for beforeSubmit (jQuery.form) and beforeSend (jQuery), and both work in the same way:

BeforeSubmit: https://github.com/jquery-form/form/blob/master/src/jquery.form.js#L212

// give pre-submit callback an opportunity to abort the submit
    if (options.beforeSubmit && options.beforeSubmit(a, this, options) === false) {
        log('ajaxSubmit: submit aborted via beforeSubmit callback');
        return this;
    }

BeforeSend: https://github.com/jquery/jquery/blob/master/src/ajax.js#L652

// Allow custom headers/mimetypes and early abort
if ( s.beforeSend && ( s.beforeSend.call( callbackContext, jqXHR, s ) === false || completed ) ) {
  // Abort if not done already and return
  return jqXHR.abort();
}

In both cases, it checks against false.

About "admin/structure/types/manage/article/form-display" I will check what is going on there.

drpal’s picture

+++ b/core/misc/ajax.es6.js
@@ -480,11 +480,19 @@
+        if (continueSubmit === false) {
+          ajax.ajaxing = false;
+        }

I'm not sure why you reverted this change. If continueSubmit is false, and you invert it, !continueSubmit, then it's true, and setting ajax.ajaxing = false; will still happen. You'll still also return the correct state from continueSubmit. Either way, this still needs some further work from #34.

gnuget’s picture

Hi Drpal.

I revert the change because of what #31 said.

Anyway, both projects jQuery.form and jQuery check against === false there, I prefer to do the same.

Also,

"admin/structure/types/manage/article/form-display"

select a widget dropdown, the progress throbber still left there when the `beforeSubmit === false` after patched.

I just checked this on Firefox, Chrome and Safari and all worked as expected, not sure what is wrong in your case :-/

I will ask someone else to test it.

Thanks!

droplet’s picture

FileSize
56.18 KB
+        continueSubmit = false;
         if (continueSubmit === false) {

You able to see the bug by add above line to your patch. And then change the select, eg `Authored by`

droplet’s picture

Hmm. It can be another CORE bug.

https://github.com/drupal/drupal/blob/8.5.x/core/modules/field_ui/field_...

Either fix it with a more standard way or a new event to shut it down. The latter one seems better? We don't know how many Contribs do the same way.

gnuget’s picture

Yes, that is what I was going to say.

The problem that you mention happens even without my patch, it is a different issue.

Should we create a new issue and set this one as RTBC?

droplet’s picture

Hmmm. the vanilla jQuery called xhr.abort() but beforeSubmit (jQuery.form) does not. Such a headcache.

https://github.com/jquery/jquery/blob/master/src/ajax.js#L656

var url = location.href + drupalSettings.path + "admin/structure/types/manage/article/form-display?ajax_form=1&_wrapper_format=drupal_ajax";
jQuery
  .ajax({
    url: url,
    beforeSend: function(xhr) {
      return false;
    }
  })
  .fail(function() {
    console.log("fail");

    // set back to false
    ajax.ajaxing = false;
  });

About patch #33,
I preferred the way on the patch I just uploaded

gnuget’s picture

Status: Needs work » Needs review
gnuget’s picture

I don't have strong opinions about any of the last patches and I'm ready to change the status to RTBC as soon as the bot finish with the tests.

Thanks for your work on #41.

David.

gnuget’s picture

Status: Needs review » Reviewed & tested by the community

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 41: 1907932-41.patch, failed testing. View results

gnuget’s picture

Status: Needs work » Reviewed & tested by the community

I re-ran the tests because the errors seemed unrelated and it is green as expected.

So switching back to RTBC again.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 41: 1907932-41.patch, failed testing. View results

gnuget’s picture

Status: Needs work » Reviewed & tested by the community

Again, unrelated errors. I re-tested the patch and it is green again. So, I'm going to switch this back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

8.x has javascript testing - would be nice to have a test for this as it is a bug fix.