-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DataGrid] Fix focus on checkbox cells #3501
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could maybe add a test where we focus the checkbox, the click the body, then check if the checkbox is still focused
How about removing We can go even further in this approach and add a prop to |
I agree on removing But only blocking a column because in this case, it is the checkbox column that has similar interaction than row click seems a bit flacky. Another idea would be to add some property to the event when a modification has been done. For example, when clicking on the checkbox, after modifying the selection, it add |
Why do you think it's flaky? I see it differently. There're cell components that may receive clicks inside, since they contain interactive elements (e.g. the checkbox and the actions column), and the click shouldn't select the row. We already have |
Flaky is probably not the correct word. It is more about how to generalize the solution. I feel like each time we will have a conflict between the event listener of nested components, we will have to find a hack in the listener of the parents to remove the duplicate. I would like to find, a solution to systematically prevent the parent from doing something already done by a child. But I do not find a clear way of doing that, and it seems that most of the stopPropagation calls are made to deal with |
I don't think it's a hack. We only need to ignore all clicks coming from a specific field. The code to check which field was clicked is the same.
This is complicated. We don't know what the child did. Currently, we only have problem with selection, but there could be other listeners on |
26c2a9d
to
a25a91e
Compare
I followed @m4theushw suggestion. If the row click is from a checkbox cell, it does not trigger the selection |
These are the results for the performance tests:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also fixes another bug I noticed:
- Open https://codesandbox.io/s/datagriddemo-material-demo-forked-s3u4i?file=/demo.js
- Select one row
- Click "Toggle checkbox selection"
- 💥
Since there's no click
sent by the checkbox, it doesn't clean the last clicked cell below:
packages/grid/_modules_/grid/hooks/features/selection/useGridSelection.ts
Outdated
Show resolved
Hide resolved
packages/grid/x-data-grid/src/tests/selection.DataGrid.test.tsx
Outdated
Show resolved
Hide resolved
You find really strange bugs 😅 I confirm, it does not appears on this branch |
Fix #3382
The stop propagation prevents having the selection event dispatched two times when clicking on the checkbox. One time by checkbox change, and the other one by row click event.
The problem is that it also prevents setting the focus on the cell. So now, interacting with the checkbox set the focus on its cell.
I did not add tests to detect the reported bug, because jsdom does not mock correctly, scrolling.