From bd32184b8936e5a4f6519dd9463f00e189a570e2 Mon Sep 17 00:00:00 2001 From: Nicolas Stepien <567105+nstepien@users.noreply.github.com> Date: Fri, 6 Mar 2026 17:42:01 +0000 Subject: [PATCH 1/2] Minor test improvements (#3991) --- test/browser/column/grouping.test.ts | 6 +-- test/browser/direction.test.ts | 14 +++--- test/browser/dragFill.test.tsx | 13 ++++-- test/browser/events.test.tsx | 35 +++++++-------- test/browser/keyboardNavigation.test.tsx | 57 +++++++++++++++--------- test/browser/rowHeight.test.ts | 8 ++-- test/browser/styles.css | 4 ++ test/browser/utils.tsx | 33 +------------- test/failOnConsole.ts | 20 ++++----- test/setupBrowser.ts | 19 ++++---- vite.config.ts | 2 +- 11 files changed, 99 insertions(+), 112 deletions(-) create mode 100644 test/browser/styles.css diff --git a/test/browser/column/grouping.test.ts b/test/browser/column/grouping.test.ts index 0e2a70a1fd..e86b552666 100644 --- a/test/browser/column/grouping.test.ts +++ b/test/browser/column/grouping.test.ts @@ -1,7 +1,7 @@ import { page, userEvent } from 'vitest/browser'; import type { ColumnOrColumnGroup } from '../../../src'; -import { safeTab, setup, tabIntoGrid, testCount, validateCellPosition } from '../utils'; +import { safeTab, setup, testCount, validateCellPosition } from '../utils'; const grid = page.getGrid(); const headerRows = grid.getHeaderRow(); @@ -252,12 +252,12 @@ test('grouping', async () => { }); test('keyboard navigation', async () => { - await setup({ columns, rows: [{}] }, true); + await setup({ columns, rows: [{}] }); // no initial active position await expect.element(grid.getActiveCell()).not.toBeInTheDocument(); - await tabIntoGrid(); + await safeTab(); await validateCellPosition(0, 3); // arrow navigation diff --git a/test/browser/direction.test.ts b/test/browser/direction.test.ts index 870eece8cf..be9c61f55e 100644 --- a/test/browser/direction.test.ts +++ b/test/browser/direction.test.ts @@ -1,7 +1,7 @@ import { page, userEvent } from 'vitest/browser'; import type { Column } from '../../src'; -import { setup, tabIntoGrid } from './utils'; +import { safeTab, setup } from './utils'; const grid = page.getGrid(); const activeCell = grid.getActiveCell(); @@ -25,27 +25,27 @@ const columns: readonly Column[] = [ const rows: readonly Row[] = []; test('should use left to right direction by default', async () => { - await setup({ rows, columns }, true); + await setup({ rows, columns }); await expect.element(grid).toHaveAttribute('dir', 'ltr'); - await tabIntoGrid(); + await safeTab(); await expect.element(activeCell).toHaveTextContent('ID'); await userEvent.keyboard('{ArrowRight}'); await expect.element(activeCell).toHaveTextContent('Name'); }); test('should use left to right direction if direction prop is set to ltr', async () => { - await setup({ rows, columns, direction: 'ltr' }, true); + await setup({ rows, columns, direction: 'ltr' }); await expect.element(grid).toHaveAttribute('dir', 'ltr'); - await tabIntoGrid(); + await safeTab(); await expect.element(activeCell).toHaveTextContent('ID'); await userEvent.keyboard('{ArrowRight}'); await expect.element(activeCell).toHaveTextContent('Name'); }); test('should use right to left direction if direction prop is set to rtl', async () => { - await setup({ rows, columns, direction: 'rtl' }, true); + await setup({ rows, columns, direction: 'rtl' }); await expect.element(grid).toHaveAttribute('dir', 'rtl'); - await tabIntoGrid(); + await safeTab(); await expect.element(activeCell).toHaveTextContent('ID'); await userEvent.keyboard('{ArrowLeft}'); await expect.element(activeCell).toHaveTextContent('Name'); diff --git a/test/browser/dragFill.test.tsx b/test/browser/dragFill.test.tsx index 3bc715fef9..16b125b353 100644 --- a/test/browser/dragFill.test.tsx +++ b/test/browser/dragFill.test.tsx @@ -88,9 +88,14 @@ test('should allow drag up using mouse', async () => { test('should focus the cell when drag handle is clicked', async () => { await setup(); - await userEvent.click(getCellsAtRowIndex(0).nth(0)); - await userEvent.click(document.body); - await expect.element(document.body).toHaveFocus(); + const cell = getCellsAtRowIndex(0).nth(0); + + await userEvent.click(cell); + await expect.element(cell).toHaveFocus(); + + cell.element().blur(); + await expect.element(cell).not.toHaveFocus(); + await userEvent.click(dragHandle); - await expect.element(getCellsAtRowIndex(0).nth(0)).toHaveFocus(); + await expect.element(cell).toHaveFocus(); }); diff --git a/test/browser/events.test.tsx b/test/browser/events.test.tsx index e0336ef99c..9f01cfbdc4 100644 --- a/test/browser/events.test.tsx +++ b/test/browser/events.test.tsx @@ -1,7 +1,7 @@ import { page, userEvent } from 'vitest/browser'; import { DataGrid } from '../../src'; -import type { Column, DataGridProps } from '../../src'; +import type { Column } from '../../src'; import { safeTab } from './utils'; interface Row { @@ -55,7 +55,9 @@ const rows: readonly Row[] = [ describe('Events', () => { it('should not select cell if onCellMouseDown prevents grid default', async () => { await page.render( - { if (args.column.key === 'col1') { event.preventGridDefault(); @@ -71,7 +73,9 @@ describe('Events', () => { it('should be able to open editor editor on single click using onCellClick', async () => { await page.render( - { if (args.column.key === 'col2') { event.preventGridDefault(); @@ -88,7 +92,9 @@ describe('Events', () => { it('should not open editor editor on double click if onCellDoubleClick prevents default', async () => { await page.render( - { if (args.column.key === 'col1') { event.preventGridDefault(); @@ -104,7 +110,9 @@ describe('Events', () => { it('should call onCellContextMenu when cell is right clicked', async () => { const onCellContextMenu = vi.fn(); - await page.render(); + await page.render( + + ); expect(onCellContextMenu).not.toHaveBeenCalled(); await userEvent.click(page.getCell({ name: '1' }), { button: 'right' }); expect(onCellContextMenu).toHaveBeenCalledExactlyOnceWith( @@ -122,7 +130,9 @@ describe('Events', () => { it('should call onActivePositionChange when cell selection is changed', async () => { const onActivePositionChange = vi.fn(); - await page.render(); + await page.render( + + ); expect(onActivePositionChange).not.toHaveBeenCalled(); @@ -181,16 +191,3 @@ describe('Events', () => { expect(onActivePositionChange).toHaveBeenCalledTimes(6); }); }); - -type EventProps = Pick< - DataGridProps, - | 'onCellMouseDown' - | 'onCellClick' - | 'onCellDoubleClick' - | 'onCellContextMenu' - | 'onActivePositionChange' ->; - -function EventTest(props: EventProps) { - return ; -} diff --git a/test/browser/keyboardNavigation.test.tsx b/test/browser/keyboardNavigation.test.tsx index 62b44b0d56..46806e1f4d 100644 --- a/test/browser/keyboardNavigation.test.tsx +++ b/test/browser/keyboardNavigation.test.tsx @@ -7,12 +7,13 @@ import { safeTab, scrollGrid, setup, - tabIntoGrid, testCount, validateCellPosition } from './utils'; const activeCell = page.getActiveCell(); +const activeSelectAllCheckbox = activeCell.getSelectAllCheckbox(); +const activeSelectCheckbox = activeCell.getByRole('checkbox', { name: 'Select', exact: true }); type Row = undefined; @@ -31,13 +32,13 @@ const columns: readonly Column[] = [ ]; test('keyboard navigation', async () => { - await setup({ columns, rows, topSummaryRows, bottomSummaryRows }, true); + await setup({ columns, rows, topSummaryRows, bottomSummaryRows }); // no initial active position await expect.element(activeCell).not.toBeInTheDocument(); // tab into the grid - await tabIntoGrid(); + await safeTab(); await validateCellPosition(0, 0); // tab to the next cell @@ -107,10 +108,10 @@ test('keyboard navigation', async () => { }); test('arrow and tab navigation', async () => { - await setup({ columns, rows, bottomSummaryRows }, true); + await setup({ columns, rows, bottomSummaryRows }); // pressing arrowleft on the leftmost cell does nothing - await tabIntoGrid(); + await safeTab(); await userEvent.keyboard('{arrowdown}'); await validateCellPosition(0, 1); await userEvent.keyboard('{arrowleft}'); @@ -132,7 +133,17 @@ test('arrow and tab navigation', async () => { }); test('grid enter/exit', async () => { - await setup({ columns, rows: Array.from({ length: 5 }), bottomSummaryRows }, true); + await page.render( + <> + + ({ length: 5 })} + bottomSummaryRows={bottomSummaryRows} + /> + + + ); const beforeButton = page.getByRole('button', { name: 'Before' }); const afterButton = page.getByRole('button', { name: 'After' }); @@ -141,8 +152,10 @@ test('grid enter/exit', async () => { await expect.element(activeCell).not.toBeInTheDocument(); // tab into the grid - await tabIntoGrid(); + await safeTab(); + await safeTab(); await validateCellPosition(0, 0); + await expect.element(activeSelectAllCheckbox).toHaveFocus(); // shift+tab tabs out of the grid if we are at the first cell await safeTab(true); @@ -150,9 +163,11 @@ test('grid enter/exit', async () => { await safeTab(); await validateCellPosition(0, 0); + await expect.element(activeSelectAllCheckbox).toHaveFocus(); await userEvent.keyboard('{arrowdown}{arrowdown}'); await validateCellPosition(0, 2); + await expect.element(activeSelectCheckbox).toHaveFocus(); // tab should focus the last active cell // click outside the grid @@ -160,12 +175,14 @@ test('grid enter/exit', async () => { await safeTab(); await userEvent.keyboard('{arrowdown}'); await validateCellPosition(0, 3); + await expect.element(activeSelectCheckbox).toHaveFocus(); // shift+tab should focus the last active cell + // click outside the grid await userEvent.click(afterButton); await safeTab(true); await validateCellPosition(0, 3); - await expect.element(activeCell.getByRole('checkbox')).toHaveFocus(); + await expect.element(activeSelectCheckbox).toHaveFocus(); // tab tabs out of the grid if we are at the last cell await userEvent.keyboard('{Control>}{end}{/Control}'); @@ -174,16 +191,15 @@ test('grid enter/exit', async () => { }); test('navigation with focusable cell renderer', async () => { - await setup({ columns, rows: Array.from({ length: 1 }), bottomSummaryRows }, true); - await tabIntoGrid(); + await setup({ columns, rows: Array.from({ length: 1 }), bottomSummaryRows }); + await safeTab(); await userEvent.keyboard('{arrowdown}'); await validateCellPosition(0, 1); // cell should not set tabIndex to 0 if it contains a focusable cell renderer await expect.element(activeCell).toHaveAttribute('tabIndex', '-1'); - const checkbox = activeCell.getByRole('checkbox'); - await expect.element(checkbox).toHaveFocus(); - await expect.element(checkbox).toHaveAttribute('tabIndex', '0'); + await expect.element(activeSelectCheckbox).toHaveFocus(); + await expect.element(activeSelectCheckbox).toHaveAttribute('tabIndex', '0'); await safeTab(); await validateCellPosition(1, 1); @@ -215,11 +231,8 @@ test('navigation when header and summary rows have focusable elements', async () } ]; - await setup( - { columns, rows: Array.from({ length: 2 }), bottomSummaryRows: [1, 2] }, - true - ); - await tabIntoGrid(); + await setup({ columns, rows: Array.from({ length: 2 }), bottomSummaryRows: [1, 2] }); + await safeTab(); // should set focus on the header filter await expect.element(page.getByTestId('header-filter1')).toHaveFocus(); @@ -259,8 +272,8 @@ test('navigation when active cell not in the viewport', async () => { for (let i = 0; i < 99; i++) { columns.push({ key: `col${i}`, name: `col${i}`, frozen: i < 5 }); } - await setup({ columns, rows, bottomSummaryRows }, true); - await tabIntoGrid(); + await setup({ columns, rows, bottomSummaryRows }); + await safeTab(); await validateCellPosition(0, 0); await userEvent.keyboard('{Control>}{end}{/Control}{arrowup}{arrowup}'); @@ -330,8 +343,8 @@ test('reset active cell when row is removed', async () => { }); test('should not change the left and right arrow behavior for right to left languages', async () => { - await setup({ columns, rows, direction: 'rtl' }, true); - await tabIntoGrid(); + await setup({ columns, rows, direction: 'rtl' }); + await safeTab(); await validateCellPosition(0, 0); await safeTab(); await validateCellPosition(1, 0); diff --git a/test/browser/rowHeight.test.ts b/test/browser/rowHeight.test.ts index b4c5087e0b..5ed9647509 100644 --- a/test/browser/rowHeight.test.ts +++ b/test/browser/rowHeight.test.ts @@ -1,7 +1,7 @@ import { page, userEvent } from 'vitest/browser'; import type { Column, DataGridProps } from '../../src'; -import { setup, tabIntoGrid, testRowCount } from './utils'; +import { safeTab, setup, testRowCount } from './utils'; const grid = page.getGrid(); @@ -19,7 +19,7 @@ function setupGrid(rowHeight: DataGridProps['rowHeight']) { width: 80 }); } - return setup({ columns, rows, rowHeight }, true); + return setup({ columns, rows, rowHeight }); } async function expectGridRows(rowHeightFn: (row: number) => number, expected: string) { @@ -36,7 +36,7 @@ test('rowHeight is number', async () => { '40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px 40px' }); await testRowCount(30); - await tabIntoGrid(); + await safeTab(); await expect.element(grid).toHaveProperty('scrollTop', 0); await userEvent.keyboard('{Control>}{end}'); const gridEl = grid.element(); @@ -52,7 +52,7 @@ test('rowHeight is function', async () => { }); await testRowCount(22); - await tabIntoGrid(); + await safeTab(); await expect.element(grid).toHaveProperty('scrollTop', 0); await userEvent.keyboard('{Control>}{end}'); const gridEl = grid.element(); diff --git a/test/browser/styles.css b/test/browser/styles.css new file mode 100644 index 0000000000..742f2b657c --- /dev/null +++ b/test/browser/styles.css @@ -0,0 +1,4 @@ +.rdg { + block-size: 1080px; + scrollbar-width: none; +} diff --git a/test/browser/utils.tsx b/test/browser/utils.tsx index ab4145c6b5..0b4558ef72 100644 --- a/test/browser/utils.tsx +++ b/test/browser/utils.tsx @@ -1,34 +1,10 @@ import { page, userEvent, type Locator } from 'vitest/browser'; -import { css } from 'ecij'; import { DataGrid } from '../../src'; import type { DataGridProps } from '../../src'; -export function setup( - props: DataGridProps, - renderBeforeAfterButtons = false -) { - const grid = ( - - ); - - if (renderBeforeAfterButtons) { - return page.render( - <> - - {grid} -
- - - ); - } - return page.render(grid); +export function setup(props: DataGridProps) { + return page.render(); } export function getRowWithCell(cell: Locator) { @@ -57,11 +33,6 @@ export async function scrollGrid(options: ScrollToOptions) { }); } -export async function tabIntoGrid() { - await userEvent.click(page.getByRole('button', { name: 'Before' })); - await safeTab(); -} - export function testCount(locator: Locator, expectedCount: number) { return expect.element(locator).toHaveLength(expectedCount); } diff --git a/test/failOnConsole.ts b/test/failOnConsole.ts index 791e26fc25..c5acfa094f 100644 --- a/test/failOnConsole.ts +++ b/test/failOnConsole.ts @@ -23,16 +23,16 @@ beforeAll(() => { }); afterEach(() => { - if (!consoleErrorOrConsoleWarnWereCalled) { - return; - } - - consoleErrorOrConsoleWarnWereCalled = false; - - // Errors thrown in `afterEach` will short-circuit subsequent `afterEach` hooks, - // thus preventing tests from being cleaned up properly and affecting other tests. - // We must therefore wait for tests to "finish" before throwing the error. + // Wait for both the test and `afterEach` hooks to complete to ensure all logs are caught onTestFinished(() => { - throw new Error('console.error() and/or console.warn() were called during the test'); + // eslint-disable-next-line vitest/no-standalone-expect + expect + .soft( + consoleErrorOrConsoleWarnWereCalled, + 'console.error() and/or console.warn() were called during the test' + ) + .toBe(false); + + consoleErrorOrConsoleWarnWereCalled = false; }); }); diff --git a/test/setupBrowser.ts b/test/setupBrowser.ts index 5f87c78123..f02b875252 100644 --- a/test/setupBrowser.ts +++ b/test/setupBrowser.ts @@ -90,15 +90,12 @@ beforeEach(async () => { afterEach(() => { vi.useRealTimers(); - // eslint-disable-next-line @eslint-react/purity - if (!document.hasFocus()) { - // Errors thrown in `afterEach` will short-circuit subsequent `afterEach` hooks, - // thus preventing tests from being cleaned up properly and affecting other tests. - // We must therefore wait for tests to "finish" before throwing the error. - onTestFinished(() => { - throw new Error( - 'Focus is set on a browser UI element at the end of a test. Use safeTab() to return focus to the page.' - ); - }); - } + // eslint-disable-next-line vitest/no-standalone-expect + expect + .soft( + // eslint-disable-next-line @eslint-react/purity + document.hasFocus(), + 'Focus is set on a browser UI element at the end of a test. Use safeTab() to return focus to the page.' + ) + .toBe(true); }); diff --git a/vite.config.ts b/vite.config.ts index 34086f656a..5c4afce666 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -133,7 +133,7 @@ export default defineConfig( ui: false, screenshotFailures: !isCI }, - setupFiles: ['test/setupBrowser.ts', 'test/failOnConsole.ts'] + setupFiles: ['test/browser/styles.css', 'test/setupBrowser.ts', 'test/failOnConsole.ts'] } }, { From e5335107ec7cebf2a668cf6305df9899e2c9d4c7 Mon Sep 17 00:00:00 2001 From: Nicolas Stepien <567105+nstepien@users.noreply.github.com> Date: Fri, 6 Mar 2026 17:51:09 +0000 Subject: [PATCH 2/2] Optimize `classnames` and `renderValue` (#3992) --- src/Cell.tsx | 4 +--- src/DataGrid.tsx | 8 +------- src/HeaderCell.tsx | 16 +++++++++------- src/cellRenderers/renderValue.tsx | 6 +----- src/utils/styleUtils.ts | 24 +++++------------------- 5 files changed, 17 insertions(+), 41 deletions(-) diff --git a/src/Cell.tsx b/src/Cell.tsx index 160c7fd58b..a71450f0bd 100644 --- a/src/Cell.tsx +++ b/src/Cell.tsx @@ -39,9 +39,7 @@ function Cell({ const { cellClass } = column; className = getCellClassname( column, - { - [cellDraggedOverClassname]: isDraggedOver - }, + isDraggedOver && cellDraggedOverClassname, typeof cellClass === 'function' ? cellClass(row) : cellClass, className ); diff --git a/src/DataGrid.tsx b/src/DataGrid.tsx index be71e20960..a3c977985b 100644 --- a/src/DataGrid.tsx +++ b/src/DataGrid.tsx @@ -1170,13 +1170,7 @@ export function DataGrid(props: DataGridPr // Scrollable containers without tabIndex are keyboard focusable in Chrome only if there is no focusable element inside // whereas they are always focusable in Firefox. We need to set tabIndex to have a consistent behavior across browsers. tabIndex={-1} - className={classnames( - rootClassname, - { - [viewportDraggingClassname]: isDragging - }, - className - )} + className={classnames(rootClassname, isDragging && viewportDraggingClassname, className)} style={{ ...style, // set scrollPadding to correctly scroll to non-sticky cells/rows diff --git a/src/HeaderCell.tsx b/src/HeaderCell.tsx index 457a139410..c677186e10 100644 --- a/src/HeaderCell.tsx +++ b/src/HeaderCell.tsx @@ -116,13 +116,15 @@ export default function HeaderCell({ sortDirection && !priority ? (sortDirection === 'ASC' ? 'ascending' : 'descending') : undefined; const { sortable, resizable, draggable } = column; - const className = getCellClassname(column, column.headerCellClass, { - [cellSortableClassname]: sortable, - [cellResizableClassname]: resizable, - [cellDraggableClassname]: draggable, - [cellDraggingClassname]: isDragging, - [cellOverClassname]: isOver - }); + const className = getCellClassname( + column, + column.headerCellClass, + sortable && cellSortableClassname, + resizable && cellResizableClassname, + draggable && cellDraggableClassname, + isDragging && cellDraggingClassname, + isOver && cellOverClassname + ); function onSort(ctrlClick: boolean) { if (onSortColumnsChange == null) return; diff --git a/src/cellRenderers/renderValue.tsx b/src/cellRenderers/renderValue.tsx index 8ea0bb06da..8fdceb3ec1 100644 --- a/src/cellRenderers/renderValue.tsx +++ b/src/cellRenderers/renderValue.tsx @@ -1,9 +1,5 @@ import type { RenderCellProps } from '../types'; export function renderValue(props: RenderCellProps) { - try { - return props.row[props.column.key as keyof R] as React.ReactNode; - } catch { - return null; - } + return props.row?.[props.column.key as keyof R] as React.ReactNode; } diff --git a/src/utils/styleUtils.ts b/src/utils/styleUtils.ts index 6b97ac54c2..cfc720e69a 100644 --- a/src/utils/styleUtils.ts +++ b/src/utils/styleUtils.ts @@ -38,37 +38,23 @@ export function getCellStyle( }; } -type ClassValue = Maybe | Record | false; +type ClassValue = Maybe; export function classnames(...args: readonly ClassValue[]) { let classname = ''; for (const arg of args) { - if (arg) { - if (typeof arg === 'string') { - classname += ` ${arg}`; - } else if (typeof arg === 'object') { - for (const key in arg) { - if (arg[key]) { - classname += ` ${key}`; - } - } - } + if (typeof arg === 'string') { + classname += ` ${arg}`; } } - return classname.trimStart(); + return classname.slice(1); } export function getCellClassname( column: CalculatedColumn, ...extraClasses: readonly ClassValue[] ): string { - return classnames( - cellClassname, - { - [cellFrozenClassname]: column.frozen - }, - ...extraClasses - ); + return classnames(cellClassname, column.frozen && cellFrozenClassname, ...extraClasses); }