From dbf82f4576e8d071dc2d41b50396fa68ab5505c2 Mon Sep 17 00:00:00 2001 From: Siddharth Kshetrapal Date: Thu, 12 Sep 2024 10:49:33 +0200 Subject: [PATCH] Select Panel: Functional adjustments (#4906) * super wip * just use the actionlist component and revert the type updates * Update packages/react/src/FilteredActionList/FilteredActionList.tsx * Some more progress * revert the type changes and cast it :/ * clean * wip * wip wip * add stories * fix linting * add tests for groups * Map groups * Update story names for e2e tests * oops remove unintended file * update story name * same - update story name * disable animations * test(vrt): update snapshots * Update tests since new action list has different semantics for group headings * logging * pass the rest * extract children and use before text * remove logging * test(vrt): update snapshots * add active styles to ActionList * rename component name to be clearer * remove variant full from examples * tiny clean up * fix showItemDividers * another tiny cleanup * pull MappedActionListItem to make it stable * test(vrt): update snapshots * show active styles only when used with keyboard * backward compat: expose id as data-id * update snapshots * add story for long strings * fishing for errors * backward compatibility for renderItem * remove todo now * add a feature flag * clean up dual filter list setup * run jests test with both states of feature flags * refactor snapshot tests with scenarios * remove feature flag for main * test(vrt): update snapshots * add feature flag to e2e matrix * test(vrt): update snapshots * backward compat: allow groupMetadata to be empty array * sigh newline * Create sour-cooks-dress.md * copy SelectPanel snapshots from main * remove unrelated changes in this PR * test(vrt): update snapshots * add more bindkeys * add for deprecated version as well * label listbox/ActionList by panel title * Create wicked-books-occur.md * remove Home and End * add test for PageDown and PageUp * update changeset * active styles for both directly and indirectly * add role=combobox * update snapshot * test(vrt): update snapshots * remove features from deprecated version * update test * only use aria-labelledby when aria-label is not passed * remove combobox for this PR --------- Co-authored-by: Armagan Ersoz Co-authored-by: broccolinisoup Co-authored-by: siddharthkp --- .changeset/wicked-books-occur.md | 7 +++ ...FilteredActionListWithModernActionList.tsx | 3 +- .../src/SelectPanel/SelectPanel.test.tsx | 48 ++++++++++++++++++- .../react/src/SelectPanel/SelectPanel.tsx | 3 ++ .../react/src/deprecated/ActionList/List.tsx | 5 ++ 5 files changed, 64 insertions(+), 2 deletions(-) create mode 100644 .changeset/wicked-books-occur.md diff --git a/.changeset/wicked-books-occur.md b/.changeset/wicked-books-occur.md new file mode 100644 index 00000000000..5fbb6f05ceb --- /dev/null +++ b/.changeset/wicked-books-occur.md @@ -0,0 +1,7 @@ +--- +"@primer/react": minor +--- + +SelectPanel: Support PageDown and PageUp for keyboard navigation + +SelectPanel: Label `listbox` by the title of the panel diff --git a/packages/react/src/FilteredActionList/FilteredActionListWithModernActionList.tsx b/packages/react/src/FilteredActionList/FilteredActionListWithModernActionList.tsx index 0d99b359275..ae791e63909 100644 --- a/packages/react/src/FilteredActionList/FilteredActionListWithModernActionList.tsx +++ b/packages/react/src/FilteredActionList/FilteredActionListWithModernActionList.tsx @@ -1,5 +1,5 @@ import type {ScrollIntoViewOptions} from '@primer/behaviors' -import {scrollIntoView} from '@primer/behaviors' +import {scrollIntoView, FocusKeys} from '@primer/behaviors' import type {KeyboardEventHandler} from 'react' import React, {useCallback, useEffect, useRef} from 'react' import styled from 'styled-components' @@ -86,6 +86,7 @@ export function FilteredActionList({ useFocusZone( { containerRef: listContainerRef, + bindKeys: FocusKeys.ArrowVertical | FocusKeys.PageUpDown, focusOutBehavior: 'wrap', focusableElementFilter: element => { return !(element instanceof HTMLInputElement) diff --git a/packages/react/src/SelectPanel/SelectPanel.test.tsx b/packages/react/src/SelectPanel/SelectPanel.test.tsx index 9d6cec1f9cb..7d6adfbb3ae 100644 --- a/packages/react/src/SelectPanel/SelectPanel.test.tsx +++ b/packages/react/src/SelectPanel/SelectPanel.test.tsx @@ -24,7 +24,7 @@ const items: SelectPanelProps['items'] = [ }, ] -function BasicSelectPanel() { +function BasicSelectPanel(passthroughProps: Record) { const [selected, setSelected] = React.useState([]) const [filter, setFilter] = React.useState('') const [open, setOpen] = React.useState(false) @@ -51,6 +51,7 @@ function BasicSelectPanel() { onOpenChange={isOpen => { setOpen(isOpen) }} + {...passthroughProps} /> ) @@ -200,6 +201,22 @@ for (const useModernActionList of [false, true]) { expect(onOpenChange).toHaveBeenLastCalledWith(false, 'click-outside') }) + it('should label the list by title unless a aria-label is explicitly passed', async () => { + const user = userEvent.setup() + + renderWithFlag(, useModernActionList) + await user.click(screen.getByText('Select items')) + expect(screen.getByRole('listbox', {name: 'test title'})).toBeInTheDocument() + }) + + it('should label the list by aria-label when explicitly passed', async () => { + const user = userEvent.setup() + + renderWithFlag(, useModernActionList) + await user.click(screen.getByText('Select items')) + expect(screen.getByRole('listbox', {name: 'Custom label'})).toBeInTheDocument() + }) + describe('selection', () => { it('should select an active option when activated', async () => { const user = userEvent.setup() @@ -288,6 +305,35 @@ for (const useModernActionList of [false, true]) { screen.getByRole('option', {name: 'item one'}).id, ) }) + + it('should support navigating through items with PageDown and PageUp', async () => { + if (!useModernActionList) return // this feature is only enabled with feature flag on + + const user = userEvent.setup() + + renderWithFlag(, useModernActionList) + + await user.click(screen.getByText('Select items')) + + // First item by default should be the active element + expect(document.activeElement!).toHaveAttribute( + 'aria-activedescendant', + screen.getByRole('option', {name: 'item one'}).id, + ) + + await user.type(document.activeElement!, '{PageDown}') + + expect(document.activeElement!).toHaveAttribute( + 'aria-activedescendant', + screen.getByRole('option', {name: 'item three'}).id, + ) + + await user.type(document.activeElement!, '{PageUp}') + expect(document.activeElement!).toHaveAttribute( + 'aria-activedescendant', + screen.getByRole('option', {name: 'item one'}).id, + ) + }) }) describe('filtering', () => { diff --git a/packages/react/src/SelectPanel/SelectPanel.tsx b/packages/react/src/SelectPanel/SelectPanel.tsx index 5763081361b..edf35fafee6 100644 --- a/packages/react/src/SelectPanel/SelectPanel.tsx +++ b/packages/react/src/SelectPanel/SelectPanel.tsx @@ -218,6 +218,9 @@ export function SelectPanel({ placeholderText={placeholderText} {...listProps} role="listbox" + // browsers give aria-labelledby precedence over aria-label so we need to make sure + // we don't accidentally override props.aria-label + aria-labelledby={listProps['aria-label'] ? undefined : titleId} aria-multiselectable={isMultiSelectVariant(selected) ? 'true' : 'false'} selectionVariant={isMultiSelectVariant(selected) ? 'multiple' : 'single'} items={itemsToRender} diff --git a/packages/react/src/deprecated/ActionList/List.tsx b/packages/react/src/deprecated/ActionList/List.tsx index d873ea97bb1..4e9a7b4b737 100644 --- a/packages/react/src/deprecated/ActionList/List.tsx +++ b/packages/react/src/deprecated/ActionList/List.tsx @@ -37,6 +37,11 @@ export interface ListPropsBase { */ id?: string + /** + * aria-label to attach to the base DOM node of the list + */ + 'aria-label'?: string + /** * A `List`-level custom `Item` renderer. Every `Item` within this `List` * without a `Group`-level or `Item`-level custom `Item` renderer will be