From e8367426573792f56aec1b0ce85da17082afd4be Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Wed, 29 Mar 2023 16:40:59 -0700 Subject: [PATCH 1/2] Menu escape should not clear selection --- packages/@react-aria/menu/src/useMenu.ts | 1 + .../selection/src/useSelectableCollection.ts | 9 +++-- .../selection/src/useSelectableList.ts | 10 ++++-- .../menu/test/MenuTrigger.test.js | 35 +++++++++++++++++-- 4 files changed, 49 insertions(+), 6 deletions(-) diff --git a/packages/@react-aria/menu/src/useMenu.ts b/packages/@react-aria/menu/src/useMenu.ts index 795a8caf061..bfc24b351a1 100644 --- a/packages/@react-aria/menu/src/useMenu.ts +++ b/packages/@react-aria/menu/src/useMenu.ts @@ -63,6 +63,7 @@ export function useMenu(props: AriaMenuOptions, state: TreeState, ref: selectionManager: state.selectionManager, collection: state.collection, disabledKeys: state.disabledKeys, + preventEscapeClearsSelection: true, shouldFocusWrap }); diff --git a/packages/@react-aria/selection/src/useSelectableCollection.ts b/packages/@react-aria/selection/src/useSelectableCollection.ts index a6ce604132a..015bfed239c 100644 --- a/packages/@react-aria/selection/src/useSelectableCollection.ts +++ b/packages/@react-aria/selection/src/useSelectableCollection.ts @@ -79,7 +79,11 @@ export interface AriaSelectableCollectionOptions { * The ref attached to the scrollable body. Used to provide automatic scrolling on item focus for non-virtualized collections. * If not provided, defaults to the collection ref. */ - scrollRef?: RefObject + scrollRef?: RefObject, + /** + * Whether the escape key should clear selection. + */ + preventEscapeClearsSelection?: boolean } export interface SelectableCollectionAria { @@ -103,6 +107,7 @@ export function useSelectableCollection(options: AriaSelectableCollectionOptions disallowTypeAhead = false, shouldUseVirtualFocus, allowsTabNavigation = false, + preventEscapeClearsSelection = false, isVirtualized, // If no scrollRef is provided, assume the collection ref is the scrollable region scrollRef = ref @@ -223,7 +228,7 @@ export function useSelectableCollection(options: AriaSelectableCollectionOptions break; case 'Escape': e.preventDefault(); - if (!disallowEmptySelection) { + if (!disallowEmptySelection && !preventEscapeClearsSelection) { manager.clearSelection(); } break; diff --git a/packages/@react-aria/selection/src/useSelectableList.ts b/packages/@react-aria/selection/src/useSelectableList.ts index 0c51a578f88..88bbc33f34d 100644 --- a/packages/@react-aria/selection/src/useSelectableList.ts +++ b/packages/@react-aria/selection/src/useSelectableList.ts @@ -74,7 +74,11 @@ export interface AriaSelectableListOptions { /** * Whether navigation through tab key is enabled. */ - allowsTabNavigation?: boolean + allowsTabNavigation?: boolean, + /** + * Whether the escape key should clear selection. + */ + preventEscapeClearsSelection?: boolean } export interface SelectableListAria { @@ -101,7 +105,8 @@ export function useSelectableList(props: AriaSelectableListOptions): SelectableL selectOnFocus = selectionManager.selectionBehavior === 'replace', disallowTypeAhead, shouldUseVirtualFocus, - allowsTabNavigation + allowsTabNavigation, + preventEscapeClearsSelection } = props; // By default, a KeyboardDelegate is provided which uses the DOM to query layout information (e.g. for page up/page down). @@ -124,6 +129,7 @@ export function useSelectableList(props: AriaSelectableListOptions): SelectableL shouldUseVirtualFocus, allowsTabNavigation, isVirtualized, + preventEscapeClearsSelection, scrollRef: ref }); diff --git a/packages/@react-spectrum/menu/test/MenuTrigger.test.js b/packages/@react-spectrum/menu/test/MenuTrigger.test.js index c3fcedd219f..76f21849c3d 100644 --- a/packages/@react-spectrum/menu/test/MenuTrigger.test.js +++ b/packages/@react-spectrum/menu/test/MenuTrigger.test.js @@ -370,7 +370,6 @@ describe('MenuTrigger', function () { act(() => {jest.runAllTimers();}); // FocusScope raf } - // Can't figure out why this isn't working for the v2 component it.each` Name | Component | props ${'MenuTrigger'} | ${MenuTrigger} | ${{onOpenChange}} @@ -389,7 +388,39 @@ describe('MenuTrigger', function () { expect(document.activeElement).toBe(button); }); - // Can't figure out why this isn't working for the v2 component + it.each` + Name | Component | props + ${'MenuTrigger'} | ${MenuTrigger} | ${{onOpenChange}} + `('$Name does not clear selection with escape', function ({Component, props}) { + let onSelectionChange = jest.fn(); + tree = renderComponent(Component, props, {selectionMode: 'multiple', defaultSelectedKeys: ['Foo'], onSelectionChange}); + let button = tree.getByRole('button'); + triggerPress(button); + act(() => {jest.runAllTimers();}); + expect(onSelectionChange).not.toHaveBeenCalled(); + + let menu = tree.getByRole('menu'); + expect(menu).toBeTruthy(); + expect(within(menu).getAllByRole('menuitemcheckbox')[0]).toHaveAttribute('aria-checked', 'true'); + fireEvent.keyDown(menu, {key: 'Escape', code: 27, charCode: 27}); + act(() => {jest.runAllTimers();}); // FocusScope useLayoutEffect cleanup + act(() => {jest.runAllTimers();}); // FocusScope raf + expect(menu).not.toBeInTheDocument(); + expect(document.activeElement).toBe(button); + expect(onSelectionChange).not.toHaveBeenCalled(); + + // reopen and make sure we still have the selection + triggerPress(button); + act(() => {jest.runAllTimers();}); + expect(onSelectionChange).not.toHaveBeenCalled(); + + menu = tree.getByRole('menu'); + expect(within(menu).getAllByRole('menuitemcheckbox')[0]).toHaveAttribute('aria-checked', 'true'); + expect(menu).toBeTruthy(); + fireEvent.keyDown(menu, {key: 'Escape', code: 27, charCode: 27}); + expect(onSelectionChange).not.toHaveBeenCalled(); + }); + it.each` Name | Component | props ${'MenuTrigger'} | ${MenuTrigger} | ${{onOpenChange}} From 879a3d4455da537703538d2a3db4c9313256eddf Mon Sep 17 00:00:00 2001 From: Rob Snow Date: Mon, 10 Apr 2023 17:49:46 -0700 Subject: [PATCH 2/2] Move logic to just be in useMenu --- packages/@react-aria/menu/src/useMenu.ts | 9 +++++++-- .../selection/src/useSelectableCollection.ts | 9 ++------- .../@react-aria/selection/src/useSelectableList.ts | 10 ++-------- 3 files changed, 11 insertions(+), 17 deletions(-) diff --git a/packages/@react-aria/menu/src/useMenu.ts b/packages/@react-aria/menu/src/useMenu.ts index bfc24b351a1..251b2138f32 100644 --- a/packages/@react-aria/menu/src/useMenu.ts +++ b/packages/@react-aria/menu/src/useMenu.ts @@ -63,7 +63,6 @@ export function useMenu(props: AriaMenuOptions, state: TreeState, ref: selectionManager: state.selectionManager, collection: state.collection, disabledKeys: state.disabledKeys, - preventEscapeClearsSelection: true, shouldFocusWrap }); @@ -75,7 +74,13 @@ export function useMenu(props: AriaMenuOptions, state: TreeState, ref: return { menuProps: mergeProps(domProps, { role: 'menu', - ...listProps + ...listProps, + onKeyDown: (e) => { + // don't clear the menu selected keys if the user is presses escape since escape closes the menu + if (e.key !== 'Escape') { + listProps.onKeyDown(e); + } + } }) }; } diff --git a/packages/@react-aria/selection/src/useSelectableCollection.ts b/packages/@react-aria/selection/src/useSelectableCollection.ts index 015bfed239c..a6ce604132a 100644 --- a/packages/@react-aria/selection/src/useSelectableCollection.ts +++ b/packages/@react-aria/selection/src/useSelectableCollection.ts @@ -79,11 +79,7 @@ export interface AriaSelectableCollectionOptions { * The ref attached to the scrollable body. Used to provide automatic scrolling on item focus for non-virtualized collections. * If not provided, defaults to the collection ref. */ - scrollRef?: RefObject, - /** - * Whether the escape key should clear selection. - */ - preventEscapeClearsSelection?: boolean + scrollRef?: RefObject } export interface SelectableCollectionAria { @@ -107,7 +103,6 @@ export function useSelectableCollection(options: AriaSelectableCollectionOptions disallowTypeAhead = false, shouldUseVirtualFocus, allowsTabNavigation = false, - preventEscapeClearsSelection = false, isVirtualized, // If no scrollRef is provided, assume the collection ref is the scrollable region scrollRef = ref @@ -228,7 +223,7 @@ export function useSelectableCollection(options: AriaSelectableCollectionOptions break; case 'Escape': e.preventDefault(); - if (!disallowEmptySelection && !preventEscapeClearsSelection) { + if (!disallowEmptySelection) { manager.clearSelection(); } break; diff --git a/packages/@react-aria/selection/src/useSelectableList.ts b/packages/@react-aria/selection/src/useSelectableList.ts index 88bbc33f34d..0c51a578f88 100644 --- a/packages/@react-aria/selection/src/useSelectableList.ts +++ b/packages/@react-aria/selection/src/useSelectableList.ts @@ -74,11 +74,7 @@ export interface AriaSelectableListOptions { /** * Whether navigation through tab key is enabled. */ - allowsTabNavigation?: boolean, - /** - * Whether the escape key should clear selection. - */ - preventEscapeClearsSelection?: boolean + allowsTabNavigation?: boolean } export interface SelectableListAria { @@ -105,8 +101,7 @@ export function useSelectableList(props: AriaSelectableListOptions): SelectableL selectOnFocus = selectionManager.selectionBehavior === 'replace', disallowTypeAhead, shouldUseVirtualFocus, - allowsTabNavigation, - preventEscapeClearsSelection + allowsTabNavigation } = props; // By default, a KeyboardDelegate is provided which uses the DOM to query layout information (e.g. for page up/page down). @@ -129,7 +124,6 @@ export function useSelectableList(props: AriaSelectableListOptions): SelectableL shouldUseVirtualFocus, allowsTabNavigation, isVirtualized, - preventEscapeClearsSelection, scrollRef: ref });