From fe18ac0ac8116445ee93858ce02b111247691623 Mon Sep 17 00:00:00 2001 From: amahajan Date: Wed, 21 May 2025 12:08:11 -0500 Subject: [PATCH 1/6] Add a failing test --- test/browser/column/renderCell.test.tsx | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/browser/column/renderCell.test.tsx b/test/browser/column/renderCell.test.tsx index 98a6a3a31e..71e5de64f1 100644 --- a/test/browser/column/renderCell.test.tsx +++ b/test/browser/column/renderCell.test.tsx @@ -116,6 +116,31 @@ describe('Custom cell renderer', () => { }); }); +test('Focus child if it sets tabIndex', async () => { + const column: Column = { + key: 'test', + name: 'test', + renderCell(props) { + return ( + <> + + External Text + + ); + } + }; + + page.render(); + + const [cell] = getCells(); + const button = page.getByRole('button', { name: 'value: 1' }); + expect(cell).toHaveTextContent('value: 1External Text'); + await userEvent.click(page.getByText('External Text')); + expect(button).toHaveFocus(); +}); + test('Cell should not steal focus when the focus is outside the grid and cell is recreated', async () => { const columns: readonly Column[] = [{ key: 'id', name: 'ID' }]; From 9babb3e796ef46b6637023f1de065572fc9d0be5 Mon Sep 17 00:00:00 2001 From: amahajan Date: Wed, 21 May 2025 12:56:05 -0500 Subject: [PATCH 2/6] Fix test --- src/Cell.tsx | 4 ++-- src/DataGrid.tsx | 17 +++++++++-------- src/hooks/useRovingTabIndex.ts | 9 +++++++++ src/index.ts | 1 + src/types.ts | 9 +++++++-- test/browser/column/renderCell.test.tsx | 4 ++++ 6 files changed, 32 insertions(+), 12 deletions(-) diff --git a/src/Cell.tsx b/src/Cell.tsx index 3516ce920c..a63e66a2a3 100644 --- a/src/Cell.tsx +++ b/src/Cell.tsx @@ -47,8 +47,8 @@ function Cell({ ); const isEditable = isCellEditableUtil(column, row); - function selectCellWrapper(openEditor?: boolean) { - selectCell({ rowIdx, idx: column.idx }, openEditor); + function selectCellWrapper(enableEditor?: boolean) { + selectCell({ rowIdx, idx: column.idx }, { enableEditor }); } function handleMouseEvent( diff --git a/src/DataGrid.tsx b/src/DataGrid.tsx index 64b6bb1323..0cc74a02e9 100644 --- a/src/DataGrid.tsx +++ b/src/DataGrid.tsx @@ -57,6 +57,7 @@ import type { Position, Renderers, RowsChangeData, + SelectCellOptions, SelectHeaderRowEvent, SelectRowEvent, SortColumn @@ -109,7 +110,7 @@ export type DefaultColumnOptions = Pick< export interface DataGridHandle { element: HTMLDivElement | null; scrollToCell: (position: PartialPosition) => void; - selectCell: (position: Position, enableEditor?: Maybe) => void; + selectCell: (position: Position, options: SelectCellOptions) => void; } type SharedDivProps = Pick< @@ -654,7 +655,7 @@ export function DataGrid(props: DataGridPr function handleFocus(event: React.FocusEvent) { // select the first header cell if the focus event is triggered by the grid if (event.target === event.currentTarget) { - selectHeaderCell({ idx: minColIdx, rowIdx: headerRowsCount }); + selectHeaderCell({ idx: minColIdx, rowIdx: headerRowsCount }, { shouldFocusCell: true }); } } @@ -838,20 +839,20 @@ export function DataGrid(props: DataGridPr ); } - function selectCell(position: Position, enableEditor?: Maybe): void { + function selectCell(position: Position, options?: SelectCellOptions): void { if (!isCellWithinSelectionBounds(position)) return; commitEditorChanges(); const samePosition = isSamePosition(selectedPosition, position); - if (enableEditor && isCellEditable(position)) { + if (options?.enableEditor && isCellEditable(position)) { const row = rows[position.rowIdx]; setSelectedPosition({ ...position, mode: 'EDIT', row, originalRow: row }); } else if (samePosition) { // Avoid re-renders if the selected cell state is the same scrollIntoView(getCellToScroll(gridRef.current!)); } else { - setShouldFocusCell(true); + setShouldFocusCell(options?.shouldFocusCell === true); setSelectedPosition({ ...position, mode: 'SELECT' }); } @@ -864,8 +865,8 @@ export function DataGrid(props: DataGridPr } } - function selectHeaderCell({ idx, rowIdx }: Position) { - selectCell({ rowIdx: minRowIdx + rowIdx - 1, idx }); + function selectHeaderCell({ idx, rowIdx }: Position, options?: SelectCellOptions): void { + selectCell({ rowIdx: minRowIdx + rowIdx - 1, idx }, options); } function getNextPosition(key: string, ctrlKey: boolean, shiftKey: boolean): Position { @@ -952,7 +953,7 @@ export function DataGrid(props: DataGridPr isCellWithinBounds: isCellWithinSelectionBounds }); - selectCell(nextSelectedCellPosition); + selectCell(nextSelectedCellPosition, { shouldFocusCell: true }); } function getDraggedOverCellIdx(currentRowIdx: number): number | undefined { diff --git a/src/hooks/useRovingTabIndex.ts b/src/hooks/useRovingTabIndex.ts index db96b23f71..2dedaf7edc 100644 --- a/src/hooks/useRovingTabIndex.ts +++ b/src/hooks/useRovingTabIndex.ts @@ -12,6 +12,15 @@ export function useRovingTabIndex(isSelected: boolean) { function onFocus(event: React.FocusEvent) { if (event.target !== event.currentTarget) { setIsChildFocused(true); + } else { + const elementToFocus = event.currentTarget.querySelector( + '[tabindex="0"]' + ); + + if (elementToFocus !== null) { + elementToFocus.focus({ preventScroll: true }); + setIsChildFocused(true); + } } } diff --git a/src/index.ts b/src/index.ts index 207b80aa9e..98cbf83f5b 100644 --- a/src/index.ts +++ b/src/index.ts @@ -48,6 +48,7 @@ export type { RenderSummaryCellProps, RowHeightArgs, RowsChangeData, + SelectCellOptions, SelectHeaderRowEvent, SelectRowEvent, SortColumn, diff --git a/src/types.ts b/src/types.ts index da6c461468..96d0013147 100644 --- a/src/types.ts +++ b/src/types.ts @@ -167,7 +167,7 @@ interface BaseCellRendererProps 'onCellMouseDown' | 'onCellClick' | 'onCellDoubleClick' | 'onCellContextMenu' > { rowIdx: number; - selectCell: (position: Position, enableEditor?: Maybe) => void; + selectCell: (position: Position, options?: SelectCellOptions) => void; } export interface CellRendererProps @@ -203,7 +203,7 @@ interface SelectCellKeyDownArgs { column: CalculatedColumn; row: TRow; rowIdx: number; - selectCell: (position: Position, enableEditor?: Maybe) => void; + selectCell: (position: Position, options?: SelectCellOptions) => void; } export interface EditCellKeyDownArgs { @@ -334,6 +334,11 @@ export interface Renderers { noRowsFallback?: Maybe; } +export interface SelectCellOptions { + enableEditor?: Maybe; + shouldFocusCell?: Maybe; +} + export interface ColumnWidth { readonly type: 'resized' | 'measured'; readonly width: number; diff --git a/test/browser/column/renderCell.test.tsx b/test/browser/column/renderCell.test.tsx index 71e5de64f1..b127ef8cd7 100644 --- a/test/browser/column/renderCell.test.tsx +++ b/test/browser/column/renderCell.test.tsx @@ -139,6 +139,10 @@ test('Focus child if it sets tabIndex', async () => { expect(cell).toHaveTextContent('value: 1External Text'); await userEvent.click(page.getByText('External Text')); expect(button).toHaveFocus(); + await userEvent.tab(); + expect(button).not.toHaveFocus(); + await userEvent.click(button); + expect(button).toHaveFocus(); }); test('Cell should not steal focus when the focus is outside the grid and cell is recreated', async () => { From 1f946ab8371d308d0c28a64815c093893b63ec02 Mon Sep 17 00:00:00 2001 From: amahajan Date: Wed, 21 May 2025 13:02:51 -0500 Subject: [PATCH 3/6] Fix group selection --- src/GroupRow.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/GroupRow.tsx b/src/GroupRow.tsx index 58330651ab..dce1e4e848 100644 --- a/src/GroupRow.tsx +++ b/src/GroupRow.tsx @@ -49,7 +49,7 @@ function GroupedRow({ const idx = viewportColumns[0].key === SELECT_COLUMN_KEY ? row.level + 1 : row.level; function handleSelectGroup() { - selectCell({ rowIdx, idx: -1 }); + selectCell({ rowIdx, idx: -1 }, { shouldFocusCell: true }); } const selectionValue = useMemo( From 442385b92a7218866df0d3ce3df5cec49f3db34c Mon Sep 17 00:00:00 2001 From: amahajan Date: Wed, 21 May 2025 13:11:08 -0500 Subject: [PATCH 4/6] Simplify --- src/DataGrid.tsx | 13 +++++-------- src/hooks/useRovingTabIndex.ts | 17 +++++++---------- 2 files changed, 12 insertions(+), 18 deletions(-) diff --git a/src/DataGrid.tsx b/src/DataGrid.tsx index 0cc74a02e9..e1bcbb78b7 100644 --- a/src/DataGrid.tsx +++ b/src/DataGrid.tsx @@ -503,7 +503,7 @@ export function DataGrid(props: DataGridPr /** * callbacks */ - const focusCellOrCellContent = useCallback( + const focusCell = useCallback( (shouldScroll = true) => { const cell = getCellToScroll(gridRef.current!); if (cell === null) return; @@ -512,10 +512,7 @@ export function DataGrid(props: DataGridPr scrollIntoView(cell); } - // Focus cell content when available instead of the cell itself - const elementToFocus = - cell.querySelector('[tabindex="0"]') ?? cell; - elementToFocus.focus({ preventScroll: true }); + cell.focus({ preventScroll: true }); }, [gridRef] ); @@ -529,11 +526,11 @@ export function DataGrid(props: DataGridPr focusSinkRef.current.focus({ preventScroll: true }); scrollIntoView(focusSinkRef.current); } else { - focusCellOrCellContent(); + focusCell(); } setShouldFocusCell(false); } - }, [shouldFocusCell, focusCellOrCellContent, selectedPosition.idx]); + }, [shouldFocusCell, focusCell, selectedPosition.idx]); useImperativeHandle(ref, () => ({ element: gridRef.current, @@ -778,7 +775,7 @@ export function DataGrid(props: DataGridPr function handleDragHandleClick() { // keep the focus on the cell but do not scroll - focusCellOrCellContent(false); + focusCell(false); } function handleDragHandleDoubleClick(event: React.MouseEvent) { diff --git a/src/hooks/useRovingTabIndex.ts b/src/hooks/useRovingTabIndex.ts index 2dedaf7edc..92f1c21aa9 100644 --- a/src/hooks/useRovingTabIndex.ts +++ b/src/hooks/useRovingTabIndex.ts @@ -10,17 +10,14 @@ export function useRovingTabIndex(isSelected: boolean) { } function onFocus(event: React.FocusEvent) { - if (event.target !== event.currentTarget) { - setIsChildFocused(true); - } else { - const elementToFocus = event.currentTarget.querySelector( - '[tabindex="0"]' - ); + const elementToFocus = event.currentTarget.querySelector( + '[tabindex="0"]' + ); - if (elementToFocus !== null) { - elementToFocus.focus({ preventScroll: true }); - setIsChildFocused(true); - } + // Focus cell content when available instead of the cell itself + if (elementToFocus !== null) { + elementToFocus.focus({ preventScroll: true }); + setIsChildFocused(true); } } From f762b25b46051b40f3990eb372579c723766c031 Mon Sep 17 00:00:00 2001 From: Aman Mahajan Date: Wed, 21 May 2025 14:51:36 -0500 Subject: [PATCH 5/6] Update test/browser/column/renderCell.test.tsx Co-authored-by: Nicolas Stepien <567105+nstepien@users.noreply.github.com> --- test/browser/column/renderCell.test.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/test/browser/column/renderCell.test.tsx b/test/browser/column/renderCell.test.tsx index b127ef8cd7..8241cb4dd9 100644 --- a/test/browser/column/renderCell.test.tsx +++ b/test/browser/column/renderCell.test.tsx @@ -136,7 +136,6 @@ test('Focus child if it sets tabIndex', async () => { const [cell] = getCells(); const button = page.getByRole('button', { name: 'value: 1' }); - expect(cell).toHaveTextContent('value: 1External Text'); await userEvent.click(page.getByText('External Text')); expect(button).toHaveFocus(); await userEvent.tab(); From b791afc1cbda0bcfc56f1dd1379e6a1cab0e89b3 Mon Sep 17 00:00:00 2001 From: Aman Mahajan Date: Wed, 21 May 2025 15:02:45 -0500 Subject: [PATCH 6/6] Update test/browser/column/renderCell.test.tsx --- test/browser/column/renderCell.test.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/test/browser/column/renderCell.test.tsx b/test/browser/column/renderCell.test.tsx index 8241cb4dd9..0cc3d76387 100644 --- a/test/browser/column/renderCell.test.tsx +++ b/test/browser/column/renderCell.test.tsx @@ -134,7 +134,6 @@ test('Focus child if it sets tabIndex', async () => { page.render(); - const [cell] = getCells(); const button = page.getByRole('button', { name: 'value: 1' }); await userEvent.click(page.getByText('External Text')); expect(button).toHaveFocus();