fix: selected option is announced without selected state#83996
fix: selected option is announced without selected state#83996neil-marcellini merged 16 commits intoExpensify:mainfrom
Conversation
|
@linhvovan29546 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] |
|
Open PR for the review, I'll check the jest test fail later |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9d831afe0
ℹ️ 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".
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product
linhvovan29546
left a comment
There was a problem hiding this comment.
@daledah We're missing the role here too:
src/components/SelectionList/SelectionListWithSections/BaseSelectionListWithSections.tsx
src/components/SelectionListWithSections/BaseSelectionListWithSections.tsx
Outdated
Show resolved
Hide resolved
|
@linhvovan29546 i updated, please check again |
|
@linhvovan29546 i updated |
src/components/SelectionList/SelectionListWithSections/BaseSelectionListWithSections.tsx
Outdated
Show resolved
Hide resolved
src/components/SelectionListWithSections/BaseSelectionListWithSections.tsx
Outdated
Show resolved
Hide resolved
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@linhvovan29546 i updated, looks like the failing check is not related, i'll merge main to fix it soon |
|
LGTM! @daledah Please add the tests step |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppN/A my emulator gets stuck Android: mWeb ChromeiOS: mWeb Safari |
|
@MelvinBot add test steps please |
|
Here are the suggested test steps for this PR: TestsPrerequisites: Use Chrome + a screen reader (JAWS on Windows, or VoiceOver on macOS). Single-select list — Pronouns picker:
Single-select list — Default Currency picker:
Multi-select list (checkbox/radio) — no regression:
Native platforms (no regression):
Offline tests
QA Steps
|
|
@MelvinBot merge main |
|
I can't merge main into this PR because I didn't open it and I'm not listed as an assignee. To have me perform the merge, add me to the PR assignees and then retry your request. Alternatively, the PR author ( |
neil-marcellini
left a comment
There was a problem hiding this comment.
Looks good thanks.
|
|
||
| const getListboxRole: GetListboxRole = () => undefined; | ||
|
|
||
| // eslint-disable-next-line import/prefer-default-export |
There was a problem hiding this comment.
NAB: Why do we eslint disable instead of just using default exports?
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @neil-marcellini 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.3.37-0 🚀
|
|
Hi @daledah. QA team failed this PR with an original issue, "selected" is still not announced 1773389715753.83996_mac-Fail.mp4 |
|
@IuliiaHerets Could you (or the QA team) test this with Chrome + JAWS? |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.37-10 🚀
|



Explanation of Change
Fixed Issues
$ #74845
PROPOSAL: #74845 (comment)
Tests
Offline tests
Same as tests
QA Steps
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