Skip to content

Make SpinnerButton form-aware for invalid form submissions #7803

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aduth merged 8 commits into from
Mar 24, 2023
Merged

Make SpinnerButton form-aware for invalid form submissions #7803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0e574a2
Make SpinnerButton form aware for invalid form submissions
aduth Feb 9, 2023
9181027
Update specs
aduth Feb 10, 2023
67f7fa0
Remove now-redundant FormStepsWait invalid handling
aduth Feb 10, 2023
7ae8e7e
Handle form within spinner button
aduth Feb 10, 2023
f1e8b3f
Unbind events from form when disconnected
aduth Feb 10, 2023
64f356d
Removes redundant form validation error testing
aduth Feb 10, 2023
3cce1a2
Unbind events from form when disconnected
aduth Feb 10, 2023
75d4ec5
Refactor tag construct
aduth Mar 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 23 additions & 47 deletions app/javascript/packages/captcha-submit-button/captcha-submit-button-element.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { SinonStub } from 'sinon';
import userEvent from '@testing-library/user-event';
import { screen, waitFor } from '@testing-library/dom';
import { screen, waitFor, fireEvent } from '@testing-library/dom';
import { useSandbox, useDefineProperty } from '@18f/identity-test-helpers';
import '@18f/identity-spinner-button/spinner-button-element';
import { CAPTCHA_EVENT_NAME } from './captcha-submit-button-element';
Expand Down Expand Up @@ -35,58 +35,30 @@ describe('CaptchaSubmitButtonElement', () => {
`;
});

it('submits the form', async () => {
it('does not prevent default form submission', async () => {
const button = screen.getByRole('button', { name: 'Submit' });
const form = document.querySelector('form')!;

sandbox.stub(form, 'submit');
let didSubmit = false;
form.addEventListener('submit', (event) => {
expect(event.defaultPrevented).to.equal(false);
event.preventDefault();
didSubmit = true;
});

await userEvent.click(button);
await waitFor(() => expect((form.submit as SinonStub).called).to.be.true());
await waitFor(() => expect(didSubmit).to.be.true());
});

context('with form validation errors', () => {
beforeEach(() => {
document.body.innerHTML = `
<form>
<input required>
<lg-captcha-submit-button>
<lg-spinner-button>
<button>Submit</button>
</lg-spinner-button>
</lg-captcha-submit-button>
</form>
`;
});

it('does not submit the form and reports validity', async () => {
const button = screen.getByRole('button', { name: 'Submit' });
const form = document.querySelector('form')!;
const input = document.querySelector('input')!;

let didSubmit = false;
form.addEventListener('submit', (event) => {
event.preventDefault();
didSubmit = true;
});

let didReportInvalid = false;
input.addEventListener('invalid', () => {
didReportInvalid = true;
});
it('unbinds form events when disconnected', () => {
const submitButton = document.querySelector('lg-captcha-submit-button')!;
const form = submitButton.form!;
form.removeChild(submitButton);

await userEvent.click(button);
sandbox.spy(submitButton, 'shouldInvokeChallenge');
fireEvent.submit(form);

expect(didSubmit).to.be.false();
expect(didReportInvalid).to.be.true();
});

it('stops or otherwise prevents the spinner button from spinning', async () => {
const button = screen.getByRole('button', { name: 'Submit' });
await userEvent.click(button);

expect(document.querySelector('.spinner-button--spinner-active')).to.not.exist();
});
expect(submitButton.shouldInvokeChallenge).not.to.have.been.called();
});

context('with configured recaptcha', () => {
Expand Down Expand Up @@ -130,7 +102,7 @@ describe('CaptchaSubmitButtonElement', () => {
expect(grecaptcha.execute).to.have.been.calledWith(RECAPTCHA_SITE_KEY, {
action: RECAPTCHA_ACTION_NAME,
});
expect(Object.fromEntries(new FormData(form))).to.deep.equal({
expect(Object.fromEntries(new window.FormData(form))).to.deep.equal({
recaptcha_token: RECAPTCHA_TOKEN_VALUE,
});
});
Expand All @@ -145,10 +117,14 @@ describe('CaptchaSubmitButtonElement', () => {
const button = screen.getByRole('button', { name: 'Submit' });
const form = document.querySelector('form')!;

sandbox.stub(form, 'submit');
let didSubmit = false;
form.addEventListener('submit', (event) => {
event.preventDefault();
didSubmit = true;
});

await userEvent.click(button);
await waitFor(() => expect((form.submit as SinonStub).called).to.be.true());
await waitFor(() => expect(didSubmit).to.be.true());

expect(grecaptcha.ready).not.to.have.been.called();
});
Expand Down
29 changes: 12 additions & 17 deletions app/javascript/packages/captcha-submit-button/captcha-submit-button-element.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,16 @@
export const CAPTCHA_EVENT_NAME = 'lg:captcha-submit-button:challenge';

class CaptchaSubmitButtonElement extends HTMLElement {
form: HTMLFormElement | null;

connectedCallback() {
this.button.addEventListener('click', (event) => this.handleButtonClick(event));
this.form = this.closest('form');

this.form?.addEventListener('submit', this.handleFormSubmit);
Copy link
Contributor Author

@aduth aduth Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This dependency on an external element when the node is connected is a bit clunky to deal with, but unfortunately the form's submit event is the most reliable way to know that all of the client-side validation has passed, which is critical both to know when captcha test should be run, as well as when the spinner button's animation should start.

Copy link
Contributor Author

@aduth aduth Mar 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an edge-case, but I was curious the specific behavior of custom elements when being moved around the page while already live / attached. It looks like they will call disconnectedCallback and then connectedCallback in its new position, which will behave how we want it to.

https://codepen.io/aduth/pen/rNZodWj?editors=1111

}

disconnectedCallback() {
this.form?.removeEventListener('submit', this.handleFormSubmit);
}

get button(): HTMLButtonElement {
Expand All @@ -13,10 +21,6 @@ class CaptchaSubmitButtonElement extends HTMLElement {
return this.querySelector('[type=hidden]')!;
}

get form(): HTMLFormElement | null {
return this.closest('form');
}

get recaptchaSiteKey(): string | null {
return this.getAttribute('recaptcha-site-key');
}
Expand Down Expand Up @@ -48,21 +52,12 @@ class CaptchaSubmitButtonElement extends HTMLElement {
return !event.defaultPrevented;
}

handleButtonClick(event: MouseEvent) {
event.preventDefault();

if (this.form && !this.form.reportValidity()) {
// Prevent any associated custom click handling, e.g. spinner button spinning
event.stopImmediatePropagation();
return;
}

handleFormSubmit = (event: SubmitEvent) => {
if (this.shouldInvokeChallenge()) {
event.preventDefault();
this.invokeChallenge();
} else {
this.submit();
}
}
};
}

declare global {
Expand Down
102 changes: 82 additions & 20 deletions app/javascript/packages/spinner-button/spinner-button-element.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { getByRole, fireEvent, screen } from '@testing-library/dom';
import type { SinonStub } from 'sinon';
import { useSandbox } from '@18f/identity-test-helpers';
import './spinner-button-element';
import type { SpinnerButtonElement } from './spinner-button-element';

describe('SpinnerButtonElement', () => {
const sandbox = useSandbox({ useFakeTimers: true });
Expand All @@ -14,20 +13,37 @@ describe('SpinnerButtonElement', () => {

interface WrapperOptions {
actionMessage?: string;

tagName?: string;

spinOnClick?: boolean;
inForm?: boolean;
isButtonTo?: boolean;
}

function createWrapper({ actionMessage, tagName = 'a', spinOnClick }: WrapperOptions = {}) {
document.body.innerHTML = `
function createWrapper({
actionMessage,
tagName = 'a',
spinOnClick,
inForm,
isButtonTo,
}: WrapperOptions = {}) {
let tag;
if (tagName === 'a') {
tag = '<a href="#">Click Me</a>';
} else {
tag = '<input type="submit" value="Click Me">';
}

if (isButtonTo) {
tag = `<form action="#">${tag}</form>`;
}

let html = `
<lg-spinner-button
long-wait-duration-ms="${longWaitDurationMs}"
${spinOnClick === undefined ? '' : `spin-on-click="${spinOnClick}"`}
>
<div class="spinner-button__content">
${tagName === 'a' ? '<a href="#">Click Me</a>' : '<input type="submit" value="Click Me">'}
${tag}
<span class="spinner-dots" aria-hidden="true">
<span class="spinner-dots__dot"></span>
<span class="spinner-dots__dot"></span>
Expand All @@ -44,7 +60,13 @@ describe('SpinnerButtonElement', () => {
}
</lg-spinner-button>`;

return document.body.firstElementChild as SpinnerButtonElement;
if (inForm) {
html = `<form action="#">${html}</form>`;
}

document.body.innerHTML = html;

return document.querySelector('lg-spinner-button')!;
}

it('shows spinner on click', async () => {
Expand All @@ -56,24 +78,64 @@ describe('SpinnerButtonElement', () => {
expect(wrapper.classList.contains('spinner-button--spinner-active')).to.be.true();
});

it('disables button without preventing form handlers', async () => {
const wrapper = createWrapper({ tagName: 'button' });
let submitted = false;
const form = document.createElement('form');
form.action = '#';
form.addEventListener('submit', (event) => {
submitted = true;
event.preventDefault();
context('inside form', () => {
it('disables button without preventing form handlers', async () => {
const wrapper = createWrapper({ tagName: 'button', inForm: true });
let didSubmit = false;
wrapper.form!.addEventListener('submit', (event) => {
didSubmit = true;
event.preventDefault();
});
const button = screen.getByRole('button', { name: 'Click Me' });

await userEvent.type(button, '{Enter}');
clock.tick(0);

expect(didSubmit).to.be.true();
expect(button.hasAttribute('disabled')).to.be.true();
});

it('unbinds events when disconnected', () => {
const wrapper = createWrapper({ tagName: 'button', inForm: true });
const form = wrapper.form!;
form.removeChild(wrapper);

sandbox.spy(wrapper, 'toggleSpinner');
fireEvent.submit(form);

expect(wrapper.toggleSpinner).not.to.have.been.called();
});
});

context('with form inside (button_to)', () => {
it('disables button without preventing form handlers', async () => {
const wrapper = createWrapper({ tagName: 'button', isButtonTo: true });
let didSubmit = false;
wrapper.form!.addEventListener('submit', (event) => {
didSubmit = true;
event.preventDefault();
});
const button = screen.getByRole('button', { name: 'Click Me' });

await userEvent.type(button, '{Enter}');
clock.tick(0);

expect(didSubmit).to.be.true();
expect(button.hasAttribute('disabled')).to.be.true();
});
document.body.appendChild(form);
form.appendChild(wrapper);
});

it('does not show spinner if form is invalid', async () => {
const wrapper = createWrapper({ tagName: 'button', inForm: true });
const form = wrapper.closest('form')!;
const input = document.createElement('input');
input.required = true;
form.appendChild(input);
const button = screen.getByRole('button', { name: 'Click Me' });

await userEvent.type(button, '{Enter}');
clock.tick(0);

expect(submitted).to.be.true();
expect(button.hasAttribute('disabled')).to.be.true();
expect(wrapper.classList.contains('spinner-button--spinner-active')).to.be.false();
});

it('announces action message', async () => {
Expand Down
Loading