Add improved live-region to FormControl validation
#4445
Conversation
🦋 Changeset detectedLatest commit: 443b761 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
| @@ -44,7 +44,7 @@ export function Announce({children, politeness = 'polite', ...rest}: AnnouncePro | |||
| // When the text of the container changes, announce the new text | |||
| const observer = new MutationObserver(mutationList => { | |||
| for (const mutation of mutationList) { | |||
| if (mutation.type === 'characterData') { | |||
| if (mutation.type === 'characterData' || mutation.type === 'childList') { | |||
There was a problem hiding this comment.
I'd love your opinion here @joshblack!
When I added <Status> initially to FormControl, I realized that the live-region element was not being populated. This seems to be because when slots.validation is true, the validation region is rendered within the DOM which the mutation observer does not seem to be catching. This appears to be due to it not being a change that would fall under characterData, as elements are either appended within <Status> or removed. Using childList catches this and populates live-region when the contents within are rendered or removed.
Not sure if there is a better way to go about this, or if this was intended behavior 🤔
There was a problem hiding this comment.
Oh whoa 🤯 I totally thought that characterData would change if the child list would change 🤔
Maybe something we can do on mutation is to include things like childList so that it gets triggered correctly and we just check manually to see if the message has changed? Let me know if you'd want to pair on this btw!
There was a problem hiding this comment.
That's a good idea, that way if contents within change but there's no movement in terms of the text content, we would only call announceFromElement if the text content has changed between mutations. I'll dig on this a bit, but I think this would be a good opportunity to pair!
| {slots.validation ? ( | ||
| <ValidationAnimationContainer show>{slots.validation}</ValidationAnimationContainer> | ||
| ) : null} | ||
| <Status> |
There was a problem hiding this comment.
I'm curious if we'd want to provide an option to turn the live region off. For example, if multiple form fields produce an error at the same time they will all be announced through the live region, which isn't ideal and is the default behavior outside of this PR.
I'm not too sure on how we should give the option to turn live regions off. Should it be a prop, or should we handle this ourselves internally?
Cc: @joshblack, would love your input here 👀
There was a problem hiding this comment.
Ooof, that's a great question 😅 This is likely something that needs to be handled at the <form> level in order to coordinate. What specific scenarios are we expecting validation feedback to be announced?
There was a problem hiding this comment.
We'd mainly want it to announce when there's a validation failure while the user is within the input. More context on why here: https://github.com/github/accessibility-audits/issues/6883#issuecomment-2021473451.
I feel like in instances in where there's more than one error, only the first should announce, as focus would ideally be set to that input, and users would need to navigate through the rest of the form in order to submit it again. There may even be cases where the consumer doesn't want live regions to begin with, which is fair.
There was a problem hiding this comment.
@TylerJDev so it seems like this case is for when feedback for an input is "instant" versus when it is submitted as part of a form, is that right? 👀
There was a problem hiding this comment.
Yup, exactly! I think the cases where all forms would suddenly show an error on submit is rare as that's the purpose of the inline validation, but there is still a chance that it could happen. Part of me feels like this might not be needed now, but something we might want to add later 🤔
|
@TylerJDev Hiya! Do we have a status on this PR? |
|
@lindseywild, this PR is waiting on #4590 to be merged as it fixes a bug that's breaking CI in this PR. Once it's merged I'll be able to set this one to ready for review! |
|
Thank you!! |
|
Going to close this PR for now, but may reopen it in the future if we decide we want to add live region support to |
https://github.com/github/accessibility-audits/issues/6883, https://github.com/github/accessibility-audits/issues/6881
Currently, our validation message is rendered conditionally based on if
slots.validationis present at the time. This may mean the live region is not added on page load, potentially preventing some screen readers from announcing the content within that region.This PR aims to introduce our new
<Status>element, and replace the live region that lives inside ofInputValidation. In the future, we may want to rely solely on thearia-describedbyattached to the input, but for now, this ensures that the live region works as expected.Changelog
New
Changed
Removed
Rollout strategy
Testing & Reviewing
Merge checklist