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

[sai6855]

This PR does following things

1. revert logic in [IconButton] Fix hover background color behavior #43271
2. retain tests from [IconButton] Fix hover background color behavior #43271
3. Add tests and logic for [material-ui][IconButton] disableRipple on MuiButtonBase doesn't disable ripple effect on IconButton #43617

closes #43617

check #43617 (comment) for more context

before: https://codesandbox.io/embed/p2smlj?module=/src/Demo.tsx

after: https://codesandbox.io/embed/g7kd38?module=/src/Demo.tsx

• I have followed (at least) the PR section of the contributing guide.

[mui-bot]

Netlify deploy preview

https://deploy-preview-43714--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against fcb49fa

[sai6855]

(I have reverted old PR logic and updated new logic in same PR, let me know if this has to be split in seperate PR's)

[DiegoAndai]

Hey @sai6855, thanks for working on this. In the "Before" demo, the hover style is broken:

Screen.Recording.2024-09-11.at.14.14.06.mov

I don't know why #43271 tests are passing 🤔

[sai6855]

@DiegoAndai I'm not sure why before and after sandboxes isn't getting updated code, meanwhile can you paste below code in these following demos to test the fix

before: https://mui.com/material-ui/react-button/#icon-button
after: https://deploy-preview-43714--material-ui.netlify.app/material-ui/react-button/#icon-button

import * as React from 'react';
import IconButton from '@mui/material/IconButton';
import Stack from '@mui/material/Stack';
import DeleteIcon from '@mui/icons-material/Delete';
import AlarmIcon from '@mui/icons-material/Alarm';
import AddShoppingCartIcon from '@mui/icons-material/AddShoppingCart';
import { ThemeProvider, createTheme } from '@mui/material/styles';

export default function IconButtons() {
  return (
    <Stack direction="row" spacing={1}>

      <ThemeProvider
        theme={createTheme({
          components: {
            MuiButtonBase: {
              defaultProps: {
                disableRipple: true,
              },
            },
          },
        })}
      >
      <IconButton aria-label="delete">
        <DeleteIcon />
      </IconButton>

      </ThemeProvider>
     
    </Stack>
  );
}

[sai6855]

the hover style is broken

Hmm, what do you mean by this 🤔 ? i tried clicking on icons in this demo, everything seems to be fine

Recording.2024-09-11.230629.mp4

[DiegoAndai]

@sai6855 I found the issue and committed it: 34eeaec. I think this covers it.

Let's wait for @siriwatknp's review to merge this.

[DiegoAndai]

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

[siriwatknp]

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

[siriwatknp]

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

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

[sai6855]

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.

[DiegoAndai]

Thanks @siriwatknp! This solution makes sense 👌🏼

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

[DiegoAndai]

@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.

[Janpot]

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

[DiegoAndai]

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?

[Janpot]

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?

[DiegoAndai]

👍🏼 agree

[siriwatknp]

Thanks for working on this @sai6855