[Button] Don't apply hover styles when disabled

[eps1lon]

The goal is to eventually enable tooltips on disabled buttons with

<Tooltip title="why it is disabled">
  <Button disabled style={{ pointerEvents: "all" }}>
    disabled button
  </Button>
</Tooltip>

In case it isn't clear: Right now this only enables Button disabled style={{ pointerEvents: "all" }} /> to not trigger hover styles on hover. There's a reason pointer-events: none is not platform standard: To enable tooltips on disabled buttons. Right now we prohibit this and it's a major customization hazard with Buttons and a common pitfall (which is why we have a dedicated docs section).
With pointer-events enabled we can leverage PointerEvents such as pointerover and pointerout.

Double clarification: Check out https://codesandbox.io/s/textbuttons-material-demo-forked-wz3dz

https://deploy-preview-26412--material-ui.netlify.app/components/buttons/#main-content

[eps1lon]

Adding for future reference. I'm inclined to special case aria-disabled to act as "disabled but focuseable" for a11y: https://css-tricks.com/making-disabled-buttons-more-inclusive/

[mui-pr-bot]

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against 8e0eb6d

[eps1lon]

@mnajdova Any potentially undesired side-effects this might have? Or is this intended to work this way?

[mnajdova]

Looks good, I would maybe write it like this:

...(!styleProps.disabled && {
  '&:hover': {...}
}),

So that we don't end up with '&:hover': false, but even that should not create any issues I think.

[eps1lon]

Personally, I'm having a hard time parsing the code you wrote. And it's creating an additional object.

So that we don't end up with '&:hover': false, but even that should not create any issues I think.

Yeah, let's try this out then.

[mnajdova]

This is prone to mistakes. For example, we also have '&:hover' on line 229 https://github.com/mui-org/material-ui/blob/8e0eb6d678243e6ffc73145ba3f8a1d30b78bc57/packages/material-ui/src/Button/Button.js#L229

In addition, what other components should incorporate this logic?

[oliviertassinari]

@eps1lon I'm confused by the changes:

1. What problem do the changes fix? I mean, how does the change fit into the targetted goal? I have tried the intended structure but it's not displaying the tooltip:

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

export default function MyApp() {
  return (
    <Tooltip title="You don't have permission to do this">
      <button type="button" disabled style={{ pointerEvents: "all" }}>
        A disabled button
      </button>
    </Tooltip>
  );
}

So far, we depend on this approach: https://getbootstrap.com/docs/5.0/components/tooltips/#disabled-elements.

2. The changes are on the Button component but the label of the PR in the title is ButtonBase. Should we rename the title? Or maybe it means that we need to update all the button components to follow this pattern?

[oliviertassinari]

What problem do the changes fix?

My bad. I got it for 1. It works with the <Button> because there is a nested <span> inside the <button>. I see two downsides with the proposed approach:

1. I have tried the <button> first over <Button> because I assume that having our tooltip work with the developer's custom button is important, especially for an unstyled tooltip. We talked about removing the intermediary span in our <Button>, if we do, it breaks this solution.
2. This approach forces all the customizations to remove the hover style when the disabled prop is set. Will the developers learn/appreciate the DX/do it?

It seems to be a tradeoff, and we need to understand the other side of the story. What's the main downside with the current approach?

[eps1lon]

What's the main downside with the current approach?

It cannot allow tooltips on disabled buttons.

Please be aware that I did not claim it already works. If that would be the case I would've included tests for tooltips, documentation updates etc. The goal is to allow tooltips on disabled buttons. Not that this change enables tooltips on disabled buttons.

Right now it only works on certain platforms. For robust support we need work in the Tooltip.

[oliviertassinari]

I have rebased on HEAD to take edfaadc in.

[oliviertassinari]

I have created two sandboxes with:

import * as React from "react";
import Button from "@material-ui/core/Button";
import Tooltip from "@material-ui/core/Tooltip";

export default function BasicTooltip() {
  return (
    <Tooltip title="why it is disabled">
      <Button disabled style={{ pointerEvents: "all" }}>
        disabled button
      </Button>
    </Tooltip>
  );
}

The commit just before #26412 (comment): https://codesandbox.io/s/basicbuttons-material-demo-forked-4oi9k OK

The commit just after #26412 (comment): https://codesandbox.io/s/basicbuttons-material-demo-forked-nyw7j KO

[oliviertassinari]

I'm closing per #26412 (comment). We could restart it later on with a different strategy.

[eps1lon]

I'm closing per #26412 (comment). We could restart it later on with a different strategy.

I think you don't understand the path forward. I'm not sure in how many other ways I can explain to you that this doesn't work yet. This change is a requirement to unlock Tooltips on disabled elements. This PR can stand on its own since it integrates with other tooltip implementations.

[oliviertassinari]

I think that we miss the description/demonstration of a problem the changes are solving. #26412 (comment) demonstrates that the case we are considering in the issue's description (what I could understand of if) is not working.

I would propose we details, with a demo, the problem these changes are solving.

I would also argue that we miss the pros vs. the cons of the solution. For instance, does it mean we need to update all the :hover logic of the core components so they can be used in our Tooltip?

[oliviertassinari]

Edit @eps1lon: Warning! The following statement is misleading. Please verify the claims before believing what is said.

Thoughts:

1. The test case feels fabricated. I can't find a case that demonstrates how the test cases/issue fits into solving a real-life problem of a developer. We talk about the potential to fit into the tooltip, I have provided codesandbox to illustrate that it might not work. How about we widen the scope of the changes? Could we target a solution to the end goal we are after, in a single PR?
2. We take care of the Button component. We have more components that extend a <button> and that set a hover style. How about we update them too?
3. If we assume that the changes actually solve a problem, and consider the implications. Does it mean that the customizations that developers will do need to take this aspect into account? Should we document it? Does it mean we should remove https://next.material-ui.com/components/buttons/#cursor-not-allowed?

[eps1lon]

The test case feels fabricated.

I don't know which person you're representing here but this purposely being obtuse is just toxic. This behavior is not acceptable. You're actively trying to prevent allowing tooltips on disabled buttons and don't have the courage to explain why. Instead you're claiming the feature request is "fabricated". Why you never use that terminology in other feature request? Who knows. Probably people who pay you.

[eps1lon]

For anybody else reading: Olivier's responses are misleading. The behavior this PR allows has been outlined sufficiently: Allowing tooltips on disabled buttons.

[oliviertassinari]

@eps1lon To be clear, my previous comment was meant to help push the solution further. I don't think that we should close the PR.

I would like to acknowledge that It's only now that I saw the edit done in the description of the page after #26412 (review). My previous comment didn't take unto account that we would rely on onpointerover to make it work. I assumed it would work as-is, hence my confusion.

With this extra information at hand, I think that we could act on #26412 (review) with:

1. Use the e2e test infra to add a test case with onpointerover
2. Update all the other components, IconButton, ListItem, BreadcrumbCollapse, CardActionArea, Fab, ToggleButton
3. I think that these points are still worth exploring. Maybe we can improve the docs.

What do you think?

---

To answer on the previous:

The test case feels fabricated.

I don't know which person you're representing here but this purposely being obtuse is just toxic.

I have expressed how I felt about the test case (in the source, I didn't saw the new codesandbox in the PR's description). It's not meant to be a fact, it's a feeling. I feel the same way as in #25578 (comment)

it's unclear what actual problem it is fixing (the codesandbox in this issue is constructed i.e. missing real world use case)

I mean, do we have a GitHub issue that goes into the problem we have in HEAD? I think that it would be great to make a case on how we can improve https://next.material-ui.com/components/tooltips/#disabled-elements. I mean to explore the why it's better tradeoff.

The behavior this PR allows has been outlined sufficiently: Allowing tooltips on disabled buttons.

I think that we could use your framework of #23726 (review) that I think is great. Do the changes done in a given PR bring value on their own?

I personally think that it would be better to go to the end goal of improving the tooltip in a single PR, but maybe the other members think otherwise, in which case, happy to be overruled.