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.

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

Jump to bottom

ActionList: Enable focusZone for roles listbox and menu #4795

Merged
merged 6 commits into from
Aug 15, 2024
Merged

ActionList: Enable focusZone for roles listbox and menu #4795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fe152fe
enable focus zone for listbox and menu
siddharthkp Jul 30, 2024
7fd4091
Create actionlist-focuszone.md
siddharthkp Jul 30, 2024
2c242bf
fix other tests that depend on tab instead of ArrowDown
siddharthkp Jul 30, 2024
70e39f6
add failing test
siddharthkp Jul 31, 2024
2eda6fe
disable focuszone inside ActionMenu
siddharthkp Jul 31, 2024
fa13d5a
wrap focus zone for menus
siddharthkp Aug 14, 2024
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/lemon-candles-deny.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": patch
---

ActionList: Enable focusZone for roles listbox and menu
49 changes: 40 additions & 9 deletions packages/react/src/ActionList/ActionList.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,23 +205,22 @@ describe('ActionList', () => {
it('should focus the button around the leading visual when tabbing to an inactive item', async () => {
const component = HTMLRender(<SingleSelectListStory />)
const inactiveOptionButton = await waitFor(() => component.getByRole('button', {name: projects[3].inactiveText}))
const inactiveIndex = projects.findIndex(project => project.inactiveText === projects[3].inactiveText)

for (let i = 0; i < inactiveIndex; i++) {
await userEvent.tab()
}

await userEvent.tab() // get focus on first element
await userEvent.keyboard('{ArrowDown}')
await userEvent.keyboard('{ArrowDown}')
expect(inactiveOptionButton).toHaveFocus()
})

it('should behave as inactive if both inactiveText and loading props are passed', async () => {
const component = HTMLRender(<SingleSelectListStory />)
const inactiveOptionButton = await waitFor(() => component.getByRole('button', {name: projects[5].inactiveText}))
const inactiveIndex = projects.findIndex(project => project.inactiveText === projects[5].inactiveText)

for (let i = 0; i < inactiveIndex; i++) {
await userEvent.tab()
}
await userEvent.tab() // get focus on first element
await userEvent.keyboard('{ArrowDown}')
await userEvent.keyboard('{ArrowDown}')
await userEvent.keyboard('{ArrowDown}')
await userEvent.keyboard('{ArrowDown}')

expect(inactiveOptionButton).toHaveFocus()
})
Expand Down Expand Up @@ -590,4 +589,36 @@ describe('ActionList', () => {

expect(mockOnSelect).toHaveBeenCalledTimes(1)
})

it('should be navigatable with arrow keys for certain roles', async () => {
HTMLRender(
<ActionList role="listbox" aria-label="Select a project">
<ActionList.Item role="option">Option 1</ActionList.Item>
<ActionList.Item role="option">Option 2</ActionList.Item>
<ActionList.Item role="option" disabled>
Option 3
</ActionList.Item>
<ActionList.Item role="option">Option 4</ActionList.Item>
<ActionList.Item role="option" inactiveText="Unavailable due to an outage">
Option 5
</ActionList.Item>
</ActionList>,
)

await userEvent.tab() // tab into the story, this should focus on the first button
expect(document.activeElement).toHaveTextContent('Option 1')

await userEvent.keyboard('{ArrowDown}')
expect(document.activeElement).toHaveTextContent('Option 2')

await userEvent.keyboard('{ArrowDown}')
expect(document.activeElement).not.toHaveTextContent('Option 3') // option 3 is disabled
expect(document.activeElement).toHaveTextContent('Option 4')

await userEvent.keyboard('{ArrowDown}')
expect(document.activeElement).toHaveAccessibleName('Unavailable due to an outage')

await userEvent.keyboard('{ArrowUp}')
expect(document.activeElement).toHaveTextContent('Option 4')
})
})
16 changes: 11 additions & 5 deletions packages/react/src/ActionList/List.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,19 +33,25 @@ export const List = React.forwardRef<HTMLUListElement, ActionListProps>(

/** if list is inside a Menu, it will get a role from the Menu */
const {
listRole,
listRole: listRoleFromContainer,
listLabelledBy,
selectionVariant: containerSelectionVariant, // TODO: Remove after DropdownMenu2 deprecation
enableFocusZone,
enableFocusZone: enableFocusZoneFromContainer,
} = React.useContext(ActionListContainerContext)

const ariaLabelledBy = slots.heading ? slots.heading.props.id ?? headingId : listLabelledBy

const listRole = role || listRoleFromContainer
const listRef = useProvidedRefOrCreate(forwardedRef as React.RefObject<HTMLUListElement>)

let enableFocusZone = false
Copy link
Contributor

Choose a reason for hiding this comment

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

If there's an existing focus zone being used for an ActionList, would the one being used here replace it? I saw an instance where there was an ActionList inside of an AnchoredOverlay that had an implicit focus zone attached:

<AnchoredOverlay
  renderAnchor={props => <Button {...props}>Button</Button>}
  focusZoneSettings={{
    focusOutBehavior: 'wrap',
  }}
>
  <ActionList role="listbox">
    <ActionList.Item>Item 1</ActionList.Item>
    <ActionList.Item>Item 2</ActionList.Item>
  </ActionList>
</AnchoredOverlay>

I'm wondering if this would be an issue or not 🤔. I think the existing cases are rare, so doesn't seem like it would be.

Copy link
Member Author

@siddharthkp siddharthkp Aug 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

focus zones can only be additive, not removed. So, if the parent AnchoredOverlay has a focus zone, that would work on all its children. Setting it to false on ActionList would not exclude it from the parent's focus zone.

Tested with ActionMenu, Autocomplete, etc. because they also use AnchoredOverlay (and it's focus zone)

if (enableFocusZoneFromContainer !== undefined) enableFocusZone = enableFocusZoneFromContainer
else if (listRole) enableFocusZone = ['menu', 'menubar', 'listbox'].includes(listRole)
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking: I see we have have menubar, I am wondering if there'd be a case where role=menubar is used with ActionList 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have one yet, but I think it's possible. The issue linked to menu + menubar pattern, so I included it just in case


useFocusZone({
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we'd want to add focusOutBehavior: 'wrap' by default, or allow consumers to add that themselves?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm wondering if we'd want to add focusOutBehavior: 'wrap' by default

Honestly, not sure, what do you think?

or allow consumers to add that themselves?

That's the part that's missing right now. I think I'd like to wait for feedback/requests before introducing focusZoneSettings like we have on other focus-zoning components

Copy link
Contributor

Choose a reason for hiding this comment

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

For role=menu, we'd definitely want it to wrap by default. With role=listbox there may be cases where it doesn't need to wrap. I'd say if we can conditionally set it to wrap based on if it's a menu or not would be a safe bet, at least for now!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

disabled: !enableFocusZone,
containerRef: listRef,
bindKeys: FocusKeys.ArrowVertical | FocusKeys.HomeAndEnd | FocusKeys.PageUpDown,
focusOutBehavior: listRole === 'menu' ? 'wrap' : undefined,
})

return (
Expand All @@ -54,14 +60,14 @@ export const List = React.forwardRef<HTMLUListElement, ActionListProps>(
variant,
selectionVariant: selectionVariant || containerSelectionVariant,
showDividers,
role: role || listRole,
role: listRole,
headingId,
}}
>
{slots.heading}
<ListBox
sx={merge(styles, sxProp as SxProp)}
role={role || listRole}
role={listRole}
aria-labelledby={ariaLabelledBy}
{...props}
ref={listRef}
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/ActionMenu/ActionMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ const Overlay: React.FC<React.PropsWithChildren<MenuOverlayProps>> = ({
listLabelledBy: ariaLabelledby || anchorAriaLabelledby || anchorId,
selectionAttribute: 'aria-checked', // Should this be here?
afterSelect: () => onClose?.('item-select'),
enableFocusZone: false, // AnchoredOverlay takes care of focus zone
}}
>
{children}
Expand Down
23 changes: 23 additions & 0 deletions packages/react/src/__tests__/ActionMenu.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,29 @@ describe('ActionMenu', () => {
})
})

it('should wrap focus when ArrowDown is pressed on the last element', async () => {
const component = HTMLRender(<Example />)
const button = component.getByRole('button')

const user = userEvent.setup()
await user.click(button)

expect(component.queryByRole('menu')).toBeInTheDocument()
const menuItems = component.getAllByRole('menuitem')

await user.keyboard('{ArrowDown}')
expect(menuItems[0]).toEqual(document.activeElement)

await user.keyboard('{ArrowDown}')
await user.keyboard('{ArrowDown}')
await user.keyboard('{ArrowDown}')
await user.keyboard('{ArrowDown}')
expect(menuItems[menuItems.length - 1]).toEqual(document.activeElement) // last elememt

await user.keyboard('{ArrowDown}')
expect(menuItems[0]).toEqual(document.activeElement) // wrap to first
})

it('should have no axe violations', async () => {
const {container} = HTMLRender(<Example />)
const results = await axe.run(container)
Expand Down
Loading