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] Fix children mouse over detection #32321

Merged
merged 4 commits into from
Jul 11, 2022

Conversation

ivan-ngchakming
Copy link
Contributor

@ivan-ngchakming ivan-ngchakming commented Apr 16, 2022

I believe this bug is also caused by facebook/react#7769, where Ref effects are called after Update, therefore childNode (set by ref passed to child) is undefined. The issue is fixed for onFocus events in #14496, this PR applies the same fix for onMouseOver events.

fixes #31198

@mui-bot
Copy link

mui-bot commented Apr 16, 2022

Details of bundle changes

Generated by 🚫 dangerJS against 18ffb5f

@ivan-ngchakming ivan-ngchakming changed the title fix: not open on hover [Tooltip] Fix children mouse over detection Apr 16, 2022
@danilo-leal danilo-leal added the component: tooltip This is the name of the generic UI component, not the React module! label Apr 18, 2022
@mnajdova mnajdova requested a review from michaldudak May 18, 2022 16:46
Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be working well. Thanks, @ivan-ngchakming!

@oliviertassinari
Copy link
Member

Do we miss a test case for this bug fix?

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label May 23, 2022
@michaldudak
Copy link
Member

@ivan-ngchakming would you mind adding a test?

@michaldudak michaldudak added the PR: needs test The pull request can't be merged label Jun 8, 2022
@hbjORbj hbjORbj removed the PR: needs test The pull request can't be merged label Jul 11, 2022
@hbjORbj hbjORbj force-pushed the tooltip/fix-not-open-on-hover branch from 3a7af87 to 18ffb5f Compare July 11, 2022 09:00
@hbjORbj
Copy link
Member

hbjORbj commented Jul 11, 2022

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 11, 2022

I wonder if we don't have two problems with these changes, It's not clear to me that it's a step forward (might be better without):

  1. on the surface, the test added is not failing on master, it's identical to not having a test case.
  2. the bug reproduction in [Tooltip] Tooltip (as slider ValueLabel) won't show up until I click the slider thumb #31198 (comment) seems to surface that the root problem is not with the Tooltip:

Screenshot 2022-07-11 at 15 10 43

So what I would propose is to:

  1. We close [Tooltip] Tooltip (as slider ValueLabel) won't show up until I click the slider thumb #31198 as a question with https://codesandbox.io/s/customizedslider-demo-material-ui-forked-yn4d9n?file=/demo.tsx:1276-1296 as the solution. AirbnbThumbComponent should forward the ref.
 interface AirbnbThumbComponentProps extends React.HTMLAttributes<unknown> {}

-function AirbnbThumbComponent(props: AirbnbThumbComponentProps) {
-  const { children, ...other } = props;
-  return (
-    <SliderThumb {...other}>
-      {children}
-      <span className="airbnb-bar" />
-      <span className="airbnb-bar" />
-      <span className="airbnb-bar" />
-    </SliderThumb>
-  );
-}
+const AirbnbThumbComponent = React.forwardRef(
+  (props: AirbnbThumbComponentProps, ref) => {
+    const { children, ...other } = props;
+    return (
+      <SliderThumb {...other} ref={ref}>
+        {children}
+        <span className="airbnb-bar" />
+        <span className="airbnb-bar" />
+        <span className="airbnb-bar" />
+      </SliderThumb>
+    );
+  }
+);

 export default function CustomizedSlider() {
   return (
  1. We revert this PR, under the argument that it's not helping since the root cause is userland and already has a relatively clear warning.

The alternative is to go a bit above and beyond:

  1. We explicitly add the support for tooltip children that don't forward the ref, as a fallback mechanism. But in this case, this PR is not a "bug" fix, it's a "new feature", the PR label would need to change. We also need a follow-up to replace the added test case to cover the use case.
  2. We update the demos in the docs to forward the ref.

What do you think about it? cc @hbjORbj @michaldudak (as co-owners of the tooltip)

oliviertassinari added a commit that referenced this pull request Jul 20, 2022
This reverts commit dc0e387bcdd6b4654f5e7fb472e6fba18c582deet .

See #32321 (comment) for why.
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 20, 2022

I have reverted this PR and moved the conversation to the issue: #31198 (comment).

daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
This reverts commit dc0e387bcdd6b4654f5e7fb472e6fba18c582deet .

See mui#32321 (comment) for why.
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tooltip] Tooltip (as slider ValueLabel) won't show up until I click the slider thumb
7 participants