Material-ui: disabled buttons have pointer-events: none which prevent overriding of cursor property

[andyford]

• [x] This is not a v0.x issue.
• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

adding cursor: not-allowed; (or other cursor types) to a the CSS of a disabled button should work for authors

Current Behavior 😯

CSS cursor property cannot be set because material-ui library sets pointer-events: none; on disabled buttons which has the side effect of not allowing the cursor property to be set

[eps1lon]

I'd hope the browser would make disabled or aria-disabled not clickable but that is unfortunately not the case. If you would remove the pointer-events: none then the button would be clickable which isn't quite right either.

We could intercept the click event and stop propagation if the button is disabled. Not sure how touch devices will behave in that case.

You can always unset the pointer-events style for your specific use case.

[oliviertassinari]

@andyford What do you think of this change?

--- a/packages/material-ui/src/ButtonBase/ButtonBase.js
+++ b/packages/material-ui/src/ButtonBase/ButtonBase.js
@@ -38,9 +38,11 @@ export const styles = {
       borderStyle: 'none', // Remove Firefox dotted outline.
     },
     '&$disabled': {
-      pointerEvents: 'none', // Disable link interactions
       cursor: 'default',
     },
+    'a&$disabled': {
+      pointerEvents: 'none', // Disable link interactions
+    },
   },
   /* Styles applied to the root element if `disabled={true}`. */
   disabled: {},

Do you want to work on it?

[eps1lon]

@oliviertassinari Wouldn't that still make any button not rendered as button clickable?

[oliviertassinari]

@eps1lon Correct, maybe then?

--- a/packages/material-ui/src/ButtonBase/ButtonBase.js
+++ b/packages/material-ui/src/ButtonBase/ButtonBase.js
@@ -38,9 +38,12 @@ export const styles = {
       borderStyle: 'none', // Remove Firefox dotted outline.
     },
     '&$disabled': {
-      pointerEvents: 'none', // Disable link interactions
+      pointerEvents: 'none', // Disable interactions
       cursor: 'default',
     },
+    'button&$disabled': {
+      pointerEvents: 'auto',
+    },
   },
   /* Styles applied to the root element if `disabled={true}`. */
   disabled: {},

[eps1lon]

I don't know to be honest. Every solution has some other flaws for some use cases.

This solution would still not allow cursor styling for a tags. However it seems like the best compromise since we allow styling for the default use case. Other use cases should set this explicitly knowing that the element will still be clickable.

[oliviertassinari]

Alternatively, we can proxy the click events, but I think that it will require too much bundle size vs how often people will need it.

[andyford]

I see this is a bit tricky. And making changes to this could cause surprises. So maybe it's enough to keep it as-is and add a note in the docs (this would have saved me a lot of time!).

Perhaps warn people in the docs that pointer-events: none; is set by default which prevents making changes to the cursor property so if authors want to change cursor, they can do so by manually adding pointer-events: auto but then they'll have to somehow implement any click prevention themselves

[oliviertassinari]

What do you think of this change?

--- a/packages/material-ui/src/ButtonBase/ButtonBase.js
+++ b/packages/material-ui/src/ButtonBase/ButtonBase.js
@@ -38,7 +38,6 @@ export const styles = {
       borderStyle: 'none', // Remove Firefox dotted outline.
     },
     '&$disabled': {
-      pointerEvents: 'none', // Disable link interactions
       cursor: 'default',
     },
   },
@@ -69,6 +68,12 @@ class ButtonBase extends React.Component {
     }
   });

+  handleClick = event => {
+    if (this.props.onClick && !this.props.disabled) {
+      this.props.onClick(event);
+    }
+  };
+
   handleMouseUp = createRippleHandler(this, 'MouseUp', 'stop');

   handleMouseLeave = createRippleHandler(this, 'MouseLeave', 'stop', event => {
@@ -241,6 +246,7 @@ class ButtonBase extends React.Component {
       focusRipple,
       focusVisibleClassName,
       onBlur,
+      onClick,
       onFocus,
       onFocusVisible,
       onKeyDown,
@@ -291,6 +297,7 @@ class ButtonBase extends React.Component {
         onMouseDown={this.handleMouseDown}
         onMouseLeave={this.handleMouseLeave}
         onMouseUp={this.handleMouseUp}
+        onClick={this.handleClick}
         onTouchEnd={this.handleTouchEnd}
         onTouchMove={this.handleTouchMove}
         onTouchStart={this.handleTouchStart}

[eps1lon]

@oliviertassinari I think without pointer events we end up polyfilling browser behavior for button[disabled]. Would your approach cover touch behavior and other click devices?

The suggestion from @andyford seems the most reasonable. Maybe add a demo and warn that allowing pointer events can make non button elements that are disabled clickable.

[oliviertassinari]

@eps1lon I can't think of any wrong side effect.

This solution would still not allow cursor styling for a tags.

Same limitation with ant: https://codesandbox.io/s/8yl2zv22rj

[eps1lon]

I mean we can try it. I'm not familiar with touch behavior so I don't how enabling pointer-events interacts with touch devices. Would need to checkout a deploy preview.

[rob2d]

AFAIK, enabling pointer events/disabling works the same with touch behavior as with mouse events.

Another problem with this issue is you can't add an informational Tooltip -- which in some use cases isn't very great (I know Material-UI is minimalist but sometimes giving reasons for things e.g. authentication not provided or non elevated permissions in an app can be night and day for UX).

At the moment I need to replace MenuItem with ListItem and do what I need accordingly for my specific use-case (unfortunately a nasty side effect of keyboard and other things go a bit inconsistent).

Granted, there is a workaround as mentioned, but it seems like something that should be considered.

[mark-tate]

Hi.. I'll give this one a go, it's a small fix.
See what you think

[oliviertassinari]

Another problem with this issue is you can't add an informational Tooltip -- which in some use cases isn't very great

@rob2d It's documented in https://material-ui.com/components/tooltips/#disabled-elements.

[oliviertassinari]

I have added a new commit in #17778 to keep the current implementation and focus on documenting the limitation. Feedback welcome.

[BenLorantfy]

What's wrong with https://github.com/mui-org/material-ui/issues/14455#issuecomment-462163189 ? I think the bigger issue is not being able to wrap disabled buttons with tooltips. IMO it's a good user experience to tell the user why a button is disabled.

Just updating the docs isn't the best solution to this IMO

[oliviertassinari]

@BenLorantfy We need pointerEvents: none to get it to work for disabled buttons: https://material-ui.com/components/tooltips/#disabled-elements (Firefox related).

[BenLorantfy]

Yeah never-mind I didn't realize the browser behaviour going on here. I understand why it's needed now 👍