[NoQA] enforce accessibility rules in AI reviewer#85083
[NoQA] enforce accessibility rules in AI reviewer#85083rushatgabhane wants to merge 6 commits intoExpensify:mainfrom
Conversation
|
@QichenZhu Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Tested here. It caught real issues and didn't have false positives |
kacper-mikolajczak
left a comment
kacper-mikolajczak
left a comment
There was a problem hiding this comment.
Thanks a lot for the effort @rushatgabhane put into improving a11y!
I've got couple of questions and I'd love to know your insight.
| ## [A11Y-1] Interactive elements must have accessible labels | ||
|
|
||
| ### Reasoning | ||
|
|
||
| Screen readers (VoiceOver/TalkBack) cannot convey the purpose of an interactive element without a text label. Icon-only buttons, image-only touchables, and components whose visible text is insufficient for context must provide `accessibilityLabel`. Without it, assistive technology announces the element as an unnamed control, making the app unusable for screen reader users. (WCAG 1.1.1, 4.1.2) |
There was a problem hiding this comment.
Following the general priority of "static linting first, ai reviewer last" - looks like this rule might overlap with react-native-a11y/has-valid-accessibility-descriptors we have enabled.
Maybe we could narrow it down, so there are no redundant violations coming both from eslint and reviewer. Thanks!
| <Pressable | ||
| accessibilityRole="button" | ||
| onPress={handleSubmit} | ||
| > |
There was a problem hiding this comment.
Examples use accessibilityRole, but since RN 0.73+ the role prop is the preferred cross-platform equivalent.
Btw, how this one overlaps with react-native-a11y/has-accessibility-props?
| ## [A11Y-10] Respect user text scaling preferences | ||
|
|
||
| ### Reasoning | ||
|
|
||
| Users with low vision rely on system-level font size settings (iOS Dynamic Type / Android Font Size) to enlarge text. Setting `allowFontScaling={false}` or `maxFontSizeMultiplier={1}` disables this, making text unreadable for these users. Layouts must accommodate scaled text using flexible containers (`minHeight`, `flexWrap`) instead of fixed pixel heights. (WCAG 1.4.4) |
There was a problem hiding this comment.
I wonder if we could restrict usage of allowFontScaling={false} or maxFontSizeMultiplier={1} via linter instead of adding new rule. What do you think?
| ## [A11Y-11] Forms must have accessible labels, errors, and instructions | ||
|
|
||
| ### Reasoning | ||
|
|
||
| Screen reader users navigate forms field by field. Each input must be associated with a descriptive label so users know what to enter. While React Native uses `placeholder` as a fallback accessible name, it disappears once the user starts typing, leaving the field unlabeled. An explicit `accessibilityLabel` (or `accessibilityLabelledBy` on Android) persists regardless of input state. Error messages must be announced when they appear using `accessibilityLiveRegion` (Android) and `AccessibilityInfo.announceForAccessibility()` (iOS). (WCAG 1.3.1, 3.3.1, 3.3.2) |
There was a problem hiding this comment.
In ideal scenario, we'd enforce those kind of a11y requirements via component design system which serves as build blocks for most of UI like forms. This would be better in terms of other angles, not just pure a11y. What do you think is needed to be achieved in our app?
| // Disabled button — screen reader doesn't know it's disabled | ||
| <Pressable | ||
| onPress={onSubmit} | ||
| style={isDisabled && styles.disabled} | ||
| disabled={isDisabled} | ||
| > | ||
| <Text>Submit</Text> | ||
| </Pressable> |
There was a problem hiding this comment.
Aren't props like disabled directly internally linked to a11y state and setting both would be redundant?
| ## [A11Y-8] Manage focus for modals and overlays | ||
|
|
||
| ### Reasoning | ||
|
|
||
| When a modal, bottom sheet, or popover opens, screen reader users can still navigate to content behind it unless focus is trapped inside the overlay. Platform-specific handling is required: | ||
|
|
||
| - **iOS/Web**: Use `accessibilityViewIsModal={true}` on the modal container — VoiceOver ignores sibling views, and React Native Web maps it to `aria-modal`. | ||
| - **Android**: Use `importantForAccessibility="no-hide-descendants"` on the background content to hide it from TalkBack. | ||
|
|
||
| Focus should move to the modal's first interactive element on open and return to the trigger on close. (WCAG 2.4.3, 2.1.2) |
There was a problem hiding this comment.
This, similar to forms remark, I think would be best to be resolved on design system level (either implicitly or enforcing correct props).
2 participants
Explanation of Change
Note: I used claude to generate this PR
Tested here
Added 12 accessibility review rules
Rules enforce WCAG 2.2 AA compliance for React Native, covering: accessible labels, semantic roles, state communication, touch target sizes, dynamic content announcements, image accessibility, color-only information, modal focus management, drag alternatives, text scaling, form accessibility, and logical focus order
Fixed Issues
$ #85082
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari