[Tooltip] Do not warn about title property passed to wrapped element if title is not rendered

[darkowic]

I want to wrap a component that receives title property with Tooltip. Tooltips throw warning that the component can not have title property. No, I can not rename the property.

• This is not a v0.x issue.
• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

There should not be a warning if title attribute is not rendered into the DOM.

Current Behavior 😯

Even if the title is not rendered the warning is still thrown.

Steps to Reproduce 🕹

https://codesandbox.io/s/tootip-title-warning-22hsx

Context 🔦

See codesandbox.

Your Environment 🌎

Tech	Version
Material-UI	v4.2.X

Is it even possible to solve? My idea is to check the element node title value after the element is rendered.

[joshwooding]

There might be a smarter way to handle our check

[oliviertassinari]

Interesting title conflict problem. I have tried the following diff locally:

diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js
index 066461540..76247c7dc 100644
--- a/packages/material-ui/src/Tooltip/Tooltip.js
+++ b/packages/material-ui/src/Tooltip/Tooltip.js
@@ -12,6 +12,8 @@ import Popper from '../Popper';
 import { useForkRef } from '../utils/reactHelpers';
 import { useIsFocusVisible } from '../utils/focusVisible';

+const useEnhancedEffect = typeof window !== 'undefined' ? React.useLayoutEffect : React.useEffect;
+
 export const styles = theme => ({
   /* Styles applied to the Popper component. */
   popper: {
@@ -181,13 +183,6 @@ function Tooltip(props) {
       return;
     }

-    // Remove the title ahead of time.
-    // We don't want to wait for the next render commit.
-    // We would risk displaying two tooltips at the same time (native + this one).
-    if (childNode) {
-      childNode.setAttribute('title', '');
-    }
-
     clearTimeout(enterTimer.current);
     clearTimeout(leaveTimer.current);
     if (enterDelay) {
@@ -316,12 +311,34 @@ function Tooltip(props) {
     open = false;
   }

+  // Remove the title ahead of time.
+  // We don't want to wait for the next render commit.
+  // We would risk displaying two tooltips at the same time (native + this one).
+  useEnhancedEffect(() => {
+    if (!open) {
+      return undefined;
+    }
+
+    const restoreTitle = childNode.getAttribute('title');
+
+    if (restoreTitle) {
+      childNode.setAttribute('title', '');
+
+      return () => {
+        childNode.setAttribute('title', restoreTitle);
+      };
+    }
+
+    return undefined;
+  }, [open]);
+
   // For accessibility and SEO concerns, we render the title to the DOM node when
   // the tooltip is hidden. However, we have made a tradeoff when
   // `disableHoverListener` is set. This title logic is disabled.
   // It's allowing us to keep the implementation size minimal.
   // We are open to change the tradeoff.
   const shouldShowNativeTitle = !open && !disableHoverListener;
   const childrenProps = {
     'aria-describedby': open ? id || defaultId.current : null,
     title: shouldShowNativeTitle && typeof title === 'string' ? title : null,
@@ -354,14 +371,6 @@ function Tooltip(props) {
       }
     : {};

-  warning(
-    !children.props.title,
-    [
-      'Material-UI: you have provided a `title` prop to the child of <Tooltip />.',
-      `Remove this title prop \`${children.props.title}\` or the Tooltip component.`,
-    ].join('\n'),
-  );
-
   return (
     <React.Fragment>
       {React.cloneElement(children, { ref: handleRef, ...childrenProps })}

It seems that we can support the double title case, even if applied on a DOM node.

@darkowic In your example, I would encourage you to apply the title on the span:

import React from "react";
import Tooltip from "@material-ui/core/Tooltip";

const SomeComponent = React.forwardRef(({ title, ...props }, ref) => (
  <span ref={ref} title={title} {...props}>
    {title}
  </span>
));

export default function SimpleTooltips() {
  return (
    <div>
      <Tooltip title="This is my tooltip title">
        <SomeComponent title="This is my title" />
      </Tooltip>
    </div>
  );
}

[oliviertassinari]

Hum, shouldn't

childNode.setAttribute('title', '');

be replaced with

childNode.removeAttribute('title');

?

[darkowic]

@darkowic In your example, I would encourage you to apply the title on the span:

import React from "react";
import Tooltip from "@material-ui/core/Tooltip";

const SomeComponent = React.forwardRef(({ title, ...props }, ref) => (
  <span ref={ref} title={title} {...props}>
    {title}
  </span>
));

export default function SimpleTooltips() {
  return (
    <div>
      <Tooltip title="This is my tooltip title">
        <SomeComponent title="This is my title" />
      </Tooltip>
    </div>
  );
}

This is a very simple example to show the problem ;) The component I want to wrap with Tooltip is more complicated and build from a few different components:

<div>
  <div>{label}<div>
  <div>
    // other stuff
    {children}
  </div>

etc.

Hum, shouldn't

childNode.setAttribute('title', '');

be replaced with

childNode.removeAttribute('title');

?

Yep, I think so. Now it keeps empty title attribute in the wrapped node when you hover the element.

@oliviertassinari about your proposition - I do not think we need to keep the restored title - we can get it from props. title has to be provided anyway to be able to open the Tooltip.

+  // Remove the title ahead of time.
+  // We don't want to wait for the next render commit.
+  // We would risk displaying two tooltips at the same time (native + this one).
+  useEnhancedEffect(() => {
+    if (!open) {
+      return undefined;
+    }
+
+   childNode.removeAttribute('title');
+
+    return () => {
+      childNode.setAttribute('title', title);
+    };
+  }, [open]);

[oliviertassinari]

about your proposition - I do not think we need to keep the restored title - we can get it from props. title has to be provided anyway to be able to open the Tooltip.

@darkowic I was wondering about it too. I fear we can't get it from the prop, the actual title applied might be different. For instance, in your example, we could apply foo as a title on the span. We don't have another way to get this information than to read the DOM.

[oliviertassinari]

@joshwooding Does it sounds good to you to remove the warning?

[darkowic]

I fear we can't get it from the prop, the actual title applied might be different. For instance, in your example, we could apply foo as a title on the span.

What's the point of wrapping a Component with Tooltip if it does not set the title? I am afraid I do not understand what you want to achieve. In my case, the component does not set any title (maybe this is wrong as you pointed out). The Tooltip component is here to do that.

Maybe it is just enough to check on update if the rendered title is different than the tooltip's title? And then throw the warning like You have wrapped with Tooltip a component that already renders different title attribute.

[oliviertassinari]

What's the point of wrapping a Component with Tooltip if it does not set the title?

@darkowic The component needs to apply the title as a DOM attribute. If the wrapped component doesn't forward the title, it won't work as intended. So, it sounds that the only change we should to is to leverage .removeAttribute(). The warning is valid, even in your case, you should use two different props.

diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js
index 066461540..9284172c7 100644
--- a/packages/material-ui/src/Tooltip/Tooltip.js
+++ b/packages/material-ui/src/Tooltip/Tooltip.js
@@ -185,7 +185,7 @@ function Tooltip(props) {
     // We don't want to wait for the next render commit.
     // We would risk displaying two tooltips at the same time (native + this one).
     if (childNode) {
-      childNode.setAttribute('title', '');
+      childNode.removeAttribute('title');
     }

     clearTimeout(enterTimer.current);

[darkowic]

You are right. I missed that obvious problem. There is no other way to handle it correctly than renaming the Component's property...

[joshwooding]

@oliviertassinari Seems like the warning should stay and removeAttribute be used?

[oliviertassinari]

@joshwooding 👍, so this issue is up for grab :)

[jamesgeorge007]

@oliviertassinari I shall take this up, would be great if you could point me in the right direction (respective file to work with) as I'm not that aware of the code-base 👏