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

[material-ui][IconButton] Fix disableRipple behaviour when disableRipple is set in MuiButtonBase theme #43714

Merged
merged 18 commits into from
Oct 9, 2024
Merged

[material-ui][IconButton] Fix disableRipple behaviour when disableRipple is set in MuiButtonBase theme #43714

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
679a841
Revert "[IconButton] Fix hover background color behavior (#43271)"
sai6855 Sep 11, 2024
7e677a4
add tests
sai6855 Sep 11, 2024
493d12c
fix test
sai6855 Sep 11, 2024
afb666b
fix diff
sai6855 Sep 11, 2024
5e5c258
fix diff
sai6855 Sep 11, 2024
0a00417
Update IconButton.test.js
sai6855 Sep 11, 2024
34eeaec
Apply fix to the colored buttons as well
DiegoAndai Sep 11, 2024
a82d288
Remove incorrect style rule
DiegoAndai Sep 11, 2024
d04a8d4
Add test for colored IconButton hover
DiegoAndai Sep 11, 2024
02e1663
switch to CSS variable
siriwatknp Sep 12, 2024
9ea0b19
Merge pull request #1 from siriwatknp/disable-ripple2
sai6855 Sep 12, 2024
1ef9fb0
Merge branch 'master' into disable-ripple
sai6855 Sep 12, 2024
96e0f14
Add experimental test
DiegoAndai Sep 13, 2024
eaca677
Merge branch 'master' into disable-ripple
DiegoAndai Sep 13, 2024
f843e03
Remove experimental test
DiegoAndai Sep 13, 2024
4eaffa2
Remove hover tests
DiegoAndai Oct 8, 2024
53eb100
Merge branch 'master' into disable-ripple
DiegoAndai Oct 8, 2024
fcb49fa
Remove unused import
DiegoAndai Oct 8, 2024
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
7 changes: 2 additions & 5 deletions packages/mui-material/src/IconButton/IconButton.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ const IconButtonRoot = styled(ButtonBase, {
}),
variants: [
{
props: { disableRipple: false },
props: (props) => !props.disableRipple,
style: {
'&:hover': {
backgroundColor: theme.vars
Expand Down Expand Up @@ -113,7 +113,7 @@ const IconButtonRoot = styled(ButtonBase, {
...Object.entries(theme.palette)
.filter(createSimplePaletteValueFilter()) // check all the used fields in the style below
.map(([color]) => ({
props: { color, disableRipple: false },
props: (props) => props.color === color && !props.disableRipple,
Copy link
Member

@DiegoAndai DiegoAndai Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@siriwatknp If I remember correctly, there's a problem with accessing color like this, isn't there? How might we implement this one?

Copy link
Member

@siriwatknp siriwatknp Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, you are right. This won't work with Pigment CSS. Let me think of something else.

Copy link
Member

Choose a reason for hiding this comment

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

Here is my proposal using CSS variable: sai6855#1.

We keep the previous logic and separate the disableRipple from the variants.

Copy link
Contributor Author

@sai6855 sai6855 Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Here is my proposal using CSS variable: sai6855#1.

We keep the previous logic and separate the disableRipple from the variants.

Sure, @DiegoAndai I'm not really familiar with the changes, but if you give the go-ahead to the sai6855#1 I can merge the PR.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @siriwatknp! This solution makes sense 👌🏼

@sai6855 may I ask you to update this PR with that solution?

Copy link
Member

Choose a reason for hiding this comment

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

@Janpot I can confirm that your quick test works, as well as CSS variables in the IconButton component when running with Karma.

I discovered that the issue I'm facing, and why the test is not working, is that for some reason I can't get the hover style to be applied. I assume this is the reason the existing test is skipped in Karma: https://github.com/mui/material-ui/blob/master/packages/mui-material/src/IconButton/IconButton.test.js#L146

I've tried fireEvent.mouseMove, fireEvent.mouseOver, fireEvent.mouseEnter, and userEvent.hover, and none are working.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's possible to trigger pseudo classes programmatically if that's what the style relies on on. So I don't believe fireEvent/userEvent will ever be able to do that. testing-library/user-event#1210

Copy link
Member

Choose a reason for hiding this comment

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

But we are currently using fireEvent in JSDOM to test the hover style: https://github.com/mui/material-ui/blob/master/packages/mui-material/src/IconButton/IconButton.test.js#L152

This is the only test we have that uses this, though, and it seems like:

  • JSDOM handles hover but not CSS variables
  • Browser tests handle CSS variables but not hover
    😅

At this point, I'm considering removing the test and opening an issue to properly implement hover tests in the future. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

We're working on a migration to vitest and in their browser mode we'll have access to real user events https://vitest.dev/guide/browser/context.html. Doesn't look like karma has an equivalent. Maybe just skip the test for now?

Copy link
Member

Choose a reason for hiding this comment

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

👍🏼 agree

style: {
'&:hover': {
backgroundColor: theme.vars
Expand Down Expand Up @@ -164,7 +164,6 @@ const IconButton = React.forwardRef(function IconButton(inProps, ref) {
color = 'default',
disabled = false,
disableFocusRipple = false,
disableRipple = false,
size = 'medium',
...other
} = props;
Expand All @@ -175,7 +174,6 @@ const IconButton = React.forwardRef(function IconButton(inProps, ref) {
color,
disabled,
disableFocusRipple,
disableRipple,
size,
};

Expand All @@ -187,7 +185,6 @@ const IconButton = React.forwardRef(function IconButton(inProps, ref) {
centerRipple
focusRipple={!disableFocusRipple}
disabled={disabled}
disableRipple={disableRipple}
ref={ref}
{...other}
ownerState={ownerState}
Expand Down
37 changes: 37 additions & 0 deletions packages/mui-material/src/IconButton/IconButton.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,23 @@ describe('<IconButton />', () => {
});
});

it('should apply the colored hover background if color is provided', function test() {
if (!/jsdom/.test(window.navigator.userAgent)) {
this.skip();
}

const { container, getByTestId } = render(
<IconButton data-testid="icon-button" color="primary" />,
);

fireEvent.mouseMove(container.firstChild, {
clientX: 19,
});
expect(getByTestId('icon-button')).toHaveComputedStyle({
backgroundColor: 'rgba(25, 118, 210, 0.04)',
});
});

it('should not apply the hover background if disableRipple is true', function test() {
if (!/jsdom/.test(window.navigator.userAgent)) {
this.skip();
Expand All @@ -171,4 +188,24 @@ describe('<IconButton />', () => {
});
expect(getByTestId('icon-button')).toHaveComputedStyle({ backgroundColor: 'rgba(0, 0, 0, 0)' });
});

it('should disable ripple if disableRipple:true is set in MuiButtonBase', async () => {
const { container, getByRole } = render(
<ThemeProvider
theme={createTheme({
components: {
MuiButtonBase: {
defaultProps: {
disableRipple: true,
},
},
},
})}
>
<IconButton TouchRippleProps={{ className: 'touch-ripple' }}>book</IconButton>,
</ThemeProvider>,
);
await ripple.startTouch(getByRole('button'));
expect(container.querySelector('.touch-ripple')).to.equal(null);
});
});
Loading