Repository navigation
Validated form controls: Suppress native popover - #73471
Conversation
| // Radio inputs need special handling because all radio inputs with the | ||
| // same `name` will be marked as invalid. | ||
| const radioSibilings = |
There was a problem hiding this comment.
One of the topics discussed in the original issue was whether this would be "worth the extra trouble".
So far, from the set of use cases that we currently have, radio inputs were the only thing that needed special handling like this. There may be more, and I have some qualms about the screen reader behavior, but perhaps not too bad to count as hard blockers.
There was a problem hiding this comment.
I'm having a hard time understanding what happens if we just... didn't have this code for radio inputs.
There was a problem hiding this comment.
My bad. Here's what happens when you click the submit button on an untouched radio group. The "Before" version has the radio handling in this hook commented out.
| Before | After |
|---|---|
![]() |
![]() |
I expanded on the code comment a bit (793ace0). Ideally we have a unit test, but unfortunately this is not accurately testable in jsdom.
There was a problem hiding this comment.
Gotcha 👍
Thinking out loud a bit: Rather than dealing with getValidityTarget as assuming there's some "one element" to be validated (which your comment seems to suggest is not actually the case), could we bind to the wrapper and watch for the event in capture phase?
Something like...
wrapperRef.current?.addEventListener( 'invalid', suppressNativePopover, true );Or maybe it should be getValidityTargets, which is usually an array of one, but could be multiple?
Definitely not a blocker.
There was a problem hiding this comment.
At the moment I'm not considering supporting multiple validity targets for a control, because there's only one slot to show an error message.
But listening to all invalid events within a control wrapper might work… 🤔 That might be preferable if we encounter additional cases that need a similar workaround.
|
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. |
|
Not sure if we'll merge, but I'll open this up to feedback. |
|
At a very high level glance at just the before and after, purely conceptually the path forward is clear: it should be one or the other, not both. |
115df92 to
4e9608c
Compare
|
I think we're all aligned that suppressing the native popovers is better for general UX. Since this is trivial to revert if it causes complications down the line, I would be comfortable trying it out to see how it goes. I've rebased this with the latest validation logic, so the PR is ready for review (with the assumption that it will actually be merged) 🙂 |
| ); | ||
|
|
||
| expect( | ||
| screen.getByRole( 'textbox', { name: /^Text1/ } ) |
There was a problem hiding this comment.
Nit: Should we expect that the name would be exactly Text1 ?
| screen.getByRole( 'textbox', { name: /^Text1/ } ) | |
| screen.getByRole( 'textbox', { name: 'Text1' } ) |
There was a problem hiding this comment.
This is just side-stepping the detail that the required prop adds a (Required) suffix to the labels. Kind of wanted to exclude that detail from these tests.
| if ( ! target.form ) { | ||
| target.focus(); | ||
| return; | ||
| } | ||
|
|
||
| const firstErrorInForm = Array.from( target.form.elements ).find( | ||
| ( el ) => ! ( el as ValidityTarget ).validity.valid | ||
| ); | ||
|
|
||
| if ( firstErrorInForm === target ) { | ||
| target.focus(); | ||
| } |
There was a problem hiding this comment.
Could we use :invalid query selector to simplify this?
| if ( ! target.form ) { | |
| target.focus(); | |
| return; | |
| } | |
| const firstErrorInForm = Array.from( target.form.elements ).find( | |
| ( el ) => ! ( el as ValidityTarget ).validity.valid | |
| ); | |
| if ( firstErrorInForm === target ) { | |
| target.focus(); | |
| } | |
| if ( ! target.form || target.form.querySelector( ':invalid' ) === target ) { | |
| target.focus(); | |
| } |
There was a problem hiding this comment.
Ugh, this is a great idea I didn't think of, but unfortunately this querySelector( ':invalid' ) seems to trigger a recursive validation check in jsdom and makes every test hang 😭 I reshuffled the original code to make it a bit simpler though (41d019f).
| // Radio inputs need special handling because all radio inputs with the | ||
| // same `name` will be marked as invalid. | ||
| const radioSibilings = |
There was a problem hiding this comment.
I'm having a hard time understanding what happens if we just... didn't have this code for radio inputs.
# Conflicts: # packages/components/CHANGELOG.md
| // Radio inputs need special handling because all radio inputs with the | ||
| // same `name` will be marked as invalid. | ||
| const radioSibilings = |
There was a problem hiding this comment.
Gotcha 👍
Thinking out loud a bit: Rather than dealing with getValidityTarget as assuming there's some "one element" to be validated (which your comment seems to suggest is not actually the case), could we bind to the wrapper and watch for the event in capture phase?
Something like...
wrapperRef.current?.addEventListener( 'invalid', suppressNativePopover, true );Or maybe it should be getValidityTargets, which is usually an array of one, but could be multiple?
Definitely not a blocker.
|
I'm trying to assess this as I review accessibility related changes in 7.0, but I'm not sure where I can test this within core. Can you point me to some examples of inputs in core using this validation? I'm aware of the stories, but I want to be testing this in a use case. |
|
@joedolson Thanks for looking at this. I believe the only use case in 7.0 is the CleanShot.2026-03-20.at.02-45-04.mp4 |
|
And it seems like there was no validation on that in 6.9, so it's actually a completely new addition, from the user perspective. Checking on it! |
|
No problems with suppressing the native error, but there is an issue with the error message - it's not explicitly connected to the input field; I'm going to open a new issue. |


What?
Closes #73015
Try suppressing the native error popover that appears when a form is validated, while keeping the focus behavior as is.
Why?
The popover can obscure our custom error messages, and are redundant.
How?
preventDefault()oninvalidevents on the validity target, and then add back the focus logic.The concern here is whether we're overlooking any other "default behavior" that would be bad to prevent.
One that I noticed so far is, at least in VoiceOver on Chrome and Safari, hitting the submit button on an invalid form plays a special sound effect in addition to announcing the first error again. This behavior is lost by doing
preventDefault(). We can mimic the announcement programmatically, but not the sound effect.Testing Instructions
Try the Validated Form Control stories in the three major browsers.
Screenshots or screencast
Before
CleanShot.2025-11-21.at.05-26-35.mp4
After
CleanShot.2025-11-21.at.05-25-55.mp4