Skip to content

fix: announce selected state in accessibility label for Chrome+VoiceOver#87122

Open
Krishna2323 wants to merge 12 commits intoExpensify:mainfrom
Krishna2323:krishna2323/issue/74875
Open

fix: announce selected state in accessibility label for Chrome+VoiceOver#87122
Krishna2323 wants to merge 12 commits intoExpensify:mainfrom
Krishna2323:krishna2323/issue/74875

Conversation

@Krishna2323
Copy link
Copy Markdown
Contributor

@Krishna2323 Krishna2323 commented Apr 3, 2026

Explanation of Change

Fixed Issues

$ #74875
PROPOSAL:

Tests

Setup

  1. Open the app in Chrome on macOS at https://dev.new.expensify.com:8082/
  2. Log in and navigate to Settings > Preferences > Language

Test 1: Selected item announces "selected" (Chrome + VoiceOver)

  1. Turn on VoiceOver (Cmd + F5)
  2. Navigate to the selected item using VO + Down Arrow or Tab
  3. Expected: VoiceOver announces the item label followed by "current item" (e.g., "English")
  4. Navigate to a non-selected item
  5. Expected: VoiceOver announces the item label without "current item"

Test 2: No double "selected" announcement (Safari + VoiceOver)

  1. Open the same Language page in Safari
  2. Turn on VoiceOver (Cmd + F5)
  3. Navigate to the selected pronoun
  4. Expected: VoiceOver announces "current item" only once

Test 3: Arrow key navigation

  1. In either browser with VoiceOver on, use arrow keys to move through the list
  2. Expected: Focus highlight moves visually and VoiceOver reads each item; the currently selected item includes "current item" in its announcement

Test 4: Multi-select lists are unaffected

  1. Navigate to a page that uses a multi-select SelectionList (e.g., Settings > Workspaces > [workspace] > Members and use the select-all / checkbox mode)
  2. Expected: Checkboxes still announce "checked" / "not checked" as before — the new label logic should not apply

Test 5: Other single-select lists

  1. Test other single-select lists such as Settings > Profile > Timezone or Settings > Preferences > Language
  2. Expected: Same behavior as Pronouns — selected item announces "selected" once, non-selected items do not
  • Verify that no errors appear in the JS console

Offline tests

  • Same as Tests

QA Steps

  • Same as Tests

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
    • MacOS: Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I verified there are no new alerts related to the canBeMissing param for useOnyx
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I used JaimeGPT to get English > Spanish translation. I then posted it in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
android_native.mp4
Android: mWeb Chrome
android_mWeb.mp4
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web_chrome_jaws.mp4
web_chrome_mac.mp4
web_chrome_nvda.mp4
web_safari_mac.mp4

Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@melvin-bot
Copy link
Copy Markdown

melvin-bot bot commented Apr 3, 2026

Hey, I noticed you changed src/languages/en.ts in a PR from a fork. For security reasons, translations are not generated automatically for PRs from forks.

If you want to automatically generate translations for other locales, an Expensify employee will have to:

  1. Look at the code and make sure there are no malicious changes.
  2. Run the Generate static translations GitHub workflow. If you have write access and the K2 extension, you can simply click: [this button]

Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running:

npx ts-node ./scripts/generateTranslations.ts --help

Typically, you'd want to translate only what you changed by running npx ts-node ./scripts/generateTranslations.ts --compare-ref main

@Krishna2323
Copy link
Copy Markdown
Contributor Author

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b9ef27dbe9

ℹ️ 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".

Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@Krishna2323
Copy link
Copy Markdown
Contributor Author

@codex review

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 4, 2026

Codecov Report

✅ Changes either increased or maintained existing code coverage, great job!

Files with missing lines Coverage Δ
...components/SelectionList/ListItem/BaseListItem.tsx 95.08% <100.00%> (+0.16%) ⬆️
...omponents/SelectionList/utils/getItemRole/index.ts 100.00% <100.00%> (ø)
...ts/SelectionList/utils/getSelectableState/index.ts 100.00% <100.00%> (ø)
... and 14 files with indirect coverage changes

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

ℹ️ 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".

@Krishna2323
Copy link
Copy Markdown
Contributor Author

@MelvinBot review this PR and confirm everything looks correct.

@MelvinBot
Copy link
Copy Markdown
Contributor

I reviewed the diff and the surrounding code. Here's my assessment:

Overall: The approach is correct and well-scoped.

The fix addresses a real Chrome+macOS VoiceOver limitation where aria-selected on role="option" elements isn't exposed to the accessibility tree. Baking the "selected" text into the label is the right workaround.

What looks good:

  • Browser/OS gate (getBrowser() === 'chrome' && getOperatingSystem() === CONST.OS.MAC_OS) ensures Safari users won't hear double "selected" announcements
  • Properly scoped to single-select lists only (isSelectableOption check excludes checkbox/radio)
  • On native platforms, getBrowser() won't return 'chrome', so the workaround is safely a no-op there
  • Translations for common.selected are present in all locale files
  • The selectedLabel is passed from the component via useLocalize, keeping getAccessibilityProps as a plain function

Minor observations (non-blocking):

  • getBrowser() and getOperatingSystem() do user-agent/platform string parsing on each call per list item. These are cheap operations so it's fine in practice, but if this list is ever performance-sensitive, the results could be memoized once at module level since browser and OS don't change at runtime.

No correctness issues found. The PR matches the testing plan described and should resolve Expensify/App#74875.

Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@Krishna2323
Copy link
Copy Markdown
Contributor Author

@MelvinBot review this PR and confirm everything looks correct.

@MelvinBot
Copy link
Copy Markdown
Contributor

I reviewed this PR again — the diff is unchanged since my previous review and my assessment stands.

Summary: The approach is correct. It works around Chromium bug #337904204 by appending a localized "selected" string to the accessibility label on Chrome desktop only. Multi-select lists, mobile Chrome, and Safari are properly excluded.

CI status: All checks pass except the PR Author Checklist (which is manual) and a few jobs still pending. No code-related failures.

No issues found — this is ready for human review.

@Krishna2323
Copy link
Copy Markdown
Contributor Author

@MelvinBot I made some changes in 1b6458c

@MelvinBot
Copy link
Copy Markdown
Contributor

Reviewed commit 1b6458c. The changes look correct — good call extending the workaround to all Chrome desktop platforms, since the Chromium bug affects Windows and Linux too, not just macOS. Using !isMobileChrome() is cleaner than the OS check.

One issue: getContainerRole (3 new files) is defined but never imported or used anywhere in the codebase. If it's intended for a follow-up change, it should be removed from this PR to avoid shipping dead code. If it's meant to be wired into a parent SelectionList component, it's missing.

Everything else looks good:

  • getItemRole platform-split utility is correctly used
  • effectiveRole is now computed before the accessible === false early return, which is fine since the returned object doesn't use it
  • isMobileChrome is properly exported from both Browser/index.ts (returns false on native) and Browser/index.website.ts (does real detection)

…ed state

Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
… selection state

Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@Krishna2323
Copy link
Copy Markdown
Contributor Author

@MelvinBot I have changed the approach, review again.

Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@MelvinBot
Copy link
Copy Markdown
Contributor

The approach changed significantly — instead of appending "selected" to the label via UA sniffing, you're now using aria-current with a platform-split getSelectableState. The core idea is sound: on web, skip accessibilityState.selected (which Chrome doesn't expose for role="option") and rely on aria-current={true} instead; on native, keep accessibilityState.selected as before. React Native Web properly forwards aria-current to the DOM, so this works.

Issues to address:

  1. Dead code: getContainerRole — The 3 new files at utils/getContainerRole/ are never imported anywhere. BaseSelectionList already uses an identical utility called getListboxRole for the container's role="listbox". These files should be removed.

  2. Dead code: CONST.ROLE.MENUITEMRADIO — Added to CONST/index.ts but never referenced. Should be removed unless there's a follow-up PR that needs it.

  3. Dead code: common.selected translations — All 10 locale files add a selected translation, but the new approach no longer calls translate('common.selected'). These should be removed.

  4. isFocused removed from checkbox/radio state — The old code set selected: !!isFocused for checkbox/radio roles. The new code drops this entirely ({checked: !!item.isSelected} only). Was this intentional? If so, it's fine — focus is communicated through other means — but worth calling out since it's a behavior change beyond the scope of the Chrome fix.

What looks good:

  • Platform split via getSelectableState is clean — web returns undefined, native returns {selected}
  • aria-current={true} is the right semantic for "current item in a set" and avoids the Chrome bug entirely
  • No more browser/OS sniffing
  • Properly scoped to single-select (non-checkbox/radio) lists

…ed for checkable roles

Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@Krishna2323
Copy link
Copy Markdown
Contributor Author

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 178743fb56

ℹ️ 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".

@Krishna2323 Krishna2323 marked this pull request as ready for review April 7, 2026 05:33
@Krishna2323 Krishna2323 requested review from a team as code owners April 7, 2026 05:33
@melvin-bot melvin-bot bot requested a review from mkhutornyi April 7, 2026 05:33
@melvin-bot melvin-bot bot requested review from heyjennahay and removed request for a team April 7, 2026 05:33
@melvin-bot
Copy link
Copy Markdown

melvin-bot bot commented Apr 7, 2026

@mkhutornyi 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]

@melvin-bot melvin-bot bot removed the request for review from a team April 7, 2026 05:33
…me only

Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@Krishna2323
Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Hooray!

ℹ️ 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".

@MelvinBot
Copy link
Copy Markdown
Contributor

Reviewed the latest diff. The dead code from my previous review (getContainerRole, MENUITEMRADIO, common.selected translations) is all gone — nice cleanup.

Issues:

  1. getSelectableState platform split does nothingindex.ts and index.web.ts are byte-for-byte identical (both return {selected: isSelected}). Either the web version should differ (e.g., return undefined to skip aria-selected on web and rely solely on aria-current), or the platform split should be removed and replaced with a single file. As-is, it's dead abstraction.

  2. getItemRole has no platform variant — It only has index.ts, no index.web.ts. If the intent is to always use role="option" on all platforms for selectable items, this could just be inlined — extracting a one-liner to its own utility with a types file is over-abstracted for what it does.

  3. Unrelated whitespace change — A blank line was removed between const icons = ... and const pressableRef = ... (line ~101). Minor, but keep diffs scoped to the fix.

The approach itself is sound: adding aria-current={true} on Chrome desktop as a workaround for the Chromium bug while preserving aria-selected via accessibilityState is a reasonable strategy. All checks pass (only the manual checklist fails).

Recommendation: simplify by removing the unnecessary getSelectableState platform split (make it a single file) and consider inlining getItemRole since it's a one-liner used in one place.

@mkhutornyi
Copy link
Copy Markdown
Contributor

Test 1: Chrome + VoiceOver not working

I am not able to select individual menu item using Tab, thus not announced at all.

test1
test1.mov

@mkhutornyi
Copy link
Copy Markdown
Contributor

Test 3: Arrow key navigation

This also doesn't work on Chrome + VoiceOver.

test3.mov

I think the testing is blocked by general bug on selectionlist, where each menu item is not selected at all by screen reader.

@Krishna2323
Copy link
Copy Markdown
Contributor Author

@mkhutornyi test steps updated, try on "Language" page.

@mkhutornyi
Copy link
Copy Markdown
Contributor

iOS and android work fine.
Just "selected" is announced before item title, while after on web.
Not blocker but just reporting as inconsistency

@mkhutornyi
Copy link
Copy Markdown
Contributor

Still same on Language page

Screen.Recording.2026-04-07.at.10.57.13.am.mov

@Krishna2323
Copy link
Copy Markdown
Contributor Author

Still same on Language page

I can see it says "current item", there seems to be focus bug but the fix is working.

@mkhutornyi
Copy link
Copy Markdown
Contributor

I think we should fix focus bug to test Chrome+VoiceOver properly.
As this PR is mainly to fix this case, I am not sure we can unblock without fixing this.

@Krishna2323
Copy link
Copy Markdown
Contributor Author

@mkhutornyi I’m not sure if we should do that here—I don’t want to stretch this PR further. I’ve already spent a lot of effort investigating and finding a proper fix.

@rushatgabhane what do you think?

@mkhutornyi
Copy link
Copy Markdown
Contributor

yes it's out of scope but I believe we should fix it separately.

@Krishna2323
Copy link
Copy Markdown
Contributor Author

@mkhutornyi bump

@mkhutornyi
Copy link
Copy Markdown
Contributor

I meant not handle in this PR but should still fix. Until then, maybe put this PR on hold.

@Krishna2323
Copy link
Copy Markdown
Contributor Author

Krishna2323 commented Apr 9, 2026

@mkhutornyi seems like it's fixed, can you check?

Monosnap.screencast.2026-04-10.00-53-34.mp4

@mkhutornyi
Copy link
Copy Markdown
Contributor

After fix, each item should be white-bordered. Still on list container from your video. Check safari for the expected behavior.

@Krishna2323
Copy link
Copy Markdown
Contributor Author

@mkhutornyi the PR is about the label announcement, I haven't modified the UI so we shouldn't care about that. Or am I missing something?

@mkhutornyi
Copy link
Copy Markdown
Contributor

I don't say that this PR is something wrong. Just blocked testing

@mkhutornyi
Copy link
Copy Markdown
Contributor

I can approve right away but for me it doesn't make sense to merge without proper test since this PR aims to fix the exactly Chrome+VoiceOver issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants