simplest possible solution

mui · Oct 8, 2019 · 3e054e3 · 3e054e3
1 parent d6bf76d
commit 3e054e3
Show file tree

Hide file tree

Showing 6 changed files with 65 additions and 59 deletions.
diff --git a/docs/src/pages/components/buttons/buttons.md b/docs/src/pages/components/buttons/buttons.md
@@ -134,3 +134,23 @@ Given that many of the interactive components rely on `ButtonBase`, you should b
 able to take advantage of it everywhere.
 
 Here is an [integration example with react-router](/guides/composition/#button).
+
+## Limitations
+
+### Cursor not-allowed
+
+Material-UI's ButtonBase component set `pointer-events: none;` on disabled buttons.
+This tradeoff prevents the usage of a different disabled cursor.
+
+If you wish to use `not-allowed`, you need to remove the pointer event style and to handle these two edge cases:
+
+1. You should prevent the interactions when the button is not implemented with an actual `<button>` element, for instance, a link `<a>` element. You can dodge the issue by specifically targeting the disabled state of the `<button>` element:
+
+```css
+.MuiButtonBase-root:disabled {
+  cursor: not-allowed;
+  pointer-events: auto;
+}
+```
+
+2. You should add `pointer-events: none;` back when you need to display [tooltips on disabled elements](/components/tooltips/#disabled-elements)
diff --git a/docs/src/pages/components/tooltips/DisabledTooltips.tsx b/docs/src/pages/components/tooltips/DisabledTooltips.tsx
@@ -5,9 +5,9 @@ import Tooltip from '@material-ui/core/Tooltip';
 export default function DisabledTooltips() {
   return (
     <Tooltip title="You don't have permission to do this">
-      <span>
+      <div>
         <Button disabled>A Disabled Button</Button>
-      </span>
+      </div>
     </Tooltip>
   );
 }
diff --git a/docs/src/pages/components/tooltips/tooltips.md b/docs/src/pages/components/tooltips/tooltips.md
@@ -73,19 +73,19 @@ A tooltip can be interactive. It won't close when the user hovers over the toolt
 
 ## Disabled Elements
 
-By default disabled elements like `<button>` do not trigger user interactions so a `Tooltip` will not activate on normal events like hover. To accommodate disabled elements, add a simple wrapper element like a `span`.
+By default disabled elements like `<button>` do not trigger user interactions so a `Tooltip` will not activate on normal events like hover. To accommodate disabled elements, add a simple wrapper element like a `div`.
 
 {{"demo": "pages/components/tooltips/DisabledTooltips.js"}}
 
 > If you're not wrapping a Material-UI component that inherits from `ButtonBase`, for instance, a native `<button>` element, you should also add the CSS property *pointer-events: none;* to your element when disabled:
 
 ```jsx
 <Tooltip title="You don't have permission to do this">
-  <span>
+  <div>
     <button disabled={disabled} style={disabled ? { pointerEvents: "none" } : {}}>
       {'A disabled button'}
     </button>
-  </span>
+  </div>
 </Tooltip>
 ```
 

diff --git a/packages/material-ui/src/ButtonBase/ButtonBase.d.ts b/packages/material-ui/src/ButtonBase/ButtonBase.d.ts
@@ -45,7 +45,7 @@ export type ButtonBaseProps<
   P = {}
 > = OverrideProps<ButtonBaseTypeMap<P, D>, D>;
 
-export type ButtonBaseClassKey = 'root' | 'a' | 'disabled' | 'focusVisible';
+export type ButtonBaseClassKey = 'root' | 'disabled' | 'focusVisible';
 
 export interface ButtonBaseActions {
   focusVisible(): void;

diff --git a/packages/material-ui/src/ButtonBase/ButtonBase.js b/packages/material-ui/src/ButtonBase/ButtonBase.js
@@ -10,9 +10,6 @@ import NoSsr from '../NoSsr';
 import { useIsFocusVisible } from '../utils/focusVisible';
 import TouchRipple from './TouchRipple';
 
-const ConditionalWrapper = ({ condition, wrapper, children }) =>
-  condition ? wrapper(children) : children;
-
 export const styles = {
   /* Styles applied to the root element. */
   root: {
@@ -40,18 +37,13 @@ export const styles = {
     '&::-moz-focus-inner': {
       borderStyle: 'none', // Remove Firefox dotted outline.
     },
-    '&$a$disabled': {
-      display: 'inline-block' /* For IE11/ MS Edge bug */,
-      pointerEvents: 'none',
-      textDecoration: 'none',
+    '&$disabled': {
+      pointerEvents: 'none', // Disable link interactions
+      cursor: 'default',
     },
   },
-  /* Styles applied to a link. */
-  a: {},
   /* Pseudo-class applied to the root element if `disabled={true}`. */
-  disabled: {
-    cursor: 'not-allowed',
-  },
+  disabled: {},
   /* Pseudo-class applied to the root element if keyboard focused. */
   focusVisible: {},
 };
@@ -264,47 +256,41 @@ const ButtonBase = React.forwardRef(function ButtonBase(props, ref) {
   const handleRef = useForkRef(handleUserRef, handleOwnRef);
 
   return (
-    <ConditionalWrapper
-      condition={other.href && disabled}
-      wrapper={wrapperChildren => <span className={classes.disabled}>{wrapperChildren}</span>}
+    <ComponentProp
+      className={clsx(
+        classes.root,
+        {
+          [classes.disabled]: disabled,
+          [classes.focusVisible]: focusVisible,
+          [focusVisibleClassName]: focusVisible,
+        },
+        className,
+      )}
+      onBlur={handleBlur}
+      onClick={onClick}
+      onFocus={handleFocus}
+      onKeyDown={handleKeyDown}
+      onKeyUp={handleKeyUp}
+      onMouseDown={handleMouseDown}
+      onMouseLeave={handleMouseLeave}
+      onMouseUp={handleMouseUp}
+      onDragLeave={handleDragLeave}
+      onTouchEnd={handleTouchEnd}
+      onTouchMove={handleTouchMove}
+      onTouchStart={handleTouchStart}
+      ref={handleRef}
+      tabIndex={disabled ? -1 : tabIndex}
+      {...buttonProps}
+      {...other}
     >
-      <ComponentProp
-        className={clsx(
-          classes.root,
-          {
-            [classes.a]: other.href,
-            [classes.disabled]: disabled,
-            [classes.focusVisible]: focusVisible,
-            [focusVisibleClassName]: focusVisible,
-          },
-          className,
-        )}
-        onBlur={handleBlur}
-        onClick={onClick}
-        onFocus={handleFocus}
-        onKeyDown={handleKeyDown}
-        onKeyUp={handleKeyUp}
-        onMouseDown={handleMouseDown}
-        onMouseLeave={handleMouseLeave}
-        onMouseUp={handleMouseUp}
-        onDragLeave={handleDragLeave}
-        onTouchEnd={handleTouchEnd}
-        onTouchMove={handleTouchMove}
-        onTouchStart={handleTouchStart}
-        ref={handleRef}
-        tabIndex={disabled ? -1 : tabIndex}
-        {...buttonProps}
-        {...other}
-      >
-        {children}
-        {!disableRipple && !disabled ? (
-          <NoSsr>
-            {/* TouchRipple is only needed client-side, x2 boost on the server. */}
-            <TouchRipple ref={rippleRef} center={centerRipple} {...TouchRippleProps} />
-          </NoSsr>
-        ) : null}
-      </ComponentProp>
-    </ConditionalWrapper>
+      {children}
+      {!disableRipple && !disabled ? (
+        <NoSsr>
+          {/* TouchRipple is only needed client-side, x2 boost on the server. */}
+          <TouchRipple ref={rippleRef} center={centerRipple} {...TouchRippleProps} />
+        </NoSsr>
+      ) : null}
+    </ComponentProp>
   );
 });
 

diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js
@@ -134,7 +134,7 @@ const Tooltip = React.forwardRef(function Tooltip(props, ref) {
             'A disabled element does not fire events.',
             "Tooltip needs to listen to the child element's events to display the title.",
             '',
-            'Place a `div` container on top of the element.',
+            'Add a simple wrapper element like a `div`.',
           ].join('\n'),
         );
       }