mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.53k stars 1.32k forks source link

[data grid] Components that are rendered in a cell do not get the onclick event the first time the cell is clicked. #15126

Closed masull closed 2 weeks ago

masull commented 2 weeks ago

Steps to reproduce

Link to live example: https://codesandbox.io/p/sandbox/cool-frost-p7wh9m

Steps:

  1. Click the link above to open codesandbox.
  2. Click the See 6 Employees button, nothing happens.
  3. Click the See 6 Employees button again, the row expands.
  4. The difference between this example and the one posted in your mui data grid Tree examples, is that I made the button into a component. If the buttons are in the original return at the bottom of the example, they get the clicked event.

Current behavior

Component inside cell needs to be clicked twice to get the onClick event.

Expected behavior

The component should get the onClicked event after one click.

Context

Render a grouped or regular cell in the data grid with onClick listeners that respond the first time they are clicked.

Your environment

npx @mui/envinfo $ npx @mui/envinfo System: OS: Windows 11 10.0.22631 Binaries: Node: 22.9.0 - C:\Program Files\nodejs\node.EXE npm: 10.9.0 - C:\work\cbd-phoenix-vite\node_modules\.bin\npm.CMD pnpm: Not Found Browsers: Chrome: Not Found Edge: Chromium (127.0.2651.74) npmPackages: @emotion/react: 11.13.3 @emotion/styled: 11.13.0 @mui/base: 5.0.0-beta.42 @mui/core-downloads-tracker: 6.1.4 @mui/icons-material: 6.1.4 @mui/lab: 6.0.0-dev.240424162023-9968b4889d @mui/material: 6.1.4 @mui/private-theming: 6.1.4 @mui/styled-engine: 6.1.4 @mui/styles: 6.1.4 @mui/system: 6.1.4 @mui/types: 7.2.18 @mui/utils: 6.1.4 @mui/x-data-grid: 7.21.0 @mui/x-data-grid-pro: 7.21.0 @mui/x-date-pickers: 7.20.0 @mui/x-internals: 7.21.0 @mui/x-license: 7.21.0 @types/react: 18.3.11 react: 18.3.1 react-dom: 18.3.1 Chrome is up to date Version 130.0.6723.70 (Official Build) (64-bit) ``` Don't forget to mention which browser you used. Output from `npx @mui/envinfo` goes here. ```

Search keywords: render cell onclick twice

michelengelen commented 2 weeks ago

There is a race condition in place. The button rerenders before the click event can get propagated to it. The rerender happens because the cell is being selected on click.

@MBilalShafi can we do something about this?

masull commented 2 weeks ago

This also happens in cells on the data grid, the cell is selected on the first click, then the react component must be clicked a second time. Is there a way to disable cell selection so that the component receives the click event?

MBilalShafi commented 2 weeks ago

Hey @masull,

Is there a specific reason for defining the ButtonComponent component inside the CustomGridTreeDataGroupingCell component scope? I was able to fix the issue by moving the Button component out: https://codesandbox.io/p/sandbox/cocky-galileo-x58wqr

Does that make sense in your use case?

masull commented 2 weeks ago

Good morning @MBilalShafi, I appreciate the workaround. I defined the ButtonComponent inside to illustrate the issue, it can be defined outside the CustomGridTreeDataGroupingCell. I've tested other react components that are defined outside the component that still require two clicks. This behavior is not constrained to the CustomGridTreeGroupingCell, it also happens on a column renderCell. I'm wondering if a useCallBack will help, and why it happens at all. This seems related to #14886

MBilalShafi commented 2 weeks ago

I've tested other react components that are defined outside the component that still require two clicks.

I'd love to see an example where it doesn't work when the component is stable. Do you have one in reach? Kindly share in some codesandbox/stackblitz example.

I'm wondering if a useCallBack will help, and why it happens at all.

Yes the useCallback should fix it too: https://codesandbox.io/p/sandbox/unruffled-pond-h9j7rv?file=%2Fsrc%2FDemo.js%3A38%2C14

I think it's an expected behavior.

When the cell is clicked, it gets re-rendered due to focus state change, due to re-render the Button component reference is also updated (since it's defined in the function scope), and the onClick handler gets fired on the previous component which is not rendered anymore, so it doesn't have any visible impact.

We should ideally not define React components inside other components, if it's inevitable to do so, they should be memoized.

masull commented 2 weeks ago

@MBilalShafi thank you, I used the techniques you suggested, and all is well.

Mark

github-actions[bot] commented 2 weeks ago

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue. Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

[!NOTE] @masull How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.