[test] The cell with id=X and field= received focus

[ddolcimascolo]

Duplicates

• I have searched the existing issues

Latest version

• I have tested the latest version

Current behavior 😯

Some of our tests trigger a new warning in tests:

console.error
    MUI: The cell with id=<redacted> and field=  received focus.
    According to the state, the focus should be at id=undefined, field=undefined.
    In the next render, the focus will be changed to match the state.
    Call `fireEvent.mouseUp` and `fireEvent.click` before to sync the focus with the state.

This is very recent.
We're following upgrades closely and are currently running v5.5.0. I can bissect if you want to find the precise version where this started happening.

Expected behavior 🤔

No warning

Steps to reproduce 🕹

Run tests of https://codesandbox.io/s/datagrid-tests-warning-9dy64?file=/src/App.spec.tsx

This is a very minimal example of our use case:

• A few columns
• The last columns contains buttons
• Some buttons trigger a Dialog with advanced controls

Context 🔦

N/A

Your environment 🌎

`npx @mui/envinfo`

  System:
    OS: Linux 5.13 Linux Mint 20.3 (Una)
  Binaries:
    Node: 16.13.0 - ~/.nvm/versions/node/v16.13.0/bin/node
    Yarn: Not Found
    npm: 8.1.0 - ~/.nvm/versions/node/v16.13.0/bin/npm
  Browsers:
    Chrome: 98.0.4758.80
    Firefox: 96.0.3

Order ID 💳 (optional)

31461

[m4theushw]

We added this warning because we keep in the state which cell has focus and since we use the mouseup event to sync it, calling fireEvent.click won't update the state. You can fix it by also calling fireEvent.mouseUp before the click.

+fireEvent.mouseUp(button);
fireEvent.click(button);

Here's the updated CodeSandbox: https://codesandbox.io/s/datagrid-tests-warning-forked-lhpzp?file=/src/App.spec.tsx:523-582

More information about it see #3486.

[ddolcimascolo]

What's frustrating is that I'm not focusing anything, I'm clicking a button... This is still unclear why I need to refactor tests that work perfectly fine, testing code that run perfectly fine in production, just to avoid warnings that you added in the lib.
It looks like you wanted to improve DX of the MUI team working on data grid, which is fine, but you expect your users to write tests too, don't you?

Can you clarify?

Thx,
David

[flaviendelangle]

but you expect your users to write tests too, don't you?

I think the warning is also usefull for your tests
Because if you have an inconsistency between your DOM and the grid state, which could cause unwanted behaviors in your tests.

[peterkmurphy]

I am also encountering the issue. This warning (actually, a console.error call) is a bad thing because it doesn't just occur when I click a button in DataGrid, but also occurs when I click things outside of it..

Let me demonstrate with a code sample, using mui-5, jest and react-testing-library. I am willing to provide a full fledged example as well if requested. Let's assume that we start with a DataGrid listing items, and each row in the DataGrid has a small button that users can launch to open a content menu for that item: copying it for example, or deleting it.

    const contextMenuButtonElement = screen.getByTestId('open-action-menu-1'); // A button in the datagrid.
    await fireEvent.mouseUp(contextMenuButtonElement); // Silences those warning in issue #3850.
    await fireEvent.click(contextMenuButtonElement); // Clicks on the button.
    const deleteContextMenuItemElement = screen.queryByText(/(Delete)/); A context menu item, for 'Deleting' an item, appears.
    userEvent.click(deleteContextMenuItemElement!); // The user chooses to delete.
    expect(screen.getByText(/(Delete this list?)/)).toBeVisible(); // A confirmatory dialog appears, with "Cancel" and "Delete" buttons.
    const cancelButton = screen.getByText(/(Cancel)/); // Find the button.
    await fireEvent.mouseUp(contextMenuButtonElement); // I need to put this in as well to silence those warnings.
    await fireEvent.click(cancelButton); // Click on the cancel button.
    expect(screen.getByText(/(Delete this list?)/)).not.toBeVisible(); // The dialog and context menu disappears, and we are back at the grid.

Having to add the second await fireEvent.mouseUp(contextMenuButtonElement); line doesn't make sense at all. At that stage, it is the confirmatory dialog that has focus, not the DataGrid. In fact, nothing has changed in the DataGrid at all; nothing has been added or removed. (The user might not even see the contextMenuButtonElement at that stage, if the confirmatory dialog is located above it). I added the line to silence warnings, but I don't think I should have added it in the first place. And I don't like how adding the line was a series of trial and error.

Yes, there probably is an inconsistency between the DOM and the grid state - and it is something that should be looked at. But since this inconsistency is inside the grid, perhaps that warning could be silenced for others not working on the MUI DataGrid, especially when they are running tests.

[flaviendelangle]

@m4theushw could we enable this warning only if an env variable is present ? (or the opposite)

[acepitcher2009]

Have been trying in many different ways to get the fix from statements above to work but they are not working on our project. we are using userEvent and i have tried to place the fireEvent.mouseUp() function before every userEvent.click function but i still get the same error as stated above. are there any other work arounds?

[ddolcimascolo]

@m4theushw could we enable this warning only if an env variable is present ? (or the opposite)

Would work I suppose. But reading others comments above the warning itself might be wrong. Or am I missing something?

[ddolcimascolo]

I got the same feeling when I originally opened the issue. My test clicks a button, I use fireEvent.click, having to do mouseUp / mouseDown or other tricks feels weird...

[acepitcher2009]

on top of the weird feeling from having to add an extra mouse event the tests would not accurately simulate the users actions when performing actions on the data grid. i think this needs to be reverted or as @flaviendelangle said have an env variable to trigger the warnings

[m4theushw]

This warning was added because we had a useLayoutEffect without dependencies inside the cell. This way, if the focus was not synced correctly, in every render, it would jump to the correct cell. Warn about this condition was a way to avoid potential issues. However, #3718 added dependencies to the useLayoutEffect that made it to only be called once: when the cell receives focus. This made the warning useless is most situations. #4129 is disabling it and only firing it if warnIfFocusStateIsNotSynced=true. Note that if you're testing the behavior of clicking outside a cell to commit the value, the warning is still useful, so much so that we keep it in the cell/row editing tests.

[peterkmurphy]

@m4theushw : but I am not clicking outside a cell to commit a value. In my example above, I could delete an item from a DataGrid, but chose not to; ergo, the value of the DataGrid remains unchanged.

[ddolcimascolo]

Thx for the fix