Skip to content

Fix missing selection attributes in ActionList items #4096

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Dec 21, 2023
Merged

Fix missing selection attributes in ActionList items #4096

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6a53d53
include selectionAttribute in ListContext
strackoverflow Dec 20, 2023
80d1f72
remove aria-selected from test to assert default ActionList selection…
strackoverflow Dec 20, 2023
6b111f7
changeset
strackoverflow Dec 20, 2023
39689c1
clarify variable names
strackoverflow Dec 20, 2023
9feca29
only apply aria selection attributes to the appropriate roles
strackoverflow Dec 20, 2023
0f212b1
infer selection attribute based on item role
strackoverflow Dec 20, 2023
afb88dc
remove variable no longer needed
strackoverflow Dec 20, 2023
dc1ea48
update snapshots
strackoverflow Dec 20, 2023
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
5 changes: 5 additions & 0 deletions .changeset/rare-jeans-rush.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

Fix missing `aria-selected` & `aria-checked` attributes in ActionList items
1 change: 0 additions & 1 deletion src/ActionList/ActionList.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down
29 changes: 21 additions & 8 deletions src/ActionList/Item.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,15 +98,24 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
: listSelectionVariant

/** 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

/** 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 = {
Expand Down Expand Up @@ -234,6 +243,10 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(

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,
Expand All @@ -244,12 +257,12 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
'aria-describedby': slots.blockDescription
? [blockDescriptionId, inactiveWarningId].join(' ')
: inactiveWarningId,
...(selectionAttribute && {[selectionAttribute]: selected}),
role: role || itemRole,
...(includeSelectionAttribute && {[itemSelectionAttribute]: selected}),
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 : {}

Expand Down
31 changes: 31 additions & 0 deletions src/__tests__/__snapshots__/Autocomplete.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -789,6 +789,7 @@ exports[`snapshots renders a menu that contains an item to add to the menu 1`] =
>
<li
aria-labelledby="0--label "
aria-selected={false}
Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like Autocomplete might have had a similar issue with list items not having aria-selected attributes, and this fix applies here as well.

className="c2 c3"
data-id="0"
id="0"
Expand Down Expand Up @@ -819,6 +820,7 @@ exports[`snapshots renders a menu that contains an item to add to the menu 1`] =
</li>
<li
aria-labelledby="1--label "
aria-selected={false}
className="c2 c3"
data-id="1"
id="1"
Expand Down Expand Up @@ -849,6 +851,7 @@ exports[`snapshots renders a menu that contains an item to add to the menu 1`] =
</li>
<li
aria-labelledby="2--label "
aria-selected={false}
className="c2 c3"
data-id="2"
id="2"
Expand Down Expand Up @@ -879,6 +882,7 @@ exports[`snapshots renders a menu that contains an item to add to the menu 1`] =
</li>
<li
aria-labelledby="3--label "
aria-selected={false}
className="c2 c3"
data-id="3"
id="3"
Expand Down Expand Up @@ -909,6 +913,7 @@ exports[`snapshots renders a menu that contains an item to add to the menu 1`] =
</li>
<li
aria-labelledby="4--label "
aria-selected={false}
className="c2 c3"
data-id="4"
id="4"
Expand Down Expand Up @@ -939,6 +944,7 @@ exports[`snapshots renders a menu that contains an item to add to the menu 1`] =
</li>
<li
aria-labelledby="5--label "
aria-selected={false}
className="c2 c3"
data-id="5"
id="5"
Expand Down Expand Up @@ -969,6 +975,7 @@ exports[`snapshots renders a menu that contains an item to add to the menu 1`] =
</li>
<li
aria-labelledby="6--label "
aria-selected={false}
className="c2 c3"
data-id="6"
id="6"
Expand Down Expand Up @@ -999,6 +1006,7 @@ exports[`snapshots renders a menu that contains an item to add to the menu 1`] =
</li>
<li
aria-labelledby="7--label "
aria-selected={false}
className="c2 c3"
data-id="7"
id="7"
Expand Down Expand Up @@ -1029,6 +1037,7 @@ exports[`snapshots renders a menu that contains an item to add to the menu 1`] =
</li>
<li
aria-labelledby="20--label "
aria-selected={false}
className="c2 c3"
data-id="20"
id="20"
Expand Down Expand Up @@ -1059,6 +1068,7 @@ exports[`snapshots renders a menu that contains an item to add to the menu 1`] =
</li>
<li
aria-labelledby="21--label "
aria-selected={false}
className="c2 c3"
data-id="21"
id="21"
Expand Down Expand Up @@ -1089,6 +1099,7 @@ exports[`snapshots renders a menu that contains an item to add to the menu 1`] =
</li>
<li
aria-labelledby="add-new-item--label "
aria-selected={false}
className="c2 c3"
data-id="add-new-item"
id="add-new-item"
Expand Down Expand Up @@ -1512,6 +1523,7 @@ exports[`snapshots renders a multiselect input 1`] = `
>
<li
aria-labelledby="0--label "
aria-selected={false}
className="c2 c3"
data-id="0"
id="0"
Expand Down Expand Up @@ -1542,6 +1554,7 @@ exports[`snapshots renders a multiselect input 1`] = `
</li>
<li
aria-labelledby="1--label "
aria-selected={false}
className="c2 c3"
data-id="1"
id="1"
Expand Down Expand Up @@ -1572,6 +1585,7 @@ exports[`snapshots renders a multiselect input 1`] = `
</li>
<li
aria-labelledby="2--label "
aria-selected={false}
className="c2 c3"
data-id="2"
id="2"
Expand Down Expand Up @@ -1602,6 +1616,7 @@ exports[`snapshots renders a multiselect input 1`] = `
</li>
<li
aria-labelledby="3--label "
aria-selected={false}
className="c2 c3"
data-id="3"
id="3"
Expand Down Expand Up @@ -1632,6 +1647,7 @@ exports[`snapshots renders a multiselect input 1`] = `
</li>
<li
aria-labelledby="4--label "
aria-selected={false}
className="c2 c3"
data-id="4"
id="4"
Expand Down Expand Up @@ -1662,6 +1678,7 @@ exports[`snapshots renders a multiselect input 1`] = `
</li>
<li
aria-labelledby="5--label "
aria-selected={false}
className="c2 c3"
data-id="5"
id="5"
Expand Down Expand Up @@ -1692,6 +1709,7 @@ exports[`snapshots renders a multiselect input 1`] = `
</li>
<li
aria-labelledby="6--label "
aria-selected={false}
className="c2 c3"
data-id="6"
id="6"
Expand Down Expand Up @@ -1722,6 +1740,7 @@ exports[`snapshots renders a multiselect input 1`] = `
</li>
<li
aria-labelledby="7--label "
aria-selected={false}
className="c2 c3"
data-id="7"
id="7"
Expand Down Expand Up @@ -1752,6 +1771,7 @@ exports[`snapshots renders a multiselect input 1`] = `
</li>
<li
aria-labelledby="20--label "
aria-selected={false}
className="c2 c3"
data-id="20"
id="20"
Expand Down Expand Up @@ -1782,6 +1802,7 @@ exports[`snapshots renders a multiselect input 1`] = `
</li>
<li
aria-labelledby="21--label "
aria-selected={false}
className="c2 c3"
data-id="21"
id="21"
Expand Down Expand Up @@ -2329,6 +2350,7 @@ exports[`snapshots renders a multiselect input with selected menu items 1`] = `
>
<li
aria-labelledby="0--label "
aria-selected={true}
className="c2 c3"
data-id="0"
id="0"
Expand Down Expand Up @@ -2359,6 +2381,7 @@ exports[`snapshots renders a multiselect input with selected menu items 1`] = `
</li>
<li
aria-labelledby="1--label "
aria-selected={true}
className="c2 c3"
data-id="1"
id="1"
Expand Down Expand Up @@ -2389,6 +2412,7 @@ exports[`snapshots renders a multiselect input with selected menu items 1`] = `
</li>
<li
aria-labelledby="2--label "
aria-selected={true}
className="c2 c3"
data-id="2"
id="2"
Expand Down Expand Up @@ -2419,6 +2443,7 @@ exports[`snapshots renders a multiselect input with selected menu items 1`] = `
</li>
<li
aria-labelledby="3--label "
aria-selected={false}
className="c2 c8"
data-id="3"
id="3"
Expand Down Expand Up @@ -2449,6 +2474,7 @@ exports[`snapshots renders a multiselect input with selected menu items 1`] = `
</li>
<li
aria-labelledby="4--label "
aria-selected={false}
className="c2 c8"
data-id="4"
id="4"
Expand Down Expand Up @@ -2479,6 +2505,7 @@ exports[`snapshots renders a multiselect input with selected menu items 1`] = `
</li>
<li
aria-labelledby="5--label "
aria-selected={false}
className="c2 c8"
data-id="5"
id="5"
Expand Down Expand Up @@ -2509,6 +2536,7 @@ exports[`snapshots renders a multiselect input with selected menu items 1`] = `
</li>
<li
aria-labelledby="6--label "
aria-selected={false}
className="c2 c8"
data-id="6"
id="6"
Expand Down Expand Up @@ -2539,6 +2567,7 @@ exports[`snapshots renders a multiselect input with selected menu items 1`] = `
</li>
<li
aria-labelledby="7--label "
aria-selected={false}
className="c2 c8"
data-id="7"
id="7"
Expand Down Expand Up @@ -2569,6 +2598,7 @@ exports[`snapshots renders a multiselect input with selected menu items 1`] = `
</li>
<li
aria-labelledby="20--label "
aria-selected={false}
className="c2 c8"
data-id="20"
id="20"
Expand Down Expand Up @@ -2599,6 +2629,7 @@ exports[`snapshots renders a multiselect input with selected menu items 1`] = `
</li>
<li
aria-labelledby="21--label "
aria-selected={false}
className="c2 c8"
data-id="21"
id="21"
Expand Down