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.

Sign up for GitHub

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

Jump to bottom

[Slider] New Slider components and hook #373

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

[Slider] New Slider components and hook #373

wants to merge 17 commits into from

Conversation

mj12albert
Copy link
Member

@mj12albert mj12albert commented Apr 29, 2024 •
edited

Closes #216
Closes #76
Closes #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. The step prop defaults to 1 and can no longer be null. (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: [Slider] Arbitrarily restrict the possible values of the slider #412
  • ...

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 [Slider] Issues with adding focus-visible styles to the thumb slot #77 without actually having to move the input
  • ...

Demos

@mj12albert mj12albert added the component: slider This is the name of the generic UI component, not the React module! label Apr 29, 2024
@mj12albert mj12albert force-pushed the feat/slider-rewrite branch 13 times, most recently from dd496d9 to 366c478 Compare May 6, 2024 10:31
@mj12albert mj12albert mentioned this pull request May 8, 2024
Open
@colmtuite

This comment was marked as resolved.

Sign in to view
@mj12albert

This comment was marked as resolved.

Sign in to view
@colmtuite
Copy link

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
Copy link

colmtuite commented May 8, 2024 •
edited

@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.

@mbrookes
Copy link
Member

mbrookes commented May 8, 2024

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

@colmtuite
Copy link

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?

Screen.Recording.2024-05-08.at.23.43.26.mov

@mbrookes

@mbrookes
Copy link
Member

mbrookes commented May 8, 2024

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

@mj12albert
Copy link
Member Author

mj12albert commented May 9, 2024 •
edited

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 79ebc32

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 😅

@colmtuite
Copy link

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.

@mj12albert
Copy link
Member Author

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

@colmtuite
Copy link

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.

@mj12albert mj12albert force-pushed the feat/slider-rewrite branch 2 times, most recently from b4723a8 to 1c97949 Compare May 11, 2024 00:58
@mui-bot
Copy link

mui-bot commented May 11, 2024 •
edited

Netlify deploy preview

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

Generated by 🚫 dangerJS against 6283c3b

atomiks
atomiks reviewed May 23, 2024
View reviewed changes
packages/mui-base/src/Slider/SliderOutput/useSliderOutput.ts Outdated
function useSliderOutput(parameters: UseSliderOutputParameters): UseSliderOutputReturnValue {
const { 'aria-live': ariaLive = 'off', rootRef } = parameters;

const { subitems } = useSliderContext();
Copy link
Contributor

Choose a reason for hiding this comment

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

Hooks should avoid using contexts since they're the responsibility of component authors. Instead they should receive everything as explicit params.

Copy link
Member Author

Choose a reason for hiding this comment

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

Like this? dfd9d44

I feel like this makes the hooks harder to use for hook users e.g. it would expose some internals (e.g. a internal changeValue function, various setStates) that users didn't need to care about at all

Maybe we could use context in the hooks for things like these?
But for things like min, max, orientation etc that are also component props, make them explicit params of the hook

What do you think @atomiks @michaldudak ?

BTW I saw the various Tab hooks also use the context, e.g. useTabsList

Copy link
Contributor

Choose a reason for hiding this comment

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

We could export contexts, which would require hook users to set them up, though it may restrict the component APIs they want to build. Hooks are mostly considered an escape hatch to begin with, so I didn't think passing all those props was too bad.

@mj12albert mj12albert force-pushed the feat/slider-rewrite branch 2 times, most recently from dfd9d44 to 5c1085d Compare May 23, 2024 07:51
@mj12albert
Copy link
Member Author

mj12albert commented May 23, 2024 •
edited

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 mj12albert force-pushed the feat/slider-rewrite branch 6 times, most recently from ee06377 to 70af66c Compare May 24, 2024 07:14
@mj12albert mj12albert mentioned this pull request May 24, 2024
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atomiks atomiks atomiks left review comments

@colmtuite colmtuite Awaiting requested review from colmtuite

@michaldudak michaldudak Awaiting requested review from michaldudak

At least 1 approving review is required to merge this pull request.

Assignees

@mj12albert mj12albert

Labels
breaking change component: slider This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
5 participants
@mj12albert @colmtuite @mbrookes @mui-bot @atomiks