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

[Axe] Run axe across all stories #5592

Merged
merged 43 commits into from
Jan 31, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
a134660
Run storybook axe scans on CI
kendallgassner Jan 21, 2025
6d3928a
test
kendallgassner Jan 21, 2025
5517189
clean
kendallgassner Jan 21, 2025
87a95d2
remove console
kendallgassner Jan 21, 2025
dc061f8
rephrase id
kendallgassner Jan 21, 2025
154e062
unused
kendallgassner Jan 21, 2025
5ebec1d
edits
kendallgassner Jan 23, 2025
3d1494d
Merge branch 'main' into kendallg/axe-test-on-ci
kendallgassner Jan 23, 2025
05bff8b
try
kendallgassner Jan 23, 2025
9c29e5e
test
kendallgassner Jan 23, 2025
a07320c
test
kendallgassner Jan 23, 2025
3b3a0b0
store
kendallgassner Jan 23, 2025
35aefd2
store
kendallgassner Jan 23, 2025
59dbd1b
store
kendallgassner Jan 23, 2025
029c4e9
fic
kendallgassner Jan 23, 2025
a35400f
fix vrt
kendallgassner Jan 24, 2025
cdbd5eb
Merge branch 'main' into kendallg/axe-test-on-ci
kendallgassner Jan 27, 2025
5b8a364
fix axe violation
kendallgassner Jan 27, 2025
f56f96c
fix
kendallgassner Jan 27, 2025
74302e6
test
kendallgassner Jan 27, 2025
0d4ca8a
test
kendallgassner Jan 27, 2025
0d8b47f
ActionMenu story fix
kendallgassner Jan 27, 2025
7b28de0
Header axe violations
kendallgassner Jan 27, 2025
597010e
fix aria-label
kendallgassner Jan 27, 2025
7af1e65
Eanable SelectPanel feature flag for FilteredActionList Story
kendallgassner Jan 27, 2025
2474ca9
format + lint + remove cli changes
kendallgassner Jan 27, 2025
762568d
lint
kendallgassner Jan 27, 2025
6c3b040
lint
kendallgassner Jan 27, 2025
c76b152
unique aria-labels on banner
kendallgassner Jan 27, 2025
61df114
replace link with button on deprecated storybook example
kendallgassner Jan 27, 2025
f3d5f67
ts-ignore message
kendallgassner Jan 27, 2025
a676ca6
ts-ignore message
kendallgassner Jan 27, 2025
bd58df1
Merge branch 'main' into kendallg/axe-test-on-ci
kendallgassner Jan 27, 2025
5c7718b
remove parantheses from file names
kendallgassner Jan 27, 2025
e62e240
Potential fix for code scanning alert no. 8: Replacement of a substri…
kendallgassner Jan 27, 2025
ee4a6bd
remove /
kendallgassner Jan 27, 2025
b992660
remove /
kendallgassner Jan 27, 2025
d931bc7
remove space
kendallgassner Jan 28, 2025
40929ea
Update e2e/components/Axe.test.ts
kendallgassner Jan 28, 2025
62b00f8
add test skip
kendallgassner Jan 28, 2025
cb1c7b4
add test skip
kendallgassner Jan 28, 2025
6912356
Merge branch 'main' into kendallg/axe-test-on-ci
kendallgassner Jan 29, 2025
e492c51
add progressbar fix
kendallgassner Jan 29, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions e2e/components/Axe.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import {test, expect} from '@playwright/test'
// @ts-ignore since this file is generated in the ci to generate this file run npx build storybook in packages/react
import componentsConfig from '../../packages/react/storybook-static/index.json'
import {visit} from '../test-helpers/storybook'
import {themes} from '../test-helpers/themes'

/**
* These stories should not be tested in the CI because they are stress-tests and
* perform slowly
*/
const SKIPPED_TESTS = [
'components-treeview-features--stress-test',
'components-treeview-features--contain-intrinsic-size',
'components-flash-features--with-icon-action-dismiss', // TODO: Remove once color-contrast issues have been resolved
'components-flash-features--with-icon-and-action', // TODO: Remove once color-contrast issues have been resolved
]

type Component = {
name: string
}

const {entries} = componentsConfig

test.describe('@aat', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
test.describe('@aat', () => {
test.describe('aat', () => {

I would just annotate this as aat, or remove it, since we're using the @aat tag at the test-level to filter things out currently (although we could totally change this in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thought was to remove all @aat tests after this is merged since we don't need to generate them ourselves

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshblack do you feel like we need the other @Aat tests now?

Copy link
Member

Choose a reason for hiding this comment

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

@kendallgassner I totally bet we could, especially the automated/boilerplate ones.

I think it can still be helpful to have this tag since there are stories where we would like to put the component in a specific state and then run axe, ideally. Like with menus to make sure we test things when it's open and closed or if we want to use the matrix test helper to make sure we run axe against prop combinations of things.

Copy link
Contributor Author

@kendallgassner kendallgassner Jan 28, 2025

Choose a reason for hiding this comment

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

@joshblack If we keep the '@Aat' in this file, doesn't it break up the tests to run concurrently in aat-report. I assumed aat-runner 1-4 were different threads.

Copy link
Member

Choose a reason for hiding this comment

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

@kendallgassner I think we should definitely keep @aat in the file for the reasons you mentioned, just was saying that we could add it at the test-level instead of in the describe (which seems like the case on line 38? let me know if I'm misreading 👀)

If we have that in place then I think we can remove the outer describe block or replace it with just describe('aat')

for (const [id, entry] of Object.entries(entries as Record<string, Component>)) {
if (SKIPPED_TESTS.includes(id)) {
continue
}

const {name} = entry
// remove parentheses and slashes from the name to avoid playwright file issues
// eslint-disable-next-line no-useless-escape
const cleanedName = name.replaceAll(/[\(\)\/]/g, '')

test.describe(id, () => {
for (const theme of themes) {
test.describe(theme, () => {
test(`${cleanedName} @aat`, async ({page}) => {
await visit(page, {
id,
globals: {
colorScheme: theme,
},
})
await expect(page).toHaveNoViolations()
})
})
}
})
}
})
16 changes: 8 additions & 8 deletions packages/react/src/ActionMenu/ActionMenu.examples.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -410,15 +410,15 @@ export const MultipleSections = () => {
</ActionMenu.Anchor>
<ActionMenu.Overlay width="small">
<ActionList>
<ActionList.Group selectionVariant="multiple">
<ActionList.Group>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes: Required ARIA attributes must be provided for Multiple Sections story

<ActionList.GroupHeading>Raw file content</ActionList.GroupHeading>
<ActionList.Item onSelect={() => alert('Workflows clicked')}>Download</ActionList.Item>
<ActionList.Item onClick={() => alert('Workflows clicked')}>Download</ActionList.Item>
<ActionList.Divider />
<ActionList.Item onSelect={() => alert('Workflows clicked')}>Jump to line</ActionList.Item>
<ActionList.Item onSelect={() => alert('Workflows clicked')}>Find in file</ActionList.Item>
<ActionList.Item onClick={() => alert('Workflows clicked')}>Jump to line</ActionList.Item>
<ActionList.Item onClick={() => alert('Workflows clicked')}>Find in file</ActionList.Item>
<ActionList.Divider />
<ActionList.Item onSelect={() => alert('Workflows clicked')}>Copy path</ActionList.Item>
<ActionList.Item onSelect={() => alert('Workflows clicked')}>Copy permalink</ActionList.Item>
<ActionList.Item onClick={() => alert('Workflows clicked')}>Copy path</ActionList.Item>
<ActionList.Item onClick={() => alert('Workflows clicked')}>Copy permalink</ActionList.Item>
</ActionList.Group>
<ActionList.Divider />
<ActionList.Group selectionVariant="multiple">
Expand All @@ -434,9 +434,9 @@ export const MultipleSections = () => {
))}
</ActionList.Group>
<ActionList.Divider />
<ActionList.Group selectionVariant="multiple">
<ActionList.Group>
<ActionList.GroupHeading>View options</ActionList.GroupHeading>
<ActionList.Item onSelect={() => alert('Delete file')} variant="danger">
<ActionList.Item onClick={() => alert('Delete file')} variant="danger">
Delete file
</ActionList.Item>
</ActionList.Group>
Expand Down
5 changes: 2 additions & 3 deletions packages/react/src/ActionMenu/ActionMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -217,9 +217,8 @@ const Anchor = React.forwardRef<HTMLElement, ActionMenuAnchorProps>(({children:
})

/** this component is syntactical sugar 🍭 */
export type ActionMenuButtonProps = Omit<ButtonProps, 'children'> & {
children: React.ReactNode
}
export type ActionMenuButtonProps = ButtonProps
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes: Buttons must have discernible text [button-name]

for components-buttongroup-features--dropdown-split


const MenuButton = React.forwardRef(({...props}, anchorRef) => {
return (
<Anchor ref={anchorRef}>
Expand Down
2 changes: 2 additions & 0 deletions packages/react/src/Banner/Banner.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export const Playground: StoryObj<typeof Banner> = {
<PageLayout>
<PageLayout.Pane divider="line" position="start">
<Banner
aria-label="Pane level banner"
onDismiss={onDismiss ? action('onDismiss') : undefined}
primaryAction={primaryAction ? <Banner.PrimaryAction>{primaryAction}</Banner.PrimaryAction> : null}
secondaryAction={
Expand All @@ -49,6 +50,7 @@ export const Playground: StoryObj<typeof Banner> = {

<PageLayout.Content>
<Banner
aria-label="Content level banner"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unique aria-labels!

onDismiss={onDismiss ? action('onDismiss') : undefined}
primaryAction={primaryAction ? <Banner.PrimaryAction>{primaryAction}</Banner.PrimaryAction> : null}
secondaryAction={
Expand Down
3 changes: 2 additions & 1 deletion packages/react/src/Button/ButtonBase.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ const ButtonBase = forwardRef(
if (
innerRef.current &&
!(innerRef.current instanceof HTMLButtonElement) &&
!((innerRef.current as unknown) instanceof HTMLAnchorElement)
!((innerRef.current as unknown) instanceof HTMLAnchorElement) &&
!((innerRef.current as HTMLElement).tagName === 'SUMMARY')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removes console.warn for components-details--default

) {
// eslint-disable-next-line no-console
console.warn('This component should be an instanceof a semantic button or anchor')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export const InactiveButtonsGroup = () => {
)

const secondaryButton = (
<IconButton aria-label="Secondary Button Aria Label" aria-disabled={true} inactive={true} icon={DashIcon} />
<IconButton aria-label="Secondary Button" aria-disabled={true} inactive={true} icon={DashIcon} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleans up bad aria-label and ensures that either aria-label or aria-labelledby is provided on the IconButton (not both).

)

return (
Expand All @@ -84,7 +84,7 @@ export const InactiveButtonsGroup = () => {

<ActionMenu open={false} onOpenChange={() => {}}>
<ActionMenu.Anchor>
<Tooltip text="this button is inactive" direction="ne" type="label">
<Tooltip text="this button is inactive" direction="ne" type="description">
{secondaryButton}
</Tooltip>
</ActionMenu.Anchor>
Expand All @@ -111,7 +111,7 @@ export const DropdownSplit = () => {
{selectedAction}
</Button>
<ActionMenu>
<ActionMenu.Button icon={TriangleDownIcon}>More options</ActionMenu.Button>
<ActionMenu.Button aria-label="More options" icon={TriangleDownIcon} />
<ActionMenu.Overlay>
<ActionList>
{actions.map((action, index) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {ThemeProvider} from '..'
import {FilteredActionList} from '../FilteredActionList'
import BaseStyles from '../BaseStyles'
import Box from '../Box'
import {FeatureFlags} from '../FeatureFlags'

const meta: Meta = {
title: 'Components/FilteredActionList',
Expand Down Expand Up @@ -57,7 +58,7 @@ export function Default(): JSX.Element {
const filteredItems = items.filter(item => item.text.toLowerCase().startsWith(filter.toLowerCase()))

return (
<>
<FeatureFlags flags={{primer_react_select_panel_with_modern_action_list: true}}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flagging in this feature cleans up 2 axe violations:

  • ARIA input fields must have an accessible name
  • Certain ARIA roles must contain particular children

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the default storybook reflects what's currently GA-ed (with the option to toggle on the feature flag in the storybook toolbar), I don't think we should add this feature flag here.

Maybe we can exclude this violation for now with a note that the fix is behind a feature flag?

<h1>Filtered Action List</h1>
<div>Please select labels that describe your issue:</div>
<FilteredActionList
Expand All @@ -66,6 +67,6 @@ export function Default(): JSX.Element {
onFilterChange={setFilter}
sx={{border: '1px solid', padding: '8px'}}
/>
</>
</FeatureFlags>
)
}
6 changes: 4 additions & 2 deletions packages/react/src/Header/Header.dev.module.css
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
.HeaderDev {
background-color: var(--label-olive-bgColor-active);
color: var(--color-prettylights-syntax-carriage-return-text);
/* stylelint-disable-next-line primer/no-display-colors */
background-color: var(--display-pine-bgColor-emphasis);
Copy link
Contributor Author

@kendallgassner kendallgassner Jan 27, 2025

Choose a reason for hiding this comment

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

Cleans up inaccessible color contrast on dev only header examples.

}

.HeaderDevItem {
padding-left: var(--base-size-24);
}

.HeaderDevLink {
color: var(--color-prettylights-syntax-markup-inserted-text);
color: var(--color-prettylights-syntax-carriage-return-text);
}
6 changes: 3 additions & 3 deletions packages/react/src/Header/Header.dev.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export const WithCss = () => (
primer_react_css_modules_ga: true,
}}
>
<Header as="summary" className={classes.HeaderDev}>
<Header className={classes.HeaderDev}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cannot have nested interactive elements.

<Header.Item id="github">
<Header.Link href="#" className={classes.HeaderDevLink}>
<Octicon icon={MarkGithubIcon} size={32} sx={{mr: 2}} />
Expand All @@ -38,7 +38,7 @@ export const WithCss = () => (
)

export const WithSx = () => (
<Header as="summary" sx={{backgroundColor: 'blue'}}>
<Header sx={{backgroundColor: 'blue', color: 'white'}}>
<Header.Item id="github">
<Header.Link href="#" sx={{fontSize: 3}}>
<Octicon icon={MarkGithubIcon} size={32} sx={{mr: 2}} />
Expand All @@ -60,7 +60,7 @@ export const WithSxAndCSS = () => (
primer_react_css_modules_ga: true,
}}
>
<Header as="summary" className={classes.HeaderDev} sx={{backgroundColor: 'orange'}}>
<Header className={classes.HeaderDev} sx={{backgroundColor: 'orange', color: 'black'}}>
<Header.Item id="github">
<Header.Link href="#" className={classes.HeaderDevLink} sx={{p: 0, color: 'black'}}>
<Octicon icon={MarkGithubIcon} size={32} sx={{mr: 2}} />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ export const Inline = () => <ProgressBar inline progress="66" sx={{width: '100px
export const Color = () => <ProgressBar progress="66" bg="done.emphasis" aria-label="Upload test.png" />

export const MultipleItems = () => (
<ProgressBar aria-valuenow={70} aria-label="Upload test.png">
<ProgressBar.Item progress={33} sx={{bg: 'accent.emphasis'}} />
<ProgressBar.Item progress={23} bg={'danger.emphasis'} />
<ProgressBar.Item progress={14} bg={'severe.emphasis'} />
<ProgressBar>
<ProgressBar.Item progress={33} aria-label="Photo Usage" sx={{bg: 'accent.emphasis'}} />
<ProgressBar.Item progress={23} aria-label="Application Usage" bg={'danger.emphasis'} />
<ProgressBar.Item progress={14} aria-label="Music Usage" bg={'severe.emphasis'} />
</ProgressBar>
)

Expand Down
17 changes: 0 additions & 17 deletions packages/react/src/UnderlineNav/UnderlineNav.dev.stories.tsx

This file was deleted.

1 change: 0 additions & 1 deletion packages/react/src/__tests__/ActionMenu.types.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import {ActionMenu} from '..'
import React from 'react'

export function actionButtonWithoutProps() {
// @ts-expect-error requires children
return <ActionMenu.Button />
}

Expand Down
9 changes: 8 additions & 1 deletion packages/react/src/__tests__/storybook.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import fs from 'node:fs'
import path from 'node:path'

const ROOT_DIRECTORY = path.resolve(__dirname, '..', '..')

// Components opted into the new story format
// TODO: Remove this allowlist when all components use the new story format
const allowlist = [
Expand Down Expand Up @@ -56,6 +57,7 @@ const allowlist = [
'Token',
'UnderlineNav2',
]

const stories = glob
.sync('src/**/*.stories.tsx', {
cwd: ROOT_DIRECTORY,
Expand All @@ -72,7 +74,12 @@ const stories = glob
const type = path.basename(filepath, '.stories.tsx').endsWith('features') ? 'feature' : 'default'
const name = type === 'feature' ? path.basename(file, '.features.stories.tsx') : path.basename(file, '.stories.tsx')

return {name, story: require(filepath), type, relativeFilepath: path.relative(ROOT_DIRECTORY, filepath)}
return {
name,
story: require(filepath),
type,
relativeFilepath: path.relative(ROOT_DIRECTORY, filepath),
}
})

const components = Object.entries(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,6 @@ const meta: Meta<typeof UnderlinePanels> = {
name: 'string',
},
},
Copy link
Contributor Author

@kendallgassner kendallgassner Jan 28, 2025

Choose a reason for hiding this comment

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

We do not need aria-labelledby and aria-label controls. A component should not have both.

'aria-labelledby': {
type: {
name: 'string',
},
},
id: {
type: {
name: 'string',
Expand All @@ -34,7 +29,6 @@ const meta: Meta<typeof UnderlinePanels> = {
},
args: {
'aria-label': 'Select a tab',
'aria-labelledby': 'tab',
id: 'test',
loadingCounters: false,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,11 @@ import type {Meta} from '@storybook/react'
import React, {useCallback, useState, useRef} from 'react'
import styled from 'styled-components'
import {ThemeProvider} from '../..'
import type {LinkProps} from '../../Link'
import Link from '../../Link'
import type {ActionMenuProps} from '../../deprecated'
import {ActionMenu, ActionList} from '../../deprecated'
import type {ItemProps} from '../../deprecated/ActionList'
import BaseStyles from '../../BaseStyles'
import {Button} from '../../Button'
import {Button, type ButtonProps} from '../../Button'

const meta: Meta = {
title: 'Deprecated/Components/ActionMenu',
Expand Down Expand Up @@ -233,7 +231,7 @@ export function ComplexListStory(): JSX.Element {
ComplexListStory.storyName = 'Complex List'

export function CustomTrigger(): JSX.Element {
const customAnchor = (props: LinkProps) => <Link {...props} sx={{cursor: 'pointer'}} />
const customAnchor = (props: ButtonProps) => <Button {...props} sx={{cursor: 'pointer'}} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Elements must only use supported ARIA attributes. Cannot have aria-expanded on link.

const [option, setOption] = useState('Select an option')
const onAction = useCallback((itemProps: ItemProps) => {
setOption(itemProps.text || '')
Expand Down
Loading