From d1c900b06dff8538947dfdfdef73a603c54d8229 Mon Sep 17 00:00:00 2001 From: damien Date: Fri, 22 Jan 2021 12:39:40 +0100 Subject: [PATCH 1/8] fix #889 shift+space to select row --- .../hooks/features/keyboard/useKeyboard.ts | 18 +++++++++--------- packages/grid/_modules_/grid/utils/domUtils.ts | 11 +++++++---- .../grid/_modules_/grid/utils/keyboardUtils.ts | 16 ++++++++-------- 3 files changed, 24 insertions(+), 21 deletions(-) diff --git a/packages/grid/_modules_/grid/hooks/features/keyboard/useKeyboard.ts b/packages/grid/_modules_/grid/hooks/features/keyboard/useKeyboard.ts index 784e35784f20d..529f5e46c6b61 100644 --- a/packages/grid/_modules_/grid/hooks/features/keyboard/useKeyboard.ts +++ b/packages/grid/_modules_/grid/hooks/features/keyboard/useKeyboard.ts @@ -76,7 +76,7 @@ export const useKeyboard = (gridRootRef: React.RefObject, apiRef ); const navigateCells = React.useCallback( - (code: string, isCtrlPressed: boolean) => { + (key: string, isCtrlPressed: boolean) => { const cellEl = findParentElementFromClassName( document.activeElement as HTMLDivElement, CELL_CSS_CLASS, @@ -90,13 +90,13 @@ export const useKeyboard = (gridRootRef: React.RefObject, apiRef : totalRowCount; let nextCellIndexes: CellIndexCoordinates; - if (isArrowKeys(code)) { - nextCellIndexes = getNextCellIndexes(code, { + if (isArrowKeys(key)) { + nextCellIndexes = getNextCellIndexes(key, { colIndex: currentColIndex, rowIndex: currentRowIndex, }); - } else if (isHomeOrEndKeys(code)) { - const colIdx = code === 'Home' ? 0 : colCount - 1; + } else if (isHomeOrEndKeys(key)) { + const colIdx = key === 'Home' ? 0 : colCount - 1; if (!isCtrlPressed) { // we go to the current row, first col, or last col! @@ -111,10 +111,10 @@ export const useKeyboard = (gridRootRef: React.RefObject, apiRef } nextCellIndexes = { colIndex: colIdx, rowIndex }; } - } else if (isPageKeys(code) || isSpaceKey(code)) { + } else if (isPageKeys(key) || isSpaceKey(key)) { const nextRowIndex = currentRowIndex + - (code.indexOf('Down') > -1 || isSpaceKey(code) + (key.indexOf('Down') > -1 || isSpaceKey(key) ? containerSizes!.viewportPageSize : -1 * containerSizes!.viewportPageSize); nextCellIndexes = { colIndex: currentColIndex, rowIndex: nextRowIndex }; @@ -167,7 +167,7 @@ export const useKeyboard = (gridRootRef: React.RefObject, apiRef }, [apiRef]); const expandSelection = React.useCallback( - (code: string) => { + (key: string) => { const rowEl = findParentElementFromClassName( document.activeElement as HTMLDivElement, ROW_CSS_CLASS, @@ -190,7 +190,7 @@ export const useKeyboard = (gridRootRef: React.RefObject, apiRef selectionFromRowIndex = selectedRowsIndex[diffWithCurrentIndex.indexOf(minIndex)]; } - const nextCellIndexes = navigateCells(code, false); + const nextCellIndexes = navigateCells(key, false); // We select the rows in between const rowIds = Array(Math.abs(nextCellIndexes.rowIndex - selectionFromRowIndex) + 1) .fill( diff --git a/packages/grid/_modules_/grid/utils/domUtils.ts b/packages/grid/_modules_/grid/utils/domUtils.ts index d628ea557eaf5..150f2c27a319a 100644 --- a/packages/grid/_modules_/grid/utils/domUtils.ts +++ b/packages/grid/_modules_/grid/utils/domUtils.ts @@ -13,14 +13,17 @@ export function findParentElementFromClassName(elem: Element, className: string) return elem.closest(`.${className}`); } -export function isCell(elem: Element | null): boolean { - return elem != null && findParentElementFromClassName(elem, CELL_CSS_CLASS) !== null; -} - export function isCellRoot(elem: Element | null): boolean { return elem != null && elem.classList.contains(CELL_CSS_CLASS); } +export function isCell(elem: Element | null): boolean { + return ( + elem != null && + (isCellRoot(elem) || findParentElementFromClassName(elem, CELL_CSS_CLASS) !== null) + ); +} + export function isHeaderTitleContainer(elem: Element): boolean { return elem && findParentElementFromClassName(elem, HEADER_CELL_TITLE_CSS_CLASS) !== null; } diff --git a/packages/grid/_modules_/grid/utils/keyboardUtils.ts b/packages/grid/_modules_/grid/utils/keyboardUtils.ts index e4e5739019989..a5b84a646bc50 100644 --- a/packages/grid/_modules_/grid/utils/keyboardUtils.ts +++ b/packages/grid/_modules_/grid/utils/keyboardUtils.ts @@ -1,10 +1,10 @@ export const MULTIPLE_SELECTION_KEYS = ['Meta', 'Control']; -export const isMultipleKey = (code: string): boolean => MULTIPLE_SELECTION_KEYS.indexOf(code) > -1; -export const isTabKey = (code: string): boolean => code === 'Tab'; -export const isSpaceKey = (code: string): boolean => code === 'Space'; -export const isArrowKeys = (code: string): boolean => code.indexOf('Arrow') === 0; -export const isHomeOrEndKeys = (code: string): boolean => code === 'Home' || code === 'End'; -export const isPageKeys = (code: string): boolean => code.indexOf('Page') === 0; +export const isMultipleKey = (key: string): boolean => MULTIPLE_SELECTION_KEYS.indexOf(key) > -1; +export const isTabKey = (key: string): boolean => key === 'Tab'; +export const isSpaceKey = (key: string): boolean => key === ' '; +export const isArrowKeys = (key: string): boolean => key.indexOf('Arrow') === 0; +export const isHomeOrEndKeys = (key: string): boolean => key === 'Home' || key === 'End'; +export const isPageKeys = (key: string): boolean => key.indexOf('Page') === 0; -export const isNavigationKey = (code: string) => - isHomeOrEndKeys(code) || isArrowKeys(code) || isPageKeys(code) || isSpaceKey(code); +export const isNavigationKey = (key: string) => + isHomeOrEndKeys(key) || isArrowKeys(key) || isPageKeys(key) || isSpaceKey(key); From 193824eb36be6b0008a08b3b5d8d3b68c0138a01 Mon Sep 17 00:00:00 2001 From: damien Date: Fri, 22 Jan 2021 16:51:23 +0100 Subject: [PATCH 2/8] add test --- .../src/tests/keyboard.DataGrid.test.tsx | 41 +++++++++++++------ test/utils/helperFn.ts | 20 +++++++++ 2 files changed, 48 insertions(+), 13 deletions(-) diff --git a/packages/grid/data-grid/src/tests/keyboard.DataGrid.test.tsx b/packages/grid/data-grid/src/tests/keyboard.DataGrid.test.tsx index 68ca1614623c2..0cde29b0bbf3b 100644 --- a/packages/grid/data-grid/src/tests/keyboard.DataGrid.test.tsx +++ b/packages/grid/data-grid/src/tests/keyboard.DataGrid.test.tsx @@ -12,11 +12,14 @@ import { } from 'test/utils'; import { spy } from 'sinon'; import { expect } from 'chai'; -import { getActiveCell } from 'test/utils/helperFn'; +import { focusCell, getActiveCell, getCell, getRow } from 'test/utils/helperFn'; import { DataGrid } from '@material-ui/data-grid'; import { useData } from 'packages/storybook/src/hooks/useData'; import { Columns } from 'packages/grid/_modules_/grid/models/colDef/colDef'; +const SPACE_KEY = { key: ' ', code: 'Space', shiftKey: false }; +const SHIFT_SPACE_KEY = { ...SPACE_KEY, shiftKey: true }; + describe(' - Keyboard', () => { // TODO v5: replace with createClientRender const render = createClientRenderStrictMode(); @@ -117,11 +120,9 @@ describe(' - Keyboard', () => { , ); - const firstCell = document.querySelector( - '[role="cell"][data-rowindex="0"][aria-colindex="0"]', - ) as HTMLElement; - firstCell.focus(); - fireEvent.keyDown(firstCell, { key: 'ArrowRight' }); + focusCell(0, 0); + expect(getActiveCell()).to.equal('0-0'); + fireEvent.keyDown(document.activeElement!, { key: 'ArrowRight' }); expect(handleKeyDown.returnValues).to.deep.equal([true]); }); @@ -140,8 +141,7 @@ describe(' - Keyboard', () => { /* eslint-disable material-ui/disallow-active-element-as-key-event-target */ it('cell navigation with arrows', () => { render(); - // @ts-ignore - document.querySelector('[role="cell"][data-rowindex="0"][aria-colindex="0"]').focus(); + focusCell(0, 0); expect(getActiveCell()).to.equal('0-0'); fireEvent.keyDown(document.activeElement!, { key: 'ArrowRight' }); expect(getActiveCell()).to.equal('0-1'); @@ -153,17 +153,32 @@ describe(' - Keyboard', () => { expect(getActiveCell()).to.equal('0-0'); }); + it('Shift + Space should select a row', () => { + render(); + focusCell(0, 0); + expect(getActiveCell()).to.equal('0-0'); + fireEvent.keyDown(document.activeElement!, SHIFT_SPACE_KEY); + const row = getRow(0); + const isSelected = row.classList.contains('Mui-selected'); + expect(isSelected).to.equal(true); + }); + + it('Space only should go to the bottom of the page', async () => { + render(); + focusCell(0, 0); + expect(getActiveCell()).to.equal('0-0'); + fireEvent.keyDown(document.activeElement!, SPACE_KEY); + expect(getActiveCell()).to.equal('3-0'); + }); + it('Home / End navigation', async () => { render(); - // @ts-ignore - document.querySelector('[role="cell"][data-rowindex="1"][aria-colindex="1"]').focus(); + focusCell(1, 1); expect(getActiveCell()).to.equal('1-1'); fireEvent.keyDown(document.activeElement!, { key: 'Home' }); expect(getActiveCell()).to.equal('1-0'); fireEvent.keyDown(document.activeElement!, { key: 'End' }); - await waitFor(() => - document.querySelector('[role="cell"][data-rowindex="1"][aria-colindex="19"]'), - ); + await waitFor(() => getCell(1, 19)); expect(getActiveCell()).to.equal('1-19'); }); /* eslint-enable material-ui/disallow-active-element-as-key-event-target */ diff --git a/test/utils/helperFn.ts b/test/utils/helperFn.ts index c2ea9682ada57..cde34b279fc8a 100644 --- a/test/utils/helperFn.ts +++ b/test/utils/helperFn.ts @@ -49,3 +49,23 @@ export function getColumnHeaders() { (node) => node!.textContent, ); } +export function getCell(rowIndex: number, colIndex: number): HTMLElement { + const cell = document.querySelector( + `[role="cell"][data-rowindex="${rowIndex}"][aria-colindex="${colIndex}"]`, + ); + if (cell == null) { + throw new Error(`Cell ${rowIndex} ${colIndex} not found`); + } + return cell as HTMLElement; +} +export function focusCell(rowIndex: number, colIndex: number) { + const cell = getCell(rowIndex, colIndex); + cell.focus(); +} +export function getRow(rowIndex: number): HTMLElement { + const row = document.querySelector(`[role="row"][data-rowindex="${rowIndex}"]`); + if (row == null) { + throw new Error(`Row ${rowIndex} not found`); + } + return row as HTMLElement; +} From b253ea00ce19b5bda7e4893dba54ffbfac33d22e Mon Sep 17 00:00:00 2001 From: damien Date: Fri, 22 Jan 2021 17:44:58 +0100 Subject: [PATCH 3/8] remove focusCell --- .../data-grid/src/tests/keyboard.DataGrid.test.tsx | 14 +++++++------- test/utils/helperFn.ts | 7 +++---- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/packages/grid/data-grid/src/tests/keyboard.DataGrid.test.tsx b/packages/grid/data-grid/src/tests/keyboard.DataGrid.test.tsx index 0cde29b0bbf3b..a2fee98b8ad96 100644 --- a/packages/grid/data-grid/src/tests/keyboard.DataGrid.test.tsx +++ b/packages/grid/data-grid/src/tests/keyboard.DataGrid.test.tsx @@ -12,7 +12,7 @@ import { } from 'test/utils'; import { spy } from 'sinon'; import { expect } from 'chai'; -import { focusCell, getActiveCell, getCell, getRow } from 'test/utils/helperFn'; +import { getActiveCell, getCell, getRow } from 'test/utils/helperFn'; import { DataGrid } from '@material-ui/data-grid'; import { useData } from 'packages/storybook/src/hooks/useData'; import { Columns } from 'packages/grid/_modules_/grid/models/colDef/colDef'; @@ -120,8 +120,8 @@ describe(' - Keyboard', () => { , ); - focusCell(0, 0); - expect(getActiveCell()).to.equal('0-0'); + getCell(0, 0).focus(); + // eslint-disable-next-line material-ui/disallow-active-element-as-key-event-target fireEvent.keyDown(document.activeElement!, { key: 'ArrowRight' }); expect(handleKeyDown.returnValues).to.deep.equal([true]); }); @@ -141,7 +141,7 @@ describe(' - Keyboard', () => { /* eslint-disable material-ui/disallow-active-element-as-key-event-target */ it('cell navigation with arrows', () => { render(); - focusCell(0, 0); + getCell(0, 0).focus(); expect(getActiveCell()).to.equal('0-0'); fireEvent.keyDown(document.activeElement!, { key: 'ArrowRight' }); expect(getActiveCell()).to.equal('0-1'); @@ -155,7 +155,7 @@ describe(' - Keyboard', () => { it('Shift + Space should select a row', () => { render(); - focusCell(0, 0); + getCell(0, 0).focus(); expect(getActiveCell()).to.equal('0-0'); fireEvent.keyDown(document.activeElement!, SHIFT_SPACE_KEY); const row = getRow(0); @@ -165,7 +165,7 @@ describe(' - Keyboard', () => { it('Space only should go to the bottom of the page', async () => { render(); - focusCell(0, 0); + getCell(0, 0).focus(); expect(getActiveCell()).to.equal('0-0'); fireEvent.keyDown(document.activeElement!, SPACE_KEY); expect(getActiveCell()).to.equal('3-0'); @@ -173,7 +173,7 @@ describe(' - Keyboard', () => { it('Home / End navigation', async () => { render(); - focusCell(1, 1); + getCell(1, 1).focus(); expect(getActiveCell()).to.equal('1-1'); fireEvent.keyDown(document.activeElement!, { key: 'Home' }); expect(getActiveCell()).to.equal('1-0'); diff --git a/test/utils/helperFn.ts b/test/utils/helperFn.ts index cde34b279fc8a..6cf493acf5cc7 100644 --- a/test/utils/helperFn.ts +++ b/test/utils/helperFn.ts @@ -1,3 +1,5 @@ +import { expect } from "chai"; + export async function raf() { return new Promise((resolve) => { // Chrome and Safari have a bug where calling rAF once returns the current @@ -58,10 +60,7 @@ export function getCell(rowIndex: number, colIndex: number): HTMLElement { } return cell as HTMLElement; } -export function focusCell(rowIndex: number, colIndex: number) { - const cell = getCell(rowIndex, colIndex); - cell.focus(); -} + export function getRow(rowIndex: number): HTMLElement { const row = document.querySelector(`[role="row"][data-rowindex="${rowIndex}"]`); if (row == null) { From 81c0be9bed87987923fea5523dbba5c1902f3336 Mon Sep 17 00:00:00 2001 From: damien Date: Fri, 22 Jan 2021 18:43:26 +0100 Subject: [PATCH 4/8] prettier --- test/utils/helperFn.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/utils/helperFn.ts b/test/utils/helperFn.ts index 6cf493acf5cc7..4c6e2420b6cfe 100644 --- a/test/utils/helperFn.ts +++ b/test/utils/helperFn.ts @@ -1,4 +1,4 @@ -import { expect } from "chai"; +import { expect } from 'chai'; export async function raf() { return new Promise((resolve) => { From 78e4cc6daec77c878451482f04968bf445446b56 Mon Sep 17 00:00:00 2001 From: damien Date: Fri, 22 Jan 2021 18:58:25 +0100 Subject: [PATCH 5/8] clean --- test/utils/helperFn.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/utils/helperFn.ts b/test/utils/helperFn.ts index 4c6e2420b6cfe..ce3eeb5cd80f2 100644 --- a/test/utils/helperFn.ts +++ b/test/utils/helperFn.ts @@ -1,5 +1,3 @@ -import { expect } from 'chai'; - export async function raf() { return new Promise((resolve) => { // Chrome and Safari have a bug where calling rAF once returns the current From 2df05b8420434a0334bc9c27e2a05061a859aa04 Mon Sep 17 00:00:00 2001 From: Olivier Tassinari Date: Fri, 22 Jan 2021 19:43:58 +0100 Subject: [PATCH 6/8] dead async --- packages/grid/data-grid/src/tests/keyboard.DataGrid.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/grid/data-grid/src/tests/keyboard.DataGrid.test.tsx b/packages/grid/data-grid/src/tests/keyboard.DataGrid.test.tsx index a2fee98b8ad96..94fcba9174d6e 100644 --- a/packages/grid/data-grid/src/tests/keyboard.DataGrid.test.tsx +++ b/packages/grid/data-grid/src/tests/keyboard.DataGrid.test.tsx @@ -163,7 +163,7 @@ describe(' - Keyboard', () => { expect(isSelected).to.equal(true); }); - it('Space only should go to the bottom of the page', async () => { + it('Space only should go to the bottom of the page', () => { render(); getCell(0, 0).focus(); expect(getActiveCell()).to.equal('0-0'); From 9db6eeef8534ebb9e32a8c06bbc0085c16caa773 Mon Sep 17 00:00:00 2001 From: Olivier Tassinari Date: Fri, 22 Jan 2021 19:44:33 +0100 Subject: [PATCH 7/8] same space as the other utils --- test/utils/helperFn.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/test/utils/helperFn.ts b/test/utils/helperFn.ts index ce3eeb5cd80f2..d138b7e1c38c9 100644 --- a/test/utils/helperFn.ts +++ b/test/utils/helperFn.ts @@ -49,6 +49,7 @@ export function getColumnHeaders() { (node) => node!.textContent, ); } + export function getCell(rowIndex: number, colIndex: number): HTMLElement { const cell = document.querySelector( `[role="cell"][data-rowindex="${rowIndex}"][aria-colindex="${colIndex}"]`, From b22d7c0bc04e5c7c111c18f2af4e2fadd7cab90e Mon Sep 17 00:00:00 2001 From: Olivier Tassinari Date: Fri, 22 Jan 2021 19:53:36 +0100 Subject: [PATCH 8/8] fix eslint warning --- .../grid/data-grid/src/tests/keyboard.DataGrid.test.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/grid/data-grid/src/tests/keyboard.DataGrid.test.tsx b/packages/grid/data-grid/src/tests/keyboard.DataGrid.test.tsx index 94fcba9174d6e..ed838ad00700d 100644 --- a/packages/grid/data-grid/src/tests/keyboard.DataGrid.test.tsx +++ b/packages/grid/data-grid/src/tests/keyboard.DataGrid.test.tsx @@ -120,9 +120,9 @@ describe(' - Keyboard', () => { , ); - getCell(0, 0).focus(); - // eslint-disable-next-line material-ui/disallow-active-element-as-key-event-target - fireEvent.keyDown(document.activeElement!, { key: 'ArrowRight' }); + const firstCell = getCell(0, 0); + firstCell.focus(); + fireEvent.keyDown(firstCell, { key: 'ArrowRight' }); expect(handleKeyDown.returnValues).to.deep.equal([true]); });