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] Vertical slider incorrectly exposed as horizontal in the accessibility tree in Chrome<124 #635

Closed
oliviertassinari opened this issue Sep 18, 2024 · 10 comments · Fixed by #583
Assignees
Labels
accessibility a11y bug 🐛 Something doesn't work component: slider This is the name of the generic UI component, not the React module!

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 18, 2024

Steps to reproduce

Steps:

  1. Open https://master--base-ui.netlify.app/components/react-slider/#vertical
    or open https://mui.com/material-ui/react-slider/#vertical-sliders

Current behavior

SCR-20240918-mzoi

Expected behavior

SCR-20240918-mzvb

Context

https://issues.chromium.org/issues/40739626 was fixed 3 years ago after Sebastian reported it, so we should now be able to remove the content in https://mui.com/material-ui/react-slider/#vertical-sliders and apply the style. This style is needed because aria-orientation is no correctly interpreted per https://issues.chromium.org/issues/40736841.

I noticed this in #636, it was weird.

Closing this issue also means getting https://mui.com/material-ui/react-slider/#vertical-sliders fixed. I opened this issue in Base UI to be at the root, for the Slider component owner.

Your environment

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

Search keywords: -

@oliviertassinari oliviertassinari added status: waiting for maintainer These issues haven't been looked at yet by a maintainer component: slider This is the name of the generic UI component, not the React module! bug 🐛 Something doesn't work accessibility a11y priority: low To delay as much as possible and removed priority: low To delay as much as possible labels Sep 18, 2024
@mj12albert mj12albert removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 19, 2024
@mj12albert
Copy link
Member

mj12albert commented Sep 20, 2024

Screenshot 2024-09-20 at 11 54 26 AM

As of 2023 Chromium plans to deprecate webkit-appearance: slider-vertical 😅 https://issues.chromium.org/issues/40261389

The recommendation is to use writing-mode instead, though for <input type="range"> it doesn't match Base UI's browser support yet

For older browsers that we need to support, there are still (non-standard) properties to fall back to: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_writing_modes/Vertical_controls#creating_vertical_range_sliders_in_older_browsers

@oliviertassinari
Copy link
Member Author

@mj12albert oh, I didn't know interesting. I guess it's mdn/content#32142.

@mj12albert
Copy link
Member

We discussed this and decided to:

  • Set writing-mode in the built-in styles
  • In the docs, recommend webkit-appearance: slider-vertical if older Chrome support is needed. We don't want to ship this with a console warning, and hopefully the usage share of Chrome<124 will decline fast.

@mj12albert mj12albert changed the title [slider] Add missing webkit-appearance: slider-vertical CSS [slider] Vertical slider incorrectly exposed as horizontal in the accessibility tree Sep 27, 2024
@mj12albert mj12albert changed the title [slider] Vertical slider incorrectly exposed as horizontal in the accessibility tree [slider] Vertical slider incorrectly exposed as horizontal in the accessibility tree in Chrome<124 Sep 27, 2024
Copy link

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue.
Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Oct 17, 2024

Great first step.

I'm reopening per:

Closing this issue also means getting https://mui.com/material-ui/react-slider/#vertical-sliders fixed. I opened this issue in Base UI to be at the root, for the Slider component owner.

There is nothing about Material Design in fixing this in Material UI, so I believe we want the two things to be done in coordination, by the same component owner.

@mj12albert
Copy link
Member

The bug where arrow keys are reversed in Chromium for vertical sliders is fixed now: https://issues.chromium.org/issues/40739626#comment9

Here's a sandbox: https://codesandbox.io/p/sandbox/xhmkxy

Though supported browser versions are different between Base and Material UI right now, do you agree with generally doing the same:

  • add CSS writing-mode to the built-in styles
  • document -webkit-appearance: slider-vertical for older browsers
  • remove the content related to the reversed arrow keys

@oliviertassinari @aarongarciah

@aarongarciah
Copy link
Member

@mj12albert not sure I follow which styles are planned to ship as part of Base UI vs Material UI.

@oliviertassinari if this issue is solved in Base UI, let's close it and open one a new one in Material UI. It doesn't make sense to keep an issue open because a project that's supposed to adopt Base UI hasn't adopted it yet. I think this is a signal that thinking about "component owners" is not a good model.

@colmtuite
Copy link
Contributor

colmtuite commented Oct 25, 2024

It doesn't make sense to keep an issue open because a project that's supposed to adopt Base UI hasn't adopted it yet.

Of course, we should stop this practice of leaving issues lingering around for ages. Once it's resolved for Base UI, it should be closed.

I think this is a signal that thinking about "component owners" is not a good model.

We don't subscribe to the idea of "component owners" in the Core team. The way we operate is the Base UI team is responsible for shipping an accessible, flexible component, and the MUI team is responsible for wrapping it.

@mj12albert If MUI Slider wraps Base UI Slider, then won't the built-in styles be the same automatically? Also, MUI Slider styles will be bundled, so we don't need to document styling?

@mj12albert
Copy link
Member

let's close it and open one a new one in Material UI

I've opened one: mui/material-ui#44237

Copy link

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue.
Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y bug 🐛 Something doesn't work component: slider This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants