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.

Sign up for GitHub

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

[Button][material-ui] Improve buttons' text selection #38158

Closed
wants to merge 11 commits into from
Closed

[Button][material-ui] Improve buttons' text selection #38158

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5beda3e
Remove incorrect user-select:none
DiegoAndai Jul 25, 2023
f042c2f
Experiment with active state for select styling
DiegoAndai Aug 22, 2023
763a4b5
Experiment 2 (disabled non-button not working)
DiegoAndai Aug 23, 2023
7cad1da
Merge branch 'master' into button-select-issue
DiegoAndai Aug 25, 2023
dd257e3
Don't destructure ownerState in style function
DiegoAndai Aug 25, 2023
dc398a4
Link components user select
DiegoAndai Aug 25, 2023
b2a1c21
Update experiment
DiegoAndai Sep 13, 2023
5a29557
Merge branch 'master' into button-select-issue
DiegoAndai Sep 13, 2023
2252e17
Rever Joy Link change
DiegoAndai Sep 13, 2023
d8d70fb
Port change to material-next ButtonBase
DiegoAndai Sep 13, 2023
a4016f1
Decrease scope of change
DiegoAndai Oct 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions docs/pages/experiments/material/button-select.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/* eslint-disable no-console */
import * as React from 'react';
import {
FormControlLabel,
Button as MaterialUIButton,
Experimental_CssVarsProvider as MaterialUICssVarsProvider,
Switch,
experimental_extendTheme as materialUIExtendTheme,
} from '@mui/material';
import { Stack } from '@mui/system';

function createHandlers(name: string) {
return {
onClick: () => console.log(`${name} click`),
};
}

export default function App() {
const [disabled, setDisabled] = React.useState(false);

return (
<MaterialUICssVarsProvider theme={materialUIExtendTheme({})}>
<Stack gap={12} alignItems="center" sx={{ my: 6 }}>
<Stack>
<FormControlLabel
control={<Switch checked={disabled} onChange={() => setDisabled((prev) => !prev)} />}
label="Toggle disabled"
/>
</Stack>
<Stack direction="row" justifyContent="space-around" sx={{ width: '80vw' }}>
<Stack gap={3}>
<Stack alignItems="flex-start">
<span>Native button</span>
<button type="button" disabled={disabled} {...createHandlers('Native button')}>
Button
</button>
</Stack>
<Stack alignItems="flex-start">
<span>Native anchor</span>
{/* eslint-disable-next-line jsx-a11y/anchor-is-valid */}
<a href="#" {...createHandlers('Native anchor')}>
Anchor
</a>
</Stack>
<Stack alignItems="flex-start">
<span>Native div</span>
<div {...createHandlers('Div')}>Button</div>
</Stack>
</Stack>

<Stack gap={3}>
<Stack alignItems="flex-start">
<span>Material UI button</span>
<MaterialUIButton
variant="contained"
disabled={disabled}
{...createHandlers('Material UI button')}
>
Button
</MaterialUIButton>
</Stack>
<Stack alignItems="flex-start">
<span>Material UI button link</span>
<MaterialUIButton
variant="contained"
disabled={disabled}
href="#"
{...createHandlers('Material UI button link')}
>
Button
</MaterialUIButton>
</Stack>
<Stack alignItems="flex-start">
<span>Material UI button as div</span>
<MaterialUIButton
variant="contained"
component="div"
disabled={disabled}
{...createHandlers('Material UI button as div')}
>
Button
</MaterialUIButton>
</Stack>
</Stack>
</Stack>
</Stack>
</MaterialUICssVarsProvider>
);
}
13 changes: 9 additions & 4 deletions packages/mui-material-next/src/ButtonBase/ButtonBase.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export const ButtonBaseRoot = styled('button', {
slot: 'Root',
shouldForwardProp: (prop) => rootShouldForwardProp(prop) && prop !== 'touchRippleRef',
overridesResolver: (props, styles) => styles.root,
})({
})<{ ownerState: ButtonBaseOwnerState }>(({ ownerState }) => ({
display: 'inline-flex',
alignItems: 'center',
justifyContent: 'center',
Expand All @@ -57,7 +57,9 @@ export const ButtonBaseRoot = styled('button', {
borderRadius: 0,
padding: 0, // Remove the padding in Firefox
cursor: 'pointer',
userSelect: 'none',
'&:active': {
userSelect: 'none',
},
verticalAlign: 'middle',
MozAppearance: 'none', // Reset
WebkitAppearance: 'none', // Reset
Expand All @@ -68,13 +70,15 @@ export const ButtonBaseRoot = styled('button', {
borderStyle: 'none', // Remove Firefox dotted outline.
},
[`&.${buttonBaseClasses.disabled}`]: {
pointerEvents: 'none', // Disable link interactions
...(!ownerState.isButton && {
pointerEvents: 'none', // Disable link interactions
}),
cursor: 'default',
},
'@media print': {
colorAdjust: 'exact',
},
}) as React.ElementType<any>;
})) as React.ElementType<any>;

/**
* `ButtonBase` contains as few styles as possible.
Expand Down Expand Up @@ -156,6 +160,7 @@ const ButtonBase = React.forwardRef(function ButtonBase<
disableTouchRipple,
focusVisible,
active,
isButton: ComponentProp === 'button',
};

const classes = useUtilityClasses(ownerState);
Expand Down
4 changes: 4 additions & 0 deletions packages/mui-material-next/src/ButtonBase/ButtonBase.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,10 @@ export type ButtonBaseProps<
};

export interface ButtonBaseOwnerState extends ButtonBaseProps {
/**
* If `true`, the DOM element is button.
*/
isButton: boolean;
/**
* If `true`, the button is active.
*/
Expand Down
13 changes: 9 additions & 4 deletions packages/mui-material/src/ButtonBase/ButtonBase.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export const ButtonBaseRoot = styled('button', {
name: 'MuiButtonBase',
slot: 'Root',
overridesResolver: (props, styles) => styles.root,
})({
})(({ ownerState }) => ({
display: 'inline-flex',
alignItems: 'center',
justifyContent: 'center',
Expand All @@ -47,24 +47,28 @@ export const ButtonBaseRoot = styled('button', {
borderRadius: 0,
padding: 0, // Remove the padding in Firefox
cursor: 'pointer',
userSelect: 'none',
verticalAlign: 'middle',
MozAppearance: 'none', // Reset
WebkitAppearance: 'none', // Reset
textDecoration: 'none',
// So we take precedent over the style of a native <a /> element.
color: 'inherit',
'&:active': {
DiegoAndai marked this conversation as resolved.
Show resolved Hide resolved
userSelect: 'none',
},
'&::-moz-focus-inner': {
borderStyle: 'none', // Remove Firefox dotted outline.
},
[`&.${buttonBaseClasses.disabled}`]: {
pointerEvents: 'none', // Disable link interactions
cursor: 'default',
...(!ownerState.isButton && {
DiegoAndai marked this conversation as resolved.
Show resolved Hide resolved
pointerEvents: 'none', // Disable link interactions
DiegoAndai marked this conversation as resolved.
Show resolved Hide resolved
}),
},
'@media print': {
colorAdjust: 'exact',
},
});
}));

/**
* `ButtonBase` contains as few styles as possible.
Expand Down Expand Up @@ -328,6 +332,7 @@ const ButtonBase = React.forwardRef(function ButtonBase(inProps, ref) {
...props,
centerRipple,
component,
isButton: ComponentProp === 'button',
DiegoAndai marked this conversation as resolved.
Show resolved Hide resolved
disabled,
disableRipple,
disableTouchRipple,
Expand Down
1 change: 0 additions & 1 deletion packages/mui-material/src/Chip/Chip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,6 @@ const ChipRoot = styled('div', {
},
({ theme, ownerState }) => ({
...(ownerState.clickable && {
userSelect: 'none',
WebkitTapHighlightColor: 'transparent',
cursor: 'pointer',
'&:hover': {
Expand Down
4 changes: 3 additions & 1 deletion packages/mui-material/src/Link/Link.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ const LinkRoot = styled(Typography, {
textDecorationColor: 'inherit',
},
}),
'&:active': {
userSelect: 'none',
},
Copy link
Member

@oliviertassinari oliviertassinari Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Will we ever need this?

I can think of 3 types of elements we can render:

  1. <a> solved by built-in behavior.
  2. <button> solved by built-in behavior.
  3. regular element, e.g. <div>. Do we want to optimize for devs that do YOLO? I would expect always 1 and 2.
Suggested change
'&:active': {
userSelect: 'none',
},

Copy link
Member Author

Choose a reason for hiding this comment

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

We will need this in Firefox to have the same behavior, as buttons in Firefox are not selectable by dragging from outside. We could add it inside the ownerState.component === 'button' condition, so it only applies to buttons, but I don't see the harm of supporting regular elements as well. What do you think?

// Same reset as ButtonBase.root
...(ownerState.component === 'button' && {
position: 'relative',
Expand All @@ -72,7 +75,6 @@ const LinkRoot = styled(Typography, {
borderRadius: 0,
padding: 0, // Remove the padding in Firefox
cursor: 'pointer',
userSelect: 'none',
verticalAlign: 'middle',
MozAppearance: 'none', // Reset
WebkitAppearance: 'none', // Reset
Expand Down
Loading