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

[Slider] Keyboard controls nonfunctional in 4.9.10+ #20645

Closed
2 tasks done
davidcalhoun opened this issue Apr 19, 2020 · 4 comments · Fixed by #20651
Closed
2 tasks done

[Slider] Keyboard controls nonfunctional in 4.9.10+ #20645

davidcalhoun opened this issue Apr 19, 2020 · 4 comments · Fixed by #20651
Labels
bug 🐛 Something doesn't work component: slider This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@davidcalhoun
Copy link
Contributor

davidcalhoun commented Apr 19, 2020

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

After focusing on the Slider component by clicking into it (or tabbing into it), the user cannot change values with keyboard left and right arrow keys.

Expected Behavior 🤔

The user should be able to change the slider value with the keyboard arrow keys.

Steps to Reproduce 🕹

Video demo: https://youtu.be/idLtSp2EKXE

Gif demo:
Kapture 2020-04-19 at 11 18 21

Steps:

  1. Open this forked Slider demo: https://codesandbox.io/s/material-demo-pqblh?file=/package.json
  2. In package.json, change the version of @material-ui/core to 4.9.9 (the last release where keyboard controls function normally)
  3. Click on the Temperature slider in the window on the right, then use the keyboard left/right arrow keys to change the values.
  4. In package.json, change the version of @material-ui/core to 4.9.10 or latest
  5. Try to perform Step 3 again.

Context 🔦

I have a map visualization tied to a slider, and it's handy to be able to hover over map features, revealing a map popup, while the Slider element still has focus. This allows the user to change the slider values (in this case a date slider, going back and forth through time) with the keyboard while the map popup is still visible. Basically an interaction where the mouse is hovered over a different element, while the Slider still has focus.

Your Environment 🌎

Tech Version
Material-UI v4.9.11
React 16.13.0
Browser Firefox 75.0, Chrome 80.0.3987.163
TypeScript
etc.
davidcalhoun added a commit to davidcalhoun/covid-19-map-south-carolina that referenced this issue Apr 19, 2020
@marcosvega91
Copy link
Contributor

Hi @davidcalhoun I think it's a problem with the configuration of your slider because with a simpler slider it works.

Try this
<Slider defaultValue={[20, 30]} />

I will check with yours

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: slider This is the name of the generic UI component, not the React module! labels Apr 19, 2020
@oliviertassinari
Copy link
Member

@davidcalhoun Thanks for the report, it's a regression from #17057.

@oliviertassinari
Copy link
Member

@davidcalhoun What do you think of this fix?

diff --git a/packages/material-ui/src/Slider/Slider.js b/packages/material-ui/src/Slider/Slider.js
index 7bbccea06..ac6603f93 100644
--- a/packages/material-ui/src/Slider/Slider.js
+++ b/packages/material-ui/src/Slider/Slider.js
@@ -100,7 +100,7 @@ function focusThumb({ sliderRef, activeIndex, setActive }) {
     !sliderRef.current.contains(document.activeElement) ||
     Number(document.activeElement.getAttribute('data-index')) !== activeIndex
   ) {
-    sliderRef.current.querySelector(`[data-index="${activeIndex}"]`).focus();
+    sliderRef.current.querySelector(`[role="slider"][data-index="${activeIndex}"]`).focus();
   }

   if (setActive) {

Do you want to submit a pull request :)? We would also need a new test case that fire a click and that assert the activeElement.

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Apr 19, 2020
@marcosvega91
Copy link
Contributor

I can do It if @davidcalhoun don't want :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: slider This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants