base-ui icon indicating copy to clipboard operation
base-ui copied to clipboard

[Slider] New Slider components and hook

Open mj12albert opened this issue 1 year ago • 15 comments

Closes https://github.com/mui/base-ui/issues/216 Closes https://github.com/mui/base-ui/issues/76 Closes https://github.com/mui/base-ui/issues/77

Major changes

  • Dropped the valueLabel slot/component, and related features
  • The rail slot/component previously referred to the full length of the slider. This is now renamed to instead be the track
  • The previous track component referred to the filled portion of the slider "bar". This component is dropped, but can be easily implemented using a new useSliderContext hook
  • The API does not expose the thumb's input element anymore, it's automatically wired to the corresponding thumb (a span by default), and allows for styling the thumb directly with :focus-visible
  • The Mark component/slot, and the marks prop will be dropped. (TODO open a new issue to redesign marks support)
  • The restricted values feature will be redesigned, and decoupled from any props related to the visual marks. (TODO open a new issue to track this)
  • ...

Summary

  • Except for the major change above, the new API
  • All relevant tests from material-ui have been ported over, a few new ones were added
  • The render prop for the Thumb component may be a special case, as it contains a "hidden" input
  • On the final location of the input - it's kept as a child of the thumb (for now) because we could solve https://github.com/mui/base-ui/issues/77 without actually having to move the input
  • ...

Demos

  • Basic/Range: https://deploy-preview-373--base-ui.netlify.app/experiments/slider/
  • Vertical: https://deploy-preview-373--base-ui.netlify.app/experiments/slider-vertical/
  • RTL: https://deploy-preview-373--base-ui.netlify.app/experiments/slider-rtl/
  • Inverted fill: https://deploy-preview-373--base-ui.netlify.app/experiments/slider-inverted-fill/
  • With <label> & aria-labelledby: https://deploy-preview-373--base-ui.netlify.app/experiments/slider-label/
  • increasing the Track area with a pseudo element vs an extra element: https://deploy-preview-373--base-ui.netlify.app/experiments/slider-track-padding/
  • gradient (heavily customized): https://deploy-preview-373--base-ui.netlify.app/experiments/slider-gradient/

mj12albert avatar Apr 29 '24 10:04 mj12albert

@mj12albert I had a quick look there and noticed the thumb still shifts to the right/left when you click the thumb.

https://github.com/mui/base-ui/assets/805073/bb0d917c-7ded-4acb-b090-ed8600e7a954

colmtuite avatar May 08 '24 08:05 colmtuite

@mj12albert I had a quick look there and noticed the thumb still shifts to the right/left when you click the thumb.

@colmtuite I updated the (other) issue before actually pushing the commit 😅 pls try again!

mj12albert avatar May 08 '24 10:05 mj12albert

Ah ok. Yeah that issue seems to be resolved.

This might be unrelated, but when I click the track, the thumb isn't centered with the pointer. The thumb is shifted slightly to the right of center. I'm guessing this issue is because the thumb is 20px but the margin-left value is -6px? Changing the margin-left value to -10px seems to resolve this issue.

colmtuite avatar May 08 '24 10:05 colmtuite

@mj12albert I pushed a commit to update the CSS to be simpler, fix a few bugs, and not rely on left, margin-bottom, or fixed values to position the thumb.

Additional notes:

  • Consider rendering the <input> as a sibling of the track rather than a child of the thumb, so we can avoid having to use focus-within? Let's chat about this before changing the code.
  • Change data-active to data-dragging?
  • Add the data-disabled attr to the track, fill, and thumb, so we can style those components directly without using :has or descendant selectors.
  • Root should have role="group", and then use aria-labelledby to connect the Output.
  • In vertical mode, when you press and hold the thumb, then move the pointer 1px up or down, the thumb jumps a large amount. Also, when you're dragging in vertical mode, the thumb becomes misaligned with the pointer.

colmtuite avatar May 08 '24 12:05 colmtuite

From a quick play I found a few issues:

Slider:

  • After clicking a thumb it retains its focus, even after the mouse is released.
  • Clicking anywhere other than the left third of the thumb causes it to jump to the right.

Gradient demo:

  • Trying to move a thumb with the keyboard after it has been clicked causes additional thumbs to be added. (Fixing the first point above will probably solve that.)

mbrookes avatar May 08 '24 22:05 mbrookes

After clicking a thumb it retains its focus, even after the mouse is released.

This is because the <input /> inside the Slider receives focus when you click the thumb, track, or tab to it. The demo uses :focus-within to apply the focus ring when the input is focused. We can't use focus-visible because the thumb is a <span> and can't receive focus directly. This is one reason I suggested having the input be a sibling of the track instead in my previous comment.

Clicking anywhere other than the left third of the thumb causes it to jump to the right.

I'm not seeing this in Chrome on Mac. Video attached. Which browser/platform are you using?

https://github.com/mui/base-ui/assets/805073/ed85a90e-dca0-4b9b-86a1-6773d8ea93d6

@mbrookes

colmtuite avatar May 08 '24 22:05 colmtuite

This is one reason I suggested having the input be a sibling of the track instead in my previous comment.

Sorry, there were no existing comments when I wrote mine, and only just submitted it without first refreshing the page.

I'm not seeing this in Chrome on Mac.

Looks like it was since fixed in ebdb65b.

Here's a new one. Vertical thumb alignment is broken in Chrome on Mac:

image

mbrookes avatar May 08 '24 23:05 mbrookes

In vertical mode, when you press and hold the thumb, then move the pointer 1px up or down, the thumb jumps a large amount. Also, when you're dragging in vertical mode, the thumb becomes misaligned with the pointer.

I will fix this - I think it's a side effect of fixing the thumb shift!

After clicking a thumb it retains its focus, even after the mouse is released.

@colmtuite Without changing the structure for now, I've updated it so the input is blurred on the Thumb's pointerup https://github.com/mui/base-ui/pull/373/commits/79ebc3271884f2c79aa66ceb2610c02e0ab7ba6a

Here's a new one. Vertical thumb alignment is broken in Chrome on Mac:

@mbrookes Fixed ~ it was just a stray css property in the demo!

Trying to move a thumb with the keyboard after it has been clicked causes additional thumbs to be added

Will continue debugging this - I know the customizations in this demo created a lot of weird issues with the keyboard behavior 😅

mj12albert avatar May 09 '24 08:05 mj12albert

Without changing the structure for now, I've updated it so the input is blurred on the Thumb's pointerup

@mj12albert Why did you do this? It's a major a11y flaw now.

colmtuite avatar May 09 '24 08:05 colmtuite

Without changing the structure for now, I've updated it so the input is blurred on the Thumb's pointerup

@mj12albert Why did you do this? It's a major a11y flaw now.

Just wanted to make a quick fix to the demo... I have reverted it

mj12albert avatar May 09 '24 11:05 mj12albert

Just wanted to make a quick fix to the demo... I have reverted it

Ok. Yeah when you click anywhere on the Slider, it should become focused. That's just how it works. Same if you tab to the Slider. I understood that comment to be referring to the focus style, which we could hide if we could use :focus-visible instead of :focus-within. We can't do that with the current set-up. But we can't prevent the Slider from receiving focus.

colmtuite avatar May 09 '24 15:05 colmtuite

Netlify deploy preview

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

Generated by :no_entry_sign: dangerJS against 37059b8865082d83d4f7566ca0b418eec0f07c37

mui-bot avatar May 11 '24 01:05 mui-bot

Root should have role="group", and then use aria-labelledby to connect the Output.

@colmtuite Did you mean input instead of output? I think aria-labelledby should connect Root<>Label and Input<>Label; and the output is connected to the input(s) using for, like in this example: https://deploy-preview-373--base-ui.netlify.app/experiments/slider-label/

(source: https://github.com/mui/base-ui/blob/2e5975b07beb3cb798d50cccc56551e20fa2c670/docs/pages/experiments/slider-label.tsx)

BTW the other points raised in https://github.com/mui/base-ui/pull/373#issuecomment-2100479210 should be fixed now (aside from moving the <input>)

mj12albert avatar May 13 '24 09:05 mj12albert

Bug: In the gradient slider demo, if you focus a thumb and then press ArrowLeft/ArrowRight, it creates new thumbs and undefined values appear.

atomiks avatar May 23 '24 05:05 atomiks

Bug: when focusing a thumb in the basic range demo, once the thumb moves past the other one, its focus ring style disappears.

atomiks avatar May 23 '24 05:05 atomiks

Bug: when focusing a thumb in the basic range demo, once the thumb moves past the other one, its focus ring style disappears.

This is tricky... when the thumbs "cross", the first thumb blurs and stops, and the second thumb gains focus and starts moving, even though it looks like the first thumb is being dragged past the second thumb (which never moves), so the :focus-visible state (and styles) doesn't get "transferred" to the second thumb

Because this causes the blur/focus handlers to fire on the Thumb, it could lead to strange and unpredictable behaviors when passing onBlur/onFocus to the Thumb. Maybe this could be worked around by using preventDefault somehow, but it wouldn't solve the issue with the :focus-visible state.

If I'm not mistaken, I don't think :focus-visible can be triggered with programmatic focus. The only idea I have right now is to override the internal sorting of useCompoundParent here (by DOM order) and/or the slider (by value), and separately track the "perceived drag" order of the thumbs in a new state if that makes any sense.

The old implementation got around this by manually tracking a focusVisible state and using a class as a style hook. What do you think if we had to fall back to doing this but with a data attribute? @colmtuite

mj12albert avatar May 23 '24 09:05 mj12albert

@mj12albert

General

  • "The previous track component referred to the filled portion of the slider "bar". This component is dropped, but can be easily implemented using a new useSliderContext hook" We cannot drop this feature, it's important for the UX. How can we bring it back? Do we need to add a Slider.Range component?
  • ✅ The components are missing the data-orientation attr. Every component except the thumb should have this attr.
  • ✅ What is the “distance” in the minDistanceBetweenValues prop? Is it a step? If so, we should replace “Distance” with “Step” in the prop name. Otherwise, it’s confusing, you might think it’s px.
  • ✅ I think minDistanceBetweenValues should default to 0.
  • If you set minDistanceBetweenValues to 5. Then drag the first thumb to value 0, and drag the second thumb to value 5. Then edit the minDistanceBetweenValues value, weird things happen. Play around with this a bit and you should notice the weirdness? It’s not necessarily an issue, because the prop seems to work fine. Just wondering why the experience is weird?

a11y

  • When you focus the slider, VO announces “You are currently on a group”. I think it should announce “You are currently on a slider”?
  • ✅ The “role=group” element is better as a <div> rather than a <span>?
  • Any slider label should be a <label>. In the range slider examples, the label is a <span>.
  • ✅ When you focus the slider, the <label> content is not announced by the SR because the “role=group” element is not labelledby the <label>.
  • Adding aria-label=“start” and aria-label=“end” to the range slider inputs might help with SR announcements. We want to have the screenreader announce “start range” and “end range” when you focus a thumb in a range slider. This might cause issues if people add more than 2 thumbs, unless we get fancy? Not sure we should address this problem in this first version. Thoughts?

Docs

  • ✅ The vertical orientation demo in the docs is not much use without styling. We should add the styles to the code block.
  • ✅ Remove the disabled example from the hero demo to keep it simple and reduce the code.
  • In the “Overlapping values” section, it says “In the below example, the thumbs cannot be moved further towards each other” but there is no example below.
  • ✅ The CSS is dodgy in the vertical orientation example. Clicking to the left or right of the gray track doesn’t activate the slider, you must click exactly on the gray track. Also, when you set the value to 100, it looks like the centre of the thumb has gone beyond the end of the track.
  • ✅ We’re not currently demoing our hooks in other components, or demoing how to create custom components or build on top of our components. So I think the “Custom subcomponents” section is out of place. We should probably remove it for now, and document it later, when we get around to shipping a solution for marks.

colmtuite avatar Jun 10 '24 06:06 colmtuite

When you focus the slider, VO announces “You are currently on a group”. I think it should announce “You are currently on a slider”?

I've done some more testing with VO, it announces the group first, then announces "...currently on a slider" when you arrow to the thumb which has the actual 'slider' role

https://github.com/mui/base-ui/assets/4997971/92e84915-2c58-4616-b9ea-944675a5b117

Adding aria-label=“start” and aria-label=“end” to the range slider inputs might help with SR announcements.

aria-label doesn't announce, but setting aria-valuetext does, currently there is a getAriaValueText prop on the thumb for this:

<Slider.Thumb getAriaValueText={(thumbValue) => `start range ${thumbValue}`} />

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

We could set this as the default if the user doesn't explicitly provide aria-valuetext, or just document it as a recommendation in the docs - what do you think? @colmtuite

mj12albert avatar Jun 14 '24 10:06 mj12albert

@mj12albert

  • When you focus a single thumb slider, VO announces “[label], group, [value], group”. I think it should announce “[label], slider, [value], group” or “[value], [label], slider, [label], group”?
  • In a range slider, when you move focus from the first thumb to the second thumb, VO announces “end range, [value], group”. I think it should announce “[value], end range, slider”.
  • What is the scale prop on Root?

colmtuite avatar Jun 27 '24 08:06 colmtuite

In a range slider, when you move focus from the first thumb to the second thumb, VO announces “end range, [value], group”. I think it should announce “[value], end range, slider”.

I've updated this so it announces "value" before "start/end range", it's announcing correctly as "slider" for me and not group though, I think this is just a VO quirk:

https://github.com/mui/base-ui/assets/4997971/18e92f84-9c10-4638-bbe7-5f733236eec2

When you focus a single thumb slider, VO announces “[label], group, [value], group”. I think it should announce “[label], slider, [value], group” or “[value], [label], slider, [label], group”?

You mean when VO focuses on the Root and not the Thumb right? I don't think it's possible to get it to announce "slider" unless it explicitly has the slider role (which is obviously incorrect). It also doesn't announce "group" like that for me, it could also be a VO issue:

https://github.com/mui/base-ui/assets/4997971/0e13fea5-7d02-4d67-b5ff-36808d55c377

What is the scale prop on Root?

The scale prop enables a non-linear scale (a feature from Material UI): https://mui.com/material-ui/react-slider/#non-linear-scale We could add a demo for this, unless you want to get rid of it? (Or are you just asking)

@colmtuite

mj12albert avatar Jun 28 '24 04:06 mj12albert

@mj12albert Ok on the a11y stuff 👍

On scale, I was just asking what it was. I read the MUI docs just now and still not sure what it does? 😅

colmtuite avatar Jun 28 '24 14:06 colmtuite

I read the MUI docs just now and still not sure what it does? 😅

Not sure if I can explain it better than the docs but let me try 😓 :

First set up a slider like this: (omitted subcomponents for simplicity)

<Slider.Root min={5} step={1} max={10} defaultValue={5} />

The possible values are: 5, 6, 7, 8,9, 10

Now we add the scale prop, which is a function that can transform the slider value.

<Slider.Root min={5} step={1} max={10} defaultValue={5} scale={(value) => 2 ** value } />

So now the default value of 5 will become 2^5 = 32 And the possible values of the slider become 2^5, 2^6, 2^7... 2^10 instead, for example to make a slider that lets you pick values among: 32kb, 64kb, 128kb, 256kb, 512kb like the Material UI demo

Does this make sense? Maybe it's the name scale that is unclear 🤔 @colmtuite

mj12albert avatar Jul 02 '24 05:07 mj12albert

@mj12albert I understand now, thanks for explaining. Let's remove this functionality+prop for now.

colmtuite avatar Jul 02 '24 07:07 colmtuite

@mj12albert I understand now, thanks for explaining. Let's remove this functionality+prop for now.

OK I've removed it

mj12albert avatar Jul 02 '24 08:07 mj12albert