From 3580959940c7efacf17c73e9d8e9712da5daf9a7 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Thu, 10 Mar 2022 17:14:27 -0800 Subject: [PATCH 1/9] initial work to focus the row instead of the cell --- .../grid/src/GridKeyboardDelegate.ts | 23 +++++++++++++++---- packages/@react-aria/grid/src/useGridCell.ts | 7 ++++-- .../selection/src/useSelectableCollection.ts | 4 ++++ .../@react-spectrum/list/src/ListView.tsx | 6 ++--- .../@react-spectrum/list/src/ListViewItem.tsx | 15 +++++++----- 5 files changed, 38 insertions(+), 17 deletions(-) diff --git a/packages/@react-aria/grid/src/GridKeyboardDelegate.ts b/packages/@react-aria/grid/src/GridKeyboardDelegate.ts index cd9aa9cb089..6aa8d978a70 100644 --- a/packages/@react-aria/grid/src/GridKeyboardDelegate.ts +++ b/packages/@react-aria/grid/src/GridKeyboardDelegate.ts @@ -22,7 +22,10 @@ export interface GridKeyboardDelegateOptions { direction: Direction, collator?: Intl.Collator, layout?: Layout>, - focusMode?: 'row' | 'cell' + focusMode?: 'row' | 'cell', + // TODO: whether to prioritize focusing the row over the cell? Really this mode is for ListView where we + // either want to focus the row or the child elements of the cell, but never the cell itself + skipCell?: boolean } export class GridKeyboardDelegate> implements KeyboardDelegate { @@ -42,8 +45,13 @@ export class GridKeyboardDelegate> implements Key this.collator = options.collator; this.layout = options.layout; this.focusMode = options.focusMode || 'row'; + // TODO: temp hack to make up/down arrow focus the row instead of trying to focus + // the same cell contents of the above/below row + this.skipCell = true; } + // TODO: Fix Home, End, Page Down, and Page Up for ListView. It should work if you are focused on the row + // or within the cell protected isCell(node: Node) { return node.type === 'cell'; } @@ -97,7 +105,7 @@ export class GridKeyboardDelegate> implements Key key = this.findNextKey(key); if (key != null) { // If focus was on a cell, focus the cell with the same index in the next row. - if (this.isCell(startItem)) { + if (!this.skipCell && this.isCell(startItem)) { let item = this.collection.getItem(key); return [...item.childNodes][startItem.index].key; } @@ -124,7 +132,7 @@ export class GridKeyboardDelegate> implements Key key = this.findPreviousKey(key); if (key != null) { // If focus was on a cell, focus the cell with the same index in the previous row. - if (this.isCell(startItem)) { + if (!this.skipCell && this.isCell(startItem)) { let item = this.collection.getItem(key); return [...item.childNodes][startItem.index].key; } @@ -247,7 +255,7 @@ export class GridKeyboardDelegate> implements Key // If global flag is not set, and a cell is currently focused, // move focus to the last cell in the parent row. - if (this.isCell(item) && !global) { + if (this.isCell(item) && !global && !this.skipCell) { let parent = this.collection.getItem(item.parentKey); let children = [...parent.childNodes]; return children[children.length - 1].key; @@ -299,7 +307,10 @@ export class GridKeyboardDelegate> implements Key return this.ref?.current?.scrollHeight; } + // TODO: check if the key is a cell key or a row key. Should always use the + // row key? Need to debug getKeyPageAbove(key: Key) { + console.log('key in getPageAbovec', key) let itemRect = this.getItemRect(key); if (!itemRect) { return null; @@ -315,7 +326,10 @@ export class GridKeyboardDelegate> implements Key return key; } + // TODO: check if the key is a cell key or a row key. Should always use the + // row key? Need to debug getKeyPageBelow(key: Key) { + console.log('key in getPageBelow', key) let itemRect = this.getItemRect(key); if (!itemRect) { @@ -381,4 +395,3 @@ export class GridKeyboardDelegate> implements Key return null; } } - diff --git a/packages/@react-aria/grid/src/useGridCell.ts b/packages/@react-aria/grid/src/useGridCell.ts index 285f0c96aaa..a0d4447b739 100644 --- a/packages/@react-aria/grid/src/useGridCell.ts +++ b/packages/@react-aria/grid/src/useGridCell.ts @@ -64,13 +64,15 @@ export function useGridCell>(props: GridCellProps // Handles focusing the cell. If there is a focusable child, // it is focused, otherwise the cell itself is focused. + // TODO: this focus logic will coerce focus to the first focusable child when clicking on a listview's row child element let focus = () => { let treeWalker = getFocusableTreeWalker(ref.current); if (focusMode === 'child') { let focusable = state.selectionManager.childFocusStrategy === 'last' ? last(treeWalker) : treeWalker.firstChild() as HTMLElement; - if (focusable) { + // TODO: only move focus to the child within the cell if focus isn't already within the cell (e.g. clicking on a buton within a listview row) + if (focusable && !ref.current.contains(document.activeElement) && ref.current !== document.activeElement) { focusSafely(focusable); return; } @@ -98,7 +100,7 @@ export function useGridCell>(props: GridCellProps let walker = getFocusableTreeWalker(ref.current); walker.currentNode = document.activeElement; - + console.log('gwaegaweg') switch (e.key) { case 'ArrowLeft': { // Find the next focusable element within the cell. @@ -213,6 +215,7 @@ export function useGridCell>(props: GridCellProps // up to the tree, and move focus to a focusable child if possible. requestAnimationFrame(() => { if (focusMode === 'child' && document.activeElement === ref.current) { + console.log('blah') focus(); } }); diff --git a/packages/@react-aria/selection/src/useSelectableCollection.ts b/packages/@react-aria/selection/src/useSelectableCollection.ts index c78b28b5be6..787864727e9 100644 --- a/packages/@react-aria/selection/src/useSelectableCollection.ts +++ b/packages/@react-aria/selection/src/useSelectableCollection.ts @@ -169,10 +169,12 @@ export function useSelectableCollection(options: SelectableCollectionOptions): S break; } case 'ArrowRight': { + console.log('arrow right') if (delegate.getKeyRightOf) { e.preventDefault(); let nextKey = delegate.getKeyRightOf(manager.focusedKey); navigateToKey(nextKey, direction === 'rtl' ? 'last' : 'first'); + console.log('next key arrow right', nextKey) } break; } @@ -201,10 +203,12 @@ export function useSelectableCollection(options: SelectableCollectionOptions): S } break; case 'PageDown': + console.log('page down') if (delegate.getKeyPageBelow) { e.preventDefault(); let nextKey = delegate.getKeyPageBelow(manager.focusedKey); navigateToKey(nextKey); + console.log('nextkey', nextKey) } break; case 'PageUp': diff --git a/packages/@react-spectrum/list/src/ListView.tsx b/packages/@react-spectrum/list/src/ListView.tsx index 2fe11fd306a..6673c8ea3e4 100644 --- a/packages/@react-spectrum/list/src/ListView.tsx +++ b/packages/@react-spectrum/list/src/ListView.tsx @@ -141,7 +141,7 @@ function ListView(props: ListViewProps, ref: DOMRef(props: ListViewProps, ref: DOMRef + {...mergedProps} + ref={rowRef} + aria-label={item.textValue}>
+ {...gridCellProps}> {isListDraggable &&
From e51bd3f37599e9757ec06fd419643259b5feffd0 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Tue, 29 Mar 2022 17:01:23 -0700 Subject: [PATCH 2/9] cleanup --- .../grid/src/GridKeyboardDelegate.ts | 17 ++++------------- packages/@react-aria/grid/src/useGridCell.ts | 2 -- .../selection/src/useSelectableCollection.ts | 4 ---- packages/@react-spectrum/list/src/ListView.tsx | 3 ++- 4 files changed, 6 insertions(+), 20 deletions(-) diff --git a/packages/@react-aria/grid/src/GridKeyboardDelegate.ts b/packages/@react-aria/grid/src/GridKeyboardDelegate.ts index 6aa8d978a70..e7ccfa4348b 100644 --- a/packages/@react-aria/grid/src/GridKeyboardDelegate.ts +++ b/packages/@react-aria/grid/src/GridKeyboardDelegate.ts @@ -23,8 +23,8 @@ export interface GridKeyboardDelegateOptions { collator?: Intl.Collator, layout?: Layout>, focusMode?: 'row' | 'cell', - // TODO: whether to prioritize focusing the row over the cell? Really this mode is for ListView where we - // either want to focus the row or the child elements of the cell, but never the cell itself + // TODO: whether to prioritize focusing the row over the cell? Specifically for Up/Down where we always want to move focus + // to the row skipCell?: boolean } @@ -36,6 +36,7 @@ export class GridKeyboardDelegate> implements Key protected collator: Intl.Collator; protected layout: Layout>; protected focusMode; + protected skipCell; constructor(options: GridKeyboardDelegateOptions) { this.collection = options.collection; @@ -45,13 +46,9 @@ export class GridKeyboardDelegate> implements Key this.collator = options.collator; this.layout = options.layout; this.focusMode = options.focusMode || 'row'; - // TODO: temp hack to make up/down arrow focus the row instead of trying to focus - // the same cell contents of the above/below row - this.skipCell = true; + this.skipCell = options.skipCell || false; } - // TODO: Fix Home, End, Page Down, and Page Up for ListView. It should work if you are focused on the row - // or within the cell protected isCell(node: Node) { return node.type === 'cell'; } @@ -307,10 +304,7 @@ export class GridKeyboardDelegate> implements Key return this.ref?.current?.scrollHeight; } - // TODO: check if the key is a cell key or a row key. Should always use the - // row key? Need to debug getKeyPageAbove(key: Key) { - console.log('key in getPageAbovec', key) let itemRect = this.getItemRect(key); if (!itemRect) { return null; @@ -326,10 +320,7 @@ export class GridKeyboardDelegate> implements Key return key; } - // TODO: check if the key is a cell key or a row key. Should always use the - // row key? Need to debug getKeyPageBelow(key: Key) { - console.log('key in getPageBelow', key) let itemRect = this.getItemRect(key); if (!itemRect) { diff --git a/packages/@react-aria/grid/src/useGridCell.ts b/packages/@react-aria/grid/src/useGridCell.ts index a0d4447b739..816b6a8567d 100644 --- a/packages/@react-aria/grid/src/useGridCell.ts +++ b/packages/@react-aria/grid/src/useGridCell.ts @@ -100,7 +100,6 @@ export function useGridCell>(props: GridCellProps let walker = getFocusableTreeWalker(ref.current); walker.currentNode = document.activeElement; - console.log('gwaegaweg') switch (e.key) { case 'ArrowLeft': { // Find the next focusable element within the cell. @@ -215,7 +214,6 @@ export function useGridCell>(props: GridCellProps // up to the tree, and move focus to a focusable child if possible. requestAnimationFrame(() => { if (focusMode === 'child' && document.activeElement === ref.current) { - console.log('blah') focus(); } }); diff --git a/packages/@react-aria/selection/src/useSelectableCollection.ts b/packages/@react-aria/selection/src/useSelectableCollection.ts index 787864727e9..c78b28b5be6 100644 --- a/packages/@react-aria/selection/src/useSelectableCollection.ts +++ b/packages/@react-aria/selection/src/useSelectableCollection.ts @@ -169,12 +169,10 @@ export function useSelectableCollection(options: SelectableCollectionOptions): S break; } case 'ArrowRight': { - console.log('arrow right') if (delegate.getKeyRightOf) { e.preventDefault(); let nextKey = delegate.getKeyRightOf(manager.focusedKey); navigateToKey(nextKey, direction === 'rtl' ? 'last' : 'first'); - console.log('next key arrow right', nextKey) } break; } @@ -203,12 +201,10 @@ export function useSelectableCollection(options: SelectableCollectionOptions): S } break; case 'PageDown': - console.log('page down') if (delegate.getKeyPageBelow) { e.preventDefault(); let nextKey = delegate.getKeyPageBelow(manager.focusedKey); navigateToKey(nextKey); - console.log('nextkey', nextKey) } break; case 'PageUp': diff --git a/packages/@react-spectrum/list/src/ListView.tsx b/packages/@react-spectrum/list/src/ListView.tsx index 6673c8ea3e4..15ae86dc023 100644 --- a/packages/@react-spectrum/list/src/ListView.tsx +++ b/packages/@react-spectrum/list/src/ListView.tsx @@ -151,7 +151,8 @@ function ListView(props: ListViewProps, ref: DOMRef Date: Tue, 29 Mar 2022 17:44:49 -0700 Subject: [PATCH 3/9] tentative approach to always skip focusing the cell --- .../grid/src/GridKeyboardDelegate.ts | 1 + packages/@react-aria/grid/src/useGridCell.ts | 17 +++++++++++++---- .../@react-spectrum/list/src/ListViewItem.tsx | 3 ++- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/packages/@react-aria/grid/src/GridKeyboardDelegate.ts b/packages/@react-aria/grid/src/GridKeyboardDelegate.ts index e7ccfa4348b..c1d3654738c 100644 --- a/packages/@react-aria/grid/src/GridKeyboardDelegate.ts +++ b/packages/@react-aria/grid/src/GridKeyboardDelegate.ts @@ -25,6 +25,7 @@ export interface GridKeyboardDelegateOptions { focusMode?: 'row' | 'cell', // TODO: whether to prioritize focusing the row over the cell? Specifically for Up/Down where we always want to move focus // to the row + // TODO: check naming convention skipCell?: boolean } diff --git a/packages/@react-aria/grid/src/useGridCell.ts b/packages/@react-aria/grid/src/useGridCell.ts index 816b6a8567d..04d99a0294d 100644 --- a/packages/@react-aria/grid/src/useGridCell.ts +++ b/packages/@react-aria/grid/src/useGridCell.ts @@ -35,7 +35,9 @@ interface GridCellProps { * Please use onCellAction at the collection level instead. * @deprecated **/ - onAction?: () => void + onAction?: () => void, + // TODO: check naming convention + skipCell?: boolean } interface GridCellAria { @@ -56,7 +58,8 @@ export function useGridCell>(props: GridCellProps isVirtualized, focusMode = 'child', shouldSelectOnPressUp, - onAction + onAction, + skipCell } = props; let {direction} = useLocale(); @@ -67,7 +70,7 @@ export function useGridCell>(props: GridCellProps // TODO: this focus logic will coerce focus to the first focusable child when clicking on a listview's row child element let focus = () => { let treeWalker = getFocusableTreeWalker(ref.current); - if (focusMode === 'child') { + if (focusMode === 'child' || skipCell) { let focusable = state.selectionManager.childFocusStrategy === 'last' ? last(treeWalker) : treeWalker.firstChild() as HTMLElement; @@ -78,8 +81,14 @@ export function useGridCell>(props: GridCellProps } } - if (!ref.current.contains(document.activeElement)) { + if (!ref.current.contains(document.activeElement) && !skipCell) { focusSafely(ref.current); + return; + } + + // TODO: If parent key exists and skipCell is true, set the focusedKey to be the parentKey + if (node.parentKey != null && skipCell) { + state.selectionManager.setFocusedKey(node.parentKey); } }; diff --git a/packages/@react-spectrum/list/src/ListViewItem.tsx b/packages/@react-spectrum/list/src/ListViewItem.tsx index b2d3e6f96cd..ab0eb2ed0ac 100644 --- a/packages/@react-spectrum/list/src/ListViewItem.tsx +++ b/packages/@react-spectrum/list/src/ListViewItem.tsx @@ -56,7 +56,8 @@ export function ListViewItem(props) { shouldSelectOnPressUp: isListDraggable }, state, rowRef); let {gridCellProps} = useGridCell({ - node: cellNode + node: cellNode, + skipCell: true }, state, cellRef); let draggableItem: DraggableItemResult; if (isListDraggable) { From 2216e982032e45f0cc59ad32d56188e0026418fb Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Wed, 30 Mar 2022 15:53:58 -0700 Subject: [PATCH 4/9] retaining focus on Listview focusable child when it is clicked --- packages/@react-aria/grid/src/useGridCell.ts | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/packages/@react-aria/grid/src/useGridCell.ts b/packages/@react-aria/grid/src/useGridCell.ts index 04d99a0294d..7a489f403a1 100644 --- a/packages/@react-aria/grid/src/useGridCell.ts +++ b/packages/@react-aria/grid/src/useGridCell.ts @@ -66,16 +66,22 @@ export function useGridCell>(props: GridCellProps let {keyboardDelegate, actions: {onCellAction}} = gridMap.get(state); // Handles focusing the cell. If there is a focusable child, - // it is focused, otherwise the cell itself is focused. - // TODO: this focus logic will coerce focus to the first focusable child when clicking on a listview's row child element + // it is focused, otherwise the cell itself is focused. If skipCell is + // true, always attempt to put focus on a focusable child if any or back on the parent + // row. let focus = () => { let treeWalker = getFocusableTreeWalker(ref.current); if (focusMode === 'child' || skipCell) { + // If focus is already on a focusable child within the cell, early return so we don't shift focus + if (ref.current.contains(document.activeElement) && ref.current !== document.activeElement) { + return; + } + let focusable = state.selectionManager.childFocusStrategy === 'last' ? last(treeWalker) : treeWalker.firstChild() as HTMLElement; - // TODO: only move focus to the child within the cell if focus isn't already within the cell (e.g. clicking on a buton within a listview row) - if (focusable && !ref.current.contains(document.activeElement) && ref.current !== document.activeElement) { + + if (focusable) { focusSafely(focusable); return; } @@ -86,7 +92,6 @@ export function useGridCell>(props: GridCellProps return; } - // TODO: If parent key exists and skipCell is true, set the focusedKey to be the parentKey if (node.parentKey != null && skipCell) { state.selectionManager.setFocusedKey(node.parentKey); } From dac22a85c8608c8248db63f72041812c8b4b9edd Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Wed, 30 Mar 2022 16:27:46 -0700 Subject: [PATCH 5/9] making ListView specific keyboard delegate Felt that the previous modifications to GridKeyboardDelegate were too specific to ListView so split out the changes. Also fixes pageUp/Down/Home/End when you are focused on the ListView row (we forgot to pass the layout to the delegate previously) --- .../grid/src/GridKeyboardDelegate.ts | 13 ++--- packages/@react-aria/list/README.md | 3 + packages/@react-aria/list/index.ts | 13 +++++ packages/@react-aria/list/package.json | 31 ++++++++++ .../list/src/ListGridKeyboardDelegate.ts | 56 +++++++++++++++++++ packages/@react-aria/list/src/index.ts | 13 +++++ packages/@react-spectrum/list/package.json | 1 + .../@react-spectrum/list/src/ListView.tsx | 10 ++-- 8 files changed, 126 insertions(+), 14 deletions(-) create mode 100644 packages/@react-aria/list/README.md create mode 100644 packages/@react-aria/list/index.ts create mode 100644 packages/@react-aria/list/package.json create mode 100644 packages/@react-aria/list/src/ListGridKeyboardDelegate.ts create mode 100644 packages/@react-aria/list/src/index.ts diff --git a/packages/@react-aria/grid/src/GridKeyboardDelegate.ts b/packages/@react-aria/grid/src/GridKeyboardDelegate.ts index c1d3654738c..65c51e7888e 100644 --- a/packages/@react-aria/grid/src/GridKeyboardDelegate.ts +++ b/packages/@react-aria/grid/src/GridKeyboardDelegate.ts @@ -22,11 +22,7 @@ export interface GridKeyboardDelegateOptions { direction: Direction, collator?: Intl.Collator, layout?: Layout>, - focusMode?: 'row' | 'cell', - // TODO: whether to prioritize focusing the row over the cell? Specifically for Up/Down where we always want to move focus - // to the row - // TODO: check naming convention - skipCell?: boolean + focusMode?: 'row' | 'cell' } export class GridKeyboardDelegate> implements KeyboardDelegate { @@ -47,7 +43,6 @@ export class GridKeyboardDelegate> implements Key this.collator = options.collator; this.layout = options.layout; this.focusMode = options.focusMode || 'row'; - this.skipCell = options.skipCell || false; } protected isCell(node: Node) { @@ -103,7 +98,7 @@ export class GridKeyboardDelegate> implements Key key = this.findNextKey(key); if (key != null) { // If focus was on a cell, focus the cell with the same index in the next row. - if (!this.skipCell && this.isCell(startItem)) { + if (this.isCell(startItem)) { let item = this.collection.getItem(key); return [...item.childNodes][startItem.index].key; } @@ -130,7 +125,7 @@ export class GridKeyboardDelegate> implements Key key = this.findPreviousKey(key); if (key != null) { // If focus was on a cell, focus the cell with the same index in the previous row. - if (!this.skipCell && this.isCell(startItem)) { + if (this.isCell(startItem)) { let item = this.collection.getItem(key); return [...item.childNodes][startItem.index].key; } @@ -253,7 +248,7 @@ export class GridKeyboardDelegate> implements Key // If global flag is not set, and a cell is currently focused, // move focus to the last cell in the parent row. - if (this.isCell(item) && !global && !this.skipCell) { + if (this.isCell(item) && !global) { let parent = this.collection.getItem(item.parentKey); let children = [...parent.childNodes]; return children[children.length - 1].key; diff --git a/packages/@react-aria/list/README.md b/packages/@react-aria/list/README.md new file mode 100644 index 00000000000..8c3905bdc98 --- /dev/null +++ b/packages/@react-aria/list/README.md @@ -0,0 +1,3 @@ +# @react-aria/list + +This package is part of [react-spectrum](https://github.com/adobe/react-spectrum). See the repo for more details. diff --git a/packages/@react-aria/list/index.ts b/packages/@react-aria/list/index.ts new file mode 100644 index 00000000000..4e9931530d8 --- /dev/null +++ b/packages/@react-aria/list/index.ts @@ -0,0 +1,13 @@ +/* + * Copyright 2022 Adobe. All rights reserved. + * This file is licensed to you under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. You may obtain a copy + * of the License at http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under + * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS + * OF ANY KIND, either express or implied. See the License for the specific language + * governing permissions and limitations under the License. + */ + +export * from './src'; diff --git a/packages/@react-aria/list/package.json b/packages/@react-aria/list/package.json new file mode 100644 index 00000000000..3ab2c006b86 --- /dev/null +++ b/packages/@react-aria/list/package.json @@ -0,0 +1,31 @@ +{ + "name": "@react-aria/list", + "version": "3.0.0-alpha.1", + "private": true, + "description": "Spectrum UI components in React", + "license": "Apache-2.0", + "main": "dist/main.js", + "module": "dist/module.js", + "types": "dist/types.d.ts", + "source": "src/index.ts", + "files": [ + "dist", + "src" + ], + "sideEffects": false, + "repository": { + "type": "git", + "url": "https://github.com/adobe/react-spectrum" + }, + "dependencies": { + "@babel/runtime": "^7.6.2", + "@react-aria/grid": "^3.2.3", + "@react-types/grid": "^3.0.2" + }, + "peerDependencies": { + "react": "^16.8.0 || ^17.0.0-rc.1" + }, + "publishConfig": { + "access": "public" + } +} diff --git a/packages/@react-aria/list/src/ListGridKeyboardDelegate.ts b/packages/@react-aria/list/src/ListGridKeyboardDelegate.ts new file mode 100644 index 00000000000..998f78fdb0f --- /dev/null +++ b/packages/@react-aria/list/src/ListGridKeyboardDelegate.ts @@ -0,0 +1,56 @@ +/* + * Copyright 2022 Adobe. All rights reserved. + * This file is licensed to you under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. You may obtain a copy + * of the License at http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under + * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS + * OF ANY KIND, either express or implied. See the License for the specific language + * governing permissions and limitations under the License. + */ + +import {GridCollection} from '@react-types/grid'; +import {GridKeyboardDelegate, GridKeyboardDelegateOptions} from '@react-aria/grid'; +import {Key} from 'react'; + +// TODO: Open to feedback about name, ListKeyboardDelegate already exists +export class ListGridKeyboardDelegate extends GridKeyboardDelegate> { + constructor(options: Omit>, 'focusMode'>) { + super({...options, focusMode: 'row'}); + } + + // TODO: think about whether or not focus should be moved to the row or to the children in the cell + // Feels kinda weird to move it to the first child in the cell when focus used to be on the last child in the + // cell above... + getKeyBelow(key: Key) { + let startItem = this.collection.getItem(key); + if (!startItem) { + return; + } + + // If focus was on a cell, start searching from the parent row + if (this.isCell(startItem)) { + key = startItem.parentKey; + } + + return this.findNextKey(key); + } + + getKeyAbove(key: Key) { + let startItem = this.collection.getItem(key); + if (!startItem) { + return; + } + + // If focus was on a cell, start searching from the parent row + if (this.isCell(startItem)) { + key = startItem.parentKey; + } + + return this.findPreviousKey(key); + } + + + // TODO: double check if getLastKey needs to be overridden here as well +} diff --git a/packages/@react-aria/list/src/index.ts b/packages/@react-aria/list/src/index.ts new file mode 100644 index 00000000000..505d9903d9a --- /dev/null +++ b/packages/@react-aria/list/src/index.ts @@ -0,0 +1,13 @@ +/* + * Copyright 2022 Adobe. All rights reserved. + * This file is licensed to you under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. You may obtain a copy + * of the License at http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under + * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS + * OF ANY KIND, either express or implied. See the License for the specific language + * governing permissions and limitations under the License. + */ + +export * from './ListGridKeyboardDelegate'; diff --git a/packages/@react-spectrum/list/package.json b/packages/@react-spectrum/list/package.json index f698114007a..b2140249000 100644 --- a/packages/@react-spectrum/list/package.json +++ b/packages/@react-spectrum/list/package.json @@ -37,6 +37,7 @@ "@react-aria/grid": "^3.2.4", "@react-aria/i18n": "^3.3.7", "@react-aria/interactions": "^3.8.2", + "@react-aria/list": "3.0.0-alpha.1", "@react-aria/listbox": "^3.4.3", "@react-aria/separator": "^3.1.6", "@react-aria/utils": "^3.11.3", diff --git a/packages/@react-spectrum/list/src/ListView.tsx b/packages/@react-spectrum/list/src/ListView.tsx index 15ae86dc023..daf317ca225 100644 --- a/packages/@react-spectrum/list/src/ListView.tsx +++ b/packages/@react-spectrum/list/src/ListView.tsx @@ -26,9 +26,9 @@ import {Content} from '@react-spectrum/view'; import type {DraggableCollectionState} from '@react-stately/dnd'; import {DragHooks} from '@react-spectrum/dnd'; import {GridCollection, GridState, useGridState} from '@react-stately/grid'; -import {GridKeyboardDelegate, useGrid, useGridSelectionCheckbox} from '@react-aria/grid'; // @ts-ignore import intlMessages from '../intl/*.json'; +import {ListGridKeyboardDelegate} from '@react-aria/list'; import ListGripper from '@spectrum-icons/ui/ListGripper'; import {ListLayout} from '@react-stately/layout'; import {ListState, useListState} from '@react-stately/list'; @@ -38,11 +38,12 @@ import {ProgressCircle} from '@react-spectrum/progress'; import {Provider, useProvider} from '@react-spectrum/provider'; import React, {ReactElement, useContext, useMemo, useRef} from 'react'; import {useCollator, useLocale, useMessageFormatter} from '@react-aria/i18n'; +import {useGrid, useGridSelectionCheckbox} from '@react-aria/grid'; import {Virtualizer} from '@react-aria/virtualizer'; interface ListViewContextValue { state: GridState>, - keyboardDelegate: GridKeyboardDelegate>, + keyboardDelegate: ListGridKeyboardDelegate>, dragState: DraggableCollectionState, onAction:(key: string) => void, isListDraggable: boolean @@ -145,14 +146,13 @@ function ListView(props: ListViewProps, ref: DOMRef new GridKeyboardDelegate({ + let keyboardDelegate = useMemo(() => new ListGridKeyboardDelegate({ collection: state.collection, disabledKeys: state.disabledKeys, ref: domRef, direction, collator, - focusMode: 'row', - skipCell: true + layout }), [state, domRef, direction, collator]); let provider = useProvider(); From b540f23da03f09d00b26c0bc333d4ff926837e7e Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Wed, 30 Mar 2022 17:12:44 -0700 Subject: [PATCH 6/9] fixing pageUp/Down operations when within the ListView row needed to override getItemRect for the ListView keyboard delegate so that it always uses the row key, namely due to the gridcell keys being unavailable in the listlayout. Changed getItem and getItemRect to protected funcs in GridKeyboardDelegate so I could do that --- .../grid/src/GridKeyboardDelegate.ts | 4 +- packages/@react-aria/list/package.json | 1 + .../list/src/ListGridKeyboardDelegate.ts | 48 +++++++++++++------ .../@react-spectrum/list/src/ListView.tsx | 4 +- 4 files changed, 38 insertions(+), 19 deletions(-) diff --git a/packages/@react-aria/grid/src/GridKeyboardDelegate.ts b/packages/@react-aria/grid/src/GridKeyboardDelegate.ts index 65c51e7888e..89380b252e3 100644 --- a/packages/@react-aria/grid/src/GridKeyboardDelegate.ts +++ b/packages/@react-aria/grid/src/GridKeyboardDelegate.ts @@ -269,11 +269,11 @@ export class GridKeyboardDelegate> implements Key return key; } - private getItem(key: Key): HTMLElement { + protected getItem(key: Key): HTMLElement { return this.ref.current.querySelector(`[data-key="${key}"]`); } - private getItemRect(key: Key): Rect { + protected getItemRect(key: Key): Rect { if (this.layout) { return this.layout.getLayoutInfo(key)?.rect; } diff --git a/packages/@react-aria/list/package.json b/packages/@react-aria/list/package.json index 3ab2c006b86..29ec3a72e86 100644 --- a/packages/@react-aria/list/package.json +++ b/packages/@react-aria/list/package.json @@ -20,6 +20,7 @@ "dependencies": { "@babel/runtime": "^7.6.2", "@react-aria/grid": "^3.2.3", + "@react-stately/virtualizer": "^3.1.7", "@react-types/grid": "^3.0.2" }, "peerDependencies": { diff --git a/packages/@react-aria/list/src/ListGridKeyboardDelegate.ts b/packages/@react-aria/list/src/ListGridKeyboardDelegate.ts index 998f78fdb0f..5467e8c90db 100644 --- a/packages/@react-aria/list/src/ListGridKeyboardDelegate.ts +++ b/packages/@react-aria/list/src/ListGridKeyboardDelegate.ts @@ -13,6 +13,7 @@ import {GridCollection} from '@react-types/grid'; import {GridKeyboardDelegate, GridKeyboardDelegateOptions} from '@react-aria/grid'; import {Key} from 'react'; +import {Rect} from '@react-stately/virtualizer'; // TODO: Open to feedback about name, ListKeyboardDelegate already exists export class ListGridKeyboardDelegate extends GridKeyboardDelegate> { @@ -20,11 +21,8 @@ export class ListGridKeyboardDelegate extends GridKeyboardDelegate extends GridKeyboardDelegate>, - keyboardDelegate: ListGridKeyboardDelegate>, + keyboardDelegate: ListGridKeyboardDelegate, dragState: DraggableCollectionState, onAction:(key: string) => void, isListDraggable: boolean @@ -146,7 +146,7 @@ function ListView(props: ListViewProps, ref: DOMRef new ListGridKeyboardDelegate({ + let keyboardDelegate = useMemo(() => new ListGridKeyboardDelegate({ collection: state.collection, disabledKeys: state.disabledKeys, ref: domRef, From 2ebabe1c4e95dbebdaabc51a1db27dce485231cc Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Thu, 31 Mar 2022 13:56:41 -0700 Subject: [PATCH 7/9] adding tests --- .../grid/src/GridKeyboardDelegate.ts | 1 - packages/@react-aria/grid/src/useGridCell.ts | 2 +- .../list/test/ListView.test.js | 261 +++++++++++++----- .../@react-spectrum/table/test/Table.test.js | 7 + 4 files changed, 202 insertions(+), 69 deletions(-) diff --git a/packages/@react-aria/grid/src/GridKeyboardDelegate.ts b/packages/@react-aria/grid/src/GridKeyboardDelegate.ts index 89380b252e3..ca1c40827b9 100644 --- a/packages/@react-aria/grid/src/GridKeyboardDelegate.ts +++ b/packages/@react-aria/grid/src/GridKeyboardDelegate.ts @@ -33,7 +33,6 @@ export class GridKeyboardDelegate> implements Key protected collator: Intl.Collator; protected layout: Layout>; protected focusMode; - protected skipCell; constructor(options: GridKeyboardDelegateOptions) { this.collection = options.collection; diff --git a/packages/@react-aria/grid/src/useGridCell.ts b/packages/@react-aria/grid/src/useGridCell.ts index 7a489f403a1..30acd58c321 100644 --- a/packages/@react-aria/grid/src/useGridCell.ts +++ b/packages/@react-aria/grid/src/useGridCell.ts @@ -80,7 +80,6 @@ export function useGridCell>(props: GridCellProps let focusable = state.selectionManager.childFocusStrategy === 'last' ? last(treeWalker) : treeWalker.firstChild() as HTMLElement; - if (focusable) { focusSafely(focusable); return; @@ -114,6 +113,7 @@ export function useGridCell>(props: GridCellProps let walker = getFocusableTreeWalker(ref.current); walker.currentNode = document.activeElement; + switch (e.key) { case 'ArrowLeft': { // Find the next focusable element within the cell. diff --git a/packages/@react-spectrum/list/test/ListView.test.js b/packages/@react-spectrum/list/test/ListView.test.js index 7cc95a77879..aed5444ed5f 100644 --- a/packages/@react-spectrum/list/test/ListView.test.js +++ b/packages/@react-spectrum/list/test/ListView.test.js @@ -37,7 +37,7 @@ function pointerEvent(type, opts) { } describe('ListView', function () { - let offsetWidth, offsetHeight; + let offsetWidth, offsetHeight, scrollHeight; let onSelectionChange = jest.fn(); let onAction = jest.fn(); let onDragStart = jest.fn(); @@ -48,10 +48,21 @@ describe('ListView', function () { expect(onSelectionChange).toHaveBeenCalledTimes(1); expect(new Set(onSelectionChange.mock.calls[0][0])).toEqual(new Set(selectedKeys)); }; + let items = [ + {key: 'foo', label: 'Foo'}, + {key: 'bar', label: 'Bar'}, + {key: 'baz', label: 'Baz'} + ]; + + let manyItems = []; + for (let i = 1; i <= 100; i++) { + manyItems.push({id: i, label: 'Foo ' + i}); + } beforeAll(function () { offsetWidth = jest.spyOn(window.HTMLElement.prototype, 'clientWidth', 'get').mockImplementation(() => 1000); offsetHeight = jest.spyOn(window.HTMLElement.prototype, 'clientHeight', 'get').mockImplementation(() => 1000); + scrollHeight = jest.spyOn(window.HTMLElement.prototype, 'scrollHeight', 'get').mockImplementation(() => 40); jest.useFakeTimers(); }); @@ -62,6 +73,7 @@ describe('ListView', function () { afterAll(function () { offsetWidth.mockReset(); offsetHeight.mockReset(); + scrollHeight.mockReset(); }); let render = (children, locale = 'en-US', scale = 'medium') => { @@ -75,10 +87,50 @@ describe('ListView', function () { return tree; }; - let getCell = (tree, text) => { - // Find by text, then go up to the element with the cell role. + let renderList = (props = {}) => { + let { + locale, + scale, + ...otherProps + } = props; + return render( + + {item => ( + + {item.label} + + )} + , + locale, + scale + ); + }; + + let renderListWithFocusables = (props = {}) => { + let { + locale, + scale, + ...otherProps + } = props; + return render( + + {item => ( + + {item.label} + button1 {item.label} + button2 {item.label} + + )} + , + locale, + scale + ); + }; + + let getRow = (tree, text) => { + // Find by text, then go up to the element with the row role. let el = tree.getByText(text); - while (el && !/gridcell|rowheader|columnheader/.test(el.getAttribute('role'))) { + while (el && !/row/.test(el.getAttribute('role'))) { el = el.parentElement; } @@ -114,7 +166,7 @@ describe('ListView', function () { expect(gridCells[0]).toHaveTextContent('Foo'); }); - it('renders a dynamic table', function () { + it('renders a dynamic listview', function () { let items = [ {key: 'foo', label: 'Foo'}, {key: 'bar', label: 'Bar'}, @@ -166,40 +218,35 @@ describe('ListView', function () { expect(gridCells[0]).toHaveTextContent('Foo'); }); - describe('keyboard focus', function () { - let items = [ - {key: 'foo', label: 'Foo'}, - {key: 'bar', label: 'Bar'}, - {key: 'baz', label: 'Baz'} - ]; - let renderList = () => render( - - {item => ( - - {item.label} - - )} - - ); + it('should retain focus on the pressed child', function () { + let tree = renderListWithFocusables(); + let button = within(getRow(tree, 'Foo')).getAllByRole('button')[1]; + act(() => triggerPress(button)); + expect(document.activeElement).toBe(button); + }); - let renderListWithFocusables = (locale, scale) => render( - - {item => ( - - {item.label} - button1 {item.label} - button2 {item.label} - - )} - , - locale, - scale - ); + it('should focus the row if the cell is pressed', function () { + let tree = renderList(); + let cell = within(getRow(tree, 'Bar')).getByRole('gridcell'); + act(() => { + cell.focus(); + jest.runAllTimers(); + }); + expect(document.activeElement).toBe(getRow(tree, 'Bar')); + }); + + it('should have an aria-label on the row for the row text content', function () { + let tree = renderList(); + expect(getRow(tree, 'Foo')).toHaveAttribute('aria-label', 'Foo'); + expect(getRow(tree, 'Bar')).toHaveAttribute('aria-label', 'Bar'); + expect(getRow(tree, 'Baz')).toHaveAttribute('aria-label', 'Baz'); + }); + describe('keyboard focus', function () { describe('Type to select', function () { it('focuses the correct cell when typing', function () { let tree = renderList(); - let target = getCell(tree, 'Baz'); + let target = getRow(tree, 'Baz'); let grid = tree.getByRole('grid'); act(() => grid.focus()); fireEvent.keyDown(grid, {key: 'B'}); @@ -215,7 +262,7 @@ describe('ListView', function () { describe('ArrowRight', function () { it('should not move focus if no focusables present', function () { let tree = renderList(); - let start = getCell(tree, 'Foo'); + let start = getRow(tree, 'Foo'); act(() => start.focus()); moveFocus('ArrowRight'); expect(document.activeElement).toBe(start); @@ -224,8 +271,8 @@ describe('ListView', function () { describe('with cell focusables', function () { it('should move focus to next cell and back to row', function () { let tree = renderListWithFocusables(); - let focusables = within(tree.getAllByRole('row')[0]).getAllByRole('button'); - let start = getCell(tree, 'Foo'); + let start = getRow(tree, 'Foo'); + let focusables = within(start).getAllByRole('button'); act(() => start.focus()); moveFocus('ArrowRight'); expect(document.activeElement).toBe(focusables[0]); @@ -236,12 +283,14 @@ describe('ListView', function () { }); it('should move focus to previous cell in RTL', function () { - let tree = renderListWithFocusables('ar-AE'); + let tree = renderListWithFocusables({locale: 'ar-AE'}); // Should move from button two to button one - let start = within(tree.getAllByRole('row')[0]).getAllByRole('button')[1]; - let end = within(tree.getAllByRole('row')[0]).getAllByRole('button')[0]; - act(() => start.focus()); + let start = within(getRow(tree, 'Foo')).getAllByRole('button')[1]; + let end = within(getRow(tree, 'Foo')).getAllByRole('button')[0]; + // Need to press to set a modality, otherwise useSelectableCollection will think this is a tab operation + act(() => triggerPress(start)); expect(document.activeElement).toHaveTextContent('button2 Foo'); + expect(document.activeElement).toBe(start); moveFocus('ArrowRight'); expect(document.activeElement).toBe(end); expect(document.activeElement).toHaveTextContent('button1 Foo'); @@ -252,7 +301,7 @@ describe('ListView', function () { describe('ArrowLeft', function () { it('should not move focus if no focusables present', function () { let tree = renderList(); - let start = getCell(tree, 'Foo'); + let start = getRow(tree, 'Foo'); act(() => start.focus()); moveFocus('ArrowLeft'); expect(document.activeElement).toBe(start); @@ -261,9 +310,8 @@ describe('ListView', function () { describe('with cell focusables', function () { it('should move focus to previous cell and back to row', function () { let tree = renderListWithFocusables(); - let focusables = within(tree.getAllByRole('row')[0]).getAllByRole('button'); - let start = getCell(tree, 'Foo'); - // console.log('start', start) + let focusables = within(getRow(tree, 'Foo')).getAllByRole('button'); + let start = getRow(tree, 'Foo'); act(() => start.focus()); moveFocus('ArrowLeft'); expect(document.activeElement).toBe(focusables[1]); @@ -274,12 +322,14 @@ describe('ListView', function () { }); it('should move focus to next cell in RTL', function () { - let tree = renderListWithFocusables('ar-AE'); + let tree = renderListWithFocusables({locale: 'ar-AE'}); // Should move from button one to button two - let start = within(tree.getAllByRole('row')[0]).getAllByRole('button')[0]; - let end = within(tree.getAllByRole('row')[0]).getAllByRole('button')[1]; - act(() => start.focus()); + let start = within(getRow(tree, 'Foo')).getAllByRole('button')[0]; + let end = within(getRow(tree, 'Foo')).getAllByRole('button')[1]; + // Need to press to set a modality, otherwise useSelectableCollection will think this is a tab operation + act(() => triggerPress(start)); expect(document.activeElement).toHaveTextContent('button1 Foo'); + expect(document.activeElement).toBe(start); moveFocus('ArrowLeft'); expect(document.activeElement).toBe(end); expect(document.activeElement).toHaveTextContent('button2 Foo'); @@ -288,9 +338,9 @@ describe('ListView', function () { }); describe('ArrowUp', function () { - it('should not change focus from first item', function () { + it('should not wrap focus', function () { let tree = renderListWithFocusables(); - let start = getCell(tree, 'Foo'); + let start = getRow(tree, 'Foo'); act(() => start.focus()); moveFocus('ArrowUp'); expect(document.activeElement).toBe(start); @@ -298,8 +348,8 @@ describe('ListView', function () { it('should move focus to above row', function () { let tree = renderListWithFocusables(); - let start = getCell(tree, 'Bar'); - let end = getCell(tree, 'Foo'); + let start = getRow(tree, 'Bar'); + let end = getRow(tree, 'Foo'); act(() => start.focus()); moveFocus('ArrowUp'); expect(document.activeElement).toBe(end); @@ -307,9 +357,9 @@ describe('ListView', function () { }); describe('ArrowDown', function () { - it('should not change focus from first item', function () { + it('should not wrap focus', function () { let tree = renderListWithFocusables(); - let start = getCell(tree, 'Baz'); + let start = getRow(tree, 'Baz'); act(() => start.focus()); moveFocus('ArrowDown'); expect(document.activeElement).toBe(start); @@ -317,13 +367,97 @@ describe('ListView', function () { it('should move focus to below row', function () { let tree = renderListWithFocusables(); - let start = getCell(tree, 'Foo'); - let end = getCell(tree, 'Bar'); + let start = getRow(tree, 'Foo'); + let end = getRow(tree, 'Bar'); act(() => start.focus()); moveFocus('ArrowDown'); expect(document.activeElement).toBe(end); }); }); + + describe('PageUp', function () { + it('should move focus to a row a page above when focus starts on a row', function () { + let tree = renderListWithFocusables({items: manyItems}); + let start = getRow(tree, 'Foo 25'); + act(() => start.focus()); + moveFocus('PageUp'); + expect(document.activeElement).toBe(getRow(tree, 'Foo 1')); + }); + + it('should move focus to a row a page above when focus starts in the row cell', function () { + let tree = renderListWithFocusables({items: manyItems}); + let focusables = within(getRow(tree, 'Foo 25')).getAllByRole('button'); + let start = focusables[0]; + act(() => triggerPress(start)); + expect(document.activeElement).toBe(start); + moveFocus('PageUp'); + expect(document.activeElement).toBe(getRow(tree, 'Foo 1')); + }); + }); + + describe('PageDown', function () { + it('should move focus to a row a page below when focus starts on a row', function () { + let tree = renderListWithFocusables({items: manyItems}); + let start = getRow(tree, 'Foo 1'); + act(() => start.focus()); + moveFocus('PageDown'); + expect(document.activeElement).toBe(getRow(tree, 'Foo 25')); + moveFocus('PageDown'); + expect(document.activeElement).toBe(getRow(tree, 'Foo 49')); + }); + + it('should move focus to a row a page below when focus starts in the row cell', function () { + let tree = renderListWithFocusables({items: manyItems}); + let focusables = within(getRow(tree, 'Foo 1')).getAllByRole('button'); + let start = focusables[0]; + act(() => triggerPress(start)); + expect(document.activeElement).toBe(start); + moveFocus('PageDown'); + expect(document.activeElement).toBe(getRow(tree, 'Foo 25')); + moveFocus('PageDown'); + expect(document.activeElement).toBe(getRow(tree, 'Foo 49')); + }); + }); + + describe('Home', function () { + it('should move focus to the first row when focus starts on a row', function () { + let tree = renderListWithFocusables({items: manyItems}); + let start = getRow(tree, 'Foo 15'); + act(() => start.focus()); + moveFocus('Home'); + expect(document.activeElement).toBe(getRow(tree, 'Foo 1')); + }); + + it('should move focus to the first row when focus starts in the row cell', function () { + let tree = renderListWithFocusables({items: manyItems}); + let focusables = within(getRow(tree, 'Foo 15')).getAllByRole('button'); + let start = focusables[0]; + act(() => triggerPress(start)); + expect(document.activeElement).toBe(start); + moveFocus('Home'); + expect(document.activeElement).toBe(getRow(tree, 'Foo 1')); + }); + }); + + describe('End', function () { + it('should move focus to the last row when focus starts on a row', function () { + let tree = renderListWithFocusables({items: manyItems}); + let start = getRow(tree, 'Foo 1'); + act(() => start.focus()); + moveFocus('End'); + expect(document.activeElement).toBe(getRow(tree, 'Foo 100')); + }); + + it('should move focus to the last row when focus starts in the row cell', function () { + let tree = renderListWithFocusables({items: manyItems}); + let focusables = within(getRow(tree, 'Foo 1')).getAllByRole('button'); + let start = focusables[0]; + act(() => triggerPress(start)); + expect(document.activeElement).toBe(start); + moveFocus('End'); + expect(document.activeElement).toBe(getRow(tree, 'Foo 100')); + }); + }); }); it('should display loading affordance with proper height (isLoading)', function () { @@ -587,14 +721,7 @@ describe('ListView', function () { }); describe('scrolling', function () { - beforeAll(() => { - jest.spyOn(window.HTMLElement.prototype, 'scrollHeight', 'get') - .mockImplementation(function () { - return 40; - }); - }); - - it('should scroll to a cell when it is focused', function () { + it('should scroll to a row when it is focused', function () { let tree = render( ); + it('should retain focus on the pressed child', function () { + let tree = renderFocusable(); + let switchToPress = tree.getAllByRole('switch')[2]; + act(() => triggerPress(switchToPress)); + expect(document.activeElement).toBe(switchToPress); + }); + it('should marshall focus to the focusable element inside a cell', function () { let tree = renderFocusable(); focusCell(tree, 'Baz 1'); From 2a92a70441f948207f2910d96f88c2e90979ecc6 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Thu, 31 Mar 2022 15:01:21 -0700 Subject: [PATCH 8/9] fixing tests after rebase --- .../@react-spectrum/list/test/ListView.test.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/@react-spectrum/list/test/ListView.test.js b/packages/@react-spectrum/list/test/ListView.test.js index aed5444ed5f..b8f6baf6666 100644 --- a/packages/@react-spectrum/list/test/ListView.test.js +++ b/packages/@react-spectrum/list/test/ListView.test.js @@ -588,13 +588,13 @@ describe('ListView', function () { let rows = tree.getAllByRole('row'); expect(rows[1]).toHaveAttribute('aria-selected', 'false'); expect(rows[2]).toHaveAttribute('aria-selected', 'false'); - act(() => userEvent.click(getCell(tree, 'Bar'), {ctrlKey: true})); + act(() => userEvent.click(getRow(tree, 'Bar'), {ctrlKey: true})); checkSelection(onSelectionChange, ['bar']); expect(rows[1]).toHaveAttribute('aria-selected', 'true'); onSelectionChange.mockClear(); - act(() => userEvent.click(getCell(tree, 'Baz'), {ctrlKey: true})); + act(() => userEvent.click(getRow(tree, 'Baz'), {ctrlKey: true})); checkSelection(onSelectionChange, ['baz']); expect(rows[1]).toHaveAttribute('aria-selected', 'false'); expect(rows[2]).toHaveAttribute('aria-selected', 'true'); @@ -610,13 +610,13 @@ describe('ListView', function () { expect(rows[0]).toHaveAttribute('aria-selected', 'false'); expect(rows[1]).toHaveAttribute('aria-selected', 'false'); expect(rows[2]).toHaveAttribute('aria-selected', 'false'); - act(() => userEvent.click(getCell(tree, 'Foo'), {ctrlKey: true})); + act(() => userEvent.click(getRow(tree, 'Foo'), {ctrlKey: true})); checkSelection(onSelectionChange, ['foo']); expect(rows[0]).toHaveAttribute('aria-selected', 'true'); onSelectionChange.mockClear(); - act(() => userEvent.click(getCell(tree, 'Baz'), {ctrlKey: true})); + act(() => userEvent.click(getRow(tree, 'Baz'), {ctrlKey: true})); checkSelection(onSelectionChange, ['foo', 'baz']); expect(rows[0]).toHaveAttribute('aria-selected', 'true'); expect(rows[1]).toHaveAttribute('aria-selected', 'false'); @@ -632,13 +632,13 @@ describe('ListView', function () { let rows = tree.getAllByRole('row'); expect(rows[1]).toHaveAttribute('aria-selected', 'false'); expect(rows[2]).toHaveAttribute('aria-selected', 'false'); - act(() => userEvent.click(getCell(tree, 'Bar'), {metaKey: true})); + act(() => userEvent.click(getRow(tree, 'Bar'), {metaKey: true})); checkSelection(onSelectionChange, ['bar']); expect(rows[1]).toHaveAttribute('aria-selected', 'true'); onSelectionChange.mockClear(); - act(() => userEvent.click(getCell(tree, 'Baz'), {metaKey: true})); + act(() => userEvent.click(getRow(tree, 'Baz'), {metaKey: true})); checkSelection(onSelectionChange, ['baz']); expect(rows[1]).toHaveAttribute('aria-selected', 'false'); expect(rows[2]).toHaveAttribute('aria-selected', 'true'); @@ -700,7 +700,7 @@ describe('ListView', function () { let row = tree.getAllByRole('row')[1]; expect(row).toHaveAttribute('aria-selected', 'false'); - act(() => userEvent.click(getCell(tree, 'Bar'), {ctrlKey: true})); + act(() => userEvent.click(getRow(tree, 'Bar'), {ctrlKey: true})); checkSelection(onSelectionChange, ['bar']); expect(row).toHaveAttribute('aria-selected', 'true'); From 488f735ba28f22ce0bc52c3ec10bde09cda92ca3 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Fri, 1 Apr 2022 15:40:24 -0700 Subject: [PATCH 9/9] adding missing isVirtualized to get rid of erronous data id --- packages/@react-spectrum/list/src/ListViewItem.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/@react-spectrum/list/src/ListViewItem.tsx b/packages/@react-spectrum/list/src/ListViewItem.tsx index ab0eb2ed0ac..91a6c3cecbb 100644 --- a/packages/@react-spectrum/list/src/ListViewItem.tsx +++ b/packages/@react-spectrum/list/src/ListViewItem.tsx @@ -57,7 +57,8 @@ export function ListViewItem(props) { }, state, rowRef); let {gridCellProps} = useGridCell({ node: cellNode, - skipCell: true + skipCell: true, + isVirtualized: true }, state, cellRef); let draggableItem: DraggableItemResult; if (isListDraggable) {