[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 12 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 |
…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 |
|
@MelvinBot merge main |
…ttings-save Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
Merged |
truph01
left a comment
truph01
left a comment
There was a problem hiding this comment.
@MelvinBot Please resolve these comments
|
|
||
| const changeVisibility = (newVisibility: RoomVisibility) => { | ||
| if (!report) { | ||
| const saveVisibility = async () => { |
There was a problem hiding this comment.
Public confirmation modal triggers when nothing changed
If a room is already public and the user taps Save without selecting a different option, selectedVisibility falls back to report?.visibility which is PUBLIC, so the "Are you sure?" modal still pops up. Could we gate the modal on draftVisibility being set — or skip the confirmation when selectedVisibility === report?.visibility?
There was a problem hiding this comment.
Fixed by the same change in d8abb61. Save is now disabled while selectedVisibility === report?.visibility (DynamicVisibilityPage.tsx:86), so the exact scenario you described — an already-public room where the user hasn't picked a different option — leaves the button disabled and the "Are you sure?" modal can't fire. The modal now only appears when Public is an actual change from the room's current visibility, which is when we do want the confirmation.
| goBack(); | ||
| }; | ||
|
|
||
| const confirmButtonOptions = { |
There was a problem hiding this comment.
Save enabled with no changes
All three pages let the user tap Save without selecting a different option, which fires the update API with old === new. Would it make sense to disable the button while draftWriteCapability is undefined (and same for the other two pages)?
There was a problem hiding this comment.
Good call — done in d8abb61. All three Save buttons are now disabled whenever the selected value equals the current live value, i.e. selected === current (which is exactly the case when the draft is still undefined and when the user re-picks the option that's already active), so the update API is never fired with old === new:
DynamicWriteCapabilityPage.tsx:55—selectedWriteCapability === currentWriteCapabilityDynamicNotificationPreferencePage.tsx:63—selectedNotificationPreference === currentNotificationPreferenceDynamicVisibilityPage.tsx:86—selectedVisibility === report?.visibility
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>
|
The Jest Unit Tests / test (job 4) (pull_request) isn't related to this PR. |
|
@MelvinBot merge latest main here |
…ttings-save Co-authored-by: Lucien Akchoté <lakchote@users.noreply.github.com>
|
Merged the latest |
|
@MelvinBot merge latest main here |
…ttings-save Co-authored-by: Lucien Akchoté <lakchote@users.noreply.github.com>
|
Merged latest |
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)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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/Videosundefined