Skip to content
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

[Slider] Issues with adding focus-visible styles to the thumb slot #77

Closed
danilo-leal opened this issue Jan 17, 2024 · 5 comments · Fixed by #373
Closed

[Slider] Issues with adding focus-visible styles to the thumb slot #77

danilo-leal opened this issue Jan 17, 2024 · 5 comments · Fixed by #373
Assignees
Labels
bug 🐛 Something doesn't work component: slider This is the name of the generic UI component, not the React module!

Comments

@danilo-leal
Copy link
Contributor

danilo-leal commented Jan 17, 2024

As surfaced in mui/material-ui#40332, there seems to be a bug with the focus treatment for the Slider component.

  • Suppose the focus styles are added to the thumb slot using our workaround class Mui-focusVisible through ${sliderClasses.focusVisible}. In that case, the problem is that they persist even if you move away from the thumb after clicking, dragging, and leaving it. That's illustrated in this comment: [base-ui][docs] Polish the Slider demos material-ui#40332 (comment)
  • If the focus styles are added using the native focus-visible pseudo-class instead, the above-described behavior does not happen anymore. However, the problem then is that none of the styles appear if focusing/targeting the Slider's thumb using the keyboard.

Search keywords: base-ui, slider, thumb, bug

@mnajdova
Copy link
Member

mnajdova commented Jan 22, 2024

I see two potential solutions to fix the root issue here, each of them is going to require non-trivial refactoring of the useSlider hook:

  1. The first solution would be using the role="slider" directly on the thumb <span /> element and getting rid of the range input and use a hidden input instead. This would mean that we will need to handle all of the touch/keyboard events typically handled by the <input type="range" /> element we use right now. Also, we should validate before going deeper if this will be fully accessible, as there are some warnings around this pattern in https://www.w3.org/WAI/ARIA/apg/patterns/slider/
  2. The second solution would be to try to merge the thumb and the input into one element. This should allow us to use the input's native focus, focus-visible selectors directly on the thumb (as they would be one element).

@gitstart
Copy link

gitstart commented Feb 1, 2024

@danilo-leal @mnajdova we would like to pick this up

@danilo-leal
Copy link
Contributor Author

Go for it. I'd assume that option 2, which @mnajdova has provided, is the preferred one to start exploring to solve this issue!

@gitstart
Copy link

gitstart commented Feb 5, 2024

I see two potential solutions to fix the root issue here, each of them is going to require non-trivial refactoring of the useSlider hook:

  1. The first solution would be using the role="slider" directly on the thumb <span /> element and getting rid of the range input and use a hidden input instead. This would mean that we will need to handle all of the touch/keyboard events typically handled by the <input type="range" /> element we use right now. Also, we should validate before going deeper if this will be fully accessible, as there are some warnings around this pattern in https://www.w3.org/WAI/ARIA/apg/patterns/slider/
  2. The second solution would be to try to merge the thumb and the input into one element. This should allow us to use the input's native focus, focus-visible selectors directly on the thumb (as they would be one element).

The second approach faces a challenge as the focus and focus-visible selectors persist when the mouse moves out of the host element. A pseudo-CSS snippet attempts to have an insight into the behavior when we finally merge both the input and thumb and can directly use the pseudo-class on the input element as shown below, but it doesn't provide the desired solution.

&:has(> input:focus-visible) {
    // box-shadow styles;
}

The reason why it seems when focus styles are added using the native focus-visible, behavior does not happen anymore is because the focus-visible style is not applied to the span element at any point in time, as the span lacks the focus-visible pseudo-class. The shadow effect is only derived from the MUI active class, which is removed when the mouse leaves the slider's thumb.

If the focus styles are added using the native focus-visible pseudo-class instead, the above-described behavior does not happen anymore. However, the problem then is that none of the styles appear if focusing/targeting the Slider's thumb using the keyboard.

While we await feedback/more clarity on this, we would like to state that one potential solution is to remove the MUI focus-visible class onMouseUp

cc @danilo-leal @mnajdova

@michaldudak michaldudak transferred this issue from mui/material-ui Feb 27, 2024
@michaldudak michaldudak changed the title [base-ui][Slider] Issues with adding focus-visible styles to the thumb slot [Slider] Issues with adding focus-visible styles to the thumb slot Feb 27, 2024
@michaldudak michaldudak added component: slider This is the name of the generic UI component, not the React module! bug 🐛 Something doesn't work labels Feb 27, 2024
@mj12albert
Copy link
Member

The second solution would be to try to merge the thumb and the input into one element. This should allow us to use the input's native focus, focus-visible selectors directly on the thumb (as they would be one element).

In the new API this is solved by not exactly by merging the thumb and input into one, but instead just swapping their tabIndex and fixing up the handling of keyDown

The (new) Thumb which is a span, can be styled directly with :focus-visible

Demo: https://deploy-preview-373--base-ui.netlify.app/experiments/slider/

@mj12albert mj12albert self-assigned this May 22, 2024
@github-project-automation github-project-automation bot moved this from Selected to Done in Base UI Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: slider This is the name of the generic UI component, not the React module!
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants