[TreeView] `onClick` on RichTreeView component is invoked twice

rgavrilov commented 2 weeks ago

Order ID or Support key 💳

52426

Search keywords

RichTreeView

Latest version

[X] I have tested the latest version

The problem in depth

I have the following code. When I click an item - the console.log statement is executed twice. How do I prevent that?

        <RichTreeView
            items={treeViewItems}
            slots={{ item: TreeItem2 }}
            slotProps={{
                item: {
                    onClick: (e) => {
                        console.log("click");
                    },
                },
            }}
        />

Your environment

`npx @mui/envinfo`

System: OS: Windows 11 10.0.22631 Binaries: Node: 18.16.0 - C:\Program Files\nodejs\node.EXE npm: 9.8.1 - C:\Program Files\nodejs\npm.CMD pnpm: Not Found Browsers: Chrome: Not Found Edge: Chromium (123.0.2420.97) npmPackages: @emotion/react: 11.11.4 @emotion/styled: ^11.11.0 => 11.11.0 @mui/base: 5.0.0-beta.40 @mui/core-downloads-tracker: 5.15.15 @mui/icons-material: ^5.15.13 => 5.15.13 @mui/material: ^5.15.13 => 5.15.15 @mui/private-theming: 5.15.14 @mui/styled-engine: 5.15.14 @mui/system: 5.15.15 @mui/types: 7.2.14 @mui/utils: 5.15.14 @mui/x-data-grid: ^6.19.6 => 6.19.6 @mui/x-data-grid-pro: ^6.19.6 => 6.19.6 @mui/x-date-pickers: 6.19.7 @mui/x-date-pickers-pro: ^6.19.7 => 6.19.7 @mui/x-license-pro: ^6.10.2 => 6.10.2 @mui/x-tree-view: ^7.3.0 => 7.3.0 @types/react: ^18.2.64 => 18.2.66 react: ^18.2.0 => 18.2.0 react-dom: ^18.2.0 => 18.2.0 typescript: ^5.2.2 => 5.4.2

flaviendelangle commented 2 weeks ago

Hi, Is this only occurring when you click on nested items or also on root items (items with no parent)?

If it's only for nested items, I have one explanation:

The item root contains its children as you can see in the screenshot below:

In TreeItem the onClick and onMouseDown prop are passed to the item content, which do not contain its children:

This explains the difference in behavior.

To be honest, I have totally overlooked this change in behavior and it is not intentional. With that being said, I think it's very weird to have onClick and onMouseDown passed to an element deep inside the item but not the other callbacks like onMouseUp or onKeyDown.

The general rule in all our components is to forward the event handlers to the root unless we can't for some reason (onChange is forwarded to the input in a TextField, not to the root which is a div, for obvious reasons).

@LukasTy @noraleonte, do you think we should keep this exception and forward the onClick and onMouseDown to the content slot (it's a breaking change but would be marked as a bug fix to keep consistency with TreeItem). Or do you think we should keep the new behavior and document it correctly (since it's one of the few behavior change).

Until then, you can fix your behavior by passing the onClick to the content:

<RichTreeView
    items={ITEMS}
    slots={{ item: TreeItem2 }}
    slotProps={{
        item: {
            slotProps: {
                content: {
                    onClick: (e) => {
                        console.log("click", e.target);
                    },
                }
            }
        },
    }}
/>

It's super verbose, and once we drop TreeItem I'm in favor to introduce new slots to shorten it:

<RichTreeView
    items={ITEMS}
    slots={{ item: TreeItem2 }}
    slotProps={{
        // Do not exist for now
        itemContent: {
            onClick: (e) => {
                console.log("click", e.target);
            },
        },
    }}
/>

LukasTy commented 2 weeks ago

do you think we should keep this exception and forward the onClick and onMouseDown to the content slot (it's a breaking change but would be marked as a bug fix to keep consistency with TreeItem). Or do you think we should keep the new behavior and document it correctly (since it's one of the few behavior change).

If there is a tangible benefit to having this behavior change, then it might make sense to keep it. But at first sight, it seems more of an oversight that could be brought up multiple times by people stumbling upon it. 🙈 I would lean towards fixing it. 🤔

noraleonte commented 2 weeks ago

I agree with Lukas. Since this does not seem to bring any specific value, and it also introduces an inconsistency, we should probably fix it 🤔

flaviendelangle commented 2 weeks ago

But isn't the fact that onClick / onMouseDown are passed to content and the other event handlers are passed to root an inconsistency on TreeItem?

The current behavior of TreeItem2 is more consistent IMHO

This code behave super weirdly right now,

<TreeItem
  onMouseDown={handleMouseDown}
  onMouseUp={handleMouseUp}
/>

flaviendelangle commented 2 weeks ago

This relates to #12850

Maybe keeping the behavior of TreeItem2 and add a onItemClick at the Tree View level that targets the content is a good approach.

People have a super easy way to pass an onClick to the content (which is a common use-case and should be easy to do) AND the behavior are consistent across event handlers.

For onMouseDown I think the current behavior on TreeItem2 is fine, it's coherent with onMouseUp and it's not a super common use-case so the slotProps approach is good enough.

flaviendelangle commented 1 week ago

On both AntDesign and React Arborists the items are not nested (childs are siblings of their parent item, like we have on the grid) so their is no root / content difference...

Kendo UI has an onItemClick (same for drag end, drag over and drag start) props at the TreeView level: https://www.telerik.com/kendo-react-ui/components/treeview/api/TreeViewProps/#toc-onitemclick

github-actions[bot] commented 1 week ago

The issue has been inactive for 7 days and has been automatically closed.

mui / mui-x