[TypeScript] Switch is missing keyboardFocused class key

[ianschmitz]

Looks like the definition for Switch or one of its inherited definitions was updated recently which removed the keyboardFocused class key.

I wonder if SwitchBase's SwitchBaseClassKey definition should also include IconButtonClassKey?
https://github.com/mui-org/material-ui/blob/466c01fc7e7bc76adf5ad34da125daff43a1b206/src/internal/SwitchBase.d.ts#L23

[ianschmitz]

There may be more to it actually. When using [email protected] I receive a console error when trying to use the keyboardFocused class key when rendering an IconButton:

Warning: Material-UI: the key `keyboardFocused` provided to the classes property is not implemented in IconButton.
You can only override one of the following: root,colorInherit,colorPrimary,colorSecondary,disabled,label

However the TypeScript definitions are happy for me to provide a keyboardFocused class key in my IconButton.

[oliviertassinari]

I wonder if SwitchBase's SwitchBaseClassKey definition should also include IconButtonClassKey?

@ianschmitz No, it shouldn't. The classes isn't inherited between the React elements like the properties are. Should we change this behavior? I have been wondering about it over the last weeks. I don't know. I like how the classes object impact is scoped and how it as few side effects. I'm not sure CSS inheritance + classes object inheritance will make things simpler.

Going back to your issue. Do you need the keyboardFocused class name on the Switch element?

[ianschmitz]

Yes a keyboardFocused class name on the Switch would be awesome.

With regards to the inheritance, i was under the assumption that the inherited classes would flow through to the rendered component. i.e. IconButtonClassKey inherites the ButtonBaseClassKey, so i assumed that any ButtonBase classes I defined would be passed through to the rendered ButtonBase.

I agree that the classes scoping is nice. My sense is that if that's the pattern we want to follow, we shouldn't be combining the class key definitions of child components, as it can cause errors such as the one i experienced with keyboardFocused.

[oliviertassinari]

@ianschmitz It sounds like:

1. we need to clean the type definition that wrongly defines the keyboardFocused class key.
2. we gonna need to reintroduce a keyboardFocusedClassName property. It might also be time to look for a better naming if possible. It's the same feature as: https://github.com/WICG/focus-visible.

[ianschmitz]

Would it be reasonable to add keyboardFocused to the Switch classes? I just mention cause I've seen a few PRs that were trying to get rid of the xxxClassName properties.

[oliviertassinari]

Would it be reasonable to add keyboardFocused to the Switch classes? I just mention cause I've seen a few PRs that were trying to get rid of the xxxClassName properties.

@ianschmitz I'm happy with this too, but you need to add it to all the components along the chain: Switch > SwitchBase > IconButton. This is going to be quite some boilerplate.
I'm also fine with doing an exception to the rule of removing the xxxClassName properties, keyboardFocused is an important state.

[ianschmitz]

I'm happy either way. Thanks @oliviertassinari !

I agree keyboard focused states are super useful. We can do some pretty nifty styling with it. We were able to completely redesign the styling of MUI's Switch component to put our own touch on it, while still keeping the awesome API of MUI. For example here is our switch with the first one currently keyboard focused:

[ianschmitz]

@ianschmitz It sounds like:

we need to clean the type definition that wrongly defines the keyboardFocused class key.
we gonna need to reintroduce a keyboardFocusedClassName property. It might also be time to look for a better naming if possible. It's the same feature as: https://github.com/WICG/focus-visible.

To clarify this, both the Switch and IconButton need the keyboard focused state.

[ianschmitz]

@oliviertassinari I think this issue is a little more wide spread than I initially thought. I just tried using MenuItem's classes prop to specify keyboardFocused. TypeScript thinks this exists since MenuItem inherits ListItemClassKey, however at runtime this presents an error:

Warning: Material-UI: the key `keyboardFocused` provided to the classes property is not implemented in MenuItem.
You can only override one of the following: root,selected

In the case of MenuItem I'm stuck since there is no ListItemProps that i could use to pass in clases to ListItem. I suspect this may the case elsewhere.

[oliviertassinari]

@ianschmitz I'm adding this issue to the v1 milestone. We need to sort that out before the stable release. We might need to introduce a breaking change.

[dmythro]

@oliviertassinari, this broke the behavior of lots of parts of our application.
Basically, we need the same keyboard focused for each component who can receive focus: Button, ButtonBase, IconButton, ListItem, MenuItem etc. And it is required by the Accessibility - to have clear visible focus from the keyboard navigation.

Property/classes related change is not appropriate as it should behave/look the same on the whole application and possible to do with theme/overrides or something like that.

If there is another issue already created please link it here - I'll follow there.

[oliviertassinari]

Property/classes related change is not appropriate as it should behave/look the same on the whole application and possible to do with theme/overrides or something like that.

@Z-AX I see your point. Of course you could wrap each component. But a theme.overrides.MuiButtonBase.focusVisible would be simpler. Do you want to work on that?

[dmythro]

@oliviertassinari actually, I'm more into the focusVisible. But as briefly refactored to focusVisible from keyboardFocused in theme creation see that it's not applied everywhere (BaseButton/IconButton is not affected at least from what I see). Or it is? Or, maybe, there is a possible way to override it in a single place? Basically, we're just setting the custom outline for this. Probably would be nice to remove ripple layout as it sticks now on focused button and reduces the contrast (which also may be bad for ADA/Accessibility).

[dmythro]

By removing ripple I mean this case which sticks while focus on the component, not removed anyhow with time:

[dmythro]

But at least I want the consistent outline, reused everywhere :)