From 936eceb37cd3626061d24fa24877044b92032d57 Mon Sep 17 00:00:00 2001 From: Aman Mahajan Date: Thu, 24 Apr 2025 11:37:17 -0500 Subject: [PATCH 01/22] Run tests in FF --- .github/workflows/ci.yml | 2 +- vite.config.ts | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9e44b02932..972ae40f00 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,7 +31,7 @@ jobs: - name: Build website run: node --run build:website - name: Install Playwright Browsers - run: npx playwright install chromium + run: npx playwright install chromium firefox - name: Test run: node --run test timeout-minutes: 4 diff --git a/vite.config.ts b/vite.config.ts index 7ec2496bf9..e1ff3b9a6b 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -102,6 +102,10 @@ export default defineConfig(({ command }) => ({ { browser: 'chromium', context: { viewport } + }, + { + browser: 'firefox', + context: { viewport } } ], commands: { resizeColumn, dragFill }, From 3a06ef87e9cd321d14b2bbc6f2024c9a69142f68 Mon Sep 17 00:00:00 2001 From: Aman Mahajan Date: Thu, 24 Apr 2025 12:19:11 -0500 Subject: [PATCH 02/22] Fix editor test, use istanbul for coverage --- package.json | 2 +- test/browser/column/renderEditCell.test.tsx | 9 +++++---- vite.config.ts | 2 +- website/routes/CommonFeatures.tsx | 11 +++++++++++ 4 files changed, 18 insertions(+), 6 deletions(-) diff --git a/package.json b/package.json index 4ba56b6cd8..338fa3aa29 100644 --- a/package.json +++ b/package.json @@ -69,7 +69,7 @@ "@typescript-eslint/parser": "^8.29.0", "@vitejs/plugin-react": "^4.4.1", "@vitest/browser": "^3.1.2", - "@vitest/coverage-v8": "^3.1.2", + "@vitest/coverage-istanbul": "^3.1.2", "@vitest/eslint-plugin": "^1.1.43", "@wyw-in-js/rollup": "^0.6.0", "@wyw-in-js/vite": "^0.6.0", diff --git a/test/browser/column/renderEditCell.test.tsx b/test/browser/column/renderEditCell.test.tsx index 2a3efc9dfc..5c3a23da9d 100644 --- a/test/browser/column/renderEditCell.test.tsx +++ b/test/browser/column/renderEditCell.test.tsx @@ -22,7 +22,7 @@ describe('Editor', () => { await userEvent.keyboard('2'); await userEvent.tab(); await expect.element(editor).not.toBeInTheDocument(); - expect(getCellsAtRowIndex(0)[0]).toHaveTextContent(/^12$/); + expect(getCellsAtRowIndex(0)[0]).toHaveTextContent(12); }); it('should open and commit changes on enter', async () => { @@ -42,7 +42,7 @@ describe('Editor', () => { page.render(); await userEvent.click(getCellsAtRowIndex(0)[0]); await userEvent.keyboard('123{enter}'); - expect(getCellsAtRowIndex(0)[0]).toHaveTextContent(/^1123$/); + expect(getCellsAtRowIndex(0)[0]).toHaveTextContent('123'); }); it('should close editor and discard changes on escape', async () => { @@ -99,6 +99,7 @@ describe('Editor', () => { const editor = page.getByRole('spinbutton', { name: 'col1-editor' }); await expect.element(editor).not.toBeInTheDocument(); expect(getGrid().element().scrollTop).toBe(2000); + await userEvent.dblClick(getCellsAtRowIndex(0)[0]); await userEvent.keyboard('123'); expect(getCellsAtRowIndex(0)).toHaveLength(2); await expect.element(editor).toHaveValue(123); @@ -193,9 +194,9 @@ describe('Editor', () => { }} /> ); - await userEvent.click(getCellsAtRowIndex(0)[1]); + await userEvent.dblClick(getCellsAtRowIndex(0)[1]); await userEvent.keyboard('yz{enter}'); - expect(getCellsAtRowIndex(0)[1]).toHaveTextContent(/^a1yz$/); + expect(getCellsAtRowIndex(0)[1]).toHaveTextContent('yz'); await userEvent.keyboard('x'); await expect .element(page.getByRole('textbox', { name: 'col2-editor' })) diff --git a/vite.config.ts b/vite.config.ts index e1ff3b9a6b..b0a0368a68 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -79,7 +79,7 @@ export default defineConfig(({ command }) => ({ test: { globals: true, coverage: { - provider: 'v8', + provider: 'istanbul', enabled: isCI, include: ['src/**/*.{ts,tsx}'], reporter: ['json'] diff --git a/website/routes/CommonFeatures.tsx b/website/routes/CommonFeatures.tsx index 0540761664..7099e843ce 100644 --- a/website/routes/CommonFeatures.tsx +++ b/website/routes/CommonFeatures.tsx @@ -207,6 +207,17 @@ function getColumns( name: 'Budget', renderCell(props) { return currencyFormatter.format(props.row.budget); + }, + renderEditCell({ row, onRowChange }) { + return ( + onRowChange({ ...row, budget: e.target.valueAsNumber })} + /> + ); } }, { From 86fde9b1e19d00d7139e43ace449d5dbe86314cf Mon Sep 17 00:00:00 2001 From: Aman Mahajan Date: Thu, 24 Apr 2025 12:27:32 -0500 Subject: [PATCH 03/22] Remove optimizeDeps --- vite.config.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/vite.config.ts b/vite.config.ts index b0a0368a68..309947ec4e 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -73,9 +73,6 @@ export default defineConfig(({ command }) => ({ server: { open: true }, - optimizeDeps: { - include: ['@vitest/coverage-v8/browser'] - }, test: { globals: true, coverage: { From 0b070749f9cd62d0659f430c4f13d574e565b70d Mon Sep 17 00:00:00 2001 From: Aman Mahajan Date: Thu, 24 Apr 2025 12:30:23 -0500 Subject: [PATCH 04/22] revert 1 change --- test/browser/column/renderEditCell.test.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/browser/column/renderEditCell.test.tsx b/test/browser/column/renderEditCell.test.tsx index 5c3a23da9d..150752e3fd 100644 --- a/test/browser/column/renderEditCell.test.tsx +++ b/test/browser/column/renderEditCell.test.tsx @@ -99,8 +99,8 @@ describe('Editor', () => { const editor = page.getByRole('spinbutton', { name: 'col1-editor' }); await expect.element(editor).not.toBeInTheDocument(); expect(getGrid().element().scrollTop).toBe(2000); - await userEvent.dblClick(getCellsAtRowIndex(0)[0]); - await userEvent.keyboard('123'); + // `1{backspace}` is needed to fix tests in FF + await userEvent.keyboard('1{backspace}123'); expect(getCellsAtRowIndex(0)).toHaveLength(2); await expect.element(editor).toHaveValue(123); expect(getGrid().element().scrollTop).toBe(0); From ef3f86fd4a29f17a03b605596946fa5b1272608a Mon Sep 17 00:00:00 2001 From: Aman Mahajan Date: Thu, 24 Apr 2025 13:20:01 -0500 Subject: [PATCH 05/22] Fix keyboard tests --- test/browser/keyboardNavigation.test.tsx | 27 ++++++++++++++++-------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/test/browser/keyboardNavigation.test.tsx b/test/browser/keyboardNavigation.test.tsx index ad63ce8622..074f6b360f 100644 --- a/test/browser/keyboardNavigation.test.tsx +++ b/test/browser/keyboardNavigation.test.tsx @@ -26,6 +26,15 @@ const columns = [ { key: 'col7', name: 'col7' } ] as const satisfies Column[]; +async function focusGrid() { + await userEvent.tab(); + + // FF focuses the grid when tabbing into it (bug ?) + if ((document.activeElement as HTMLDivElement).classList.contains('rdg')) { + await userEvent.tab(); + } +} + test('keyboard navigation', async () => { setup({ columns, rows, topSummaryRows, bottomSummaryRows }); @@ -33,7 +42,7 @@ test('keyboard navigation', async () => { await expect.element(getSelectedCell()).not.toBeInTheDocument(); // tab into the grid - await userEvent.tab(); + await focusGrid(); validateCellPosition(0, 0); // tab to the next cell @@ -106,7 +115,7 @@ test('arrow and tab navigation', async () => { setup({ columns, rows, bottomSummaryRows }); // pressing arrowleft on the leftmost cell does nothing - await userEvent.tab(); + await focusGrid(); await userEvent.keyboard('{arrowdown}'); validateCellPosition(0, 1); await userEvent.keyboard('{arrowleft}'); @@ -145,7 +154,7 @@ test('grid enter/exit', async () => { // tab into the grid await userEvent.click(beforeButton); - await userEvent.tab(); + await focusGrid(); validateCellPosition(0, 0); // shift+tab tabs out of the grid if we are at the first cell @@ -179,7 +188,7 @@ test('grid enter/exit', async () => { test('navigation with focusable cell renderer', async () => { setup({ columns, rows: new Array(1), bottomSummaryRows }); - await userEvent.tab(); + await focusGrid(); await userEvent.keyboard('{arrowdown}'); validateCellPosition(0, 1); @@ -220,7 +229,7 @@ test('navigation when header and summary rows have focusable elements', async () ]; setup({ columns, rows: new Array(2), bottomSummaryRows }); - await userEvent.tab(); + await focusGrid(); // should set focus on the header filter expect(document.getElementById('header-filter1')).toHaveFocus(); @@ -260,7 +269,7 @@ test('navigation when selected cell not in the viewport', async () => { columns.push({ key: `col${i}`, name: `col${i}`, frozen: i < 5 }); } setup({ columns, rows, bottomSummaryRows }); - await userEvent.tab(); + await focusGrid(); validateCellPosition(0, 0); await userEvent.keyboard('{Control>}{end}{/Control}{arrowup}{arrowup}'); @@ -299,7 +308,7 @@ test('reset selected cell when column is removed', async () => { const { rerender } = page.render(); - await userEvent.tab(); + await focusGrid(); await userEvent.keyboard('{arrowdown}{arrowright}'); validateCellPosition(1, 1); @@ -321,7 +330,7 @@ test('reset selected cell when row is removed', async () => { const { rerender } = page.render(); - await userEvent.tab(); + await focusGrid(); await userEvent.keyboard('{arrowdown}{arrowdown}{arrowright}'); validateCellPosition(1, 2); @@ -332,7 +341,7 @@ test('reset selected cell when row is removed', async () => { test('should not change the left and right arrow behavior for right to left languages', async () => { setup({ rows, columns, direction: 'rtl' }); - await userEvent.tab(); + await focusGrid(); validateCellPosition(0, 0); await userEvent.tab(); validateCellPosition(1, 0); From 5c4e3add7e7cf5369a7214de2b3ff4b9d1120c95 Mon Sep 17 00:00:00 2001 From: Aman Mahajan Date: Thu, 24 Apr 2025 13:21:55 -0500 Subject: [PATCH 06/22] Revert --- website/routes/CommonFeatures.tsx | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/website/routes/CommonFeatures.tsx b/website/routes/CommonFeatures.tsx index 7099e843ce..0540761664 100644 --- a/website/routes/CommonFeatures.tsx +++ b/website/routes/CommonFeatures.tsx @@ -207,17 +207,6 @@ function getColumns( name: 'Budget', renderCell(props) { return currencyFormatter.format(props.row.budget); - }, - renderEditCell({ row, onRowChange }) { - return ( - onRowChange({ ...row, budget: e.target.valueAsNumber })} - /> - ); } }, { From 6ab1bbd08880ce09deee20b6a7930d87bb27229b Mon Sep 17 00:00:00 2001 From: Aman Mahajan Date: Thu, 24 Apr 2025 13:37:46 -0500 Subject: [PATCH 07/22] Fix rowHeight tests --- test/browser/keyboardNavigation.test.tsx | 28 +++++++++--------------- test/browser/rowHeight.test.ts | 6 ++--- test/browser/utils.tsx | 11 +++++++++- 3 files changed, 23 insertions(+), 22 deletions(-) diff --git a/test/browser/keyboardNavigation.test.tsx b/test/browser/keyboardNavigation.test.tsx index 074f6b360f..5234e67096 100644 --- a/test/browser/keyboardNavigation.test.tsx +++ b/test/browser/keyboardNavigation.test.tsx @@ -3,6 +3,7 @@ import { page, userEvent } from '@vitest/browser/context'; import { DataGrid, SelectColumn } from '../../src'; import type { Column } from '../../src'; import { + focusIntoGrid, getCellsAtRowIndex, getSelectedCell, scrollGrid, @@ -26,15 +27,6 @@ const columns = [ { key: 'col7', name: 'col7' } ] as const satisfies Column[]; -async function focusGrid() { - await userEvent.tab(); - - // FF focuses the grid when tabbing into it (bug ?) - if ((document.activeElement as HTMLDivElement).classList.contains('rdg')) { - await userEvent.tab(); - } -} - test('keyboard navigation', async () => { setup({ columns, rows, topSummaryRows, bottomSummaryRows }); @@ -42,7 +34,7 @@ test('keyboard navigation', async () => { await expect.element(getSelectedCell()).not.toBeInTheDocument(); // tab into the grid - await focusGrid(); + await focusIntoGrid(); validateCellPosition(0, 0); // tab to the next cell @@ -115,7 +107,7 @@ test('arrow and tab navigation', async () => { setup({ columns, rows, bottomSummaryRows }); // pressing arrowleft on the leftmost cell does nothing - await focusGrid(); + await focusIntoGrid(); await userEvent.keyboard('{arrowdown}'); validateCellPosition(0, 1); await userEvent.keyboard('{arrowleft}'); @@ -154,7 +146,7 @@ test('grid enter/exit', async () => { // tab into the grid await userEvent.click(beforeButton); - await focusGrid(); + await focusIntoGrid(); validateCellPosition(0, 0); // shift+tab tabs out of the grid if we are at the first cell @@ -188,7 +180,7 @@ test('grid enter/exit', async () => { test('navigation with focusable cell renderer', async () => { setup({ columns, rows: new Array(1), bottomSummaryRows }); - await focusGrid(); + await focusIntoGrid(); await userEvent.keyboard('{arrowdown}'); validateCellPosition(0, 1); @@ -229,7 +221,7 @@ test('navigation when header and summary rows have focusable elements', async () ]; setup({ columns, rows: new Array(2), bottomSummaryRows }); - await focusGrid(); + await focusIntoGrid(); // should set focus on the header filter expect(document.getElementById('header-filter1')).toHaveFocus(); @@ -269,7 +261,7 @@ test('navigation when selected cell not in the viewport', async () => { columns.push({ key: `col${i}`, name: `col${i}`, frozen: i < 5 }); } setup({ columns, rows, bottomSummaryRows }); - await focusGrid(); + await focusIntoGrid(); validateCellPosition(0, 0); await userEvent.keyboard('{Control>}{end}{/Control}{arrowup}{arrowup}'); @@ -308,7 +300,7 @@ test('reset selected cell when column is removed', async () => { const { rerender } = page.render(); - await focusGrid(); + await focusIntoGrid(); await userEvent.keyboard('{arrowdown}{arrowright}'); validateCellPosition(1, 1); @@ -330,7 +322,7 @@ test('reset selected cell when row is removed', async () => { const { rerender } = page.render(); - await focusGrid(); + await focusIntoGrid(); await userEvent.keyboard('{arrowdown}{arrowdown}{arrowright}'); validateCellPosition(1, 2); @@ -341,7 +333,7 @@ test('reset selected cell when row is removed', async () => { test('should not change the left and right arrow behavior for right to left languages', async () => { setup({ rows, columns, direction: 'rtl' }); - await focusGrid(); + await focusIntoGrid(); validateCellPosition(0, 0); await userEvent.tab(); validateCellPosition(1, 0); diff --git a/test/browser/rowHeight.test.ts b/test/browser/rowHeight.test.ts index 1c9628c6aa..6f51e05303 100644 --- a/test/browser/rowHeight.test.ts +++ b/test/browser/rowHeight.test.ts @@ -1,7 +1,7 @@ import { page, userEvent } from '@vitest/browser/context'; import type { Column, DataGridProps } from '../../src'; -import { getRows, setup } from './utils'; +import { focusIntoGrid, getRows, setup } from './utils'; type Row = number; @@ -30,7 +30,7 @@ test('rowHeight is number', async () => { }); expect(getRows()).toHaveLength(30); - await userEvent.tab(); + await focusIntoGrid(); expect(grid.scrollTop).toBe(0); await userEvent.keyboard('{Control>}{end}'); expect(grid.scrollTop + grid.clientHeight).toBe(grid.scrollHeight); @@ -46,7 +46,7 @@ test('rowHeight is function', async () => { }); expect(getRows()).toHaveLength(22); - await userEvent.tab(); + await focusIntoGrid(); expect(grid.scrollTop).toBe(0); await userEvent.keyboard('{Control>}{end}'); expect(grid.scrollTop + grid.clientHeight).toBe(grid.scrollHeight); diff --git a/test/browser/utils.tsx b/test/browser/utils.tsx index 5c476acefd..b30afda3cb 100644 --- a/test/browser/utils.tsx +++ b/test/browser/utils.tsx @@ -1,4 +1,4 @@ -import { page } from '@vitest/browser/context'; +import { page, userEvent } from '@vitest/browser/context'; import { css } from '@linaria/core'; import { DataGrid } from '../../src'; @@ -76,3 +76,12 @@ export async function scrollGrid({ await new Promise(requestAnimationFrame); } } + +export async function focusIntoGrid() { + await userEvent.tab(); + + // FF focuses the grid when tabbing into it (bug ?) + if ((document.activeElement as HTMLDivElement).classList.contains('rdg')) { + await userEvent.tab(); + } +} From 215d4c71ab4eafb3f571f0c7d018dd470f638bd2 Mon Sep 17 00:00:00 2001 From: Aman Mahajan Date: Thu, 24 Apr 2025 16:43:44 -0500 Subject: [PATCH 08/22] Fix focus in FF --- src/DataGrid.tsx | 22 ++++++++++++++++++---- test/browser/keyboardNavigation.test.tsx | 19 +++++++++---------- test/browser/rowHeight.test.ts | 6 +++--- test/browser/utils.tsx | 11 +---------- vite.config.ts | 4 ++-- 5 files changed, 33 insertions(+), 29 deletions(-) diff --git a/src/DataGrid.tsx b/src/DataGrid.tsx index 452d461dc3..ff108ea6a0 100644 --- a/src/DataGrid.tsx +++ b/src/DataGrid.tsx @@ -484,6 +484,7 @@ export function DataGrid(props: DataGridPr const selectedCellIsWithinViewportBounds = isCellWithinViewportBounds(selectedPosition); const scrollHeight = headerRowHeight + totalRowHeight + summaryRowsHeight + horizontalScrollbarHeight; + const shouldFocusGrid = !selectedCellIsWithinSelectionBounds; /** * The identity of the wrapper function is stable so it won't break memoization @@ -499,9 +500,7 @@ export function DataGrid(props: DataGridPr const selectRowLatest = useLatestFunc(selectRow); const handleFormatterRowChangeLatest = useLatestFunc(updateRow); const selectCellLatest = useLatestFunc(selectCell); - const selectHeaderCellLatest = useLatestFunc(({ idx, rowIdx }: Position) => { - selectCell({ rowIdx: minRowIdx + rowIdx - 1, idx }); - }); + const selectHeaderCellLatest = useLatestFunc(selectHeaderCell); /** * callbacks @@ -661,6 +660,16 @@ export function DataGrid(props: DataGridPr } } + // FF focuses the grid when tabbing into it (bug ?) + function handleFocus(event: React.FocusEvent) { + if (event.target === event.currentTarget) { + if (shouldFocusGrid) { + // Select the first header cell if there is no selected cell + selectHeaderCell({ idx: 0, rowIdx: headerRowsCount }); + } + } + } + function handleScroll(event: React.UIEvent) { const { scrollTop, scrollLeft } = event.currentTarget; flushSync(() => { @@ -867,6 +876,10 @@ export function DataGrid(props: DataGridPr } } + function selectHeaderCell({ idx, rowIdx }: Position) { + selectCell({ rowIdx: minRowIdx + rowIdx - 1, idx }); + } + function getNextPosition(key: string, ctrlKey: boolean, shiftKey: boolean): Position { const { idx, rowIdx } = selectedPosition; const isRowSelected = selectedCellIsWithinSelectionBounds && idx === -1; @@ -1216,6 +1229,7 @@ export function DataGrid(props: DataGridPr } dir={direction} ref={gridRef} + onFocus={handleFocus} onScroll={handleScroll} onKeyDown={handleKeyDown} onCopy={handleCellCopy} @@ -1252,7 +1266,7 @@ export function DataGrid(props: DataGridPr selectedPosition.rowIdx === mainHeaderRowIdx ? selectedPosition.idx : undefined } selectCell={selectHeaderCellLatest} - shouldFocusGrid={!selectedCellIsWithinSelectionBounds} + shouldFocusGrid={shouldFocusGrid} direction={direction} /> diff --git a/test/browser/keyboardNavigation.test.tsx b/test/browser/keyboardNavigation.test.tsx index 5234e67096..ad63ce8622 100644 --- a/test/browser/keyboardNavigation.test.tsx +++ b/test/browser/keyboardNavigation.test.tsx @@ -3,7 +3,6 @@ import { page, userEvent } from '@vitest/browser/context'; import { DataGrid, SelectColumn } from '../../src'; import type { Column } from '../../src'; import { - focusIntoGrid, getCellsAtRowIndex, getSelectedCell, scrollGrid, @@ -34,7 +33,7 @@ test('keyboard navigation', async () => { await expect.element(getSelectedCell()).not.toBeInTheDocument(); // tab into the grid - await focusIntoGrid(); + await userEvent.tab(); validateCellPosition(0, 0); // tab to the next cell @@ -107,7 +106,7 @@ test('arrow and tab navigation', async () => { setup({ columns, rows, bottomSummaryRows }); // pressing arrowleft on the leftmost cell does nothing - await focusIntoGrid(); + await userEvent.tab(); await userEvent.keyboard('{arrowdown}'); validateCellPosition(0, 1); await userEvent.keyboard('{arrowleft}'); @@ -146,7 +145,7 @@ test('grid enter/exit', async () => { // tab into the grid await userEvent.click(beforeButton); - await focusIntoGrid(); + await userEvent.tab(); validateCellPosition(0, 0); // shift+tab tabs out of the grid if we are at the first cell @@ -180,7 +179,7 @@ test('grid enter/exit', async () => { test('navigation with focusable cell renderer', async () => { setup({ columns, rows: new Array(1), bottomSummaryRows }); - await focusIntoGrid(); + await userEvent.tab(); await userEvent.keyboard('{arrowdown}'); validateCellPosition(0, 1); @@ -221,7 +220,7 @@ test('navigation when header and summary rows have focusable elements', async () ]; setup({ columns, rows: new Array(2), bottomSummaryRows }); - await focusIntoGrid(); + await userEvent.tab(); // should set focus on the header filter expect(document.getElementById('header-filter1')).toHaveFocus(); @@ -261,7 +260,7 @@ test('navigation when selected cell not in the viewport', async () => { columns.push({ key: `col${i}`, name: `col${i}`, frozen: i < 5 }); } setup({ columns, rows, bottomSummaryRows }); - await focusIntoGrid(); + await userEvent.tab(); validateCellPosition(0, 0); await userEvent.keyboard('{Control>}{end}{/Control}{arrowup}{arrowup}'); @@ -300,7 +299,7 @@ test('reset selected cell when column is removed', async () => { const { rerender } = page.render(); - await focusIntoGrid(); + await userEvent.tab(); await userEvent.keyboard('{arrowdown}{arrowright}'); validateCellPosition(1, 1); @@ -322,7 +321,7 @@ test('reset selected cell when row is removed', async () => { const { rerender } = page.render(); - await focusIntoGrid(); + await userEvent.tab(); await userEvent.keyboard('{arrowdown}{arrowdown}{arrowright}'); validateCellPosition(1, 2); @@ -333,7 +332,7 @@ test('reset selected cell when row is removed', async () => { test('should not change the left and right arrow behavior for right to left languages', async () => { setup({ rows, columns, direction: 'rtl' }); - await focusIntoGrid(); + await userEvent.tab(); validateCellPosition(0, 0); await userEvent.tab(); validateCellPosition(1, 0); diff --git a/test/browser/rowHeight.test.ts b/test/browser/rowHeight.test.ts index 6f51e05303..1c9628c6aa 100644 --- a/test/browser/rowHeight.test.ts +++ b/test/browser/rowHeight.test.ts @@ -1,7 +1,7 @@ import { page, userEvent } from '@vitest/browser/context'; import type { Column, DataGridProps } from '../../src'; -import { focusIntoGrid, getRows, setup } from './utils'; +import { getRows, setup } from './utils'; type Row = number; @@ -30,7 +30,7 @@ test('rowHeight is number', async () => { }); expect(getRows()).toHaveLength(30); - await focusIntoGrid(); + await userEvent.tab(); expect(grid.scrollTop).toBe(0); await userEvent.keyboard('{Control>}{end}'); expect(grid.scrollTop + grid.clientHeight).toBe(grid.scrollHeight); @@ -46,7 +46,7 @@ test('rowHeight is function', async () => { }); expect(getRows()).toHaveLength(22); - await focusIntoGrid(); + await userEvent.tab(); expect(grid.scrollTop).toBe(0); await userEvent.keyboard('{Control>}{end}'); expect(grid.scrollTop + grid.clientHeight).toBe(grid.scrollHeight); diff --git a/test/browser/utils.tsx b/test/browser/utils.tsx index b30afda3cb..5c476acefd 100644 --- a/test/browser/utils.tsx +++ b/test/browser/utils.tsx @@ -1,4 +1,4 @@ -import { page, userEvent } from '@vitest/browser/context'; +import { page } from '@vitest/browser/context'; import { css } from '@linaria/core'; import { DataGrid } from '../../src'; @@ -76,12 +76,3 @@ export async function scrollGrid({ await new Promise(requestAnimationFrame); } } - -export async function focusIntoGrid() { - await userEvent.tab(); - - // FF focuses the grid when tabbing into it (bug ?) - if ((document.activeElement as HTMLDivElement).classList.contains('rdg')) { - await userEvent.tab(); - } -} diff --git a/vite.config.ts b/vite.config.ts index 309947ec4e..ab0f20b539 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -81,7 +81,7 @@ export default defineConfig(({ command }) => ({ include: ['src/**/*.{ts,tsx}'], reporter: ['json'] }, - testTimeout: isCI ? 10000 : 5000, + testTimeout: 20_000, restoreMocks: true, sequence: { shuffle: true @@ -107,7 +107,7 @@ export default defineConfig(({ command }) => ({ ], commands: { resizeColumn, dragFill }, viewport, - headless: true, + headless: false, screenshotFailures: process.env.CI !== 'true' }, setupFiles: ['test/setupBrowser.ts'] From 398fe1375f65856895b93a6c81a8f9720aa55875 Mon Sep 17 00:00:00 2001 From: Aman Mahajan Date: Thu, 24 Apr 2025 16:44:07 -0500 Subject: [PATCH 09/22] headless --- vite.config.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vite.config.ts b/vite.config.ts index ab0f20b539..853f3836bf 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -107,7 +107,7 @@ export default defineConfig(({ command }) => ({ ], commands: { resizeColumn, dragFill }, viewport, - headless: false, + headless: true, screenshotFailures: process.env.CI !== 'true' }, setupFiles: ['test/setupBrowser.ts'] From 313bde69937dd893d9748c899053e5a3b578f115 Mon Sep 17 00:00:00 2001 From: Aman Mahajan Date: Thu, 24 Apr 2025 16:50:01 -0500 Subject: [PATCH 10/22] ESLint --- src/DataGrid.tsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/DataGrid.tsx b/src/DataGrid.tsx index ff108ea6a0..31234294a9 100644 --- a/src/DataGrid.tsx +++ b/src/DataGrid.tsx @@ -666,6 +666,8 @@ export function DataGrid(props: DataGridPr if (shouldFocusGrid) { // Select the first header cell if there is no selected cell selectHeaderCell({ idx: 0, rowIdx: headerRowsCount }); + } else { + // TODO: check if this is needed } } } From c6decaefc00224add99c0b1452f1688d6acdbfb1 Mon Sep 17 00:00:00 2001 From: Aman Mahajan Date: Fri, 25 Apr 2025 08:49:57 -0500 Subject: [PATCH 11/22] Tweak focus logic --- src/DataGrid.tsx | 18 ++++++------------ src/HeaderCell.tsx | 15 ++------------- src/HeaderRow.tsx | 3 --- 3 files changed, 8 insertions(+), 28 deletions(-) diff --git a/src/DataGrid.tsx b/src/DataGrid.tsx index 31234294a9..b9169a3710 100644 --- a/src/DataGrid.tsx +++ b/src/DataGrid.tsx @@ -660,16 +660,8 @@ export function DataGrid(props: DataGridPr } } - // FF focuses the grid when tabbing into it (bug ?) - function handleFocus(event: React.FocusEvent) { - if (event.target === event.currentTarget) { - if (shouldFocusGrid) { - // Select the first header cell if there is no selected cell - selectHeaderCell({ idx: 0, rowIdx: headerRowsCount }); - } else { - // TODO: check if this is needed - } - } + function handleFocus() { + selectHeaderCell({ idx: 0, rowIdx: headerRowsCount }); } function handleScroll(event: React.UIEvent) { @@ -1200,6 +1192,9 @@ export function DataGrid(props: DataGridPr aria-multiselectable={isSelectable ? true : undefined} aria-colcount={columns.length} aria-rowcount={ariaRowCount} + // Scrollable containers without tabIndex are keyboard focusable in Chrome only if there is no focusable element inside + // whereas they are are always focusable in Firefox. We need to set tabIndex to have a consistent behavior across browsers. + tabIndex={shouldFocusGrid ? 0 : -1} className={clsx( rootClassname, { @@ -1231,7 +1226,7 @@ export function DataGrid(props: DataGridPr } dir={direction} ref={gridRef} - onFocus={handleFocus} + onFocus={shouldFocusGrid ? handleFocus : undefined} onScroll={handleScroll} onKeyDown={handleKeyDown} onCopy={handleCellCopy} @@ -1268,7 +1263,6 @@ export function DataGrid(props: DataGridPr selectedPosition.rowIdx === mainHeaderRowIdx ? selectedPosition.idx : undefined } selectCell={selectHeaderCellLatest} - shouldFocusGrid={shouldFocusGrid} direction={direction} /> diff --git a/src/HeaderCell.tsx b/src/HeaderCell.tsx index a374d9b903..11209025ec 100644 --- a/src/HeaderCell.tsx +++ b/src/HeaderCell.tsx @@ -61,7 +61,6 @@ type SharedHeaderRowProps = Pick< | 'selectCell' | 'onColumnResize' | 'onColumnResizeEnd' - | 'shouldFocusGrid' | 'direction' | 'onColumnsReorder' >; @@ -85,7 +84,6 @@ export default function HeaderCell({ sortColumns, onSortColumnsChange, selectCell, - shouldFocusGrid, direction, dragDropKey }: HeaderCellProps) { @@ -155,14 +153,6 @@ export default function HeaderCell({ } } - function handleFocus(event: React.FocusEvent) { - onFocus?.(event); - if (shouldFocusGrid) { - // Select the first header cell if there is no selected cell - selectCell({ idx: 0, rowIdx }); - } - } - function onKeyDown(event: React.KeyboardEvent) { const { key } = event; if (sortable && (key === ' ' || key === 'Enter')) { @@ -254,14 +244,13 @@ export default function HeaderCell({ aria-rowspan={rowSpan} aria-selected={isCellSelected} aria-sort={ariaSort} - // set the tabIndex to 0 when there is no selected cell so grid can receive focus - tabIndex={shouldFocusGrid ? 0 : tabIndex} + tabIndex={tabIndex} className={className} style={{ ...getHeaderCellStyle(column, rowIdx, rowSpan), ...getCellStyle(column, colSpan) }} - onFocus={handleFocus} + onFocus={onFocus} onClick={onClick} onKeyDown={onKeyDown} {...draggableProps} diff --git a/src/HeaderRow.tsx b/src/HeaderRow.tsx index 9ece21827f..e1e0ec0b18 100644 --- a/src/HeaderRow.tsx +++ b/src/HeaderRow.tsx @@ -22,7 +22,6 @@ export interface HeaderRowProps extends SharedDataGr selectCell: (position: Position) => void; lastFrozenColumnIndex: number; selectedCellIdx: number | undefined; - shouldFocusGrid: boolean; direction: Direction; headerRowClass: Maybe; } @@ -59,7 +58,6 @@ function HeaderRow({ lastFrozenColumnIndex, selectedCellIdx, selectCell, - shouldFocusGrid, direction }: HeaderRowProps) { const dragDropKey = useId(); @@ -85,7 +83,6 @@ function HeaderRow({ onSortColumnsChange={onSortColumnsChange} sortColumns={sortColumns} selectCell={selectCell} - shouldFocusGrid={shouldFocusGrid && index === 0} direction={direction} dragDropKey={dragDropKey} /> From a84c72e4061c700f86b2280222949faf4895e921 Mon Sep 17 00:00:00 2001 From: Aman Mahajan Date: Fri, 25 Apr 2025 09:29:54 -0500 Subject: [PATCH 12/22] Fix test --- src/DataGrid.tsx | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/DataGrid.tsx b/src/DataGrid.tsx index b9169a3710..7bfaf5ae15 100644 --- a/src/DataGrid.tsx +++ b/src/DataGrid.tsx @@ -660,8 +660,11 @@ export function DataGrid(props: DataGridPr } } - function handleFocus() { - selectHeaderCell({ idx: 0, rowIdx: headerRowsCount }); + 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: 0, rowIdx: headerRowsCount }); + } } function handleScroll(event: React.UIEvent) { From ea0674455573fdd73ae4b92400a027cf93fe164a Mon Sep 17 00:00:00 2001 From: Aman Mahajan Date: Fri, 25 Apr 2025 09:38:54 -0500 Subject: [PATCH 13/22] Fix test in FF --- test/browser/dragFill.test.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/browser/dragFill.test.tsx b/test/browser/dragFill.test.tsx index c0c0fdd8fe..e6f5481382 100644 --- a/test/browser/dragFill.test.tsx +++ b/test/browser/dragFill.test.tsx @@ -70,7 +70,9 @@ test('should update single row using mouse', async () => { await commands.dragFill('a1', 'a2'); await expect.element(getCellsAtRowIndex(1)[0]).toHaveTextContent('a1'); await expect.element(getCellsAtRowIndex(2)[0]).toHaveTextContent('a3'); - await expect.element(getCellsAtRowIndex(0)[0]).toHaveFocus(); + // https://bugzilla.mozilla.org/show_bug.cgi?id=1961462 + const rowIdx = navigator.userAgent.includes('Firefox') ? 1 : 0; + await expect.element(getCellsAtRowIndex(rowIdx)[0]).toHaveFocus(); }); test('should update multiple rows using mouse', async () => { From 3bfbfac87ddbf8aab2e677d4b72bf5a73748d751 Mon Sep 17 00:00:00 2001 From: amahajan Date: Fri, 16 May 2025 10:55:57 -0500 Subject: [PATCH 14/22] Revert firefox workaround --- test/browser/dragFill.test.tsx | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/browser/dragFill.test.tsx b/test/browser/dragFill.test.tsx index e6f5481382..c0c0fdd8fe 100644 --- a/test/browser/dragFill.test.tsx +++ b/test/browser/dragFill.test.tsx @@ -70,9 +70,7 @@ test('should update single row using mouse', async () => { await commands.dragFill('a1', 'a2'); await expect.element(getCellsAtRowIndex(1)[0]).toHaveTextContent('a1'); await expect.element(getCellsAtRowIndex(2)[0]).toHaveTextContent('a3'); - // https://bugzilla.mozilla.org/show_bug.cgi?id=1961462 - const rowIdx = navigator.userAgent.includes('Firefox') ? 1 : 0; - await expect.element(getCellsAtRowIndex(rowIdx)[0]).toHaveFocus(); + await expect.element(getCellsAtRowIndex(0)[0]).toHaveFocus(); }); test('should update multiple rows using mouse', async () => { From a00f60e76ab8294bd4e40fd152db7cca98970bfd Mon Sep 17 00:00:00 2001 From: amahajan Date: Fri, 16 May 2025 11:10:00 -0500 Subject: [PATCH 15/22] try beta version --- package.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index 75936bc6a9..80d043ee5b 100644 --- a/package.json +++ b/package.json @@ -68,8 +68,8 @@ "@typescript-eslint/eslint-plugin": "^8.32.0", "@typescript-eslint/parser": "^8.32.0", "@vitejs/plugin-react": "^4.4.1", - "@vitest/browser": "^3.1.3", - "@vitest/coverage-istanbul": "^3.1.3", + "@vitest/browser": "^3.2.0-beta.1", + "@vitest/coverage-istanbul": "^3.2.0-beta.1", "@vitest/eslint-plugin": "^1.1.44", "@wyw-in-js/rollup": "^0.6.0", "@wyw-in-js/vite": "^0.6.0", @@ -94,7 +94,7 @@ "rolldown": "^1.0.0-beta.8", "typescript": "~5.8.2", "vite": "^6.3.5", - "vitest": "^3.1.3", + "vitest": "^3.2.0-beta.1", "vitest-browser-react": "^0.1.1" }, "peerDependencies": { From cecf08bd9b66abba0328318d8d0b6bb58f3e032f Mon Sep 17 00:00:00 2001 From: amahajan Date: Fri, 16 May 2025 11:21:22 -0500 Subject: [PATCH 16/22] Revert "try beta version" This reverts commit a00f60e76ab8294bd4e40fd152db7cca98970bfd. --- package.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index 80d043ee5b..75936bc6a9 100644 --- a/package.json +++ b/package.json @@ -68,8 +68,8 @@ "@typescript-eslint/eslint-plugin": "^8.32.0", "@typescript-eslint/parser": "^8.32.0", "@vitejs/plugin-react": "^4.4.1", - "@vitest/browser": "^3.2.0-beta.1", - "@vitest/coverage-istanbul": "^3.2.0-beta.1", + "@vitest/browser": "^3.1.3", + "@vitest/coverage-istanbul": "^3.1.3", "@vitest/eslint-plugin": "^1.1.44", "@wyw-in-js/rollup": "^0.6.0", "@wyw-in-js/vite": "^0.6.0", @@ -94,7 +94,7 @@ "rolldown": "^1.0.0-beta.8", "typescript": "~5.8.2", "vite": "^6.3.5", - "vitest": "^3.2.0-beta.1", + "vitest": "^3.1.3", "vitest-browser-react": "^0.1.1" }, "peerDependencies": { From faafac22014571c876bf231d90b923a405d179d4 Mon Sep 17 00:00:00 2001 From: amahajan Date: Fri, 16 May 2025 11:21:59 -0500 Subject: [PATCH 17/22] Try pinning vitest --- package.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index 75936bc6a9..273ed68a5c 100644 --- a/package.json +++ b/package.json @@ -68,8 +68,8 @@ "@typescript-eslint/eslint-plugin": "^8.32.0", "@typescript-eslint/parser": "^8.32.0", "@vitejs/plugin-react": "^4.4.1", - "@vitest/browser": "^3.1.3", - "@vitest/coverage-istanbul": "^3.1.3", + "@vitest/browser": "3.1.1", + "@vitest/coverage-istanbul": "3.1.1", "@vitest/eslint-plugin": "^1.1.44", "@wyw-in-js/rollup": "^0.6.0", "@wyw-in-js/vite": "^0.6.0", @@ -94,7 +94,7 @@ "rolldown": "^1.0.0-beta.8", "typescript": "~5.8.2", "vite": "^6.3.5", - "vitest": "^3.1.3", + "vitest": "3.1.1", "vitest-browser-react": "^0.1.1" }, "peerDependencies": { From a50bd1df2fdba36655521849a339036949f4236f Mon Sep 17 00:00:00 2001 From: amahajan Date: Fri, 16 May 2025 11:25:11 -0500 Subject: [PATCH 18/22] Revert "Try pinning vitest" This reverts commit faafac22014571c876bf231d90b923a405d179d4. --- package.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index 273ed68a5c..75936bc6a9 100644 --- a/package.json +++ b/package.json @@ -68,8 +68,8 @@ "@typescript-eslint/eslint-plugin": "^8.32.0", "@typescript-eslint/parser": "^8.32.0", "@vitejs/plugin-react": "^4.4.1", - "@vitest/browser": "3.1.1", - "@vitest/coverage-istanbul": "3.1.1", + "@vitest/browser": "^3.1.3", + "@vitest/coverage-istanbul": "^3.1.3", "@vitest/eslint-plugin": "^1.1.44", "@wyw-in-js/rollup": "^0.6.0", "@wyw-in-js/vite": "^0.6.0", @@ -94,7 +94,7 @@ "rolldown": "^1.0.0-beta.8", "typescript": "~5.8.2", "vite": "^6.3.5", - "vitest": "3.1.1", + "vitest": "^3.1.3", "vitest-browser-react": "^0.1.1" }, "peerDependencies": { From 6a9e3ab92050a7b2eb17fcab062af0d036fff78b Mon Sep 17 00:00:00 2001 From: amahajan Date: Fri, 16 May 2025 11:26:51 -0500 Subject: [PATCH 19/22] Revrt timeout --- vite.config.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vite.config.ts b/vite.config.ts index 853f3836bf..309947ec4e 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -81,7 +81,7 @@ export default defineConfig(({ command }) => ({ include: ['src/**/*.{ts,tsx}'], reporter: ['json'] }, - testTimeout: 20_000, + testTimeout: isCI ? 10000 : 5000, restoreMocks: true, sequence: { shuffle: true From 0a38ac8b6bb1960ed20ba9bc89392aa2e7c31faa Mon Sep 17 00:00:00 2001 From: amahajan Date: Fri, 16 May 2025 11:31:14 -0500 Subject: [PATCH 20/22] try `maxWorkers` --- vite.config.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/vite.config.ts b/vite.config.ts index 309947ec4e..70ae18c0f6 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -75,6 +75,7 @@ export default defineConfig(({ command }) => ({ }, test: { globals: true, + maxWorkers: 8, coverage: { provider: 'istanbul', enabled: isCI, From 2f534b5eb2fef894ef81f749cdde55a12d3817b3 Mon Sep 17 00:00:00 2001 From: amahajan Date: Fri, 16 May 2025 11:34:44 -0500 Subject: [PATCH 21/22] `maxWorkers: 4,` --- vite.config.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vite.config.ts b/vite.config.ts index 70ae18c0f6..306d69dc03 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -75,7 +75,7 @@ export default defineConfig(({ command }) => ({ }, test: { globals: true, - maxWorkers: 8, + maxWorkers: 4, coverage: { provider: 'istanbul', enabled: isCI, From 256839edf2900098c04d6a392ededb7599176558 Mon Sep 17 00:00:00 2001 From: amahajan Date: Fri, 16 May 2025 11:35:33 -0500 Subject: [PATCH 22/22] increate testTimeout --- vite.config.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vite.config.ts b/vite.config.ts index 306d69dc03..db829a6c45 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -82,7 +82,7 @@ export default defineConfig(({ command }) => ({ include: ['src/**/*.{ts,tsx}'], reporter: ['json'] }, - testTimeout: isCI ? 10000 : 5000, + testTimeout: 20_000, restoreMocks: true, sequence: { shuffle: true