[DataGrid] Fix "stop editing" integration with IME e.g. Japanese

[Gumichocopengin8]

https://deploy-preview-5257--material-ui-x.netlify.app/x/react-data-grid/editing/#making-a-column-editable

Close #2338

I used event.which to fix this issue because it seems like it is the best way for now. However, it is deprecated. In the future, need to find alternatives.

[oliviertassinari]

@Gumichocopengin8 Thanks for working on this problem. First, we would need to add a new test case. The test cases in mui/material-ui#23050 and mui/material-ui#19499 could help.

[Gumichocopengin8]

@oliviertassinari which files should I write tests in?

[m4theushw]

You can add the test in

mui-x/packages/grid/x-data-grid-pro/src/tests/cellEditing.DataGridPro.new.test.tsx

Lines 669 to 670 in 03855a9

	describe('by pressing a printable character', () => {
	it(`should publish 'cellEditStart' with reason=printableKeyDown`, () => {

The fix you proposed should also be added to the row editing in https://github.com/mui/mui-x/blob/master/packages/grid/x-data-grid/src/hooks/features/editRows/useGridRowEditing.new.ts

Then we have a file only for tests with row editing, which has the same structure from the cell editing. You can almost just duplicate the new test.

mui-x/packages/grid/x-data-grid-pro/src/tests/rowEditing.DataGridPro.new.test.tsx

Lines 681 to 682 in 03855a9

	describe('by pressing a printable character', () => {
	it(`should publish 'rowEditStart' with reason=printableKeyDown`, () => {

[Gumichocopengin8]

@m4theushw thank you for the information.

---

I added test cases.

[Gumichocopengin8]

@oliviertassinari @m4theushw Could you please review this PR? Thanks!

[m4theushw]

@cherniavskii @DanailH could you test the fix on macOS? The shortcut to trigger the IME is different on Windows. I'm good with the tests added.

[cherniavskii]

Nice!
It works on macOS with Enter, but I would suggest to handle Tab key as well, since it can be used in IME on MacOS as well:

diff --git a/packages/grid/x-data-grid/src/hooks/features/e
ditRows/useGridCellEditing.new.ts b/packages/grid/x-data-gr
id/src/hooks/features/editRows/useGridCellEditing.new.ts
index 00519a0938..5357c34da9 100644
--- a/packages/grid/x-data-grid/src/hooks/features/editRows
/useGridCellEditing.new.ts
+++ b/packages/grid/x-data-grid/src/hooks/features/editRows
/useGridCellEditing.new.ts
@@ -137,6 +137,9 @@ export const useGridCellEditing = (
           }
           reason = GridCellEditStopReasons.enterKeyDown;
         } else if (event.key === 'Tab') {
+          if (event.which === 229) {
+            return;
+          }
           reason = event.shiftKey
             ? GridCellEditStopReasons.shiftTabKeyDown
             : GridCellEditStopReasons.tabKeyDown;
diff --git a/packages/grid/x-data-grid/src/hooks/features/e
ditRows/useGridRowEditing.new.ts b/packages/grid/x-data-gri
d/src/hooks/features/editRows/useGridRowEditing.new.ts
index 4514805788..7fcc203619 100644
--- a/packages/grid/x-data-grid/src/hooks/features/editRows
/useGridRowEditing.new.ts
+++ b/packages/grid/x-data-grid/src/hooks/features/editRows/useGridRowEditing.new.ts
@@ -179,6 +179,9 @@ export const useGridRowEditing = (
           }
           reason = GridRowEditStopReasons.enterKeyDown;
         } else if (event.key === 'Tab') {
+          if (event.which === 229) {
+            return;
+          }
           const columnFields = gridColumnFieldsSelector(apiRef).filter((field) =>
             apiRef.current.isCellEditable(apiRef.current.getCellParams(params.id, field)),
           );

Here's how it works with the changes above applied

Screen.Recording.2022-06-25.at.08.28.41.mov

[cherniavskii]

Let's do this for old editing implementation as well, since it's an easy change: https://github.com/mui/mui-x/blob/master/packages/grid/x-data-grid/src/hooks/features/editRows/useGridCellEditing.old.ts

[Gumichocopengin8]

@cherniavskii Thanks for reviewing.
I believe that Japanese people usually use space key to change the word instead of tab key. That's why I just applied the change only for enter. Do you still want the change for tab?

[oliviertassinari]

Do you still want the change for tab?

Agree with @cherniavskii; it's what mui/material-ui#23044 was exactly about after only handling Enter in mui/material-ui#19435. From my perspective, we should filter ALL key events, regardless of the value of event.key if event.which === 229, so Tab included. I don't think that we can predict how IME assistant works on all the platforms/browsers. On macOS, Tab support seems to be required to change tabs, same goes for Shift+Tab.

Screen.Recording.2022-06-25.at.12.22.04.mov

It's what select2 does https://github.com/select2/select2/pull/2483/files#diff-1fb8e09d75683cc4dd4a88850714595fR2105. Otherwise, we can copy Ant Design and go with, keeping track of the composite events: https://github.com/react-component/select/pull/505/files.

[Gumichocopengin8]

added a new commit to handle tab key. Also, edited implementation in useGridCellEditing.old.ts and added a test case for it.

[Gumichocopengin8]

@m4theushw @cherniavskii Could you review this again, please?

[mui-bot]

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	284.8	667.5	469.5	450.12	146.147
Sort 100k rows	ms	512.3	1,051.7	512.3	865.4	195.452
Select 100k rows	ms	141.3	261.3	224.6	215.78	41.723
Deselect 100k rows	ms	153.4	378	191.5	241.4	88.957

Generated by 🚫 dangerJS against 052373f

[m4theushw]

I'm happy with the tests added. @cherniavskii could you test on macOS if the fix works in both new and old editing APIs?

[cherniavskii]

Works with both old and new editing APIs.
Thank you @Gumichocopengin8, great work! 🎉

@Gumichocopengin8 Are you OK with licensing your changes to the test files under MIT license?

We are going to update our license in the repository to make all our test files MIT licensed. Currently, some of the tests files have a commercial license.

[Gumichocopengin8]

@cherniavskii

@Gumichocopengin8 Are you OK with licensing your changes to the test files under MIT license?
We are going to update our license to cover all test files in the repo by MIT license.

Yes, I'm ok with it. Thank you for reviewing!