Material-ui: [Tooltip] Using custom component as children not working

[SuEric]

Using custom components as children of tooltip is not working

• [X] The issue is present in the latest release.
• [ ] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Tooltip does not work

Expected Behavior 🤔

Tooltip should work after forwarding the ref

Steps to Reproduce 🕹

https://codesandbox.io/s/material-demo-0yx08?file=/IconButton.js
Steps:

1. Create a custom component that contains a simple button element
2. Use the custom component as children of Tooltip
3. Notice how tooltip does not work

Context 🔦

Using custom components as children of tooltip is not working

Your Environment 🌎

USE SANDBOX LINK

[oliviertassinari]

@SuEric You also need to forward the props, on top of the ref:

const MyButton = forwardRef((props, ref) => <button ref={ref} {...props}>+</button>);

I have already seen this question a few times. I think that we could improve the documentation, creating more contrast, and avoiding text-overflow.

```diff diff --git a/docs/src/pages/guides/composition/composition.md b/docs/src/pages/guides/composition/composition.md index bd1ee1735..7d317e284 100644 --- a/docs/src/pages/guides/composition/composition.md +++ b/docs/src/pages/guides/composition/composition.md @@ -165,25 +165,31 @@ ref forwarding. However, only the following component types can be given aref`

If you don't use one of the above types when using your components in conjunction with Material-UI, you might see a warning from
React in your console similar to:
-> Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?

-Be aware that you will still get this warning for lazy and memo components if their
-wrapped component can't hold a ref.
+> Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?

+Be aware that you will still get this warning for lazy and memo components if their wrapped component can't hold a ref.
In some instances an additional warning is issued to help with debugging, similar to:
+

Invalid prop component supplied to ComponentName. Expected an element type that can hold a ref.

Only the two most common use cases are covered. For more information see this section in the official React docs.

diff --const MyButton = props => <div role="button" {...props} />; -+const MyButton = React.forwardRef((props, ref) => <div role="button" {...props} ref={ref} />); +-const MyButton = () => <div role="button" />; ++const MyButton = React.forwardRef((props, ref) => ++ <div role="button" {...props} ref={ref} /> ++); + <Button component={MyButton} />;

````

It might actually be enough, otherwise, we could improve on the warning side of things

diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js
index d21812392..61c037d89 100644
--- a/packages/material-ui/src/Tooltip/Tooltip.js
+++ b/packages/material-ui/src/Tooltip/Tooltip.js
@@ -431,6 +431,22 @@ const Tooltip = React.forwardRef(function Tooltip(props, ref) {
     ref: handleRef,
   };

+  if (process.env.NODE_ENV !== 'production') {
+    childrenProps['data-mui'] = true;
+
+    // eslint-disable-next-line react-hooks/rules-of-hooks
+    React.useEffect(() => {
+      if (childNode && !childNode.getAttribute('data-mui')) {
+        console.error(
+          [
+            'Material-UI: The `children` component of the Tooltip do not forward its props correctly.',
+            'Please make sure the props are spread on the same element the ref is applied to.',
+          ].join('\n'),
+        );
+      }
+    }, [childNode]);
+  }
+
   const interactiveWrapperListeners = {};

   if (!disableTouchListener) {

[SuEric]

@oliviertassinari Taking a look right now and will close if I don't have any other issues.

Thank you so much for the detailed explanation.

[SuEric]

@oliviertassinari Question;

So... All or at least most of the material-ui component needs that the developer forward both the ref and the props when wrapping a children component?? In order to preserve the proper behavior?

[dfernandez-asapp]

@oliviertassinari I think what is missing in the docs and catch me by surprise too is that the Tooltip implementation relies on passing the onMouseOver, onMouseLeave, onFocus, onBlur, onTouchStart, and onTouchEnd event handlers to the children. See: https://github.com/mui-org/material-ui/blob/next/packages/material-ui/src/Tooltip/Tooltip.js#L425

The documentation says that you only need an element that is able to hold a ref, but that's not what is happening in the code. I wonder if it makes sense to change the code use useEffect and use a DOM event handler.

[oliviertassinari]

@dfernandez-asapp How would you solve the issue? What did you think of my proposed changes?

[dfernandez-asapp]

If I understood correctly the proposed change is to pass props to the element.

That works. My expectation as a user is that any component with a ref will work, so the workaround breaks my expectation. If that is the design decision, is ok... it should be added to the docs (probably a TS interface will help to catch errors early, for the ones that use TS). Also is interesting that the code relies on onMouseOver but not on onMouseEnter because of a bug barely commented in the code.... so basically it makes the child component dependent on the tooltip internals.

Design matters aside, honestly, I didn't run into this issue before because I was using a div or another MUI component as a child of the tooltip. So it may not be an issue for many users. Is possible to re-implement the code so it only relies on the ref. That tooltip code is already intricate, so I'm not sure about it (I'll love to help, but right now I'm unable to, sorry).

I don't know how good or bad is to rely on effects and addEventListener, if you have a table of many elements it will add many listeners...(unlike React that uses a single event handler) that was a concern at the time of IE8 but probably the popper stuff and the React hooks are "heavier" than that. If I have to re-implement the tooltip from scratch I'll go with the useEffect route... is not so React-ish but it will work with any component without having to rely on the children props interface.

[oliviertassinari]

@dfernandez-asapp Ok, great, so we can move forward with the proposed change and complement it with some documentation. We expect the wrapped element to forwarded the ref and all its props. I think that we could go all-in in the direction, anytime we the forward ref, ask for forwarding props too.

Regarding why you can blame the lines and look at the history to understand the problem we solved with it.
If you have ideas around how we could improve the implementation, feel free to give them a try, we have a comprehensive testing suite.