[DataGrid] Store the outlined cell in the state #7111

m4theushw · 2022-12-07T00:03:08Z

Here I'm moving to the state the information of which cell has the outline. Previously, the outline style was added by CSS using :focus-within, but for cell selection a cell might focused but not have the outline. During normal usage, e.g. keyboard navigation, the outline is synced with the focus inside apiRef.current.setCellFocus. This means that if the focus is not correctly updated, the cell won't have the outline. A way to review this PR is to battle test the focus logic ensuring that the focus is always updated.

To implement the correct behavior for cell selection I'll open another PR allowing apiRef.current.setCellFocus to skip the outline update. Then we can add a apiRef.current.setOutlinedCell to update only the outline state.

Release

Breaking changes

To update the outline style of a focused cell, use the .MuiDataGrid-cell--outlined CSS class instead of the :focus-within selector.
```
-'.MuiDataGrid-cell:focus-within': {
+'.MuiDataGrid-cell--outlined': {
```

mui-bot · 2022-12-07T00:08:36Z

	Messages
📖	Netlify deploy preview: https://deploy-preview-7111--material-ui-x.netlify.app/

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	663.9	1,086.2	663.9	819.42	186.867
Sort 100k rows	ms	617.4	1,176.4	1,176.4	958.38	216.739
Select 100k rows	ms	227.5	329.3	290.3	279.32	35.852
Deselect 100k rows	ms	151.2	326.4	201.3	214.86	60.098

Generated by 🚫 dangerJS against c7ea049

m4theushw · 2022-12-07T23:17:23Z

packages/grid/x-data-grid/src/hooks/features/focus/useGridFocus.ts

@@ -224,7 +258,7 @@ export const useGridFocus = (
    [apiRef],
  );

-  const focussedColumnGroup = unstable_gridFocusColumnGroupHeaderSelector(apiRef);
+  const focusedColumnGroup = unstable_gridFocusColumnGroupHeaderSelector(apiRef);


Just a typo.

m4theushw · 2022-12-07T23:18:03Z

packages/grid/x-data-grid/src/hooks/features/focus/useGridFocus.ts

+  const handleCellBlur = useEventCallback(() => {
+    if (lastKeydownEvent.current?.key !== 'Tab') {
+      return;
+    }
+    apiRef.current.setState((state) => ({
+      ...state,
+      // The tabIndex is kept to allow the focus to return to this cell if Shift+Tab is pressed
+      focus: { cell: null, columnHeader: null, columnGroupHeader: null },
+      outline: { cell: null },
+    }));
+    apiRef.current.forceUpdate();
+  });
+
+  const handleCellFocus = useEventCallback((params: GridCellParams) => {
+    if (lastKeydownEvent.current?.key === 'Tab') {
+      apiRef.current.setCellFocus(params.id, params.field);
+    }
+  });


This is the fix for #3844

m4theushw · 2022-12-07T23:21:17Z

packages/grid/x-data-grid/src/components/cell/GridCell.tsx

-
-  if (
-    process.env.NODE_ENV === 'test' &&
-    rootProps.experimentalFeatures?.warnIfFocusStateIsNotSynced


This flag is useless now because when a cell is focused via keyboard, onFocus is called before we update the focus state. I don't consider a breaking change because it's already experimental and it didn't have a good reception: #3850

github-actions · 2022-12-13T10:46:31Z

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

DanailH

Nice work! I just have a couple of notes/questions but other than those it's good from my side

packages/grid/x-data-grid/src/components/containers/GridRootStyles.ts

DanailH · 2022-12-15T14:02:57Z

packages/grid/x-data-grid/src/hooks/features/focus/useGridFocus.ts

+    apiRef.current.setState((state) => ({
+      ...state,
+      // The tabIndex is kept to allow the focus to return to this cell if Shift+Tab is pressed
+      focus: { cell: null, columnHeader: null, columnGroupHeader: null },


It's more of a general question but do we also have a focus on a row? I'm asking because of the row selection and things like row grouping.

No, because rows don't have the outline.

packages/grid/x-data-grid/src/models/api/gridFocusApi.ts

cherniavskii · 2023-01-03T13:33:41Z

packages/grid/x-data-grid/src/components/containers/GridRootStyles.ts

@@ -124,7 +124,7 @@ export const GridRootStyles = styled('div', {
      padding: '0 10px',
      boxSizing: 'border-box',
    },
-    [`& .${gridClasses.columnHeader}:focus-within, & .${gridClasses.cell}:focus-within`]: {
+    [`& .${gridClasses.columnHeader}:focus-within, & .${gridClasses['cell--outlined']}`]: {


Should we ditch :focus-within in the column header as well?

I don't think we need to do this change now. Creating a .MuiDataGrid-columnHeader--outlined will create complexity that's not necessary for now. If some day we implement a range selection for column headers, then we'll need to do this.

packages/grid/x-data-grid/src/components/containers/GridRootStyles.ts

cherniavskii · 2023-01-03T14:45:50Z

docs/data/data-grid/events/events.json

@@ -6,6 +6,13 @@
    "params": "GridAggregationModel",
    "event": "MuiEvent<{}>"
  },
+  {
+    "projects": ["x-data-grid", "x-data-grid-pro", "x-data-grid-premium"],
+    "name": "cellBlur",


Do we still need cellFocusIn/cellFocusOut events?

Yes, for editing. cellBlur is tied to the blur DOM event, meaning that it's published every time a cell loses focus, including when the focus is lost to a portaled element inside the same cell. In the other hand, cellFocusOut is only published when focus state is updated. If a cell loses focus to an portaled element inside it, cellFocusOut is not published but cellBlur is.

cherniavskii · 2023-01-03T14:49:23Z

docs/data/migration/migration-data-grid-v5/migration-data-grid-v5.md

+- To update the outline style of a focused cell, use the `.MuiDataGrid-cell--outlined` class instead of the `:focus-within` selector.
+  ```diff
+  -'.MuiDataGrid-cell:focus-within': {
+  +'.MuiDataGrid-cell--outlined': {


Suggested change

+'.MuiDataGrid-cell--outlined': {

+'.MuiDataGrid-cell--outlined': {

- `.${gridClasses.cell}:focus-within`

+ `.${gridClasses['cell--outlined']}`

Maybe include an example with gridClasses as well?

packages/grid/x-data-grid/src/hooks/features/focus/gridFocusState.ts

packages/grid/x-data-grid/src/hooks/features/focus/gridFocusStateSelector.ts

cherniavskii · 2023-01-04T18:36:44Z

To implement the correct behavior for cell selection I'll open another PR allowing apiRef.current.setCellFocus to skip the outline update. Then we can add a apiRef.current.setOutlinedCell to update only the outline state.

I'm not sure we have to decouple focus/outline states to implement the correct behavior for cell selection.
Did you consider storing the range end cell in the state instead? This should be enough information how to adjust the range after initial selection

m4theushw · 2023-01-04T19:37:08Z

Did you consider storing the range end cell in the state instead? This should be enough information how to adjust the range after initial selection

@cherniavskii I'm not sure I understood you. The problem is not to know which cell was the last selected, but to put the outline in another cell. By default, the outlined cell is the last one clicked. In a range selection created by clicking two cells, the outlined cell is the last one, however, I need to keep the outline in the first clicked cell.

cherniavskii · 2023-01-04T21:49:45Z

In a range selection created by clicking two cells, the outlined cell is the last one, however, I need to keep the outline in the first clicked cell.

Sorry, I thought you were solving a different issue.
Could you provide more details on the issue you're solving? I don't understand why the outline has to stay in the first-clicked cell.

m4theushw · 2023-01-06T12:36:01Z

Could you provide more details on the issue you're solving? I don't understand why the outline has to stay in the first-clicked cell.

@cherniavskii The outline has to stay in the first-clicked cell only in cell selection. It's how most spreadsheet apps work: #6567 (comment) For everything else nothing has changed, the outlined cell is always the last clicked cell.

cherniavskii · 2023-01-06T15:09:26Z

I believe the focus and outline states are always synced in Spreadsheet.
Consider this example:

Screen.Recording.2023-01-06.at.16.04.01.mov

After Shift+Click the first cell is still outlined and focused. You can check that by navigating using the arrow keys - the focus will move from the cell that was outlined, not the one that was clicked with Shift.

This reverts commit 0203346.

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

[DataGrid] Store in the state the cell outlined

4babd8b

m4theushw force-pushed the cell-outline branch from 08a0fcb to 4babd8b Compare December 7, 2022 00:15

m4theushw commented Dec 7, 2022

View reviewed changes

m4theushw added the breaking change label Dec 7, 2022

Update migration guide

3199aea

m4theushw requested review from cherniavskii, DanailH and MBilalShafi December 7, 2022 23:41

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

DanailH changed the title ~~[DataGrid] Store in the state the cell outlined~~ [DataGrid] Store the outlined cell in the state Dec 15, 2022

DanailH reviewed Dec 15, 2022

View reviewed changes

m4theushw added 3 commits January 2, 2023 19:31

Merge branch 'next' into cell-outline

55686c7

Decrease specificity

7623bd9

Fix return type

00bffba

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

m4theushw added 3 commits January 2, 2023 21:23

Fix return tag

70cd4d4

Fix conflicts

9b36e02

yarn prettier

ee400ee

cherniavskii reviewed Jan 3, 2023

View reviewed changes

m4theushw added 3 commits January 4, 2023 09:37

Fix feature showcase demo

7535fc0

Add example with gridClasses

297b7ca

Don't document new selectors

c7ea049

m4theushw requested review from DanailH and cherniavskii January 4, 2023 13:32

DanailH approved these changes Jan 5, 2023

View reviewed changes

m4theushw merged commit 0203346 into mui:next Jan 6, 2023

m4theushw deleted the cell-outline branch January 6, 2023 14:41

m4theushw mentioned this pull request Jan 11, 2023

[discussion] Preparing v6 #3287

Closed

41 tasks

m4theushw added a commit to m4theushw/mui-x that referenced this pull request Jan 11, 2023

Revert "[DataGrid] Store the outlined cell in the state (mui#7111)"

96225fd

This reverts commit 0203346.

m4theushw mentioned this pull request Jan 11, 2023

Revert "[DataGrid] Store the outlined cell in the state (#7111)" #7481

Merged

m4theushw added a commit that referenced this pull request Jan 12, 2023

Revert "[DataGrid] Store the outlined cell in the state (#7111)" (#7481)

fbf8102

MBilalShafi mentioned this pull request Jan 16, 2023

[DataGrid] Remove GridCellIdentifier redundant type #7578

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataGrid] Store the outlined cell in the state #7111

[DataGrid] Store the outlined cell in the state #7111

m4theushw commented Dec 7, 2022 •

edited

Loading

mui-bot commented Dec 7, 2022 •

edited

Loading

m4theushw Dec 7, 2022

m4theushw Dec 7, 2022

m4theushw Dec 7, 2022

github-actions bot commented Dec 13, 2022

DanailH left a comment

DanailH Dec 15, 2022

m4theushw Jan 2, 2023

cherniavskii Jan 3, 2023

m4theushw Jan 4, 2023

cherniavskii Jan 3, 2023

m4theushw Jan 4, 2023

cherniavskii Jan 3, 2023

m4theushw Jan 4, 2023

cherniavskii commented Jan 4, 2023

m4theushw commented Jan 4, 2023

cherniavskii commented Jan 4, 2023

m4theushw commented Jan 6, 2023

cherniavskii commented Jan 6, 2023

[DataGrid] Store the outlined cell in the state #7111

[DataGrid] Store the outlined cell in the state #7111

Conversation

m4theushw commented Dec 7, 2022 • edited Loading

Release

Breaking changes

mui-bot commented Dec 7, 2022 • edited Loading

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 13, 2022

DanailH 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

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

cherniavskii commented Jan 4, 2023

m4theushw commented Jan 4, 2023

cherniavskii commented Jan 4, 2023

m4theushw commented Jan 6, 2023

cherniavskii commented Jan 6, 2023

m4theushw commented Dec 7, 2022 •

edited

Loading

mui-bot commented Dec 7, 2022 •

edited

Loading