-
Notifications
You must be signed in to change notification settings - Fork 577
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
Changes from all commits
a134660
6d3928a
5517189
87a95d2
dc061f8
154e062
5ebec1d
3d1494d
05bff8b
9c29e5e
a07320c
3b3a0b0
35aefd2
59dbd1b
029c4e9
a35400f
cdbd5eb
5b8a364
f56f96c
74302e6
0d4ca8a
0d8b47f
7b28de0
597010e
7af1e65
2474ca9
762568d
6c3b040
c76b152
61df114
f3d5f67
a676ca6
bd58df1
5c7718b
e62e240
ee4a6bd
b992660
d931bc7
40929ea
62b00f8
cb1c7b4
6912356
e492c51
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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', () => { | ||
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() | ||
}) | ||
}) | ||
} | ||
}) | ||
} | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -410,15 +410,15 @@ export const MultipleSections = () => { | |
</ActionMenu.Anchor> | ||
<ActionMenu.Overlay width="small"> | ||
<ActionList> | ||
<ActionList.Group selectionVariant="multiple"> | ||
<ActionList.Group> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixes: |
||
<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"> | ||
|
@@ -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> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixes: |
||
|
||
const MenuButton = React.forwardRef(({...props}, anchorRef) => { | ||
return ( | ||
<Anchor ref={anchorRef}> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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={ | ||
|
@@ -49,6 +50,7 @@ export const Playground: StoryObj<typeof Banner> = { | |
|
||
<PageLayout.Content> | ||
<Banner | ||
aria-label="Content level banner" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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={ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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} /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cleans up bad |
||
) | ||
|
||
return ( | ||
|
@@ -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> | ||
|
@@ -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) => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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', | ||
|
@@ -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}}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Flagging in this feature cleans up 2 axe violations:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -66,6 +67,6 @@ export function Default(): JSX.Element { | |
onFilterChange={setFilter} | ||
sx={{border: '1px solid', padding: '8px'}} | ||
/> | ||
</> | ||
</FeatureFlags> | ||
) | ||
} |
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,7 @@ export const WithCss = () => ( | |
primer_react_css_modules_ga: true, | ||
}} | ||
> | ||
<Header as="summary" className={classes.HeaderDev}> | ||
<Header className={classes.HeaderDev}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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}} /> | ||
|
@@ -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}} /> | ||
|
@@ -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}} /> | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,11 +16,6 @@ const meta: Meta<typeof UnderlinePanels> = { | |
name: 'string', | ||
}, | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do not need |
||
'aria-labelledby': { | ||
type: { | ||
name: 'string', | ||
}, | ||
}, | ||
id: { | ||
type: { | ||
name: 'string', | ||
|
@@ -34,7 +29,6 @@ const meta: Meta<typeof UnderlinePanels> = { | |
}, | ||
args: { | ||
'aria-label': 'Select a tab', | ||
'aria-labelledby': 'tab', | ||
id: 'test', | ||
loadingCounters: false, | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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', | ||
|
@@ -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'}} /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Elements must only use supported ARIA attributes. Cannot have |
||
const [option, setOption] = useState('Select an option') | ||
const onAction = useCallback((itemProps: ItemProps) => { | ||
setOption(itemProps.text || '') | ||
|
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.
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.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.
My thought was to remove all
@aat
tests after this is merged since we don't need to generate them ourselvesThere 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.
@joshblack do you feel like we need the other @Aat tests 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.
@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.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.
@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.
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.
@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 thedescribe
(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 justdescribe('aat')