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

[material-ui][Slider] Set CSS writing-mode and update vertical slider docs #44537

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import RangeSlider from '../../../../../../docs/data/material/components/slider/
import SliderSizes from '../../../../../../docs/data/material/components/slider/SliderSizes';
import TrackFalseSlider from '../../../../../../docs/data/material/components/slider/TrackFalseSlider';
import TrackInvertedSlider from '../../../../../../docs/data/material/components/slider/TrackInvertedSlider';
import VerticalAccessibleSlider from '../../../../../../docs/data/material/components/slider/VerticalAccessibleSlider';
import VerticalSlider from '../../../../../../docs/data/material/components/slider/VerticalSlider';

export default function Slider() {
Expand Down Expand Up @@ -125,12 +124,6 @@ export default function Slider() {
<TrackInvertedSlider />
</div>
</section>
<section>
<h2> Vertical Accessible Slider</h2>
<div className="demo-container">
<VerticalAccessibleSlider />
</div>
</section>
<section>
<h2> Vertical Slider</h2>
<div className="demo-container">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import RangeSlider from '../../../../../docs/data/material/components/slider/Ran
import SliderSizes from '../../../../../docs/data/material/components/slider/SliderSizes.tsx';
import TrackFalseSlider from '../../../../../docs/data/material/components/slider/TrackFalseSlider.tsx';
import TrackInvertedSlider from '../../../../../docs/data/material/components/slider/TrackInvertedSlider.tsx';
import VerticalAccessibleSlider from '../../../../../docs/data/material/components/slider/VerticalAccessibleSlider.tsx';
import VerticalSlider from '../../../../../docs/data/material/components/slider/VerticalSlider.tsx';

export default function Slider() {
Expand Down Expand Up @@ -126,12 +125,6 @@ export default function Slider() {
<TrackInvertedSlider />
</div>
</section>
<section>
<h2> Vertical Accessible Slider</h2>
<div className="demo-container">
<VerticalAccessibleSlider />
</div>
</section>
<section>
<h2> Vertical Slider</h2>
<div className="demo-container">
Expand Down
28 changes: 0 additions & 28 deletions docs/data/material/components/slider/VerticalAccessibleSlider.js

This file was deleted.

28 changes: 0 additions & 28 deletions docs/data/material/components/slider/VerticalAccessibleSlider.tsx

This file was deleted.

This file was deleted.

50 changes: 25 additions & 25 deletions docs/data/material/components/slider/VerticalSlider.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,36 +2,13 @@ import * as React from 'react';
import Stack from '@mui/material/Stack';
import Slider from '@mui/material/Slider';

function valuetext(value) {
return `${value}°C`;
}

const marks = [
{
value: 0,
label: '0°C',
},
{
value: 20,
label: '20°C',
},
{
value: 37,
label: '37°C',
},
{
value: 100,
label: '100°C',
},
];

export default function VerticalSlider() {
return (
<Stack sx={{ height: 300 }} spacing={1} direction="row">
<Slider
aria-label="Temperature"
orientation="vertical"
getAriaValueText={valuetext}
getAriaValueText={getAriaValueText}
valueLabelDisplay="auto"
defaultValue={30}
/>
Expand All @@ -45,11 +22,34 @@ export default function VerticalSlider() {
<Slider
getAriaLabel={() => 'Temperature'}
orientation="vertical"
getAriaValueText={valuetext}
getAriaValueText={getAriaValueText}
defaultValue={[20, 37]}
valueLabelDisplay="auto"
marks={marks}
/>
</Stack>
);
}

function getAriaValueText(value) {
return `${value}°C`;
}

const marks = [
{
value: 0,
label: '0°C',
},
{
value: 20,
label: '20°C',
},
{
value: 37,
label: '37°C',
},
{
value: 100,
label: '100°C',
},
];
50 changes: 25 additions & 25 deletions docs/data/material/components/slider/VerticalSlider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,36 +2,13 @@ import * as React from 'react';
import Stack from '@mui/material/Stack';
import Slider from '@mui/material/Slider';

function valuetext(value: number) {
return `${value}°C`;
}

const marks = [
{
value: 0,
label: '0°C',
},
{
value: 20,
label: '20°C',
},
{
value: 37,
label: '37°C',
},
{
value: 100,
label: '100°C',
},
];

export default function VerticalSlider() {
return (
<Stack sx={{ height: 300 }} spacing={1} direction="row">
<Slider
aria-label="Temperature"
orientation="vertical"
getAriaValueText={valuetext}
getAriaValueText={getAriaValueText}
valueLabelDisplay="auto"
defaultValue={30}
/>
Expand All @@ -45,11 +22,34 @@ export default function VerticalSlider() {
<Slider
getAriaLabel={() => 'Temperature'}
orientation="vertical"
getAriaValueText={valuetext}
getAriaValueText={getAriaValueText}
defaultValue={[20, 37]}
valueLabelDisplay="auto"
marks={marks}
/>
</Stack>
);
}

function getAriaValueText(value: number) {
return `${value}°C`;
}

const marks = [
{
value: 0,
label: '0°C',
},
{
value: 20,
label: '20°C',
},
{
value: 37,
label: '37°C',
},
{
value: 100,
label: '100°C',
},
];
20 changes: 13 additions & 7 deletions docs/data/material/components/slider/slider.md
Original file line number Diff line number Diff line change
Expand Up @@ -98,17 +98,23 @@

## Vertical sliders

Set the `orientation` prop to `"vertical"` to create vertical sliders. The thumb will track vertical movement instead of horizontal movement.

Check warning on line 101 in docs/data/material/components/slider/slider.md

View workflow job for this annotation

GitHub Actions / runner / vale

[vale] reported by reviewdog 🐶 [Google.Will] Avoid using 'will'. Raw Output: {"message": "[Google.Will] Avoid using 'will'.", "location": {"path": "docs/data/material/components/slider/slider.md", "range": {"start": {"line": 101, "column": 82}}}, "severity": "WARNING"}

{{"demo": "VerticalSlider.js"}}

**WARNING**: Chrome, Safari and newer Edge versions that is any browser based on WebKit exposes `<Slider orientation="vertical" />` as horizontal ([chromium issue #40736841](https://issues.chromium.org/issues/40736841)).
By applying `-webkit-appearance: slider-vertical;` the slider is exposed as vertical.
:::warning
Chrome versions below 124 implement `aria-orientation` incorrectly for vertical sliders and exposes them as `'horizontal'` in the accessibility tree. ([Chromium issue #40736841](https://issues.chromium.org/issues/40736841))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Chrome versions below 124 implement `aria-orientation` incorrectly for vertical sliders and exposes them as `'horizontal'` in the accessibility tree. ([Chromium issue #40736841](https://issues.chromium.org/issues/40736841))
Chrome versions below 124 implement `aria-orientation` incorrectly for vertical sliders and expose them as `'horizontal'` in the accessibility tree. ([Chromium issue #40736841](https://issues.chromium.org/issues/40736841))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


The `-webkit-appearance: slider-vertical` CSS property can be used to correct this for these older versions, with the trade-off of causing a console warning in newer Chrome versions:

However, by applying `-webkit-appearance: slider-vertical;` keyboard navigation for horizontal keys (<kbd class="key">Arrow Left</kbd>, <kbd class="key">Arrow Right</kbd>) is reversed ([chromium issue #40739626](https://issues.chromium.org/issues/40739626)).
Usually, up and right should increase and left and down should decrease the value.
If you apply `-webkit-appearance` you could prevent keyboard navigation for horizontal arrow keys for a truly vertical slider.
This might be less confusing to users compared to a change in direction.
```css
.MuiSlider-thumb input {
-webkit-appearance: slider-vertical;
}
```

{{"demo": "VerticalAccessibleSlider.js"}}
For Chrome 124 and newer, the slider includes the CSS [`writing-mode`](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_writing_modes/Vertical_controls#range_sliders_meters_and_progress_bars) property that fixes this bug.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users don't have to add this themselves, right? I would remove/rewrite this, otherwise I think people might think they need to provide it themselves.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated!

:::

## Marks placement

Expand Down
6 changes: 6 additions & 0 deletions packages/mui-material/src/Slider/useSlider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -687,6 +687,11 @@ export function useSlider(parameters: UseSliderParameters): UseSliderReturnValue
};
};

let cssWritingMode: 'vertical-rl' | 'vertical-lr' | undefined;
if (orientation === 'vertical') {
cssWritingMode = isRtl ? 'vertical-rl' : 'vertical-lr';
}

const getHiddenInputProps = <ExternalProps extends Record<string, unknown> = {}>(
externalProps: ExternalProps = {} as ExternalProps,
): UseSliderHiddenInputProps<ExternalProps> => {
Expand Down Expand Up @@ -724,6 +729,7 @@ export function useSlider(parameters: UseSliderParameters): UseSliderReturnValue
// So that VoiceOver's focus indicator matches the thumb's dimensions
width: '100%',
height: '100%',
writingMode: cssWritingMode,
},
};
};
Expand Down
1 change: 0 additions & 1 deletion test/regressions/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,6 @@ const blacklist = [
'docs-components-skeleton/Facebook.png', // Flaky image loading
'docs-components-skeleton/SkeletonChildren.png', // flaky image loading
'docs-components-skeleton/YouTube.png', // Flaky image loading
'docs-components-slider/VerticalAccessibleSlider.png', // Redundant
'docs-components-snackbars/ConsecutiveSnackbars.png', // Needs interaction
'docs-components-snackbars/CustomizedSnackbars.png', // Redundant
'docs-components-snackbars/DirectionSnackbar.png', // Needs interaction
Expand Down
Loading