-
Notifications
You must be signed in to change notification settings - Fork 14
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.
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
Improve form validation for URL picker #6214
Conversation
// Sync HTML validation state with displayed error. | ||
useLayoutEffect(() => { | ||
input.current!.setCustomValidity(error ?? ''); | ||
}, [error]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do this in validateInput
, but that would break if ever there was a situation where the form started out in an error state. Doing it this way ensures that the validation state is always in sync with the displayed error.
if ('validity' in el && !el.validity.valid) { | ||
el.focus(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTMLFormElement.reportValidity
does this normally as part of showing the browser-native validation error. Unfortunately it isn't possible to get the focusing without the native validation message.
variant="primary" | ||
type="submit" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes form submission always go through the form's "submit" event, so submission works consistently.
82c54aa
to
7927436
Compare
Revise how validation for the URL input works so that: - Validation errors due to the `required` property on the input and the custom URL validation are handled the same way. Previously a native form validation error would be presented if `required` was missing and a custom error was shown for invalid URLs. - Pressing the "Submit" button focuses the first input field with an error - Validation issues with the input field are shown when changes are committed (eg. when the field is blurred), not just when it loses focus. This is done by: - Integrating custom constraint validation errors with the native validation API, by using the `Input.error` prop. - Doing validation in the "change" event of the input, instead of the form's "submit" event. Note that when the user commits and submits at the same time by pressing enter when the input has focus, the layout effect that sets the validity runs before the "submit" event. - Setting `noValidate` on the form to disable the browser's own UI messages. This disables both the UI message but also the behavior of focusing the first input with an error, which we want to keep. - Use the `useValidateOnSubmit` hook to focus the first field with an error on submission. All the above aligns with how eg. React Aria handles communicating validation state to the browser while displaying a custom message. See https://react-spectrum.adobe.com/react-aria/forms.html. Part of hypothesis/product-backlog#1535
7927436
to
33e4a82
Compare
@@ -74,9 +83,10 @@ export default function URLPicker({ | |||
</Button>, | |||
<Button | |||
data-testid="submit-button" | |||
form={formId} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#form
I remember trying to make this button a submit one, and having to discard it because it was technically not "in" the form. It was this easy 😅
Depends on hypothesis/frontend-shared#1534.
Revise how validation for the URL input works so that:
Validation errors due to the
required
property on the input and the customURL validation are handled the same way. Previously a native form validation
error would be presented if
required
was missing and a custom error wasshown for invalid URLs.
Pressing the "Submit" button focuses the first input field with an error
Validation issues with the input field are shown when changes are committed
(eg. when the field is blurred), not just when it loses focus.
This is done by:
Integrating custom constraint validation errors with the native validation
API, by using the
Input.error
prop.Doing validation in the "change" event of the input, instead of the form's
"submit" event. Note that when the user commits and submits at the same time
by pressing enter when the input has focus, the browser dispatches "change" before "submit"
Setting
noValidate
on the form to disable the browser's own UI messages.This disables both the UI message but also the behavior of focusing the
first input with an error, which we want to keep.
Use the
useValidateOnSubmit
hook to focus the first field with anerror on submission.
All the above aligns with how eg. React Aria handles communicating validation
state to the browser while displaying a custom message. See
https://react-spectrum.adobe.com/react-aria/forms.html.
Part of hypothesis/product-backlog#1535