fix: break VoiceOver grouping on iOS so task checkbox is independently focusable#91530
Conversation
…y focusable Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
…sions on other platforms Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37a3806a51
ℹ️ 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".
|
Mostly looks good. Just one issue found. After toggle checkbox, old checked state is announced. This happens on both pages.
|
…fter toggle Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@mkhutornyi could you please check again? |
| )} | ||
| <PressableWithSecondaryInteraction | ||
| ref={popoverAnchorRef} | ||
| accessible={shouldBreakAccessibilityGrouping() && isCreatedTaskReportAction(action) ? false : undefined} |
There was a problem hiding this comment.
As setting false here, no pressed feedback. I'm afraid QA will mark this as regression
Screen.Recording.2026-05-25.at.3.50.00.AM.mov
production:
Screen.Recording.2026-05-25.at.3.51.11.AM.mov
There was a problem hiding this comment.
Tried to fix that, could you please check?
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
| const [prevIsTaskCompletedFromOnyx, setPrevIsTaskCompletedFromOnyx] = useState(isTaskCompletedFromOnyx); | ||
| const [isTaskCompleted, setIsTaskCompleted] = useState(isTaskCompletedFromOnyx); |
There was a problem hiding this comment.
Double states for the same state variable seems workaround to me
There was a problem hiding this comment.
Now local state is only used when shouldBreakGrouping && isScreenReaderActive (iOS + VoiceOver).
| const [prevIsTaskCompletedFromOnyx, setPrevIsTaskCompletedFromOnyx] = useState(isTaskCompletedFromOnyx); | ||
| const [isTaskCompleted, setIsTaskCompleted] = useState(isTaskCompletedFromOnyx); | ||
|
|
||
| if (prevIsTaskCompletedFromOnyx !== isTaskCompletedFromOnyx) { | ||
| setPrevIsTaskCompletedFromOnyx(isTaskCompletedFromOnyx); | ||
| setIsTaskCompleted(isTaskCompletedFromOnyx); | ||
| } |
There was a problem hiding this comment.
This local state update logic now runs on every platform, not just iOS.
If completeTask/reopenTask ever fails or is queued offline and the underlying Onyx state doesn't flip, the local state might read "checked" briefly until the next re-render syncs back. Please verify this case.
And consider scoping this to shouldBreakGrouping only.
There was a problem hiding this comment.
Scoped. setLocalIsTaskCompleted is only called on iOS + VoiceOver. On all other platforms, isTaskCompleted resolves directly to the Onyx value.
…le click check Signed-off-by: krishna2323 <belivethatkg@gmail.com>
…edback for title area Signed-off-by: krishna2323 <belivethatkg@gmail.com>
melvin-bot
Bot
requested review from
joekaufmanexpensify and
mkhutornyi
and removed request for
a team
|
@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] |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9523a108e8
ℹ️ 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>
| <View style={[styles.flexRow, styles.flex1]}> | ||
| <Checkbox | ||
| onPress={callFunctionIfActionIsAllowed(() => { | ||
| // If we're already navigating to these task editing pages, early return not to mark as completed, otherwise we would have not found page. |
There was a problem hiding this comment.
Please restore this comment
| const iconWrapperStyle = StyleUtils.getTaskPreviewIconWrapper(hasAssignee ? avatarSize : undefined); | ||
|
|
||
| const shouldShowGreenDotIndicator = isOpenTaskReport(taskContextReport, action) && isReportManager(taskContextReport); | ||
| const taskAccessibilityLabel = taskTitleWithoutImage ? `${translate('task.task')}: ${taskTitleWithoutImage}` : translate('task.task'); |
There was a problem hiding this comment.
Raw html is announced here.
|
When double tap "Take a test drive", it opens test drive modal instead of navigating to detail page. |
|
@mkhutornyi could you please test again? Now it should only announce the checkbox state when toggled. |
|
@Krishna2323 |
|
The above bug is flaky. Mostly works well, so I won't block on this. |
| const shouldShowGreenDotIndicator = isOpenTaskReport(taskContextReport, action) && isReportManager(taskContextReport); | ||
| const taskTitlePlainText = Parser.htmlToText(taskTitle); | ||
| const taskAccessibilityLabel = taskTitlePlainText ? `${translate('task.task')}: ${taskTitlePlainText}` : translate('task.task'); | ||
| const shouldUseSplitTaskCheckboxAccessibility = shouldBreakGrouping && isScreenReaderActive; |
There was a problem hiding this comment.
NAB: Should we use this flag everywhere? shouldBreakGrouping && isScreenReaderActive is duplicated in many places.
ReviewThe core fix is sound: gating a A few things worth confirming before merge — mostly because this PR touches three shared components that are used well beyond the task flow. 1. Removed blur-on-click in
|
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ 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". |
|
Android TalkBack: toggle checkbox doesn't announce anything. android.mov |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #77574 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
…blur Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@cristipaval ready again. |
🤖 Code ReviewThe approach is sound — breaking VoiceOver's accessibility grouping on iOS via 🔴 Worth confirming1. 2. The screen-reader 3. No unit tests for the new logic. 🟡 Minor / cleanup
Things that look correct
I focused on correctness and the blast radius of the shared-component edits; I did not run the app with a screen reader. Reply with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00c16401bb
ℹ️ 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>
|
@MelvinBot review again |
🤖 Code Review (re-review)Re-reviewed the delta since my last pass (commit ✅ Addressed
🟡 Still open (non-blocking — same as last review)
🟢 Minor (optional)An external URL pasted inside a task title (anchor with no Verification notes
Reply with |
|
@cristipaval @mkhutornyi please check again 🙇🏻 |
|
🚧 @cristipaval has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
9 participants
Explanation of Change
Fixed Issues
$ #77574
PROPOSAL:
Tests
Here are the test steps:
Prerequisites:
iOS (Primary fix):
Android:
Web:
Offline tests
QA Steps
Same as tests
Verify that no errors appear in the JS console
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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_hybrid.mp4
Android: mWeb Chrome
android_mWeb.mp4
iOS: Native
ios_hybrid.mp4
iOS: mWeb Safari
ios_mWeb.mp4
MacOS: Chrome / Safari
web_chrome.mp4