From 3580959940c7efacf17c73e9d8e9712da5daf9a7 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Thu, 10 Mar 2022 17:14:27 -0800 Subject: [PATCH 01/24] 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 02/24] 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 03/24] 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 04/24] 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 05/24] 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 06/24] 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 07/24] 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 08/24] 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 312aaed646e3b18d96b2c7554ead3955cc737da6 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Fri, 1 Apr 2022 15:19:25 -0700 Subject: [PATCH 09/24] refactor to have outer div behave as both gridcell and grid row still need to fix pageUp/down --- packages/@react-aria/grid/src/useGridCell.ts | 11 +++------ .../list/src/ListGridKeyboardDelegate.ts | 24 ++----------------- .../@react-spectrum/list/src/ListView.tsx | 4 ++-- .../@react-spectrum/list/src/ListViewItem.tsx | 15 ++++++++---- 4 files changed, 18 insertions(+), 36 deletions(-) diff --git a/packages/@react-aria/grid/src/useGridCell.ts b/packages/@react-aria/grid/src/useGridCell.ts index 30acd58c321..2dd47b1dff5 100644 --- a/packages/@react-aria/grid/src/useGridCell.ts +++ b/packages/@react-aria/grid/src/useGridCell.ts @@ -58,8 +58,7 @@ export function useGridCell>(props: GridCellProps isVirtualized, focusMode = 'child', shouldSelectOnPressUp, - onAction, - skipCell + onAction } = props; let {direction} = useLocale(); @@ -71,7 +70,7 @@ export function useGridCell>(props: GridCellProps // row. let focus = () => { let treeWalker = getFocusableTreeWalker(ref.current); - if (focusMode === 'child' || skipCell) { + if (focusMode === 'child') { // 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; @@ -86,14 +85,10 @@ export function useGridCell>(props: GridCellProps } } - if (!ref.current.contains(document.activeElement) && !skipCell) { + if (!ref.current.contains(document.activeElement)) { focusSafely(ref.current); return; } - - if (node.parentKey != null && skipCell) { - state.selectionManager.setFocusedKey(node.parentKey); - } }; let {itemProps, isPressed} = useSelectableItem({ diff --git a/packages/@react-aria/list/src/ListGridKeyboardDelegate.ts b/packages/@react-aria/list/src/ListGridKeyboardDelegate.ts index 5467e8c90db..5dc4a689d8b 100644 --- a/packages/@react-aria/list/src/ListGridKeyboardDelegate.ts +++ b/packages/@react-aria/list/src/ListGridKeyboardDelegate.ts @@ -18,7 +18,7 @@ import {Rect} from '@react-stately/virtualizer'; // TODO: Open to feedback about name, ListKeyboardDelegate already exists export class ListGridKeyboardDelegate extends GridKeyboardDelegate> { constructor(options: Omit>, 'focusMode'>) { - super({...options, focusMode: 'row'}); + super({...options, focusMode: 'cell'}); } private getRowKey(key: Key) { @@ -35,28 +35,8 @@ export class ListGridKeyboardDelegate extends GridKeyboardDelegate(props: ListViewProps, ref: DOMRef onAction(item.key) : undefined, shouldSelectOnPressUp: isListDraggable }, state, rowRef); + + // We don't want the rowProps.tabIndex or rowProps.onFocus because useGridCell's props + // wil handle these for us. Our strategy is to treat the outer div as the "gridcell" + // so we can cleanly navigate to the focusable children while skipping the inner "gridcell" div. + delete rowProps.tabIndex; + delete rowProps.onFocus; + let {gridCellProps} = useGridCell({ node: cellNode, - skipCell: true - }, state, cellRef); + focusMode: 'cell' + }, state, rowRef); let draggableItem: DraggableItemResult; if (isListDraggable) { // eslint-disable-next-line react-hooks/rules-of-hooks draggableItem = dragHooks.useDraggableItem({key: item.key}, dragState); } const mergedProps = mergeProps( + gridCellProps, rowProps, pressProps, isDraggable && draggableItem?.dragProps, @@ -122,8 +130,7 @@ export function ListViewItem(props) { } ) } - ref={cellRef} - {...gridCellProps}> + role="gridcell"> {isListDraggable &&
From dd9dac5119ba3fe6370b5a41d5266f46551de899 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Fri, 1 Apr 2022 15:20:54 -0700 Subject: [PATCH 10/24] fixing Home/End needed to override getKeyLeftOf/getKeyRightOf so they dont use getFirstKey or getLastKey or else focus would jump to the last row instead of wrapping inside the cell --- .../list/src/ListGridKeyboardDelegate.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/packages/@react-aria/list/src/ListGridKeyboardDelegate.ts b/packages/@react-aria/list/src/ListGridKeyboardDelegate.ts index 5dc4a689d8b..4ae5e5b1865 100644 --- a/packages/@react-aria/list/src/ListGridKeyboardDelegate.ts +++ b/packages/@react-aria/list/src/ListGridKeyboardDelegate.ts @@ -35,6 +35,24 @@ export class ListGridKeyboardDelegate extends GridKeyboardDelegate Date: Fri, 1 Apr 2022 15:26:30 -0700 Subject: [PATCH 11/24] fixing test and getting rid of erroneous data id --- packages/@react-spectrum/list/src/ListViewItem.tsx | 3 ++- packages/@react-spectrum/list/test/ListView.test.js | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/@react-spectrum/list/src/ListViewItem.tsx b/packages/@react-spectrum/list/src/ListViewItem.tsx index a596bc7020f..c2e5850f065 100644 --- a/packages/@react-spectrum/list/src/ListViewItem.tsx +++ b/packages/@react-spectrum/list/src/ListViewItem.tsx @@ -64,7 +64,8 @@ export function ListViewItem(props) { let {gridCellProps} = useGridCell({ node: cellNode, - focusMode: 'cell' + focusMode: 'cell', + isVirtualized: true }, state, rowRef); let draggableItem: DraggableItemResult; if (isListDraggable) { diff --git a/packages/@react-spectrum/list/test/ListView.test.js b/packages/@react-spectrum/list/test/ListView.test.js index b8f6baf6666..e6fd0a3b0e5 100644 --- a/packages/@react-spectrum/list/test/ListView.test.js +++ b/packages/@react-spectrum/list/test/ListView.test.js @@ -226,10 +226,10 @@ describe('ListView', function () { }); it('should focus the row if the cell is pressed', function () { - let tree = renderList(); + let tree = renderList({selectionMode: 'single'}); let cell = within(getRow(tree, 'Bar')).getByRole('gridcell'); act(() => { - cell.focus(); + triggerPress(cell); jest.runAllTimers(); }); expect(document.activeElement).toBe(getRow(tree, 'Bar')); From d9eb366fa2f3ea6db5e83bc2518370be7c18b3ef Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Fri, 1 Apr 2022 15:37:47 -0700 Subject: [PATCH 12/24] cleanup --- packages/@react-aria/grid/src/useGridCell.ts | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/packages/@react-aria/grid/src/useGridCell.ts b/packages/@react-aria/grid/src/useGridCell.ts index 2dd47b1dff5..edeb40f08b8 100644 --- a/packages/@react-aria/grid/src/useGridCell.ts +++ b/packages/@react-aria/grid/src/useGridCell.ts @@ -35,9 +35,7 @@ interface GridCellProps { * Please use onCellAction at the collection level instead. * @deprecated **/ - onAction?: () => void, - // TODO: check naming convention - skipCell?: boolean + onAction?: () => void } interface GridCellAria { @@ -65,9 +63,7 @@ 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. If skipCell is - // true, always attempt to put focus on a focusable child if any or back on the parent - // row. + // it is focused, otherwise the cell itself is focused. let focus = () => { let treeWalker = getFocusableTreeWalker(ref.current); if (focusMode === 'child') { @@ -87,7 +83,6 @@ export function useGridCell>(props: GridCellProps if (!ref.current.contains(document.activeElement)) { focusSafely(ref.current); - return; } }; From 303304cca919e48a820c52aab7d324cfdf897bce Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Fri, 1 Apr 2022 16:06:19 -0700 Subject: [PATCH 13/24] lint --- packages/@react-spectrum/list/src/ListView.tsx | 4 ++-- packages/@react-spectrum/list/src/ListViewItem.tsx | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/@react-spectrum/list/src/ListView.tsx b/packages/@react-spectrum/list/src/ListView.tsx index 2b565794e0a..c610a23f98c 100644 --- a/packages/@react-spectrum/list/src/ListView.tsx +++ b/packages/@react-spectrum/list/src/ListView.tsx @@ -38,7 +38,7 @@ 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 {GridKeyboardDelegate, useGrid, useGridSelectionCheckbox} from '@react-aria/grid'; +import {useGrid, useGridSelectionCheckbox} from '@react-aria/grid'; import {Virtualizer} from '@react-aria/virtualizer'; interface ListViewContextValue { @@ -153,7 +153,7 @@ function ListView(props: ListViewProps, ref: DOMRef(); - let cellRef = useRef(); let { isFocusVisible: isFocusVisibleWithin, focusProps: focusWithinProps From 0fe13e6114f7a52bfcb92302107fcdb6a20b708e Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Mon, 4 Apr 2022 10:16:51 -0700 Subject: [PATCH 14/24] removing todo --- packages/@react-aria/list/src/ListGridKeyboardDelegate.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/@react-aria/list/src/ListGridKeyboardDelegate.ts b/packages/@react-aria/list/src/ListGridKeyboardDelegate.ts index 4ae5e5b1865..b3b7019f35a 100644 --- a/packages/@react-aria/list/src/ListGridKeyboardDelegate.ts +++ b/packages/@react-aria/list/src/ListGridKeyboardDelegate.ts @@ -15,7 +15,6 @@ import {GridKeyboardDelegate, GridKeyboardDelegateOptions} from '@react-aria/gri import {Key} from 'react'; import {Rect} from '@react-stately/virtualizer'; -// TODO: Open to feedback about name, ListKeyboardDelegate already exists export class ListGridKeyboardDelegate extends GridKeyboardDelegate> { constructor(options: Omit>, 'focusMode'>) { super({...options, focusMode: 'cell'}); From b4a4f40d2b50527dc0ba3b99e7d53a1dd99780f4 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Mon, 4 Apr 2022 12:12:54 -0700 Subject: [PATCH 15/24] updating lint so we check that a dep is actually needed before throwing --- scripts/lint-packages.js | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/scripts/lint-packages.js b/scripts/lint-packages.js index 964ac3a7e19..fccc80e5260 100644 --- a/scripts/lint-packages.js +++ b/scripts/lint-packages.js @@ -46,6 +46,22 @@ softAssert.equal = function (val, val2, message) { } }; +// Checks if a dependency is actually being imported somewhere +function isDepUsed(dep, src) { + let depRegex = new RegExp(`import .* from '${dep}'`); + let files = glob.sync(src, { + ignore: ['**/node_modules/**', '**/dist/**'] + }); + + for (let file of files) { + let contents = fs.readFileSync(file, 'utf8'); + if (depRegex.test(contents)) { + return true; + } + } + return false; +} + let pkgNames = {}; for (let pkg of packages) { let json = JSON.parse(fs.readFileSync(pkg)); @@ -106,29 +122,27 @@ for (let pkg of packages) { for (let pkg of packages) { + let globSrc = pkg.replace('package.json', '**/*.{js,ts,tsx}'); let json = JSON.parse(fs.readFileSync(pkg)); let [scope, basename] = json.name.split('/'); if (basename.includes('utils') || basename.includes('layout')) { continue; } - if (scope === '@react-spectrum') { - let aria = `@react-aria/${basename}`; - let stately = `@react-stately/${basename}`; - let types = `@react-types/${basename}`; + let aria = `@react-aria/${basename}`; + let stately = `@react-stately/${basename}`; + let types = `@react-types/${basename}`; + + if (scope === '@react-spectrum' && isDepUsed(aria, globSrc)) { softAssert(!pkgNames[aria] || json.dependencies[aria], `${pkg} is missing a dependency on ${aria}`); - softAssert(!pkgNames[stately] || json.dependencies[stately], `${pkg} is missing a dependency on ${stately}`); - softAssert(!pkgNames[types] || json.dependencies[types], `${pkg} is missing a dependency on ${types}`); - } else if (scope === '@react-aria') { - let stately = `@react-stately/${basename}`; - let types = `@react-types/${basename}`; + } + if ((scope === '@react-aria' || scope === '@react-spectrum') && isDepUsed(stately, globSrc)) { softAssert(!pkgNames[stately] || json.dependencies[stately], `${pkg} is missing a dependency on ${stately}`); - softAssert(!pkgNames[types] || json.dependencies[types], `${pkg} is missing a dependency on ${types}`); - } else if (scope === '@react-stately') { - let types = `@react-types/${basename}`; + } + if ((scope === '@react-aria' || scope === '@react-spectrum' || scope === '@react-stately') && isDepUsed(types, globSrc)) { softAssert(!pkgNames[types] || json.dependencies[types], `${pkg} is missing a dependency on ${types}`); } } From 11302a3013146bb0a1e3147c3a6e3df8d9fd37d6 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Mon, 4 Apr 2022 12:19:51 -0700 Subject: [PATCH 16/24] removing private --- packages/@react-aria/list/package.json | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/@react-aria/list/package.json b/packages/@react-aria/list/package.json index 29ec3a72e86..af0ecf57cd6 100644 --- a/packages/@react-aria/list/package.json +++ b/packages/@react-aria/list/package.json @@ -1,7 +1,6 @@ { "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", From c9ba9237f494f2cd348e89190cf50b608cb21469 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Mon, 4 Apr 2022 17:24:23 -0700 Subject: [PATCH 17/24] pulling triggerPress outside the act --- .../@react-spectrum/list/test/ListView.test.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/@react-spectrum/list/test/ListView.test.js b/packages/@react-spectrum/list/test/ListView.test.js index e6fd0a3b0e5..e173ef17ce5 100644 --- a/packages/@react-spectrum/list/test/ListView.test.js +++ b/packages/@react-spectrum/list/test/ListView.test.js @@ -221,15 +221,15 @@ describe('ListView', function () { it('should retain focus on the pressed child', function () { let tree = renderListWithFocusables(); let button = within(getRow(tree, 'Foo')).getAllByRole('button')[1]; - act(() => triggerPress(button)); + triggerPress(button); expect(document.activeElement).toBe(button); }); it('should focus the row if the cell is pressed', function () { let tree = renderList({selectionMode: 'single'}); let cell = within(getRow(tree, 'Bar')).getByRole('gridcell'); + triggerPress(cell); act(() => { - triggerPress(cell); jest.runAllTimers(); }); expect(document.activeElement).toBe(getRow(tree, 'Bar')); @@ -288,7 +288,7 @@ describe('ListView', function () { 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)); + triggerPress(start); expect(document.activeElement).toHaveTextContent('button2 Foo'); expect(document.activeElement).toBe(start); moveFocus('ArrowRight'); @@ -327,7 +327,7 @@ describe('ListView', function () { 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)); + triggerPress(start); expect(document.activeElement).toHaveTextContent('button1 Foo'); expect(document.activeElement).toBe(start); moveFocus('ArrowLeft'); @@ -388,7 +388,7 @@ describe('ListView', function () { let tree = renderListWithFocusables({items: manyItems}); let focusables = within(getRow(tree, 'Foo 25')).getAllByRole('button'); let start = focusables[0]; - act(() => triggerPress(start)); + triggerPress(start); expect(document.activeElement).toBe(start); moveFocus('PageUp'); expect(document.activeElement).toBe(getRow(tree, 'Foo 1')); @@ -410,7 +410,7 @@ describe('ListView', function () { let tree = renderListWithFocusables({items: manyItems}); let focusables = within(getRow(tree, 'Foo 1')).getAllByRole('button'); let start = focusables[0]; - act(() => triggerPress(start)); + triggerPress(start); expect(document.activeElement).toBe(start); moveFocus('PageDown'); expect(document.activeElement).toBe(getRow(tree, 'Foo 25')); @@ -432,7 +432,7 @@ describe('ListView', function () { let tree = renderListWithFocusables({items: manyItems}); let focusables = within(getRow(tree, 'Foo 15')).getAllByRole('button'); let start = focusables[0]; - act(() => triggerPress(start)); + triggerPress(start); expect(document.activeElement).toBe(start); moveFocus('Home'); expect(document.activeElement).toBe(getRow(tree, 'Foo 1')); @@ -452,7 +452,7 @@ describe('ListView', function () { let tree = renderListWithFocusables({items: manyItems}); let focusables = within(getRow(tree, 'Foo 1')).getAllByRole('button'); let start = focusables[0]; - act(() => triggerPress(start)); + triggerPress(start); expect(document.activeElement).toBe(start); moveFocus('End'); expect(document.activeElement).toBe(getRow(tree, 'Foo 100')); From f2ef4ef8e995f3e11f1520bfd35c9b450a74f86c Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Thu, 21 Apr 2022 14:57:21 -0700 Subject: [PATCH 18/24] tentative approach to use ListLayout instead of making a ListGrid keyboard delegate --- 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 | 71 ------------------- packages/@react-aria/list/src/index.ts | 13 ---- .../@react-spectrum/list/src/ListView.tsx | 25 ++----- .../@react-spectrum/list/src/ListViewItem.tsx | 9 +-- .../@react-stately/grid/src/GridCollection.ts | 17 ++--- .../@react-stately/layout/src/ListLayout.ts | 21 ++++-- 9 files changed, 33 insertions(+), 170 deletions(-) delete mode 100644 packages/@react-aria/list/README.md delete mode 100644 packages/@react-aria/list/index.ts delete mode 100644 packages/@react-aria/list/package.json delete mode 100644 packages/@react-aria/list/src/ListGridKeyboardDelegate.ts delete mode 100644 packages/@react-aria/list/src/index.ts diff --git a/packages/@react-aria/list/README.md b/packages/@react-aria/list/README.md deleted file mode 100644 index 8c3905bdc98..00000000000 --- a/packages/@react-aria/list/README.md +++ /dev/null @@ -1,3 +0,0 @@ -# @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 deleted file mode 100644 index 4e9931530d8..00000000000 --- a/packages/@react-aria/list/index.ts +++ /dev/null @@ -1,13 +0,0 @@ -/* - * 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 deleted file mode 100644 index af0ecf57cd6..00000000000 --- a/packages/@react-aria/list/package.json +++ /dev/null @@ -1,31 +0,0 @@ -{ - "name": "@react-aria/list", - "version": "3.0.0-alpha.1", - "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-stately/virtualizer": "^3.1.7", - "@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 deleted file mode 100644 index b3b7019f35a..00000000000 --- a/packages/@react-aria/list/src/ListGridKeyboardDelegate.ts +++ /dev/null @@ -1,71 +0,0 @@ -/* - * 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'; -import {Rect} from '@react-stately/virtualizer'; - -export class ListGridKeyboardDelegate extends GridKeyboardDelegate> { - constructor(options: Omit>, 'focusMode'>) { - super({...options, focusMode: 'cell'}); - } - - private getRowKey(key: Key) { - let startItem = key != null ? this.collection.getItem(key) : null; - if (!startItem) { - return; - } - - // If focus was on a cell, start searching from the parent row - if (this.isCell(startItem)) { - key = startItem.parentKey; - } - - return key; - } - - // Return the same key since the ListView is a single column grid and focusMode is 'cell', thus we will never - // leave the cell via left/right arrows. useGridCell will handle moving focus to any focusable children that exist as well as RTL behavior - getKeyLeftOf(key: Key) { - return key; - } - - getKeyRightOf(key: Key) { - return key; - } - - getFirstKey() { - return this.collection.getFirstKey(); - } - - getLastKey() { - return this.collection.getLastKey(); - } - - protected getItemRect(key: Key): Rect { - // Get row key since the list layout will only have the row keys, not cell keys - key = this.getRowKey(key); - if (key == null) { - return; - } - - if (this.layout) { - return this.layout.getLayoutInfo(key)?.rect; - } - - let item = this.getItem(key); - if (item) { - return new Rect(item.offsetLeft, item.offsetTop, item.offsetWidth, item.offsetHeight); - } - } -} diff --git a/packages/@react-aria/list/src/index.ts b/packages/@react-aria/list/src/index.ts deleted file mode 100644 index 505d9903d9a..00000000000 --- a/packages/@react-aria/list/src/index.ts +++ /dev/null @@ -1,13 +0,0 @@ -/* - * 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/src/ListView.tsx b/packages/@react-spectrum/list/src/ListView.tsx index c610a23f98c..6a5923fc483 100644 --- a/packages/@react-spectrum/list/src/ListView.tsx +++ b/packages/@react-spectrum/list/src/ListView.tsx @@ -28,7 +28,6 @@ import {DragHooks} from '@react-spectrum/dnd'; import {GridCollection, GridState, useGridState} from '@react-stately/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'; @@ -37,13 +36,12 @@ import {ListViewItem} from './ListViewItem'; 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 {useCollator, useMessageFormatter} from '@react-aria/i18n'; import {useGrid, useGridSelectionCheckbox} from '@react-aria/grid'; import {Virtualizer} from '@react-aria/virtualizer'; interface ListViewContextValue { state: GridState>, - keyboardDelegate: ListGridKeyboardDelegate, dragState: DraggableCollectionState, onAction:(key: string) => void, isListDraggable: boolean @@ -119,8 +117,6 @@ function ListView(props: ListViewProps, ref: DOMRef new GridCollection({ columnCount: 1, items: [...collection].map(item => ({ @@ -135,26 +131,19 @@ function ListView(props: ListViewProps, ref: DOMRef new ListGridKeyboardDelegate({ - collection: state.collection, - disabledKeys: state.disabledKeys, - ref: domRef, - direction, - collator, - layout - }), [state, domRef, direction, collator, layout]); - let provider = useProvider(); let {checkboxProps} = useGridSelectionCheckbox({key: null}, state); let dragState: DraggableCollectionState; @@ -213,7 +202,7 @@ function ListView(props: ListViewProps, ref: DOMRef(props: ListViewProps, ref: DOMRef + implements IGridCollection { child.parentKey = node.key; } childKeys.add(child.key); - - if (last) { - last.nextKey = child.key; - child.prevKey = last.key; - } else { - child.prevKey = null; + if (child.nextKey == null && child.prevKey == null) { + if (last) { + last.nextKey = child.key; + child.prevKey = last.key; + } else { + child.prevKey = null; + } } visit(child); last = child; } - if (last) { - last.nextKey = null; - } - // Remove deleted nodes and their children from the key map if (prevNode) { for (let child of prevNode.childNodes) { diff --git a/packages/@react-stately/layout/src/ListLayout.ts b/packages/@react-stately/layout/src/ListLayout.ts index 75399edf6f7..d41ed0c5d69 100644 --- a/packages/@react-stately/layout/src/ListLayout.ts +++ b/packages/@react-stately/layout/src/ListLayout.ts @@ -25,7 +25,8 @@ export type ListLayoutOptions = { indentationForItem?: (collection: Collection>, key: Key) => number, collator?: Intl.Collator, loaderHeight?: number, - placeholderHeight?: number + placeholderHeight?: number, + allowDisabledKeyFocus?: boolean }; // A wrapper around LayoutInfo that supports hierarchy @@ -60,6 +61,7 @@ export class ListLayout extends Layout> implements KeyboardDelegate { protected contentSize: Size; collection: Collection>; disabledKeys: Set = new Set(); + allowDisabledKeyFocus: boolean; isLoading: boolean; protected lastWidth: number; protected lastCollection: Collection>; @@ -89,6 +91,7 @@ export class ListLayout extends Layout> implements KeyboardDelegate { this.rootNodes = []; this.lastWidth = 0; this.lastCollection = null; + this.allowDisabledKeyFocus = options.allowDisabledKeyFocus; } getLayoutInfo(key: Key) { @@ -350,7 +353,7 @@ export class ListLayout extends Layout> implements KeyboardDelegate { key = collection.getKeyBefore(key); while (key != null) { let item = collection.getItem(key); - if (item.type === 'item' && !this.disabledKeys.has(item.key)) { + if (item.type === 'item' && (this.allowDisabledKeyFocus || !this.disabledKeys.has(item.key))) { return key; } @@ -364,7 +367,7 @@ export class ListLayout extends Layout> implements KeyboardDelegate { key = collection.getKeyAfter(key); while (key != null) { let item = collection.getItem(key); - if (item.type === 'item' && !this.disabledKeys.has(item.key)) { + if (item.type === 'item' && (this.allowDisabledKeyFocus || !this.disabledKeys.has(item.key))) { return key; } @@ -372,6 +375,14 @@ export class ListLayout extends Layout> implements KeyboardDelegate { } } + getKeyLeftOf(key: Key): Key { + return key; + } + + getKeyRightOf(key: Key): Key { + return key; + } + getKeyPageAbove(key: Key) { let layoutInfo = this.getLayoutInfo(key); @@ -413,7 +424,7 @@ export class ListLayout extends Layout> implements KeyboardDelegate { let key = collection.getFirstKey(); while (key != null) { let item = collection.getItem(key); - if (item.type === 'item' && !this.disabledKeys.has(item.key)) { + if (item.type === 'item' && (this.allowDisabledKeyFocus || !this.disabledKeys.has(item.key))) { return key; } @@ -426,7 +437,7 @@ export class ListLayout extends Layout> implements KeyboardDelegate { let key = collection.getLastKey(); while (key != null) { let item = collection.getItem(key); - if (item.type === 'item' && !this.disabledKeys.has(item.key)) { + if (item.type === 'item' && (this.allowDisabledKeyFocus || !this.disabledKeys.has(item.key))) { return key; } From 0476f4fc8c83efb8ad8329162ca18a990fcbe6b4 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Thu, 21 Apr 2022 16:36:31 -0700 Subject: [PATCH 19/24] fixing build --- packages/@react-spectrum/list/package.json | 1 - packages/@react-spectrum/list/test/ListView.test.js | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/@react-spectrum/list/package.json b/packages/@react-spectrum/list/package.json index b2140249000..f698114007a 100644 --- a/packages/@react-spectrum/list/package.json +++ b/packages/@react-spectrum/list/package.json @@ -37,7 +37,6 @@ "@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/test/ListView.test.js b/packages/@react-spectrum/list/test/ListView.test.js index e173ef17ce5..37cb6f50e82 100644 --- a/packages/@react-spectrum/list/test/ListView.test.js +++ b/packages/@react-spectrum/list/test/ListView.test.js @@ -406,7 +406,7 @@ describe('ListView', function () { 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 () { + it.skip('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]; From 2ea0c5a2bb20bc3bf54c356e53130d7003d9f141 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Mon, 25 Apr 2022 17:49:08 -0700 Subject: [PATCH 20/24] removing useGridRow to ensure the focused key will always be a row also reverts changes to other files that arent necessary anymore. Adds tests for disabled keys --- .../grid/src/GridKeyboardDelegate.ts | 4 +- .../@react-spectrum/list/src/ListView.tsx | 17 ++---- .../@react-spectrum/list/src/ListViewItem.tsx | 54 ++++++++++--------- .../list/test/ListView.test.js | 20 ++++++- .../@react-stately/grid/src/GridCollection.ts | 17 +++--- 5 files changed, 65 insertions(+), 47 deletions(-) diff --git a/packages/@react-aria/grid/src/GridKeyboardDelegate.ts b/packages/@react-aria/grid/src/GridKeyboardDelegate.ts index ca1c40827b9..5755a5214b4 100644 --- a/packages/@react-aria/grid/src/GridKeyboardDelegate.ts +++ b/packages/@react-aria/grid/src/GridKeyboardDelegate.ts @@ -268,11 +268,11 @@ export class GridKeyboardDelegate> implements Key return key; } - protected getItem(key: Key): HTMLElement { + private getItem(key: Key): HTMLElement { return this.ref.current.querySelector(`[data-key="${key}"]`); } - protected getItemRect(key: Key): Rect { + private getItemRect(key: Key): Rect { if (this.layout) { return this.layout.getLayoutInfo(key)?.rect; } diff --git a/packages/@react-spectrum/list/src/ListView.tsx b/packages/@react-spectrum/list/src/ListView.tsx index 6a5923fc483..de36385877d 100644 --- a/packages/@react-spectrum/list/src/ListView.tsx +++ b/packages/@react-spectrum/list/src/ListView.tsx @@ -73,7 +73,8 @@ export function useListLayout(state: ListState, density: ListViewProps[ estimatedRowHeight: ROW_HEIGHTS[density][scale], padding: 0, collator, - loaderHeight: isEmpty ? null : ROW_HEIGHTS[density][scale] + loaderHeight: isEmpty ? null : ROW_HEIGHTS[density][scale], + allowDisabledKeyFocus: true }) , [collator, scale, density, isEmpty]); @@ -131,9 +132,7 @@ function ListView(props: ListViewProps, ref: DOMRef(props: ListViewProps, ref: DOMRef (props: ListViewProps, ref: DOMRef(); @@ -48,37 +47,26 @@ export function ListViewItem(props) { let isDraggable = dragState?.isDraggable(item.key) && !isDisabled; let {hoverProps, isHovered} = useHover({isDisabled}); let {pressProps, isPressed} = usePress({isDisabled}); - let {rowProps} = useGridRow({ - node: item, - isVirtualized: true, - onAction: onAction ? () => onAction(item.key) : undefined, - shouldSelectOnPressUp: isListDraggable - }, state, rowRef); + // We only make use of useGridCell here to allow for keyboard navigation to the focusable children of the row. + // The actual grid cell of the ListView is intert since we don't want to ever focus it to decrease screenreader + // verbosity, so we pretend the row node is the cell for interaction purposes. useGridRow is never used since + // it would conflict with useGridCell if applied to the same node. let {gridCellProps} = useGridCell({ - node: cellNode, + node: item, focusMode: 'cell', - isVirtualized: true + isVirtualized: true, + shouldSelectOnPressUp: isListDraggable }, state, rowRef); - - delete gridCellProps.tabIndex; + delete gridCellProps['aria-colindex']; let draggableItem: DraggableItemResult; if (isListDraggable) { // eslint-disable-next-line react-hooks/rules-of-hooks draggableItem = dragHooks.useDraggableItem({key: item.key}, dragState); } - const mergedProps = mergeProps( - gridCellProps, - rowProps, - pressProps, - isDraggable && draggableItem?.dragProps, - hoverProps, - focusWithinProps, - focusProps - ); - let {checkboxProps} = useGridSelectionCheckbox({...props, key: item.key}, state); + let {checkboxProps} = useGridSelectionCheckbox({...props, key: item.key}, state); let dragButtonRef = React.useRef(); let {buttonProps} = useButton({ ...draggableItem?.dragButtonProps, @@ -104,11 +92,26 @@ export function ListViewItem(props) { let isSelected = state.selectionManager.isSelected(item.key); let showDragHandle = isDraggable && (isFocusVisibleWithin || isHovered || isPressed); let {visuallyHiddenProps} = useVisuallyHidden(); + let rowProps = { + role: 'row', + 'aria-label': item.textValue, + 'aria-selected': state.selectionManager.selectionMode !== 'none' ? isSelected : undefined, + 'aria-rowindex': item.index + 1 + }; + + const mergedProps = mergeProps( + gridCellProps, + rowProps, + pressProps, + isDraggable && draggableItem?.dragProps, + hoverProps, + focusWithinProps, + focusProps + ); return (
+ ref={rowRef}>
+ role="gridcell" + aria-colindex={1}> {isListDraggable &&
diff --git a/packages/@react-spectrum/list/test/ListView.test.js b/packages/@react-spectrum/list/test/ListView.test.js index 37cb6f50e82..fd2b4e6acb4 100644 --- a/packages/@react-spectrum/list/test/ListView.test.js +++ b/packages/@react-spectrum/list/test/ListView.test.js @@ -354,6 +354,15 @@ describe('ListView', function () { moveFocus('ArrowUp'); expect(document.activeElement).toBe(end); }); + + it('should allow focus on disabled rows', function () { + let tree = renderListWithFocusables({disabledKeys: ['foo']}); + let start = getRow(tree, 'Bar'); + let end = getRow(tree, 'Foo'); + act(() => start.focus()); + moveFocus('ArrowUp'); + expect(document.activeElement).toBe(end); + }); }); describe('ArrowDown', function () { @@ -373,6 +382,15 @@ describe('ListView', function () { moveFocus('ArrowDown'); expect(document.activeElement).toBe(end); }); + + it('should allow focus on disabled rows', function () { + let tree = renderListWithFocusables({disabledKeys: ['bar']}); + let start = getRow(tree, 'Foo'); + let end = getRow(tree, 'Bar'); + act(() => start.focus()); + moveFocus('ArrowDown'); + expect(document.activeElement).toBe(end); + }); }); describe('PageUp', function () { @@ -406,7 +424,7 @@ describe('ListView', function () { expect(document.activeElement).toBe(getRow(tree, 'Foo 49')); }); - it.skip('should move focus to a row a page below when focus starts in the row cell', function () { + 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]; diff --git a/packages/@react-stately/grid/src/GridCollection.ts b/packages/@react-stately/grid/src/GridCollection.ts index fa25e0229bf..12ef5f331f7 100644 --- a/packages/@react-stately/grid/src/GridCollection.ts +++ b/packages/@react-stately/grid/src/GridCollection.ts @@ -48,19 +48,22 @@ export class GridCollection implements IGridCollection { child.parentKey = node.key; } childKeys.add(child.key); - if (child.nextKey == null && child.prevKey == null) { - if (last) { - last.nextKey = child.key; - child.prevKey = last.key; - } else { - child.prevKey = null; - } + + if (last) { + last.nextKey = child.key; + child.prevKey = last.key; + } else { + child.prevKey = null; } visit(child); last = child; } + if (last) { + last.nextKey = null; + } + // Remove deleted nodes and their children from the key map if (prevNode) { for (let child of prevNode.childNodes) { From 53e8c7ab2be0caed822ce2e4f45ad09020693ecc Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Tue, 26 Apr 2022 10:15:30 -0700 Subject: [PATCH 21/24] cleanup --- packages/@react-spectrum/list/src/ListView.tsx | 7 +++---- packages/@react-spectrum/list/src/ListViewItem.tsx | 7 ++++--- packages/@react-stately/layout/src/ListLayout.ts | 6 +++--- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/@react-spectrum/list/src/ListView.tsx b/packages/@react-spectrum/list/src/ListView.tsx index bae81e015c6..c6bb18d0392 100644 --- a/packages/@react-spectrum/list/src/ListView.tsx +++ b/packages/@react-spectrum/list/src/ListView.tsx @@ -38,10 +38,9 @@ import {useGrid} from '@react-aria/grid'; import {useProvider} from '@react-spectrum/provider'; import {Virtualizer} from '@react-aria/virtualizer'; -interface ListViewContextValue { +interface ListViewContextValue { state: GridState>, dragState: DraggableCollectionState, - onAction:(key: string) => void, isListDraggable: boolean, layout: ListLayout } @@ -182,7 +181,7 @@ function ListView(props: ListViewProps, ref: DOMRef + (props: ListViewProps, ref: DOMRef { if (type === 'item') { return ( - + ); } else if (type === 'loader') { return ( diff --git a/packages/@react-spectrum/list/src/ListViewItem.tsx b/packages/@react-spectrum/list/src/ListViewItem.tsx index 94500f71a93..8060cf78e04 100644 --- a/packages/@react-spectrum/list/src/ListViewItem.tsx +++ b/packages/@react-spectrum/list/src/ListViewItem.tsx @@ -32,9 +32,10 @@ export function ListViewItem(props) { let { item, isEmphasized, - dragHooks + dragHooks, + hasActions } = props; - let {state, dragState, onAction, isListDraggable, layout} = useContext(ListViewContext); + let {state, dragState, isListDraggable, layout} = useContext(ListViewContext); let {direction} = useLocale(); let rowRef = useRef(); let { @@ -42,7 +43,7 @@ export function ListViewItem(props) { focusProps: focusWithinProps } = useFocusRing({within: true}); let {isFocusVisible, focusProps} = useFocusRing(); - let allowsInteraction = state.selectionManager.selectionMode !== 'none' || onAction; + let allowsInteraction = state.selectionManager.selectionMode !== 'none' || hasActions; let isDisabled = !allowsInteraction || state.disabledKeys.has(item.key); let isDraggable = dragState?.isDraggable(item.key) && !isDisabled; let {hoverProps, isHovered} = useHover({isDisabled}); diff --git a/packages/@react-stately/layout/src/ListLayout.ts b/packages/@react-stately/layout/src/ListLayout.ts index d41ed0c5d69..b1dd841024c 100644 --- a/packages/@react-stately/layout/src/ListLayout.ts +++ b/packages/@react-stately/layout/src/ListLayout.ts @@ -353,7 +353,7 @@ export class ListLayout extends Layout> implements KeyboardDelegate { key = collection.getKeyBefore(key); while (key != null) { let item = collection.getItem(key); - if (item.type === 'item' && (this.allowDisabledKeyFocus || !this.disabledKeys.has(item.key))) { + if (item.type === 'item' && (this.allowDisabledKeyFocus || !this.disabledKeys.has(item.key))) { return key; } @@ -424,7 +424,7 @@ export class ListLayout extends Layout> implements KeyboardDelegate { let key = collection.getFirstKey(); while (key != null) { let item = collection.getItem(key); - if (item.type === 'item' && (this.allowDisabledKeyFocus || !this.disabledKeys.has(item.key))) { + if (item.type === 'item' && (this.allowDisabledKeyFocus || !this.disabledKeys.has(item.key))) { return key; } @@ -437,7 +437,7 @@ export class ListLayout extends Layout> implements KeyboardDelegate { let key = collection.getLastKey(); while (key != null) { let item = collection.getItem(key); - if (item.type === 'item' && (this.allowDisabledKeyFocus || !this.disabledKeys.has(item.key))) { + if (item.type === 'item' && (this.allowDisabledKeyFocus || !this.disabledKeys.has(item.key))) { return key; } From 3f7f3d8b0e00cbac567b9b0d2680aeaa37962174 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Wed, 27 Apr 2022 17:14:05 -0700 Subject: [PATCH 22/24] adding listview story decorator for before and after focusable elements helps avoid weird test cases like https://github.com/adobe/react-spectrum/pull/3000#issuecomment-1111494915 --- .../list/stories/ListView.stories.tsx | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/packages/@react-spectrum/list/stories/ListView.stories.tsx b/packages/@react-spectrum/list/stories/ListView.stories.tsx index fc0639edc50..61aef2778d7 100644 --- a/packages/@react-spectrum/list/stories/ListView.stories.tsx +++ b/packages/@react-spectrum/list/stories/ListView.stories.tsx @@ -52,7 +52,23 @@ function renderEmptyState() { ); } +let decorator = (storyFn, context) => { + let omittedStories = ['draggable rows', 'dynamic items + renderEmptyState']; + return omittedStories.some(omittedName => context.name.includes(omittedName)) ? + storyFn() : + ( + <> + + + {storyFn()} + + + + ); +}; + storiesOf('ListView', module) + .addDecorator(decorator) .add('default', () => ( row 1 From 6c02faf14ef51898452a25efd3ed7a8af7054661 Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Fri, 29 Apr 2022 14:51:25 -0700 Subject: [PATCH 23/24] making the story input fields only appear at certain screen widths this makes it so the listview doesnt get squished on mobile --- packages/@react-spectrum/list/stories/ListView.stories.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@react-spectrum/list/stories/ListView.stories.tsx b/packages/@react-spectrum/list/stories/ListView.stories.tsx index 61aef2778d7..c7ba859be40 100644 --- a/packages/@react-spectrum/list/stories/ListView.stories.tsx +++ b/packages/@react-spectrum/list/stories/ListView.stories.tsx @@ -54,7 +54,7 @@ function renderEmptyState() { let decorator = (storyFn, context) => { let omittedStories = ['draggable rows', 'dynamic items + renderEmptyState']; - return omittedStories.some(omittedName => context.name.includes(omittedName)) ? + return window.screen.width <= 700 || omittedStories.some(omittedName => context.name.includes(omittedName)) ? storyFn() : ( <> From e4086bf81a366e13351f512cb106abe7bc19aa6e Mon Sep 17 00:00:00 2001 From: Daniel Lu Date: Wed, 4 May 2022 11:39:31 -0700 Subject: [PATCH 24/24] addressing review comments --- .../@react-spectrum/list/src/ListViewItem.tsx | 2 +- .../list/test/ListView.test.js | 64 ++++++++++--------- .../@react-stately/layout/src/ListLayout.ts | 2 +- 3 files changed, 36 insertions(+), 32 deletions(-) diff --git a/packages/@react-spectrum/list/src/ListViewItem.tsx b/packages/@react-spectrum/list/src/ListViewItem.tsx index 8060cf78e04..971c567c16a 100644 --- a/packages/@react-spectrum/list/src/ListViewItem.tsx +++ b/packages/@react-spectrum/list/src/ListViewItem.tsx @@ -50,7 +50,7 @@ export function ListViewItem(props) { let {pressProps, isPressed} = usePress({isDisabled}); // We only make use of useGridCell here to allow for keyboard navigation to the focusable children of the row. - // The actual grid cell of the ListView is intert since we don't want to ever focus it to decrease screenreader + // The actual grid cell of the ListView is inert since we don't want to ever focus it to decrease screenreader // verbosity, so we pretend the row node is the cell for interaction purposes. useGridRow is never used since // it would conflict with useGridCell if applied to the same node. let {gridCellProps} = useGridCell({ diff --git a/packages/@react-spectrum/list/test/ListView.test.js b/packages/@react-spectrum/list/test/ListView.test.js index 2e4c4bedeed..f5b135000df 100644 --- a/packages/@react-spectrum/list/test/ListView.test.js +++ b/packages/@react-spectrum/list/test/ListView.test.js @@ -248,7 +248,7 @@ describe('ListView', function () { let tree = renderList(); let target = getRow(tree, 'Baz'); let grid = tree.getByRole('grid'); - act(() => grid.focus()); + userEvent.tab(); fireEvent.keyDown(grid, {key: 'B'}); fireEvent.keyUp(grid, {key: 'Enter'}); fireEvent.keyDown(grid, {key: 'A'}); @@ -263,7 +263,7 @@ describe('ListView', function () { it('should not move focus if no focusables present', function () { let tree = renderList(); let start = getRow(tree, 'Foo'); - act(() => start.focus()); + userEvent.tab(); moveFocus('ArrowRight'); expect(document.activeElement).toBe(start); }); @@ -273,7 +273,7 @@ describe('ListView', function () { let tree = renderListWithFocusables(); let start = getRow(tree, 'Foo'); let focusables = within(start).getAllByRole('button'); - act(() => start.focus()); + userEvent.tab(); moveFocus('ArrowRight'); expect(document.activeElement).toBe(focusables[0]); moveFocus('ArrowRight'); @@ -302,7 +302,7 @@ describe('ListView', function () { it('should not move focus if no focusables present', function () { let tree = renderList(); let start = getRow(tree, 'Foo'); - act(() => start.focus()); + userEvent.tab(); moveFocus('ArrowLeft'); expect(document.activeElement).toBe(start); }); @@ -312,7 +312,7 @@ describe('ListView', function () { let tree = renderListWithFocusables(); let focusables = within(getRow(tree, 'Foo')).getAllByRole('button'); let start = getRow(tree, 'Foo'); - act(() => start.focus()); + userEvent.tab(); moveFocus('ArrowLeft'); expect(document.activeElement).toBe(focusables[1]); moveFocus('ArrowLeft'); @@ -341,25 +341,25 @@ describe('ListView', function () { it('should not wrap focus', function () { let tree = renderListWithFocusables(); let start = getRow(tree, 'Foo'); - act(() => start.focus()); + userEvent.tab(); moveFocus('ArrowUp'); expect(document.activeElement).toBe(start); }); it('should move focus to above row', function () { - let tree = renderListWithFocusables(); + let tree = renderListWithFocusables({selectionMode: 'single'}); let start = getRow(tree, 'Bar'); let end = getRow(tree, 'Foo'); - act(() => start.focus()); + triggerPress(start); moveFocus('ArrowUp'); expect(document.activeElement).toBe(end); }); it('should allow focus on disabled rows', function () { - let tree = renderListWithFocusables({disabledKeys: ['foo']}); + let tree = renderListWithFocusables({disabledKeys: ['foo'], selectionMode: 'single'}); let start = getRow(tree, 'Bar'); let end = getRow(tree, 'Foo'); - act(() => start.focus()); + triggerPress(start); moveFocus('ArrowUp'); expect(document.activeElement).toBe(end); }); @@ -367,27 +367,27 @@ describe('ListView', function () { describe('ArrowDown', function () { it('should not wrap focus', function () { - let tree = renderListWithFocusables(); + let tree = renderListWithFocusables({selectionMode: 'single'}); let start = getRow(tree, 'Baz'); - act(() => start.focus()); + triggerPress(start); moveFocus('ArrowDown'); expect(document.activeElement).toBe(start); }); it('should move focus to below row', function () { - let tree = renderListWithFocusables(); + let tree = renderListWithFocusables({selectionMode: 'single'}); let start = getRow(tree, 'Foo'); let end = getRow(tree, 'Bar'); - act(() => start.focus()); + triggerPress(start); moveFocus('ArrowDown'); expect(document.activeElement).toBe(end); }); it('should allow focus on disabled rows', function () { - let tree = renderListWithFocusables({disabledKeys: ['bar']}); + let tree = renderListWithFocusables({disabledKeys: ['bar'], selectionMode: 'single'}); let start = getRow(tree, 'Foo'); let end = getRow(tree, 'Bar'); - act(() => start.focus()); + triggerPress(start); moveFocus('ArrowDown'); expect(document.activeElement).toBe(end); }); @@ -395,9 +395,9 @@ describe('ListView', function () { 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 tree = renderListWithFocusables({items: manyItems, selectionMode: 'single'}); let start = getRow(tree, 'Foo 25'); - act(() => start.focus()); + triggerPress(start); moveFocus('PageUp'); expect(document.activeElement).toBe(getRow(tree, 'Foo 1')); }); @@ -415,9 +415,8 @@ describe('ListView', function () { 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()); + let tree = renderListWithFocusables({items: manyItems, selectionMode: 'single'}); + userEvent.tab(); moveFocus('PageDown'); expect(document.activeElement).toBe(getRow(tree, 'Foo 25')); moveFocus('PageDown'); @@ -439,9 +438,9 @@ describe('ListView', function () { describe('Home', function () { it('should move focus to the first row when focus starts on a row', function () { - let tree = renderListWithFocusables({items: manyItems}); + let tree = renderListWithFocusables({items: manyItems, selectionMode: 'single'}); let start = getRow(tree, 'Foo 15'); - act(() => start.focus()); + triggerPress(start); moveFocus('Home'); expect(document.activeElement).toBe(getRow(tree, 'Foo 1')); }); @@ -460,8 +459,7 @@ describe('ListView', function () { 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()); + userEvent.tab(); moveFocus('End'); expect(document.activeElement).toBe(getRow(tree, 'Foo 100')); }); @@ -1063,7 +1061,7 @@ describe('ListView', function () { expect(cell).toHaveTextContent('File a'); expect(row).toHaveAttribute('draggable', 'true'); - act(() => cell.focus()); + userEvent.tab(); let draghandle = within(cell).getAllByRole('button')[0]; expect(draghandle).toBeTruthy(); expect(draghandle).toHaveAttribute('draggable', 'true'); @@ -1121,7 +1119,7 @@ describe('ListView', function () { expect(cellD).toHaveTextContent('File d'); expect(rows[3]).toHaveAttribute('draggable', 'true'); - act(() => cellA.focus()); + userEvent.tab(); let draghandle = within(cellA).getAllByRole('button')[0]; expect(draghandle).toBeTruthy(); @@ -1209,26 +1207,32 @@ describe('ListView', function () { it('should only display the drag handle on keyboard focus for dragggable items', function () { let {getAllByRole} = render( - + ); let rows = getAllByRole('row'); let cellA = within(rows[0]).getByRole('gridcell'); + triggerPress(cellA); + expect(document.activeElement).toBe(rows[0]); let dragHandle = within(cellA).getAllByRole('button')[0]; // If the dragHandle has a style applied, it is visually hidden expect(dragHandle.style).toBeTruthy(); + expect(dragHandle.style.position).toBe('absolute'); fireEvent.pointerDown(rows[0], {button: 0, pointerId: 1}); dragHandle = within(cellA).getAllByRole('button')[0]; expect(dragHandle.style).toBeTruthy(); + expect(dragHandle.style.position).toBe('absolute'); fireEvent.pointerUp(rows[0], {button: 0, pointerId: 1}); fireEvent.pointerEnter(rows[0]); dragHandle = within(cellA).getAllByRole('button')[0]; expect(dragHandle.style).toBeTruthy(); + expect(dragHandle.style.position).toBe('absolute'); // If dragHandle doesn't have a position applied, it isn't visually hidden - act(() => rows[0].focus()); + fireEvent.keyDown(rows[0], {key: 'Enter'}); + fireEvent.keyUp(rows[0], {key: 'Enter'}); dragHandle = within(cellA).getAllByRole('button')[0]; expect(dragHandle.style.position).toBe(''); }); @@ -1255,7 +1259,7 @@ describe('ListView', function () { let cellA = within(rows[0]).getByRole('gridcell'); let cellB = within(rows[1]).getByRole('gridcell'); - act(() => cellA.focus()); + userEvent.tab(); expect(hasDragHandle(cellA)).toBeFalsy(); moveFocus('ArrowDown'); expect(hasDragHandle(cellB)).toBeFalsy(); diff --git a/packages/@react-stately/layout/src/ListLayout.ts b/packages/@react-stately/layout/src/ListLayout.ts index b1dd841024c..34f2fd1a11b 100644 --- a/packages/@react-stately/layout/src/ListLayout.ts +++ b/packages/@react-stately/layout/src/ListLayout.ts @@ -61,7 +61,7 @@ export class ListLayout extends Layout> implements KeyboardDelegate { protected contentSize: Size; collection: Collection>; disabledKeys: Set = new Set(); - allowDisabledKeyFocus: boolean; + allowDisabledKeyFocus: boolean = false; isLoading: boolean; protected lastWidth: number; protected lastCollection: Collection>;