[DataGrid] Fix focus on checkbox cells

[alexfauquette]

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.

[flaviendelangle]

We could maybe add a test where we focus the checkbox, the click the body, then check if the checkbox is still focused

[m4theushw]

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.

How about removing event.stopPropagation and, on handleRowClick, do nothing if the event.target is inside the checkbox column? I started to do that to avoid selecting the row when opening the actions column menu. The alternative would be to stop propagation but I didn't want this hack.

https://github.com/mui-org/material-ui-x/blob/dd4b11fb73dd33c4945550619bf517c05bc4211b/packages/grid/_modules_/grid/hooks/features/selection/useGridSelection.ts#L338-L340

We can go even further in this approach and add a prop to GridColDef (see the description of #3492) to not select rows when an element inside the cell is clicked. I think that with more features being added this will become a common problem.

[alexfauquette]

I agree on removing event.stopPropagation this would also fix #3382

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 event.selectionDone: true such that when the row is listening to a click, it knows that selection has already been done by another listener before.

[m4theushw]

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 disableSelectionOnClick but it disables the selection when clicking any column, not individually. The problem with keeping event.stopPropagation() is that eventually we will have to revisit this component to remove it.

[alexfauquette]

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 ActionCell/row interactions. So yes your solution should work, and we will see in the future if new features imply to redesign our strategy or not

[m4theushw]

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 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.

I would like to find, a solution to systematically prevent the parent from doing something already done by a child.

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 rowClick.

[alexfauquette]

I followed @m4theushw suggestion. If the row click is from a checkbox cell, it does not trigger the selection

[mui-bot]

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	196.7	410.2	296.4	291.12	74.557
Sort 100k rows	ms	368.9	805.4	695.5	660.36	157.068
Select 100k rows	ms	209.5	383	253.6	276.58	60.148
Deselect 100k rows	ms	150.3	257.7	185.9	200.1	46.904

Generated by 🚫 dangerJS against e1d4eda

[m4theushw]

This also fixes another bug I noticed:

1. Open https://codesandbox.io/s/datagriddemo-material-demo-forked-s3u4i?file=/demo.js
2. Select one row
3. Click "Toggle checkbox selection"
4. 💥

Since there's no click sent by the checkbox, it doesn't clean the last clicked cell below:

https://github.com/mui-org/material-ui-x/blob/ed223c349df133419b025b08199c7e36ff82f1d4/packages/grid/_modules_/grid/hooks/features/focus/useGridFocus.ts#L134

[alexfauquette]

You find really strange bugs 😅 I confirm, it does not appears on this branch