-
Notifications
You must be signed in to change notification settings - Fork 832
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
Feature/accessibility #1503
Feature/accessibility #1503
Conversation
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/mui-org/material-ui-pickers/lhzbsti2x |
@ahayes91 if you could take a look on the deployment preview of this PR to check accessibility changes - you will really help! |
@@ -71,8 +71,8 @@ export const DateTimePickerTabs: React.SFC<DateTimePickerTabsProps> = ({ | |||
className={classes.tabs} | |||
indicatorColor={indicatorColor} | |||
> | |||
<Tab value="date" icon={<>{dateRangeIcon}</>} /> | |||
<Tab value="time" icon={<>{timeIcon}</>} /> | |||
<Tab value="date" aria-label="pick date" icon={<>{dateRangeIcon}</>} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Todo make replaceable
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
Sure, I'd be very happy to test this out - I'll go with a few SR/browser combinations. Overall issues:
VoiceOver + Chrome, MacOS
VoiceOver + Safari, Mac OS
VoiceOver + Safari, iOS
ChromeVox + Chrome, Dell Chromebook
JAWS 2019 + Chrome, Windows 10
NVDA + FireFox, Windows 10
Talkback + Chrome, Moto G6 Android V9
|
@ahayes91 Oh wow, thank you so much for diving deep in the accessibility of the component! This is one of the best reviews I have seen from the community to date! I remember similar excellent feedback grom @ryancogswell :) |
lib/src/views/Calendar/Year.tsx
Outdated
color={selected ? 'primary' : undefined} | ||
children={children} | ||
ref={forwardedRef} | ||
onKeyPress={onSpaceOrEnter(() => onSelect(value))} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What motivates keypress over keydown?
|
||
// prevent any side effects | ||
e.preventDefault(); | ||
e.stopPropagation(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should be cautious about the usage of stopPropagation. I think that it would be great to add a comment on the "why?" we use it here for our future self. https://css-tricks.com/dangers-stopping-event-propagation/
@ahayes91 Thank you so much for your testing! This really helps, I am in progress fixing issues. Wondering do you have twitter? I'd like to tweet about your help with pickers accessiblity 💪 But some things I noticed:
|
@dmtrKovalenko I have used the Pagination as a workbench to experiment around the implementation of the latest "state" guidelines. I think that it's something we can leverage for the days in the calendar. Actually, we could almost use the same logic :D. For instance, it would fix the following issue: consider the selected day, there is no visual clue that it has the focus. We can't distinguish the "selected" to the "selected + focus" state as we can in the WAI-ARIA example. |
Yes indeed - @theaislinnhayes 👍
If you're seeing the same on that example or on another accessible example https://dequeuniversity.com/library/aria/date-pickers/sf-date-picker then I'd say don't worry about it - Chrome's got its own problems anyway given that version 80 is definitely broken with VoiceOver 🙄
I'd be inclined to ditch the mask altogether to be honest. Neither of those few accessible datepicker examples that we have (https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/datepicker-dialog.html or https://dequeuniversity.com/library/aria/date-pickers/sf-date-picker) use one - putting the desired format as a placeholder or in the label would be enough for context for all users, and while you'd save a couple of key presses with a mask, it really does hinder the a11y experience. To me it feels like a case of sacrificing a11y user experience for the sake of convenience for non-SR-users.
Can we update the demo examples to show the proper placeholder, so that developers have a better idea of what should be passed?
👍 - just retested this quickly with VoiceOver on Safari, and hitting Enter on a chosen date in the calendar view is good. The enter button doesn't work on the other buttons though (like previous/next month/ year dropdown) and it also seems like there isn't a way to choose a year from the dropdown either with Space or Enter.... I can retest this again in a few days anyway 💪 |
I agree with @ahayes91, the inputs masks are not a great idea from an accessibility perspective, so they are required to be removed. On the note of browsers detecting screen readers being used, there are much larger issues around it, like privacy and discrimination. The entire standard is built around creating user interfaces and user experiences that work for all. This is why for example keyboard accessibility results in allowing all other input methods used by the disabled communities to work very well, as they emulate the keyboard and thus are backwards compatible with all other user experiences. |
Masks will not be removed from the datepicker. But there is an option to disable masked input |
If we remove the mask support, would we save this dependency 800 B? https://bundlephobia.com/result?p=rifm@0.11.0 |
@oliviertassinari yes, but I have a strong opinion that mask is required |
OK, fair enough. Maybe somebody will open an issue about it, and as the component gain adoption and the issue upvotes, we will re-evaluate in the future? |
Mask will be disabled for any other from en-US locale (example is here https://dev.material-ui-pickers.dev/localization/date-fns). For the other locales it needs to be enabled manually |
Codecov Report
@@ Coverage Diff @@
## next #1503 +/- ##
==========================================
- Coverage 91.61% 90.17% -1.44%
==========================================
Files 60 60
Lines 1502 1649 +147
Branches 243 282 +39
==========================================
+ Hits 1376 1487 +111
- Misses 93 126 +33
- Partials 33 36 +3
Continue to review full report at Codecov.
|
We keep loosing code coverage percentage, my experiments with collecting coverage from cypress tests are not working. Moved experiments to #1507 |
@dmtrKovalenko Great iteration on the accessibility of the component! Well done 😍. I have a couple of feedback based on https://next.material-ui-pickers.dev/demo/datepicker:
This is, for instance, to be compared with jQuery UI (134ms): http://react-day-picker.js.org/ seems to perform great with about 90ms of delay. While jQuery UI feels a bit slow, I think that it would be great to look into the performance of our date picker. Google recommends 100ms as the upper boundary for a great experience :D. |
Have the accessibility improvements for datepicker been released yet? I couldn't figure out if this is linked to any of the latest releases 🤓 |
@ahayes91 not sure if you're still waiting on an answer, but my understanding is that this went out in the 4.x alpha releases. I recently had to upgrade to 4 Alpha 10 to address these a11y issues (and thank you for being so involved in the process!). I picked 10 because it is the last one before requiring an upgrade to Material-UI core 5 Alpha. I wish these changes could've been published in a stable version, but I'm glad to be able to use them now anyway. |
This PR closes #1462
Description
PR is giantly improving accessibility of pickers.