Skip to content

DataForm: make isValid.custom async#71161

Closed
oandregal wants to merge 9 commits intotrunkfrom
update/validated-text-component-async-validation
Closed

DataForm: make isValid.custom async#71161
oandregal wants to merge 9 commits intotrunkfrom
update/validated-text-component-async-validation

Conversation

@oandregal
Copy link
Member

@oandregal oandregal commented Aug 11, 2025

See alternative PR at #71412

What?

This PR enables customers to use async custom validation.

Why?

Some validation may happen in the server, so consumers need the ability to make async calls.

How?

Testing Instructions

  • Open the storybook (npm install && npm run storybook:dev).
  • Visit the story "DataViews > DataForm > Validation".
Screen.Recording.2025-08-20.at.14.34.21.mov

@oandregal oandregal self-assigned this Aug 11, 2025
@oandregal oandregal requested a review from mirka August 11, 2025 09:17
@oandregal oandregal added [Type] Enhancement A suggestion for improvement. [Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond [Package] Components /packages/components [Feature] Component System WordPress component system [Package] DataViews /packages/dataviews labels Aug 11, 2025
@oandregal
Copy link
Member Author

@mirka @jameskoster There's this use case where the DataForm field needs to make a query to an endpoint to run some validation logic, which requires validation to be async.

One thing I've noticed with this approach is that there's no communication to the user that validation is being processed asynchronously. Do you have any thoughts on this? Perhaps we could make the ControlWithError component display a yellow message such as "validating..." or something along those lines?

@jameskoster
Copy link
Contributor

there's no communication to the user that validation is being processed asynchronously.

Yeah the delay in validation stood out to me and doesn’t seem ideal. A very short delay is probably okay, but anything more than ~300ms would benefit from some visual feedback. I suppose we could add aria-busy=“true” to the field (if we’re not already), and use that to style accordingly? Adding a small inline Spinner to the input might be the simplest solution.

@mirka
Copy link
Member

mirka commented Aug 11, 2025

aria-busy is probably not appropriate here, but adding an inline spinner seems feasible. Or for better compatibility with other types of control components, some kind of "validating ⌛" message in the error field (this will be read out to screen readers with aria-live).

However, I'm wondering whether even that is necessary 🤔 I think we've decided not to show an explicit "validated ✅" state, which kind of means that input is assumed to be valid in most cases, and we only give any kind of feedback when truly necessary. In that sense, adding a "validating ⌛" state kind of highlights the fact that there's validation going on, and almost forces the user to keep their attention there until a result is returned, even though it'll be valid in most cases. And then we don't even show a ✅ state.

So maybe we can just be optimistic there and not show a waiting state at all.

But then again, there will sometimes be fields where most input is actually invalid — I'm thinking like a username field in a sign up form. Then maybe we do need a waiting state and an explicit ✅ state for these cases.

@jameskoster Let me know if we want to try the waiting state, and I'll try to spin up a prototype at the component level.

@oandregal oandregal requested a review from Copilot August 12, 2025 05:42
@jameskoster
Copy link
Contributor

Good points there! I agree, without confirmation (when the input is valid) the purpose of the spinner is unclear and could even seem broken. But with that said a long delay without any feedback makes the UX feel unresponsive. The videos in the OP where there’s a noticeable delay in validation seem really clunky to me.

In examples like user name fields I suspect most users expect validation to take a second, so showing a waiting state and ‘Valid’ message seems reasonable to me. Perhaps this is true for most async inputs, do we have other real-world examples? If so, maybe async validation inputs have slightly different rules? E.g:

  • Validation message is visible when valid, and during validation.
  • Validation message reads “Validating” during waiting state.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@mirka
Copy link
Member

mirka commented Aug 13, 2025

I have a demo going in #71184

@oandregal oandregal force-pushed the update/validated-text-component-async-validation branch from 59b1b0b to ed5952c Compare August 19, 2025 09:45
@oandregal oandregal removed [Package] Components /packages/components [Feature] Component System WordPress component system labels Aug 19, 2025
@oandregal
Copy link
Member Author

oandregal commented Aug 19, 2025

@mirka @jameskoster I've updated this PR to leverage #71184 Would you be able to give me a confidence check about direction before I update every DataForm control? This is the current state:

Screen.Recording.2025-08-19.at.11.50.51.mov

A few things in my mind:

  • A control with async validation never removes the "Validated" label once it comes back to a valid state after an invalid one. I think it'd be neat if the "Validated" message was removed after a few seconds, as it can be noisy (in situations with many async controls) or confusing (when async and sync validated controls are in use together). However, seeing that's how the underlying component works (story) I'm reluctant to change the behavior only for DataForm.
  • For a control with async validation, when the value is invalid and there's a value change, there's no Validating... message, the value transitions to Valid directly:
Screen.Recording.2025-08-19.at.12.00.45.mov
  • The onValidate callback only runs on blur the first time; then, it is called on every change:
Screen.Recording.2025-08-19.at.12.01.15.mov

@github-actions
Copy link

Flaky tests detected in ed5952c.
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/17065866542
📝 Reported issues:

@jameskoster
Copy link
Contributor

I think it'd be neat if the "Validated" message was removed after a few seconds

Seems reasonable to try.

For a control with async validation, when the value is invalid and there's a value change, there's no Validating... message, the value transitions to Valid directly

I'm not entirely sure why that is happening, but @mirka will have a better idea.

@mirka
Copy link
Member

mirka commented Aug 19, 2025

  • A control with async validation never removes the "Validated" label once it comes back to a valid state after an invalid one. I think it'd be neat if the "Validated" message was removed after a few seconds, as it can be noisy

Ok, I'll try adding this behavior in the components. Though, I wonder if it will be annoying with the layout shifts, given that the user will likely be interacting with the rest of the form when the "Validated ✅" indicator unmounts.

  • For a control with async validation, when the value is invalid and there's a value change, there's no Validating... message, the value transitions to Valid directly:

Thanks for catching. I've identified the bug — proposed fix in #71260.

  • The onValidate callback only runs on blur the first time; then, it is called on every change:

We decided on this global behavior as part of the initial specs. Is your concern about firing too many server calls? If so, I think that should be dealt with by debouncing the call. (I think the reasoning for this validation timing logic was that it's not necessarily obvious to the user that they need to blur the field for it to revalidate.)

@oandregal
Copy link
Member Author

We decided on this global behavior as part of the initial specs. Is your concern about firing too many server calls? If so, I think that should be dealt with by debouncing the call. (I think the reasoning for this validation timing logic was that it's not necessarily obvious to the user that they need to blur the field for it to revalidate.)

Yeah, I was thinking that validating on blur is not clear. This is more visible in forms with a single field to validate: for example, quick edit or action modals (the current examples we have in the site editor). Users need to click outside the control for it to trigger validation in those scenarios.

@oandregal oandregal force-pushed the update/validated-text-component-async-validation branch from ed5952c to 092c1ef Compare August 20, 2025 12:29
@oandregal oandregal marked this pull request as ready for review August 20, 2025 12:30
@github-actions
Copy link

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: oandregal <oandregal@git.wordpress.org>
Co-authored-by: jameskoster <jameskoster@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>

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

@mirka
Copy link
Member

mirka commented Aug 20, 2025

I was thinking that validating on blur is not clear. This is more visible in forms with a single field to validate: for example, quick edit or action modals

Is something like #71282 useful in these cases?

@oandregal
Copy link
Member Author

oandregal commented Aug 21, 2025

Yeah, that highlights a few things we need to fix in the validation flow when we use the panel layout:

Screen.Recording.2025-08-21.at.18.49.43.mov
  • can users close a dropdown/modal that is invalid? If they can, do we display the validity state in the panel somehow?
  • what happens when we reopen the panel, and the component is invalid, but the user hasn't interacted with it yet? Can we force-trigger validation or "mark it" as invalid somehow?
  • required label: it's missing in the dropdown/modal that opens from the panel (same as they have in the regular layout)

In any case, I think it's a different conversation: even if we switched from validating onBlur to validate upon interaction, the issues would persist (though it'd perhaps clearer that field is invalid).

@jameskoster
Copy link
Contributor

My 2c: it should be possible to close an invalid field dialog, but we should show the validation status when invalid in the panel somehow. I suppose this is necessary for forms that have an initially-invalid field (e.g. 'Agree to terms' checkbox).

required label: it's missing in the dropdown/modal that opens from the panel (same as they have in the regular layout)

We hide the label (where the 'required' demarkation lives) in favor of showing the dialog title. We don't show both because 99% of the time they're going to be duplicative. I suppose it was decided to show only the dialog title to avoid this:

Screenshot 2025-08-22 at 09 48 47

However, showing the label (because it contains the 'required' detail) now seems more important (so long as there remains an accessible dialog title).

Perhaps the dialog should be header-less (no visible title or close button), but include 'Apply' and 'Cancel' buttons? Then we can show only the label:

Screenshot 2025-08-22 at 10 04 28

Alternatively we could make the dialog title more explicit. There's still some duplication here, but it avoids adding additional buttons and allows us to keep a close button (which are generally an expectation when interacting with dialogs):

Screenshot 2025-08-22 at 10 01 45

I can go either way on this. There's something nice about the explicit buttons—they provide a clear way to discard/cancel any changes, which is missing in the current UX. But a visible dialog title might be useful for combined fields, e.g. a group of inputs with title 'Edit Address' seems neat and tidy. To be totally honest; part of me wants the throw out the popover approach altogether and just use modals with visible titles and apply/cancel buttons, but that's probably a controversial opinion 😅

@mirka
Copy link
Member

mirka commented Aug 22, 2025

There's something nice about the explicit buttons—they provide a clear way to discard/cancel any changes

Another advantage of explicit save/cancel buttons is that we can make it so users can naturally submit the value and dismiss the popover with the Enter key. Right now you'd have to hit Escape to close, even you want to commit the value.

@jameskoster
Copy link
Contributor

Yeah, I think the UX is clearer with the buttons. Another option would be to combine both approaches outlined in my previous comment; Dialog title + close button + explicit buttons. Basically a mini modal.

But I know there has been a lot of visual design pushback on including buttons like this in the past so I'd welcome feedback from @WordPress/gutenberg-design before committing.

@oandregal oandregal force-pushed the update/validated-text-component-async-validation branch from 092c1ef to 2d50200 Compare August 26, 2025 16:09
@oandregal oandregal marked this pull request as draft August 26, 2025 16:09
@oandregal
Copy link
Member Author

Switching this to draft again because I'm incorporating some logic coming from #71345

@oandregal oandregal force-pushed the update/validated-text-component-async-validation branch from 2d50200 to d61877f Compare August 27, 2025 12:27
@oandregal oandregal force-pushed the update/validated-text-component-async-validation branch from d61877f to e52acd2 Compare August 27, 2025 17:08
@oandregal
Copy link
Member Author

oandregal commented Aug 27, 2025

Worked a bit on this today. I may split this PR is smaller ones when direction is clear, but I wanted to solve a few things we need to address: validation needs to run only once, it needs to support sync and async modes, and consumers need to receive the validation status (so they can control things outside DataForm's scope, such as a "save button").

Screen.Recording.2025-08-27.at.19.17.54.mov

(For context: the 1st approach to validation exposed a isItemValid( item, fields, form ). Validation ran in consumer-code only/when they wanted. With the introduction of the validated UI components, we're running validation twice: one in isItemValid and another in the leaf UI component to display proper error/messages — we need to consolidate.)

The current proposal:

  • The leaf UI component handles a single-field validation and updates its UI accordingly (color, messages). It relays the validation message to the form. Link for text control (others would work the same).
  • The form is notified when any leaf UI control was validated and updates the form state accordingly, then, relays it to the consumer. Link to form.

Things to address/clarify about direction:

  • Absorb component built-in validation. While this direction works for custom validation, we also need to absorb built-in validation into this model: for example, required is delegated to the browsers to handle (and we can have more built-in validation in the future such as min/max, patterns, etc.). I think we can leverage the Constraint Validation API for this, but I haven't investigated that yet.
  • API for consumers. This PR proposes a reactive onValidate callback consumers can use. An alternative to that would be to relay the message via the existing onChange method, but I see two issues:
    • the onChange method fires more often than component validation. For example, the onChange fires every time the user types anything, but component validation only fires on blur the 1st time (then, it happens on change).
    • validation can be async, so we'll need to execute the onChange callback upon user changes (sync) and upon validation result is received (async). It sounds like mixing concerns.
  • isItemValid. We may want to deprecate this API, though I wonder if we still need it to validate the form when the user hasn't interacted yet (on load).

cc @mirka @youknowriad for thoughts, as I've been discussing separate things with both of you. Hope this is context enough and not too long.

Comment on lines +686 to +687
disabled={ isFormBusy || ! isFormValid }
isBusy={ isFormBusy }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a button be busy and disabled, @mirka? Note this weird interaction:

  • Button is disabled because form is invalid.
  • Some interaction triggers an async validation: button becomes busy, and changes appearance to "active".
  • Async validation resolves and button goes back to invalid.
Screen.Recording.2025-08-27.at.19.22.06.mov

@oandregal
Copy link
Member Author

I paired with Riad to discuss the current approach (validation is uncontrolled by the consumers, they receive the status via onValidate callback):

  • How to relay the built-in component validation state to the consumer (e.g., required state), which is now controlled. This may be solvable through the Constraint API.
  • By having two callbacks (onChange and onValidate) that fire separately, consumer code could run into race conditions if both trigger related effects (e.g., updating a URL).
  • Initial validated state. We may still need a utility to calculate it before any user modification (on load).

Riad suggested we could try a controlled approach to validation (the consumer passes the validation status via prop):

  • This approach requires we generate all messages for validation rules (including rules that could have been built-in by the component/browser, such as required). We may need to pass everything as "custom validation" (customValidity) to the component. I need to investigate this more.

@oandregal
Copy link
Member Author

Riad suggested we could try a controlled approach to validation

Here's the PR #71412 It works for sync validation, I need to update it to also cover async.

@oandregal
Copy link
Member Author

We're going to go with #71412 instead

@oandregal oandregal closed this Oct 16, 2025
@oandregal oandregal deleted the update/validated-text-component-async-validation branch October 16, 2025 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond [Package] DataViews /packages/dataviews [Type] Enhancement A suggestion for improvement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants