[material-ui][IconButton] Fix disableRipple behaviour when disableRipple is set in MuiButtonBase theme by sai6855 · Pull Request #43714 · mui/material-ui

@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 · 2024-09-11T17:36:58Z

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 · 2024-09-11T20:55:09Z

@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 · 2024-09-11T22:19:51Z

        .filter(createSimplePaletteValueFilter()) // check all the used fields in the style below
        .map(([color]) => ({
-          props: { color, disableRipple: false },
+          props: (props) => props.color === color && !props.disableRipple,


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

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

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

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

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.

Thanks @siriwatknp! This solution makes sense 👌🏼

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

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

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

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?

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?

👍🏼 agree

Use CSS variable

siriwatknp · 2024-10-09T03:43:01Z

Thanks for working on this @sai6855

Revert "[IconButton] Fix hover background color behavior (mui#43271)"

679a841

This reverts commit d9d0a37.

sai6855 marked this pull request as draft September 11, 2024 16:29

sai6855 added 2 commits September 11, 2024 22:06

add tests

7e677a4

fix test

493d12c

sai6855 requested a review from siriwatknp September 11, 2024 16:45

sai6855 added 2 commits September 11, 2024 22:20

fix diff

afb666b

fix diff

5e5c258

sai6855 force-pushed the disable-ripple branch from fa42d80 to 5e5c258 Compare September 11, 2024 16:52

Update IconButton.test.js

0a00417

sai6855 changed the title ~~Revert "[IconButton] Fix hover background color behavior (#43271)"~~ [material-ui][IconButton] Fix disableRipple behaviour when disableRipple is set in MuiButtonBase theme Sep 11, 2024

sai6855 requested a review from DiegoAndai September 11, 2024 17:02

sai6855 marked this pull request as ready for review September 11, 2024 17:07

Apply fix to the colored buttons as well

34eeaec

DiegoAndai added 2 commits September 11, 2024 18:10

Remove incorrect style rule

a82d288

Add test for colored IconButton hover

d04a8d4

DiegoAndai reviewed Sep 11, 2024

View reviewed changes

switch to CSS variable

02e1663

zannager added the scope: icon button Changes related to the icon button. label Sep 12, 2024

siriwatknp assigned sai6855 and siriwatknp Sep 12, 2024

sai6855 and others added 4 commits September 12, 2024 21:02

Merge pull request #1 from siriwatknp/disable-ripple2

9ea0b19

Use CSS variable

Merge branch 'master' into disable-ripple

1ef9fb0

Add experimental test

96e0f14

Merge branch 'master' into disable-ripple

eaca677

DiegoAndai added 4 commits September 13, 2024 14:51

Remove experimental test

f843e03

Remove hover tests

4eaffa2

Merge branch 'master' into disable-ripple

53eb100

Remove unused import

fcb49fa

DiegoAndai requested review from siriwatknp and removed request for siriwatknp October 8, 2024 19:34

siriwatknp approved these changes Oct 9, 2024

View reviewed changes

DiegoAndai merged commit 9c69a40 into mui:master Oct 9, 2024

DiegoAndai mentioned this pull request Oct 9, 2024

[material-ui][IconButton] disableRipple on MuiButtonBase doesn't disable ripple effect on IconButton #43617

Closed

Hoang-Nguyen-Huy mentioned this pull request Nov 9, 2024

[Snyk] Upgrade @mui/icons-material from 6.1.3 to 6.1.4 Hoang-Nguyen-Huy/fer-lab1#20

Merged

Uh oh!

Conversation

sai6855 commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mui-bot commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Netlify deploy preview

Bundle size report

Uh oh!

sai6855 commented Sep 11, 2024

Uh oh!

DiegoAndai commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sai6855 commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sai6855 commented Sep 11, 2024

Uh oh!

DiegoAndai commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DiegoAndai Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siriwatknp Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siriwatknp Sep 12, 2024

Choose a reason for hiding this comment

Uh oh!

sai6855 Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DiegoAndai Sep 12, 2024

Choose a reason for hiding this comment

Uh oh!

DiegoAndai Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

Janpot Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

DiegoAndai Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

Janpot Oct 5, 2024

Choose a reason for hiding this comment

Uh oh!

DiegoAndai Oct 8, 2024

Choose a reason for hiding this comment

Uh oh!

siriwatknp commented Oct 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sai6855 commented Sep 11, 2024 •

edited

Loading

mui-bot commented Sep 11, 2024 •

edited

Loading

DiegoAndai commented Sep 11, 2024 •

edited

Loading

sai6855 commented Sep 11, 2024 •

edited

Loading

DiegoAndai commented Sep 11, 2024 •

edited

Loading

DiegoAndai Sep 11, 2024 •

edited

Loading

siriwatknp Sep 12, 2024 •

edited

Loading

sai6855 Sep 12, 2024 •

edited

Loading