Skip to content

feat: support selectionMode="replace" in grid collection test utils #8028

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.

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

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
a32b16e
attempt to get rid of jest calls in menu util
LFDanLu Jan 30, 2025
e326a35
update RSP testing docs to directly mention mocks that maybe needed
LFDanLu Jan 30, 2025
9481673
bump versions of RTL to 16
LFDanLu Jan 30, 2025
f625818
use alternative to calling jest run timers in menu option selection
LFDanLu Mar 31, 2025
69d289f
fixing types and properly testing long press
LFDanLu Mar 31, 2025
366c1f8
fix lint
LFDanLu Mar 31, 2025
5a502b8
Merge branch 'main' of github.com:adobe/react-spectrum into test_util…
LFDanLu Mar 31, 2025
8f55260
revert to pre testing library bump for clean slate
LFDanLu Mar 31, 2025
93cbb33
fix build and another submenu edge case
LFDanLu Mar 31, 2025
5207639
fix react 16 bug
LFDanLu Apr 1, 2025
14452d8
update return type of advanceTimer and docs copy
LFDanLu Apr 2, 2025
8bd33c4
Initial support for tree highlight selection support
LFDanLu Apr 2, 2025
dd52a74
move some general fixes from selectionMode="replace" branch here
LFDanLu Apr 2, 2025
5ef9f19
add highlight selection support to gridlist, listbox, and table
LFDanLu Apr 2, 2025
fc4b727
add test for deselection with modifier and add gridlist tests
LFDanLu Apr 3, 2025
40d7b2f
Merge branch 'test_util_bug_fixes' of github.com:adobe/react-spectrum…
LFDanLu Apr 3, 2025
972e5a2
fix build
LFDanLu Apr 3, 2025
53763bd
add listbox test and fix logic for keyboard selection in utils
LFDanLu Apr 3, 2025
96b4a85
add table util highlight selection tests and add proper keyboard navi…
LFDanLu Apr 3, 2025
94b383c
Merge branch 'main' of github.com:adobe/react-spectrum into highlight…
LFDanLu Apr 14, 2025
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
1 change: 1 addition & 0 deletions packages/@react-aria/test-utils/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
"url": "https://github.com/adobe/react-spectrum"
},
"dependencies": {
"@react-aria/utils": "^3.28.1",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should have a dependency going in this direction, test-utils should never depend on our actual packages
too easy to make a circular dependency

"@swc/helpers": "^0.5.0"
},
"peerDependencies": {
Expand Down
13 changes: 11 additions & 2 deletions packages/@react-aria/test-utils/src/events.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,17 @@
*/

import {act, fireEvent} from '@testing-library/react';
import {isMac} from '@react-aria/utils';
import {UserOpts} from './types';

export const DEFAULT_LONG_PRESS_TIME = 500;
export function getAltKey(): 'Alt' | 'ControlLeft' {
Copy link
Member

Choose a reason for hiding this comment

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

? what does isMac return in a test environment?
I feel like we should hook this into the user config if we need it

return isMac() ? 'Alt' : 'ControlLeft';
}

export function getMetaKey(): 'MetaLeft' | 'ControlLeft' {
return isMac() ? 'MetaLeft' : 'ControlLeft';
}

/**
* Simulates a "long press" event on a element.
Expand Down Expand Up @@ -58,9 +66,10 @@ export async function triggerLongPress(opts: {element: HTMLElement, advanceTimer
}

// Docs cannot handle the types that userEvent actually declares, so hopefully this sub set is okay
export async function pressElement(user: {click: (element: Element) => Promise<void>, keyboard: (keys: string) => Promise<void>, pointer: (opts: {target: Element, keys: string}) => Promise<void>}, element: HTMLElement, interactionType: UserOpts['interactionType']): Promise<void> {
export async function pressElement(user: {click: (element: Element) => Promise<void>, keyboard: (keys: string) => Promise<void>, pointer: (opts: {target: Element, keys: string, coords?: any}) => Promise<void>}, element: HTMLElement, interactionType: UserOpts['interactionType']): Promise<void> {
if (interactionType === 'mouse') {
await user.click(element);
// Add coords with pressure so this isn't detected as a virtual click
await user.pointer({target: element, keys: '[MouseLeft]', coords: {pressure: .5}});
} else if (interactionType === 'keyboard') {
// TODO: For the keyboard flow, I wonder if it would be reasonable to just do fireEvent directly on the obtained row node or if we should
// stick to simulting an actual user's keyboard operations as closely as possible
Expand Down
48 changes: 36 additions & 12 deletions packages/@react-aria/test-utils/src/gridlist.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
*/

import {act, within} from '@testing-library/react';
import {getAltKey, getMetaKey, pressElement, triggerLongPress} from './events';
import {GridListTesterOpts, GridRowActionOpts, ToggleGridRowOpts, UserOpts} from './types';
import {pressElement, triggerLongPress} from './events';

interface GridListToggleRowOpts extends ToggleGridRowOpts {}
interface GridListRowActionOpts extends GridRowActionOpts {}
Expand Down Expand Up @@ -57,20 +57,21 @@ export class GridListTester {
}

// TODO: RTL
private async keyboardNavigateToRow(opts: {row: HTMLElement}) {
let {row} = opts;
private async keyboardNavigateToRow(opts: {row: HTMLElement, useAltKey?: boolean}) {
let {row, useAltKey} = opts;
let altKey = getAltKey();
let rows = this.rows;
let targetIndex = rows.indexOf(row);
if (targetIndex === -1) {
throw new Error('Option provided is not in the gridlist');
}

if (document.activeElement !== this._gridlist || !this._gridlist.contains(document.activeElement)) {
if (document.activeElement !== this._gridlist && !this._gridlist.contains(document.activeElement)) {
act(() => this._gridlist.focus());
}

if (document.activeElement === this._gridlist) {
await this.user.keyboard('[ArrowDown]');
await this.user.keyboard(`${useAltKey ? `[${altKey}>]` : ''}[ArrowDown]${useAltKey ? `[/${altKey}]` : ''}`);
} else if (this._gridlist.contains(document.activeElement) && document.activeElement!.getAttribute('role') !== 'row') {
do {
await this.user.keyboard('[ArrowLeft]');
Expand All @@ -82,22 +83,34 @@ export class GridListTester {
}
let direction = targetIndex > currIndex ? 'down' : 'up';

if (useAltKey) {
await this.user.keyboard(`[${altKey}>]`);
}
for (let i = 0; i < Math.abs(targetIndex - currIndex); i++) {
await this.user.keyboard(`[${direction === 'down' ? 'ArrowDown' : 'ArrowUp'}]`);
}
if (useAltKey) {
await this.user.keyboard(`[/${altKey}]`);
}
};

/**
* Toggles the selection for the specified gridlist row. Defaults to using the interaction type set on the gridlist tester.
* Note that this will endevor to always add/remove JUST the provided row to the set of selected rows.
*/
async toggleRowSelection(opts: GridListToggleRowOpts): Promise<void> {
let {
row,
needsLongPress,
checkboxSelection = true,
interactionType = this._interactionType
interactionType = this._interactionType,
// TODO: perhaps this should just be shouldUseModifierKeys?
selectionBehavior = 'toggle'
Comment on lines +107 to +108
Copy link
Member Author

Choose a reason for hiding this comment

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

This API naming feels unclear TBH, people may be confused as to when they would apply it. When set to selectionBehavior="replace" this causes the alt/meta key to be used when keyboard navigating/pressing to select a row so that it doesn't cause your previous selection to be replaced on focus/click. I wasn't certain if something like shouldUseModifierKeys was clear either since a user may not know what the "modifier keys" even do

Copy link
Member

Choose a reason for hiding this comment

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

I think selectionBehavior is fine or gridListSelectionBehavior if you want to be clear it should match, then the user profiles/interactions should handle most things behind the scenes

} = opts;

let altKey = getAltKey();
let metaKey = getMetaKey();

if (typeof row === 'string' || typeof row === 'number') {
row = this.findRow({rowIndexOrText: row});
}
Expand All @@ -116,9 +129,15 @@ export class GridListTester {

// this would be better than the check to do nothing in events.ts
// also, it'd be good to be able to trigger selection on the row instead of having to go to the checkbox directly
if (interactionType === 'keyboard' && !checkboxSelection) {
await this.keyboardNavigateToRow({row});
await this.user.keyboard('{Space}');
if (interactionType === 'keyboard' && (!checkboxSelection || !rowCheckbox)) {
await this.keyboardNavigateToRow({row, useAltKey: selectionBehavior === 'replace'});
if (selectionBehavior === 'replace') {
await this.user.keyboard(`[${altKey}>]`);
}
await this.user.keyboard('[Space]');
if (selectionBehavior === 'replace') {
await this.user.keyboard(`[/${altKey}]`);
}
return;
}
if (rowCheckbox && checkboxSelection) {
Expand All @@ -132,9 +151,14 @@ export class GridListTester {

// Note that long press interactions with rows is strictly touch only for grid rows
await triggerLongPress({element: cell, advanceTimer: this._advanceTimer, pointerOpts: {pointerType: 'touch'}});

} else {
await pressElement(this.user, cell, interactionType);
if (selectionBehavior === 'replace' && interactionType !== 'touch') {
await this.user.keyboard(`[${metaKey}>]`);
}
await pressElement(this.user, row, interactionType);
if (selectionBehavior === 'replace' && interactionType !== 'touch') {
await this.user.keyboard(`[/${metaKey}]`);
}
}
}
}
Expand Down Expand Up @@ -166,7 +190,7 @@ export class GridListTester {
return;
}

await this.keyboardNavigateToRow({row});
await this.keyboardNavigateToRow({row, useAltKey: true});
await this.user.keyboard('[Enter]');
} else {
await pressElement(this.user, row, interactionType);
Expand Down
67 changes: 48 additions & 19 deletions packages/@react-aria/test-utils/src/listbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
*/

import {act, within} from '@testing-library/react';
import {getAltKey, getMetaKey, pressElement, triggerLongPress} from './events';
import {ListBoxTesterOpts, UserOpts} from './types';
import {pressElement, triggerLongPress} from './events';

interface ListBoxToggleOptionOpts {
/**
Expand All @@ -31,7 +31,16 @@ interface ListBoxToggleOptionOpts {
/**
* Whether the option needs to be long pressed to be selected. Depends on the listbox's implementation.
*/
needsLongPress?: boolean
needsLongPress?: boolean,
/**
* Whether the listbox has a selectionBehavior of "toggle" or "replace" (aka highlight selection). This affects the user operations
* required to toggle option selection by adding modifier keys during user actions, useful when performing multi-option selection in a "selectionBehavior: 'replace'" listbox.
* If you would like to still simulate user actions (aka press) without these modifiers keys for a "selectionBehavior: replace" listbox, simply omit this option.
* See the [RAC Listbox docs](https://react-spectrum.adobe.com/react-aria/ListBox.html#selection-behavior) for more info on this behavior.
*
* @default 'toggle'
*/
selectionBehavior?: 'toggle' | 'replace'
}

interface ListBoxOptionActionOpts extends Omit<ListBoxToggleOptionOpts, 'keyboardActivation' | 'needsLongPress'> {
Expand Down Expand Up @@ -85,44 +94,52 @@ export class ListBoxTester {

// TODO: this is basically the same as menu except for the error message, refactor later so that they share
// TODO: this also doesn't support grid layout yet
private async keyboardNavigateToOption(opts: {option: HTMLElement}) {
let {option} = opts;
private async keyboardNavigateToOption(opts: {option: HTMLElement, useAltKey?: boolean}) {
let {option, useAltKey} = opts;
let altKey = getAltKey();
let options = this.options();
let targetIndex = options.indexOf(option);
if (targetIndex === -1) {
throw new Error('Option provided is not in the listbox');
}

if (document.activeElement !== this._listbox || !this._listbox.contains(document.activeElement)) {
if (document.activeElement !== this._listbox && !this._listbox.contains(document.activeElement)) {
act(() => this._listbox.focus());
}

await this.user.keyboard('[ArrowDown]');

// TODO: not sure about doing same while loop that exists in other implementations of keyboardNavigateToOption,
// feels like it could break easily
if (document.activeElement?.getAttribute('role') !== 'option') {
await act(async () => {
option.focus();
});
await this.user.keyboard(`${useAltKey ? `[${altKey}>]` : ''}[ArrowDown]${useAltKey ? `[/${altKey}]` : ''}`);
}

let currIndex = options.indexOf(document.activeElement as HTMLElement);
if (currIndex === -1) {
throw new Error('ActiveElement is not in the listbox');
}
let direction = targetIndex > currIndex ? 'down' : 'up';

let direction = targetIndex > currIndex ? 'down' : 'up';
if (useAltKey) {
await this.user.keyboard(`[${altKey}>]`);
}
for (let i = 0; i < Math.abs(targetIndex - currIndex); i++) {
await this.user.keyboard(`[${direction === 'down' ? 'ArrowDown' : 'ArrowUp'}]`);
}
if (useAltKey) {
await this.user.keyboard(`[/${altKey}]`);
}
};

/**
* Toggles the selection for the specified listbox option. Defaults to using the interaction type set on the listbox tester.
*/
async toggleOptionSelection(opts: ListBoxToggleOptionOpts): Promise<void> {
let {option, needsLongPress, keyboardActivation = 'Enter', interactionType = this._interactionType} = opts;
let {
option,
needsLongPress,
keyboardActivation = 'Enter',
interactionType = this._interactionType,
// TODO: perhaps this should just be shouldUseModifierKeys?
selectionBehavior = 'toggle'
} = opts;

let altKey = getAltKey();
let metaKey = getMetaKey();

if (typeof option === 'string' || typeof option === 'number') {
option = this.findOption({optionIndexOrText: option});
Expand All @@ -137,8 +154,14 @@ export class ListBoxTester {
return;
}

await this.keyboardNavigateToOption({option});
await this.keyboardNavigateToOption({option, useAltKey: selectionBehavior === 'replace'});
if (selectionBehavior === 'replace') {
await this.user.keyboard(`[${altKey}>]`);
}
await this.user.keyboard(`[${keyboardActivation}]`);
if (selectionBehavior === 'replace') {
await this.user.keyboard(`[/${altKey}]`);
}
} else {
if (needsLongPress && interactionType === 'touch') {
if (this._advanceTimer == null) {
Expand All @@ -147,7 +170,13 @@ export class ListBoxTester {

await triggerLongPress({element: option, advanceTimer: this._advanceTimer, pointerOpts: {pointerType: 'touch'}});
} else {
if (selectionBehavior === 'replace' && interactionType !== 'touch') {
await this.user.keyboard(`[${metaKey}>]`);
}
await pressElement(this.user, option, interactionType);
if (selectionBehavior === 'replace' && interactionType !== 'touch') {
await this.user.keyboard(`[/${metaKey}]`);
}
}
}
}
Expand Down Expand Up @@ -177,7 +206,7 @@ export class ListBoxTester {
return;
}

await this.keyboardNavigateToOption({option});
await this.keyboardNavigateToOption({option, useAltKey: true});
await this.user.keyboard('[Enter]');
} else {
await pressElement(this.user, option, interactionType);
Expand Down
2 changes: 1 addition & 1 deletion packages/@react-aria/test-utils/src/menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ export class MenuTester {
return;
}

if (document.activeElement !== menu || !menu.contains(document.activeElement)) {
if (document.activeElement !== menu && !menu.contains(document.activeElement)) {
act(() => menu.focus());
}

Expand Down
2 changes: 1 addition & 1 deletion packages/@react-aria/test-utils/src/select.ts
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ export class SelectTester {
return;
}

if (document.activeElement !== listbox || !listbox.contains(document.activeElement)) {
if (document.activeElement !== listbox && !listbox.contains(document.activeElement)) {
act(() => listbox.focus());
}
await this.keyboardNavigateToOption({option});
Expand Down
Loading