diff --git a/.changeset/rotten-goats-stare.md b/.changeset/rotten-goats-stare.md new file mode 100644 index 00000000000..3e9fe4bd87b --- /dev/null +++ b/.changeset/rotten-goats-stare.md @@ -0,0 +1,5 @@ +--- +'@primer/react': minor +--- + +FilteredActionList: Remove `usingRemoveActiveDescendant` feature flag, add private prop diff --git a/packages/react/src/FilteredActionList/FilteredActionList.tsx b/packages/react/src/FilteredActionList/FilteredActionList.tsx index d6d3fe6dfec..4c2b59b2cc2 100644 --- a/packages/react/src/FilteredActionList/FilteredActionList.tsx +++ b/packages/react/src/FilteredActionList/FilteredActionList.tsx @@ -22,7 +22,6 @@ import {ActionListContainerContext} from '../ActionList/ActionListContainerConte import {isValidElementType} from 'react-is' import {useAnnouncements} from './useAnnouncements' import {clsx} from 'clsx' -import {useFeatureFlag} from '../FeatureFlags' const menuScrollMargins: ScrollIntoViewOptions = {startMargin: 0, endMargin: 8} @@ -45,6 +44,26 @@ export interface FilteredActionListProps extends Partial void + /** + * Private API for use internally only. Adds the ability to switch between + * `active-descendant` and roving tabindex. + * + * By default, FilteredActionList uses `aria-activedescendant` to manage focus. + * + * Roving tabindex is an alternative focus management method that moves + * focus to the list items themselves instead of keeping focus on the input. + * + * Improper usage can lead to inaccessible experiences, so this prop should be used with caution. + * + * For usage, refer to the documentation: + * + * WAI-ARIA `aria-activedescendant`: https://www.w3.org/TR/wai-aria-1.2/#aria-activedescendant + * + * Roving Tabindex: https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface/#kbd_roving_tabindex + * + * @default 'active-descendant' + */ + _PrivateFocusManagement?: 'roving-tabindex' | 'active-descendant' } export function FilteredActionList({ @@ -67,6 +86,7 @@ export function FilteredActionList({ announcementsEnabled = true, fullScreenOnNarrow, onSelectAllChange, + _PrivateFocusManagement = 'active-descendant', ...listProps }: FilteredActionListProps): JSX.Element { const [filterValue, setInternalFilterValue] = useProvidedStateOrCreate(externalFilterValue, undefined, '') @@ -85,7 +105,7 @@ export function FilteredActionList({ const scrollContainerRef = useRef(null) const inputRef = useProvidedRefOrCreate(providedInputRef) - const usingRemoveActiveDescendant = useFeatureFlag('primer_react_select_panel_remove_active_descendant') + const usingRovingTabindex = _PrivateFocusManagement === 'roving-tabindex' const [listContainerElement, setListContainerElement] = useState(null) const activeDescendantRef = useRef() @@ -164,7 +184,6 @@ export function FilteredActionList({ [activeDescendantRef], ) - // BEGIN: Todo remove when we remove usingRemoveActiveDescendant const listContainerRefCallback = useCallback( (node: HTMLUListElement | null) => { setListContainerElement(node) @@ -175,10 +194,9 @@ export function FilteredActionList({ useEffect(() => { onInputRefChanged?.(inputRef) }, [inputRef, onInputRefChanged]) - //END: Todo remove when we remove usingRemoveActiveDescendant useFocusZone( - !usingRemoveActiveDescendant + !usingRovingTabindex ? { containerRef: {current: listContainerElement}, bindKeys: FocusKeys.ArrowVertical | FocusKeys.PageUpDown, @@ -196,7 +214,7 @@ export function FilteredActionList({ }, } : undefined, - [listContainerElement, usingRemoveActiveDescendant], + [listContainerElement, usingRovingTabindex], ) useEffect(() => { @@ -209,7 +227,7 @@ export function FilteredActionList({ }, [items, inputRef]) useEffect(() => { - if (usingRemoveActiveDescendant) { + if (usingRovingTabindex) { const inputAndListContainerElement = inputAndListContainerRef.current if (!inputAndListContainerElement) return const list = listRef.current @@ -228,21 +246,22 @@ export function FilteredActionList({ inputAndListContainerElement.removeEventListener('focusin', handleFocusIn) } } - }, [items, inputRef, listContainerElement, usingRemoveActiveDescendant]) // Re-run when items change to update active indicators + }, [items, inputRef, listContainerElement, usingRovingTabindex]) // Re-run when items change to update active indicators useEffect(() => { - if (usingRemoveActiveDescendant && !loading) { + if (usingRovingTabindex && !loading) { setIsInputFocused(inputRef.current && inputRef.current === document.activeElement ? true : false) } - }, [loading, inputRef, usingRemoveActiveDescendant]) + }, [loading, inputRef, usingRovingTabindex]) useAnnouncements( items, - usingRemoveActiveDescendant ? listRef : {current: listContainerElement}, + usingRovingTabindex ? listRef : {current: listContainerElement}, inputRef, announcementsEnabled, loading, messageText, + _PrivateFocusManagement, ) useScrollFlash(scrollContainerRef) @@ -265,7 +284,7 @@ export function FilteredActionList({ let firstGroupIndex = 0 const actionListContent = ( {}} + onKeyDown={usingRovingTabindex ? onInputKeyDown : () => {}} placeholder={placeholderText} role="combobox" aria-expanded="true" diff --git a/packages/react/src/FilteredActionList/useAnnouncements.tsx b/packages/react/src/FilteredActionList/useAnnouncements.tsx index 697b1232e4e..c3026078eba 100644 --- a/packages/react/src/FilteredActionList/useAnnouncements.tsx +++ b/packages/react/src/FilteredActionList/useAnnouncements.tsx @@ -5,7 +5,6 @@ import {announce as liveRegionAnnounce} from '@primer/live-region-element' import {useCallback, useEffect, useRef} from 'react' import type {FilteredActionListProps} from './index' import type {ItemInput} from '../SelectPanel' -import {useFeatureFlag} from '../FeatureFlags' // we add a delay so that it does not interrupt default screen reader announcement and queues after it const delayMs = 500 @@ -19,7 +18,6 @@ const useFirstRender = () => { return firstRender.current } -//TODO remove this when we remove usingRemoveActiveDescendant const getItemWithActiveDescendant = ( listRef: React.RefObject, items: FilteredActionListProps['items'], @@ -39,7 +37,6 @@ const getItemWithActiveDescendant = ( return {index, text, selected} } -//TODO remove this when we remove usingRemoveActiveDescendant export const useAnnouncements = ( items: FilteredActionListProps['items'], @@ -48,8 +45,10 @@ export const useAnnouncements = ( enabled: boolean = true, loading: boolean = false, message?: {title: string; description: string}, + focusManagement?: 'active-descendant' | 'roving-tabindex', ) => { - const usingRemoveActiveDescendant = useFeatureFlag('primer_react_select_panel_remove_active_descendant') + const usingRovingTabindex = focusManagement === 'roving-tabindex' + const liveRegion = document.querySelector('live-region') // Notify user of the number of items available @@ -67,7 +66,7 @@ export const useAnnouncements = ( useEffect( function announceInitialFocus() { const focusHandler = () => { - if (usingRemoveActiveDescendant) { + if (usingRovingTabindex) { const announcementText = `${items.length} item${items.length > 1 ? 's' : ''} available, ${selectedItems} selected.` announce(announcementText, { delayMs, @@ -98,7 +97,7 @@ export const useAnnouncements = ( inputElement?.addEventListener('focus', focusHandler) return () => inputElement?.removeEventListener('focus', focusHandler) }, - [listContainerRef, inputRef, items, liveRegion, announce, usingRemoveActiveDescendant, selectedItems], + [listContainerRef, inputRef, items, liveRegion, announce, usingRovingTabindex, selectedItems], ) const isFirstRender = useFirstRender() @@ -113,7 +112,7 @@ export const useAnnouncements = ( return } - if (usingRemoveActiveDescendant) { + if (usingRovingTabindex) { const announcementText = `${items.length} item${items.length > 1 ? 's' : ''} available, ${selectedItems} selected.` announce(announcementText, { @@ -147,7 +146,7 @@ export const useAnnouncements = ( items, listContainerRef, liveRegion, - usingRemoveActiveDescendant, + usingRovingTabindex, message?.title, message?.description, loading, diff --git a/packages/react/src/SelectPanel/SelectPanel.test.tsx b/packages/react/src/SelectPanel/SelectPanel.test.tsx index ccffe06d79c..f7a028948ab 100644 --- a/packages/react/src/SelectPanel/SelectPanel.test.tsx +++ b/packages/react/src/SelectPanel/SelectPanel.test.tsx @@ -19,16 +19,11 @@ export function getLiveRegion(): LiveRegionElement { throw new Error('No live-region found') } -const renderWithFlag = (children: React.ReactNode, flag: boolean) => { - return render( - - {children} - , - ) +const renderWithProp = (element: React.ReactElement, flag?: boolean) => { + // true = 'use roving tabindex' + // false = 'use aria-activedescendant' + const focusManagement = flag ? 'roving-tabindex' : 'active-descendant' + return render(React.cloneElement(element, {_PrivateFocusManagement: focusManagement})) } const items: SelectPanelProps['items'] = [ @@ -79,7 +74,7 @@ globalThis.Element.prototype.scrollTo = vi.fn() for (const usingRemoveActiveDescendant of [false, true]) { describe('SelectPanel', () => { it('should render an anchor to open the select panel using `placeholder`', () => { - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp() expect(screen.getByText('Select items')).toBeInTheDocument() @@ -93,7 +88,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should open the select panel when activating the trigger', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -114,7 +109,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should close the select panel when pressing Escape', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) await user.keyboard('{Escape}') @@ -126,7 +121,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should close the select panel when clicking outside of the select panel', async () => { const user = userEvent.setup() - renderWithFlag( + renderWithProp( <> @@ -143,7 +138,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should open a dialog that is labelled by `title` and described by `subtitle`', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -194,7 +189,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) // Open by click await user.click(screen.getByText('Select items')) @@ -221,7 +216,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should label the list by title unless a aria-label is explicitly passed', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) expect(screen.getByRole('listbox', {name: 'test title'})).toBeInTheDocument() }) @@ -229,7 +224,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should label the list by aria-label when explicitly passed', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) expect(screen.getByRole('listbox', {name: 'Custom label'})).toBeInTheDocument() }) @@ -239,7 +234,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { // This panel contains another focusable thing (the IconButton) that should not receive focus // when the panel opens. - renderWithFlag( + renderWithProp( {}} onFilterChange={() => {}} @@ -267,7 +262,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should select an active option when activated', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -303,7 +298,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should support navigating through items with ArrowUp and ArrowDown', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -382,7 +377,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should support navigating through items with PageDown and PageUp', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -451,7 +446,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { ) } - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -492,6 +487,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { onOpenChange={isOpen => { setOpen(isOpen) }} + _PrivateFocusManagement={usingRemoveActiveDescendant ? 'roving-tabindex' : 'active-descendant'} /> ) } @@ -598,7 +594,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should filter the list of items when the user types into the input', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -645,7 +641,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('displays a loading spinner on first open', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -655,7 +651,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('displays a loading skeleton on first open', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -665,7 +661,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('displays a loading spinner in the text input if items are already loaded', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -682,7 +678,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should announce initially focused item', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) expect(screen.getByLabelText('Filter items')).toHaveFocus() @@ -739,7 +735,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { ) } - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) expect(screen.getByLabelText('Filter items')).toHaveFocus() @@ -749,7 +745,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should announce filtered results', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) expect(screen.getByLabelText('Filter items')).toHaveFocus() @@ -802,7 +798,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should announce default empty message when no results are available (no custom message is provided)', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -854,7 +850,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { ) } - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -876,7 +872,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should accept a className to style the component', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -888,7 +884,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should display the default empty state message when there is no matching item after filtering (No custom message is provided)', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -901,7 +897,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should display the default empty state message when there is no item after the initial load (No custom message is provided)', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await waitFor(async () => { await user.click(screen.getByText('Select items')) @@ -911,7 +907,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should display the custom empty state message when there is no matching item after filtering', async () => { const user = userEvent.setup() - renderWithFlag( + renderWithProp( { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await waitFor(async () => { await user.click(screen.getByText('Select items')) @@ -953,7 +949,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { const handleAction = vi.fn() const user = userEvent.setup() - renderWithFlag( + renderWithProp( , usingRemoveActiveDescendant, ) @@ -1011,7 +1007,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should render the provided `footer` at the bottom of the dialog', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) expect(screen.getByText('test footer')).toBeInTheDocument() @@ -1086,7 +1082,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should render groups with items', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) const listbox = screen.getByRole('listbox') @@ -1105,7 +1101,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should select items within groups', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -1137,7 +1133,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('selections render as radios when variant modal and single select', async () => { const user = userEvent.setup() - renderWithFlag( + renderWithProp( {}} selected={undefined} />, usingRemoveActiveDescendant, ) @@ -1152,7 +1148,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('save and oncancel buttons are present when variant modal', async () => { const user = userEvent.setup() - renderWithFlag( {}} />, usingRemoveActiveDescendant) + renderWithProp( {}} />, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -1181,7 +1177,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should render selected items at the top by default when FF on', async () => { const user = userEvent.setup() - renderWithFlag( + renderWithProp( , @@ -1198,7 +1194,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should not render selected items at the top by default when FF off', async () => { const user = userEvent.setup() - renderWithFlag( + renderWithProp( , @@ -1215,7 +1211,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should not render selected items at the top when showSelectedOptionsFirst set to false', async () => { const user = userEvent.setup() - renderWithFlag( + renderWithProp( , usingRemoveActiveDescendant, ) @@ -1231,7 +1227,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { describe('disableFullscreenOnNarrow prop', () => { const renderSelectPanelWithFlags = (flags: Record, props: Record = {}) => { - return renderWithFlag( + return renderWithProp( , @@ -1352,7 +1348,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should render a Select All checkbox when showSelectAll is true', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -1364,7 +1360,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should not render a Select All checkbox when showSelectAll is false', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -1375,7 +1371,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should select all items when the Select All checkbox is clicked', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -1390,7 +1386,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should deselect all items when the Deselect All checkbox is clicked', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -1411,7 +1407,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should update Select All checkbox to indeterminate state when some items (but not all) are selected', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -1427,7 +1423,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should update Select All checkbox to checked when all items are selected manually', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -1445,7 +1441,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { it('should update Select All checkbox label to "Deselect all" when all items are selected', async () => { const user = userEvent.setup() - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items')) @@ -1491,7 +1487,7 @@ for (const usingRemoveActiveDescendant of [false, true]) { ) } - renderWithFlag(, usingRemoveActiveDescendant) + renderWithProp(, usingRemoveActiveDescendant) await user.click(screen.getByText('Select items'))