From 3666c66fae9f541ff2e3f8310d06f28f07e40b5e Mon Sep 17 00:00:00 2001 From: Reid Barber Date: Tue, 28 Apr 2026 15:11:00 -0500 Subject: [PATCH] fix(Collections): handle falsy keys properly (#9993) * fix(Collections): handle falsy keys properly * add regression tests --- .../src/card/WaterfallLayout.tsx | 4 +- packages/@react-spectrum/s2/src/TreeView.tsx | 6 +-- .../react-aria-components/src/DragAndDrop.tsx | 2 +- packages/react-aria-components/src/Table.tsx | 4 +- packages/react-aria-components/src/Tree.tsx | 4 +- .../src/TreeDropTargetDelegate.ts | 8 ++-- .../react-aria-components/test/Select.test.js | 25 +++++++++++ .../react-aria-components/test/Table.test.js | 35 +++++++++++++++- .../react-aria-components/test/Tree.test.tsx | 41 +++++++++++++++++++ .../src/collections/BaseCollection.ts | 4 +- packages/react-aria/src/select/useSelect.ts | 4 +- packages/react-aria/src/table/useTableRow.ts | 2 +- .../src/table/TableCollection.ts | 2 +- 13 files changed, 120 insertions(+), 21 deletions(-) diff --git a/packages/@adobe/react-spectrum/src/card/WaterfallLayout.tsx b/packages/@adobe/react-spectrum/src/card/WaterfallLayout.tsx index c1187c6053c..68b660f0e46 100644 --- a/packages/@adobe/react-spectrum/src/card/WaterfallLayout.tsx +++ b/packages/@adobe/react-spectrum/src/card/WaterfallLayout.tsx @@ -203,7 +203,7 @@ export class WaterfallLayout extends BaseLayout implements KeyboardDelegat let rect = new Rect(layoutInfo.rect.maxX + 1, layoutInfo.rect.y, layoutInfo.rect.width + this.horizontalSpacing, layoutInfo.rect.height); key = this._findClosest(layoutInfo.rect, rect)?.key; - if (!key) { + if (key == null) { rect = new Rect(layoutInfo.rect.maxX + 1, 0, layoutInfo.rect.width + this.horizontalSpacing, this.virtualizer.contentSize.height); key = this._findClosest(layoutInfo.rect, rect)?.key; } @@ -218,7 +218,7 @@ export class WaterfallLayout extends BaseLayout implements KeyboardDelegat let rect = new Rect(layoutInfo.rect.x - layoutInfo.rect.width - this.horizontalSpacing - 1, layoutInfo.rect.y, layoutInfo.rect.width + this.horizontalSpacing, layoutInfo.rect.height); key = this._findClosest(layoutInfo.rect, rect)?.key; - if (!key) { + if (key == null) { rect = new Rect(layoutInfo.rect.x - layoutInfo.rect.width - this.horizontalSpacing - 1, 0, layoutInfo.rect.width + this.horizontalSpacing, this.virtualizer.contentSize.height); key = this._findClosest(layoutInfo.rect, rect)?.key; } diff --git a/packages/@react-spectrum/s2/src/TreeView.tsx b/packages/@react-spectrum/s2/src/TreeView.tsx index 0b3ca51252a..fa19922aee9 100644 --- a/packages/@react-spectrum/s2/src/TreeView.tsx +++ b/packages/@react-spectrum/s2/src/TreeView.tsx @@ -594,10 +594,10 @@ function isNextSelected(id: Key | undefined, state: TreeState) { let keyAfter = state.collection.getKeyAfter(id); // We need to skip non-item nodes because the selection manager will map non-item nodes to their parent before checking selection - let node = keyAfter ? state.collection.getItem(keyAfter) : null; - while (node && node.type !== 'item' && keyAfter) { + let node = keyAfter != null ? state.collection.getItem(keyAfter) : null; + while (node && node.type !== 'item' && keyAfter != null) { keyAfter = state.collection.getKeyAfter(keyAfter); - node = keyAfter ? state.collection.getItem(keyAfter) : null; + node = keyAfter != null ? state.collection.getItem(keyAfter) : null; } return keyAfter != null && state.selectionManager.isSelected(keyAfter); diff --git a/packages/react-aria-components/src/DragAndDrop.tsx b/packages/react-aria-components/src/DragAndDrop.tsx index b41e550fc39..28fd90c791b 100644 --- a/packages/react-aria-components/src/DragAndDrop.tsx +++ b/packages/react-aria-components/src/DragAndDrop.tsx @@ -84,7 +84,7 @@ export function useDndPersistedKeys(selectionManager: MultipleSelectionManager, if (nextKey != null) { let targetLevel = dropState.collection.getItem(dropTargetKey)?.level ?? 0; // Skip over any rows that are descendants of the target ("after" position should be after all children) - while (nextKey) { + while (nextKey != null) { let node = dropState.collection.getItem(nextKey); // eslint-disable-next-line max-depth if (!node) { diff --git a/packages/react-aria-components/src/Table.tsx b/packages/react-aria-components/src/Table.tsx index b14c5dd280a..36492be2a34 100644 --- a/packages/react-aria-components/src/Table.tsx +++ b/packages/react-aria-components/src/Table.tsx @@ -137,7 +137,7 @@ class TableCollection extends BaseCollection implements ITableCollection | null; while (lastCell && lastCell.type !== 'cell') { - lastCell = lastCell.prevKey ? this.getItem(lastCell.prevKey) as GridNode | null : null; + lastCell = lastCell.prevKey != null ? this.getItem(lastCell.prevKey) as GridNode | null : null; } if (lastCell) { let numberOfCellsInRow = (lastCell.colIndex ?? lastCell.index) + (lastCell.colSpan ?? 1); @@ -308,7 +308,7 @@ class TableCollection extends BaseCollection implements ITableCollection; let key = self.getKeyAfter(node.key); - node = key ? self.getItem(key) : null; + node = key != null ? self.getItem(key) : null; if (node && node.parentKey === item.parentKey) { break; } diff --git a/packages/react-aria-components/src/Tree.tsx b/packages/react-aria-components/src/Tree.tsx index 276fd15fe31..5bb5a5dade2 100644 --- a/packages/react-aria-components/src/Tree.tsx +++ b/packages/react-aria-components/src/Tree.tsx @@ -109,12 +109,12 @@ class TreeCollection extends BaseCollection { while (node) { yield node as Node; if (node.type === 'section') { - node = node.nextKey ? this.getItem(node.nextKey) : null; + node = node.nextKey != null ? this.getItem(node.nextKey) : null; } else { // This will include both item and content nodes // We handle the content nodes in useCollectionRenderer and ListLayout let key = this.getKeyAfter(node.key); - node = key ? this.getItem(key) : null; + node = key != null ? this.getItem(key) : null; } } } diff --git a/packages/react-aria-components/src/TreeDropTargetDelegate.ts b/packages/react-aria-components/src/TreeDropTargetDelegate.ts index b7407e537c9..253333da7c7 100644 --- a/packages/react-aria-components/src/TreeDropTargetDelegate.ts +++ b/packages/react-aria-components/src/TreeDropTargetDelegate.ts @@ -163,9 +163,9 @@ export class TreeDropTargetDelegate { let parentKey = currentItem?.parentKey; let ancestorTargets: ItemDropTarget[] = []; - while (parentKey) { + while (parentKey != null) { let parentItem = collection.getItem(parentKey); - let nextItem = parentItem?.nextKey ? collection.getItem(parentItem.nextKey) : null; + let nextItem = parentItem?.nextKey != null ? collection.getItem(parentItem.nextKey) : null; let isLastChildAtLevel = !nextItem || nextItem.parentKey !== parentKey; if (isLastChildAtLevel) { @@ -193,7 +193,7 @@ export class TreeDropTargetDelegate { // Handle converting "after" to "before next" for non-ambiguous cases if (potentialTargets.length === 1) { let nextKey = collection.getKeyAfter(target.key); - let nextNode = nextKey ? collection.getItem(nextKey) : null; + let nextNode = nextKey != null ? collection.getItem(nextKey) : null; if (nextKey != null && nextNode && currentItem && nextNode.level != null && currentItem.level != null && nextNode.level > currentItem.level) { let beforeTarget = { type: 'item', @@ -225,7 +225,7 @@ export class TreeDropTargetDelegate { let currentItem = this.state!.collection.getItem(originalTarget.key); let parentKey = currentItem?.parentKey; - if (!parentKey) { + if (parentKey == null) { return potentialTargets[0]; } diff --git a/packages/react-aria-components/test/Select.test.js b/packages/react-aria-components/test/Select.test.js index d7cc5b6d6bf..b88094f5d3b 100644 --- a/packages/react-aria-components/test/Select.test.js +++ b/packages/react-aria-components/test/Select.test.js @@ -343,6 +343,31 @@ describe('Select', () => { expect(select).not.toHaveAttribute('data-invalid'); }); + it('should support arrow key navigation to a falsy key', async () => { + let onSelectionChange = jest.fn(); + let {getByRole} = render( + + ); + + let button = getByRole('button'); + act(() => button.focus()); + + await user.keyboard('{ArrowRight}'); + expect(onSelectionChange).toHaveBeenCalledTimes(1); + expect(onSelectionChange).toHaveBeenLastCalledWith(0); + expect(button).toHaveTextContent('0'); + }); + it('should support falsy (0) as a valid default value', async () => { let {getByRole} = render(