[TreeView] Add support for checkbox selection

[flaviendelangle]

Fixes #6019
Doc preview

The main inspiration is https://mui.com/x/react-data-grid/row-selection/#checkbox-selection

• Add checkbox selection on TreeItem
• Add checkbox selection on TreeItem2
• Add doc
• Add tests

[mui-bot]

Deploy preview: https://deploy-preview-11452--material-ui-x.netlify.app/

Updated pages:

• docs/data/tree-view/rich-tree-view/selection/selection.md
• docs/data/tree-view/simple-tree-view/selection/selection.md

Generated by 🚫 dangerJS against 1aa1a58

[github-actions]

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

[joserodolfofreitas]

Nice! This is looking quite solid. I'd say we can release it soon and quickly bring value for those willing to install a pre-release.

To have a good clean implementation we need

1. To add a baseCheckbox slot like on the grid to allow using other design systems

I agree, and I suppose users can style the default checkbox, which probably cover the majority of customization use cases, so maybe we can release the slot on a follow up?

2. To add improve the plugin system so that each plugin can pass stuff to the context, having to add stuff to useTreeViewContextValueBuilder is bad because we have one plugin that is aware of the existing of all the others.

I suppose this is also a separate PR. Must it be delivered first?

[flaviendelangle]

I agree, and I suppose users can style the default checkbox, which probably cover the majority of customization use cases, so maybe we can release the slot on a follow up?

Right now they can't pass props to the default checkbox specifically on the tree view (they'd need a lot to do it)
All they can do it use the theme to pass default props to all their checkbox throughout their application (even the one not in tree view).

Their is nothing preventing us from adding the slot in a follow up though. I think it will quickly be useful, but there is no reason to be blocking.

I suppose this is also a separate PR. Must it be delivered first?

For now this is only used internally since nobody except us can build plugins, so nothing blocking here 👍

[github-actions]

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

[flaviendelangle]

Would be great if TS would have caught this one...

[LukasTy]

EDIT: I propose to not support the children automatic selection on this 1st version to avoid delaying the release too much. And to create an issue to explore how to support it.

The support could look something like:

• Add a new prop enableAutomaticDescendantSelection (name to be determined of course)
• If this prop is enabled, then the model only contains the selection of the leaves. Any id in the model that is not the leave is ignore. Items with children have their selection deduced from the selection state of their children. When using the checkbox selection, we support the indeterminate state.

I have no objections against separating the delivery of the complete feature behavior.
The proposed approach makes sense, but I feel that the enableAutomaticDescendantSelection might make more sense as the default behavior.
WDYT? 🤔

[noraleonte]

I am also in favor of creating a follow up for automatic children selection 👌
But I agree that the behavior should exist and should probably be the default 🤔

I don't really like the idea of the selection of the parents being determined strictly by the state of their children. It could become very messy for nested trees. A flow I think also makes sense is the one that Figma (and other similar software) uses for layer selection.

If you have a tree like this:

- 1
  - 1.1
  - 1.2
- 2
  - 2.1
  - 2.2

If you select 2.1 & 2.2 the selection state is ['2.1','2.2']. However, if you select 1, it automatically selects the children as well, and the state is ['1', '1.1', '1.2'].

figma.selection.mp4

[flaviendelangle]

but I feel that the enableAutomaticDescendantSelection might make more sense as the default behavior.

It's a BC so it could only be opt-out in v8 even if the feature is released before (which is far from obvious)
But I don't have a strong opinion about what the default behavior should be in the end.
If we feel like none of those two use-cases is used a lot more than the other, then maybe it would be a better DX to have an enum prop rather than a boolean (something like descendantSelection?: 'independant' | 'automatic' with perhaps automatic as the default.

---

@noraleonte , interesting
This is yet another behavior, which don't seems to make sense with checkbox but do make sense without them.
Selecting a parent selects the children, but selecting all the children do not select the parent (if I understand correctly).

With this one, I do agree that we don't have a problem with the model structure, it can always represent what has been selected and the visual selection is basically isSelected(itemId) || hasSomeAncestorSelected(itemId).

[flaviendelangle]

We don't want to toggle the expansion when clicking on the checkbox

[LukasTy]

Have you tried setting event = defaultMuiPrevented in the checkbox change handler? 🤔
Maybe it would allow us to ditch the || case? 🤔

[flaviendelangle]

Yes, the onChange event is not shared with the onClick one so it does not work
I could add an onClick on the checkbox and prevent there of course, I'll see if it works. If it allow us to skip the ref then it's probably an improvement

[LukasTy]

Ah, damn, indeed, those are different kinds of events. 🙈
In that case, I'm fine with both approaches. However, naturally, if ref is used only for this check, then it probably makes sense to intercept the onClick on the checkbox and avoid this problem that way. 👍

[flaviendelangle]

I tried, the defaultMuiPrevented works.
The only problem is that it's usually a property our users set to disable our behavior and that we very rarely set ourselves (I only found one occurence in the core codebase).
I don't know if this would have side effects, to be honest I didn't find any...

Here is the diff for TreeItem:

diff --git a/packages/x-tree-view/src/TreeItem/TreeItemContent.tsx b/packages/x-tree-view/src/TreeItem/TreeItemContent.tsx
index 161d3d3af..c16d94c5b 100644
--- a/packages/x-tree-view/src/TreeItem/TreeItemContent.tsx
+++ b/packages/x-tree-view/src/TreeItem/TreeItemContent.tsx
@@ -3,6 +3,7 @@ import PropTypes from 'prop-types';
 import clsx from 'clsx';
 import Checkbox from '@mui/material/Checkbox';
 import { useTreeItemState } from './useTreeItemState';
+import { MuiCancellableEvent } from '../internals/models/MuiCancellableEvent';
 
 export interface TreeItemContentProps extends React.HTMLAttributes<HTMLElement> {
   className?: string;
@@ -85,7 +86,6 @@ const TreeItemContent = React.forwardRef(function TreeItemContent(
   } = useTreeItemState(itemId);
 
   const icon = iconProp || expansionIcon || displayIcon;
-  const checkboxRef = React.useRef<HTMLButtonElement>(null);
 
   const handleMouseDown = (event: React.MouseEvent<HTMLDivElement>) => {
     preventSelection(event);
@@ -95,8 +95,8 @@ const TreeItemContent = React.forwardRef(function TreeItemContent(
     }
   };
 
-  const handleClick = (event: React.MouseEvent<HTMLDivElement>) => {
-    if (checkboxRef.current?.contains(event.target as HTMLElement)) {
+  const handleClick = (event: React.MouseEvent<HTMLDivElement> & MuiCancellableEvent) => {
+    if (event.defaultMuiPrevented) {
       return;
     }
 
@@ -111,6 +111,12 @@ const TreeItemContent = React.forwardRef(function TreeItemContent(
     }
   };
 
+  const handleCheckboxClick = (
+    event: React.MouseEvent<HTMLButtonElement> & MuiCancellableEvent,
+  ) => {
+    event.defaultMuiPrevented = true;
+  };
+
   return (
     /* eslint-disable-next-line jsx-a11y/click-events-have-key-events,jsx-a11y/no-static-element-interactions -- Key event is handled by the TreeView */
     <div
@@ -131,8 +137,8 @@ const TreeItemContent = React.forwardRef(function TreeItemContent(
           className={classes.checkbox}
           checked={selected}
           onChange={handleCheckboxSelection}
+          onClick={handleCheckboxClick}
           disabled={disabled || disableSelection}
-          ref={checkboxRef}
           tabIndex={-1}
         />
       )}

[LukasTy]

Great feature and superb execution! 💯 🎉
Leaving last few questions / discussion points. 👍

[LukasTy]

Gotcha. It makes sense given the existing implementation. 👌
I'd just be curious to hear if @mui/material-ui thinks that this is correct. 🤔

[LukasTy]

Have you tried setting event = defaultMuiPrevented in the checkbox change handler? 🤔
Maybe it would allow us to ditch the || case? 🤔