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
- Use Chrome
- Navigate to admin/modules
- Scroll down
- Triple click the 'Save configuration' button
- Check the network page chrome provides which has 3 POST request.
- Apply the patch
- The network page does now contain just one POST request
- Use Chrome
- Log out
- Navigate to user/register
- Enter a user name and mail address
- Triple click the submit button
- Depending on your speed you get
- Exception
- Already taken
Proposed resolution
Integrate https://github.com/sun/jquery-form-submit-single
Remaining tasks
- Upgrade code to depend on https://github.com/sun/jquery-form-submit-single
- Backport to D7
- Possibly backport to D6
User interface changes
API changes
Related Issues
#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?
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedApart 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.
Comment #2
franzOne 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.
Comment #3
helmo CreditAttribution: helmo commentedRelated support request: https://drupal.org/node/1089102
Entity Construction Kit (ECK): #2093061: Double click on save button creates duplicates
Comment #4
helmo CreditAttribution: helmo commentedRelated installer issue: #881494: Double-clicking the "Install Drupal" button completely breaks everything
Comment #5
helmo CreditAttribution: helmo commentedWhat about something like: http://stackoverflow.com/questions/2830542/prevent-double-submission-of-...
Comment #6
clemens.tolboomThis 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.
Comment #6.0
clemens.tolboomUpdated issue summary.
Comment #6.1
clemens.tolboomAdded steps to reproduce
Comment #7
clemens.tolboomAdding something like
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.
Comment #7.0
clemens.tolboomUpdated issue summary.
Comment #8
clemens.tolboom[edit]wrong issue[/edit]
Comment #9
nod_tag
Comment #10
nod_I'd rather disable the button clicked so that feedback is given to the user. Also it needs a detach method.
Comment #11
clemens.tolboom@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 :-/
Comment #11.0
clemens.tolboomAdded user/register example
Comment #12
nod_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.
Comment #13
clemens.tolboomWe applied patch to a D7 forum site where we had too much double postings from our users.
Comment #14
clemens.tolboomI failed to make the button text change as the current solution changes form.submit and has no context of what button triggered it.
Comment #15
clemens.tolboomAdded related links into issue fields
Comment #16
helmo CreditAttribution: helmo commentedThe D7 patch from #13 is working great. Let's fix this before even more duplicate issues pop up ;)
Comment #17
sunBriefly 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:
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?
Comment #18
sunOK, 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? :)
Comment #19
nod_Still not finished but my take on it, closer to what happens on ajax submissions.
Comment #21
sunAfter 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? :)
Comment #22
Bojhan CreditAttribution: Bojhan commentedInteresting
Comment #23
clemens.tolboomLooks 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
?
Comment #24
sunI 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?
Comment #25
nod_Works great as far as I can see.
Made a couple changes:
.data()
thing, so that other scripts can see what's happening and more importantly to not require jQuery for this. Planning on aDrupal.form.serialize()
method down the line so we can swap jquery out as well.Simple and straightforward, sun++
Comment #26
nod_Sorry wrong patch up there.
Comment #27
Bojhan CreditAttribution: Bojhan commented@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.
Comment #28
nod_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.
Comment #29
sun@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.
Comment #30
nod_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 whysingle
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.
Comment #31
Bojhan CreditAttribution: Bojhan commentedThanks 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 :)
Comment #32
nod_Rerolled patch to get the event namespaced.
Commenting preventDefault, Firefox still works fine with the attr change.
Comment #33
Wim LeersTested 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.Comment #34
nod_Fine with me, now who'll RTBC this now? :þ
Comment #35
Wim LeersI 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!
Comment #36
sunSorry, I wrote those docs in the middle of the night... just removing some snarky language there ;)
Comment #37
Wim LeersHah :) I personally found it acceptable :)
Re-reviewed. Still RTBC.
Comment #38
tim.plunkettThis 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.
Comment #39
sunThe 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).
Comment #40
tim.plunkett#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.
Comment #41
xjm36: drupal8.form-single-submit.34.patch queued for re-testing.
Comment #42
Dries CreditAttribution: Dries commentedI believe this is a bug fix, not a feature request. Committed to 8.x.
Comment #43
webchickJust 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!
Comment #44
webchickAlso, can/should we backport this to 7.x and 6.x?
Comment #45
Wim LeersYAY! So happy that in the not-too-distant future, I won't be able to break things this way anymore! :)
Thanks, sun!
Comment #46
sunThis 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.
Comment #47
helmo CreditAttribution: helmo commentedGreat work and idea to create a proper library for this. Hope we can quickly get this in.
Comment #48
nod_Comment #49
clemens.tolboomHow should one backport this as on() is jQuery 1.7 ?
I did this
which seems to work but this needs some feedback to make it better.
Some observation made while testing
Comment #50
clemens.tolboomPatch from #49 breaks on admin/structure/features/*/recreate when adding a view.
[edit]On Firefox but not on Chrome[/edit]
Comment #51
fawwad.nirvana CreditAttribution: fawwad.nirvana commentedAfter long discussion i think there is no solution till now. If any body know please share.
Comment #52
clemens.tolboom@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
Comment #53
mgiffordJust tested this in D7. Works great! Would like to see it brought into the next release.
Comment #55
fawwad.nirvana CreditAttribution: fawwad.nirvana commented@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);
});
}
};
Comment #56
clemens.tolboomThere 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.
Comment #57
mgiffordThis 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
Comment #58
mgiffordComment #59
nod_patch needs to use .once() not the submitloop hack.
Comment #60
mgiffordSo, #49 + something like this:
Comment #61
nod_Sorry was on my phone yesterday.
We can't use .on() but we can use .delegate(), so replacing this piece of code by:
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?
Comment #62
nod_Comment #63
mgiffordComment #64
mgiffordThe 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.
Comment #65
clemens.tolboomAny chance for an interdiff with the D8 version?
Comment #66
mgiffordThis 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.
Comment #67
fawwad.nirvana CreditAttribution: fawwad.nirvana commented@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.
Comment #68
mgiffordI thought i'd removed those.. Guess not.
Comment #70
helmo CreditAttribution: helmo commentedJust tried the patch from #68 again on Core 7.29 and still works OK.
Comment #72
helmo CreditAttribution: helmo commentedSounds like a testbot hickup...
Comment #76
dcam CreditAttribution: dcam commentedComment #81
helmo CreditAttribution: helmo commentedtestbot back to normal :)
Comment #84
tim.plunkettIsn't this second line indented too far?
Comment #85
mgiffordYup...
Comment #86
tim.plunkettIt seems like @nod_ was okay with this patch as it was, so back to RTBC.
Comment #87
rakesh.gectcr@mgifford #85
Thanks! the patch working fine for me....
Comment #90
dcam CreditAttribution: dcam commentedComment #93
helmo CreditAttribution: helmo commentedback to rtbc ... agian.
Comment #94
JvE CreditAttribution: JvE commentedThe 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.
Comment #95
helmo CreditAttribution: helmo commented@JvE: could you be more specific? What do you mean by "the error message".
Did you clear your JS cache after applying the patch?
Comment #97
JvE CreditAttribution: JvE commented@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.
Comment #98
David_Rothstein CreditAttribution: David_Rothstein commentedAs 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?
(minor) The blank line at the end should not be there (and it wasn't in the Drupal 8 version).
Comment #99
JvE CreditAttribution: JvE commentedI 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.
Comment #100
clemens.tolboomI somehow expected forum comments would be fixed too with this patch. But this is not the case.
@JvE thanks for pointing out fieldset / vertical tabs. Any idea whether we could build a matrix of forms lacking this?
Comment #101
puddyglumEdit: I created a new issue for a potential server-side fix for this (for authenticated users) #2449847: Prevent duplicate form submissions for authenticated users
Comment #102
GuyPaddock CreditAttribution: GuyPaddock at RedBottle Design, LLC for House at Work commentedAs 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:
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?
Comment #103
clemens.tolboomI 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
Comment #104
clemens.tolboomMake sure to test with Chrome
Comment #105
zterry95 CreditAttribution: zterry95 commentedhow about using lock mechanisms to proceed this issue?
https://api.drupal.org/api/drupal/includes!lock.inc/group/lock/7
Comment #106
GuyPaddock CreditAttribution: GuyPaddock at RedBottle Design, LLC for House at Work commented@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.
Comment #107
johnhanley CreditAttribution: johnhanley commentedWhat's the status of this patch and more specifically the back-port to D7?
Comment #110
hanoiiI 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 persystem_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.
Comment #112
hanoiiOk, 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.
Comment #114
hanoiihmm, not sure why that failed, trying again?
Comment #115
helmo CreditAttribution: helmo at Initfour websolutions for Aegir Cooperative commentedThanks @hanoii, it works ok and it would be nice to get this committed finally.
Comment #116
JvE CreditAttribution: JvE at iO commentedAfter this patch I can no longer use
if I have a search form on my homepage? That doesn't seem right.
Comment #117
hanoii@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 newform_process_button()
. In that function, I am only adding form.js on buttons ifjavascript_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).
Comment #118
hanoiiComment #119
hanoiiBTW, 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.
Comment #120
hanoiiThe interdiff was incomplete.
Comment #121
nithinkolekar CreditAttribution: nithinkolekar commentedthis patch should be must have for all commerce sites having add to cart, pay-now button etc.
Comment #122
chegor CreditAttribution: chegor as a volunteer commentedUsing this patch on several sites. Can we move this into the core?
Comment #123
nithinkolekar CreditAttribution: nithinkolekar commenteduntil this patch get committed , is anyone experienced avoiding double submit using captcha technique? (just alternative solution for time being)
Comment #124
Matt V. CreditAttribution: Matt V. as a volunteer commentedThe 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.
Comment #128
torotil CreditAttribution: torotil at more onion commentedSeems that this was just a CI fluke. Setting to "needs review" as the tests pass now.
Comment #129
torotil CreditAttribution: torotil at more onion commentedThis 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.Comment #130
torotil CreditAttribution: torotil at more onion commentedIt 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.
Comment #131
MichelleWIth #130, if I create a new node and double click on the save I get two nodes created. Shouldn't this prevent that?
Comment #132
torotil CreditAttribution: torotil at more onion commentedThe 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?
Comment #133
MichelleThe 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.
Comment #134
torotil CreditAttribution: torotil at more onion commentedA 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.
Comment #135
CKIDOWHey... 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?
Comment #136
torotil CreditAttribution: torotil at more onion commented@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?
Comment #137
monta CreditAttribution: monta commented#130 patch works fine in 7.56
Comment #138
chrisrockwell CreditAttribution: chrisrockwell commentedNot ready to set back to needs work but I'm experiencing the same as @Michelle in #133, doing some debugging now.
Comment #139
robertwb CreditAttribution: robertwb commentedI 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.
Comment #140
jebeze_alexander CreditAttribution: jebeze_alexander commentedI 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
/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.
Comment #141
mdjamiruddin CreditAttribution: mdjamiruddin at SynapseIndia Outsourcing Pvt. Ltd. commentedI 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.
Comment #142
albertski CreditAttribution: albertski at Xeno Media, Inc. commentedVerified 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).
Comment #143
marc.groth CreditAttribution: marc.groth commentedFurther 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.
Comment #144
MustangGB CreditAttribution: MustangGB commentedComment #145
joseph.olstadBumping to 7.70, this didn't make it into 7.60
Comment #147
jOksanen CreditAttribution: jOksanen commentedI 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);
Comment #148
torotil CreditAttribution: torotil at more onion commentedI’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?
Comment #149
Oliver Huynh CreditAttribution: Oliver Huynh as a volunteer and at Jufist commentedCheck out here:
https://github.com/oliverhuynh/prevent_double_submit
I have no time to make it as a project
Comment #150
joseph.olstadpatch #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.
Comment #151
commonpike CreditAttribution: commonpike as a volunteer commenteddrupal 7.66 is out. is this fix in there ?
Comment #152
mcdruidComment #153
joseph.olstadComment #154
joseph.olstadComment #155
MustangGB CreditAttribution: MustangGB commentedComment #156
chegor CreditAttribution: chegor as a volunteer commentedComment #157
ebodor CreditAttribution: ebodor commentedAny luck getting the patch committed in 7.70? The patch has been out there for three years and fixed for D8 awhile now.
Comment #158
chegor CreditAttribution: chegor as a volunteer commentedComment #160
joseph.olstadretriggered test, appears to be a test runner issue.
Comment #161
LeDucDuBleuet CreditAttribution: LeDucDuBleuet as a volunteer commentedAny chance of getting this patch in the next 7.74?
Comment #162
Ron Collins CreditAttribution: Ron Collins commented+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.
Comment #163
izmeez CreditAttribution: izmeez commentedJust updating issue tag.
Comment #164
chegor CreditAttribution: chegor as a volunteer commentedComment #165
ressa CreditAttribution: ressa at Ardea commentedThis 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.
From #3179845: [meta] Priorities for 2020-12-02 bugfix release of Drupal 7.76 / 7.77
Comment #166
poker10 CreditAttribution: poker10 commentedAny 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.
Comment #167
izmeez CreditAttribution: izmeez commented@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.
Comment #168
mcdruidFirst 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
...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?
Comment #169
mcdruidNoting 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?
Comment #170
joseph.olstad@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:
appears that D9 already takes care of this in various checks, no one has reported an issue
Comment #171
mcdruid@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.
Comment #172
mcdruidFiled 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..
Comment #173
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedI 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:
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 thesubmit()
manually triggered after the asynchronous call will not pass the check anymore, because the attribute will be already added in that time: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):
Comment #174
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedActually @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 mentionedadmin/people/create
and similar).form.js
contains also other features, which can probably? cause issues on existing sites. D8 patch also does not forced theform.js
to all pages with forms, it was included only on pages whereform.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 thejavascript_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?.
Comment #175
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedWe 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 withjavascript_always_use_jquery
settings, as that has a different purpose).2. Added a new library named
drupal.form-single-submit
(inmisc/form-single-submit.js
), where I have moved theDrupal.behaviors.formSingleSubmit
frommisc/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!
Comment #176
MustangGB CreditAttribution: MustangGB commentedCorrect 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);
Comment #177
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedYes, 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).
Comment #178
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedAdding 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.
Comment #179
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedLets have a look on this from @Fabianx.
Comment #180
mcdruidThe D7 maintainers discussed this in chat and Fabian approved it, so removing FM tag.
Looks like it still needs a CR though.
Comment #181
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedI have created a draft CR here: https://www.drupal.org/node/3365142
Uploading a patch with the addition of a new variable to the
default.settings.php
. Other than that there are only minor tweaks to the comments.Comment #183
mcdruidThank 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.
Comment #186
jlopes23 CreditAttribution: jlopes23 as a volunteer commentedIs 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
Comment #187
MustangGB CreditAttribution: MustangGB commented@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 removingform_process_button
from the relevant submit button#process
arrays.