[test] Warn when focusing cells without syncing the state

[m4theushw]

Previously, every time we had to fireEvent.keyDown in a cell, we would usually focus the cell with cell.focus() first. This approach has the problem that it doesn't sync the focus state, because the grid doesn't listen to the focus event, but only mouseup and keyup. For some simple test cases this is not a problem, however, if the focus moves from one cell to another this might cause an unexpected behavior. Take as example the test I had to modify in #3484:

const { setProps } = render(<NavigationTestCaseNoScrollX />);

getCell(8, 1).focus();
expect(getActiveCell()).to.equal('8-1');
fireEvent.keyDown(document.activeElement!, { key: 'End', ctrlKey: true });
expect(getActiveCell()).to.equal('9-2'); // The focused cell according to the state is 9-2

getCell(8, 1).focus(); // Try to focus another cell
expect(getActiveCell()).to.equal('8-1');
setProps({ foo: 'bar' }); // Simulates a rerender
expect(getActiveCell()).to.equal('8-1'); // It fails

Above we're navigating with the keyboard to the cell 9-2. After the keyboard event, the state is correctly updated to store that 9-2 is the focused cell. The problem comes if we try to focus another cell. For a brief moment it works, but if a rerender occurs in the meantime, the focus gets reset to the value in the state. This PR shows a warning every time that a cell receives focus while the state is not correctly synced. To fix the test above, we can drop cell.focus() and simulate a click:

-cell.focus();
+fireEvent.mouseUp(cell);
+fireEvent.click(cell);

If you also need to ensure that a row is not selected when clicking, use disableSelectionOnClick or first focus on a column header and navigate with the arrow keys to the desired cell. The alternative to this PR would be to update the focus on the focus event, but I don't know if it's worth it to change this now. The focus is always preceded by a click or keyboard event and we already use these events.

[github-actions]

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

[github-actions]

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

[github-actions]

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

[github-actions]

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

[github-actions]

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

[mui-bot]

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	192.5	364.4	264.3	270.04	59.704
Sort 100k rows	ms	401.9	822.7	696.7	645.42	141.224
Select 100k rows	ms	130.1	296.9	186.1	209.86	69.354
Deselect 100k rows	ms	118.7	191	135.1	148.66	26.088

Generated by 🚫 dangerJS against d38d69a

[github-actions]

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

[alexfauquette]

A big thanks for this PR, it explains why I was struggling to write tests on cell focus yesterday 😅

[alexfauquette]

why not fireClickEvent(cell)?

[flaviendelangle]

Maybe we could use mousePress utility method from the core

[m4theushw]

mousePress doesn't work. Some tests fail but I don't know why.

[alexfauquette]

Is it necessary?
In cellEditing.DataGridPro.test.tsx you remove all the cell.focus() but do not add fireEvent.mouseUp()

[m4theushw]

Yes, even though it's a double click, it uses the cell that received mouseUp to update the state. Below, cellParams is assigned by the mouseUp listener. If we don't fire it, the focus is never updated.

https://github.com/mui-org/material-ui-x/blob/9285a97a5e19eca7d06cb8f3d06e0ee1a1f05823/packages/grid/_modules_/grid/hooks/features/focus/useGridFocus.ts#L167-L168