Material-ui: [Tooltip] onFocus does not work on TextField if using a Tooltip with it

[kidokidozh]

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

Current Behavior 😯

onFocus does not work if using a Tooltip with TextField:

<Tooltip title="Some hint">
  <TextField onFocus={() => {console.log("This cannot work!")}} />
</Tooltip>

This problem does not exist in Material-UI version <=4.7.1

Expected Behavior 🤔

onFocus can work when using a Tooltip.

Steps to Reproduce 🕹

https://codesandbox.io/s/modest-proskuriakova-z9sob

Steps:

1. Add a TextField to let user input something.
2. Bind an onFocus function to the TextField.
   -> The function works well.
3. Add a Tooltip to the TextField to show some hint for user.
   -> The onFocus function does not work any more.

Context 🔦

I found this issue after I upgrade Material-UI to latest version. Now I have to use native input instead of TextField to avoid this problem.
Please correct me if I'm using the component wrongly. Thanks in advance!

Checked the source code, I think the problem is caused by this commit: https://github.com/mui-org/material-ui/commit/c98b9c47c5df2c94a72d5ce2b5169c1c53a1d842

Your Environment 🌎

| Tech | Version |
| ----------- | ------- |
| Material-UI | v4.9.4 |
| React | v16.13.0 |
| Browser | Chrome 80 |

[eps1lon]

Introduced in #18687 which should be reverted until we unterstand mouseEnter/over event propagation in react.

[oliviertassinari]

Agree, #18687 wasn't probably the best fix for this. It didn't account for components, like the TextField. that might apply the event on a nested element.

I think that a clean solution would be to add an argument to handleFocus, handleLeave, and handleEnter to correctly forward the prop event handler: meaning, staying at the React level only, not looking into the DOM.

diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js
index 3f7991e68..53c1b9ee2 100644
--- a/packages/material-ui/src/Tooltip/Tooltip.js
+++ b/packages/material-ui/src/Tooltip/Tooltip.js
@@ -280,13 +280,15 @@ const Tooltip = React.forwardRef(function Tooltip(props, ref) {
     }
   };

-  const handleEnter = event => {
+  const handleEnter = (forward = true) => event => {
     const childrenProps = children.props;

     if (
       event.type === 'mouseover' &&
       childrenProps.onMouseOver &&
-      event.currentTarget === childNode
+      forward
     ) {
       childrenProps.onMouseOver(event);
     }
@@ -326,7 +328,7 @@ const Tooltip = React.forwardRef(function Tooltip(props, ref) {
     }
   };

-  const handleFocus = event => {
+  const handleFocus = (forward = true) => event => {
     // Workaround for https://github.com/facebook/react/issues/7769
     // The autoFocus of React might trigger the event before the componentDidMount.
     // We need to account for this eventuality.
@@ -336,11 +338,11 @@ const Tooltip = React.forwardRef(function Tooltip(props, ref) {

     if (isFocusVisible(event)) {
       setChildIsFocusVisible(true);
-      handleEnter(event);
+      handleEnter()(event);
     }

     const childrenProps = children.props;
-    if (childrenProps.onFocus && event.currentTarget === childNode) {
+    if (childrenProps.onFocus && forward) {
       childrenProps.onFocus(event);
     }
   };
@@ -362,11 +364,11 @@ const Tooltip = React.forwardRef(function Tooltip(props, ref) {
     }, theme.transitions.duration.shortest);
   };

-  const handleLeave = event => {
+  const handleLeave = (forward = true) => event => {
     const childrenProps = children.props;

     if (event.type === 'blur') {
-      if (childrenProps.onBlur && event.currentTarget === childNode) {
+      if (childrenProps.onBlur && forward) {
         childrenProps.onBlur(event);
       }
       handleBlur(event);
@@ -401,7 +403,7 @@ const Tooltip = React.forwardRef(function Tooltip(props, ref) {
     clearTimeout(touchTimer.current);
     event.persist();
     touchTimer.current = setTimeout(() => {
-      handleEnter(event);
+      handleEnter()(event);
     }, enterTouchDelay);
   };

@@ -449,29 +451,32 @@ const Tooltip = React.forwardRef(function Tooltip(props, ref) {
     className: clsx(other.className, children.props.className),
   };

+  const interactiveWrapperListeners = {};
+
   if (!disableTouchListener) {
     childrenProps.onTouchStart = handleTouchStart;
     childrenProps.onTouchEnd = handleTouchEnd;
   }

   if (!disableHoverListener) {
-    childrenProps.onMouseOver = handleEnter;
-    childrenProps.onMouseLeave = handleLeave;
+    childrenProps.onMouseOver = handleEnter();
+    childrenProps.onMouseLeave = handleLeave();
+
+    if (interactive) {
+      interactiveWrapperListeners.onMouseOver = handleEnter(false);
+      interactiveWrapperListeners.onMouseLeave = handleLeave(false);
+    }
   }

   if (!disableFocusListener) {
-    childrenProps.onFocus = handleFocus;
-    childrenProps.onBlur = handleLeave;
-  }
+    childrenProps.onFocus = handleFocus();
+    childrenProps.onBlur = handleLeave();

-  const interactiveWrapperListeners = interactive
-    ? {
-        onMouseOver: childrenProps.onMouseOver,
-        onMouseLeave: childrenProps.onMouseLeave,
-        onFocus: childrenProps.onFocus,
-        onBlur: childrenProps.onBlur,
-      }
-    : {};
+    if (interactive) {
+      interactiveWrapperListeners.onFocus = handleFocus(false);
+      interactiveWrapperListeners.onBlur = handleLeave(false);
+    }
+  }

   if (process.env.NODE_ENV !== 'production') {
     if (children.props.title) {

[eps1lon]

The following test as well as the existing one for #18679 should pass:

// https://github.com/mui-org/material-ui/issues/19883
it('should not prevent event handlers of children', () => {
  const handleFocus = spy(event => event.currentTarget);
  // Tooltip should not assume that event handlers of children are attached to the
  // outermost host
  const TextField = React.forwardRef(function TextField(props, ref) {
    return (
      <div ref={ref}>
        <input type="text" {...props} />
      </div>
    );
  });
  const { getByRole } = render(
    <Tooltip interactive open title="test">
      <TextField onFocus={handleFocus} />
    </Tooltip>,
  );
  const input = getByRole('textbox');

  input.focus();

  // return value is event.currentTarget
  expect(handleFocus.callCount).to.equal(1);
  expect(handleFocus.returned(input)).to.equal(true);
});

[oliviertassinari]

All 💚

39 passing (2s)

[oliviertassinari]

@kidokidozh Do you want to work on a pull request? :)

[sudoStatus200]

@oliviertassinari Hi, I would like to work on this.

[sudoStatus200]

@oliviertassinari There is one thing I noticed.
If you add disableFocusListener= {true} prop to tooltip then onFocus event works.
Like this:

 <Tooltip title="Some hint" disableFocusListener={true} > 
        <TextField
          onFocus={() => {
            console.log("This  works  fine");
          }}
        />
      </Tooltip>

and when disableFocusListener={false} then onFocus is not working.

But I think when diableFocusListener is true then onFocus should not work, So it means this prop also not working as it should be.
Can you just verify is this also a bug or not? I Will try to fix both of these issues in the same PR.

[eps1lon]

@sudoStatus200 I think we should look at disableFocusListener after the proposed changes. Might be that disableFocusListener just affects the symptoms but doesn't have to do anything with the cause of the issue.

[ShehryarShoukat96]

@sudoStatus200 are you still working on this issue?

[netochaves]

Can I take this?

[oliviertassinari]

@netochaves "good first issues" are meant to ramp up new contributors, given you have already done a none negligible number of contributions, I think that it would be better to focus on harder issues. Thanks

[ShehryarShoukat96]

@oliviertassinari Can I work on this issue?

[oliviertassinari]

@ShehryarShoukat96 Sure :)