| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
ControlWithError: Show validating state when transitioning from error state #71260
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
Conversation
mirka
added
[Type] Bug
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
This works for the issue reported, though I've noted another thing: note the transition from empty to filled. It displays "valid" => "validating" => "valid". Screen.Recording.2025-08-20.at.08.52.00.mov |
What if we make the storybook always "slow" enough to display the states? While I appreciate the randomness, I favor the educational purpose of the storybook more. |
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.
Looks good to me 👍 Thanks for the fix!
| act( () => jest.advanceTimersByTime( 1 ) ); | ||
|
|
||
| await waitFor( () => { | ||
| expect( screen.getByText( 'Validated' ) ).toBeInTheDocument(); |
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.
I always prefer toBeVisible() because with toBeInTheDocument() we can't guarantee the user actually sees it.
.not.toBeInTheDocument() for things we want to ensure user "does not see" makes sense, since .not.ToBeVisible() will error out.
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.
Thanks for catching, that was an oversight. I'll give the intern a stern talking to add this rule to my Cursor.
| // Wait before showing a validating state. | ||
| const timer = setTimeout( () => { | ||
| validityTarget?.setCustomValidity( '' ); | ||
| setErrorMessage( validityTarget?.validationMessage ); |
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.
Maybe in this case we want to setErrorMessage( undefined )? There is no error while validation is running.
The use case is interaction of the onValidate custom (server-side) validation with client-side validation, e.g., when there is something like <input type="email"> and the value is not email.
In that case, when additional custom validation is in flight, and status is validating, do we want errorMessage to show the "not email" error or not.
I'm not sure if we even support this combination of custom and native validation.
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.
I'll be adding tests soon to help document these intended behaviors, but yes, we do support the combination of attribute-based and customValidity. I tried to match how the native APIs handle it, in that the customValidity message will be prioritized when it's a non-empty string, but otherwise the rest of the attribute-based validation errors will remain. It also matches the native browser logic of how errors messages are displayed — there can be multiple errors but only one error message is shown at a time.
In that case, when additional custom validation is in flight, and status is
validating, do we wanterrorMessageto show the "not email" error or not.
That's a good question. I'm not sure if there's a universally better behavior, but I think in the general case, yes, we do want to show the "not email" error without waiting for the custom validator result.
Let's say there's a 50/50 chance of the custom async validator responding as error, and we wait for the async result before notifying the user about the "not email" error. This means that 50% of the time we made the user wait for no reason, because we already knew about the "not email" 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.
Turns out, in practice it did feel better to make the user wait in all cases 😅 Going with setErrorMessage( undefined ) as in your suggestion.
Makes sense 👍 125b759
Ugh, I don't know what the ideal behavior would be here. It's a tricky UX problem rooted in the fact that we're mixing validations that don't show an explicit "valid" indicator (all sync validation, including HTML attribute-based ones) and those that do (any async validation). On one hand, I feel like we should stay in the "explicit indicator" paradigm once we show an explicit valid indicator for any given field, because it will be weird for some validation rules to show it and some to not. But then, we could also say that about the entire form, where it's weird that some fields have explicit indicators and some don't. Let's revisit this in conjunction with the "auto-dismiss the 'valid' indicator" experimentation. I don't think we have a clear UX solution to this root problem yet. (cc @jameskoster) |
|
Flaky tests detected in bc50407. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/17249040691
|
|
I think the problem in the video is that the 'valid' message appears briefly before the 'validating' one. If we could eliminate that first flash of 'valid' then I think we'd be ok. Why does that happen, is it because we're mixing sync and async validation logic on the same input? |
Right. At that moment the local state is "valid", until the async validator tells us otherwise. I continued to experiment with a some patterns, and I came to the conclusion that if a field has an async validator, it's most intuitive to delay any valid/invalid status updates until the async validator responds: CleanShot.2025-08-22.at.12-45-05.mp4It kind of maximizes user wait times, because we would make them wait to show them errors we already knew from local validation. But it still feels most natural because all validations (regardless of sync/async) take about the same amount of time. It's predictable. Another benefit of this is that a local error message will not be suddenly overwritten by an async error as the server responds. (I also tried a variation on this, where we don't even trigger the custom validator while there are local errors (e.g. |
|
Size Change: +10 B (0%) Total Size: 1.92 MB
ℹ️ View Unchanged
|
|
I also discovered some more tweaks necessary to fully support async, so I will continue iterating in #71310. |
| /> | ||
| ); | ||
| }, | ||
| play: async ( { canvasElement } ) => { |
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 is nice and weird 😅 The first time I encountered I felt it was a bug. It'd be nice to trigger this intentionally from a button instead of executing it on load.
It makes it impossible to test/learn how this component works: for example, due to having this setup, the first user validation will trigger on change, not on blur. So I think I'd rather not have this if this can't be triggered by users.
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.
That's fair 😅 Unfortunately there's no way to disable the autoplay on the individual story pages. I'll keep the story unexported so it's still available for test purposes (Jest was a bit unreliable for this one).
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 is working nicely for me.
Labels
5 participants
Fixes bug reported in #71161 (comment)
What?
Fixes a bug in
ControlWithErrorwhere a "validating" state would not show when transitioning from an "error" state.Testing Instructions
To test in Storybook, tweak the story logic so the validation response is always slow:
Go to the Validated Form Controls ▸ Overview ▸ Async Validation story. Type "error" and blur the field to trigger an error message. Then, change the input to clear the error. A "validating" indicator should show on both validation attempts.
In the newly added unit tests (dea1939), a test case should fail on this exact condition without the fix (1046981) applied.
Screen recording
CleanShot.2025-08-20.at.06-10-40.mp4