fix: Hide from Assistive Tech and Remove Interactive Role#79590
fix: Hide from Assistive Tech and Remove Interactive Role#79590Uzaifm127 wants to merge 10 commits intoExpensify:mainfrom
Conversation
|
@dominictb 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] |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3e2e0e659
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
TS check is failing. |
|
Fixing... |
|
@dominictb Fixed. |
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
|
@Uzaifm127 please fix conflicts, @dominictb lemme know if rest looks okay? |
|
Fixed the conflicts. |
|
I'm checking this. Should be done today |
src/components/MenuItem.tsx
Outdated
| {(isHovered) => ( | ||
| <PressableWithSecondaryInteraction | ||
| onPress={shouldCheckActionAllowedOnPress ? callFunctionIfActionIsAllowed(onPressAction, isAnonymousAction) : onPressAction} | ||
| // eslint-disable-next-line react/jsx-props-no-spreading |
There was a problem hiding this comment.
Let's not disable lint unless we had a very specific reason to do so. If you had one, please add a code comment here explaining.
src/components/MenuItem.tsx
Outdated
| accessible={shouldBeAccessible} | ||
| tabIndex={tabIndex} | ||
| role={interactive ? role : undefined} | ||
| // eslint-disable-next-line react/jsx-props-no-spreading |
src/components/MenuItem.tsx
Outdated
| : { | ||
| accessible: false, | ||
| })} | ||
| focusable={interactive} |
There was a problem hiding this comment.
Do we need to set this prop when we already had tabIndex?
dominictb
left a comment
There was a problem hiding this comment.
Please add recordings. You don't have to add recordings for all cases, just the original reported places.
Also please update the QA Steps section to only cover 1-2 places. It's great to see your Tests section should cover all manual test cases but please leave 1-2 cases only for QA Steps to help testing this PR easier.
|
@Valforte Can you run test builds for this? |
|
Building |
|
🚧 @Valforte has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
@dominictb Thanks for pointing out. Checking... |
|
|
|
Extremely sorry everyone, Closed the PR. |

Explanation of Change
Two accessibility issues:
interactive={false}were still receiving accessibility props (role, accessibilityLabel, accessible) that made screen readers announce them as actionable elements.Fixed Issues
$ #79243
$ #77497
$ #77496
PROPOSAL: #79243 (comment)
Tests
First issue: Toggle/checkbox labels announced twice
Second issue: Plain text elements announced as actionable
Offline tests
N/A - These are accessibility attribute changes that don't affect offline behavior or network requests.
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 method[STYLE.md](http://style.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
N/A - Accessibility changes
Android: mWeb Chrome
N/A - Accessibility changes
iOS: Native
N/A - Accessibility changes
iOS: mWeb Safari
N/A - Accessibility changes
MacOS: Chrome / Safari
N/A - Accessibility changes