mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
94.05k stars 32.32k forks source link

Tooltip doesn't work for <IconButton disabled> #8416

Open korywka opened 7 years ago

korywka commented 7 years ago

Expected Behavior

Tooltip is visible for hover

Current Behavior

Tooltip is hidden for hover

Steps to Reproduce (for bugs)

<Tooltip title="Tooltip" placement="bottom">
  <IconButton disabled>
  <Done/>
  </IconButton>
</Tooltip>

Your Environment

Tech Version
Material-UI v1.0.0-beta.12
oliviertassinari commented 7 years ago

Disabled elements do not fire events. You can however place a DIV over top of the element and listen to the event fired on that element instead.

https://stackoverflow.com/questions/18113937/fire-onmouseover-event-when-element-is-disabled

Implementing this suggestion looks like this, and it work.

        <Tooltip title="Tooltip" placement="bottom">
          <div>
            <IconButton disabled>
              <Done />
            </IconButton>
          </div>
        </Tooltip>
oliviertassinari commented 7 years ago

I was also thinking of using the component property but it doesn't work because of the pointer-events: none; style:

<Tooltip title="Tooltip" placement="bottom">
  <IconButton component="div" disabled>
    <Done />
  </IconButton>
</Tooltip>
korywka commented 7 years ago

@oliviertassinari Sorry, didn't know. Thanks.

oliviertassinari commented 7 years ago

@bravecow I think that we can add a warning if more people raise this issue.

dskoda1 commented 7 years ago

So is the accepted solution to this going to be putting a div between buttons and tooltips? Having a tooltip is typically the most helpful on disabled buttons, to indicate why the button is disabled.

What if we added a prop to the Tooltip component that signified it to appear even when the child is disabled? It would just implement this solution under the hood but at least the user wouldn't be left wondering why tooltips don't work on disabled buttons.

mririgoyen commented 6 years ago

How do you turn off the tooltip warning?

oliviertassinari commented 6 years ago

How do you turn off the tooltip warning?

@goyney https://github.com/mui-org/material-ui/issues/8416#issuecomment-332556082

mririgoyen commented 6 years ago

@oliviertassinari How do I turn off the tooltip warning without cluttering up the DOM?

I have several toolbars of buttons that are disabled when content is loading. Each button has a tooltip on it. Once the document loads, they all enable. Having to wrap every. single. button. in a span to suppress this warning is obnoxious.

oliviertassinari commented 6 years ago

@goyney What about conditionally rendering a Tooltip when needed? As far as I understand it, you don't want to display any tooltip when the button is disabled.

mririgoyen commented 6 years ago

I always want to display a tooltip. How about a suppressWarnings prop or something on the tooltip.

oliviertassinari commented 6 years ago

I always want to display a tooltip

@goyney Even when the button is disabled?

mririgoyen commented 6 years ago

Yes, that is what I said. lol

dskoda1 commented 6 years ago

Like I mentioned above, tooltips are of extra use to users when a button is disabled @oliviertassinari, in order to indicate to them why a button is disabled.

mririgoyen commented 6 years ago

@dskoda1 I have opened #11601.

man-u-13 commented 4 years ago

Disabled elements do not fire events. You can however place a DIV over top of the element and listen to the event fired on that element instead.

https://stackoverflow.com/questions/18113937/fire-onmouseover-event-when-element-is-disabled

Implementing this suggestion looks like this, and it work.

        <Tooltip title="Tooltip" placement="bottom">
          <div>
            <IconButton disabled>
              <Done />
            </IconButton>
          </div>
        </Tooltip>

This helps in showing the tooltip on the disabled button but the button which is enclosed in the 'div' loses its styling for me. What am I missing here?

BuangHosen commented 3 years ago

@man-u-13 try to replace div tag with React.Fragment or empty tag

piranna commented 3 years ago

@man-u-13 try to replace div tag with React.Fragment or empty tag

Thank you @BuangHosen, using React.Fragment works! :-D

Jazzmanpw commented 2 years ago

try to replace div tag with React.Fragment or empty tag

@BuangHosen but React.Fragment breaks the tooltip, because there's no element to get ref for

Floriferous commented 2 years ago

This is a really painful thing for all mui components. Adding a tooltip on a disabled element is such an essential UX detail (you should always explain why something is disabled, your user won't know, and you the developer will forget all the reasons), that every time I write this I'm a bit frustrated.

// API I want

<IconButton tooltip="Snooze a task" />;
<IconButton tooltip="You can't snooze an important task" disabled />;

Adding a wrapper like this is a terrible solution:

const MyCustomIconButton = ({ disabled, tooltip, ...props}) => {
  let iconButton = <IconButton {...props} />;

  if (tooltip) {
    if (disabled) {
      iconButton = <div>{iconButton}</div>;
    }

    return <Tooltip title={tooltip}>{iconButton}</Tooltip>;
  }

  return iconButton
}
bytasv commented 1 year ago

@mui/core would you consider taking another look at this issue? Maybe we can find a way to improve DX without forcing developers to manually add wrapping elements?

I.e. what if Tooltip component accepted the prop which would take care of adding a wrapper under the hood (one of the suggesteds options above)?

<Tooltip resolveDisabled>

I'm also wondering why is reported with error level and not as a warning? IMO this is not exactly correct as there can be a case when this does not break anything, say button which has disabled prop, but still have pointer-events: all. I think such case should be reported as warning instead.

CleanShot 2023-01-07 at 12 06 45@2x

In addition, I think it would be nice to have the option to disable these warnings/errors globally/per-instance, maybe <Tooltip suppressWarnings> that could also be added as a global prop default 🤔

oliviertassinari commented 1 year ago

@bytasv Do you have more context on the problem you faced?


i.e. what if Tooltip component accepted the prop which would take care of adding a wrapper under the hood (one of the suggesteds options above)?

This idea was suggested and discussed a bit in https://github.com/mui/material-ui/issues/11601#issue-326783141. The arguments there are probably still up-to-date.

I'm also wondering why is reported with error level and not as a warning?

I don't understand, the cases consider seem to be broken too. For example, what's the link with pointer-events: all? This is an SVG element only property.

In addition, I think it would be nice to have the option to disable these warnings/errors globally/per-instance, maybe <Tooltip suppressWarnings> that could also be added as a global prop default 🤔

Which use case requires to suppress the warning?

bytasv commented 1 year ago

Here is codesandbox example, within Tooltip I have button which is disabled. At the same time I have Icon inside that button. When I hover over the button, even though it's disabled I can still see tooltip (that's expected). If you look at console MUI still throws error (not even warning) that Tooltip is wrapping disabled button.

CleanShot 2023-01-08 at 18 28 38@2x
  1. From my perspective all works fine as I want, so I don't want to see error/warning at all. That's why IMO having way to suppress such warnings would be a good thing too
  2. Even if tooltip is not triggering on the button itself I may choose to ignore that and i'd like to keep my console clean

https://github.com/mui/material-ui/issues/11601#issue-326783141 as for options discussed there - seems that we might be on the same page except that the discussion mentioned hasn't been resolved. Since this can't be resolved automatically (because of potentially breaking layout) it would be nice to provide "hands free" way to do the wrapping or any other way that would allow to show tooltip even on the disabled items.

Anyways if we choose not to handle disabled cases via prop, then I think we should at least address error/warning part:

  1. Minimum use warning level and not error?
  2. Ideally allow suppressing such messages via configuration
oliviertassinari commented 1 year ago

@bytasv I think that we should label https://codesandbox.io/s/tender-meadow-chtejn?file=/demo.tsx as an invalid case. It doesn't show the tooltip correctly on hover, the console error looks correct to me.

Jan-08-2023 18-12-15

People can do, which seems to behave correctly:

import * as React from "react";
import Tooltip from "@mui/material/Tooltip";
import DeleteIcon from "@mui/icons-material/Delete";

export default function BasicTooltip() {
  return (
    <button type="button" disabled style={{ pointerEvents: "all" }}>
      <Tooltip title="Delete">
        <DeleteIcon />
      </Tooltip>
    </button>
  );
}

https://codesandbox.io/s/competent-dew-if6uec?file=/demo.tsx

bytasv commented 1 year ago

IMO we can hint users of what could be potentially breaking but we should still let them choose to ignore if they are fine to take the risks. The example that I've shared with a current Tooltip implementation might be more than enough for some POC type implementations and even if it doesn't show tooltip on a button itself as long as it appears when hovering on a child I'm fine with that and I'd like to be able to filter out framework thrown errors/warnings. Alternatively maybe someone is actually fine by not showing tooltip when button is disabled at all but we don't provide a way to opt-out from these errors/warnings without having them modify how they render things.

As a developer I'd at least want to have an option to say "Ok MUI, I've heard you, but I choose to ignore this, please don't bother me about it again". Currently there is no way to do that without rewriting how each tooltip is used (which is not the best DX IMO). That could be:

  1. Prop or global configuration to suppress warnings
  2. Using warning level instead of error (so at least I can switch these messages in dev tools)

Still the best (for me) would be if I could use some prop like resolveDisabled to add the wrapper automatically without causing errors

rpgroot commented 1 year ago

Place the button inside the span tag.

<Tooltip title="You don't have permission to do this">
  <span>
    <Button disabled>A Disabled Button</Button>
  </span>
</Tooltip>
oliviertassinari commented 1 year ago

than enough for some POC type implementations

@bytasv I think people ignore warnings & errors in the console in the POC mode, I personally more or less do.

Using warning level instead of error (so at least I can switch these messages in dev tools)

We define the warning vs. error policy for the docs callouts https://www.notion.so/mui-org/Callouts-and-their-usage-in-the-docs-7e709a02c72142cd8b1a0829861435c5#5e769cdb5102440ab4bb7bbd16e43ada. Error is for when your setup is wrong, either it will either fail today or tomorrow.

For the console message in the code, I had this RFC https://github.com/mui/material-ui/issues/21979 that goes in the direction you are suggesting. Why not (A.).

maybe someone is actually fine by not showing tooltip when button is disabled at all but we don't provide a way to opt-out from these errors/warnings without having them modify how they render things.

Developers can explicitly hide the tooltip when disabled, which doesn't show the console message.

<Tooltip title="Delete" open={false}>

https://codesandbox.io/s/spring-dew-0vg8m7?file=/demo.tsx:187-224

It sounds like we could improve the error message to make this more obvious. Per https://www.notion.so/mui-org/Technical-writing-7e55b517ac2e489a9ddb6d0f6dd765de#85219822c3194b6f8a49ee08ea82b90a, we could give this alternative way out in the error message (B.).

diff --git a/packages/mui-material/src/Tooltip/Tooltip.js b/packages/mui-material/src/Tooltip/Tooltip.js
index a9976a632f..d53ed8093a 100644
--- a/packages/mui-material/src/Tooltip/Tooltip.js
+++ b/packages/mui-material/src/Tooltip/Tooltip.js
@@ -301,10 +301,10 @@ const Tooltip = React.forwardRef(function Tooltip(inProps, ref) {
         console.error(
           [
             'MUI: You are providing a disabled `button` child to the Tooltip component.',
-            'A disabled element does not fire events.',
-            "Tooltip needs to listen to the child element's events to display the title.",
+            'A disabled button does not fire mouse events.',
+            "But the Tooltip needs to listen to the child element's events to display the title.",
             '',
-            'Add a simple wrapper element, such as a `span`.',
+            'To solve the issue, you can add a wrapper element (e.g. a `span`) around the child or hide the tooltip with `open={false}`.',
           ].join('\n'),
         );
       }

I'm reopening, I think that we could apply suggestions A. and B.

eps1lon commented 1 year ago

The proper solution for this is to land https://github.com/mui/material-ui/pull/27719

bytasv commented 1 year ago

@bytasv I think people ignore warnings & errors in the console in the POC mode, I personally more or less do.

IMO that's a bold assumption - i.e. I do ignore SOME warnings/errors which I consider not important, but I still want to see important info without the noise. Having Tooltip complain about disabled is not important to me and I'd like to be able to not see that without having to change the code or disabled ALL errors entirely

We define the warning vs. error policy for the docs callouts https://www.notion.so/mui-org/Callouts-and-their-usage-in-the-docs-7e709a02c72142cd8b1a0829861435c5#5e769cdb5102440ab4bb7bbd16e43ada. Error is for when your setup is wrong, either it will either fail today or tomorrow.

Let's look at these two levels - "something will fail today or tomorrow" 👇 vs "a situation that could cause unexpected behaviour"

These two lights are basically error vs warning level console log messages - when I'm using Tooltip with disabled element, I get error which tells me that "this will fail today or tomorrow" - but in reality it does not, the only thing that fails is that tooltip message won't be shown when action is disabled anyways, so while it's not probably the EXPECTED behaviour, I wouldn't call it failure.

Now considering those check engine lights examples I'd say the analogy could be if the car threw you RED check engine light as soon as one of the light bulbs wasn't working. We could also argue that it is important because during the night if you loose the second bulb you might need to drive in total darkness. IMO it's important to differentiate what's consider a "functional failure" which doesn't allow us to carry on the work vs what is probably not good but allows us to carry on

Developers can explicitly hide the tooltip when disabled, which doesn't show the console message.

I'm not arguing that there is no way to hide these warnings (one can use one of previously mentioned workarounds), what I as developer expect from a library is to be able to tell it "that I know better what I'm doing, thank you for warning me, I will take it from here" - pretty much in a similar fashion as we instruct OS to "not show me these popup warnings again", well except that this could probably be done using some config flag.

gpspake commented 1 year ago

I don't see this mentioned anywhere but another problem with wrapping the disabled button in a span is it breaks ButtonGroup. In our case, we want to disable buttons in a button group and provide tooltips about why they're disabled.

image image
vishal-kadmos commented 1 year ago

ah 🤯 why it is such a pain to fix simple thing. Though error is self explanatory, not finding proper solution. I am using Mui Tooltip with Mui X Datagrid (paid version)

Original Error/warning

Screenshot 2023-04-28 at 4 00 00 PM

rning: React does not recognize the `touchRippleRef` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `touchrippleref` instead. If you accidentally passed it from a parent component, remove it from the DOM element.

Screenshot 2023-04-28 at 3 46 47 PM Screenshot 2023-04-28 at 3 46 27 PM

Following is the sample code

getActions: (params: GridRenderCellParams) => {
          return [
            <Tooltip title="Title 1" key={`edit_${params.id}`}>
              <GridActionsCellItem
                key={`edit_${params.id}`}
                icon={<EditIcon />}
                label={"Label 1"}
                disabled={true}
                onClick={handleSomething}
                showInMenu={false}
              />
            </Tooltip>,
            <Tooltip title={"title 2"} key={`action_${params.id}`}>
              <GridActionsCellItem
                key={`action_${params.id}`}
                icon={
                  <Switch
                    checked={checked}
                    disableRipple
                  />
                }
                onClick={handleSomething2}
                label={"Label 2"}
                disabled={true}
                disableRipple
                showInMenu={false}
              />
            </Tooltip>,
          ];
        },

@oliviertassinari any help. thanks

ghost commented 1 year ago

Is there any way to suppress the You are providing a disabled 'button' child to the Tooltip component message altogether? In my case, when I have a disabled IconButton/Button, I do not want the tooltip showing.

auauwolff commented 1 year ago

vishal-kadmos

I ran into the same issue...

Solution that i found is to add a sx style to the GridActionCellItem component like so:

<GridActionsCellItem
      disabled={params.row.actions.delete}
      color="secondary"
      onClick={() => onDelete(params)}
      sx={{
        '&.Mui-disabled': {
          pointerEvents: 'all',
        },
      }}
      icon={
        <Tooltip
          title={
            params.row.actions.delete
              ?  cannot delete
              :  can delete
          }
        >
          <Delete />
        </Tooltip>
      }
    />
ttduongtran commented 1 year ago

I had the same issue.

I tried to wrap it into component, and it works as well

My code:

<Tooltip title="move up" placement="right">
      <Box>
        <IconButton
          onClick={() => handleMoveUp(idx)}
          disabled={idx === 0 ? true : false}
        >
          <Iconify icon={'ion:caret-up'} color={idx === 0 ? '#EEEEEE' : '#25314C'} />
        </IconButton>
      </Box>
    </Tooltip>
AndrewEastwood commented 3 months ago

how about this?

<Button
  // suppress the MUI warning about using a disabled button with tooltip
  component={!evaluationResponseId || pdfActionStatus.isRunning ? 'span' : 'button'}
...