Updated: Comment #7

Problem/Motivation

Why is there no double click prevention built into Drupal core? Surely this should be standard functionality?

D7: the current patch (form.js) only works on forms using fieldsets or vertial tabs according to @JvE in #99

Steps to reproduce

  1. Use Chrome
  2. Navigate to admin/modules
  3. Scroll down
  4. Triple click the 'Save configuration' button
  5. Check the network page chrome provides which has 3 POST request.
  6. Apply the patch
  7. The network page does now contain just one POST request
  1. Use Chrome
  2. Log out
  3. Navigate to user/register
  4. Enter a user name and mail address
  5. Triple click the submit button
  6. Depending on your speed you get
    • Exception
    • Already taken

Proposed resolution

Integrate https://github.com/sun/jquery-form-submit-single

Remaining tasks

User interface changes

API changes

#494096: Using Chrome ConfirmForms do not double click prevention causing problems
#881494: Double-clicking the "Install Drupal" button completely breaks everything
#483876: Disable install button once clicked

Original report by @ltwinner

I'm working on a site for someone and they complained that if they double click quickly on the node save form two nodes get created. I was surprised at this and checked it myself and found it was true.

Why is there no double click prevention built into Drupal core? Surely this should be standard functionality? Lots of sites have this, I know vBulletin has it anyway from looking at their source code and you can see it yourself if you double click fast on a vBulletin forum's submit new post button.

So has this feature been overlooked for Drupal or is it left out by design?

CommentFileSizeAuthor
#181 interdiff_175-181.txt1.73 KBpoker10
#181 1705618-181.patch8.1 KBpoker10
#175 interdiff_130-175.txt8.67 KBpoker10
#175 1705618-175.patch7.51 KBpoker10
#130 interdiff-1705618-124-130.txt352 bytestorotil
#130 form-single-submit-1705618-130.patch5.39 KBtorotil
#124 interdiff-1705618-117-124.txt1.16 KBMatt V.
#124 form-single-submit-1705618-124.patch5.25 KBMatt V.
#120 interdiff-1705618-112-117.txt4.44 KBhanoii
#117 interdiff-1705618-112-117.txt2.84 KBhanoii
#117 form-single-submit-1705618-117.patch5.25 KBhanoii
#112 interdiff-1705618-110-112.txt1.59 KBhanoii
#112 form-single-submit-1705618-112.patch4.92 KBhanoii
#110 form-single-submit-1705618-110.patch3.33 KBhanoii
#85 form-single-submit-1705618-85.patch2.87 KBmgifford
#68 form-single-submit-1705618-68.patch2.87 KBmgifford
#63 form-single-submit-1705618-63.patch2.94 KBmgifford
#58 form-single-submit-1705618-57.patch2.9 KBmgifford
#49 form-single-submit-1705618-48-do-not-test.patch2.83 KBclemens.tolboom
#36 interdiff.txt3.64 KBsun
#36 drupal8.form-single-submit.34.patch2.88 KBsun
#33 interdiff.txt838 bytesWim Leers
#33 core-js-single-submit-1705618-33.patch3.04 KBWim Leers
#32 core-js-single-submit-1705618-32.patch2.93 KBnod_
#26 core-js-single-submit-1705618-26.patch2.92 KBnod_
#25 core-js-single-submit-1705618-25.patch2.9 KBnod_
#21 drupal8.form-double-submit.21.patch2.9 KBsun
#19 core-js-preventSubmit.patch1.81 KBnod_
#13 drupal.forms-system.1705618-13-DRUPAL7.patch491 bytesclemens.tolboom
#18 drupal8.form-double-submit.18.patch2.94 KBsun
#17 drupal8.form-button-multi-submit.do-not-test.patch2.01 KBsun
#6 drupal8.forms-system.1705618-6.patch488 bytesclemens.tolboom
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Priority: Normal » Major

Apart from the question why it's missing, I'd like to ask for how can we deal with it with least impact?

It's a very basic feature everybody expects in Drupal core.

franz’s picture

Version: 7.x-dev » 8.x-dev

One of the evil bugs this is known to cause: #494096: Using Chrome ConfirmForms do not double click prevention causing problems

We could start by buildind a way to test this, which involves trying to make 2 parallel equal posts to the same form, perhaps.

helmo’s picture

helmo’s picture

helmo’s picture

clemens.tolboom’s picture

Title: Double click prevention on form submission » GENERIC: Double click prevention on form submission
Status: Active » Needs review
FileSize
488 bytes

This is almost a META as we have too many issues similar to this one. I'm trying to revive the patch from #483876-6: Disable install button once clicked.

@helmo discovered Firefox is not re-submitting when double click on the submit button.

Attached patch is same as #483876-6: Disable install button once clicked but that issue is closed long time a go.

clemens.tolboom’s picture

Issue summary: View changes

Updated issue summary.

clemens.tolboom’s picture

Issue summary: View changes

Added steps to reproduce

clemens.tolboom’s picture

+++ b/core/misc/form.js
@@ -79,4 +79,15 @@ Drupal.behaviors.fillUserInfoFromCookie = {
+      $(e.target).bind('submit', function(e) { e.preventDefault(); });

Adding something like

      $(e.target).css('border', '3px solid red');

is a step forward? But I guess this needs a class and probably a spinner.

I've updated the issue summary with Steps to reproduce.

clemens.tolboom’s picture

Issue summary: View changes

Updated issue summary.

clemens.tolboom’s picture

[edit]wrong issue[/edit]

nod_’s picture

Issue tags: +JavaScript

tag

nod_’s picture

Title: GENERIC: Double click prevention on form submission » Double click prevention on form submission
Status: Needs review » Needs work

I'd rather disable the button clicked so that feedback is given to the user. Also it needs a detach method.

clemens.tolboom’s picture

@nod_ in #483876-6: Disable install button once clicked @seutje wants to disable the form.

But changing the button text would be a good user cue.

I'm still unsure about the ajax interaction and from what I read in #494096-14: Using Chrome ConfirmForms do not double click prevention causing problems @tim.plunkett writing tests for double click seems impossible :-/

clemens.tolboom’s picture

Issue summary: View changes

Added user/register example

nod_’s picture

oh that's right, haven't thought of that, he has a fair point. Looks like we could do both to get the feature and the user feedback.

clemens.tolboom’s picture

Version: 8.x-dev » 7.x-dev
Issue summary: View changes
Status: Needs work » Needs review
FileSize
491 bytes

We applied patch to a D7 forum site where we had too much double postings from our users.

clemens.tolboom’s picture

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

I failed to make the button text change as the current solution changes form.submit and has no context of what button triggered it.

helmo’s picture

The D7 patch from #13 is working great. Let's fix this before even more duplicate issues pop up ;)

sun’s picture

Status: Needs work » Needs review
FileSize
2.01 KB

Briefly discussed this issue with @nod_ in IRC.

Double-clicks/submits present a major problem, since Drupal, as a non-persistent/daemonized and scripted PHP application is architecturally and technically not able to detect the situation. The moment you click a form submit button, your user agent/browser issues a new HTTP request to the web server. The PHP script on the web server does not have a shared application state; any information that may be contained in previous or parallel requests is unknown.

Duplicate form submits can happen within milliseconds (typical double-click, typically performed by users being used to Microsoft Windows), but they can also happen in intervals of seconds (typically caused by the web site not "reacting" due to too much server load or too expensive and slow form processing operations).

In all cases, it is the end-user on the client-side, who triggers consecutive form submits of the identical data.

Based on my own testing, all modern browsers - except Firefox - do not implement a double-click/submit protection at this point. Until all major browsers implement such a protection, Drupal core should provide a custom protection, in order to prevent multiple (and identical) form submits.

Such a custom protection can only be implemented via JavaScript. In turn, this means we can't do anything about clients that have JavaScript disabled.

As a developer, I'd like to be able to simply specify the following for the buttons of my form, in order to signify that only a single submit shall be possible:

$form['actions']['submit'] = array(
  '#single' => TRUE,
);

The biggest question is whether it is technically feasible to implement an opt-out instead of an opt-in.

That is, because form buttons may have further event handlers attached to them (i.e., Ajax and other stuff). A protection against multiple submits could potentially break such implementations.

As far as I understood the built-in double-submit protection in Firefox, that one actually checks whether any aspects of the form values to be submitted have changed. If there are any differences, a consecutive form submit is executed. But if there are no changes compared to the previously invoked form submit, then a consecutive form submit is ignored.

In an ideal world, we'd re-implement aforementioned logic in JS. But I'm not sure whether that is technically feasible. (?)

On top of that logic, all forms using a method="GET" are idempotent by design and HTTP standards: Any amount of form submits will lead to the exact same response and result. For that reason, all forms being submitted via GET do not need this protection.

Since @nod_ mentioned that an opt-out approach should be possible (and might even allow us to clean up some other stuff), I prototyped a first patch for the backend related Form API changes, using an opt-out approach, too. That is, because pretty much every (POST) form in Drupal results in massive havoc, in case it is submitted more than once.

Regarding the client-side implementation, it would be interesting to see whether we can feature-detect Firefox's built-in protection, so we do not duplicate that?

sun’s picture

OK, I guess @nod_ will hate me, but SCNR ;)

Here's a super-naive and overly simplistic re-implementation of the built-in double-submit protection in Firefox. :)

Thoughts? :)

nod_’s picture

Still not finished but my take on it, closer to what happens on ajax submissions.

sun’s picture

After further discussion in IRC, merged what could be merged from @nod_'s patch, but retaining the value-based approach (as opposed to button-based approach), removed the server-side facility that allowed individual buttons to opt-out, and added a swath of documentation, primarily to explain what is expected to be supported and what isn't (and why).

Thoughts? :)

Bojhan’s picture

Interesting

clemens.tolboom’s picture

Looks like a decent solution but iirc someone (@bojhan?) asked for a visual cue in one of the other related issues. (I cannot find that one).
- changing the button text
- disabling the form visual
?

sun’s picture

I can perfectly understand the desire, but I'd be very hesitant with the idea of manipulating anything in the user interface. Doing so inherently requires you to manage the state of all manipulations you are performing, which gets incredibly complex very fast.

And once you start to manipulate the interface, any other potentially running behaviors/scripts inherently need to be aware of this facility, in order to react or integrate accordingly.

If you think through that direction to its full extent, you will quickly end up with a fully-fledged state tracking machine for forms, which most other scripts would have to integrate with. — I think that would be overkill and not really desired.

Another argument for not manipulating the interface would be consistency with (1) regular browser/user-agent behavior and (2) the vast majority of other websites.

Considering the built-in double-submit protection in Firefox, there is no visual indication when that kicks in either - the browser is just simply smart enough to figure out that you did not intend to post the identical form values more than once under the hood.

Likewise, a custom double-submit protection as proposed here is not a new invention; the approach is implemented on many websites already - but despite that, you'll hardly find a site that provides an indication in the interface. (In fact, based on my personal experience, the few sites that are manipulating the interface are exactly the sites on which you typically run into weird user interface behavior bugs, because yes, proper state tracking is rocket science ;))

I'd highly recommend to keep this implementation as simple as possible. We should bear in mind that we're working around a problem space that is 100% in the domain of user agents and really ought to be addressed there (following Firefox's lead), as opposed to custom client-side scripts. — In fact, in parallel to this Drupal specific workaround, we should ensure to file bug reports/feature requests to the major browser vendors, so that we're hopefully able to remove this custom fix from Drupal core as soon as possible.

Thoughts?

nod_’s picture

Works great as far as I can see.

Made a couple changes:

  • strict equal (enforced by JSHint)
  • Using the actual data attribute (namespaced with drupal) instead of jquery .data() thing, so that other scripts can see what's happening and more importantly to not require jQuery for this. Planning on a Drupal.form.serialize() method down the line so we can swap jquery out as well.

Simple and straightforward, sun++

nod_’s picture

Bojhan’s picture

@sun Could you elaborate on the occasions where it goes wrong? I imagine one could click on it multiple times and get confused that it doesn't get executed (for many reasons this could happen). I do agree that, if we cannot be fully reliable - that we shouldn't expose this to the end-user, it is nice though if we can.

nod_’s picture

Assume the website is slow, loading page or submitting a form takes a couple seconds (that's not that unusual to see).

Submit a form once.
Script disable the button that was clicked (or even all submit buttons on the form).
You made a mistake in the form, you hit cancel/stop. Buttons are disabled, you can't do anything with the form. With this solution as long as you change a value you can resubmit the form.

For state control that goes wrong. Look at the toolbar. Currently the menu button is not behaving properly.

sun’s picture

@Bojhan:

The main problem of interface manipulations is that you need to manage the state between form ↔ buttons (plural), whereas a form submit is triggered through one of the buttons only, and multiple buttons (but not necessarily all buttons) may submit the form.

There may also be buttons in the form that do not submit the form, but are triggering Ajax and client-side operations only. In addition, a form can be submitted via keyboard and without a button at all.

Upon form submit, you'd have to (1) figure out which button has been clicked [and if the submit was triggered via ENTER in a text field, the button to be clicked is not clearly defined by the HTML spec; cf. Poll choice edit form], (2) whether the clicked button submits the form or triggers client-side operations only, and if those complex conditions are met, (3) disable all buttons that can submit the form (Save, Preview, Delete, etc) and potentially replace their button labels with "Please wait...", and lastly (4) hope that the form will actually be submitted and will cause a HTTP redirect.

In case anything goes wrong, you need to revert the interface manipulations of step (3), which in turn requires you to record all changes that were performed and also to backup the original states, so you can revert them.

However, my primary concern with manipulating the interface is a usability/consistency concern: This would affect all forms and the behavior would be a Drupalism, which you do not see on any other website.

At the same time, it is a problem that ought to be solved and hopefully will be solved by native web browsers. Out of the major web browser vendors, Firefox doesn't appear to be far away of being able to (consistently) provide feedback in the interface, and naively speaking, I assume that the only reason for why they didn't do it was that Firefox would be the only browser who'd be doing it (→ Firefoxism). Once other vendors are catching up, there's a good chance for a native cross-browser experience to arise, which will be consistent on all sites on the net.

@nod_: Thanks for the polishing! :) Questions:

1. Do we want to namespace/identify the 'submit' event into 'submit.single'?

2. Speaking of events, do we want to trigger an event on e.preventDefault(), or do you think that e.isDefaultPrevented() will be sufficient for potential edge-cases?

3. I guess the .data() → .attr() change is fine, and I didn't think of that possibility. Though one thing that we should ensure is to not break a built-in protection in browsers by manipulating the DOM. The easiest way to test that is to start Firefox and (1) comment out the e.preventDefault() line and (2) double-click the Save button on the comment form.

4. Speaking of built-in protections, there's still the question of whether we're able to feature-detect them (and in turn, skip this custom behavior if available). However, that is minor and could happily be done in a follow-up issue.

nod_’s picture

1. Yes, and it's relates to 2. As well. But it should be something like submit.preventDoubleSubmit or something. Out of context, single doesn't make sense.

2. For edge cases people can always $('body').off('.preventDoubleSubmit'); and remove that altogether. That's why single isn't specific enough too.

3. shouldn't mess it up, have to test.

4. Haven't seen anything that'd allow us to feature detect so far.

Bojhan’s picture

Thanks for the thorough response! I think this is fine to commit without it affecting the UI. I am less concerned about Drupalisms when it enhances the UI greatly, in this case its questionable it will even behave nicely in all cases - therefor lets get this in :)

nod_’s picture

FileSize
2.93 KB

Rerolled patch to get the event namespaced.

Commenting preventDefault, Firefox still works fine with the attr change.

Wim Leers’s picture

FileSize
3.04 KB
838 bytes

Tested manually. Works great :) So glad to finally see this getting resolved!

I wanted to RTBC, but … I was testing edge cases, and I found one in which it broke down: <form method="get"> works, <form method="GeT"> works too, but… <form method=" get"> fails. I.e. the selector in the JS is case insensitive (good), but requires zero spaces around the value (bad). Changing the selector from [method="GET"] to [method~="GET"] fixes that.

I also think the context parameter should be removed since this behavior does not actually use the context at all.

nod_’s picture

Fine with me, now who'll RTBC this now? :þ

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

I only made very minor changes. I was not involved at all with the making of this patch. Hence I don't think it's a stretch for me to RTBC this.

So: RTBC!

sun’s picture

Sorry, I wrote those docs in the middle of the night... just removing some snarky language there ;)

Wim Leers’s picture

Hah :) I personally found it acceptable :)

Re-reviewed. Still RTBC.

tim.plunkett’s picture

This issue was kicked off again in #17, which mentions an opt-in solution.
That is later revised to be opt-out.
And then in a patch after that, it was removed altogether, and this seems to be forced for all forms.

Why was the opt-in or opt-out part abandoned?

Also the issue summary is very out of date.

sun’s picture

The opt-in/opt-out part was suggested in #17 and was pretty much abandoned directly after in #18, after typing out how Firefox's built-in protection appears to work and realizing how that can be translated into JavaScript.

The form value-based approach only kicks in when an actual form submit is executed. The protection only applies when a form is actually submitted through regular means. In turn, there's no need for anyone to opt-in or opt-out, because there is no use-case for doing so.


FWIW, since there is no shared approach yet, the main concept of this code has been converted into a stand-alone library, since the problem is not unique to Drupal but affects many other web applications, too:
https://github.com/sun/jquery-form-submit-single

But to be clear, the hope is that this issue will ultimately be resolved directly in all major web browsers, eliminating this custom client-side workaround (in Drupal and elsewhere).

tim.plunkett’s picture

#39 is a great explanation, I have no problem with it, it just wasn't clear before.
That would be good info to put in the issue summary.

xjm’s picture

Dries’s picture

Category: Feature request » Bug report
Status: Reviewed & tested by the community » Fixed

I believe this is a bug fix, not a feature request. Committed to 8.x.

webchick’s picture

Just wanted to cut in and say this is a really clever solution to this long-standing problem. It's totally transparent to the end-user, doesn't result in weird "hung up" issues on a disabled button when a form died in the middle of processing, etc. and yet achieves the goal in not quadruple+ posting things accidentally.

Great work!

webchick’s picture

Also, can/should we backport this to 7.x and 6.x?

Wim Leers’s picture

YAY! So happy that in the not-too-distant future, I won't be able to break things this way anymore! :)

Thanks, sun!

sun’s picture

This could be backported to D7 and D6, but before/when doing so, I think we should pull in the library from https://github.com/sun/jquery-form-submit-single instead, so that we have a properly managed dependency that may be shared and improved by multiple projects.

helmo’s picture

Issue summary: View changes
Status: Fixed » Needs work
Issue tags: -Needs issue summary update +Needs backport to D7

Great work and idea to create a proper library for this. Hope we can quickly get this in.

nod_’s picture

Version: 8.x-dev » 7.x-dev
clemens.tolboom’s picture

Status: Needs work » Needs review
FileSize
2.83 KB
+++ b/core/misc/form.js
@@ -60,6 +60,63 @@ Drupal.behaviors.formUpdated = {
+    $('body').once('form-single-submit')
+      .on('submit.singleSubmit', 'form:not([method~="GET"])', onFormSubmit);
+  }
+};

How should one backport this as on() is jQuery 1.7 ?

I did this

+    $('form:not([method~="GET"])').submit(function(e) {
+      onFormSubmit(e);
+    });

which seems to work but this needs some feedback to make it better.

Some observation made while testing

  1. misc/form.js is not included on ie http://drupal.d7/admin/people/create (it is on D8). That means related issues needs still another fix?
  2. a form submitted through code by a link is not protected
  3. D7 patch from #13 prevents submission when using author autocomplete using enter key to select user. People using that should check this new one.
clemens.tolboom’s picture

Patch from #49 breaks on admin/structure/features/*/recreate when adding a view.
[edit]On Firefox but not on Chrome[/edit]

fawwad.nirvana’s picture

After long discussion i think there is no solution till now. If any body know please share.

clemens.tolboom’s picture

@fawwad.nirvana please help this issue forward.

This issue is not long, is fixed in Drupal 8 and needs review for it's Drupal 7 derivative from #49

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Just tested this in D7. Works great! Would like to see it brought into the next release.

fawwad.nirvana’s picture

@clemens.tolboom D7 patch from #49 prevents submission when using Hierarchical Select.
Actually there is a function Drupal.behaviors.formSingleSubmit is calling twice whenever i have selected Hierarchical Select drop down. So for time being i did some changes in bold letter but this is temporary solution. I do not know why this function is calling two times.

Drupal.behaviors.formSingleSubmit = {
attach: function () {
function onFormSubmit (e) {
var $form = $(e.currentTarget);
var formValues = $form.serialize();
var previousValues = $form.attr('data-drupal-form-submit-last');
var loop = window.submitloop;
if (previousValues === formValues && loop!=1) {
e.preventDefault();
}
else {
$form.attr('data-drupal-form-submit-last', formValues);
window.submitloop=1;
}
}

$('form:not([method~="GET"])').submit(function(e) {
onFormSubmit(e);
});
}
};

clemens.tolboom’s picture

There are still questions need for an answer. See #49

Next we need to add the patch again without -do-no-test to run D7 tests.

@fawwad.nirvana please add more info. I have no clue on what to do to setup that module to do a manual test. Please add steps to reproduce similar as the summary has. Preferably a drush script.

mgifford’s picture

Status: Needs work » Needs review

This re-roll includes @fawwad.nirvana's suggestion, but it seems to be missing the if(window.submitloop==1) which would need to be there.

On the questions in #49

  1. Let's open up a new related issue for admin/people/create since we can't rely on misc/form.js there.
  2. A form submitted through code is an edge case I think. This would be a problem with the D8 solution as well, right?
  3. There are definitely problems with the author autocomplete on D7 & D8, but don't think they are related to the patch.
mgifford’s picture

nod_’s picture

patch needs to use .once() not the submitloop hack.

mgifford’s picture

So, #49 + something like this:

Drupal.behaviors.formSingleSubmit.once(function() = {
...
});
nod_’s picture

Sorry was on my phone yesterday.

+    $('form:not([method~="GET"])').submit(function(e) {
+      onFormSubmit(e);
+    });

We can't use .on() but we can use .delegate(), so replacing this piece of code by:

      $('body').once('form-single-submit')
        .delegate('form:not([method~="GET"])', 'submit.singleSubmit', onFormSubmit);

then the code is pretty much exactly the same as D8.

Yes programatic submit could be an issue but It's not our business to mess with it. It's purely a real user click on a button thing.

This should fix autocomplete problems no?

nod_’s picture

Status: Needs review » Needs work
mgifford’s picture

Status: Needs work » Needs review
FileSize
2.94 KB
mgifford’s picture

The patch works fine now for content. I took a quick look at admin/people/create and hitting submit many times didn't have the same result as through a node. Mostly I think because you need that unique email address.

Anyways, thanks for your direction @nod_ it seems to work fine in my manual tests. Really not familiar enough with JS to have constructed that myself. Hopefully the bot likes it too.

clemens.tolboom’s picture

Any chance for an interdiff with the D8 version?

mgifford’s picture

This isn't giving me anything useful:
$ interdiff drupal8.form-single-submit.34.patch form-single-submit-1705618-63.patch

There are just enough little differences I think to be able to pull out something discrete.

fawwad.nirvana’s picture

@mgifford
I think in form-single-submit-1705618-63.patch there is no need of
var loop = window.submitloop;
and window.submitloop=1;
so we have to remove these.

mgifford’s picture

I thought i'd removed those.. Guess not.

helmo’s picture

Status: Needs review » Reviewed & tested by the community

Just tried the patch from #68 again on Core 7.29 and still works OK.

helmo’s picture

Status: Needs work » Reviewed & tested by the community

CollapsedInsert tests, LOB fields (DatabaseInsertLOBTestCase) [Database]

Sounds like a testbot hickup...

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Needs work » Needs review

Status: Needs work » Needs review
helmo’s picture

Status: Needs review » Reviewed & tested by the community

testbot back to normal :)

Status: Needs work » Needs review
tim.plunkett’s picture

+++ b/misc/form.js
@@ -58,6 +58,64 @@ Drupal.behaviors.formUpdated = {
+    $('body').once('form-single-submit')
+        .delegate('form:not([method~="GET"])', 'submit.singleSubmit', onFormSubmit);

Isn't this second line indented too far?

mgifford’s picture

Yup...

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

It seems like @nod_ was okay with this patch as it was, so back to RTBC.

rakesh.gectcr’s picture

@mgifford #85
Thanks! the patch working fine for me....

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 85: form-single-submit-1705618-85.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 85: form-single-submit-1705618-85.patch, failed testing.

Status: Needs work » Needs review
helmo’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc ... agian.

JvE’s picture

The patch from #85 mad ethings worse for me.
Without patch I was able to create 2 duplicate nodes, with patch that went up to 6.

Any suggestions as to why this happens on my site?
I use jquery_update with jQuery 1.7
I also use the unique_field module to set the node titles unique but that does not help either. I get the error message from that module, but I also get duplicate nodes.

helmo’s picture

@JvE: could you be more specific? What do you mean by "the error message".
Did you clear your JS cache after applying the patch?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 85: form-single-submit-1705618-85.patch, failed testing.

JvE’s picture

Status: Needs work » Reviewed & tested by the community

@helmo: Thank you.
Turns out I ran into a bug in Chrome: Cached javascript resources are used even on forced refresh.

Patch seems to work as intended.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

As others have pointed out above, this patch does nothing to fix many forms throughout Drupal (the user registration form is one of them mentioned above, but plenty of others) because this JavaScript file isn't even used on those pages. If you look at where form.js is added to the page in Drupal 7, it's really not that many places... forms with collapsible fieldsets and that kind of thing. Almost nowhere else.

It doesn't make sense to have this protection in core but then almost never use it. If it's supposed to be generic shouldn't the code be added in drupal.js instead? (I am not sure if this affects Drupal 8 too.)

Also, I see a lot of discussion of Chrome and Firefox above... has anyone tested this in Internet Explorer at all?

+    $('body').once('form-single-submit')
+      .delegate('form:not([method~="GET"])', 'submit.singleSubmit', onFormSubmit);
+
+  }

(minor) The blank line at the end should not be there (and it wasn't in the Drupal 8 version).

JvE’s picture

I have tested it with the node creation form in Firefox (33), Chrome (38), Internet Explorer (7, 8, 9 and 10) and confirm that without the patch double-submit occurs on all but Firefox.
With the patch double-submit is prevented without errors in all these browsers.

I did notice a side-effect on Firefox:
If you submit a form and something goes wrong server-side for whatever reason, and you navigate back, you can not resubmit the form.

I agree that this fix should not be in form.js. That is not added to forms without fieldsets or vertical tabs.

clemens.tolboom’s picture

Issue summary: View changes

I somehow expected forum comments would be fixed too with this patch. But this is not the case.

I agree that this fix should not be in form.js. That is not added to forms without fieldsets or vertical tabs.

@JvE thanks for pointing out fieldset / vertical tabs. Any idea whether we could build a matrix of forms lacking this?

puddyglum’s picture

Edit: I created a new issue for a potential server-side fix for this (for authenticated users) #2449847: Prevent duplicate form submissions for authenticated users

GuyPaddock’s picture

As a workaround for David Rothstein's concern about the form.js not being loaded consistently throughout the site, we're using the patch from #85 and running with the following snippet in a "usability" feature we use site-wide:

/**
 * Implements hook_init().
 */
function my_site_usability_init() {
  // Pull in the Drupal forms library always, to address DDO-1705618.
  drupal_add_library('system', 'drupal.form');
}

That seems to make this fix work for all the forms we need it to work on. Are there any side effects to having the form library loaded always?

clemens.tolboom’s picture

Status: Needs work » Needs review

I ran into double click using https://www.drupal.org/project/button_field

The last testbot failure of #85 looks like a kickup so let's retest.

@GuyPaddock will test your #102

clemens.tolboom’s picture

Issue summary: View changes

Make sure to test with Chrome

zterry95’s picture

how about using lock mechanisms to proceed this issue?

https://api.drupal.org/api/drupal/includes!lock.inc/group/lock/7

GuyPaddock’s picture

@zterry95: This issue is client-side. Locking is server side. I don't see the connection. We're talking about preventing multiple requests from kicking off from a double-click in the browser, not making sure that the requests process one-by-one.

I will say, though, that in a case where the server needs to ensure that there are not any duplicates created (Profile2, for example), a lock or database constraint is typically necessary. But, whether or not a lock or constraint is required would depend on the situation. In the case of the node add form, there is currently no duplicate node check (unless a validation is configured by the site admin) so a lock would not be of use in that situation.

johnhanley’s picture

What's the status of this patch and more specifically the back-port to D7?

  • Dries committed 502e953 on 8.3.x
    Issue #1705618 by nod_, sun, clemens.tolboom, Wim Leers: Double click...

  • Dries committed 502e953 on 8.3.x
    Issue #1705618 by nod_, sun, clemens.tolboom, Wim Leers: Double click...
hanoii’s picture

I kind of needed this for a site on D7. Attached is the same patch that was committed to 8.x, with the addition of making sure form.js everytime there's a button, by adding the library on the ajax_process_form() which is the process function for both 'button' and 'submit' for elements as per system_element_info() .

This was to solve what @David_Rothstein said on #98. Another option was doing what he said and adding it to drupal.js, but I wanted to keep this form-related and similar to d8.

Status: Needs review » Needs work

The last submitted patch, 110: form-single-submit-1705618-110.patch, failed testing.

hanoii’s picture

Status: Needs work » Needs review
FileSize
4.92 KB
1.59 KB

Ok, I am probably gonna be knackered about this, because I am removing a test, but I really didn't find a way around it.

This was reason enough to confirm that this code should not be in drupal.js, as that relies on jquery, but adding that makes the default homepage always to need jquery because of the login form and its button, which is adding form.js, and that adding jquery.

So testing for javascript_always_use_jquery in the standard homepage does not work any more as expected, although that's not necessary an issue.

javascript_always_use_jquery is still tested further down under other circumstances.

Status: Needs review » Needs work

The last submitted patch, 112: form-single-submit-1705618-112.patch, failed testing.

hanoii’s picture

Status: Needs work » Needs review

hmm, not sure why that failed, trying again?

helmo’s picture

Thanks @hanoii, it works ok and it would be nice to get this committed finally.

JvE’s picture

Status: Needs review » Needs work

After this patch I can no longer use

$conf['javascript_always_use_jquery'] = FALSE;

if I have a search form on my homepage? That doesn't seem right.

hanoii’s picture

@JvE why would someone want to use that config? :) just kidding..

I honestly just found about its existence while working on this patch. I agree that adding a new functionality by affecting a previous one is not the way to go in pushing this to be accepted.

This is another attempt at it. Instead of adding the library on ajax_process_form() which was added by several others elements and has really nothing to do with this, I created a new form_process_button() . In that function, I am only adding form.js on buttons if javascript_always_use_jquery is not set to false. By doing this, I readded the test I removed before.

So the end behaviour would be:

- Normal sites will get the double submit protection by properly adding form.js and its dependencies when a button is added.
- Sites that uses javascript_always_use_jquery, will keep the exact same behaviour as before, albeit not having the new double submit protection except when specifically added by any other reason (again, as before).

hanoii’s picture

Status: Needs work » Needs review
hanoii’s picture

BTW, in looking for why I didn't know about javascript_always_use_jquery I found it was added on 7.36, as per #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page.

I don't think there's much documentation about it except https://www.drupal.org/node/2462717.

Eventually if this is accepted probably worth mentioning about the special case of double submit somewhere in the change record.

hanoii’s picture

FileSize
4.44 KB

The interdiff was incomplete.

nithinkolekar’s picture

this patch should be must have for all commerce sites having add to cart, pay-now button etc.

chegor’s picture

Using this patch on several sites. Can we move this into the core?

nithinkolekar’s picture

until this patch get committed , is anyone experienced avoiding double submit using captcha technique? (just alternative solution for time being)

Matt V.’s picture

The patch from #117 looks good to me and works well. That said, I made some small modifications to the comments, to correct minor typo and grammar issues.

Status: Needs review » Needs work

The last submitted patch, 124: form-single-submit-1705618-124.patch, failed testing.

  • Dries committed 502e953 on 8.4.x
    Issue #1705618 by nod_, sun, clemens.tolboom, Wim Leers: Double click...

  • Dries committed 502e953 on 8.4.x
    Issue #1705618 by nod_, sun, clemens.tolboom, Wim Leers: Double click...
torotil’s picture

Status: Needs work » Needs review

Seems that this was just a CI fluke. Setting to "needs review" as the tests pass now.

torotil’s picture

Status: Needs review » Needs work

This patch basically breaks all JavaScripts that do asynchronous tasks during submit().

In order to do that such a JavaScript usually calls event.preventDefault() and after it's done it calls submit() again. The second submit is then prevented by the JS in this patch. That means the form is never submitted.

Maybe we could only count a submission event if e.isDefaultPrevented() is false. This would at least work if this submit-handler is called last.

torotil’s picture

This would at least work if this submit-handler is called last.

It seems delegate() handlers are called after normal submit handlers. So skipping the check if the submit was already cancelled will work for most JavaScripts out there.

I've made this change to the original patch.

Michelle’s picture

Status: Needs review » Needs work

WIth #130, if I create a new node and double click on the save I get two nodes created. Shouldn't this prevent that?

torotil’s picture

The patch works as expected for me: Without the patch I can create duplicate nodes like you describe. With the patch I can't.

Are you sure the patch was applied and no cached version of the JS was used?

Michelle’s picture

The patch was applied, yes. I don't know about js caching. I did a drush cc all. I ended up going with the Hide Submit module instead of this as that worked for us. If it's working for you, then I guess put it back to needs review.

torotil’s picture

Status: Needs work » Needs review

A drush cc all doesn't cut it in this case. Core JS only gets a core-version suffix if that doesn't change (and it won't for a patch) the JS will be served from your browser's cache.

CKIDOW’s picture

Hey... thx for this patch in advance!

If there are more than one drupal form on the page, the behavior is only attached to the first form. Are there any solution on this?

torotil’s picture

@CKIDOW: The behaviour is not attached to any specific form, but to the body itself (waiting for events to bubble up).

Could you provide a minimal test-case?

monta’s picture

Status: Needs review » Reviewed & tested by the community

#130 patch works fine in 7.56

chrisrockwell’s picture

Not ready to set back to needs work but I'm experiencing the same as @Michelle in #133, doing some debugging now.

robertwb’s picture

I can say that #130 is functioning as expected on chrome with Drupal Core 7.56. Suggest clearing browser cache before testing for those having trouble.

jebeze_alexander’s picture

I know that this is a javascript/jquery only solution and not at all a sitewide fix, but for those of you looking for a quick fix for specific issues until you can get the patch working, here is the workaround that I found that works.

I created a block and made it visible only to authenticated users on specific content types with the following embedded code:

SCRIPT

$(function () { 
    var clickCount = 0;
    $("#edit-published").click(function(event){
        if(clickCount++ > 0) {
            event.preventDefault()
        }
    });
    $("#edit-submit").click(function(event){
        if(clickCount++ > 0) {
            event.preventDefault()
        }
    });
});

/SCRIPT

The code increments a counter after a button click, and executes preventDefault when the counter is above zero. This effectively disables additional clicks on any of the identified buttons.

This solves my specific problem: non-tech-savvy content managers who don't understand the difference between single and double-click when either saving drafts or publishing content (hence the two buttons identified in the code).

I think this can be easily adjusted to fit other specific issues by figuring out the button ID and loading the code only on the necessary pages.

mdjamiruddin’s picture

I think, double click prevention feature can be managed by disabling the button once it is clicked by JS. The server side script can be developed in such a way that can also handle this double click without any impact.

albertski’s picture

Verified the patch looks good. Tested the patch and it works great (At first it didn't work but once I cleared my browser cache it worked).

marc.groth’s picture

Further to jebeze_alexander's comment... If you need to consider required fields when clicking the submit button then simply update the code to be on form submit instead i.e.

$(function () { 
    var clickCount = 0;
    $('form').submit(function(event){
        if (clickCount++ > 0) {
            event.preventDefault();
        }
    });
});
MustangGB’s picture

Issue tags: +Drupal 7.60 target
joseph.olstad’s picture

Issue tags: -Drupal 7.60 target +Drupal 7.70 target

Bumping to 7.70, this didn't make it into 7.60

The last submitted patch, 25: core-js-single-submit-1705618-25.patch, failed testing. View results

jOksanen’s picture

I noticed this behavior in drupal registrations and login as well. We were losing a few customers due to this. My workaround is just vanilla javascript on the forms pages.

var form = document.querySelector('form');
form.addEventListener('submit', function() {
this.querySelector('input[type="submit"]')
.setAttribute('disabled', 'disabled');
}, false);

torotil’s picture

I’m using #130 on multiple sites with AJAX, clientside validation and other forms JS form processing since March 2017: No problems since then. Anyone else using this in production?

Oliver Huynh’s picture

Check out here:
https://github.com/oliverhuynh/prevent_double_submit

I have no time to make it as a project

joseph.olstad’s picture

Issue tags: -Drupal 7.70 target +Drupal 7.66 target

patch #130 could go in any time really, a version of it is already in D8. For those having issues it's your browser cache, clear your browser cache.

commonpike’s picture

drupal 7.66 is out. is this fix in there ?

mcdruid’s picture

Issue tags: -Drupal 7.66 target +Drupal 7.68 target
joseph.olstad’s picture

joseph.olstad’s picture

MustangGB’s picture

Issue tags: -Drupal 7.68 target +Drupal 7.69 target
chegor’s picture

Issue tags: -Drupal 7.69 target +Drupal 7.70 target
ebodor’s picture

Any luck getting the patch committed in 7.70? The patch has been out there for three years and fixed for D8 awhile now.

chegor’s picture

Issue tags: -Drupal 7.70 target +Drupal 7.73 target

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 130: form-single-submit-1705618-130.patch, failed testing. View results

joseph.olstad’s picture

Status: Needs work » Needs review

retriggered test, appears to be a test runner issue.

LeDucDuBleuet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Drupal 7.73 target +Drupal 7.74 target

Any chance of getting this patch in the next 7.74?

Ron Collins’s picture

+1 to committing this. It's been8 years. It's not getting any better. Applies cleanly and solves the problem. Not sure what the testing issue is but doesn't look related.

izmeez’s picture

Just updating issue tag.

chegor’s picture

ressa’s picture

This issue is included in #3192080: [meta] Priorities for 2021-04-07 release of Drupal 7, so tagging with "Drupal 7.xx target" probably isn't necessary any longer ... Remaining issues will be transferred to a new meta issue for the next planned D7 release.

Let's try using meta issues to list the priority issues for upcoming bugfix / maintenance releases of D7.

Using the e.g. "Drupal 7.74 target" tags frequently gets messed up by security releases, and it's generally harder to keep track.

From #3179845: [meta] Priorities for 2020-12-02 bugfix release of Drupal 7.76 / 7.77

poker10’s picture

Any chance, this will be included in the upcoming D7 December release? I don't think there will be many maintenance releases until D7 EOL, so it will be worth to push it the sooner the better.

izmeez’s picture

@poker10 you could add a comment to #3216978: [meta] Priorities for 2021-12-01 release of Drupal 7 with your request to "bump" it up. It's already in the issue summary.

mcdruid’s picture

Issue tags: -Drupal 7.79 target

First pass review; this looks pretty similar to what's in D9:

https://git.drupalcode.org/project/drupal/-/blob/9.3.0-beta3/core/misc/f...

However, the addition made in #130 is not there.

Does the statement in #129

This patch basically breaks all JavaScripts that do asynchronous tasks during submit().

...also apply to D8/9 then?

If so, the change in #130 should be made in D9 and backported.

On that basis, I'd be more comfortable committing a previous patch which more closely matches D9 today.

I'll have another look at this tomorrow (although I intend to begin a freeze pretty much now before the release in one week).

Am I missing something about why #129/130 only applies to D7?

mcdruid’s picture

Noting that the change in question is included here (this project is mentioned in the IS):

https://github.com/sun/jquery-form-submit-single/blob/master/src/jquery....

Is there an existing issue to add that in the D8/9 queue?

joseph.olstad’s picture

@mcdruid there is not an existing issue as far as I can see but it could be that D8/D9 does not have this issue

scouring the code I found:

grep isDefaultPrevented * -R| grep -v '.min'
drupal/core/assets/vendor/jquery-form/src/jquery.form.js:		if (!e.isDefaultPrevented()) { // if event has been canceled, don't proceed
drupal/core/assets/vendor/jquery-joyride/jquery.joyride-2.1.js:          if (!event.isDefaultPrevented() && event.keyCode &&
drupal/core/assets/vendor/jquery.ui/ui/widgets/dialog.js:				if ( this.options.closeOnEscape && !event.isDefaultPrevented() && event.keyCode &&
drupal/core/assets/vendor/jquery.ui/ui/widgets/dialog.js:				if ( event.keyCode !== $.ui.keyCode.TAB || event.isDefaultPrevented() ) {
drupal/core/assets/vendor/jquery.ui/ui/widget.js:			event.isDefaultPrevented() );
drupal/core/assets/vendor/jquery/jquery.js:		this.isDefaultPrevented = src.defaultPrevented ||
drupal/core/assets/vendor/jquery/jquery.js:	isDefaultPrevented: returnFalse,
drupal/core/assets/vendor/jquery/jquery.js:		this.isDefaultPrevented = returnTrue;
drupal/core/assets/vendor/jquery/jquery.js:		if ( !onlyHandlers && !event.isDefaultPrevented() ) {
drupal/core/modules/editor/js/editor.js:          if (event.isDefaultPrevented()) {
drupal/core/modules/editor/js/editor.es6.js:          if (event.isDefaultPrevented()) {

appears that D9 already takes care of this in various checks, no one has reported an issue

mcdruid’s picture

@joseph.olstad IIUC isDefaultPrevented() is part of the jQuery API:

https://api.jquery.com/event.isdefaultprevented/

...and the fact that it appears elsewhere in D9 doesn't mean this specific difference between what's in D9 and what's in the patch under review can be ignored.

I'm inclined to commit #124 on the basis that a follow-up is filed for D9 to add the extra line(s) discussed in #168 which would then be backported.

mcdruid’s picture

Filed a child issue for D9: #3251249: Should double-click prevention return early when isDefaultPrevented?

In the meantime we can discuss whether committing #124 (which matches what's in D9 closely) to D7 would cause problems, and if so whether workarounds would be available etc..

poker10’s picture

I think that one possible problematic scenario could be when some contrib module, theme, or custom code intercepts the form submit process in a way like this:

  $('#form-id').one('submit', function(){
    event.preventDefault();
    // do something asynchronous and when finished submit the form again
    setTimeout(function() {
      $('#form-id').submit();
    }, 1000);
  });

This will cause that the initial form submit is prevented, but the code which adds the data-drupal-form-submit-last attribute will still run. And then the submit() manually triggered after the asynchronous call will not pass the check anymore, because the attribute will be already added in that time:

if (previousValues === formValues) {
  e.preventDefault();
  ...
}

Maybe this is not the exact usecase mentioned by the comment #129, but still some D7 sites could be using something like this. In D8 this probably does not caused problems, because the doubleclick prevention was introduced pretty early (in 2014). But we should be cautious for D7.

Potentially, could we also think about adding a possibility to opt-out of this? Especially if we do not include the fix from #130. Then it could greatly help sites which will encounter any problems caused by this change.

-----------

Edit: Also one additional D7 behavior change (already mentioned in #99):

I did notice a side-effect:
If you submit a form and something goes wrong server-side for whatever reason, and you navigate back, you can not resubmit the form.

poker10’s picture

Actually @bnjmnm pointed out in the D10 child issue, that we should be also cautions while including the form.js on all pages where it was not previously included (for example like the mentioned admin/people/create and similar). form.js contains also other features, which can probably? cause issues on existing sites. D8 patch also does not forced the form.js to all pages with forms, it was included only on pages where form.js actually was loaded (see child issue about the comment form).

So maybe it would be better to add this new behavior (Drupal.behaviors.formSingleSubmit) to the new separate JS file and include that file like it is done in the current patch, together with the option to opt out via a new settings variable (not only the javascript_always_use_jquery, as it has a different purpose).

Also agree with @mcdruid, that it should be better to commit the patch which is the same as D8/9/10 version and then only introduce additional conditions via a backport of the #3251249: Should double-click prevention return early when isDefaultPrevented?.

poker10’s picture

We have discussed this with @mcdruid and @Fabianx on Slack and decided, that the best approach will be to include the double click protection in a new separate library, together with a possibility to OPT-OUT of this behavior.

I am uploading a new patch for D7. It is based on patch #130, with these changes (see interdiff):

1. There is a new variable javascript_use_double_submit_protection for possibility to opt-out from double click protection behavior entirely (it is better not to mix it with javascript_always_use_jquery settings, as that has a different purpose).

2. Added a new library named drupal.form-single-submit (in misc/form-single-submit.js), where I have moved the Drupal.behaviors.formSingleSubmit from misc/form.js.

3. The tweak from #130 from Drupal.behaviors.formSingleSubmit is removed, so that we can commit the same code as it is in D10. We can add this later in follow-ups (D10 first).

4. Updated the comment in form_process_button.

5. Added a new test to verify the behavior of form_process_button and new variable (javascript_use_double_submit_protection).

---------------

We need to document this behavior and a new variable javascript_use_double_submit_protection in the change record.

Unfortunatelly D7 does not have JS testing framework, so each feedback/manual testing will be appreaciated, so that we can move this forward :) Thanks!

MustangGB’s picture

Correct me if I'm wrong, as I've not tested the patch.

But it doesn't look like the user is given any indication that this is happening.

I expected to see something along these lines:

$('.form-actions input', $form).attr('disabled', true);

poker10’s picture

Yes, that is true, there is no visual indication about the double click prevention.

The reason is that the situation in D10 is the same (no visual indication) and the javascript part of the D7 patch is practically 1:1 backport of the D10 code. We do not want to introduce additional changes which are not in D10 currently. If there are some ideas like this, or the one from #130, these could be handled in follow-up issues (D10 first).

poker10’s picture

Issue tags: +Pending Drupal 7 commit

Adding a tag for review from other maintainer, as I think this issue should be ready - it was RTBC, I have "just" extracted the code to the separate library and added a new variable to add possibility to opt-out.

poker10’s picture

Lets have a look on this from @Fabianx.

mcdruid’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Pending Drupal 7 commit, -Needs framework manager review +RTBM

The D7 maintainers discussed this in chat and Fabian approved it, so removing FM tag.

Looks like it still needs a CR though.

poker10’s picture

  • mcdruid committed 353a22fc on 7.x
    Issue #1705618 by sun, nod_, mgifford, hanoii, clemens.tolboom, poker10...
mcdruid’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs backport to D7, -RTBM

Thank you everybody that contributed.

I'm afraid assigning credit is a bit of a best effort on an issue as long, old, and meandering as this one. Apologies if I've missed anything.

  • mcdruid committed e523ff5a on 7.x
    Issue #1705618: new js file missing from previous commit
    

Status: Fixed » Closed (fixed)

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

jlopes23’s picture

Is there any way to disable this feature just in specific pages?

I want to have the feature enable in the full site apart of a specific page.

thanks

MustangGB’s picture

@pjflopes

This is not really the best place to ask.

But you can either disable it globally with $conf['javascript_use_double_submit_protection'] = FALSE;

Or presumably on a per form basis with a hook_form_alter() and removing form_process_button from the relevant submit button #process arrays.