-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix(number-input): update value of errorId for aria-labelledby on input #4295
fix(number-input): update value of errorId for aria-labelledby on input #4295
Conversation
Deploy preview for the-carbon-components ready! Built with commit c0ea57c https://deploy-preview-4295--the-carbon-components.netlify.com |
Deploy preview for carbon-components-react failed. Built with commit c0ea57c https://app.netlify.com/sites/carbon-components-react/deploys/5d9fad3026a9260007fd7e07 |
Deploy preview for carbon-elements ready! Built with commit c0ea57c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this has a DAP error -- aria-describedby
still referencing an invalid error ID. But if you cause an error the violation goes away.
The id tj-input-error-id specified for WAI-ARIA property aria-describedby on element input is not valid
I'm also not seeing aria-labelledby
being set on the input element when there is an error if that was the intention.
@dakahn do you mind taking another look using this deployment URL? https://deploy-preview-4295--carbon-components-react.netlify.com/ In my testing instructions, I put the link to the published storybook, not the one for my PR deployment 😅 I fixed that link now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on Chrome latest using current DAP ruleset and looks 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 - Thanks @jendowns!
@dakahn After running DAP on the deployment URL (https://deploy-preview-4295--carbon-components-react.netlify.com/), I'm seeing a new DAP error. "the accessible name must match or contain the visible label text.". Can you check? (see screenshot) |
Just checked again @snidersd using the September 2019 ruleset on Chrome Using that deploy preview link I don't have that error 😕 |
@dakahn Are you using the IBM Accessibility WCAG 2.1 September 2019 ruleset or IBM Accessibility September 2019? I'm seeing the issue using WCAG 2.1. |
I have the same error ("Accessible name must match or contain the visible label text") when testing with the same ruleset |
Closes #4198
In the originating issue,
aria-labelledby
(which was referring theid
of the error message) was always present even when there wasn't an error, causing a DAP violation.So this PR proposes tying the
errorId
to theerror
message, since their values are connected. So if theerror
exists, so does theerrorId
-- otherwise both arenull
.NOTE: in the process of looking into this, I noticed that the
aria-invalid
anddata-invalid
attributes are tied to theinvalid
prop & won't be updated if an invalid entry is added. I opened an issue here: #4294Changelog
Changed
errorId
intoerror
message conditional logic & make itnull
by defaultTesting / Reviewing
NumberInput
story here: https://deploy-preview-4295--carbon-components-react.netlify.com/input
and confirm that there is noaria-labelledby
attributeinput
again to confirm that this is now anaria-labelledby
attribute & it correctly matches theid
of the error message container.