[DataGrid] Drop autoFocus

[m4theushw]

We've been using autoFocus to set the focus when a cell enters the edit mode. The problem with this approach is that in #2098 multiple cells will be in edit mode at the same time, so using this prop on multiple elements is wrong. This PR modifies the current behavior to use the state to control which cell should receive focus.

Preview: https://deploy-preview-2239--material-ui-x.netlify.app/components/data-grid/demo/

[oliviertassinari]

Note that iOS is supposed to not support the autoFocus attribute: https://caniuse.com/?search=autofocus.

We have a regression on the keyboard navigation with the row checkbox selection feature. The last version that seems to behave correctly is v4.0.0-alpha.30: https://codesandbox.io/s/material-demo-forked-v9cqw. it's the input that is supposed to be focused when on the checkbox selection column, the arrow keys are supposed to work too. The correct behavior was introduced in #1421, but without any tests. I think an example of what we shouldn't do: bundle 5 fixes, add no tests.

[flaviendelangle]

Should we document the focus in this custom edit component demo ?

[m4theushw]

Should we document the focus in this custom edit component demo ?

@flaviendelangle I prefer to keep it as it is. But in the row editing yes will we have to explain better how to create custom edit components.

[m4theushw]

Note that iOS is supposed to not support the autoFocus attribute: https://caniuse.com/?search=autofocus.

We have a regression on the keyboard navigation with the row checkbox selection feature. The last version that seems to behave correctly is v4.0.0-alpha.30: https://codesandbox.io/s/material-demo-forked-v9cqw. it's the input that is supposed to be focused when on the checkbox selection column, the arrow keys are supposed to work too. The correct behavior was introduced in #1421, but without any tests. I think an example of what we shouldn't do: bundle 5 fixes, add no tests.

@oliviertassinari I fixed this in 20ef450. I had the same problem with the checkbox not checking, but in the row editing. I didn't think we would have this problem here, however we had. The solution is a bit hacky.

[m4theushw]

@mui-org/x Could someone review this one? It would be nice to check if there're no regressions. It's the only thing blocking #2098, which is already done.

[oliviertassinari]

Nothing blocking, only me explore any possible opportunities.

[oliviertassinari]

We rarely use event listeners in the capture phase in the core components. I'm always suspicious when we use them. So, I tried without, I couldn't help myself. The CI is green, the demos seems to work. What am I missing?

diff --git a/packages/grid/_modules_/grid/hooks/features/focus/useGridFocus.ts b/packages/grid/_modules_/grid/hooks/features/focus/useGridFocus.ts
index 22191bae..dbec238c 100644
--- a/packages/grid/_modules_/grid/hooks/features/focus/useGridFocus.ts
+++ b/packages/grid/_modules_/grid/hooks/features/focus/useGridFocus.ts
@@ -190,17 +190,9 @@ export const useGridFocus = (apiRef: GridApiRef, props: Pick<GridComponentProps,

   React.useEffect(() => {
     const doc = ownerDocument(apiRef.current.rootElementRef!.current as HTMLElement);
-    const handleDocumentClickAsync = (event) => {
-      // If the state is changed during the capture phase, onChange will not be fired
-      // By delaying the state update, there is time for other callbacks to be called first
-      // See https://github.com/facebook/react/issues/13424
-      clickTimeout.current = setTimeout(() => handleDocumentClick(event));
-    };
-    doc.addEventListener('click', handleDocumentClickAsync, true);
-
+    doc.addEventListener('click', handleDocumentClick);
     return () => {
-      clearTimeout(clickTimeout.current);
-      doc.removeEventListener('click', handleDocumentClickAsync, true);
+      doc.removeEventListener('click', handleDocumentClick);
     };
   }, [apiRef, handleDocumentClick]);

---

Off-topic. I personally find the current approach a bit backward. We set the focus state, then the state calls .focus(). Maybe the opposite might be easier to work with to me? We call .focus() where need. The grid listens to the focus/blur event to update its state if needed. The tricky part might be around virtualization and headers, I don't know.

[m4theushw]

We rarely use event listeners in the capture phase in the core components. I'm always suspicious when we use them. So, I tried without, I couldn't help myself. The CI is green, the demos seems to work. What am I missing?

True, we don't need to add the listener in the capture phase now. Probably I did this because the code from this PR is actually from #2098, which at the time it was opened #2179 was not merged yet, so stopping the propagation in a onCellClick would also break the focus.

Off-topic. I personally find the current approach a bit backward. We set the focus state, then the state calls .focus().

The tricky part for me is the Portal support. At the beginning we were doing this approach but we inverted.

[oliviertassinari]

How can a cell be in edit mode while not editable?

[m4theushw]

Updated. Answering your question, if apiRef.current.setCellMode is called in a cell that cannot be edited it will turn to edit mode. We need to add a check there.

[oliviertassinari]

Looks great