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

Storybook a11y tests with Storybook Test Runner #383

Merged
merged 4 commits into from
Jun 5, 2024

Conversation

ty2k
Copy link
Contributor

@ty2k ty2k commented Jun 5, 2024

This PR adds testing using Storybook Test Runner and axe-playwright to the React component library. See Storybook's Accessibility tests page for background information.

This new test run requires a running Storybook instance. With Storybook running locally at localhost:6006, run npm run storybook-test to see its output.

Unlike in #368, I am unable to get the Select component to throw the error "ARIA hidden element must not be focusable or contain focusable elements." I'm unsure why this is so far. I've tried adding a manual delay to the postVisit step to see if it has something to do with the test runner hitting the page before the hidden element is rendered, and this didn't have any effect. More exploration required.

Two genuine errors are being surfaced by this test suite: The stories Button/Icon and Button/PlaceholderIconButton are both failing "Ensures buttons have discernible text". These will get addressed in a future PR.

Test result screenshot

For now, I'm not adding the test run to any GitHub Actions workflow since the tests are failing.

@ty2k ty2k added the enhancement New feature or request label Jun 5, 2024
@ty2k ty2k added this to the Components library v0.1.0 release milestone Jun 5, 2024
@ty2k ty2k requested a review from mkernohanbc June 5, 2024 00:15
@ty2k ty2k self-assigned this Jun 5, 2024
@mkernohanbc
Copy link
Contributor

Two genuine errors are being surfaced by this test suite: The stories Button/Icon and Button/PlaceholderIconButton are both failing "Ensures buttons have discernible text". These will get addressed in a future PR.

This is because there's no aria-label or aria-labelledby prop, right? In the docs, we flag that if users are using icon-only buttons, they need to pass one of those labels to make them accessible. Is the fix simply to add placeholder aria-labels to those stories, or are you contemplating a change to the component itself to bake in that label in those use-cases?

@ty2k
Copy link
Contributor Author

ty2k commented Jun 5, 2024

Two genuine errors are being surfaced by this test suite: The stories Button/Icon and Button/PlaceholderIconButton are both failing "Ensures buttons have discernible text". These will get addressed in a future PR.

This is because there's no aria-label or aria-labelledby prop, right? In the docs, we flag that if users are using icon-only buttons, they need to pass one of those labels to make them accessible. Is the fix simply to add placeholder aria-labels to those stories, or are you contemplating a change to the component itself to bake in that label in those use-cases?

Right, there is no discernible text from the button contents, aria-label, aria-labelledby, or any field within the SVG that would suffice.

I think adding an aria-label prop to those stories is the simplest, with explanatory text in the docs.

I don't think we can do anything further to bake more logic into the component due to the nature of the problem. Check out the article Accessible Icon Buttons for background info. IMO, there's no perfect way to satisfy this rule in all cases using logic in the component, and we can't control what folks will be using in the children slot, so the best we can do is educate them.

@ty2k ty2k merged commit dc0410c into main Jun 5, 2024
1 check passed
@ty2k ty2k deleted the test/storybook-a11y-tests branch June 5, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants