[DataGrid] Edit Cell Navigation

[dtassone]

Provide the cell edit accessibility keys described in #1032

Preview: https://deploy-preview-1205--material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-rows--edit-rows-basic

[DanailH]

Can we add a description and the correct lables to the PR that explains the problem and what we want to achieve. It's very difficult to review PRs without a context of what the change aims to do.

[oliviertassinari]

It takes about 25-33 ms to switch from the view mode to the edit mode. Not instant, but fast. It's similar to the DX in airtable 👍

I would recommend these 4 changes of UX:

• When we start the edit mode on a cell and stop it with Escape, then we should set the focus on the cell. Otherwise, it breaks the keyboard navigation flow.
• When we are on view mode on a cell Delete should likely empty the field without starting the edit mode. I personally find it more intuitive without any real downsides.
• When we click somewhere else on the data grid, it should likely trigger a stop action.
• When we are on view mode on a cell and we press an alphabetical character, then trigger the start action, empty the textbox, and set the pressed character. You can see https://github.com/mui-org/material-ui/blob/5511e9c32fda435d7de7c64a44ba8c32483dab44/packages/material-ui-lab/src/TreeView/TreeView.js#L24-L26

[oliviertassinari]

Something else. We should likely have a preventDefault on double click when we know that we want to start editing. There is currently a blue flash, it doesn't feel clean:

[oliviertassinari]

It looks great, we are close. The behavior that feels wrong from a UX perspective:

1. Tab Only works once (it seems to)
2. Shift+Tab doesn't work (it move to the top left, not the previous cell)
3. The regression in https://www.argos-ci.com/mui-org/material-ui-x/builds/277 suggests that the trigger of the exit mode is too greedy.
4. No mobile support, it's probably for the best. A regular form sounds a lot better, I assume it would be messed up if we tried to do anything. It's what airtable do.

[DanailH]

Are we going to update the docs as part of this PR or a separate one?

[dtassone]

Are we going to update the docs as part of this PR or a separate one?

separate

[dtassone]

3. The regression in https://www.argos-ci.com/mui-org/material-ui-x/builds/277 suggests that the trigger of the exit mode is too greedy.

There is an onBlur on the cell that switches back to the view mode.
So if you click outside the cell then it commits the changes as requested.
if you edit 2 cells then onBlur on the first cell is triggered.

[oliviertassinari]

There is an onBlur on the cell that switches back to the view mode.

I can think of two options:

1. We add params to not focus the textbox or to not exit the edit mode. This retains the visual regression test as before. The objective is to see the visual state without the textbox focused.
2. We forget about the visual test coverage for the edit mode state without the focus. We remove the dead code in the snap. We wait for the row edit feature to add visual coverage again on it.

---

One last issue with the UX I have seen, we lost the focus on the cell when using Escape

https://deploy-preview-1205--material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-rows--edit-rows-basic

[dtassone]

There is an onBlur on the cell that switches back to the view mode.

I can think of two options:

1. We add params to not focus the textbox or to not exit the edit mode. This retains the visual regression test as before. The objective is to see the visual state without the textbox focused.
2. We forget about the visual test coverage for the edit mode state without the focus. We remove the dead code in the snap. We wait for the row edit feature to add visual coverage again on it.

Or 3 we just edit 1 cell in the test snap

[oliviertassinari]

Or 3 we just edit 1 cell in the test snap

Which is option 2, no? What's the difference?

[dtassone]

Or 3 we just edit 1 cell in the test snap

Which is option 2, no? What's the difference?

Hmm yes I thought you wanted to remove it completely

[oliviertassinari]

At each reader it will refocus the cell container? Sounds like it will create bugs. For instance, what if I have a focusable checkbox in my cell? Happy to delay the resolution to the next bug a developer will open as it should allow us to add a test case for this. Now if we double check this point now, it should actually be better.

[oliviertassinari]

It seems to work really well :)