Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Tooltip] onFocus does not work on TextField if using a Tooltip with it #19883

Closed
2 tasks done
kidokidozh opened this issue Feb 28, 2020 · 13 comments · Fixed by #20252
Closed
2 tasks done

[Tooltip] onFocus does not work on TextField if using a Tooltip with it #19883

kidokidozh opened this issue Feb 28, 2020 · 13 comments · Fixed by #20252
Labels
bug 🐛 Something doesn't work component: tooltip This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@kidokidozh
Copy link

kidokidozh commented Feb 28, 2020

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

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: c98b9c4

Your Environment 🌎

Tech Version
Material-UI v4.9.4
React v16.13.0
Browser Chrome 80
@eps1lon eps1lon added bug 🐛 Something doesn't work component: tooltip This is the name of the generic UI component, not the React module! labels Feb 28, 2020
@eps1lon
Copy link
Member

eps1lon commented Feb 28, 2020

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

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 28, 2020

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
Copy link
Member

eps1lon commented Feb 28, 2020

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
Copy link
Member

All 💚

39 passing (2s)

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Feb 28, 2020
@oliviertassinari
Copy link
Member

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

@sudoStatus200
Copy link

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

@sudoStatus200
Copy link

sudoStatus200 commented Feb 29, 2020

@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
Copy link
Member

eps1lon commented Mar 1, 2020

@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.

@sheerryy
Copy link
Contributor

@sudoStatus200 are you still working on this issue?

@netochaves
Copy link
Contributor

Can I take this?

@oliviertassinari
Copy link
Member

@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

@sheerryy
Copy link
Contributor

@oliviertassinari Can I work on this issue?

@oliviertassinari
Copy link
Member

@shehryarshoukat96 Sure :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: tooltip This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants