From 6a53d535b401699984f59e7f981ec9f4ec99f378 Mon Sep 17 00:00:00 2001 From: David Strack Date: Wed, 20 Dec 2023 13:36:08 -0600 Subject: [PATCH 1/8] include selectionAttribute in ListContext --- src/ActionList/Item.tsx | 5 ++++- src/ActionList/List.tsx | 13 +++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/ActionList/Item.tsx b/src/ActionList/Item.tsx index c2148c9bec7..b6d4bf1418c 100644 --- a/src/ActionList/Item.tsx +++ b/src/ActionList/Item.tsx @@ -74,6 +74,7 @@ export const Item = React.forwardRef( role: listRole, showDividers, selectionVariant: listSelectionVariant, + selectionAttribute: listSelectionAttribute, } = React.useContext(ListContext) const {selectionVariant: groupSelectionVariant} = React.useContext(GroupContext) const {container, afterSelect, selectionAttribute} = React.useContext(ActionListContainerContext) @@ -97,6 +98,8 @@ export const Item = React.forwardRef( ? groupSelectionVariant : listSelectionVariant + const itemSelectionAttribute: ActionListProps['selectionAttribute'] = selectionAttribute || listSelectionAttribute + /** Infer item role based on the container */ let itemRole: ActionListItemProps['role'] if (container === 'ActionMenu') { @@ -244,7 +247,7 @@ export const Item = React.forwardRef( 'aria-describedby': slots.blockDescription ? [blockDescriptionId, inactiveWarningId].join(' ') : inactiveWarningId, - ...(selectionAttribute && {[selectionAttribute]: selected}), + ...(itemSelectionAttribute && {[itemSelectionAttribute]: selected}), role: role || itemRole, id: itemId, } diff --git a/src/ActionList/List.tsx b/src/ActionList/List.tsx index ba2178d3efa..06eff45bfa1 100644 --- a/src/ActionList/List.tsx +++ b/src/ActionList/List.tsx @@ -26,10 +26,15 @@ export type ActionListProps = React.PropsWithChildren<{ * The ARIA role describing the function of `List` component. `listbox` or `menu` are a common values. */ role?: AriaRole + + selectionAttribute?: 'aria-selected' | 'aria-checked' }> & SxProp -type ContextProps = Pick & { +type ContextProps = Pick< + ActionListProps, + 'variant' | 'selectionVariant' | 'showDividers' | 'role' | 'selectionAttribute' +> & { headingId?: string } @@ -58,19 +63,23 @@ export const List = React.forwardRef( const { listRole, listLabelledBy, + selectionAttribute, selectionVariant: containerSelectionVariant, // TODO: Remove after DropdownMenu2 deprecation } = React.useContext(ActionListContainerContext) const ariaLabelledBy = slots.heading ? slots.heading.props.id ?? headingId : listLabelledBy + const listSelectionVariant = selectionVariant || containerSelectionVariant + const defaultSelectionAttribute = listSelectionVariant === 'multiple' ? 'aria-checked' : 'aria-selected' return ( {slots.heading} From 80d1f72f2589d6171388dc6428023553917e3762 Mon Sep 17 00:00:00 2001 From: David Strack Date: Wed, 20 Dec 2023 13:36:55 -0600 Subject: [PATCH 2/8] remove aria-selected from test to assert default ActionList selection behavior --- src/ActionList/ActionList.test.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/ActionList/ActionList.test.tsx b/src/ActionList/ActionList.test.tsx index b3d8c814c41..a6a8a431c7a 100644 --- a/src/ActionList/ActionList.test.tsx +++ b/src/ActionList/ActionList.test.tsx @@ -44,7 +44,6 @@ function SingleSelectListStory(): JSX.Element { key={index} role="option" selected={index === selectedIndex} - aria-selected={index === selectedIndex} onSelect={() => setSelectedIndex(index)} disabled={project.disabled} inactiveText={project.inactiveText} From 6b111f7258ec015b9b9e2ee8c79e488ce80e9df9 Mon Sep 17 00:00:00 2001 From: David Strack Date: Wed, 20 Dec 2023 13:38:23 -0600 Subject: [PATCH 3/8] changeset --- .changeset/rare-jeans-rush.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/rare-jeans-rush.md diff --git a/.changeset/rare-jeans-rush.md b/.changeset/rare-jeans-rush.md new file mode 100644 index 00000000000..2368e160309 --- /dev/null +++ b/.changeset/rare-jeans-rush.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +Fix missing `aria-selected` & `aria-checked` attributes in ActionList items From 39689c11df374c7e0d9c6f491e2d544308691d78 Mon Sep 17 00:00:00 2001 From: David Strack Date: Wed, 20 Dec 2023 14:53:33 -0600 Subject: [PATCH 4/8] clarify variable names --- src/ActionList/Item.tsx | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/ActionList/Item.tsx b/src/ActionList/Item.tsx index b6d4bf1418c..984a5ee53bc 100644 --- a/src/ActionList/Item.tsx +++ b/src/ActionList/Item.tsx @@ -101,15 +101,17 @@ export const Item = React.forwardRef( const itemSelectionAttribute: ActionListProps['selectionAttribute'] = selectionAttribute || listSelectionAttribute /** Infer item role based on the container */ - let itemRole: ActionListItemProps['role'] + let inferredItemRole: ActionListItemProps['role'] if (container === 'ActionMenu') { - if (selectionVariant === 'single') itemRole = 'menuitemradio' - else if (selectionVariant === 'multiple') itemRole = 'menuitemcheckbox' - else itemRole = 'menuitem' + if (selectionVariant === 'single') inferredItemRole = 'menuitemradio' + else if (selectionVariant === 'multiple') inferredItemRole = 'menuitemcheckbox' + else inferredItemRole = 'menuitem' } else if (container === 'SelectPanel' && listRole === 'listbox') { - if (selectionVariant !== undefined) itemRole = 'option' + if (selectionVariant !== undefined) inferredItemRole = 'option' } + const itemRole = role || inferredItemRole + const {theme} = useTheme() const activeStyles = { @@ -248,11 +250,11 @@ export const Item = React.forwardRef( ? [blockDescriptionId, inactiveWarningId].join(' ') : inactiveWarningId, ...(itemSelectionAttribute && {[itemSelectionAttribute]: selected}), - role: role || itemRole, + role: itemRole, id: itemId, } - const containerProps = _PrivateItemWrapper ? {role: role || itemRole ? 'none' : undefined} : menuItemProps + const containerProps = _PrivateItemWrapper ? {role: itemRole ? 'none' : undefined} : menuItemProps const wrapperProps = _PrivateItemWrapper ? menuItemProps : {} From 9feca2915d06ad05e20a3888bdbca80d0fc2e67b Mon Sep 17 00:00:00 2001 From: David Strack Date: Wed, 20 Dec 2023 15:02:53 -0600 Subject: [PATCH 5/8] only apply aria selection attributes to the appropriate roles --- src/ActionList/Item.tsx | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/ActionList/Item.tsx b/src/ActionList/Item.tsx index 984a5ee53bc..a728bcd860f 100644 --- a/src/ActionList/Item.tsx +++ b/src/ActionList/Item.tsx @@ -239,6 +239,10 @@ export const Item = React.forwardRef( const ItemWrapper = _PrivateItemWrapper || React.Fragment + // only apply aria-selected and aria-checked to selectable items + const selectableRoles = ['menuitemradio', 'menuitemcheckbox', 'option'] + const includeSelectionAttribute = itemSelectionAttribute && itemRole && selectableRoles.includes(itemRole) + const menuItemProps = { onClick: clickHandler, onKeyPress: keyPressHandler, @@ -249,7 +253,7 @@ export const Item = React.forwardRef( 'aria-describedby': slots.blockDescription ? [blockDescriptionId, inactiveWarningId].join(' ') : inactiveWarningId, - ...(itemSelectionAttribute && {[itemSelectionAttribute]: selected}), + ...(includeSelectionAttribute && {[itemSelectionAttribute]: selected}), role: itemRole, id: itemId, } From 0f212b1fd27c3ebab24fc7d051cbbf131f1680e6 Mon Sep 17 00:00:00 2001 From: David Strack Date: Wed, 20 Dec 2023 15:51:29 -0600 Subject: [PATCH 6/8] infer selection attribute based on item role --- src/ActionList/Item.tsx | 10 +++++++--- src/ActionList/List.tsx | 10 +--------- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/ActionList/Item.tsx b/src/ActionList/Item.tsx index a728bcd860f..a5736755e67 100644 --- a/src/ActionList/Item.tsx +++ b/src/ActionList/Item.tsx @@ -74,7 +74,6 @@ export const Item = React.forwardRef( role: listRole, showDividers, selectionVariant: listSelectionVariant, - selectionAttribute: listSelectionAttribute, } = React.useContext(ListContext) const {selectionVariant: groupSelectionVariant} = React.useContext(GroupContext) const {container, afterSelect, selectionAttribute} = React.useContext(ActionListContainerContext) @@ -98,8 +97,6 @@ export const Item = React.forwardRef( ? groupSelectionVariant : listSelectionVariant - const itemSelectionAttribute: ActionListProps['selectionAttribute'] = selectionAttribute || listSelectionAttribute - /** Infer item role based on the container */ let inferredItemRole: ActionListItemProps['role'] if (container === 'ActionMenu') { @@ -112,6 +109,13 @@ export const Item = React.forwardRef( const itemRole = role || inferredItemRole + /** Infer the proper selection attribute based on the item's role */ + let inferredSelectionAttribute: 'aria-selected' | 'aria-checked' | undefined + if (itemRole === 'menuitemradio' || itemRole === 'menuitemcheckbox') inferredSelectionAttribute = 'aria-checked' + else if (itemRole === 'option') inferredSelectionAttribute = 'aria-selected' + + const itemSelectionAttribute = selectionAttribute || inferredSelectionAttribute + const {theme} = useTheme() const activeStyles = { diff --git a/src/ActionList/List.tsx b/src/ActionList/List.tsx index 06eff45bfa1..25d9265f900 100644 --- a/src/ActionList/List.tsx +++ b/src/ActionList/List.tsx @@ -26,15 +26,10 @@ export type ActionListProps = React.PropsWithChildren<{ * The ARIA role describing the function of `List` component. `listbox` or `menu` are a common values. */ role?: AriaRole - - selectionAttribute?: 'aria-selected' | 'aria-checked' }> & SxProp -type ContextProps = Pick< - ActionListProps, - 'variant' | 'selectionVariant' | 'showDividers' | 'role' | 'selectionAttribute' -> & { +type ContextProps = Pick & { headingId?: string } @@ -63,13 +58,11 @@ export const List = React.forwardRef( const { listRole, listLabelledBy, - selectionAttribute, selectionVariant: containerSelectionVariant, // TODO: Remove after DropdownMenu2 deprecation } = React.useContext(ActionListContainerContext) const ariaLabelledBy = slots.heading ? slots.heading.props.id ?? headingId : listLabelledBy const listSelectionVariant = selectionVariant || containerSelectionVariant - const defaultSelectionAttribute = listSelectionVariant === 'multiple' ? 'aria-checked' : 'aria-selected' return ( ( showDividers, role: role || listRole, headingId, - selectionAttribute: selectionAttribute || defaultSelectionAttribute, }} > {slots.heading} From afb88dc0e0e5219fe88970e13ea94f8ca9432eb7 Mon Sep 17 00:00:00 2001 From: David Strack Date: Wed, 20 Dec 2023 15:54:01 -0600 Subject: [PATCH 7/8] remove variable no longer needed --- src/ActionList/List.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/ActionList/List.tsx b/src/ActionList/List.tsx index 25d9265f900..ba2178d3efa 100644 --- a/src/ActionList/List.tsx +++ b/src/ActionList/List.tsx @@ -62,13 +62,12 @@ export const List = React.forwardRef( } = React.useContext(ActionListContainerContext) const ariaLabelledBy = slots.heading ? slots.heading.props.id ?? headingId : listLabelledBy - const listSelectionVariant = selectionVariant || containerSelectionVariant return ( Date: Wed, 20 Dec 2023 16:07:57 -0600 Subject: [PATCH 8/8] update snapshots --- .../__snapshots__/Autocomplete.test.tsx.snap | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/__tests__/__snapshots__/Autocomplete.test.tsx.snap b/src/__tests__/__snapshots__/Autocomplete.test.tsx.snap index 74982962b7d..c569d6033de 100644 --- a/src/__tests__/__snapshots__/Autocomplete.test.tsx.snap +++ b/src/__tests__/__snapshots__/Autocomplete.test.tsx.snap @@ -789,6 +789,7 @@ exports[`snapshots renders a menu that contains an item to add to the menu 1`] = >