Skip to content

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

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from

Conversation

mirka
Copy link
Member

@mirka mirka commented Aug 19, 2025

Fixes bug reported in #71161 (comment)

What?

Fixes a bug in ControlWithError where 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:

diff --git a/packages/components/src/validated-form-controls/components/stories/overview.story.tsx b/packages/components/src/validated-form-controls/components/stories/overview.story.tsx
index 6f5e9ac174..f3f4d07ddd 100644
--- a/packages/components/src/validated-form-controls/components/stories/overview.story.tsx
+++ b/packages/components/src/validated-form-controls/components/stories/overview.story.tsx
@@ -182,7 +182,7 @@ export const AsyncValidation: StoryObj< typeof ValidatedInputControl > = {
 					},
 					// Mimics a random server response time.
 					// eslint-disable-next-line no-restricted-syntax
-					Math.random() < 0.5 ? 1500 : 300
+					Math.random() < 0.5 ? 1500 : 1500
 				);
 			}, 500 ),
 			[]

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

@mirka mirka self-assigned this Aug 19, 2025
@mirka mirka requested a review from ajitbohra as a code owner August 19, 2025 21:12
@mirka mirka added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Aug 19, 2025
@mirka mirka requested review from oandregal and a team August 19, 2025 21:12
Copy link

github-actions bot commented Aug 19, 2025

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mirka <[email protected]>
Co-authored-by: tyxla <[email protected]>
Co-authored-by: jsnajdr <[email protected]>
Co-authored-by: oandregal <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@oandregal
Copy link
Member

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

@oandregal
Copy link
Member

To test in Storybook, tweak the story logic so the validation response is always slow:

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.

Copy link
Member

@tyxla tyxla left a 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();
Copy link
Member

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.

Copy link
Member Author

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.

@@ -134,6 +134,9 @@ function UnforwardedControlWithError< C extends React.ReactElement >(
case 'validating': {
// Wait before showing a validating state.
const timer = setTimeout( () => {
validityTarget?.setCustomValidity( '' );
setErrorMessage( validityTarget?.validationMessage );
Copy link
Member

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.

Copy link
Member Author

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 want errorMessage to 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.

@mirka
Copy link
Member Author

mirka commented Aug 20, 2025

@oandregal

What if we make the storybook always "slow" enough to display the states?

Makes sense 👍 125b759

I've noted another thing: note the transition from empty to filled. It displays "valid" => "validating" => "valid".

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)

Copy link

Flaky tests detected in a50671f.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/17112349514
📝 Reported issues:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants