Skip to content

Conversation

@jztang
Copy link
Contributor

@jztang jztang commented Apr 21, 2019

I'd like to work on #15316, as it seems to be inactive. I tried to comment there but didn't get any response, so I'm hoping this PR will get someone's attention. I'm still new to this, so I'd like some guidance if possible.

From what I can tell, the focus is set using handleFocus:
https://github.com/mui-org/material-ui/blob/947853d0fde0d3048f653069bb1febe6dbde9577/packages/material-ui-lab/src/Slider/Slider.js#L315-L317

When the slider thumb is clicked, handleClick is called:
https://github.com/mui-org/material-ui/blob/947853d0fde0d3048f653069bb1febe6dbde9577/packages/material-ui-lab/src/Slider/Slider.js#L323-L329

So to fix this, do I just call handleFocus in handleClick?

Fixes #15316

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 21, 2019

Details of bundle changes.

Comparing: c867c5f...5cc57ab

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core 0.00% 0.00% 311,394 311,394 84,601 84,601
@material-ui/core/Paper 0.00% 0.00% 67,623 67,623 20,124 20,124
@material-ui/core/Paper.esm 0.00% 0.00% 60,988 60,988 19,020 19,020
@material-ui/core/Popper 0.00% 0.00% 31,114 31,114 10,803 10,803
@material-ui/core/Textarea 0.00% 0.00% 5,468 5,468 2,365 2,365
@material-ui/core/TrapFocus 0.00% 0.00% 3,731 3,731 1,565 1,565
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 15,943 15,943 5,777 5,777
@material-ui/core/useMediaQuery 0.00% 0.00% 2,106 2,106 974 974
@material-ui/lab -0.01% -0.00% 140,997 140,978 42,651 42,650
@material-ui/styles 0.00% 0.00% 51,151 51,151 15,148 15,148
@material-ui/system 0.00% 0.00% 11,765 11,765 3,923 3,923
Button 0.00% 0.00% 85,901 85,901 25,732 25,732
Modal 0.00% 0.00% 20,575 20,575 6,603 6,603
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 51,486 51,486 11,342 11,342
docs.main 0.00% 0.00% 648,595 648,595 202,362 202,362
packages/material-ui/build/umd/material-ui.production.min.js 0.00% 0.00% 293,109 293,109 82,525 82,525

Generated by 🚫 dangerJS against 5cc57ab

@eps1lon
Copy link
Member

eps1lon commented Apr 22, 2019

So to fix this, do I just call handleFocus in handleClick?

Better just set the focused state

I'm not sure this solves #15316 though. I would've thought clicking the thumb already focuses it at which point keyboard navigation should "just" work. Maybe it's just an issue with even listeners attached to different targets. Feel free to work on this.

@jztang
Copy link
Contributor Author

jztang commented Apr 28, 2019

So I found out that if I remove event.preventDefault() from handleMouseDown, the focus stays on the slider button after clicking on it, allowing for keyboard manipulation:

https://github.com/mui-org/material-ui/blob/947853d0fde0d3048f653069bb1febe6dbde9577/packages/material-ui-lab/src/Slider/Slider.js#L366-L383

I am not sure if removing event.preventDefault() will break something else, or how we should handle touch devices. When using a touch device, the focus does not stay on the slider button after tapping it, even with this change.

@eps1lon
Copy link
Member

eps1lon commented Apr 29, 2019

@jztang I think this is relevant if the slider is in a swipeable drawer but preventing default/bubbling is only necessary for pointer moves.

Best remove the default prevention and push some code so we can look at the tests or deploy preview.

@eps1lon eps1lon added the scope: slider Changes related to the slider. label Apr 30, 2019
@eps1lon eps1lon added the accessibility a11y label Apr 30, 2019
@jztang jztang marked this pull request as ready for review April 30, 2019 15:02
@jztang

This comment has been minimized.

@joshwooding

This comment has been minimized.

@eps1lon
Copy link
Member

eps1lon commented May 2, 2019

Looks good. The PR that introduced this (#11889) didn't explain why it was necessary. We'll so how it pans out.

@jztang Thanks.

@eps1lon eps1lon merged commit e507497 into mui:next May 2, 2019
@eps1lon eps1lon changed the title [Slider] Save focus on slider after click [Slider] Save focus after click May 2, 2019
@jztang jztang deleted the slider-focus branch May 2, 2019 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accessibility a11y scope: slider Changes related to the slider.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Slider] focus is not saved on slider after click

4 participants