[DataGrid] Refactor keyboard/click event management #3275

alexfauquette · 2021-11-24T13:17:18Z

This is a first step to handle all the keyboard events in their feature hook #3265 (comment)

alexfauquette · 2021-11-26T14:08:52Z

The handleCellKeyDown can not be completely merged with handleCellNavigationKeyDown because of the early return

if ((event.target as any).nodeType === 1 && !isGridCellRoot(event.target as Element)) {
        return;
}

this condition prevents from applying keyboard shortcuts if the focus is not on a cell component (for example a select input)

To apply keyboard events inside cells (for example when the focus is on a button) the event cellNavigationKeyDown is dispatched. This one triggers handleCellNavigationKeyDown which does not check if the event.target is a Cell component.

So the solution still needs improvement, but at least it solves the two issues

github-actions · 2021-11-27T12:21:06Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

flaviendelangle

To avoid breaking changes, we should still fire columnHeaderNavigationKeydown even if we do not use it internally.

flaviendelangle · 2021-11-29T09:27:03Z

packages/grid/_modules_/grid/components/columnSelection/GridHeaderCheckbox.tsx

@@ -103,7 +103,7 @@ const GridHeaderCheckbox = React.forwardRef<HTMLInputElement, GridColumnHeaderPa
          event.stopPropagation();
        }
        if (isNavigationKey(event.key) && !event.shiftKey) {


Shouldn't we fire the columnHeaderKeyDown on any key, not just navigation one ?

Should we remove this handler and let columnHeaderKeyDown to propagate freely?

I did not noticed, but we are already firing columnHeaderKeyDown on any key except Space (because of the stop event propagation)

The stop eventPropagation is here to avoid a preventDefault which would disable the validation of the input with Space key.

I'm putting back the columnHeaderNavigationKeyDown and add a TODO such as it should be remove in v6

For the stopPropagation, I see two options:

keep the stopPropagation and fire columnHeaderKeyDown

remove the stopPropagation, and call the handleChange the the key pressed is a Space

m4theushw

This is a draft PR to explain the proposed solution in #3265 (comment)

Is it still in draft? I don't know how rigorous should I review it.

packages/grid/_modules_/grid/hooks/features/selection/useGridSelection.ts

m4theushw · 2021-11-30T01:03:21Z

packages/grid/_modules_/grid/utils/domUtils.ts

@@ -24,6 +24,10 @@ export function isGridHeaderCellRoot(elem: Element | null): boolean {
  return elem != null && elem.classList.contains(gridClasses.columnHeader);
 }

+export function isGridHeaderSelectAllCheckBox(elem: Element | null): boolean {
+  return elem != null && elem.ariaLabel === 'Select All Rows checkbox';


It could have been translated.

m4theushw · 2021-11-30T01:03:58Z

packages/grid/_modules_/grid/hooks/features/keyboard/useGridKeyboardNavigation.ts

    GridEventListener<GridEvents.columnHeaderNavigationKeyDown>
  >(
    (params, event) => {
      event.preventDefault();
+      if (
+        !isGridHeaderCellRoot(event.target as HTMLElement) &&
+        !isGridHeaderSelectAllCheckBox(event.target as HTMLElement)


Checking params.field is safer.

m4theushw · 2021-11-30T01:07:40Z

packages/grid/_modules_/grid/components/columnSelection/GridHeaderCheckbox.tsx

@@ -103,7 +103,7 @@ const GridHeaderCheckbox = React.forwardRef<HTMLInputElement, GridColumnHeaderPa
          event.stopPropagation();
        }
        if (isNavigationKey(event.key) && !event.shiftKey) {


Should we remove this handler and let columnHeaderKeyDown to propagate freely?

github-actions · 2021-12-01T16:11:56Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

m4theushw

It would be nice to add some test cases to cover the fixed issues.

packages/grid/_modules_/grid/hooks/features/keyboard/useGridKeyboard.ts

packages/grid/_modules_/grid/hooks/features/selection/useGridSelection.ts

m4theushw

We miss a test for #3275 (comment) in selection.DataGridPro.test.tsx.

m4theushw · 2021-12-07T18:03:09Z

packages/grid/x-data-grid/src/tests/selection.DataGrid.test.tsx

+    it('should select one row at a time on Shift + Space', () => {
+      render(<TestDataGridSelection />);
+      getCell(0, 0).focus();
+      fireEvent.keyDown(document.activeElement || document.body, { key: ' ', shiftKey: true });


Why document.activeElement || document.body? Was it to snooze the ESLint rule "Don't use document.activeElement as a target for keyboard events."? 😁 You can fix it properly by passing the cell element as bellow:

https://github.com/mui-org/material-ui-x/blob/1d82bea672afc1ece26739a47b2de7ae298de9bc/packages/grid/x-data-grid-pro/src/tests/clipboard.DataGridPro.test.tsx#L91-L93

Not really, it is to follow the documentation :)

This will also test that the element in question can even receive keyboard events

https://testing-library.com/docs/guide-events#keydown

The element is passed to improve readability: mui/material-ui#20945

This will also test that the element in question can even receive keyboard events

This check is embedded into the custom fireEvent we use. See https://github.com/mui-org/material-ui/blob/69b7e4d911e8ff99d17a74330496ae69e7cc0ade/test/utils/createRenderer.tsx#L622

I've done the modifications, but I'm not really convinced that it improves readability

The core team has element which are all meaning full, leading to testing code such as

fireEvent.keyDown(button, ...); fireEvent.keyDown(firstTab, ...);

We are navigating between cells so most of the components with focus are cells which only differ by there coordinates which is translated by

fireEvent.keyDown(cell01, ...); fireEvent.keyDown(cell23, ...);

Take as example the following test case:

expect(getActiveCell()).to.equal('8-1'); fireEvent.keyDown(document.activeElement!, { key: 'ArrowDown' }); fireEvent.keyDown(document.activeElement!, { key: 'ArrowRight' }); fireEvent.keyDown(document.activeElement!, { key: 'ArrowLeft' }); fireEvent.keyDown(document.activeElement!, { key: 'ArrowUp' });

Can you easily spot where the focus is after the last keyDown? 😛

packages/grid/x-data-grid/src/tests/selection.DataGrid.test.tsx

alexfauquette · 2021-12-08T09:00:39Z

@m4theushw I added a commit to prevent keyboard "Shift+Space" from starting editing the cell, which is a pain when selecting rows in an editable DataGrid

m4theushw

Good job.

alexfauquette · 2022-01-27T11:52:09Z

packages/grid/_modules_/grid/hooks/features/keyboard/useGridKeyboardNavigation.ts

-        !isGridHeaderCellRoot(event.target as HTMLElement) &&
-        !isGridHeaderSelectAllCheckBox(event.target as HTMLElement)
-      ) {
+      if (!params.field) {
        return;
      }
      const dimensions = apiRef.current.getRootDimensions();


This modification

alexfauquette changed the title ~~Demo~~ [DataGrid] refacto Keyboeard/Click event management Nov 24, 2021

alexfauquette changed the title ~~[DataGrid] refacto Keyboeard/Click event management~~ [DataGrid] Refacto Keyboeard/Click event management Nov 24, 2021

alexfauquette force-pushed the user-interaction branch from 1ad3e8d to 53642ca Compare November 24, 2021 15:00

alexfauquette mentioned this pull request Nov 24, 2021

[DataGrid] Pressing Shift + space should select new row without having to unselect the other row first #3265

Closed

2 tasks

alexfauquette self-assigned this Nov 24, 2021

alexfauquette force-pushed the user-interaction branch from ab551e7 to b69ff2a Compare November 26, 2021 12:23

alexfauquette marked this pull request as ready for review November 26, 2021 13:59

alexfauquette requested review from flaviendelangle, DanailH and m4theushw November 26, 2021 13:59

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 27, 2021

alexfauquette added 6 commits November 29, 2021 09:17

remove useless case

f741503

move click selection in a distinct function

1d6698f

move keyboard selecting logic to the selection hook

17acb42

set back the early return

7ccd579

prettier

785a8e5

remove columnHeaderKeyDown

5de9f76

alexfauquette force-pushed the user-interaction branch from b69ff2a to 5de9f76 Compare November 29, 2021 08:37

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 29, 2021

correct the rebase

26232df

flaviendelangle reviewed Nov 29, 2021

View reviewed changes

m4theushw reviewed Nov 30, 2021

View reviewed changes

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 1, 2021

alexfauquette added 3 commits December 2, 2021 10:50

Merge branch 'master' into user-interaction

4b81e4b

remove isSpaceKey

c8266a0

PR feedbacks

dc803cd

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 2, 2021

alexfauquette requested review from flaviendelangle and m4theushw December 3, 2021 09:36

m4theushw reviewed Dec 4, 2021

View reviewed changes

alexfauquette added 5 commits December 6, 2021 10:13

Merge remote-tracking branch 'upstream/master' into user-interaction

ef4e6d4

PR feedbacks

80f5c9e

add tests

4f5903f

fix tests

1f48a63

add mui#3220 modifications

30fd7e7

alexfauquette requested a review from m4theushw December 6, 2021 16:06

m4theushw reviewed Dec 7, 2021

View reviewed changes

alexfauquette added 2 commits December 8, 2021 09:09

keep current behavior

c3fbe43

prevent shift+space from starting editing

e67871f

add tests

26d254c

alexfauquette requested a review from m4theushw December 8, 2021 17:19

remove document.activeElement from tests

ec0b9db

m4theushw changed the title ~~[DataGrid] Refacto Keyboeard/Click event management~~ [DataGrid] Refactor keyboard/click event management Dec 10, 2021

m4theushw added the component: data grid This is the name of the generic UI component, not the React module! label Dec 10, 2021

m4theushw approved these changes Dec 14, 2021

View reviewed changes

alexfauquette merged commit 76ffe73 into mui:master Dec 14, 2021

alexfauquette deleted the user-interaction branch December 14, 2021 08:54

igor-serzhan mentioned this pull request Jan 13, 2022

[DataGridPro] can't use TextField in header #3599

Closed

2 tasks

alexfauquette mentioned this pull request Jan 18, 2022

[DataGrid] Fix input element in custom header #3624

Merged

alexfauquette commented Jan 27, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataGrid] Refactor keyboard/click event management #3275

[DataGrid] Refactor keyboard/click event management #3275

alexfauquette commented Nov 24, 2021 •

edited

alexfauquette commented Nov 26, 2021

github-actions bot commented Nov 27, 2021

flaviendelangle left a comment

flaviendelangle Nov 29, 2021

m4theushw Nov 30, 2021

alexfauquette Dec 2, 2021

m4theushw left a comment

m4theushw Nov 30, 2021

m4theushw Nov 30, 2021

m4theushw Nov 30, 2021

github-actions bot commented Dec 1, 2021

m4theushw left a comment •

edited

m4theushw left a comment

m4theushw Dec 7, 2021 •

edited

alexfauquette Dec 8, 2021

m4theushw Dec 8, 2021

alexfauquette Dec 9, 2021

m4theushw Dec 14, 2021

alexfauquette commented Dec 8, 2021

m4theushw left a comment

alexfauquette Jan 27, 2022

[DataGrid] Refactor keyboard/click event management #3275

[DataGrid] Refactor keyboard/click event management #3275

Conversation

alexfauquette commented Nov 24, 2021 • edited

alexfauquette commented Nov 26, 2021

github-actions bot commented Nov 27, 2021

flaviendelangle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4theushw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 1, 2021

m4theushw left a comment • edited

Choose a reason for hiding this comment

m4theushw left a comment

Choose a reason for hiding this comment

m4theushw Dec 7, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexfauquette commented Dec 8, 2021

m4theushw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexfauquette commented Nov 24, 2021 •

edited

m4theushw left a comment •

edited

m4theushw Dec 7, 2021 •

edited