fix: current active menu button not getting announced in settings profile page#85193
fix: current active menu button not getting announced in settings profile page#85193mollfpr merged 4 commits intoExpensify:mainfrom
Conversation
|
@aimane-chnaif 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] |
|
I added screenshots of web platform only because for native platforms(narrow layout), we don't show the user the selected menu item and the details page in the same view. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1405b9bc1
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
No product review needed |
yes, we should fix other pages too |
src/components/MenuItem.tsx
Outdated
| role={interactive ? role : undefined} | ||
| accessibilityLabel={`${enhancedAccessibilityLabel}${brickRoadIndicator ? `. ${translate('common.yourReviewIsRequired')}` : ''}`} | ||
| accessible={shouldBeAccessible} | ||
| accessibilityState={{selected}} |
There was a problem hiding this comment.
Introduce new prop like isTab (or you can find better name)
accessibilityState={isTab ? {selected: isFocused} : undefined}
role={isTab ? CONST.ROLE.TAB : undefined}
There was a problem hiding this comment.
Should we only pass the prop role=CONST.ROLE.TAB to menuitem and do like this
accessibilityState={role === CONST.ROLE.TAB ? {selected: focused} : undefined}For role prop we already have a condition here:
App/src/components/MenuItem.tsx
Line 797 in 0a415ed
so i don't think we should do isTab for this @aimane-chnaif
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
Some behaviors different from production:
web.movI am not sure if it's fine. @rushatgabhane can you please confirm? |
|
@MelvinBot why could that be |
|
Both issues stem from how react-native-web's 1. Space key doesn't work with
|
I mean fair enough. If QA wants it to be tab then we can't support space key |
|
ok, agree not blocker |
|
sounds good! @aimane-chnaif please watch out for |
|
🚧 @mollfpr 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mollfpr in version: 9.3.41-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.41-4 🚀
|

Explanation of Change
Added accessibility attribute (selected) for menu buttons in settings profile page, so that selected menu buttons are properly announced as selected to the user.
Fixed Issues
$#75556
PROPOSAL:#75556 (comment)
Tests
Prerequisites:
Test 1:
Test 2:
Test 3:
Precondition: Have some workspace already created.
Offline tests
Same as tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
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
macos.web.mp4