Update disable-on-submit styling to use active styling by aduth · Pull Request #6564 · 18F/identity-idp

aduth · 2022-07-08T14:58:57Z

Why:

For consistency with spinner button activation, which shows with dark blue background while awaiting completion
To reduce the abruptness of transitioning from an active to disabled button on submission

Screen recording:

Before:

disable-before.mov

After:

disable-after.mov

Background:

Previously, this pull request was aimed at removing the disable-on-submit effect altogether:

Click to see previous summary

Why:

Improve consistency: Currently, this behavior happens in some forms and not others. This makes it so that a button is never disabled on form submission.
It's unnecessary: Our controllers should be tolerant to handling repeated submissions, and if they aren't, that should be addressed at the controller level, and not by relying on some fragile/unpredictable client-side behavior.
Contributing toward an eventual removal of the form-validation JavaScript pack
Improve performance: The form-validation pack is in the critical path for JavaScript (sign-in page), and reduction in its code size should contribute to smaller bundle sizes and faster page loads

zachmargolis · 2022-07-08T15:51:34Z

It's unnecessary: Our controllers should be tolerant to handling repeated submissions, and if they aren't, that should be addressed at the controller level, and not by relying on some fragile/unpredictable client-side behavior.

So in spirit I agree with this... we should be able to handle double submissions, but we have had errors in the past, which is why we added this... here's an old ticket that's not super specific... but just to highlight that we don't have good visibility into which forms tend to get double submitted right now, but we've had errors times in LG history attributed to this

aduth · 2022-07-08T17:00:34Z

So in spirit I agree with this... we should be able to handle double submissions, but we have had errors in the past, which is why we added this... here's an old ticket that's not super specific... but just to highlight that we don't have good visibility into which forms tend to get double submitted right now, but we've had errors times in LG history attributed to this

Thanks for the extra context. As you mention, it's kinda hard to tell what forms may still be problematic.

Maybe we pivot the other direction then, and make sure we're consistent with all submission buttons being disabled?

I recall chatting about this at some point in the past with @anniehirshman-gsa , not sure how strongly you had felt about that Annie. I don't especially love the disable-on-submit behavior, though I'm more concerned with consistency above all else.

Coincidentally, I had been contemplating the addition of a new SubmitButton component to standardize our default submit button styling (big, wide, block-display with 2.5rem vertical margins), which could be a good entrypoint to attach the disable-on-submit behavior independent from the form-validation pack.

jmhooper · 2022-07-11T14:17:02Z

I'm with Zach here. Some of the bugs we had with double submitting in the past were very tricky. I don't like the disable functionality either, but I do think aiming for consistency is probably the safer option

aduth · 2022-07-26T15:30:45Z

Update: I chatted with @anniehirshman-gsa about this and we may want to standardize on the "always disable" approach, but also want to explore using the SpinnerButton / design system "Active" styling instead of the gray disabling for the pending submission. I will plan to make those updates here, but this work is lower priority, so I won't be able to get to it immediately.

**Why**: - Improve consistency: Currently, this behavior happens in some forms and not others. This makes it so that a button is never disabled on form submission. - It's unnecessary: Our controllers should be tolerant to handling repeated submissions, and if they aren't, that should be addressed at the controller level, and not by relying on some fragile/unpredictable client-side behavior. - Contributing toward an eventual removal of the `form-validation` JavaScript pack changelog: Improvements, Forms, Improve consistency of form submission by leaving buttons enabled after submission

This reverts commit 0d3193a.

Only use when explicitly applied, since otherwise if the user clicks on a grayed, disabled button, they'd get the blue background only while mouse is held down

aduth · 2022-08-08T12:49:07Z

Circling back to this, I spent some time on Friday afternoon to get this updated with the proposal to use the active styling on submit. I think the new effect is a good compromise? I also checked with @anniehirshman-gsa and have added to the biweekly design system sync (at least to get some buy-in to the general approach for disable-on-submit).

I marked the pull request as ready to review, and updated the original title and description with new details and before/after screen recordings.

zachmargolis

LGTM

zachmargolis · 2022-08-08T15:32:20Z

-  /** @type {HTMLInputElement[]} */
-  const fields = Array.from(form.querySelectorAll('.field,[required]'));
+export function initialize(form: HTMLFormElement) {
+  const fields: HTMLInputElement[] = Array.from(form.querySelectorAll('.field,[required]'));


another idea about a space

Suggested change

const fields: HTMLInputElement[] = Array.from(form.querySelectorAll('.field,[required]'));

const fields: HTMLInputElement[] = Array.from(form.querySelectorAll('.field, [required]'));

Updated in f15bb40.

zachmargolis · 2022-08-08T15:32:55Z

+
+.usa-button:disabled.usa-button--active,
+.usa-button--disabled.usa-button--active {
+  @include set-text-and-bg('primary-darker', $context: 'Button');


👀 I have never seen kwards in SCSS like this, what a time to be alive

👍 For context, the general approach here is just to repeat the same styles as the default active styles from USWDS, but with higher specificity to make sure the active styles override the disabled styles.

https: //github.com//pull/6564#discussion_r940379678 Co-Authored-By: Zach Margolis <zbmargolis@gmail.com>

**Why**: Because the styling was only implemented with the expectation of use with the default button, and conflicts with other button states (e.g. unstyled buttons used as the submit buttons on some forms). Previously: #6564 changelog: Improvements, Forms, Use pressed state appearance for buttons disabled on form submission

* Remove upstream-patched disabled button styling See: uswds/uswds#4077 * Limit active disabled button styles to default button **Why**: Because the styling was only implemented with the expectation of use with the default button, and conflicts with other button states (e.g. unstyled buttons used as the submit buttons on some forms). Previously: #6564 changelog: Improvements, Forms, Use pressed state appearance for buttons disabled on form submission

**Why**: Because the spinner button retains its original shape by setting the text color to transparent, which was being overridden by the new active styles introduced in #6564. changelog: Bug Fixes, Buttons, Fix spinner button active state appearance

aduth force-pushed the aduth-rm-disable-on-submit branch from 5e2f484 to cda74cc Compare July 8, 2022 15:21

aduth marked this pull request as draft July 8, 2022 17:00

aduth added 3 commits August 5, 2022 14:35

Revert "Remove form disable-on-submit behavior"

baa3435

This reverts commit 0d3193a.

Show form-submitted disabled buttons as active styling

fde066b

aduth force-pushed the aduth-rm-disable-on-submit branch from cda74cc to fde066b Compare August 5, 2022 18:41

aduth added 3 commits August 8, 2022 08:31

Update specs for active styling

bf1a82c

Convert form-validation to TypeScript

f8514d3

Avoid applying active disabled effect to native active

6f3478b

Only use when explicitly applied, since otherwise if the user clicks on a grayed, disabled button, they'd get the blue background only while mouse is held down

aduth changed the title ~~Remove form disable-on-submit behavior~~ Update disable-on-submit styling to use active styling Aug 8, 2022

aduth marked this pull request as ready for review August 8, 2022 12:46

zachmargolis approved these changes Aug 8, 2022

View reviewed changes

Improve clarity of individual selector parts

f15bb40

https: //github.com//pull/6564#discussion_r940379678 Co-Authored-By: Zach Margolis <zbmargolis@gmail.com>

aduth merged commit d80f2cc into main Aug 10, 2022

aduth deleted the aduth-rm-disable-on-submit branch August 10, 2022 12:13

aduth mentioned this pull request Aug 15, 2022

Limit active disabled button styles to default button #6744

Merged

aduth mentioned this pull request Aug 17, 2022

Use self-contained component for SimpleForm submit buttons #6771

Merged

aduth mentioned this pull request Aug 17, 2022

Fix spinner button active color #6774

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update disable-on-submit styling to use active styling#6564

Update disable-on-submit styling to use active styling#6564
aduth merged 7 commits into
mainfrom
aduth-rm-disable-on-submit

aduth commented Jul 8, 2022 •

edited

Loading

Uh oh!

zachmargolis commented Jul 8, 2022

Uh oh!

aduth commented Jul 8, 2022

Uh oh!

jmhooper commented Jul 11, 2022

Uh oh!

aduth commented Jul 26, 2022 •

edited

Loading

Uh oh!

aduth commented Aug 8, 2022 •

edited

Loading

Uh oh!

zachmargolis left a comment

Uh oh!

Uh oh!

zachmargolis Aug 8, 2022

Uh oh!

aduth Aug 9, 2022

Uh oh!

zachmargolis Aug 8, 2022

Uh oh!

aduth Aug 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	const fields: HTMLInputElement[] = Array.from(form.querySelectorAll('.field,[required]'));
	const fields: HTMLInputElement[] = Array.from(form.querySelectorAll('.field, [required]'));

Uh oh!

Conversation

aduth commented Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zachmargolis commented Jul 8, 2022

Uh oh!

aduth commented Jul 8, 2022

Uh oh!

jmhooper commented Jul 11, 2022

Uh oh!

aduth commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduth commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zachmargolis Aug 8, 2022

Choose a reason for hiding this comment

Uh oh!

aduth Aug 9, 2022

Choose a reason for hiding this comment

Uh oh!

zachmargolis Aug 8, 2022

Choose a reason for hiding this comment

Uh oh!

aduth Aug 8, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aduth commented Jul 8, 2022 •

edited

Loading

aduth commented Jul 26, 2022 •

edited

Loading

aduth commented Aug 8, 2022 •

edited

Loading