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

[mj12albert]

• Set the CSS writing-mode property in the <input type="range"> element's styles to ensure orientation is exposed correctly in the accessibility tree
• Added more keyboard interaction tests using fireEvent.keyDown, most of the existing tests use fireEvent.change which did not catch some incorrect behaviors
• Handle all keypresses in the keydown handler instead of splitting them across the keydown and change handlers. This takes care of a side effect of using CSS writing-mode to fix the a11y issue – it causes native range inputs to render top-to-bottom in LTR (reference), and bottom-to-top in RTL, which is counter-intuitive 😆 (but its the spec).

Closes #44237

• I have followed (at least) the PR section of the contributing guide.

[mui-bot]

Netlify deploy preview

• docs/data/material/components/slider/slider.md

@material-ui/core: parsed: +0.12% , gzip: +0.14%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against c279aea

[mj12albert]

Fixed the rest of the comments ~

[aarongarciah]

@mj12albert arrow keys are still reversed. Is that expected? e.g. in https://deploy-preview-44537--material-ui.netlify.app/material-ui/react-slider/#vertical-sliders

[mj12albert]

@mj12albert arrow keys are still reversed. Is that expected? e.g. in https://deploy-preview-44537--material-ui.netlify.app/material-ui/react-slider/#vertical-sliders

🥲 nope – let me fix this

Fix for reversed arrow keys is still pending

[aarongarciah]

Behavior wise, it looks good. I left some inline comments.

Do you think any of the introduced changes could be considered breaking? I think this PR has grown to be more than what the PR description states. I'm tagging @DiegoAndai for an additional review.

[aarongarciah]

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))

[aarongarciah]

Ping

[mj12albert]

9f9587c
Fixed ~ @aarongarciah

[aarongarciah]

Do we really need these constants? The values of the constants could be considered constants themselves.

[DiegoAndai]

The values of the constants could be considered constants themselves.

@aarongarciah, what do you mean?

I think handling the key constants like this is something we should've done in Material UI long ago to avoid typos. How is Base UI handling this?

[aarongarciah]

I mean I don't see an improvements when reading

event.key === ARROW_UP

vs

event.key === 'ArrowUp'

The latter is one less file. I also don't see the point on defining these constants at the component level. The downside is potential typos. If we have a convention for this we should just follow the convention and forget my comment. I'm just wary of creating unnecessary abstractions, but that's a personal preference.

[DiegoAndai]

Yes, I would say the only upside of the constants is avoiding typos, and there should be a convention but that's out of scope for this PR. I also agree these shouldn't be defined at the component level.

I would revert this change in this PR, and create an issue to eventually implement the convention throughout the components.

[mj12albert]

We made them all constants, exactly to avoid typos ~ https://github.com/mui/base-ui/blob/master/packages/react/src/composite/composite.ts#L12-L24

[mj12albert]

I believe there's also a performance gain from this, although I don't know how much: https://www.notion.so/mui-org/Performance-guide-memory-allocations-134cbfe7b66080cba325db614278fc20?pvs=4#134cbfe7b6608042afb5f000dca0ab4f

[mj12albert]

I would revert this change in this PR, and create an issue to eventually implement the convention throughout the components.

OK then ~

[aarongarciah]

Looks like some of the tests are being repeated in different blocks. Do you think we need that repetition?

[DiegoAndai]

I agree. For example, if the "Home" and "End" keys have the same behavior on LTR and RTL, we can rely on the LTR tests and not repeat them for RTL.

Do you see some value in the repetition @mj12albert?

[mj12albert]

To make it easier to read and at least appear less repetitive, let's just have one single describe('keyboard interactions'... and organize everything in there instead of spreading it out (this file is very long already): 93083a8

Do you see some value in the repetition

Yes! Better safe than sorry, the combination of direction/orientation/various keys gets confusing 😅
In particular, I think it's important to repeat a bunch of combinations for the when step is null and restricted by marks version of the slider, as it almost becomes a different component

[DiegoAndai]

Regarding:

Do you think any of the introduced changes could be considered breaking?

From the description:

It causes native range inputs to render top-to-bottom in LTR (reference), and bottom-to-top in RTL, which is counter-intuitive 😆 (but its the spec).

I think this will be considered a breaking change for people using RTL 🤔 It might not, but we have had people complain in the past about changing an element's role to fix a11y issues 😅 , and this is a more considerable change.

@aarongarciah @mj12albert can you think of other changes that could be considered breaking?

What do you think about adding it as an opt-in API and making it the default in the next major?

I think this PR has grown to be more than what the PR description states.

Would it be possible to split this into two PRs? One with the writing-mode fix and another with the arrow keys refactor and added tests? This would shield us if we need to revert something; we don't revert everything, only the potential issue.

[mj12albert]

It causes native range inputs to render top-to-bottom in LTR (reference), and bottom-to-top in RTL, which is counter-intuitive 😆 (but its the spec).

I think this will be considered a breaking change for people using RTL

This rendering thing only affects the native <input type="range"> element, which is visually hidden, it does not change how the Material UI Slider component appears

The only (breaking) user-facing change, is that previously if you had a <Slider orientation="vertical">, and was relying on -webkit-appearance to fix the a11y issue, we recommended preventDefault in userland to suppress the "reversed" left/right arrow keys, because the default browser behavior is undesired in most cases.

What do you think about adding it as an opt-in API

So to opt-in to the "unreversed" arrow keys, you would be un-doing preventDefault in userland.

It only be breaking if you were actually using the "reversed" left/right arrow keys before (that our docs were recommending you suppress!) which I would guess most users would not be doing

Does that makes sense? It's confusing, as there are actually 3 different factors to consider (orientation, text direction, "polarity") @DiegoAndai @aarongarciah

Would it be possible to split this into two PRs? One with the writing-mode fix and another with the arrow keys refactor and added tests?

That is OK too, let's do this then ~ the only quirk is that the writing-mode only PR would still have to (temporarily) explain why the left/right arrow keys appear reversed as the reason is now differen – with -webkit-appearance it's because of a browser bug, with writing-mode it's because the spec is odd

[DiegoAndai]

Thanks for the quick response @mj12albert 😊

Let me know if I understand correctly:

This rendering thing only affects the native element, which is visually hidden, it does not change how the Material UI Slider component appears

So, the Material UI Slider will still be rendered bottom-to-top for both LRT and RTL?

It only be breaking if you were actually using the "reversed" left/right arrow keys before (that our docs were recommending you suppress!) which I would guess most users would not be doing

The changes would be:

• For the users who did follow our recommendation, nothing would change (but now they would be able to remove preventDefault if they want as they're not "reversed" anymore)
• For the users who didn't follow our recommendation, the left/right arrow keys would be swapped

Is that right?

the only quirk is that the writing-mode only PR would still have to (temporarily) explain why the left/right arrow keys appear reversed as the reason is now differen – with -webkit-appearance it's because of a browser bug, with writing-mode it's because the spec is odd

Ok, so splitting them is not a great idea either. I was thinking the refactor was completely separate from the fix. With this explanation I would keep both changes in the same PR.

I would revert the constants and open a separate issue to do the same Base UI does, though.

[mj12albert]

So, the Material UI Slider will still be rendered bottom-to-top for both LRT and RTL?

Yes ~ sandbox here just to be sure: https://codesandbox.io/p/sandbox/material-ui-slider-writing-mode-jkw4kn?file=%2Fsrc%2FApp.js%3A1%2C1

The changes would be:

For the users who did follow our recommendation, nothing would change (but now they would be able to remove preventDefault if they want as they're not "reversed" anymore)
For the users who didn't follow our recommendation, the left/right arrow keys would be swapped

Yes ~ thats correct, I've also removed the constants now @DiegoAndai

[mj12albert]

the only quirk is that the writing-mode only PR would still have to (temporarily) explain why the left/right arrow keys appear reversed as the reason is now differen – with -webkit-appearance it's because of a browser bug, with writing-mode it's because the spec is odd

Clarification: I may not have been specific enough here (though this is just to document the fact and doesn't affect the PR now) - with writing-mode, it's ArrowUp/ArrowDown that become reversed, instead of Left/Right, because the spec says it fills from top-to-bottom, "up" is the direction that represents "less" so ArrowUp will decrement the value instead of incrementing it (and same for ArrowDown)

https://codesandbox.io/p/sandbox/material-slider-writing-mode-reversed-up-down-s7x6qf?file=%2Fsrc%2FDemo.tsx%3A34%2C1

[DiegoAndai]

@mj12albert I'm confused, you shared two demos:

1. https://codesandbox.io/p/sandbox/material-ui-slider-writing-mode-jkw4kn?file=%2Fsrc%2FApp.js%3A1%2C1
2. https://codesandbox.io/p/sandbox/material-slider-writing-mode-reversed-up-down-s7x6qf?file=%2Fsrc%2FDemo.tsx%3A34%2C1

In the first one, both sliders increase with the Up Arrow
In the second one, the slider decreases with the Up Arrow

But both have writing-mode: "vertical-lr", what am I missing? 😅

[mj12albert]

In the first one, both sliders increase with the Up Arrow

This first one is with the fixes in this PR!

In the second one, the slider decreases with the Up Arrow

This second one is just a fork of the VerticalAccessibleSlider demo on master, the only change being -webkit-appearance: slider-vertical is replaced with writing-mode, just to show that without the changes in this PR, different keys become reversed for a different reason if only that CSS is changed @DiegoAndai

[DiegoAndai]

This first one is with the fixes in this PR!
This second one is just a fork of the VerticalAccessibleSlider demo on master

I see, it makes sense now 😅.

From the description:

Handle all keypresses in the keydown handler instead of splitting them across the keydown and change handlers

I see that in this PR we're handling all keys in the keydown handler, but I don't see the removal from the change handlers. Am I missing something?

---

@aarongarciah If we released this in a minor version, we would have to add an explanation to the changelog. The breaking change would be that for the users who didn't follow our recommendation of preventing default. For them, the left/right arrow keys would be swapped when updating.

I consider it a 'softer' breaking change as it will only affect people who didn't follow the recommendation. But, there will probably be some users that notice the change and are not happy with it, so if we wanted to play it extra safe, we could hold until v7. Your call.

[DiegoAndai]

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.

[mj12albert]

Updated!

[aarongarciah]

I think we can ship this in a minor.

[mj12albert]

I see that in this PR we're handling all keys in the keydown handler, but I don't see the removal from the change handlers. Am I missing something?

f463c0f
I added a comment there, if you asked as well then it's for sure un-obvious 😅
The change handler handles changes from Pointer and Touch events, it wasn't obvious to me either, thought it wasn't needed until I deleted it and the tests failed

[DiegoAndai]

I added a comment there, if you asked as well then it's for sure un-obvious

I didn't explain my question well enough 😅 here it goes again:

• Before this PR, createHandleHiddenInputKeyDown only handled PageUp, PageDown and shift+arrow changes. So I assume ArrowRight, for example, was handled elsewhere.
• With this PR, createHandleHiddenInputKeyDown is now handling all keys, including the ArrowRight example.
• But I don't see ArrowRight handling being removed from anywhere, so is it being handled in two places?

[mj12albert]

• But I don't see ArrowRight handling being removed from anywhere, so is it being handled in two places?

@DiegoAndai In the keydown handler, preventDefault is called on all the relevant keys:

material-ui/packages/mui-material/src/Slider/useSlider.ts

Lines 357 to 371 in c279aea

	const createHandleHiddenInputKeyDown =
	(otherHandlers: EventHandlers) => (event: React.KeyboardEvent<HTMLInputElement>) => {
	if (
	[
	'ArrowUp',
	'ArrowDown',
	'ArrowLeft',
	'ArrowRight',
	'PageUp',
	'PageDown',
	'Home',
	'End',
	].includes(event.key)
	) {
	event.preventDefault();

So it wouldn't get handled twice (I think lots of tests would have broke if it did)