Fix: Remove accessible={false} blocking Appium automation#78527
Fix: Remove accessible={false} blocking Appium automation#78527AndrewGable merged 4 commits intoExpensify:mainfrom
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
7058d20 to
d40eef5
Compare
rushatgabhane
left a comment
There was a problem hiding this comment.
found some potential accessibility regressions
d40eef5 to
779fb24
Compare
|
@rushatgabhane I addressed you comments with last push, can you please check it? |
| role={getButtonRole(true)} | ||
| isNested | ||
| accessibilityLabel={translate('iou.viewDetails')} | ||
| accessible={false} |
There was a problem hiding this comment.
will this will make the child components inaccessible ?
There was a problem hiding this comment.
No, this will NOT make children inaccessible. In fact, it does the opposite:
- Line 732 sets accessible={false} on the parent PressableWithoutFeedback wrapper
- This is the correct pattern because it allows child components to be individually accessible
- Inside this wrapper, you have interactive children like:
- Lines 784-800: Previous carousel button with accessible and accessibilityLabel={translate('common.previous')}
- Lines 801-821: Next carousel button with accessible and accessibilityLabel={translate('common.next')}
Setting accessible={false} on the parent prevents it from being treated as a single opaque accessibility node, allowing screen readers to traverse into and interact with the child buttons individually. If you
kept the parent as accessible={true}, the children would be hidden from the accessibility tree.
| @@ -1919,7 +1919,7 @@ function PureReportActionItem({ | |||
| preventDefaultContextMenu={draftMessage === undefined && !hasErrors} | |||
| withoutFocusOnSecondaryInteraction | |||
| accessibilityLabel={translate('accessibilityHints.chatMessage')} | |||
| accessible | |||
There was a problem hiding this comment.
in this case this property is by default equal to true, and removing it just removing unnecessary declaration
Address review comments by removing accessible={false} from interactive
elements (buttons, chat messages) and reverting hardcoded accessibility
labels. Restore accessible={false} to decorative checkmarks to prevent
duplicate screen reader announcements.
- Remove accessible={false} from ApprovalWorkflowSection button
- Remove accessible={false} from PureReportActionItem chat messages
- Revert OptionRowLHN accessibilityLabel to original implementation
- Restore accessible={false} to checkmark icons in BaseListItem components
This ensures interactive elements are accessible to both screen readers
and Appium automation while preventing redundant announcements.
779fb24 to
f89872b
Compare
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-02-06.at.9.26.25.PM.mov |
|
🎯 @shubham1206agra, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #81711. |
|
@AndrewGable 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] |
|
🚧 @AndrewGable has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/AndrewGable in version: 9.3.17-0 🚀
|
|
Hi @kirillbilchenko. Do we need QA this? |
|
@IuliiaHerets I don't think so, not sure if it can affect anything in functionality |
|
@kirillbilchenko so can we check it off from the list? |
|
@IuliiaHerets I see that there is some assumption that this pr is causing some issues, so I'm still checking what will be the outcome, to understand why |
| withoutFocusOnSecondaryInteraction | ||
| accessibilityLabel={translate('accessibilityHints.chatMessage')} | ||
| accessible | ||
| accessibilityRole={CONST.ROLE.BUTTON} |
There was a problem hiding this comment.
@kirillbilchenko Sorry for the late question, but why do we need this?
There was a problem hiding this comment.
@shubham1206agra sure, from the guide https://github.com/Expensify/App/blob/main/contributingGuides/ACCESSIBILITY.md
ensure that the pressable component has a role. This is especially important for users with visual disabilities who rely on screen readers to navigate the app. All Pressable components have a accessibilityRole prop that can be used to set the role of the pressable component.
So chat message in the end is interactive element that user need press on to open context menu, and that's the reason
There was a problem hiding this comment.
This line introduced multiple regressions.
#82095
#37447 (comment)
There was a problem hiding this comment.
@situchan could be the case, but this is something mentioned in the guide, so in this case make sense to update the guide or add some additional information about this corner case. This is very good catch in the end, but I'm not sure how this can be prevented in future.
There was a problem hiding this comment.
@NJ-2020 by the way revert will not break this pr, this was added only because I was told to follow accessibility guide that we have in repo, and that's the only reason why it was added, I gave my explanation before, so maybe accessibilityRole={Platform.OS !== 'web' ? CONST.ROLE.BUTTON : undefined} adding fix like this can solve the issue, and worth to add comment.
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.17-9 🚀
|
Summary
This PR fixes accessibility issues that were blocking Appium automation by removing accessible={false} properties from various components and making UI elements properly accessible for automated testing.
Should resolve this #79520
How to verify changes:
Build the application for ios from main and observe the affected components in appium inspector, you will see that MoneyRequestReportPreviewContent for example is only accessible as root container, but not any child components inside this container.
Quick Appium Validation Guide - Main vs ANAD-8910
Setup (One-Time)
Install Appium
npm install -g appium
appium driver install xcuitest
Download Appium Inspector
https://github.com/appium/appium-inspector/releases
Test Main Branch (Baseline)
git checkout main
npm install
cd ios && pod install && cd ..
npx react-native run-ios --simulator="iPhone 15 Pro"
Terminal 1: Start Appium
appium
Terminal 2: Open Appium Inspector
Capabilities:
{
"platformName": "iOS",
"deviceName": "iPhone 15 Pro",
"automationName": "XCUITest",
"app": "/path/to/NewExpensify.app",
"bundleId": "com.expensify.chat.dev"
}
In Appium Inspector, check these and take screenshots:
Chat List:
Money Request:
Test ANAD-8910 Branch (Fixed)
git checkout ANAD-8910-fix-accessibility-issue
npm run clean
npm install
cd ios && pod install && cd ..
npx react-native run-ios --simulator="iPhone 15 Pro"
Restart Appium (Ctrl+C, then start again)
appium
Reopen Appium Inspector with same capabilities
In Appium Inspector, check the same items:
Chat List:
Money Request:
Selection Lists:
Chat Messages:
Key Validation Points
✅ Appium can now identify:
✅ No regressions:
Changes
Fixed Issues
The issue is that some elements are not accessible for automated tests, and there is no issue raised or created, we are working on autotest suite and it can help us to create more reliable element selectors, and cover more scenarios
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 stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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