-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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.
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
[core] Remove useIsFocusVisible util #42467
Conversation
Netlify deploy previewhttps://deploy-preview-42467--material-ui.netlify.app/ Link: parsed: -2.54% 😍, gzip: -2.82% 😍 Bundle size reportDetails of bundle changes (Toolpad) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a precaution, have you tested that all these components behave identically once moving to use the :focus-visible
pseudo selector in the oldest browsers we support? Just to make sure there won't introduce some subtle breaking changes with this.
I tested all browsers, both minimally supported and latest versions. The only thing "not working" is the Link's focus visible in Firefox 115, but that's not working on This is ready for review 😊 |
1dce511
to
16dddbb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! Left some small comments and questions.
45f9abe
to
6c31464
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay!
/** | ||
* Returns a boolean indicating if the event's target has :focus-visible | ||
*/ | ||
export default function isFocusVisible(element: Element): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have expected this to come from Base UI.
?
export default function isFocusVisible(element: Element): boolean { | |
// TODO import from Base UI | |
export default function isFocusVisible(element: Element): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in the description: "ideally, we would use the :focus-visible
pseudo-selector directly, but that would mean removing the focusVisible
classes and refactoring many components."
The isFocusVisible
function is an intermediary step, so I don't think we should implement it in Base UI. When we reimplement the look of the library, we should use :focus-visible
directly, deprecate the focusVisible
classes, and eventually remove this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this one. 50% of the use of isFocusVisible
is to apply the focusVisible class name. We can eventually remove this. The other 50% is to change React states or JavaScript behaviors. So I don't see how this isFocusVisible()
helper can be removed.
(I landed here from https://www.notion.so/engineering-mui-utils-purpose-9a9fc9da3a004864b6c4e1f4d1f24f95?d=43a963028e3f410fa7eb7f0366a23968&pvs=4#d4907a42a7b0408ca14802a2880d2362).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other 50% is to change React states or JavaScript behaviors
From the components in this PR, the only one that uses isFocusVisible
to manage JavaScript behavior is ButtonBase
. Two out of three of those are related to ripple pulse, which, if I recall correctly, is not present in MD3.
The remaining one of the three relates to event handling, which should eventually come from Base UI. If they don't have a use case for managing JavaScript behavior, then I don't think Material UI will. cc: @michaldudak.
I'm not sure; I haven't dug into this yet as it's not a priority at all, but we might be able to remove it eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use such a utility currently in Base UI. However, there's one use case where it may help - when we focus an element programmatically during an mousedown event it sometimes incorrectly enters the focus-visible state. This happens as browsers (seemingly) wait for the click event to determine if a mouse interaction caused focus change. A custom utility could fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two out of three of those are related to ripple pulse, which, if I recall correctly, is not present in MD3.
@DiegoAndai If by pulse we mean:
Screen.Recording.2024-09-16.at.18.13.12.mov
Agree, it's gone. I think it's a great move of MD3, didn't like that. Otherwise, ripple is still present: https://material-web.dev/components/button/.
uses isFocusVisible to manage JavaScript behavior
What seems to be true:
- Button: can be replaced with
:focus-visible
- Slider: can be replaced with
:focus-visible
- Switch: can be replaced with
:focus-visible
- Link: can be replaced with
:focus-visible
- Rating: can be replaced with
:focus-visible
- ButtonBase: pulse ripple, on its way out
- Tooltip: required for behavior. Calling .focus() shouldn't show the tooltip. It's also necessary in Base UI's Tooltip, but today this logic is hidden under https://github.com/floating-ui/floating-ui/blob/ecd627e4676a50643edcdf8915a6acefa50ee3d8/packages/react/src/hooks/useFocus.ts#L124.
So, ok, it's not 50% 😄
cc @atomiks for awareness. There could be value, e.g. for logic like
if (process.env.NODE_ENV !== 'production' && !/jsdom/.test(window.navigator.userAgent)) { |
Part of #40958
Closes: #14420
As
:focus-visible
is now supported in all our supported browsers (reference), we can remove this utility and rely completely on:focus-visible
.This is an intermediate step: ideally, we would use the
:focus-visible
pseudo-selector directly, but that would mean removing thefocusVisible
classes and refactoring many components.