Add `Input.error` prop, `useValidateOnSubmit` hook #1534

robertknight · 2024-04-26T12:23:14Z

This PR adds a couple of APIs designed to make handling custom validation errors and custom presentation of validation errors easier:

The new Input.error prop specifies an error message that is synced to the underlying <input> DOM element using HTMLInputElement.setCustomValidity. This in turn will prevent submission of any containing form which has validation checks enabled (the default).
For forms which want to control presentation of validation error messages, the new useValidateOnSubmit hook can be used together with a form's noValidate property. This hook implements the parts of native validation that are still useful when validation errors are presented in a custom way: checking the validation status using HTMLFormElement.checkValidity and focusing the first control with an error

See the tests for useValidateOnSubmit for a worked example of a simple form using both of these.

I plan to use these new changes downstream to resolve hypothesis/product-backlog#1535. This PR only applies the change for Input components, but the same change should be applied to Select and Textarea in future.

codecov · 2024-04-26T12:30:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (1a337f4) to head (9c4d65e).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main     #1534   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           61        63    +2     
  Lines          972      1001   +29     
  Branches       373       382    +9     
=========================================
+ Hits           972      1001   +29

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

acelaya

Looks good in general.

acelaya · 2024-04-29T09:19:27Z

src/hooks/use-validation-error.ts

+    if (ref.current) {
+      ref.current.setCustomValidity(error ?? '');
+    }


Suggested change

if (ref.current) {

ref.current.setCustomValidity(error ?? '');

}

ref.current?.setCustomValidity(error ?? '');

acelaya · 2024-04-29T09:25:51Z

src/components/input/Input.tsx

+  if (error) {
+    feedback = 'error';
+  }


This will overwrite an explicitly set feedback when an error is set, which might not be super intuitive:

// This will render as `feedback="error"`. <Input error="Review this and that" feedback="warning" />

That said, considering we intend to get rid of the feedback prop, maybe we can just accept this trade-off in exchange for simplicity.

That said, considering we intend to get rid of the feedback prop, maybe we can just accept this trade-off in exchange for simplicity.

That's my plan.

Works for me then

acelaya · 2024-04-29T09:34:22Z

src/hooks/use-validate-on-submit.ts

+  const onSubmit = (event: Event) => {
+    if (event.type !== 'submit') {
+      throw new Error('Event type is not "submit"');
+    }


Would typing event as SubmitEvent help avoiding this check? If I'm not wrong, with that we would ensure the returned event handler is not allowed anywhere other than onSubmit, as TypeScript will complain.

This is safer nonetheless.

That's a good idea. I will do. I might keep the check in case there are any JS consumers in tests that misuse this somehow.

acelaya · 2024-04-29T09:41:55Z

src/hooks/use-validate-on-submit.ts

+        if ('validity' in el && !el.validity.valid) {
+          if (!foundFirst) {
+            el.focus();
+            foundFirst = true;
+          }
+
+          // If the user has focused an empty, required input field and
+          // triggered a submission by pressing Enter, that will not trigger
+          // a "change" event. Trigger this event to allow the input's "change"
+          // handler to update its custom error.
+          if (el.validity.valueMissing) {
+            el.dispatchEvent(new Event('change'));
+          }
+        }
+      }


If I'm getting this right, this if statement does two things:

Dispatch change event for every invalid element.

Focus the first invalid element.

I think all nested ifs make it a bit harder to follow due to all the conditions you have to mentally drag.

Perhaps a simple solution would be to invert the validity check condition and discard that early:

Suggested change

if ('validity' in el && !el.validity.valid) {

if (!foundFirst) {

el.focus();

foundFirst = true;

}

// If the user has focused an empty, required input field and

// triggered a submission by pressing Enter, that will not trigger

// a "change" event. Trigger this event to allow the input's "change"

// handler to update its custom error.

if (el.validity.valueMissing) {

el.dispatchEvent(new Event('change'));

}

}

}

if (!('validity' in el) || el.validity.valid) {

continue;

}

if (!foundFirst) {

el.focus();

foundFirst = true;

}

// If the user has focused an empty, required input field and

// triggered a submission by pressing Enter, that will not trigger

// a "change" event. Trigger this event to allow the input's "change"

// handler to update its custom error.

if (el.validity.valueMissing) {

el.dispatchEvent(new Event('change'));

}

}

Then this translates into 3 individual conditions which are easier to mentally parse.

Yep, this is better. Will do.

acelaya · 2024-04-29T10:08:18Z

src/hooks/test/use-validate-on-submit-test.js

+    const onSelectURL = sinon.stub();
+    const form = mount(<CustomForm onSelectURL={onSelectURL} />);
+    changeURL(form, 'not valid');
+
+    const input = form.find('input').getDOMNode();
+    const focusStub = sinon.stub(input, 'focus');
+    submitForm(form);
+
+    assert.calledOnce(focusStub);


Perhaps you could attach the component to the DOM when mounting it (mount(..., { attachTo: container })), and then check if document.activeElement is equal to form.find('input').getDOMNode() instead of stubbing the focus method. But I have to admit I have faced flaky tests when trying to do that, so I'm fine with this if the other approach is problematic.

But I have to admit I have faced flaky tests when trying to do that, so I'm fine with this if the other approach is problematic.

Indeed that's the rationale here - a call to HTMLElement.focus is much easier to check for as there is no asynchrony involved.

This sets a custom validation error for the input component which is communicated to the browser via `HTMLInputElement.setCustomValidity`. This, along with any native validation constraints such as `required`, will prevent form submission until they are resolved.

By default forms handle validation automatically when submitted, displaying an error message and focusing the first control with an error. This hook is useful if the caller wants to customize how validation error messages are presented on submission.

robertknight force-pushed the custom-form-validation branch 2 times, most recently from ee9c7df to 2e9049f Compare April 26, 2024 12:28

robertknight force-pushed the custom-form-validation branch from 2e9049f to 141b196 Compare April 26, 2024 12:36

robertknight marked this pull request as ready for review April 26, 2024 13:59

robertknight mentioned this pull request Apr 26, 2024

Improve form validation for URL picker hypothesis/lms#6214

Merged

robertknight requested a review from acelaya April 29, 2024 08:41

acelaya approved these changes Apr 29, 2024

View reviewed changes

robertknight added 2 commits April 29, 2024 11:22

Add Input.error prop

b94d151

This sets a custom validation error for the input component which is communicated to the browser via `HTMLInputElement.setCustomValidity`. This, along with any native validation constraints such as `required`, will prevent form submission until they are resolved.

robertknight force-pushed the custom-form-validation branch from 141b196 to 9c4d65e Compare April 29, 2024 10:22

robertknight merged commit 8888841 into main Apr 29, 2024
4 checks passed

robertknight deleted the custom-form-validation branch April 29, 2024 10:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Input.error` prop, `useValidateOnSubmit` hook #1534

Add `Input.error` prop, `useValidateOnSubmit` hook #1534

robertknight commented Apr 26, 2024

codecov bot commented Apr 26, 2024 •

edited

Loading

acelaya left a comment

acelaya Apr 29, 2024

acelaya Apr 29, 2024

robertknight Apr 29, 2024

acelaya Apr 29, 2024

acelaya Apr 29, 2024

robertknight Apr 29, 2024 •

edited

Loading

acelaya Apr 29, 2024

robertknight Apr 29, 2024

acelaya Apr 29, 2024

robertknight Apr 29, 2024

Add Input.error prop, useValidateOnSubmit hook #1534

Add Input.error prop, useValidateOnSubmit hook #1534

Conversation

robertknight commented Apr 26, 2024

codecov bot commented Apr 26, 2024 • edited Loading

Codecov Report

acelaya 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

robertknight Apr 29, 2024 • edited Loading

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

Add `Input.error` prop, `useValidateOnSubmit` hook #1534

Add `Input.error` prop, `useValidateOnSubmit` hook #1534

codecov bot commented Apr 26, 2024 •

edited

Loading

robertknight Apr 29, 2024 •

edited

Loading