[A11y] Add Save button to Report settings single-select pages (Write capability, Notification preference, Visibility)#94557
[A11y] Add Save button to Report settings single-select pages (Write capability, Notification preference, Visibility)#94557MelvinBot wants to merge 8 commits into
Conversation
…button Co-authored-by: truph01 <truph01@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🤖 I analyzed the failing checks. There's no code failure to fix — every code/CI check passes (typecheck, all 8 The single red check is PR Author Checklist / checklist, which failed with:
This is the manual author checklist in the PR description — it can only be cleared by you checking the boxes (it's intentionally human-only, so I won't fill it out on your behalf). Once you complete the checklist, that check will pass and the PR will be green. When I first looked, several |
|
@MelvinBot Mark this PR as ready for 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: d85eb1a072
ℹ️ 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".
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@MelvinBot Check and resolve all comments above |
…rop redundant React Compiler memoization Co-authored-by: truph01 <truph01@users.noreply.github.com>
| const {accountID: currentUserAccountID} = useCurrentUserPersonalDetails(); | ||
| const isMoneyRequest = isMoneyRequestReport(report); | ||
| const currentNotificationPreference = getReportNotificationPreference(report); | ||
| // Keep the draft undefined until the user picks a row so we always fall back to the live preference. |
There was a problem hiding this comment.
Put an empty line before the comment unless it’s on the first line of a block.
| // Keep the draft undefined until the user picks a row so we always fall back to the live preference. | |
| // Keep the draft undefined until the user picks a row so we always fall back to the live preference. |
There was a problem hiding this comment.
Done in 0f72407 — added the blank line before the comment.
| return; | ||
| } | ||
| updateRoomVisibility(report.reportID, report.visibility, newVisibility); | ||
| // Selecting Public is a sensitive change, so it still has to be confirmed before we persist it. |
There was a problem hiding this comment.
| // Selecting Public is a sensitive change, so it still has to be confirmed before we persist it. | |
| // Selecting Public is a sensitive change, so it still has to be confirmed before we persist it. |
There was a problem hiding this comment.
Done in 0f72407 — added the blank line before the comment.
|
@MelvinBot Resolve the above comments |
Doesn't seem like it worked 😄 , can you accept the suggestions @truph01? |
|
@MelvinBot Resolve the above comments |
Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
@MelvinBot Resolve conflicts |
…ttings-save Co-authored-by: truph01 <truph01@users.noreply.github.com> # Conflicts: # src/pages/settings/Report/DynamicNotificationPreferencePage.tsx # src/pages/settings/Report/DynamicVisibilityPage.tsx # src/pages/settings/Report/DynamicWriteCapabilityPage.tsx
|
🤖 Resolved the merge conflicts and pushed the merge commit ( What conflicted: All three files ( How I resolved it: Kept this PR's intended behavior (draft state + Verification:
The React Compiler compliance check couldn't run locally (it failed resolving the base-branch ref in this environment, not on the code) — it'll run normally in CI. PR is now |
|
|
|
@MelvinBot Merge main |
…ttings-save Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
🤖 Merged latest |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c555d0b035
ℹ️ 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".
|
|
||
| const {showConfirmModal} = useConfirmModal(); | ||
|
|
||
| const [selectedVisibility, setSelectedVisibility] = useState<RoomVisibility | undefined>(report?.visibility); |
There was a problem hiding this comment.
When an Onyx update changes report.visibility after this page mounts but before the user chooses a row, selectedVisibility remains the old value; pressing Save then sends that stale value using the newly updated report.visibility as the previous value, reverting the external change. This also affects selectedWriteCapability in DynamicWriteCapabilityPage.tsx:29. Use an initially undefined draft and fall back to the live report value until the first explicit selection, as the notification-preference page does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot I think it is a valid concern. Please resolve it
There was a problem hiding this comment.
Agreed — fixed in 3f2079c. Both pages now mirror the notification-preference page: the draft stays undefined until the user picks a row, and the selected value falls back to the live report value until then. So an Onyx update to report.visibility / report.writeCapability after mount is no longer frozen out and can't be reverted on Save.
DynamicVisibilityPage.tsx:40-41—draftVisibility ?? report?.visibilityDynamicWriteCapabilityPage.tsx:33-34—draftWriteCapability ?? currentWriteCapability
onSelectRow now sets the draft, and initiallyFocusedItemKey still points at the live value so the correct row is focused on open.
| ListItem={SingleSelectListItem} | ||
| onSelectRow={(option) => updateNotificationPreferenceForReportAction(option.value)} | ||
| onSelectRow={(option) => setDraftNotificationPreference(option.value)} | ||
| confirmButtonOptions={confirmButtonOptions} |
There was a problem hiding this comment.
On devices with a bottom gesture/home-indicator inset, this new fixed Save footer receives only its regular pb5 spacing because the surrounding ScreenWrapper explicitly disables bottom safe-area padding and the SelectionList does not set addBottomSafeAreaPadding. The same combination occurs in all three changed pages, so their Save buttons can intrude into the unsafe area; pass addBottomSafeAreaPadding to each list or let the wrapper provide the inset.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot it is a valid concern. Please resolve it
There was a problem hiding this comment.
Agreed — fixed in 8fe8710. Each of the three pages sets includeSafeAreaPaddingBottom={false} on its ScreenWrapper, so the SelectionList's fixed Save footer (FixedFooter) was getting only its base pb5 spacing and no bottom inset. Passing addBottomSafeAreaPadding to each list makes FixedFooter add the device bottom inset (via useBottomSafeSafeAreaPaddingStyle → raw useSafeAreaPaddings), so the Save button no longer intrudes into the home-indicator/gesture area. Because the wrapper's bottom padding is disabled, there's no double-count, and since a footer is present the list's own container padding is skipped — the inset lands exactly on the footer. This matches the established pattern (e.g. CardTypeStep).
…rite capability pages
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
…e-select pages
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
@MelvinBot Remove the unnecessary comments you added in this PR |
Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
Done in a3649dd — removed the redundant draft-state explanation comments from all three pages (the two-line I kept one comment: |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-07-26.at.21.08.24.movScreen.Recording.2026-07-26.at.21.09.37.movScreen.Recording.2026-07-26.at.21.11.47.movAndroid: mWeb ChromeiOS: HybridAppScreen.Recording.2026-07-26.at.21.18.30.moviOS: mWeb SafariScreen.Recording.2026-07-26.at.21.16.30.movMacOS: Chrome / SafariScreen.Recording.2026-07-26.at.21.17.17.mov |
3 participants
Explanation of Change
This is PR 1 of the batched rollout plan that extends the WCAG 3.2.2 ("On Input") fix from #92749 to the remaining single-select pages. It covers the three Report settings pages, grouped together because they live in the same directory and share the identical
SelectionList+SingleSelectListItempattern.Before this change each page persisted the change and navigated away the instant a row was selected, which is the "change of context on input" that WCAG 3.2.2 prohibits — especially disruptive for screen-reader users. The fix mirrors #92749: the selection is held in local draft state (the page stays open, the checkmark moves), and the change is only persisted + the page closed when the user taps a Save button added via
confirmButtonOptions.Pages fixed:
DynamicWriteCapabilityPage—onSelectRownow updates draftselectedWriteCapability;SavecallsupdateWriteCapability+goBack.DynamicNotificationPreferencePage—onSelectRownow updates draftselectedNotificationPreference;SavecallsupdateNotificationPreference+goBack.DynamicVisibilityPageSavepattern, but thePublicconfirmation modal is preserved: selectingPublicjust moves the checkmark, and the existing "Are you sure?" modal is now gated on Save (if the user cancels the modal, nothing is persisted and the page stays open).No
SelectionListcomponent changes; behavior is identical on web and native; optimistic/offline behavior is unchanged.Fixed Issues
$ #74834
PROPOSAL: #74834 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Private/Restrictedand verify the page stays open until Save. SelectPublic, tap Save, and verify the "Are you sure?" confirmation modal appears; confirm → visibility saves and page closes; cancel → nothing saved and page stays open.Offline tests
Same as Tests — selecting an option and tapping Save should optimistically apply the change while offline.
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.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