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

Integrate axe-playwright into storybook/test-runner #828

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

angelikatyborska
Copy link
Contributor

@angelikatyborska angelikatyborska commented Aug 19, 2024

Resolves #414

This PR adds a test-storybook command that runs axe-core on all of our stories, including mdx stories. It runs this command on CI.

I fixed every single accessibility violation except for color contrasts because that requires design skills. I have disabled color contrasts checks for now so we can merge this PR.

All code changes in this PR apply to storybook only. No changes are made to the CSS produced by the library.

@angelikatyborska angelikatyborska force-pushed the feature/414-can-we-lint-a11y-issues-on-ci branch 2 times, most recently from 417e5d8 to 646829d Compare August 19, 2024 10:27
@angelikatyborska angelikatyborska force-pushed the feature/414-can-we-lint-a11y-issues-on-ci branch from 646829d to 6ce0603 Compare August 19, 2024 10:32
@angelikatyborska angelikatyborska changed the title Integrate axe-core/playwright into storybook/test-runner Integrate axe-playwright into storybook/test-runner Aug 19, 2024
@@ -111,8 +111,21 @@ DefaultOutline.parameters = {
],
};

const baseTabArgs = {
colorVariant: ['tab'],
role: 'tab',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

aria-selected is not allowed on normal buttons, so a tab role must be added. And then the button needs to we wrapped in a tablist because tabs must be children of tablists only, they cannot stand alone.

@@ -91,7 +91,7 @@ breadCrumbsMenu.innerHTML = `
</svg>
</li>`;

export const header = document.createElement('header');
export const header = document.createElement('div');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This component is already wrapped in a header landmark, so it cannot have another header landmark inside of it.

@@ -86,7 +86,6 @@ const menu = `
}).outerHTML
}
</li>
<li role="separator"></li>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #829

@@ -47,9 +44,12 @@ const labels = [
'Sales',
];

labels.forEach((label) =>
buttonList.appendChild(listItem.appendChild(buttomComponent(label)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This cause appendChild returns the child, not the parent, so this code was resulting in buttons being appended directly to the list, and the list item was unused.

Comment on lines +25 to +28
// TODO: enable this rule again once all color contrast issues are fixed
'color-contrast': {
enabled: false,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be enbled when working on #814

<dl class="u-grid u-grid-cols-3@m u-gap-m">
<div class="u-grid u-grid-cols-3@m u-gap-m">
Copy link
Contributor Author

@angelikatyborska angelikatyborska Aug 19, 2024

Choose a reason for hiding this comment

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

It is not possible to write a valid definition list that has this layout. There can't be a double-nested div in dl, so it's not possible to put a badge in there. It is very limited which children are valid in a dl.

This is valid:

<dl>
	<dt></dt>
	<dd></dd>
</dl>
<dl>
	<div>
		<dt></dt>
		<dd></dd>
	</div>
</dl>

This is not valid

<dl>
	<div>
		<div>
			<dt></dt>
			<dd></dd>
		</div>
	</div>
</dl>
<dl>
	<div>
		<div>
		</div>
	</div>
	<dt></dt>
	<dd></dd>
</dl>

</li>
</ul>
<div class="u-list-none u-flex u-flex-wrap u-items-end a-button--tab-container u-margin-m-bottom" role="tablist" aria-label="Patient data">
<button class="a-button a-button--tab u-margin-s2-right" role="tab" aria-selected="true" aria-controls="tab-panel-1" id="tab-1" tabindex="0">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Buttons with role tab should be direct children of a tablist

@@ -13,20 +13,23 @@ By default `hidden`, `auto`, and `scroll` values are available, at all direction
{`
<div class="u-overflow-hidden u-bg-brand-2" style="height: 5rem">
u-overflow-x-auto Jelly cake marshmallow. Pie cotton candy chupa chups marzipan liquorice cheesecake wafer. Gummies cheesecake oat cake sugar plum icing cupcake tiramisu bonbon. Cotton candy chupa chups tootsie roll chupa chups cotton candy liquorice jelly gingerbread. Pastry gummi bears liquorice tart cotton candy marshmallow. Ice cream cotton candy chocolate cake cookie. Bonbon candy jelly-o sugar plum cotton candy carrot cake icing ice cream. Sweet chocolate marzipan. Candy canes danish cake carrot cake bonbon. Gummi bears sesame snaps tart bear claw pie chocolate bar. Ice cream candy canes sugar plum cookie. Macaroon biscuit biscuit carrot cake liquorice. Muffin pudding gingerbread powder jelly-o chocolate bar powder jelly beans toffee.
<a href="https://duckduckgo.com">DDG</a>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There must be a focusable element in an overflowing container

@angelikatyborska angelikatyborska marked this pull request as ready for review August 19, 2024 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can we lint a11y issues on CI?
1 participant