[Tooltip] Tooltips aren't displayed on focus on TextFields

[codeheroics]

• 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 🤔

Using a Tooltip component with disableHoverListener around a TextField component should display the tooltip on focus

Current Behavior 😯

No tooltip is displayed

Steps to Reproduce 🕹

Link: https://codesandbox.io/s/ooywx0xlq9

1. Click on the TextField, nothing is displayed.

Testing anywhere the following code sample is enough to reproduce :

<Tooltip title="TestTooltip" disableHoverListener>
  <TextField label="TestTooltip" />
</Tooltip>

Context 🔦

Display hints for user entering their data.

This used to work, and, after trying a few versions, appears to be broken starting v3.1.1

Your Environment 🌎

Tech	Version
Material-UI	v3.9.2
React	16.6.3
Browser	Firefox 66b, Chromium 71

As always, thanks for the great project 🙂

[oliviertassinari]

@codeheroics Yeah, it's because we make sure the tooltip child still has the focus before opening it. We can work around the problem with:

--- a/packages/material-ui/src/Tooltip/Tooltip.js
+++ b/packages/material-ui/src/Tooltip/Tooltip.js
@@ -71,6 +71,17 @@ export const styles = theme => ({
   },
 });

+function isDescendant(parent, child) {
+  let node = child;
+  while (node != null) {
+    if (node === parent) {
+      return true;
+    }
+    node = node.parentNode;
+  }
+  return false;
+}
+
 class Tooltip extends React.Component {
   ignoreNonTouchEvents = false;

@@ -125,12 +136,14 @@ class Tooltip extends React.Component {
   };

   handleFocus = event => {
     event.persist();
     // The autoFocus of React might trigger the event before the componentDidMount.
     // We need to account for this eventuality.
     this.focusTimer = setTimeout(() => {
       // We need to make sure the focus hasn't moved since the event was triggered.
-      if (this.childrenRef === document.activeElement) {
+      if (isDescendant(this.childrenRef, document.activeElement)) {
         this.handleEnter(event);
       }
     });

@eps1lon What do you think?

[eps1lon]

Looks good to me. Could you explain why we can only trigger handleEnter after cDM? From a naive standpoint it looks like we could just remove this hole logic. And forward handleFocus to handleEnter.

[oliviertassinari]

The active element on the page can quickly move without triggering the correct focus / blur events. The settimeout is a simple hack to dodge the autofocus problem. I think that you can find the history with a git blame.

[eps1lon]

The active element on the page can quickly move without triggering the correct focus / blur events. The settimeout is a simple hack to dodge the autofocus problem. I think that you can find the history with a git blame.

Can't reproduce the original issue described in #12372 with previous mui versions. Maybe this was a bug in react? Otherwise we should apply this behavior to every component that handles focus.

I'm fairly certain this was caused by facebook/react#7769. Maybe add a warning if we receive focus before ref?

[oliviertassinari]

Note for self, isDescendant() can be replaced with the native .contains() API.

Can't reproduce the original issue described in #12372

@eps1lon Try that out:

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

const Index = () => {
  const [open, setOpen] = React.useState(false);
  return (
    <React.Fragment>
      <button onClick={() => setOpen(true)}>hello</button>
      {open ? (
        <Tooltip title="Title">
          <Input value="value" autoFocus />
        </Tooltip>
      ) : null}
    </React.Fragment>
  );
};

export default Index;

I'm fairly certain this was caused by facebook/react#7769. Maybe add a warning if we receive focus before ref?

Yes, you are right! I propose the following fix:

--- a/packages/material-ui/src/Tooltip/Tooltip.js
+++ b/packages/material-ui/src/Tooltip/Tooltip.js
@@ -125,15 +125,14 @@ class Tooltip extends React.Component {
   };

   handleFocus = event => {
-    event.persist();
+    // 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.
-    this.focusTimer = setTimeout(() => {
-      // We need to make sure the focus hasn't moved since the event was triggered.
-      if (this.childrenRef === document.activeElement) {
-        this.handleEnter(event);
-      }
-    });
+    if (!this.childrenRef) {
+      this.childrenRef = event.currentTarget;
+    }
+
+    this.handleEnter(event);

     const childrenProps = this.props.children.props;
     if (childrenProps.onFocus) {
diff --git a/packages/material-ui/src/Tooltip/Tooltip.test.js b/packages/material-ui/src/Tooltip/Tooltip.test.js
index 58a5adcd0..936d679fb 100644
--- a/packages/material-ui/src/Tooltip/Tooltip.test.js
+++ b/packages/material-ui/src/Tooltip/Tooltip.test.js
@@ -1,10 +1,12 @@
 import React from 'react';
 import { assert } from 'chai';
+import PropTypes from 'prop-types';
 import { spy, useFakeTimers } from 'sinon';
 import consoleErrorMock from 'test/utils/consoleErrorMock';
 import { createShallow, createMount, getClasses, unwrap } from '@material-ui/core/test-utils';
 import Popper from '../Popper';
 import Tooltip from './Tooltip';
+import Input from '../Input';
 import createMuiTheme from '../styles/createMuiTheme';

 function persist() {}
@@ -12,7 +14,7 @@ function persist() {}
 const TooltipNaked = unwrap(Tooltip);
 const theme = createMuiTheme();

describe('<Tooltip />', () => {
   let shallow;
   let mount;
   let classes;
@@ -170,6 +172,28 @@ describe('<Tooltip />', () => {
     it('should mount without any issue', () => {
       mount(<Tooltip {...defaultProps} open />);
     });
+
+    it('should handle autoFocus + onFocus forwarding', () => {
+      const AutoFocus = props => (
+        <div>
+          {props.open ? (
+            <Tooltip title="Title">
+              <Input value="value" autoFocus />
+            </Tooltip>
+          ) : null}
+        </div>
+      );
+      AutoFocus.propTypes = {
+        open: PropTypes.bool,
+      };
+
+      const wrapper = mount(<AutoFocus />);
+      wrapper.setProps({ open: true });
+      assert.strictEqual(wrapper.find(Popper).props().open, false);
+      clock.tick(0);
+      wrapper.update();
+      assert.strictEqual(wrapper.find(Popper).props().open, true);
+    });
   });

   describe('prop: delay', () => {

It should solve #14468, #12371 and #12853 with less code.

[oliviertassinari]

@eps1lon What do you think about it? @codeheroics Do you want to submit a pull-request? :)

[eps1lon]

Your example is working fine for me. Not sure that this issue can be reproduced deterministically.

Your fix looks fine to me though. I think we have something similar in place somewhere else. It is at least more predictable than using 0 delay timeouts.

[oliviertassinari]

@eps1lon Here you are: https://3ml4nm811.codesandbox.io/.

[eps1lon]

Interesting. 1.4.1 has issues, 1.4.0 not. If I open it in its own window and incrementally go from 1.4.0 to 1.4.2 I have no error. It will not focus until I switch tab though.

All of that is saying to me that either codesandbox is not equipped for this kind of debugging or we have triggered an edge case. Nevertheless the event.currentTarget fix may work. Still scary use the DOM imperatively before refs.

[codeheroics]

@oliviertassinari Sure! I'll test and send it right away, though I feel kind of guilty since you've done all the work and written all the code already 😅

[oliviertassinari]

@codeheroics Peer review is important :)