-
Notifications
You must be signed in to change notification settings - Fork 24
♿ Add VoiceOver and TalkBack Accessibility Support #72
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
base: develop
Are you sure you want to change the base?
♿ Add VoiceOver and TalkBack Accessibility Support #72
Conversation
⚙️ upgrade yarn to v4.11.0 (for compatibility with OIDC)
Release/v2.3.1
Release/v2.4.0
|
Hey @JasperVercammen, thank you for putting this PR together. Will review ASAP 🙌 |
troberts-28
left a comment
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.
Thanks again for the PR @JasperVercammen. There's a little bit of work to do to get this release-ready - have left you some comments. It's also be great make this complete by adding the little bits of accesibility support needed for TimerPickerModal (roles and labels for the modal components, announcement for the modal etc.)
| const announcement = formatValue | ||
| ? formatValue(newValue) | ||
| : String(newValue); | ||
| AccessibilityInfo.announceForAccessibility(announcement); |
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's the reason for not using announceForAccessibilityWithOptions with queue: false for decrementing (as for incrementing?)
| isScreenReaderEnabled | ||
| ? { | ||
| text: formatValue | ||
| ? formatValue(latestDuration.current) |
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.
This is memoized so this won't receive the latest value of this ref - you'll end up with a stale value here I think
| const TimerPicker = forwardRef<TimerPickerRef, TimerPickerProps>( | ||
| (props, ref) => { | ||
| const { | ||
| accessibilityLabel, |
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.
Doesn't look like this is used?
| hint?: string; | ||
| hours?: string; | ||
| minutes?: string; | ||
| picker?: string; |
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.
Not sure this is used anywhere?
| viewabilityConfigCallbackPairs={ | ||
| viewabilityConfigCallbackPairs | ||
| accessibilityRole={ | ||
| isScreenReaderEnabled ? "adjustable" : undefined |
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'm not sure we all need all of these ternaries? I think a lot of these are harmless even if there's no screen reader so would be cleaner to just set them. Not certain but looks as though the only two that actually matter are: importantForAccessibility="no-hide-descendants" and accessibilityElementsHidden
|
|
||
| // Format functions for accessibility announcements | ||
| const formatDayValue = (value: number) => | ||
| padDaysWithZero ? padNumber(value) : String(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.
These should probably include a label right (e.g. 5 days)? Otherwise the announcement would just say the number, which doesn't make it clear which picker the value refers to. I'm also not sure that we need to pad the value with a zero here; that's a visual concern and not really of any use to someone using a screen reader
| const formatDayValue = (value: number) => | ||
| padDaysWithZero ? padNumber(value) : String(value); | ||
|
|
||
| const formatHourValue = (value: number) => { |
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.
These format fns are all pure functions so should be moved out of the component (otherwise these will be recreated on every render, breaking the memoization of DurationScroll. I think we could probably combine them into a single formatAccessibilityValue utility fn
| onMomentumScrollEnd={onMomentumScrollEnd} | ||
| onScroll={onScroll} | ||
| renderItem={renderItem} | ||
| scrollEnabled={!isDisabled} |
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.
Think we probably need: accessibilityState={{ disabled: isDisabled }}
|
|
||
| **Internationalization:** | ||
|
|
||
| You can customize the accessibility labels for internationalization: |
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.
Accessibility labels aren't for internationalization though?
| ref={flatListRef} | ||
| contentContainerStyle={ | ||
| styles.durationScrollFlatListContentContainer | ||
| <View |
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.
Adding this extra View is potentially problematic. It could well interfere with people's layout — it has no style prop, so it'll use default flex behaviour and users can't target it with styling. There's already a wrapper View in this component in the return, I think we can just move these accessibility props there.
♿ Add VoiceOver and TalkBack Accessibility Support
Summary
Implements comprehensive screen reader accessibility for the timer picker component, enabling VoiceOver (iOS) and TalkBack (Android) users to interact with all picker columns using native swipe gestures.
What's Changed
New Files
src/utils/useScreenReaderEnabled.ts- Hook to detect screen reader state with automatic change detectionModified Files
src/components/DurationScroll/types.ts- Added accessibility-related propssrc/components/DurationScroll/DurationScroll.tsx- Implemented accessible picker with adjustable rolesrc/components/TimerPicker/types.ts- Added accessibility label propssrc/components/TimerPicker/TimerPicker.tsx- Integrated screen reader detection and passed accessibility propssrc/index.ts- ExporteduseScreenReaderEnabledhookREADME.md- Documented new accessibility features and propsFeatures
✅ Native Screen Reader Support
✅ Immediate Audio Feedback
padWithZero, 12-hour format, etc.)✅ Conditional Behavior
✅ Internationalization Ready
New Props
TimerPicker/TimerPickerModalaccessibilityLabel?: string- Label for entire pickeraccessibilityLabels?: { days, hours, minutes, seconds, picker, hint }- Per-column labelsTechnical Implementation
AccessibilityInfoAPI to detect VoiceOver/TalkBack stateaccessibilityRole="adjustable"importantForAccessibility="no-hide-descendants"accessibilityElementsHidden(iOS) andimportantForAccessibility(Android)Breaking Changes
None - fully backward compatible. Default behavior unchanged when screen readers are disabled.
Testing
Manual Testing Required:
Automated Tests:
All existing tests pass. Accessibility features don't interfere with test queries (default
isScreenReaderEnabled={false}).Documentation
Updated README with:
useScreenReaderEnabledhook in custom components