Prevent multiple form submissions #1018

quis · 2018-10-02T15:21:53Z

Although the problem described here is specific to Notify it feels like the solution could be more generally applicable. The code here is a rough first go at adapting it to GOV.UK Frontend. I might not have put it in the right place. It doesn’t have tests. I haven’t even checked that it works.

In Notify we’ve seen users complaining that they got sent an invitation email twice. This is probably because someone on their team clicked the ‘send invite’ button twice (even though they think they only clicked it once). Some users will just double click everything on a web page because Windows.

Double form submission is a common issue on web pages, and there are a number of different ways to prevent it. I chose to do it this way because:

temporarily, not permanently disabling the button means that this addresses the double clicking issue without breaking things if the user did, really want to click the button again deliberately (eg an AJAX request times out and nothing happens)
doing it with a data attribute, rather than the disabled attribute means that the interaction behaviour of the button doesn’t change (disabled buttons can’t be focused, for example)

Adapted from https://github.com/alphagov/notifications-admin/blob/93e7f9213533790ffb52a8f3afadbace9145eea2/app/assets/javascripts/preventDuplicateFormSubmissions.js

kr8n3r · 2018-10-03T08:22:23Z

Hi @quis thanks for taking the time and contributed this.
As you've identified it yourself this pull request is not ready to be properly reviewed but it has some interesting points.

One thing that springs to mind is whether we should only trigger debounce on input type='submit' or button and not a tags?

NickColley · 2018-10-03T09:42:49Z

I love this idea but I'm worried about the impact it could have.

GOV.UK Frontend is intended to work in any service and we cannot be sure that buttons will always be used in a traditional form context, for example someone may build a number counter component that relies on being able to press 'increase' quickly.

It's also hard to know if this would result in any negative results since if this triggers in the wrong circumstance users are likely to blame themselves.

With this in mind, do you think it's possible to get what you have learned here and turn it into guidance?

I wonder if this is something that could be handled server side? Given that the server could know how quickly requests are coming in.

Failing this we could consider making this an opt in feature for buttons?

What do you think? Do you think I'm being overly cautious?

quis · 2018-10-03T10:49:47Z

In the original implementation this was scoped to only elements with type="submit" inside a <form>.

Might be sensible to replicate that here because it would prevent this behaviour being applied to:

links, which should submit idempotent GET requests
buttons which don’t submit forms but trigger Javascript events (like @NickColley’s hypothetical number counter)

Thoughts?

NickColley · 2018-10-09T14:57:04Z

It does seem reasonable to only do this when a button is using type=submit, we should bare in mind that if a user does not supply a type to their button submit is the implicit default.

In terms of detecting if it is in a form I would be careful not to check any process that walks up the DOM is not slow for performance, it might be that that it is not necessary to check if the button is inside a form?

Interested to hear what others think.

As a side note if we go with this I believe we can do it in memory rather than setting attributes in the DOM.

Edit: I had a go trying something out:

var DEBOUNCE_TIMEOUT_IN_SECONDS = 0.5
var debounceFormSubmitTimer = null
  
button.addEventListener('click', function(event) {
  var isInsideAForm = event.target.form
  if (!isInsideAForm) {
    return;
  }
  if (debounceFormSubmitTimer) {
    event.preventDefault()
    return false;
  }
  debounceFormSubmitTimer = setTimeout(function () {
    debounceFormSubmitTimer = null
  }, DEBOUNCE_TIMEOUT_IN_SECONDS * 1000)
});

quis · 2018-10-19T09:42:32Z

@NickColley I think this:

var isInsideAForm = event.target.form

should be something more like:

var isSubmittingAForm = event.target.form && event.target.type === 'submit'

(I don’t know if that’s the right property names, but you get the idea)

Otherwise I think this is a solid implementation.

NickColley · 2018-10-19T10:31:31Z

Good shout

I've made an example page to test this: https://output.jsbin.com/deriwub

psuedo code for demonstration:

var DEBOUNCE_TIMEOUT_IN_SECONDS = 0.5
var debounceFormSubmitTimer = null

var buttons = document.querySelectorAll('button, input')
buttons.forEach(button => {
  button.addEventListener('click', function(event) {
    var $button = event.target
    var isSubmitButton = $button.type === 'submit'
    var isInsideAForm = $button.form

    if (!isSubmitButton || !isInsideAForm) {
      return;
    }
 
    if (debounceFormSubmitTimer) {
      event.preventDefault()
      return false;
    }

    debounceFormSubmitTimer = setTimeout(function () {
      debounceFormSubmitTimer = null
    }, DEBOUNCE_TIMEOUT_IN_SECONDS * 1000)
  }
});

Edge 17 on Windows 10, IE11 on Windows 10, IE8-10 on Windows 7 (✅)

I'm testing through browserstack but I wonder if windows users in IE and Edge already are protected against double clicking?

Before

After

Firefox 62.0.2 (64-bit) on OSX 10.13.3 (✅)

Before

After

Google Chrome Version 69.0.3497.100 on OSX 10.13.3 (✅)

Before

After

Safari Version 11.0.3 on OSX 10.13.3 (✅)

Before

After

NickColley · 2018-10-19T11:43:55Z

Another thought, it feels like there's a few needs here:

If I accidentally double click since I'm used to that, don't submit the form
If the form takes long and I get impatient, dont submit the form if it's already in progress

I think this only covers the first case @quis maybe some sort of loading pattern would be necessary to satisfy both? Have you seen your complaints go down after you implemented that fix?

joelanman · 2018-10-19T12:46:44Z

Might this ever prevent valid clicks? So if I click but my connection has gone wrong, and then my connection is fixed, I sometimes find I need to click again, this would prevent that?

NickColley · 2018-10-19T13:45:40Z

@joelanman at the moment the implementation only prevents clicks that happen immediately afterwards, in my example there's a 500millisecond window, Chris' example is a 1.5 second window

joelanman · 2018-10-19T14:56:21Z

ah yes sorry, this PR is just about double clicks, not any click after the first one

quis · 2018-10-23T13:01:48Z

Regarding the duration of the timeout, basing it on how quickly the user expects the UI to respond seems sensible. In other words it shouldn’t interfere with someone who re-clicks because they think that the system hasn’t registered their first click. The generally accepted figure seems to be 1 second¹.

https://www.nngroup.com/articles/response-times-3-important-limits/

NickColley · 2018-10-24T09:10:47Z

@quis so do you think the timeout should be 1.5s as suggested to cover that case?

quis · 2018-10-24T09:13:34Z

@NickColley I think 1s. Not sure what I based the original 1.5s number on.

NickColley · 2018-10-25T16:28:22Z

I've been busy recently but I have pushed the work I have done, I think the only thing left to do is write tests for this, so will pick this up when I get some time.

penx · 2018-12-14T11:43:11Z

I don’t disagree with this as a technique but would question whether it should be done by default.

There may be edge cases where this functionality obstructs functionality - eg clicking a list of delete buttons in quick succession or a medium style “clap” button.

Perhaps this could only be applied to buttons that explicitly request it, eg with a data-prevent-doubleclick attribute or a js-prevent-doubleclick class

NickColley · 2019-01-09T13:17:59Z

There may be edge cases where this functionality obstructs functionality - eg clicking a list of delete buttons in quick succession or a medium style “clap” button.

In these cases we'd expect users to be using <button type="button"></button>.

The idea of having this as an explicit opt-in is interesting, I'll ask the team what they think, thanks. 👍

36degrees · 2019-01-17T16:11:14Z

src/components/button/button.test.js

+        await page.evaluate(() => {
+          document.querySelector('button').outerHTML = `
+            <form>
+              <button type="button">Save and continue</button>


It'd be great to have a nicer way of doing this, but in the short term this does the job nicely 👍

I wonder if we should wrap the existing button from the page rather than replace it though – there's a chance we could change the markup and break this functionality, but it wouldn't be picked up by the test because it'd be hardcoded to use the old markup.

Yes, I did not want to spend hours having the isolated tests. I think we should solve that as part of the technical debt we've got recorded...

I'll try and wrap the existing button, that sounds fair enough.

36degrees · 2019-01-17T16:12:45Z

CHANGELOG.md

@@ -20,6 +20,10 @@

  ([PR #N](https://github.com/alphagov/govuk-frontend/pull/N))

+- Prevent multiple form submissions


I think we might want to be more specific here – we're really debouncing form submission so that you can't submit more than once a second, which isn't quite the same thing this implies.

src/components/button/button.js

@@ -11,6 +11,8 @@
 import '../../vendor/polyfills/Event' // addEventListener and event.target normaliziation

 var KEY_SPACE = 32
+var DEBOUNCE_TIMEOUT_IN_SECONDS = 1
+var debounceFormSubmitTimer = null


36degrees · 2019-01-17T16:14:07Z

src/components/button/button.js

@@ -33,12 +35,47 @@ Button.prototype.handleKeyDown = function (event) {
  }
 }

+/**
+* Add event handler for click


Slightly pedantic, but this doesn't add the event handler – it is the event handler.

36degrees · 2019-01-17T16:15:05Z

src/components/button/button.test.js

+
+        const href = await page.evaluate(() => document.body.getElementsByTagName('a')[0].getAttribute('href'))
+
+        // we need to start the waitForNavigation() before the keyboard action


keyboard action?

recycled code 😬

36degrees · 2019-01-17T16:16:52Z

src/components/button/button.test.js

+            </form>
+          `
+
+          window.__BUTTON_PRESSED_COUNT = 0


To make the intent super clear, I'd consider naming this something like __BUTTON_CLICK_EVENTS as we're counting the number of events fired, not the number of times the button was actually pressed?

36degrees · 2019-01-17T16:20:06Z

src/components/button/button.test.js

+
+        expect(submitCount).toBe(1)
+      })
+      it('when a user clicks again intentionally it is not prevented', async () => {


Again to make the intent really clear here (what does intentionally mean?) perhaps something like '(it) does not prevent additional clicks after one second'?

I tried to capture something closer to a user need than a technical implementation, since we care that pressing the submit twice after a period of time is what we want to allow.

I'll make this clearer

36degrees · 2019-01-17T16:20:28Z

src/components/button/button.test.js

@@ -41,5 +41,102 @@ describe('/components/button', () => {
      const url = await page.url()
      expect(url).toBe(baseUrl + href)
    })
+    describe('debouncing', () => {
+      it('does not block button links', async () => {


These tests are great 👍

src/components/button/button.js

@@ -11,6 +11,8 @@
 import '../../vendor/polyfills/Event' // addEventListener and event.target normaliziation

 var KEY_SPACE = 32
+var DEBOUNCE_TIMEOUT_IN_SECONDS = 1
+var debounceFormSubmitTimer = null


src/components/button/button.js

@@ -11,6 +11,8 @@
 import '../../vendor/polyfills/Event' // addEventListener and event.target normaliziation

 var KEY_SPACE = 32
+var DEBOUNCE_TIMEOUT_IN_SECONDS = 1
+var debounceFormSubmitTimer = null


edwardhorsford · 2019-01-17T16:39:41Z

👋 I really like the spirit of this - I've seen quite a few services where the continue button goes disabled when you click it - I presume because the team / framework is disabling the button on submission. Besides the accessibility issues of disabled buttons I generally find it distracting.

With that said, I do wonder if this should be the concern of the server to deal with. As others have said, there may be cases where you do want to send data quickly, or connections might time out. Only the server knows what's actually been received - I'd hope there are frontend architecture patterns for recognising multiple submissions in a reliable way.

The server solution presumably being harder - if we can provide built-in solutions so we avoid teams having disabled buttons - all the better.

NickColley · 2019-01-17T16:42:00Z

@edwardhorsford one alternative is we turn this into guidance with a recommendation to do this server side for sure, I'm personally a little uncomfortable with how broad this JavaScript could apply based on @36degrees and @penx 's feedback.

36degrees

Generally looking good, but a few minor things I think might need tidying up.

36degrees · 2019-02-08T14:28:31Z

src/components/button/button.js

+Button.prototype.debounce = function (event) {
+  var target = event.target
+  // Check the button that is clicked on has the preventDoubleClick feature enabled
+  if (target.attributes && target.getAttribute('data-prevent-double-click') !== 'true') {


If the target doesn't have any attributes (not sure how likely that is), this won't evaluate the second statement. I think it needs to be flipped:

Suggested change

if (target.attributes && target.getAttribute('data-prevent-double-click') !== 'true') {

if (!target.attributes || target.getAttribute('data-prevent-double-click') !== 'true') {

Actually, I think getAttribute is generally safe to use, and will just return null if the attribute isn't set, so not sure the first check is necessary? In which case this could just be

Suggested change

if (target.attributes && target.getAttribute('data-prevent-double-click') !== 'true') {

if (target.getAttribute('data-prevent-double-click') !== 'true') {

I've been guarding them since if you try to use 'getAttribute' on document it will throw.

Not sure this makes sense here though.

36degrees · 2019-02-08T14:29:18Z

src/components/button/button.js

+
+  var $button = event.target
+
+  // We only want to to handle submit buttons that are used in forms


If this is now an opt-in behaviour, do we need to be as strict here?

If users add data-prevent-double-click, I think they'd expect it to work, regardless of whether it's in a form / a submit button?

Yeah could probably simplify this now...

36degrees · 2019-02-08T14:37:13Z

src/components/button/button.test.js

+        // Links should take a user to another part of the page or url, so debouncing it wouldnt have any impact.
+        // But we can check that we're not blocking any initial clicks.
+
+        await page.goto(baseUrl + '/components/button/link/preview', { waitUntil: 'load' })


The way the logic is currently written, this test is never going to reach the part of the function that tests that the element is an input / button, because it'll already have returned because it doesn't have data-prevent-double-click.

What are we trying to test here? Do we need to be testing clicking a link that has data-prevent-double-click set to true? Or is this test now redundant?

36degrees · 2019-02-08T14:37:43Z

src/components/button/button.test.js

+        const url = await page.url()
+        expect(url).toBe(baseUrl + href)
+      })
+      it('does not trigger on buttons with type=button', async () => {


Same thing applies here – this is just going to be returning because of the lack of data-prevent-double-click

36degrees · 2019-02-08T14:38:41Z

src/components/button/button.yaml

+- name: preventDoubleClick
+  type: boolean
+  required: false
+  description: Prevent accidental double clicks from submitting forms multiple times


Should we be clear that this doesn't affect links?

Have seen users complaining that they got an invitation email twice. This is probably because they clicked the ‘send invite’ button twice even though they think they only clicked it once. Double form submission is a common issue on web pages, and there are a number of different ways to prevent it. I’ve chosen to do it this way because: - temporarily, not permanently disabling the button means that this addresses the double clicking issue without breaking things if the user did, really want to click the button again deliberately (for whatever reason) - doing it with a `data` attribute, rather than the `disabled` attribute means that the interaction behaviour of the button doesn’t change ( `disabled` buttons can’t be focused, for example) Adapted from https://github.com/alphagov/notifications-admin/blob/93e7f9213533790ffb52a8f3afadbace9145eea2/app/assets/javascripts/preventDuplicateFormSubmissions.js

src/components/button/button.test.js

+
+        expect(submitCount).toBe(1)
+      })
+      it('does not prevent intentional multiple clicks', async () => {


adamsilver · 2019-02-15T14:26:07Z

Will this also stop users from submitting the form twice by pressing enter within a form field?

NickColley · 2019-02-15T14:33:50Z

@adamsilver no it does not, potentially the same users that have motor impairments (for example tremors) that cause them to double click a button would be helped by that though. Something to think about in the future.

hannalaakso

I think this is good to go. Really cool tests 🙌 I like the idea of the alternative PR by @NickColley that allows either delegating event listeners to document or binding them to button element. But that that logic feels a bit hidden from API point of view and could be tricky to document for various scenarios. I think releasing this as an opt in feature will allow us to gather more research about whether we need alternatives to globally listening for events.

We’ve also now got some guidance which amongst other things addresses checking for accidental clicks on server side.

NickColley · 2019-03-01T15:20:22Z

Thanks everyone that contributed to this, took us a long time to get it right but I'm happy with what we've managed here.

Special thanks to @quis for the original contribution.

We will release this with some new guidance next week.

Take advantage of new protection against accidental duplicate submissions of the journey form. ref: alphagov/govuk-frontend#1018 Fixes #42 Signed-off-by: James Gauld <james.gauld@engineering.digital.dwp.gov.uk>

GOV.UK Frontend has a double click on buttons protector built in so favouring that. Plus moved inclusion of the GOV.UK Frontend scripts into our compiled JS rather than including as extra HTTP request. Details of how it works here - alphagov/govuk-frontend#1018

quis force-pushed the debounce-buttons branch from f371e53 to c2db34e Compare October 2, 2018 15:22

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1018 November 29, 2018 10:16 Inactive

NickColley self-assigned this Jan 17, 2019

NickColley force-pushed the debounce-buttons branch 5 times, most recently from 832f91e to 874f21d Compare January 17, 2019 15:11

36degrees reviewed Jan 17, 2019

View reviewed changes

penx reviewed Jan 17, 2019

View reviewed changes

NickColley force-pushed the debounce-buttons branch from 874f21d to c208f01 Compare January 17, 2019 16:35

NickColley force-pushed the debounce-buttons branch 4 times, most recently from c6f42ea to ed182ab Compare February 8, 2019 14:02

36degrees reviewed Feb 8, 2019

View reviewed changes

NickColley removed the request for review from timpaul February 11, 2019 10:53

NickColley force-pushed the debounce-buttons branch from 7473dc5 to a63befa Compare February 11, 2019 13:05

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1018 February 12, 2019 14:43 Inactive

36degrees reviewed Feb 13, 2019

View reviewed changes

src/components/button/button.test.js

expect(submitCount).toBe(1)

})

it('does not prevent intentional multiple clicks', async () => {

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

NickColley added 2 commits February 14, 2019 09:49

Add tests for button preventDoubleClick behaviour

eee4429

Update CHANGELOG

887cb1e

NickColley force-pushed the debounce-buttons branch from a63befa to 887cb1e Compare February 14, 2019 17:42

govuk-design-system-ci deployed to govuk-frontend-review-pr-1018 February 14, 2019 17:42 View deployment

amyhupe mentioned this pull request Feb 18, 2019

Write guidance for multiple form submission feature alphagov/govuk-design-system#797

Closed

4 tasks

hannalaakso approved these changes Feb 25, 2019

View reviewed changes

NickColley added this to the 2.8.0 milestone Feb 25, 2019

NickColley merged commit 4e423aa into alphagov:master Mar 1, 2019

36degrees mentioned this pull request Mar 5, 2019

Release v2.8.0 🚀 #1237

Merged

aliuk2012 mentioned this pull request Mar 29, 2019

Remove toolkit button sass alphagov/finder-frontend#1016

Merged

NickColley mentioned this pull request May 22, 2019

Set debounceFormSubmitTimer at instance level instead of global #1370

Merged

jonheslop mentioned this pull request Oct 28, 2019

PP-5783 - Move to new GOV.UK Frontend double click protection alphagov/pay-frontend#1126

Merged

		@@ -20,6 +20,10 @@

		([PR #N](https://github.com/alphagov/govuk-frontend/pull/N))

		- Prevent multiple form submissions


		const href = await page.evaluate(() => document.body.getElementsByTagName('a')[0].getAttribute('href'))

		// we need to start the waitForNavigation() before the keyboard action

	if (target.attributes && target.getAttribute('data-prevent-double-click') !== 'true') {
	if (!target.attributes \|\| target.getAttribute('data-prevent-double-click') !== 'true') {

	if (target.attributes && target.getAttribute('data-prevent-double-click') !== 'true') {
	if (target.getAttribute('data-prevent-double-click') !== 'true') {


		var $button = event.target

		// We only want to to handle submit buttons that are used in forms

Prevent multiple form submissions #1018

Prevent multiple form submissions #1018

Conversation

quis commented Oct 2, 2018

kr8n3r commented Oct 3, 2018

NickColley commented Oct 3, 2018

quis commented Oct 3, 2018

NickColley commented Oct 9, 2018 • edited Loading

quis commented Oct 19, 2018

NickColley commented Oct 19, 2018 • edited Loading

Edge 17 on Windows 10, IE11 on Windows 10, IE8-10 on Windows 7 (✅)

Before

After

Firefox 62.0.2 (64-bit) on OSX 10.13.3 (✅)

Before

After

Google Chrome Version 69.0.3497.100 on OSX 10.13.3 (✅)

Before

After

Safari Version 11.0.3 on OSX 10.13.3 (✅)

Before

After

NickColley commented Oct 19, 2018

joelanman commented Oct 19, 2018

NickColley commented Oct 19, 2018

joelanman commented Oct 19, 2018

quis commented Oct 23, 2018

NickColley commented Oct 24, 2018

quis commented Oct 24, 2018

NickColley commented Oct 25, 2018

penx commented Dec 14, 2018

NickColley commented Jan 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

edwardhorsford commented Jan 17, 2019

NickColley commented Jan 17, 2019

36degrees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

adamsilver commented Feb 15, 2019

NickColley commented Feb 15, 2019 • edited Loading

hannalaakso left a comment • edited Loading

Choose a reason for hiding this comment

NickColley commented Mar 1, 2019 • edited Loading

NickColley commented Oct 9, 2018 •

edited

Loading

NickColley commented Oct 19, 2018 •

edited

Loading

NickColley commented Feb 15, 2019 •

edited

Loading

hannalaakso left a comment •

edited

Loading

NickColley commented Mar 1, 2019 •

edited

Loading