[DataGrid] Improve selection with keyboard #4157

flaviendelangle · 2022-03-11T09:32:35Z

When reducing a selection range, keep the focus key selected
Drop useGridKeyboard (movie the publication of cellNavigationKeydown in useGridKeyboardNavigation and the content of expandCellSelection in useGridSelection)

mui-bot · 2022-03-11T09:37:55Z

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	264.9	585.6	479.7	428.68	124.531
Sort 100k rows	ms	444.3	999.9	911.4	821.8	205.618
Select 100k rows	ms	118.4	283.8	189.2	186.3	57.85
Deselect 100k rows	ms	104.4	345.2	141.5	175	86.854

Generated by 🚫 dangerJS against 8f0c73e

packages/grid/x-data-grid/src/hooks/features/selection/useGridSelection.ts

flaviendelangle · 2022-03-11T12:53:17Z

packages/grid/x-data-grid/src/DataGrid/useDataGridComponent.tsx

@@ -64,6 +63,7 @@ export const useDataGridComponent = (props: DataGridProcessedProps) => {
  useGridInitializeState(rowsMetaStateInitializer, apiRef, props);
  useGridInitializeState(columnMenuStateInitializer, apiRef, props);

+  useGridKeyboardNavigation(apiRef, props);


useGridSelection must consume the cellKeydown after the navigation have been applied.

alexfauquette

From the preview, it seems you did not fix the issue #4156

In expandRowRangeSelection, there is

if (startIndex > endIndex) {
endId = visibleRowIds[endIndex + 1];
} else {
endId = visibleRowIds[endIndex - 1];
}

Which forget the case when user click on the same cell

else if (startIndex === endIndex) {
	endId = visibleRowIds[endIndex];
}

packages/grid/x-data-grid/src/hooks/features/selection/useGridSelection.ts

flaviendelangle · 2022-03-11T14:09:28Z

I mis-read the issue and fixed another bug 😆
My bug was with the keyboard and the issue is talking about the mouse.
I will also fix it then.

alexfauquette

The bug could be tested, but I'm not sure it's interesting to add it

m4theushw

I would be very careful when changing how the selection works. I would prefer to have a PR only fixing #4156. With #4157 I can't expand the selection holding Shift while pressing ArrowDown.

This happens because we do nothing if the target is not the cell root. There's a TODO to improve this check but I don't know the side effects doing it now.

mui-x/packages/grid/x-data-grid/src/hooks/features/selection/useGridSelection.ts

Line 387 in 50f084d

if (!isGridCellRoot(event.target as Element)) {

packages/grid/x-data-grid/src/hooks/features/selection/useGridSelection.ts

m4theushw · 2022-03-11T15:35:56Z

packages/grid/x-data-grid/src/hooks/features/selection/useGridSelection.ts

@@ -108,6 +112,9 @@ export const useGridSelection = (
        const visibleRowIds = gridVisibleSortedRowIdsSelector(apiRef);
        const startIndex = visibleRowIds.findIndex((rowId) => rowId === startId);
        const endIndex = visibleRowIds.findIndex((rowId) => rowId === endId);


Could apiRef.current.getRowIndexRelativeToVisibleRows be used here?

I don't think so because when expanding with the mouse, we support cross-page expansion.

You click on a row of page 1
You go to page 2
You shift click a row of page 2

=> It selects all the row between your two clicks.

packages/grid/x-data-grid/src/hooks/features/selection/useGridSelection.ts

flaviendelangle · 2022-03-14T12:52:54Z

I would be very careful when changing how the selection works. I would prefer to have a PR only fixing #4156.

I will extract the fix for #4156 in a standalone PR 👍

This happens because we do nothing if the target is not the cell root. There's a TODO to improve this check but I don't know the side effects doing it now.

@m4theushw ok so before this PR, the isGridCellRoot check was done to select to row by pressing Enter + Shift or to select all rows by pressing Ctrl + A
But it was not applied to expand the row selection by pressing Shift + Arrow Up / Down.

Do we need the check at all for selection ? Was this check for some scenarios but not others the right behavior or just an error ?

m4theushw · 2022-03-14T14:00:26Z

Do we need the check at all for selection ? Was this check for some scenarios but not others the right behavior or just an error ?

The check was added to fix #701. It might be useless now. But if keep it, then it's better to extend to check if the keydown came from inside the cell, not only the root.

github-actions · 2022-03-17T13:15:27Z

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

packages/grid/x-data-grid/src/hooks/features/keyboardNavigation/useGridKeyboardNavigation.ts

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

packages/grid/x-data-grid/src/hooks/features/selection/useGridSelection.ts

m4theushw · 2022-03-21T17:24:06Z

packages/grid/x-data-grid/src/hooks/features/keyboardNavigation/useGridKeyboardNavigation.ts

+      // Get the most recent params because the cell mode may have changed by another listener
+      const cellParams = apiRef.current.getCellParams(params.id, params.field);
+
+      if (cellParams.cellMode !== GridCellModes.Edit && isNavigationKey(event.key)) {


Space is not a navigation key if it is pressed when the focus is on the checkbox column. It jumps if a click only on the border of the checkbox. By doing an additional check here, you can remove the event.preventDefault. Also, the modification in the order that the hooks are called can be reverted.

m4theushw · 2022-03-21T17:34:52Z

packages/grid/x-data-grid/src/hooks/features/selection/useGridSelection.ts

+        if (focusCell && focusCell.id !== params.id) {
+          event.preventDefault();
+
+          const isPreviousRowSelected = apiRef.current.isRowSelected(focusCell.id);


We're relying on the fact that the focus was not updated yet. If the order of the listeners is changed, or we change to listen for the event in the capture phase, it won't work. Would it work if lastRowToggled is used here instead?

We're relying on the fact that the focus was not updated yet.

The opposite, I missed-named this variable, it should be isNewRowSelected
I access the previous row through params.id and the new one through focusCell.id

I don't think we could easily cover all use cases
If you navigate with the keyboard without selecting rows, your "previousRow" still needs to be the cell the focus is currently in, not the last row toggled.
I agree that this is a constraint on the call order and that is why I am putting useGridKeyboardNavigation before useGridSelection.

Sounds fair. I only don't like the constraint on the call order, but I don't have any other simple solution. We could listen to cellFocusIn and try to track if the new focus came from the keyboard navigation. It's a way to not depend on the call order but much more complex. We can leave it as it is.

Alternatively, the keyboard navigation could publish an event keyboardNavigation with the cell before and after.

packages/grid/x-data-grid/src/hooks/features/selection/useGridSelection.ts

m4theushw · 2022-03-21T17:47:28Z

packages/grid/x-data-grid/src/hooks/features/selection/useGridSelection.ts

+              } else {
+                // We are navigating to the top of the page and adding selected rows
+                start = newRowIndex;
+                end = previousRowIndex - 1;


There's a bug here. If I press ArrowUp already with Shift pressed, it ignores the first row.

m4theushw · 2022-03-21T17:52:04Z

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

@@ -346,48 +346,48 @@ describe('<DataGrid /> - Selection', () => {
    });
  });

+  /* eslint-disable material-ui/disallow-active-element-as-key-event-target */


Could you update the tests so we don't need to ignore this ESLint rule? To keep consistent with the core.

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

packages/grid/x-data-grid/src/hooks/features/selection/useGridSelection.ts

m4theushw

Ignore.

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

Co-authored-by: Matheus Wichman <matheushw@outlook.com>

[DataGrid] Improve selection with keyboard

c72652f

flaviendelangle self-assigned this Mar 11, 2022

flaviendelangle added 6 commits March 11, 2022 11:04

Work

5d6ea78

Work

0243a26

Work

d97b70e

Work

1e0da31

Work

0c5773c

Work

520511e

flaviendelangle commented Mar 11, 2022

View reviewed changes

flaviendelangle marked this pull request as ready for review March 11, 2022 12:57

flaviendelangle requested review from m4theushw and alexfauquette and removed request for m4theushw March 11, 2022 12:57

alexfauquette reviewed Mar 11, 2022

View reviewed changes

packages/grid/x-data-grid/src/hooks/features/selection/useGridSelection.ts Outdated Show resolved Hide resolved

Work

911e2bd

flaviendelangle requested a review from alexfauquette March 11, 2022 14:18

alexfauquette approved these changes Mar 11, 2022

View reviewed changes

m4theushw reviewed Mar 11, 2022

View reviewed changes

flaviendelangle requested a review from m4theushw March 14, 2022 13:00

flaviendelangle added 5 commits March 15, 2022 10:20

Fix

d86ddcf

Work

b665400

Merge branch 'master' into selection-keyboard

91fb036

Merge branch 'master' into selection-keyboard

d35bc99

Work

8f2dc11

flaviendelangle requested a review from alexfauquette March 16, 2022 09:59

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

Merge

bc982a4

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

flaviendelangle added 2 commits March 17, 2022 14:34

Work

b6c27ad

Merge branch 'master' into selection-keyboard

e84a1df

alexfauquette reviewed Mar 18, 2022

View reviewed changes

packages/grid/x-data-grid/src/hooks/features/keyboardNavigation/useGridKeyboardNavigation.ts Outdated Show resolved Hide resolved

Work

48d383a

m4theushw reviewed Mar 21, 2022

View reviewed changes

Merge branch 'master' into selection-keyboard

11814b9

flaviendelangle requested a review from m4theushw March 23, 2022 09:03

Code review: Matheus

aab0102

m4theushw reviewed Mar 24, 2022

View reviewed changes

packages/grid/x-data-grid/src/tests/selection.DataGrid.test.tsx Outdated Show resolved Hide resolved

m4theushw reviewed Mar 24, 2022

View reviewed changes

packages/grid/x-data-grid/src/hooks/features/selection/useGridSelection.ts Outdated Show resolved Hide resolved

m4theushw reviewed Mar 24, 2022

View reviewed changes

packages/grid/x-data-grid/src/hooks/features/selection/useGridSelection.ts Show resolved Hide resolved

flaviendelangle added 4 commits March 25, 2022 08:49

Work

836c759

Merge branch 'master' into selection-keyboard

77f1a11

Fix lint

e7f2fbf

Code review: Matheus

76d4344

flaviendelangle requested a review from m4theushw March 29, 2022 11:02

flaviendelangle added component: data grid This is the name of the generic UI component, not the React module! feature: Selection Related to the data grid Selection feature labels Mar 29, 2022

m4theushw reviewed Mar 29, 2022

View reviewed changes

flaviendelangle requested a review from m4theushw March 29, 2022 13:48

m4theushw approved these changes Mar 29, 2022

View reviewed changes

packages/grid/x-data-grid/src/tests/selection.DataGrid.test.tsx Outdated Show resolved Hide resolved

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

8f0c73e

Co-authored-by: Matheus Wichman <matheushw@outlook.com>

flaviendelangle merged commit dcaeab3 into mui:master Mar 29, 2022

flaviendelangle deleted the selection-keyboard branch March 29, 2022 15:00

alexfauquette pushed a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022

[DataGrid] Improve selection with keyboard (mui#4157)

2b7378f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataGrid] Improve selection with keyboard #4157

[DataGrid] Improve selection with keyboard #4157

flaviendelangle commented Mar 11, 2022 •

edited

Loading

mui-bot commented Mar 11, 2022 •

edited

Loading

flaviendelangle Mar 11, 2022

alexfauquette left a comment •

edited

Loading

flaviendelangle commented Mar 11, 2022 •

edited

Loading

alexfauquette left a comment

m4theushw left a comment •

edited

Loading

m4theushw Mar 11, 2022

flaviendelangle Mar 14, 2022

flaviendelangle commented Mar 14, 2022

m4theushw commented Mar 14, 2022

github-actions bot commented Mar 17, 2022

m4theushw Mar 21, 2022 •

edited

Loading

m4theushw Mar 21, 2022

flaviendelangle Mar 23, 2022

m4theushw Mar 24, 2022

flaviendelangle Mar 25, 2022

m4theushw Mar 21, 2022 •

edited

Loading

flaviendelangle Mar 23, 2022

m4theushw Mar 21, 2022

flaviendelangle Mar 29, 2022

m4theushw left a comment •

edited

Loading

[DataGrid] Improve selection with keyboard #4157

[DataGrid] Improve selection with keyboard #4157

Conversation

flaviendelangle commented Mar 11, 2022 • edited Loading

mui-bot commented Mar 11, 2022 • edited Loading

Choose a reason for hiding this comment

alexfauquette left a comment • edited Loading

Choose a reason for hiding this comment

flaviendelangle commented Mar 11, 2022 • edited Loading

alexfauquette left a comment

Choose a reason for hiding this comment

m4theushw left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaviendelangle commented Mar 14, 2022

m4theushw commented Mar 14, 2022

github-actions bot commented Mar 17, 2022

m4theushw Mar 21, 2022 • edited Loading

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

m4theushw Mar 21, 2022 • edited Loading

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 • edited Loading

Choose a reason for hiding this comment

flaviendelangle commented Mar 11, 2022 •

edited

Loading

mui-bot commented Mar 11, 2022 •

edited

Loading

alexfauquette left a comment •

edited

Loading

flaviendelangle commented Mar 11, 2022 •

edited

Loading

m4theushw left a comment •

edited

Loading

m4theushw Mar 21, 2022 •

edited

Loading

m4theushw Mar 21, 2022 •

edited

Loading

m4theushw left a comment •

edited

Loading