fix: button is announced without a state#84244
fix: button is announced without a state#84244daledah wants to merge 21 commits intoExpensify:mainfrom
Conversation
|
|
|
@QichenZhu 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 review. Since we fixed the |
JmillsExpensify
left a comment
There was a problem hiding this comment.
Fix is good with product.
@daledah, how's it going? |
|
@QichenZhu it's working on the Android now, please move forward android.mov |
@daledah, good to know it works on Android, but our main concern is the iOS side. |
|
@QichenZhu i updated and the result belows:
|
|
@daledah, I'm wondering why this PR mixes the patch and |
@QichenZhu things may not work on simulator. please trigger Adhoc and ask for QA confirmation if needed on C+ channel only if needed |
|
@rushatgabhane, thanks for the heads up! I'll test on a physical device or ask in Slack for help once we settle on a solution. |
|
@QichenZhu I updated to use |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-native.webmAndroid: mWeb Chromeandroid-web.webmiOS: HybridAppios-native.mp4iOS: mWeb Safariios-web.mp4 |
QichenZhu
left a comment
There was a problem hiding this comment.
The test step says 'when expanded, the button should indicate "expanded"', but focus moves away from the button when expanded, so it may not be announced on all platforms. I think this is fine.
|
btw if accessibilityHint isn't supported on web, we can use |
|
@MelvinBot review |
Code ReviewOverall approach (using However, I found one issue: Bug: Expand/Collapse logic is invertedconst accessibilityExpandState = isMenuVisible ? CONST.ACCESSIBILITY_LABELS.EXPAND : CONST.ACCESSIBILITY_LABELS.COLLAPSE;When accessibilityLabel={isExpanded ? CONST.ACCESSIBILITY_LABELS.COLLAPSE : CONST.ACCESSIBILITY_LABELS.EXPAND}The label describes the action the button will perform: "Collapse" when expanded, "Expand" when collapsed. The PR does the opposite. Fix: Swap the ternary: const accessibilityExpandState = isMenuVisible ? CONST.ACCESSIBILITY_LABELS.COLLAPSE : CONST.ACCESSIBILITY_LABELS.EXPAND;Other notes (non-blocking)
|
|
Good catch from Melvin. Since we changed 'collapsed' to 'collapse', we are announcing an action instead of the current state. |
|
Ah that is confusing but I think makes sense? |
|
@JmillsExpensify @dangrous @rushatgabhane could you confirm what's more user-friendly: announcing the current state ('Collapsed' / 'Expanded'), or the action the button will perform ('Expand' when collapsed, 'Collapse' when expanded)? |
|
I've looked through accessibility guidelines and it's a little confusing but I think it is preferred to announce the current state (so I looked mainly at the Does that make sense? |
that sounds good! @daledah could you please update the labels |
|
@MelvinBot review |
Code ReviewThe previous review's ternary inversion issue has been resolved — the PR now correctly uses Bug:
|
QichenZhu
left a comment
There was a problem hiding this comment.
I looked mainly at the
aria-expandedhere and the react docs here (which seems to imply we should ensure this is in theaccessibilityStateobject as well as correctly settingaria-expanded
@dangrous yeah, that's the original proposal we agreed on. We need a patch to let iOS support accessibilityState, but it didn't work on @daledah's side for some reason, even though it worked for me. So we switched to the current workaround. However, based on your and MelvinBot's comments, it's buggy and doesn't follow the standard.
|
Verified @MelvinBot's comment is fixed. 84244.mp4 |
|
OKay got it. Do we want to include the accessibilityState fix in here as well, just in case? I guess we should stick with at LEAST this fix if that one wasn't working during testing, but probably good to include both unless that would conflict with itself |
|
Bumping the above - should we add both fixes, or stick with this slightly weird one for now? I'd prefer to get the accessibilityState in there and rely on that, but happy to include both if it won't cause conflicts. |
|
@QichenZhu Could you please reapply the patch for the |
|
@daledah sure, I can test it on my side. |
|
NAB: button state is still announced as collapsed after expanding. This might be caused by a react-native bug iOS Accessibility Value Out of Sync #35774 RPReplay_Final1774580297-2026-03-27.03_02_47.179.mp4 |
@rushatgabhane, the cc @dangrous |
|
Thanks for the progress here! Just reviewed the potential new PR |
|
Closed because #86499 was merged |


Explanation of Change
Fixed Issues
$ #76929
PROPOSAL: #76929 (comment)
Tests
Prerequisite:
Offline tests
QA Steps
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
Screen.Recording.2026-03-19.at.14.47.21.mov
Android: mWeb Chrome
android-mweb.mov
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari