From 33b82448fef843007f04b04fb464f60456c327e9 Mon Sep 17 00:00:00 2001 From: Lisa Smith Date: Thu, 19 Dec 2024 16:33:05 +0000 Subject: [PATCH] feat(a11y): require Tree aria-label or aria-labelledby --- .../SpaceTreeNode/SpaceTreeNode.stories.tsx | 6 +- .../SpaceTreeNode/SpaceTreeNode.unit.test.tsx | 6 +- .../SpaceTreeNode.unit.test.tsx.snap | 40 +++++ src/components/Tree/Tree.stories.tsx | 4 + src/components/Tree/Tree.test.tsx | 32 +++- src/components/Tree/Tree.test.tsx.snap | 12 ++ src/components/Tree/Tree.tsx | 5 + src/components/Tree/Tree.types.ts | 164 +++++++++--------- src/components/Tree/Tree.typetest.ts | 22 +++ .../TreeNodeBase/TreeNodeBase.stories.tsx | 2 +- .../TreeNodeBase/TreeNodeBase.test.tsx | 26 ++- .../TreeNodeBase/TreeNodeBase.test.tsx.snap | 6 + 12 files changed, 227 insertions(+), 98 deletions(-) create mode 100644 src/components/Tree/Tree.typetest.ts diff --git a/src/components/SpaceTreeNode/SpaceTreeNode.stories.tsx b/src/components/SpaceTreeNode/SpaceTreeNode.stories.tsx index ccd67ac8c..22c795f67 100644 --- a/src/components/SpaceTreeNode/SpaceTreeNode.stories.tsx +++ b/src/components/SpaceTreeNode/SpaceTreeNode.stories.tsx @@ -30,7 +30,11 @@ export default { }; const TreeWrapper = (Story) => ( - + ); diff --git a/src/components/SpaceTreeNode/SpaceTreeNode.unit.test.tsx b/src/components/SpaceTreeNode/SpaceTreeNode.unit.test.tsx index e4691ce96..12531d7b0 100644 --- a/src/components/SpaceTreeNode/SpaceTreeNode.unit.test.tsx +++ b/src/components/SpaceTreeNode/SpaceTreeNode.unit.test.tsx @@ -13,7 +13,11 @@ import Tree from '../Tree'; describe('', () => { const mount = async (component) => { return mountAndWait( - + {component} ); diff --git a/src/components/SpaceTreeNode/SpaceTreeNode.unit.test.tsx.snap b/src/components/SpaceTreeNode/SpaceTreeNode.unit.test.tsx.snap index 0d396ffad..8ebccd29e 100644 --- a/src/components/SpaceTreeNode/SpaceTreeNode.unit.test.tsx.snap +++ b/src/components/SpaceTreeNode/SpaceTreeNode.unit.test.tsx.snap @@ -2,6 +2,7 @@ exports[` snapshot checks divider dot position in compact mode 1`] = ` snapshot checks divider dot position in compact mode } >
snapshot checks divider dot position in compact mode exports[` snapshot should match snapshot 1`] = ` snapshot should match snapshot 1`] = ` } >
snapshot should match snapshot 1`] = ` exports[` snapshot should match snapshot with action 1`] = ` snapshot should match snapshot with action 1`] = ` } >
snapshot should match snapshot with action 1`] = ` exports[` snapshot should match snapshot with className 1`] = ` snapshot should match snapshot with className 1`] = ` } >
snapshot should match snapshot with className 1`] = ` exports[` snapshot should match snapshot with firstLine 1`] = ` snapshot should match snapshot with firstLine 1`] = ` } >
snapshot should match snapshot with firstLine 1`] = ` exports[` snapshot should match snapshot with id 1`] = ` snapshot should match snapshot with id 1`] = ` } >
snapshot should match snapshot with id 1`] = ` exports[` snapshot should match snapshot with isAlert 1`] = ` snapshot should match snapshot with isAlert 1`] = ` } >
snapshot should match snapshot with isAlert 1`] = ` exports[` snapshot should match snapshot with isAlertMuted 1`] = ` snapshot should match snapshot with isAlertMuted 1`] } >
snapshot should match snapshot with isAlertMuted 1`] exports[` snapshot should match snapshot with isDisabled 1`] = ` snapshot should match snapshot with isDisabled 1`] = } >
snapshot should match snapshot with isDisabled 1`] = exports[` snapshot should match snapshot with isEnterRoom 1`] = ` snapshot should match snapshot with isEnterRoom 1`] = } >
snapshot should match snapshot with isEnterRoom 1`] = exports[` snapshot should match snapshot with isError 1`] = ` snapshot should match snapshot with isError 1`] = ` } >
snapshot should match snapshot with isError 1`] = ` exports[` snapshot should match snapshot with isMention 1`] = ` snapshot should match snapshot with isMention 1`] = ` } >
snapshot should match snapshot with isMention 1`] = ` exports[` snapshot should match snapshot with isNewActivity 1`] = ` snapshot should match snapshot with isNewActivity 1`] } >
snapshot should match snapshot with isNewActivity 1`] exports[` snapshot should match snapshot with isSelected 1`] = ` snapshot should match snapshot with isSelected 1`] = } >
snapshot should match snapshot with isSelected 1`] = exports[` snapshot should match snapshot with isSelected and draft 1`] = ` snapshot should match snapshot with isSelected and dr } >
snapshot should match snapshot with isSelected and dr exports[` snapshot should match snapshot with isUnread 1`] = ` snapshot should match snapshot with isUnread 1`] = ` } >
snapshot should match snapshot with isUnread 1`] = ` exports[` snapshot should match snapshot with multiple string secondLine 1`] = ` snapshot should match snapshot with multiple string s } >
snapshot should match snapshot with multiple string s exports[` snapshot should match snapshot with secondLine 1`] = ` snapshot should match snapshot with secondLine 1`] = } >
snapshot should match snapshot with secondLine 1`] = exports[` snapshot should match snapshot with style 1`] = ` snapshot should match snapshot with style 1`] = ` } >
snapshot should match snapshot with style 1`] = ` exports[` snapshot should match snapshot with teamColor 1`] = ` snapshot should match snapshot with teamColor 1`] = ` } >
( )), + 'aria-label': 'some tree', }; const WithRoot = Template>(Tree).bind({}); @@ -137,6 +138,7 @@ WithRoot.args = { (node) => , { excludeRootNode: false } ), + 'aria-label': 'some tree', }; const TreeWithScroll = Template>(Tree).bind({}); @@ -153,6 +155,7 @@ TreeWithScroll.args = { (node) => , { excludeRootNode: false } ), + 'aria-label': 'some tree', }; const DynamicTree = Template(() => { @@ -180,6 +183,7 @@ const DynamicTree = Template(() => { isRenderedFlat={true} shouldNodeFocusBeInset={true} excludeTreeRoot={true} + aria-label="some tree" > {mapTree( mappedTree, diff --git a/src/components/Tree/Tree.test.tsx b/src/components/Tree/Tree.test.tsx index 4b85fbda5..e05730c18 100644 --- a/src/components/Tree/Tree.test.tsx +++ b/src/components/Tree/Tree.test.tsx @@ -3,6 +3,7 @@ import { mount } from 'enzyme'; import '@testing-library/jest-dom'; import { render, waitFor, waitForElementToBeRemoved } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; +import { omit } from 'lodash'; import Tree, { TREE_CONSTANTS, TreeProps } from './index'; import { createTreeNode as tNode } from './test.utils'; @@ -36,6 +37,7 @@ const getSampleTree = () => { describe('', () => { const commonProps = { treeStructure: tNode('root', true, [tNode('1'), tNode('2')]), + 'aria-label': 'some tree', }; afterAll(() => { @@ -162,7 +164,7 @@ describe('', () => { expect.assertions(1); const label = 'test'; - const container = mount(); + const container = mount(); const element = container.find(Tree).getDOMNode(); expect(element.getAttribute('aria-label')).toBe('test'); @@ -172,7 +174,9 @@ describe('', () => { expect.assertions(1); const labelBy = 'label-id'; - const container = mount(); + const container = mount( + + ); const element = container.find(Tree).getDOMNode(); expect(element.getAttribute('aria-labelledby')).toBe(labelBy); @@ -200,7 +204,7 @@ describe('', () => { it.each(['ArrowUp', 'ArrowDown', 'ArrowLeft', 'ArrowRight'])( 'should do nothing when tree structure is empty and user presses %s', async (key) => { - render(); + render(); const focusedElement = document.activeElement; await userEvent.keyboard(`{${key}}`); @@ -215,7 +219,7 @@ describe('', () => { const { getByRole } = render( // eslint-disable-next-line jsx-a11y/no-static-element-interactions
- + {() => 'TreeNodeBase 1'} @@ -238,7 +242,7 @@ describe('', () => { const tree = getSampleTree(); const { getByTestId } = render( - + {mapTree( convertNestedTree2MappedTree(tree), (node) => ( @@ -316,6 +320,7 @@ describe('', () => { scrollToNode, }} onToggleNode={onToggleNode} + aria-label="some tree" > {mapTree( convertNestedTree2MappedTree(tree), @@ -385,7 +390,11 @@ describe('', () => { const user = userEvent.setup(); const { getByTestId, getByText } = render( - + {() => ( <> @@ -422,6 +431,7 @@ describe('', () => { treeStructure={tree} excludeTreeRoot={false} virtualTreeConnector={virtualTreeConnector} + aria-label="some tree" > {mapTree( convertNestedTree2MappedTree(tree), @@ -668,7 +678,7 @@ describe('', () => { describe('dynamically changing tree', () => { const getTreeComponent = (tree, excludeTreeRoot = false) => { return ( - + {mapTree( convertNestedTree2MappedTree(tree), (node) => @@ -883,7 +893,13 @@ describe('', () => { describe('selection', () => { const getTreeComponent = (tree, { ref, ...props }: Partial = {}) => { return ( - + {mapTree( convertNestedTree2MappedTree(tree), (node) => ( diff --git a/src/components/Tree/Tree.test.tsx.snap b/src/components/Tree/Tree.test.tsx.snap index 415a22746..78a79b5c8 100644 --- a/src/components/Tree/Tree.test.tsx.snap +++ b/src/components/Tree/Tree.test.tsx.snap @@ -2,6 +2,7 @@ exports[` snapshot should match snapshot 1`] = ` snapshot should match snapshot 1`] = ` } >
snapshot should match snapshot 1`] = ` exports[` snapshot should match snapshot with className 1`] = ` snapshot should match snapshot with className 1`] = ` } >
snapshot should match snapshot with className 1`] = ` exports[` snapshot should match snapshot with id 1`] = ` snapshot should match snapshot with id 1`] = ` } >
snapshot should match snapshot with id 1`] = ` exports[` snapshot should match snapshot with style 1`] = ` snapshot should match snapshot with style 1`] = ` } >
snapshot should match snapshot with style 1`] = ` exports[` snapshot should match snapshot with style 2`] = ` snapshot should match snapshot with style 2`] = ` } >
snapshot should match snapshot with style 2`] = ` exports[` snapshot should match snapshot with style 3`] = ` snapshot should match snapshot with style 3`] = ` } >
) => { ...rest } = props; + // aria-label or aria-labelledby is required on the Props type, but union type enforcement doesn't work properly with forwardRef + if (!rest['aria-label'] && !rest['aria-labelledby']) { + console.warn('MRV2: Tree must have aria-label or aria-labelledby'); + } + const treeRef = useRef(); const itemSelection = useItemSelected({ diff --git a/src/components/Tree/Tree.types.ts b/src/components/Tree/Tree.types.ts index 234d17d16..591ca5e3a 100644 --- a/src/components/Tree/Tree.types.ts +++ b/src/components/Tree/Tree.types.ts @@ -6,6 +6,7 @@ import { ReactNode, } from 'react'; import { ItemSelection, UseItemSelectedProps } from '../../hooks/useItemSelected'; +import { AriaLabelRequired } from '../../utils/a11y'; /** * The key codes used to navigate the tree. @@ -97,107 +98,108 @@ export type TreeIdNodeMap = Map; /** * The props of the Tree component. + * aria-label or aria-labelledby is required, but union type enforcement doesn't work properly with forwardRef */ -export interface Props - extends Partial>, - DetailedHTMLProps, HTMLDivElement> { - /** - * Custom class for overriding this component's CSS. - */ - className?: string; - - /** - * Custom id for overriding this component's CSS. - */ - id?: string; +export type Props = Partial> & + DetailedHTMLProps, HTMLDivElement> & + AriaLabelRequired & { + /** + * Custom class for overriding this component's CSS. + */ + className?: string; - /** - * Child components of this component. - */ - children?: ReactNode; + /** + * Custom id for overriding this component's CSS. + */ + id?: string; - /** - * Custom style for overriding this component's CSS. - */ - style?: CSSProperties; + /** + * Child components of this component. + */ + children?: ReactNode; - /** - * Determines whether the focus ring around tree nodes should be inset or outset - * It has only visual effect. - */ - shouldNodeFocusBeInset?: boolean; + /** + * Custom style for overriding this component's CSS. + */ + style?: CSSProperties; - /** - * The initial tree structure - * - * It is used to build an internal tree for navigation and follow open/close states. - * - * The tree can be updated dynamically via `treeStructure`, but `isOpen` will be migrated from the old tree: - * If the node exists the both old and new tree then the value used from the old tree otherwise - * falls back to the `isOpenByDefault ?? true` - */ - treeStructure: TreeRoot; + /** + * Determines whether the focus ring around tree nodes should be inset or outset + * It has only visual effect. + */ + shouldNodeFocusBeInset?: boolean; - /** - * Tree structure can be rendered 2 ways in the DOM: - * 1) Nested: The tree is rendered as a nested list where the structure reflect the semantic structure of the tree - * 2) Flat: The tree is rendered as a single level list where the DOM does not reflect the semantic structure of the tree, and - * we need to provide additional aria attributes to re-build it for the accessibility tree. - * Virtualized trees are usually rendered flat. - * @default true - */ - isRenderedFlat?: boolean; + /** + * The initial tree structure + * + * It is used to build an internal tree for navigation and follow open/close states. + * + * The tree can be updated dynamically via `treeStructure`, but `isOpen` will be migrated from the old tree: + * If the node exists the both old and new tree then the value used from the old tree otherwise + * falls back to the `isOpenByDefault ?? true` + */ + treeStructure: TreeRoot; - /** - * Determines if the tree root should be excluded from the tree keyboard navigation. - * - * In many case we want to hide the root of the tree, for example when the tree used for grouping some list items. - * - * Note: it does not change the visibility of the root node. - * @default true - */ - excludeTreeRoot?: boolean; + /** + * Tree structure can be rendered 2 ways in the DOM: + * 1) Nested: The tree is rendered as a nested list where the structure reflect the semantic structure of the tree + * 2) Flat: The tree is rendered as a single level list where the DOM does not reflect the semantic structure of the tree, and + * we need to provide additional aria attributes to re-build it for the accessibility tree. + * Virtualized trees are usually rendered flat. + * @default true + */ + isRenderedFlat?: boolean; - /** - * The selection mode of the tree nodes. - * - * Note: When user click to select a node and `selectableNodes` is `any` and the node is not a leaf node, the node will be opened/closed. - * WCAG Tree patter sample implementation has the same behavior. - * - * @see {@link https://www.w3.org/WAI/ARIA/apg/patterns/treeview/ WCAG Tree Pattern} - */ - selectableNodes?: 'leafOnly' | 'any'; + /** + * Determines if the tree root should be excluded from the tree keyboard navigation. + * + * In many case we want to hide the root of the tree, for example when the tree used for grouping some list items. + * + * Note: it does not change the visibility of the root node. + * @default true + */ + excludeTreeRoot?: boolean; - /** - * Set of functions to communicate with virtualized tree and sync states. - */ - virtualTreeConnector?: { /** - * External function to scroll to a node. - * This is used when the tree is rendered in a virtualized tree. + * The selection mode of the tree nodes. * - * @param id + * Note: When user click to select a node and `selectableNodes` is `any` and the node is not a leaf node, the node will be opened/closed. + * WCAG Tree patter sample implementation has the same behavior. + * + * @see {@link https://www.w3.org/WAI/ARIA/apg/patterns/treeview/ WCAG Tree Pattern} */ - scrollToNode: (id: TreeNodeId) => void; + selectableNodes?: 'leafOnly' | 'any'; /** - * Toggle open/close state of the tree node. + * Set of functions to communicate with virtualized tree and sync states. + */ + virtualTreeConnector?: { + /** + * External function to scroll to a node. + * This is used when the tree is rendered in a virtualized tree. + * + * @param id + */ + scrollToNode: (id: TreeNodeId) => void; + + /** + * Toggle open/close state of the tree node. + * + * @param id + * @param isOpen + */ + setNodeOpen?: (id: TreeNodeId, isOpen: boolean) => void | Promise; + }; + + /** + * Called when a node's open / close state is toggled. * * @param id * @param isOpen */ - setNodeOpen?: (id: TreeNodeId, isOpen: boolean) => void | Promise; + onToggleNode?: (id: TreeNodeId, isOpen: boolean) => void; }; - /** - * Called when a node's open / close state is toggled. - * - * @param id - * @param isOpen - */ - onToggleNode?: (id: TreeNodeId, isOpen: boolean) => void; -} - /** * Props of the virtualized tree hook * @internal diff --git a/src/components/Tree/Tree.typetest.ts b/src/components/Tree/Tree.typetest.ts new file mode 100644 index 000000000..61201afdf --- /dev/null +++ b/src/components/Tree/Tree.typetest.ts @@ -0,0 +1,22 @@ +import { Expect, ExpectExtends, ExpectFalse } from '../../utils/typetest.util'; +import { Props, TreeRoot } from './Tree.types'; + +// eslint-disable-next-line @typescript-eslint/no-unused-vars +type cases = [ + Expect>, + Expect>, + Expect< + ExpectExtends< + Props, + { treeStructure: TreeRoot; 'aria-label': 'abc'; 'aria-labelledby': 'some-id' } + > + >, + + ExpectFalse>, + ExpectFalse>, + ExpectFalse>, + + ExpectFalse>, + + ExpectFalse>> +]; diff --git a/src/components/TreeNodeBase/TreeNodeBase.stories.tsx b/src/components/TreeNodeBase/TreeNodeBase.stories.tsx index e753595ee..16826ab26 100644 --- a/src/components/TreeNodeBase/TreeNodeBase.stories.tsx +++ b/src/components/TreeNodeBase/TreeNodeBase.stories.tsx @@ -36,7 +36,7 @@ export default { }; const TreeWrapper = ({ children }) => ( - + {children} ); diff --git a/src/components/TreeNodeBase/TreeNodeBase.test.tsx b/src/components/TreeNodeBase/TreeNodeBase.test.tsx index 3b0d4374c..2dab024e2 100644 --- a/src/components/TreeNodeBase/TreeNodeBase.test.tsx +++ b/src/components/TreeNodeBase/TreeNodeBase.test.tsx @@ -195,7 +195,12 @@ describe('TreeNodeBase', () => { const tree: TreeNode = tNode('root', true, [tNode('1'), tNode('2')]); container = mount( - + {mapTree( convertNestedTree2MappedTree(tree), (node) => ( @@ -255,7 +260,12 @@ describe('TreeNodeBase', () => { const tree: TreeNode = tNode('root', true, [tNode('1'), tNode('2')]); container = mount( - + {mapTree( convertNestedTree2MappedTree(tree), (node) => ( @@ -285,7 +295,7 @@ describe('TreeNodeBase', () => { const tree: TreeNode = tNode('root', false, [tNode('1'), tNode('2')]); // prettier-ignore container = mount( - + {mapTree( convertNestedTree2MappedTree(tree), (node) => ( @@ -310,7 +320,7 @@ describe('TreeNodeBase', () => { const tree: TreeNode = tNode('root', false, [tNode('1'), tNode('2')]); // prettier-ignore container = mount( - + {mapTree( convertNestedTree2MappedTree(tree), (node) => ( @@ -547,7 +557,7 @@ describe('TreeNodeBase', () => { const tree = getSampleTree(); // prettier-ignore return render( - + {mapTree(convertNestedTree2MappedTree(tree), (node) => ( {() =>( @@ -609,7 +619,11 @@ describe('TreeNodeBase', () => { const user = userEvent.setup(); render( - + ); diff --git a/src/components/TreeNodeBase/TreeNodeBase.test.tsx.snap b/src/components/TreeNodeBase/TreeNodeBase.test.tsx.snap index eabf29fb7..a9ce95694 100644 --- a/src/components/TreeNodeBase/TreeNodeBase.test.tsx.snap +++ b/src/components/TreeNodeBase/TreeNodeBase.test.tsx.snap @@ -362,6 +362,7 @@ exports[`TreeNodeBase snapshot should match snapshot without tree context 1`] = exports[`TreeNodeBase snapshot should not render the content of the hidden nodes 1`] = `