Skip to content

[Slider] Move focus to correct thumb when using keyboard #731

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

Merged
merged 4 commits into from
Jun 22, 2021

Conversation

jjenzz
Copy link
Contributor

@jjenzz jjenzz commented Jun 21, 2021

Fixes #730

  • Thumb focusing logic fires every time value changes (this fixes Home and End keypress too)
  • Updated to use PointerEvents instead which meant I could remove some setTimeout logic
  • Renamed SliderPart to SliderImpl for consistency

There is the following outstanding issue which I will fix in a separate PR:

CleanShot.2021-06-21.at.13.22.51.mp4

When the thumb reaches the same value as another thumb, I have to press the arrow one extra time than I should need to for it to move.

const target = event.target as HTMLElement;
target.setPointerCapture(event.pointerId);
// Prevent browser focus behaviour because we focus a thumb manually when values change.
event.preventDefault();
Copy link
Contributor Author

@jjenzz jjenzz Jun 21, 2021

Choose a reason for hiding this comment

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

This change allowed me to remove the setTimeout stuff from here. We couldn't do this before because of this mouse event comment, but we're no longer using mouse events.

event.preventDefault();
// Touch devices have a delay before focusing so won't focus if touch immediately moves
// away from target (sliding). We want thumb to focus regardless.
if (context.thumbs.has(target)) {
Copy link
Contributor Author

@jjenzz jjenzz Jun 21, 2021

Choose a reason for hiding this comment

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

Checking context instead of using the isThumb method. This allowed me to remove the data-radix-slider-thumb attribute that we had left over.

@benoitgrelard
Copy link
Contributor

Love all the cleanup!
Wondering if these changes also fix #615 possibly?

@jjenzz
Copy link
Contributor Author

jjenzz commented Jun 21, 2021

Wondering if these changes also fix #615 possibly?

I wondered that too but we're unable to replicate the issue locally. When we release this I'll pull into that repo that I have locally and check.

@jjenzz
Copy link
Contributor Author

jjenzz commented Jun 21, 2021

In fact, if I run a build and copy paste build output into that repo I can verify that way. Will give that a go.

"@radix-ui/react-use-controllable-state": "workspace:*",
"@radix-ui/react-use-direction": "workspace:*",
"@radix-ui/react-use-layout-effect": "workspace:*",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use this package in collection.tsx that was moved into Slider a while back but we forgot to add the dep.

@jjenzz
Copy link
Contributor Author

jjenzz commented Jun 21, 2021

Wondering if these changes also fix #615 possibly?

Nope, looks like this PR actually makes it worse and I can't figure out why snowpack is reinstantiating the thumb, it's so weird 😬

This change fixes it but I don't understand why we would have to do that:

image

When I do some console.logs, the value never changes to undefined so I can't work out why it would reinstantiate.

@jjenzz jjenzz merged commit fda73e4 into main Jun 22, 2021
@jjenzz jjenzz deleted the fix-slider-key-focus branch June 22, 2021 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Slider] Focus doesn't jump to next thumb when crossing in multiple slider type
2 participants