ControlWithError: Connect validation messages to controls via aria-describedby#76742
ControlWithError: Connect validation messages to controls via aria-describedby#76742
ControlWithError: Connect validation messages to controls via aria-describedby#76742Conversation
| // The `help` prop is rendered visually by BaseControl but is not | ||
| // programmatically associated with the combobox input via aria-describedby. | ||
| // This is a pre-existing bug in ComboboxControl, not caused by ControlWithError. |
| // The `help` prop is rendered visually by BaseControl but is not | ||
| // programmatically associated with the toggle group via aria-describedby. | ||
| // Additionally, the validity target is a hidden delegate radio input, not the | ||
| // toggle group itself. These are pre-existing bugs, not caused by ControlWithError. |
| required, | ||
| } ) } | ||
| <div aria-live="polite">{ showMessage && message() }</div> | ||
| <div aria-live="polite">{ visibleMessage }</div> |
There was a problem hiding this comment.
The original issue suggested replacing this live region with a11y.speak(), but this would require a substantial refactor. Let's leave it this way for now.
There was a problem hiding this comment.
Should we track it separately?
|
Flaky tests detected in 28dc66e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/23588713796
|
|
Size Change: +130 B (0%) Total Size: 7.69 MB
ℹ️ View Unchanged
|
|
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. |
aduth
left a comment
There was a problem hiding this comment.
I'll preemptively approve this because I think this sufficiently addresses majority usage and there's some time sensitivity, though as noted in review comments I wonder if there's some edge cases that could lead the association to be inadvertently dropped.
| await waitFor( () => { | ||
| expect( checkbox ).toHaveAccessibleDescription( | ||
| expect.stringContaining( 'Constraints not satisfied' ) | ||
| ); | ||
| } ); | ||
| expect( checkbox ).toHaveAccessibleDescription( | ||
| expect.stringContaining( 'You must agree to continue' ) | ||
| ); |
There was a problem hiding this comment.
Do we want order to matter in the description text? And should we guarantee a delimiter between the sentences so that screen readers announce it as two separate statements?
For example, this assertion passes for me locally, but I'm not positive on the order of error message vs. help text, and maybe we'd want a period between the statements to avoid it being read aloud as one continuous sentence.
await waitFor( () => {
expect( checkbox ).toHaveAccessibleDescription(
'You must agree to continue Constraints not satisfied'
);
} );There was a problem hiding this comment.
Adding punctuation would be good, yes.
There was a problem hiding this comment.
I added trailing periods to the descriptions in the tests, which match WP copy conventions anyway (1738692).
I think the description order makes sense here, given that it matches the visual order, and also because it's harder to ensure trailing punctuation in built-in validation message strings coming from the browser. (In this case, I believe "Constraints not satisfied" is a "fake" string coming specifically from jsdom.)
There was a problem hiding this comment.
Also confirmed that our single in-app use case has a trailing period:
| } | ||
| } | ||
|
|
||
| setDescribedBy( target, !! visibleMessage ); |
There was a problem hiding this comment.
One concern about this: Is it possible that a child component could re-render and re-apply its own aria-describedby (thus clobbering our error message addition) without triggering the effect to re-run? For example, if someone dynamically changes the help value of a control component; nothing about this effect or its dependencies would detect that the child would have re-rendered.
I'm not sure I entirely understand the complications that led to the imperative approach you described in the initial message, but would it be possible to either:
- Expect each control component to accept an optional
aria-describedbyand merge it with its own help message idref, then we could pass ours from this wrapper component - Or, handle
helphere instead, crafting the fullaria-describedbyto be used
Worst case, maybe we could do something with a MutationObserve to detect and handle any child re-renders.
There was a problem hiding this comment.
Right. I'm hoping 1 is possible, but I'll have to think through the feasibility/scalability in the general case (in relation to #76741 as well). My gut feeling is that it may be hard for developers integrating with ControlWithError to understand the aria-describedby plumbing necessary to make their custom component work accessibility with the validation system. So there's an API design and quality control concern there too.
Will follow up as part of #76741.
|
Just noting that we're in RC now. Is this a regression or a bug fix to a new feature? Just double checking. |
|
The form field validation is a new feature, but the implementation does not conform to WCAG criteria, so it's a legitimate backport for WP 7. |
4092b1c to
28dc66e
Compare
|
There was a conflict while trying to cherry-pick the commit to the wp/7.0 branch. Please resolve the conflict manually and create a PR to the wp/7.0 branch. PRs to wp/7.0 are similar to PRs to trunk, but you should base your PR on the wp/7.0 branch instead of trunk. |
…-describedby` (#76742) * Components: Connect validation error messages to inputs via aria-describedby * Add per-control aria-describedby tests, simplify useEffect * Add changelog * Fix type errors in tests and control-with-error * Add trailing periods to descriptions in tests * Fix unit tests after recent group=>radiogroup changes --------- Co-authored-by: mirka <[email protected]> Co-authored-by: aduth <[email protected]> Co-authored-by: joedolson <[email protected]> Co-authored-by: ciampo <[email protected]> Co-authored-by: ellatrix <[email protected]> # Conflicts: # packages/components/CHANGELOG.md
#76835) * `ControlWithError`: Connect validation messages to controls via `aria-describedby` (#76742) * Components: Connect validation error messages to inputs via aria-describedby * Add per-control aria-describedby tests, simplify useEffect * Add changelog * Fix type errors in tests and control-with-error * Add trailing periods to descriptions in tests * Fix unit tests after recent group=>radiogroup changes --------- Co-authored-by: mirka <[email protected]> Co-authored-by: aduth <[email protected]> Co-authored-by: joedolson <[email protected]> Co-authored-by: ciampo <[email protected]> Co-authored-by: ellatrix <[email protected]> # Conflicts: # packages/components/CHANGELOG.md * Revert test changes for this branch --------- Co-authored-by: aduth <[email protected]> Co-authored-by: joedolson <[email protected]> Co-authored-by: ciampo <[email protected]> Co-authored-by: ellatrix <[email protected]>
|
Manually cherry picked into |
…-describedby` (#76742) * Components: Connect validation error messages to inputs via aria-describedby * Add per-control aria-describedby tests, simplify useEffect * Add changelog * Fix type errors in tests and control-with-error * Add trailing periods to descriptions in tests * Fix unit tests after recent group=>radiogroup changes --------- Co-authored-by: mirka <[email protected]> Co-authored-by: aduth <[email protected]> Co-authored-by: joedolson <[email protected]> Co-authored-by: ciampo <[email protected]> Co-authored-by: ellatrix <[email protected]>
What?
Closes #76694
Explicitly connect validation error messages to their respective form controls using
aria-describedby, so that screen reader users can access error messages when navigating back to a field.Why?
Validation errors were displayed visually and announced via
aria-live, but not programmatically associated with the input viaaria-describedby. The errors are tied to the element through the Constraint Validation API, but if a user missed the live announcement (e.g. due to distraction or another announcement overriding it), there was no way to discover the error by returning to the field.How?
ValidityIndicatoranidprop and apply it to its root<p>element.ControlWithError, generate a stablemessageIdviauseId()and pass it toValidityIndicator.aria-describedbyon the validity target element via auseEffect, merging themessageIdalongside any existingaria-describedbyvalues (e.g. from ahelpprop). In other words, we can't merge the attribute throughcloneElementbecause we can't know the internals of the cloned element and whether ahelpprop or something maps toaria-describedbyinternally.Known limitation
For controls that use a hidden element as a validation delegate (
ValidatedCustomSelectControl,ValidatedFormTokenField,ValidatedToggleGroupControl), thearia-describedbyis attached to the hidden delegate rather than the interactive control, making this fix rather useless. Tracked separately in #76741, and bumped from this PR because it doesn't affect shipping features in WP 7.0.Testing Instructions
Automated
New test coverage across 13 dedicated test files, one per validated control:
helpprop (CheckboxControl,TextControl,TextareaControl,InputControl,NumberControl,SelectControl,RadioControl,RangeControl,ToggleControl): tests that the help description is preserved and that the validation error is appended alongside it.helpwiring bugs (ComboboxControl,ToggleGroupControl): tests asserting correct behavior, skipped with comments explaining the pre-existing issue.FormTokenField,CustomSelectControl): tests that the interactive control's built-in description is not broken by validation.The
control-with-error.tsxtest file retains tests for corearia-describedbymechanism behavior: basic connection, rawaria-describedbyprop preservation,customValiditypath, and cleanup on error resolution.Manual
npm run storybook:devand navigate to theValidatedTextControlstory.requiredvia the controls panel).Use of AI Tools
Authored with Cursor (Claude).