mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.2k stars 32.09k forks source link

[ButtonBase] Unfocus does not clear ripple #28181

Closed siriwatknp closed 3 years ago

siriwatknp commented 3 years ago

keyboard tab between ButtonBase component does not clear the focus ripple. I can confirm that revert the code on this commit does not have this issue (in ButtonBase.js). https://github.com/mui-org/material-ui/commit/5f30983bfa16195237fde55a78d5e43b151a29fa

cc @michaldudak can you help take a look?

Current Behavior 😯

Screen Shot 2564-09-06 at 14 58 24

Expected Behavior 🤔

it should clear the ripple element

Steps to Reproduce 🕹

Steps:

  1. open https://next.material-ui.com/components/buttons/#main-content
  2. tab through the docs

Context 🔦

Your Environment 🌎

`npx @mui/envinfo` ``` Don't forget to mention which browser you used. Output from `npx @mui/envinfo` goes here. ```
michaldudak commented 3 years ago

Yes, I've noticed that and I'm already working on a fix.

eps1lon commented 3 years ago

I think I've talked repeatedly about not doing the abstraction. The ButtonBase is integral to the codebase and shouldn't be jeopardized by the unstyled effort. The unstyled effort can be a separate experiment that doesn't need to affect the core product.

oliviertassinari commented 3 years ago

The ButtonBase is integral to the codebase and shouldn't be jeopardized by the unstyled effort

@eps1lon If you could expand on the why, I think that it would be great.

Looking at this regression, it seems that one of the values of using unstyled in the material design components is that it allows surfacing problems earlier. For instance, maybe we missed a test case, or maybe we didn't use the best abstraction for useButton or maybe it's something completely unrelated.

eps1lon commented 3 years ago

@eps1lon If you could expand on the why, I think that it would be great.

Why do I need to explain that a fundamental component should not be affected by experiments? Like what would you expect to hear?

michaldudak commented 3 years ago

I think I've talked repeatedly about not doing the abstraction.

You mean not to implement Button with useButton? If so, I suppose I may have missed that. In the useButton PR, both @mnajdova and @oliviertassinari suggested using the new hook in the ButtonBase and I haven't seen any objections from your side.

As for the issue itself, I already pinpointed it and I'm adding few tests to ButtonBase to help to avoid such problems in the future.

siriwatknp commented 3 years ago

I remember we talked about this (not sure exactly which meeting) that if the unstyled abstraction does not introduce breaking change to the core, we will do it in the core.

michaldudak commented 3 years ago

As discussed today in the planning, we will restore the original implementation and create a separate private package with Material components (no matter if they have breaking changes or not) built using @mui/core.