Skip to content
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

Merged
merged 3 commits into from
Oct 10, 2019
Merged

fix(number-input): update value of errorId for aria-labelledby on input #4295

merged 3 commits into from
Oct 10, 2019

Conversation

jendowns
Copy link
Contributor

@jendowns jendowns commented Oct 10, 2019

Closes #4198

In the originating issue, aria-labelledby (which was referring the id 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 the error message, since their values are connected. So if the error exists, so does the errorId -- otherwise both are null.

NOTE: in the process of looking into this, I noticed that the aria-invalid and data-invalid attributes are tied to the invalid prop & won't be updated if an invalid entry is added. I opened an issue here: #4294

Changelog

Changed

  • move errorId into error message conditional logic & make it null by default

Testing / Reviewing

  1. Open the NumberInput story here: https://deploy-preview-4295--carbon-components-react.netlify.com/
  2. Inspect the input and confirm that there is no aria-labelledby attribute
  3. Enter an invalid entry and then check the input again to confirm that this is now an aria-labelledby attribute & it correctly matches the id of the error message container.

@jendowns jendowns requested a review from a team as a code owner October 10, 2019 19:06
@ghost ghost requested review from dakahn and emyarod October 10, 2019 19:06
@jendowns jendowns changed the title fix(number-input): update how errorId is applied for aria-labelledby fix(number-input): update value of errorId for aria-labelledby on input Oct 10, 2019
@netlify
Copy link

netlify bot commented Oct 10, 2019

Deploy preview for the-carbon-components ready!

Built with commit c0ea57c

https://deploy-preview-4295--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Oct 10, 2019

Deploy preview for carbon-components-react failed.

Built with commit c0ea57c

https://app.netlify.com/sites/carbon-components-react/deploys/5d9fad3026a9260007fd7e07

@netlify
Copy link

netlify bot commented Oct 10, 2019

Deploy preview for carbon-elements ready!

Built with commit c0ea57c

https://deploy-preview-4295--carbon-elements.netlify.com

Copy link
Contributor

@dakahn dakahn left a 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.

@jendowns
Copy link
Contributor Author

jendowns commented Oct 10, 2019

@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

Copy link
Contributor

@dakahn dakahn left a 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 👍

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @jendowns!

@asudoh asudoh merged commit aa98ed6 into carbon-design-system:master Oct 10, 2019
@snidersd
Copy link

@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)
Screen Shot 2019-10-11 at 2 10 59 PM

@dakahn
Copy link
Contributor

dakahn commented Oct 11, 2019

Just checked again @snidersd using the September 2019 ruleset on Chrome
https://deploy-preview-4295--carbon-components-react.netlify.com/

Using that deploy preview link I don't have that error 😕

@snidersd
Copy link

@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.
Screen Shot 2019-10-11 at 2 23 36 PM

@emyarod
Copy link
Member

emyarod commented Oct 14, 2019

I have the same error ("Accessible name must match or contain the visible label text") when testing with the same ruleset

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AVT 1 - NumberInput DAP violation for ID references
5 participants