From ad64a1f4f9b2c5e071c991838582ad91666c3f99 Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Mon, 28 Sep 2026 10:00:32 +0530 Subject: [PATCH 01/23] Offer the workspace members bulk actions in the floating bar Signed-off-by: krishna2323 --- src/components/Table/Table.tsx | 9 + src/components/Table/TableBody.tsx | 2 + src/components/Table/TableBulkActionBar.tsx | 36 +++ src/components/Table/TableContext.tsx | 11 + src/components/Table/index.tsx | 5 + .../Tables/WorkspaceMembersTable/index.tsx | 6 + src/pages/workspace/WorkspaceMembersPage.tsx | 6 +- tests/ui/WorkspaceMembersTest.tsx | 241 +++++++----------- 8 files changed, 160 insertions(+), 156 deletions(-) create mode 100644 src/components/Table/TableBulkActionBar.tsx diff --git a/src/components/Table/Table.tsx b/src/components/Table/Table.tsx index 9d790e4dcefb..6d659ccdac59 100644 --- a/src/components/Table/Table.tsx +++ b/src/components/Table/Table.tsx @@ -36,6 +36,7 @@ import useSelection from './middlewares/selection'; import useSorting from './middlewares/sorting'; import {shouldUseTableSemantics} from './tableAccessibility'; import {doesBodyRenderWhenEmpty} from './TableBody'; +import TableBulkActionBar from './TableBulkActionBar'; import TableContext from './TableContext'; import TableEmptyState from './TableEmptyStates/TableEmptyState'; import TableNoResultsState from './TableEmptyStates/TableNoResultsState'; @@ -429,6 +430,12 @@ function Table 0 && !!tableHeaderElement && hasPageHeader && !(shouldUseNarrowTableLayout && !title); + // The bar is only ever a wide layout affordance, since the narrow one selects rows through its own header. Whether + // it is on screen is decided here rather than in the bar, because the list has to reserve the space it floats over + // and the two must not disagree about when that is. + const hasBulkActionBarChild = childrenArray.some((child) => React.isValidElement(child) && child.type === TableBulkActionBar); + const isBulkActionBarVisible = hasBulkActionBarChild && !shouldUseNarrowLayout && selectedKeys.length > 0; + const tableListMetadata = useMemo( () => getTableListMetadata({ @@ -516,6 +523,8 @@ function Table(); const { ListEmptyComponent, @@ -399,6 +400,7 @@ function TableBodyList({contentContainerStyle, emptyMessage, onLayout, style, .. listContentContainerStyle, tableBodyContentContainerStyle, contentContainerStyle, + isBulkActionBarVisible && styles.bulkActionBarListSpacing, shouldRenderEmptyStateInList && styles.flexGrow1, shouldUseNarrowTableLayout && typeof contentMinHeight === 'number' && diff --git a/src/components/Table/TableBulkActionBar.tsx b/src/components/Table/TableBulkActionBar.tsx new file mode 100644 index 000000000000..0bc7fefc2098 --- /dev/null +++ b/src/components/Table/TableBulkActionBar.tsx @@ -0,0 +1,36 @@ +import BulkActionBar from '@components/BulkActionBar'; +import type {BulkActionBarProps} from '@components/BulkActionBar/types'; + +import React from 'react'; + +import type {TableData} from './types'; + +import {useTableContext} from './TableContext'; + +type TableBulkActionBarProps = Omit, 'selectedCount' | 'onClearSelection'> & { + /** Replaces what the bar's close button does, which is otherwise to clear the table's own selection. */ + onClearSelection?: () => void; +}; + +/** + * The floating bulk action bar for the table's selection. Render it as a child of `` and it mounts itself + * whenever the wide layout has rows selected, with the list reserving the space it floats over. + */ +function TableBulkActionBar({onClearSelection, ...bulkActionBarProps}: TableBulkActionBarProps) { + const {selectedKeys, tableMethods, isBulkActionBarVisible} = useTableContext(); + + if (!isBulkActionBarVisible) { + return null; + } + + return ( + + ); +} + +export default TableBulkActionBar; +export type {TableBulkActionBarProps}; diff --git a/src/components/Table/TableContext.tsx b/src/components/Table/TableContext.tsx index 63d55d217baf..a1f9698dd25f 100644 --- a/src/components/Table/TableContext.tsx +++ b/src/components/Table/TableContext.tsx @@ -55,6 +55,15 @@ type TableContextValue = { }, isEmptyResult: false, isDefaultViewEmpty: false, + selectedKeys: [], + isBulkActionBarVisible: false, shouldUseNarrowTableLayout: false, isMobileSelectionEnabled: false, }; diff --git a/src/components/Table/index.tsx b/src/components/Table/index.tsx index ca994883f0c5..c68d07362daa 100644 --- a/src/components/Table/index.tsx +++ b/src/components/Table/index.tsx @@ -36,6 +36,7 @@ import composeTableListHeader from './composeTableListHeader'; import TableComponent from './Table'; import TableBody from './TableBody'; +import TableBulkActionBar from './TableBulkActionBar'; import TableContext from './TableContext'; import TableEmptyState from './TableEmptyStates/TableEmptyState'; import TableNoResultsState from './TableEmptyStates/TableNoResultsState'; @@ -58,6 +59,7 @@ import TableRow from './TableRow'; * - `Table.EmptyState` - Renders when the table has no rows * - `Table.NoResultsState` - Renders when the table has rows, but the user has filtered all of them out * - `Table.LoadingState` - Renders a loading indicator when the table is loading its data + * - `Table.BulkActionBar` - The floating bar of bulk actions for the current selection */ const Table = Object.assign(TableComponent, { /** The React context for accessing table state directly. */ @@ -86,6 +88,9 @@ const Table = Object.assign(TableComponent, { /** Renders a loading indicator when the table is loading its data */ LoadingState: TableLoadingState, + + /** The floating bar of bulk actions, shown in the wide layout while rows are selected */ + BulkActionBar: TableBulkActionBar, }); export default Table; diff --git a/src/components/Tables/WorkspaceMembersTable/index.tsx b/src/components/Tables/WorkspaceMembersTable/index.tsx index 2b88b77ac975..9d2c4e9cbbe7 100644 --- a/src/components/Tables/WorkspaceMembersTable/index.tsx +++ b/src/components/Tables/WorkspaceMembersTable/index.tsx @@ -1,3 +1,4 @@ +import type {DropdownOption, WorkspaceMemberBulkActionType} from '@components/ButtonWithDropdownMenu/types'; import type {CompareItemsCallback, FilterConfig, IsItemInFilterCallback, IsItemInSearchCallback, TableColumn, TableData, TableHandle} from '@components/Table'; import Table, {composeTableListHeader} from '@components/Table'; import compareOptionalValues from '@components/Table/compareOptionalValues'; @@ -52,6 +53,9 @@ type WorkspaceMembersTableProps = { shouldShowCustomField2Column: boolean; onRowSelectionChange: (selectedRowKeys: string[]) => void; headerComponent?: React.ReactElement; + + /** The actions the selected members support, offered in the floating bar the wide layout shows while rows are selected. */ + bulkActionOptions?: Array>; }; const WORKSPACE_MEMBER_FILTER_VALUES = { @@ -75,6 +79,7 @@ export default function WorkspaceMembersTable({ members, onRowSelectionChange, headerComponent, + bulkActionOptions, }: WorkspaceMembersTableProps) { const {translate, localeCompare} = useLocalize(); const {shouldUseNarrowLayout, isMediumScreenWidth} = useResponsiveLayout(); @@ -302,6 +307,7 @@ export default function WorkspaceMembersTable({ + {!!bulkActionOptions && }
); } diff --git a/src/pages/workspace/WorkspaceMembersPage.tsx b/src/pages/workspace/WorkspaceMembersPage.tsx index f1f05138fc85..3fe9720c28c7 100644 --- a/src/pages/workspace/WorkspaceMembersPage.tsx +++ b/src/pages/workspace/WorkspaceMembersPage.tsx @@ -743,7 +743,10 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers return null; } const bulkActionOptions = getBulkActionsButtonOptions(); - return (shouldUseNarrowLayout ? canSelectMultiple : selectedEmployees.length > 0) ? ( + + // The wide layout offers these actions in the floating bar over the table instead, so its header keeps the + // page's own buttons no matter what is selected. + return shouldUseNarrowLayout && canSelectMultiple ? ( variant={CONST.BUTTON_VARIANT.SUCCESS} shouldAlwaysShowDropdownMenu @@ -854,6 +857,7 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers shouldShowCustomField2Column={shouldShowCustomField2Column} onRowSelectionChange={setSelectedEmployees} headerComponent={tableHeaderComponent} + bulkActionOptions={canWriteMembers ? getBulkActionsButtonOptions() : undefined} /> )} diff --git a/tests/ui/WorkspaceMembersTest.tsx b/tests/ui/WorkspaceMembersTest.tsx index 4053871aff27..b492b0b3c51a 100644 --- a/tests/ui/WorkspaceMembersTest.tsx +++ b/tests/ui/WorkspaceMembersTest.tsx @@ -1,6 +1,5 @@ import {act, fireEvent, render, screen, waitFor, within} from '@testing-library/react-native'; -import ButtonWithDropdownMenu from '@components/ButtonWithDropdownMenu'; import ComposeProviders from '@components/ComposeProviders'; import HTMLEngineProvider from '@components/HTMLEngineProvider'; import {LocaleContextProvider} from '@components/LocaleContextProvider'; @@ -55,6 +54,21 @@ const renderPage = (initialRouteName: typeof SCREENS.WORKSPACE.MEMBERS, initialP ); }; +/** + * The wide layout offers the bulk actions in the floating bar over the table, which gives the first few an inline + * button and moves the rest behind "More". Opening that menu puts every action on screen at once, whichever half of + * the bar it landed in. Only the bar's button carries this label, so the page's own "More" button is not matched. + */ +const openBulkActions = async () => { + const moreButton = screen.queryByLabelText(TestHelper.translateLocal('common.more')); + if (!moreButton) { + return; + } + + fireEvent.press(moreButton); + await waitForBatchedUpdatesWithAct(); +}; + const selectCheckboxByMemberName = (memberName: string) => { const memberEmailByName: Record = { Owner: 'owner@gmail.com', @@ -144,46 +158,25 @@ describe('WorkspaceMembers', () => { // Select admin option by clicking the checkbox selectCheckboxByMemberName('Admin'); - const dropdownMenuButtonTestID = 'WorkspaceMembersPage-header-dropdown-menu-button'; - - // Wait for selection mode to be active and click the dropdown menu button - await waitFor(() => { - expect(screen.getByTestId(dropdownMenuButtonTestID)).toBeOnTheScreen(); - }); - // Click the "1 selected" button to open the menu - const dropdownButton = screen.getByTestId(dropdownMenuButtonTestID); - const bulkActionsDropdown = screen.UNSAFE_getAllByType(ButtonWithDropdownMenu).find(({props}) => props.testID === dropdownMenuButtonTestID); - expect(bulkActionsDropdown?.props.shouldPopoverUseScrollView).toBe(true); - fireEvent.press(dropdownButton); - - await waitForBatchedUpdatesWithAct(); - - // Wait for menu items to be visible - await waitFor(() => { - const makeMemberText = TestHelper.translateLocal('workspace.people.makeMember', {count: 1}); - expect(screen.getByText(makeMemberText)).toBeOnTheScreen(); - }); + // Open the actions the bar moved behind "More", so every action the selection supports is on screen + await openBulkActions(); - // Find and verify "Make member" dropdown menu item + // Find and verify the "Make member" action const makeMemberText = TestHelper.translateLocal('workspace.people.makeMember', {count: 1}); - const makeMemberMenuItem = screen.getByTestId(`PopoverMenuItem-${makeMemberText}`); - expect(makeMemberMenuItem).toBeOnTheScreen(); + expect(screen.getByText(makeMemberText)).toBeOnTheScreen(); - // Find and verify "Make auditor" dropdown menu item + // Find and verify the "Make auditor" action const makeAuditorText = TestHelper.translateLocal('workspace.people.makeAuditor', {count: 1}); - const makeAuditorMenuItem = screen.getByTestId(`PopoverMenuItem-${makeAuditorText}`); - expect(makeAuditorMenuItem).toBeOnTheScreen(); + expect(screen.getByText(makeAuditorText)).toBeOnTheScreen(); - // Find and verify "Make card admin" dropdown menu item + // Find and verify the "Make card admin" action const makeCardAdminText = TestHelper.translateLocal('workspace.people.makeCardAdmin', {count: 1}); - const makeCardAdminMenuItem = screen.getByTestId(`PopoverMenuItem-${makeCardAdminText}`); - expect(makeCardAdminMenuItem).toBeOnTheScreen(); + expect(screen.getByText(makeCardAdminText)).toBeOnTheScreen(); - // Find and verify "Make admin" dropdown menu item is not present + // Find and verify the "Make admin" action is not present const makeAdminText = TestHelper.translateLocal('workspace.people.makeAdmin', {count: 1}); - const makeAdminMenuItem = screen.queryByTestId(`PopoverMenuItem-${makeAdminText}`); - expect(makeAdminMenuItem).not.toBeOnTheScreen(); + expect(screen.queryByText(makeAdminText)).not.toBeOnTheScreen(); unmount(); await waitForBatchedUpdatesWithAct(); @@ -200,44 +193,25 @@ describe('WorkspaceMembers', () => { // Select member option by clicking the checkbox selectCheckboxByMemberName('Member'); - const dropdownMenuButtonTestID = 'WorkspaceMembersPage-header-dropdown-menu-button'; - // Wait for selection mode to be active and click the dropdown menu button - await waitFor(() => { - expect(screen.getByTestId(dropdownMenuButtonTestID)).toBeOnTheScreen(); - }); - - // Click the "1 selected" button to open the menu - const dropdownButton = screen.getByTestId(dropdownMenuButtonTestID); - fireEvent.press(dropdownButton); - - await waitForBatchedUpdatesWithAct(); + // Open the actions the bar moved behind "More", so every action the selection supports is on screen + await openBulkActions(); - // Wait for menu items to be visible - await waitFor(() => { - const makeAdminText = TestHelper.translateLocal('workspace.people.makeAdmin', {count: 1}); - expect(screen.getByText(makeAdminText)).toBeOnTheScreen(); - }); - - // Find and verify "Make admin" dropdown menu item + // Find and verify the "Make admin" action const makeAdminText = TestHelper.translateLocal('workspace.people.makeAdmin', {count: 1}); - const makeAdminMenuItem = screen.getByTestId(`PopoverMenuItem-${makeAdminText}`); - expect(makeAdminMenuItem).toBeOnTheScreen(); + expect(screen.getByText(makeAdminText)).toBeOnTheScreen(); - // Find and verify "Make auditor" dropdown menu item + // Find and verify the "Make auditor" action const makeAuditorText = TestHelper.translateLocal('workspace.people.makeAuditor', {count: 1}); - const makeAuditorMenuItem = screen.getByTestId(`PopoverMenuItem-${makeAuditorText}`); - expect(makeAuditorMenuItem).toBeOnTheScreen(); + expect(screen.getByText(makeAuditorText)).toBeOnTheScreen(); - // Find and verify "Make card admin" dropdown menu item + // Find and verify the "Make card admin" action const makeCardAdminText = TestHelper.translateLocal('workspace.people.makeCardAdmin', {count: 1}); - const makeCardAdminMenuItem = screen.getByTestId(`PopoverMenuItem-${makeCardAdminText}`); - expect(makeCardAdminMenuItem).toBeOnTheScreen(); + expect(screen.getByText(makeCardAdminText)).toBeOnTheScreen(); - // Find and verify "Make member" dropdown menu item is not present + // Find and verify the "Make member" action is not present const makeMemberText = TestHelper.translateLocal('workspace.people.makeMember', {count: 1}); - const makeMemberMenuItem = screen.queryByTestId(`PopoverMenuItem-${makeMemberText}`); - expect(makeMemberMenuItem).not.toBeOnTheScreen(); + expect(screen.queryByText(makeMemberText)).not.toBeOnTheScreen(); unmount(); await waitForBatchedUpdatesWithAct(); @@ -254,44 +228,25 @@ describe('WorkspaceMembers', () => { // Select auditor option by clicking the checkbox selectCheckboxByMemberName('Auditor'); - const dropdownMenuButtonTestID = 'WorkspaceMembersPage-header-dropdown-menu-button'; - - // Wait for selection mode to be active and click the dropdown menu button - await waitFor(() => { - expect(screen.getByTestId(dropdownMenuButtonTestID)).toBeOnTheScreen(); - }); - - // Click the "1 selected" button to open the menu - const dropdownButton = screen.getByTestId(dropdownMenuButtonTestID); - fireEvent.press(dropdownButton); - await waitForBatchedUpdatesWithAct(); - - // Wait for menu items to be visible - await waitFor(() => { - const makeMemberText = TestHelper.translateLocal('workspace.people.makeMember', {count: 1}); - expect(screen.getByText(makeMemberText)).toBeOnTheScreen(); - }); + // Open the actions the bar moved behind "More", so every action the selection supports is on screen + await openBulkActions(); - // Find and verify "Make member" dropdown menu item + // Find and verify the "Make member" action const makeMemberText = TestHelper.translateLocal('workspace.people.makeMember', {count: 1}); - const makeMemberMenuItem = screen.getByTestId(`PopoverMenuItem-${makeMemberText}`); - expect(makeMemberMenuItem).toBeOnTheScreen(); + expect(screen.getByText(makeMemberText)).toBeOnTheScreen(); - // Find and verify "Make admin" dropdown menu item + // Find and verify the "Make admin" action const makeAdminText = TestHelper.translateLocal('workspace.people.makeAdmin', {count: 1}); - const makeAdminMenuItem = screen.getByTestId(`PopoverMenuItem-${makeAdminText}`); - expect(makeAdminMenuItem).toBeOnTheScreen(); + expect(screen.getByText(makeAdminText)).toBeOnTheScreen(); - // Find and verify "Make card admin" dropdown menu item + // Find and verify the "Make card admin" action const makeCardAdminText = TestHelper.translateLocal('workspace.people.makeCardAdmin', {count: 1}); - const makeCardAdminMenuItem = screen.getByTestId(`PopoverMenuItem-${makeCardAdminText}`); - expect(makeCardAdminMenuItem).toBeOnTheScreen(); + expect(screen.getByText(makeCardAdminText)).toBeOnTheScreen(); - // Find and verify "Make auditor" dropdown menu item is not present + // Find and verify the "Make auditor" action is not present const makeAuditorText = TestHelper.translateLocal('workspace.people.makeAuditor', {count: 1}); - const makeAuditorMenuItem = screen.queryByTestId(`PopoverMenuItem-${makeAuditorText}`); - expect(makeAuditorMenuItem).not.toBeOnTheScreen(); + expect(screen.queryByText(makeAuditorText)).not.toBeOnTheScreen(); unmount(); await waitForBatchedUpdatesWithAct(); @@ -312,44 +267,25 @@ describe('WorkspaceMembers', () => { // Select options by clicking the checkboxes selectCheckboxByMemberName('Auditor'); selectCheckboxByMemberName('Admin'); - const dropdownMenuButtonTestID = 'WorkspaceMembersPage-header-dropdown-menu-button'; - - // Wait for selection mode to be active and click the dropdown menu button - await waitFor(() => { - expect(screen.getByTestId(dropdownMenuButtonTestID)).toBeOnTheScreen(); - }); - - // Click the "2 selected" button to open the menu - const dropdownButton = screen.getByTestId(dropdownMenuButtonTestID); - fireEvent.press(dropdownButton); - await waitForBatchedUpdatesWithAct(); + // Open the actions the bar moved behind "More", so every action the selection supports is on screen + await openBulkActions(); - // Wait for menu items to be visible - await waitFor(() => { - const makeMemberText = TestHelper.translateLocal('workspace.people.makeMember', {count: 2}); - expect(screen.getByText(makeMemberText)).toBeOnTheScreen(); - }); - - // Find and verify "Make members" dropdown menu item (plural form for 2 selected items) + // Find and verify the "Make members" action (plural form for 2 selected items) const makeMemberText = TestHelper.translateLocal('workspace.people.makeMember', {count: 2}); - const makeMemberMenuItem = screen.getByTestId(`PopoverMenuItem-${makeMemberText}`); - expect(makeMemberMenuItem).toBeOnTheScreen(); + expect(screen.getByText(makeMemberText)).toBeOnTheScreen(); - // Find and verify "Make admins" dropdown menu item (plural form for 2 selected items) + // Find and verify the "Make admins" action (plural form for 2 selected items) const makeAdminText = TestHelper.translateLocal('workspace.people.makeAdmin', {count: 2}); - const makeAdminMenuItem = screen.getByTestId(`PopoverMenuItem-${makeAdminText}`); - expect(makeAdminMenuItem).toBeOnTheScreen(); + expect(screen.getByText(makeAdminText)).toBeOnTheScreen(); - // Find and verify "Make auditors" dropdown menu item (plural form for 2 selected items) + // Find and verify the "Make auditors" action (plural form for 2 selected items) const makeAuditorText = TestHelper.translateLocal('workspace.people.makeAuditor', {count: 2}); - const makeAuditorMenuItem = screen.getByTestId(`PopoverMenuItem-${makeAuditorText}`); - expect(makeAuditorMenuItem).toBeOnTheScreen(); + expect(screen.getByText(makeAuditorText)).toBeOnTheScreen(); - // Find and verify "Make card admins" dropdown menu item (plural form for 2 selected items) + // Find and verify the "Make card admins" action (plural form for 2 selected items) const makeCardAdminText = TestHelper.translateLocal('workspace.people.makeCardAdmin', {count: 2}); - const makeCardAdminMenuItem = screen.getByTestId(`PopoverMenuItem-${makeCardAdminText}`); - expect(makeCardAdminMenuItem).toBeOnTheScreen(); + expect(screen.getByText(makeCardAdminText)).toBeOnTheScreen(); unmount(); await waitForBatchedUpdatesWithAct(); @@ -376,23 +312,22 @@ describe('WorkspaceMembers', () => { }); selectCheckboxByMemberName('Member'); - fireEvent.press(screen.getByTestId('WorkspaceMembersPage-header-dropdown-menu-button')); - await waitForBatchedUpdatesWithAct(); + await openBulkActions(); const removeText = TestHelper.translateLocal('workspace.people.removeMembersTitle', {count: 1}); - expect(screen.getByTestId(`PopoverMenuItem-${removeText}`)).toBeOnTheScreen(); + expect(screen.getByText(removeText)).toBeOnTheScreen(); const makeAuditorText = TestHelper.translateLocal('workspace.people.makeAuditor', {count: 1}); - expect(screen.getByTestId(`PopoverMenuItem-${makeAuditorText}`)).toBeOnTheScreen(); + expect(screen.getByText(makeAuditorText)).toBeOnTheScreen(); const makeAdminText = TestHelper.translateLocal('workspace.people.makeAdmin', {count: 1}); - expect(screen.queryByTestId(`PopoverMenuItem-${makeAdminText}`)).not.toBeOnTheScreen(); + expect(screen.queryByText(makeAdminText)).not.toBeOnTheScreen(); const makeCardAdminText = TestHelper.translateLocal('workspace.people.makeCardAdmin', {count: 1}); - expect(screen.queryByTestId(`PopoverMenuItem-${makeCardAdminText}`)).not.toBeOnTheScreen(); + expect(screen.queryByText(makeCardAdminText)).not.toBeOnTheScreen(); const makePeopleAdminText = TestHelper.translateLocal('workspace.people.makePeopleAdmin', {count: 1}); - expect(screen.queryByTestId(`PopoverMenuItem-${makePeopleAdminText}`)).not.toBeOnTheScreen(); + expect(screen.queryByText(makePeopleAdminText)).not.toBeOnTheScreen(); unmount(); await waitForBatchedUpdatesWithAct(); @@ -419,14 +354,13 @@ describe('WorkspaceMembers', () => { }); selectCheckboxByMemberName('Auditor'); - fireEvent.press(screen.getByTestId('WorkspaceMembersPage-header-dropdown-menu-button')); - await waitForBatchedUpdatesWithAct(); + await openBulkActions(); const makeMemberText = TestHelper.translateLocal('workspace.people.makeMember', {count: 1}); - expect(screen.getByTestId(`PopoverMenuItem-${makeMemberText}`)).toBeOnTheScreen(); + expect(screen.getByText(makeMemberText)).toBeOnTheScreen(); const makeAdminText = TestHelper.translateLocal('workspace.people.makeAdmin', {count: 1}); - expect(screen.queryByTestId(`PopoverMenuItem-${makeAdminText}`)).not.toBeOnTheScreen(); + expect(screen.queryByText(makeAdminText)).not.toBeOnTheScreen(); unmount(); await waitForBatchedUpdatesWithAct(); @@ -452,28 +386,27 @@ describe('WorkspaceMembers', () => { // When that payer is bulk-selected and the actions dropdown is opened selectCheckboxByMemberName('Admin'); - fireEvent.press(await screen.findByTestId('WorkspaceMembersPage-header-dropdown-menu-button')); - await waitForBatchedUpdatesWithAct(); + await openBulkActions(); // Then the Remove option is still available const removeText = TestHelper.translateLocal('workspace.people.removeMembersTitle', {count: 1}); await waitFor(() => { - expect(screen.getByTestId(`PopoverMenuItem-${removeText}`)).toBeOnTheScreen(); + expect(screen.getByText(removeText)).toBeOnTheScreen(); }); // ...the demotions that would strip the payer of pay capability are hidden const makeMemberText = TestHelper.translateLocal('workspace.people.makeMember', {count: 1}); - expect(screen.queryByTestId(`PopoverMenuItem-${makeMemberText}`)).not.toBeOnTheScreen(); + expect(screen.queryByText(makeMemberText)).not.toBeOnTheScreen(); const makeAuditorText = TestHelper.translateLocal('workspace.people.makeAuditor', {count: 1}); - expect(screen.queryByTestId(`PopoverMenuItem-${makeAuditorText}`)).not.toBeOnTheScreen(); + expect(screen.queryByText(makeAuditorText)).not.toBeOnTheScreen(); const makeCardAdminText = TestHelper.translateLocal('workspace.people.makeCardAdmin', {count: 1}); - expect(screen.queryByTestId(`PopoverMenuItem-${makeCardAdminText}`)).not.toBeOnTheScreen(); + expect(screen.queryByText(makeCardAdminText)).not.toBeOnTheScreen(); - // ...but Make payments admin IS offered — Payments Admin is a valid payer role + // ...but Make payments admin IS offered, since Payments Admin is a valid payer role const makePaymentsAdminText = TestHelper.translateLocal('workspace.people.makePaymentsAdmin', {count: 1}); - expect(screen.getByTestId(`PopoverMenuItem-${makePaymentsAdminText}`)).toBeOnTheScreen(); + expect(screen.getByText(makePaymentsAdminText)).toBeOnTheScreen(); unmount(); await waitForBatchedUpdatesWithAct(); @@ -502,25 +435,24 @@ describe('WorkspaceMembers', () => { // When that payer is bulk-selected and the actions dropdown is opened selectCheckboxByMemberName('Member'); - fireEvent.press(await screen.findByTestId('WorkspaceMembersPage-header-dropdown-menu-button')); - await waitForBatchedUpdatesWithAct(); + await openBulkActions(); // Then the Remove option is still available const removeText = TestHelper.translateLocal('workspace.people.removeMembersTitle', {count: 1}); await waitFor(() => { - expect(screen.getByTestId(`PopoverMenuItem-${removeText}`)).toBeOnTheScreen(); + expect(screen.getByText(removeText)).toBeOnTheScreen(); }); - // ...and "Make workspace admin" IS offered — Admin is a valid payer role + // ...and "Make workspace admin" IS offered, since Admin is a valid payer role const makeAdminText = TestHelper.translateLocal('workspace.people.makeAdmin', {count: 1}); - expect(screen.getByTestId(`PopoverMenuItem-${makeAdminText}`)).toBeOnTheScreen(); + expect(screen.getByText(makeAdminText)).toBeOnTheScreen(); // ...but the demotions that would strip the payer of pay capability stay hidden const makeMemberText = TestHelper.translateLocal('workspace.people.makeMember', {count: 1}); - expect(screen.queryByTestId(`PopoverMenuItem-${makeMemberText}`)).not.toBeOnTheScreen(); + expect(screen.queryByText(makeMemberText)).not.toBeOnTheScreen(); const makeAuditorText = TestHelper.translateLocal('workspace.people.makeAuditor', {count: 1}); - expect(screen.queryByTestId(`PopoverMenuItem-${makeAuditorText}`)).not.toBeOnTheScreen(); + expect(screen.queryByText(makeAuditorText)).not.toBeOnTheScreen(); unmount(); await waitForBatchedUpdatesWithAct(); @@ -537,9 +469,8 @@ describe('WorkspaceMembers', () => { // Select all fireEvent.press(screen.getByLabelText(TestHelper.translateLocal('workspace.common.selectAll'))); - // Open dropdown - fireEvent.press(await screen.findByTestId('WorkspaceMembersPage-header-dropdown-menu-button')); - await waitForBatchedUpdatesWithAct(); + // Open the bulk actions + await openBulkActions(); // Click "Remove members" const removeText = TestHelper.translateLocal('workspace.people.removeMembersTitle', {count: 3}); @@ -582,11 +513,10 @@ describe('WorkspaceMembers', () => { }); }; - const selectAdminAndOpenDropdown = async () => { + const selectAdminAndOpenBulkActions = async () => { await screen.findByText(ADMIN_OPTION); selectCheckboxByMemberName('Admin'); - fireEvent.press(await screen.findByTestId('WorkspaceMembersPage-header-dropdown-menu-button')); - await waitForBatchedUpdatesWithAct(); + await openBulkActions(); }; it('should show the unable-to-remove modal when removing a RuleBot enforcing agent rules', async () => { @@ -595,7 +525,7 @@ describe('WorkspaceMembers', () => { const {unmount} = renderPage(SCREENS.WORKSPACE.MEMBERS, {policyID: policy.id}); await waitForBatchedUpdatesWithAct(); - await selectAdminAndOpenDropdown(); + await selectAdminAndOpenBulkActions(); const removeMenuItem = screen.getByText(TestHelper.translateLocal('workspace.people.removeMembersTitle', {count: 1})); fireEvent.press(removeMenuItem, { @@ -620,7 +550,7 @@ describe('WorkspaceMembers', () => { const {unmount} = renderPage(SCREENS.WORKSPACE.MEMBERS, {policyID: policy.id}); await waitForBatchedUpdatesWithAct(); - await selectAdminAndOpenDropdown(); + await selectAdminAndOpenBulkActions(); const makeMemberMenuItem = screen.getByText(TestHelper.translateLocal('workspace.people.makeMember', {count: 1})); fireEvent.press(makeMemberMenuItem, { @@ -690,13 +620,14 @@ describe('WorkspaceMembers', () => { }; fireEvent.press(getSelectAllCheckbox()); await waitForBatchedUpdatesWithAct(); - expect(screen.getByTestId('WorkspaceMembersPage-header-dropdown-menu-button')).toBeOnTheScreen(); + const selectedLabel = TestHelper.translateLocal('workspace.common.selected', {count: 1}); + expect(screen.getByText(selectedLabel)).toBeOnTheScreen(); // Then clearing the search drops the selection, because it only ever applied to the searched rows fireEvent.changeText(searchInput, ''); await waitForBatchedUpdatesWithAct(); expect(screen.getByText(ADMIN_OPTION)).toBeOnTheScreen(); - expect(screen.queryByTestId('WorkspaceMembersPage-header-dropdown-menu-button')).not.toBeOnTheScreen(); + expect(screen.queryByText(selectedLabel)).not.toBeOnTheScreen(); expect(getSelectAllCheckbox()).not.toBeChecked(); unmount(); From c963cc97ae3ad721998826698d52d25c616f257f Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Mon, 28 Sep 2026 10:06:46 +0530 Subject: [PATCH 02/23] Drop focus before Esc clears a selection, so no focus ring appears Signed-off-by: krishna2323 --- src/components/BulkActionBar/index.tsx | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/components/BulkActionBar/index.tsx b/src/components/BulkActionBar/index.tsx index cde025fa2bb0..90356b8161b2 100644 --- a/src/components/BulkActionBar/index.tsx +++ b/src/components/BulkActionBar/index.tsx @@ -18,6 +18,7 @@ import useTheme from '@hooks/useTheme'; import useThemeStyles from '@hooks/useThemeStyles'; import Accessibility from '@libs/Accessibility'; +import blurActiveElement from '@libs/Accessibility/blurActiveElement'; import shouldPopoverUseScrollView from '@libs/shouldPopoverUseScrollView'; import CONST from '@src/CONST'; @@ -87,7 +88,16 @@ function BulkActionBarContent({ // keyup, shortcuts run on keydown, so ordering can't defer to them. `willAlertModalBecomeVisible` covers the open // animation, `isVisible` covers everything after, and an RHP only ever sets the latter. const [modal] = useOnyx(ONYXKEYS.MODAL); - useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ESCAPE, onClearSelection, {isActive: !modal?.willAlertModalBecomeVisible && !modal?.isVisible}); + useKeyboardShortcut( + CONST.KEYBOARD_SHORTCUTS.ESCAPE, + () => { + // A key press makes whatever holds focus match `:focus-visible`, so a checkbox clicked with the mouse would + // light up with a focus ring just as the selection it belongs to disappears. + blurActiveElement(); + onClearSelection(); + }, + {isActive: !modal?.willAlertModalBecomeVisible && !modal?.isVisible}, + ); useEffect(() => { if (!moreAnchorRef.current || !isMoreMenuVisible) { From 11fd6a4577f3b2feb264ed11fc75b07124f7e15c Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Mon, 28 Sep 2026 10:14:31 +0530 Subject: [PATCH 03/23] fix knip check. Signed-off-by: krishna2323 --- src/components/Table/TableBulkActionBar.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/Table/TableBulkActionBar.tsx b/src/components/Table/TableBulkActionBar.tsx index 0bc7fefc2098..8e7da80193fc 100644 --- a/src/components/Table/TableBulkActionBar.tsx +++ b/src/components/Table/TableBulkActionBar.tsx @@ -33,4 +33,3 @@ function TableBulkActionBar({onClearSelection, ...bulkActionBarProps } export default TableBulkActionBar; -export type {TableBulkActionBarProps}; From db1fb97f22563fb1a2a003a05c85ae30b2a1e2e0 Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Tue, 29 Sep 2026 08:20:02 +0530 Subject: [PATCH 04/23] Make the bulk action bar keyboard and screen reader accessible Signed-off-by: krishna2323 --- .../BulkActionBar/BulkActionBarButton.tsx | 1 + src/components/BulkActionBar/index.tsx | 46 +++-- .../useBulkActionBarFocus/index.native.ts | 8 + .../useBulkActionBarFocus/index.ts | 160 ++++++++++++++++++ .../useBulkActionBarFocus/types.ts | 14 ++ .../MoneyRequestReportActionsList.tsx | 18 +- src/components/Table/TableBody.tsx | 2 +- src/languages/de.ts | 5 + src/languages/el.ts | 5 + src/languages/en.ts | 5 + src/languages/es.ts | 5 + src/languages/fr.ts | 5 + src/languages/it.ts | 5 + src/languages/ja.ts | 5 + src/languages/nl.ts | 5 + src/languages/pl.ts | 5 + src/languages/pt-BR.ts | 5 + src/languages/zh-hans.ts | 5 + src/styles/index.ts | 7 + 19 files changed, 290 insertions(+), 21 deletions(-) create mode 100644 src/components/BulkActionBar/useBulkActionBarFocus/index.native.ts create mode 100644 src/components/BulkActionBar/useBulkActionBarFocus/index.ts create mode 100644 src/components/BulkActionBar/useBulkActionBarFocus/types.ts diff --git a/src/components/BulkActionBar/BulkActionBarButton.tsx b/src/components/BulkActionBar/BulkActionBarButton.tsx index f01abb64e003..ab744c33a719 100644 --- a/src/components/BulkActionBar/BulkActionBarButton.tsx +++ b/src/components/BulkActionBar/BulkActionBarButton.tsx @@ -73,6 +73,7 @@ function BulkActionBarButton({option, onSubItemSelected}: BulkAction size={CONST.BUTTON_SIZE.SMALL} isDisabled={option.disabled} accessibilityLabel={option.text} + accessibilityState={hasSubMenu ? {expanded: isMenuVisible} : undefined} sentryLabel={option.sentryLabel} > {!!option.icon && ( diff --git a/src/components/BulkActionBar/index.tsx b/src/components/BulkActionBar/index.tsx index 81542e6e105c..52b7204b110a 100644 --- a/src/components/BulkActionBar/index.tsx +++ b/src/components/BulkActionBar/index.tsx @@ -7,6 +7,7 @@ import Text from '@components/Text'; import ThemeProvider from '@components/ThemeProvider'; import ThemeStylesProvider from '@components/ThemeStylesContextProvider'; +import useAccessibilityAnnouncement from '@hooks/useAccessibilityAnnouncement'; import useInvertedThemePreference from '@hooks/useInvertedThemePreference'; import useKeyboardShortcut from '@hooks/useKeyboardShortcut'; import {useMemoizedLazyExpensifyIcons} from '@hooks/useLazyAsset'; @@ -18,7 +19,7 @@ import useTheme from '@hooks/useTheme'; import useThemeStyles from '@hooks/useThemeStyles'; import Accessibility from '@libs/Accessibility'; -import blurActiveElement from '@libs/Accessibility/blurActiveElement'; +import mergeRefs from '@libs/mergeRefs'; import shouldPopoverUseScrollView from '@libs/shouldPopoverUseScrollView'; import {setDisableDismissOnEscape} from '@userActions/Modal'; @@ -39,6 +40,7 @@ import type {BulkActionBarProps} from './types'; import BulkActionBarButton from './BulkActionBarButton'; import BulkActionBarMenuTheme from './BulkActionBarMenuTheme'; import {defaultPopoverAnchorPosition, MORE_MENU_ANCHOR_ALIGNMENT} from './popoverPosition'; +import useBulkActionBarFocus from './useBulkActionBarFocus'; /** * The bar's contents. Everything here takes its colors from the theme it is rendered under, which `BulkActionBar` @@ -77,6 +79,9 @@ function BulkActionBarContent({ const icons = useMemoizedLazyExpensifyIcons(['Close', 'DownArrow', 'UpArrow']); const {calculatePopoverPosition} = usePopoverPosition(); + const barElementRef = useRef | null>(null); + const {handleFocusBeforeClose} = useBulkActionBarFocus(barElementRef); + const moreAnchorRef = useRef | null>(null); const [isMoreMenuVisible, setIsMoreMenuVisible] = useState(false); const [moreMenuAnchorPosition, setMoreMenuAnchorPosition] = useState(defaultPopoverAnchorPosition); @@ -94,16 +99,13 @@ function BulkActionBarContent({ const isCoveredByModal = !!modal?.isVisible && !(isFocused && modal?.type === CONST.MODAL.MODAL_TYPE.RIGHT_DOCKED); const shouldClearSelectionOnEscape = !modal?.willAlertModalBecomeVisible && !isCoveredByModal; - useKeyboardShortcut( - CONST.KEYBOARD_SHORTCUTS.ESCAPE, - () => { - // A key press makes whatever holds focus match `:focus-visible`, so a checkbox clicked with the mouse would - // light up with a focus ring just as the selection it belongs to disappears. - blurActiveElement(); - onClearSelection(); - }, - {isActive: shouldClearSelectionOnEscape}, - ); + const clearSelection = () => { + handleFocusBeforeClose(); + onClearSelection(); + }; + + // Esc inside a text field is how you leave the field, so leave the selection alone there. + useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ESCAPE, clearSelection, {isActive: shouldClearSelectionOnEscape, captureOnInputs: false}); // Whichever Esc handler subscribed last runs first, so holding the pane back keeps it from closing out from under // a selection Esc was meant to clear. @@ -139,16 +141,29 @@ function BulkActionBarContent({ }; }, [isMoreMenuVisible, calculatePopoverPosition]); + // The bar appears where nothing was before, without taking focus, so a screen reader only learns about it and about + // the size of the selection it describes from an announcement. + useAccessibilityAnnouncement(countLabel, !isSelectedCountLoading, {shouldAnnounceOnWeb: true, shouldAnnounceOnNative: true, politeness: 'polite'}); + return ( onBarLayout(event.nativeEvent.layout.width)} > {/* Sized for a three-digit count so the bar keeps still as the selection grows, and so swapping the spinner for the count does not resize it either. */} - {isSelectedCountLoading ? : {countLabel}} + {isSelectedCountLoading ? ( + + ) : ( + {countLabel} + )} {!!noticeText && {noticeText}} {inlineOptions.map((option) => ( @@ -165,6 +180,7 @@ function BulkActionBarContent({ size={CONST.BUTTON_SIZE.SMALL} onPress={() => setIsMoreMenuVisible((isVisible) => !isVisible)} accessibilityLabel={translate('common.more')} + accessibilityState={{expanded: isMoreMenuVisible}} sentryLabel={CONST.SENTRY_LABEL.BULK_ACTION_BAR.MORE} > {translate('common.more')} @@ -197,8 +213,8 @@ function BulkActionBarContent({ )} {}; + +/** Native has no tab order and no focus ring, so the bar's controls are reached through the screen reader instead. */ +const useBulkActionBarFocus: UseBulkActionBarFocus = () => ({handleFocusBeforeClose: noop}); + +export default useBulkActionBarFocus; diff --git a/src/components/BulkActionBar/useBulkActionBarFocus/index.ts b/src/components/BulkActionBar/useBulkActionBarFocus/index.ts new file mode 100644 index 000000000000..07fbf27029cb --- /dev/null +++ b/src/components/BulkActionBar/useBulkActionBarFocus/index.ts @@ -0,0 +1,160 @@ +import FOCUSABLE_SELECTOR from '@libs/focusableSelector'; +import isHTMLElement from '@libs/isHTMLElement'; +import markProgrammaticFocus from '@libs/programmaticFocus'; + +import CONST from '@src/CONST'; + +import type {RefObject} from 'react'; + +import {useEffect, useRef} from 'react'; + +import type UseBulkActionBarFocus from './types'; + +const HOME_KEY = 'Home'; +const END_KEY = 'End'; + +function getBarElement(barRef: RefObject): HTMLElement | null { + return isHTMLElement(barRef.current) ? barRef.current : null; +} + +function getItems(bar: HTMLElement): HTMLElement[] { + return Array.from(bar.querySelectorAll(FOCUSABLE_SELECTOR)); +} + +/** + * Whether the browser is already drawing a focus ring on this element. Browsers that cannot be asked are assumed to be + * drawing one, so a ring is never taken away from someone who navigated there with the keyboard. + */ +function isShowingFocusRing(element: HTMLElement): boolean { + try { + return element.matches(':focus-visible'); + } catch { + return true; + } +} + +/** + * Makes the bar a single stop in the tab order, the way the ARIA toolbar pattern asks for, and settles where focus goes + * when the selection is cleared. + * + * Tab moves onto the bar and straight off it again, while Left, Right, Home and End move between its actions. Giving + * every action its own tab stop would otherwise make a four-action bar five stops between the list and whatever follows + * it. + */ +const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef) => { + // What Tab lands on, kept in step with wherever focus actually went, so returning to the bar comes back to the + // action last used rather than to the first one. + const tabStopRef = useRef(null); + + // Where focus was before the bar took it. The bar goes away with the selection, so focus has to be handed back. + const lastFocusedOutsideRef = useRef(null); + + // The fitting pass changes which actions have a button of their own, and React restores each new button's own tab + // stop, so the roving one is reapplied after every render rather than once on mount. + useEffect(() => { + const bar = getBarElement(barRef); + if (!bar) { + return; + } + + const items = getItems(bar); + const tabStop = tabStopRef.current && items.includes(tabStopRef.current) ? tabStopRef.current : items.at(0); + tabStopRef.current = tabStop ?? null; + + for (const item of items) { + item.tabIndex = item === tabStop ? 0 : -1; + } + }); + + useEffect(() => { + const handleFocusIn = (event: FocusEvent) => { + const target = event.target; + if (!isHTMLElement(target)) { + return; + } + + const bar = getBarElement(barRef); + if (bar?.contains(target)) { + tabStopRef.current = target; + return; + } + + lastFocusedOutsideRef.current = target; + }; + + document.addEventListener('focusin', handleFocusIn); + + return () => document.removeEventListener('focusin', handleFocusIn); + }, [barRef]); + + useEffect(() => { + const bar = getBarElement(barRef); + if (!bar) { + return; + } + + const handleKeyDown = (event: KeyboardEvent) => { + const items = getItems(bar); + if (items.length === 0) { + return; + } + + const currentIndex = isHTMLElement(document.activeElement) ? items.indexOf(document.activeElement) : -1; + if (currentIndex === -1) { + return; + } + + let nextIndex: number; + if (event.key === CONST.KEYBOARD_SHORTCUTS.ARROW_LEFT.shortcutKey) { + nextIndex = (currentIndex - 1 + items.length) % items.length; + } else if (event.key === CONST.KEYBOARD_SHORTCUTS.ARROW_RIGHT.shortcutKey) { + nextIndex = (currentIndex + 1) % items.length; + } else if (event.key === HOME_KEY) { + nextIndex = 0; + } else if (event.key === END_KEY) { + nextIndex = items.length - 1; + } else { + return; + } + + // Left and Right scroll the page horizontally, and Home and End jump it, so the bar has to claim them. + event.preventDefault(); + items.at(nextIndex)?.focus(); + }; + + bar.addEventListener('keydown', handleKeyDown); + + return () => bar.removeEventListener('keydown', handleKeyDown); + }, [barRef]); + + const handleFocusBeforeClose = () => { + const active = document.activeElement; + if (!isHTMLElement(active)) { + return; + } + + const bar = getBarElement(barRef); + if (bar?.contains(active)) { + const previous = lastFocusedOutsideRef.current; + if (!previous?.isConnected) { + active.blur(); + return; + } + + // Focus is being put back where the user left it rather than moved somewhere new, so it arrives without a ring. + markProgrammaticFocus(previous); + previous.focus(); + return; + } + + // Esc makes whatever holds focus match `:focus-visible`, so a checkbox clicked with the mouse would light up + // with a focus ring just as the selection it belongs to disappears. + if (!isShowingFocusRing(active)) { + markProgrammaticFocus(active); + } + }; + + return {handleFocusBeforeClose}; +}; + +export default useBulkActionBarFocus; diff --git a/src/components/BulkActionBar/useBulkActionBarFocus/types.ts b/src/components/BulkActionBar/useBulkActionBarFocus/types.ts new file mode 100644 index 000000000000..43608c2e985b --- /dev/null +++ b/src/components/BulkActionBar/useBulkActionBarFocus/types.ts @@ -0,0 +1,14 @@ +import type {RefObject} from 'react'; + +type BulkActionBarFocus = { + /** + * Settles where focus should be before the selection goes away and takes the bar with it. Call it from every path + * that clears the selection. + */ + handleFocusBeforeClose: () => void; +}; + +type UseBulkActionBarFocus = (barRef: RefObject) => BulkActionBarFocus; + +export default UseBulkActionBarFocus; +export type {BulkActionBarFocus}; diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx index 5344b559e761..36e95b85914d 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx @@ -293,13 +293,20 @@ function MoneyRequestReportActionsListContent({reportIDFromRoute, onLayout}: Mon rules, }); + // The narrow layout puts its selection controls in a header above the list, while the wide layout floats a bar over + // the end of it. The floating bar has to follow the list here rather than lead it, or Tab reaches it only after the + // rest of the app has been walked through and wrapped back around. + const selectionToolbar = ( + + ); + return ( - + {shouldUseNarrowLayout && selectionToolbar} )} + {!shouldUseNarrowLayout && selectionToolbar} ); } diff --git a/src/components/Table/TableBody.tsx b/src/components/Table/TableBody.tsx index 71bd22567065..fbf8c976bdae 100644 --- a/src/components/Table/TableBody.tsx +++ b/src/components/Table/TableBody.tsx @@ -385,7 +385,7 @@ function TableBodyList({contentContainerStyle, emptyMessage, onLayout, style, .. ref={listRef} data={listData} - style={[styles.flex1, styles.mnh0]} + style={[styles.flex1, styles.mnh0, isBulkActionBarVisible && styles.bulkActionBarScrollPadding]} showsVerticalScrollIndicator={false} maintainVisibleContentPosition={{disabled: true}} ListHeaderComponent={pageHeaderElement} diff --git a/src/languages/de.ts b/src/languages/de.ts index 1b8f0e88be85..ce5939f0bfd8 100644 --- a/src/languages/de.ts +++ b/src/languages/de.ts @@ -34,6 +34,11 @@ type StateValue = { type States = Record; type AllCountries = Record; const translations: TranslationDeepObject = { + bulkActionBar: { + label: 'Massenaktionen', + clearSelection: 'Auswahl aufheben', + loadingSelection: 'Auswahl wird geladen', + }, common: { durationDays: ({count}: {count: number}) => ({ one: `1 Tag`, diff --git a/src/languages/el.ts b/src/languages/el.ts index 150e5abc1e95..3e831bcf90d1 100644 --- a/src/languages/el.ts +++ b/src/languages/el.ts @@ -31,6 +31,11 @@ type StateValue = { type States = Record; type AllCountries = Record; const translations: TranslationDeepObject = { + bulkActionBar: { + label: 'Μαζικές ενέργειες', + clearSelection: 'Εκκαθάριση επιλογής', + loadingSelection: 'Φόρτωση επιλογής', + }, common: { durationDays: ({count}: {count: number}) => ({ one: `1 ημέρα`, diff --git a/src/languages/en.ts b/src/languages/en.ts index 467b9a867020..2c040501478f 100644 --- a/src/languages/en.ts +++ b/src/languages/en.ts @@ -36,6 +36,11 @@ type States = Record; type AllCountries = Record; const translations = { + bulkActionBar: { + label: 'Bulk actions', + clearSelection: 'Clear selection', + loadingSelection: 'Loading selection', + }, common: { durationDays: ({count}: {count: number}) => ({ one: '1 day', diff --git a/src/languages/es.ts b/src/languages/es.ts index f8b9b288a7d6..cb7b7626afca 100644 --- a/src/languages/es.ts +++ b/src/languages/es.ts @@ -20,6 +20,11 @@ import type en from './en'; import type {AllConnectionName, PolicyConnectionSyncStage} from './TranslationTypes'; import type {TranslationDeepObject} from './types'; const translations: TranslationDeepObject = { + bulkActionBar: { + label: 'Acciones masivas', + clearSelection: 'Borrar selección', + loadingSelection: 'Cargando selección', + }, common: { durationDays: ({count}: {count: number}) => ({ one: `1 día`, diff --git a/src/languages/fr.ts b/src/languages/fr.ts index 25aa1b34cabc..48b0b2be2b8f 100644 --- a/src/languages/fr.ts +++ b/src/languages/fr.ts @@ -34,6 +34,11 @@ type StateValue = { type States = Record; type AllCountries = Record; const translations: TranslationDeepObject = { + bulkActionBar: { + label: 'Actions groupées', + clearSelection: 'Effacer la sélection', + loadingSelection: 'Chargement de la sélection', + }, common: { durationDays: ({count}: {count: number}) => ({ one: `${count} jour`, diff --git a/src/languages/it.ts b/src/languages/it.ts index 6064e9b5cc38..89fdd54608fd 100644 --- a/src/languages/it.ts +++ b/src/languages/it.ts @@ -34,6 +34,11 @@ type StateValue = { type States = Record; type AllCountries = Record; const translations: TranslationDeepObject = { + bulkActionBar: { + label: 'Azioni in blocco', + clearSelection: 'Cancella selezione', + loadingSelection: 'Caricamento della selezione', + }, common: { durationDays: ({count}: {count: number}) => ({ one: `1 giorno`, diff --git a/src/languages/ja.ts b/src/languages/ja.ts index 042a7c0d4ab0..47150e5a047d 100644 --- a/src/languages/ja.ts +++ b/src/languages/ja.ts @@ -34,6 +34,11 @@ type StateValue = { type States = Record; type AllCountries = Record; const translations: TranslationDeepObject = { + bulkActionBar: { + label: '一括操作', + clearSelection: '選択を解除', + loadingSelection: '選択を読み込み中', + }, common: { durationDays: ({count}: {count: number}) => ({ one: `1 日`, diff --git a/src/languages/nl.ts b/src/languages/nl.ts index eae278301c6c..bf57b4a65385 100644 --- a/src/languages/nl.ts +++ b/src/languages/nl.ts @@ -34,6 +34,11 @@ type StateValue = { type States = Record; type AllCountries = Record; const translations: TranslationDeepObject = { + bulkActionBar: { + label: 'Bulkacties', + clearSelection: 'Selectie wissen', + loadingSelection: 'Selectie laden', + }, common: { durationDays: ({count}: {count: number}) => ({ one: `1 dag`, diff --git a/src/languages/pl.ts b/src/languages/pl.ts index 482f1f887b4a..c5bef799ab18 100644 --- a/src/languages/pl.ts +++ b/src/languages/pl.ts @@ -34,6 +34,11 @@ type StateValue = { type States = Record; type AllCountries = Record; const translations: TranslationDeepObject = { + bulkActionBar: { + label: 'Akcje zbiorcze', + clearSelection: 'Wyczyść zaznaczenie', + loadingSelection: 'Ładowanie zaznaczenia', + }, common: { durationDays: ({count}: {count: number}) => ({ one: `1 dzień`, diff --git a/src/languages/pt-BR.ts b/src/languages/pt-BR.ts index ce56c5cad2bc..dc01dd5533d3 100644 --- a/src/languages/pt-BR.ts +++ b/src/languages/pt-BR.ts @@ -34,6 +34,11 @@ type StateValue = { type States = Record; type AllCountries = Record; const translations: TranslationDeepObject = { + bulkActionBar: { + label: 'Ações em massa', + clearSelection: 'Limpar seleção', + loadingSelection: 'Carregando seleção', + }, common: { durationDays: ({count}: {count: number}) => ({ one: `${count} dia`, diff --git a/src/languages/zh-hans.ts b/src/languages/zh-hans.ts index ba103eb5ca9e..68d0e80e395b 100644 --- a/src/languages/zh-hans.ts +++ b/src/languages/zh-hans.ts @@ -34,6 +34,11 @@ type StateValue = { type States = Record; type AllCountries = Record; const translations: TranslationDeepObject = { + bulkActionBar: { + label: '批量操作', + clearSelection: '清除选择', + loadingSelection: '正在加载选择', + }, common: { durationDays: ({count}: {count: number}) => ({ one: `1 天`, diff --git a/src/styles/index.ts b/src/styles/index.ts index c85b7c7f0472..4f95b663800a 100644 --- a/src/styles/index.ts +++ b/src/styles/index.ts @@ -5572,6 +5572,13 @@ const staticStyles = (theme: ThemeColors) => paddingBottom: bulkActionBarHeight + CONST.BULK_ACTION_BAR.BOTTOM_OFFSET + CONST.BULK_ACTION_BAR.LIST_GAP, }, + // Keeps a row the browser scrolls into view, such as one reached with Tab, from settling underneath the bar + // (WCAG 2.4.11). The content spacing above only lifts the end of the list clear of it. Applied to the scrolling + // container, which is what the browser measures that scroll against. + bulkActionBarScrollPadding: { + scrollPaddingBottom: bulkActionBarHeight + CONST.BULK_ACTION_BAR.BOTTOM_OFFSET + CONST.BULK_ACTION_BAR.LIST_GAP, + }, + // Wide enough for a three-digit count, so the bar does not resize as the selection grows past 9 or 99. A // selection can cover far more rows than are on screen. Also keeps the width steady while the count loads. bulkActionBarCount: { From b6d297b8d5ea5b0855439e1a24c129a3487373ed Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Tue, 29 Sep 2026 08:38:52 +0530 Subject: [PATCH 05/23] Let Tab move through the bulk action bar's actions Signed-off-by: krishna2323 --- .../useBulkActionBarFocus/index.ts | 95 +------------------ 1 file changed, 5 insertions(+), 90 deletions(-) diff --git a/src/components/BulkActionBar/useBulkActionBarFocus/index.ts b/src/components/BulkActionBar/useBulkActionBarFocus/index.ts index 07fbf27029cb..b235f523ef79 100644 --- a/src/components/BulkActionBar/useBulkActionBarFocus/index.ts +++ b/src/components/BulkActionBar/useBulkActionBarFocus/index.ts @@ -1,26 +1,10 @@ -import FOCUSABLE_SELECTOR from '@libs/focusableSelector'; import isHTMLElement from '@libs/isHTMLElement'; import markProgrammaticFocus from '@libs/programmaticFocus'; -import CONST from '@src/CONST'; - -import type {RefObject} from 'react'; - import {useEffect, useRef} from 'react'; import type UseBulkActionBarFocus from './types'; -const HOME_KEY = 'Home'; -const END_KEY = 'End'; - -function getBarElement(barRef: RefObject): HTMLElement | null { - return isHTMLElement(barRef.current) ? barRef.current : null; -} - -function getItems(bar: HTMLElement): HTMLElement[] { - return Array.from(bar.querySelectorAll(FOCUSABLE_SELECTOR)); -} - /** * Whether the browser is already drawing a focus ring on this element. Browsers that cannot be asked are assumed to be * drawing one, so a ring is never taken away from someone who navigated there with the keyboard. @@ -33,39 +17,11 @@ function isShowingFocusRing(element: HTMLElement): boolean { } } -/** - * Makes the bar a single stop in the tab order, the way the ARIA toolbar pattern asks for, and settles where focus goes - * when the selection is cleared. - * - * Tab moves onto the bar and straight off it again, while Left, Right, Home and End move between its actions. Giving - * every action its own tab stop would otherwise make a four-action bar five stops between the list and whatever follows - * it. - */ +/** Settles where focus goes when the selection is cleared and the bar that describes it disappears. */ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef) => { - // What Tab lands on, kept in step with wherever focus actually went, so returning to the bar comes back to the - // action last used rather than to the first one. - const tabStopRef = useRef(null); - // Where focus was before the bar took it. The bar goes away with the selection, so focus has to be handed back. const lastFocusedOutsideRef = useRef(null); - // The fitting pass changes which actions have a button of their own, and React restores each new button's own tab - // stop, so the roving one is reapplied after every render rather than once on mount. - useEffect(() => { - const bar = getBarElement(barRef); - if (!bar) { - return; - } - - const items = getItems(bar); - const tabStop = tabStopRef.current && items.includes(tabStopRef.current) ? tabStopRef.current : items.at(0); - tabStopRef.current = tabStop ?? null; - - for (const item of items) { - item.tabIndex = item === tabStop ? 0 : -1; - } - }); - useEffect(() => { const handleFocusIn = (event: FocusEvent) => { const target = event.target; @@ -73,9 +29,8 @@ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef) => { return; } - const bar = getBarElement(barRef); - if (bar?.contains(target)) { - tabStopRef.current = target; + const bar = barRef.current; + if (isHTMLElement(bar) && bar.contains(target)) { return; } @@ -87,54 +42,14 @@ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef) => { return () => document.removeEventListener('focusin', handleFocusIn); }, [barRef]); - useEffect(() => { - const bar = getBarElement(barRef); - if (!bar) { - return; - } - - const handleKeyDown = (event: KeyboardEvent) => { - const items = getItems(bar); - if (items.length === 0) { - return; - } - - const currentIndex = isHTMLElement(document.activeElement) ? items.indexOf(document.activeElement) : -1; - if (currentIndex === -1) { - return; - } - - let nextIndex: number; - if (event.key === CONST.KEYBOARD_SHORTCUTS.ARROW_LEFT.shortcutKey) { - nextIndex = (currentIndex - 1 + items.length) % items.length; - } else if (event.key === CONST.KEYBOARD_SHORTCUTS.ARROW_RIGHT.shortcutKey) { - nextIndex = (currentIndex + 1) % items.length; - } else if (event.key === HOME_KEY) { - nextIndex = 0; - } else if (event.key === END_KEY) { - nextIndex = items.length - 1; - } else { - return; - } - - // Left and Right scroll the page horizontally, and Home and End jump it, so the bar has to claim them. - event.preventDefault(); - items.at(nextIndex)?.focus(); - }; - - bar.addEventListener('keydown', handleKeyDown); - - return () => bar.removeEventListener('keydown', handleKeyDown); - }, [barRef]); - const handleFocusBeforeClose = () => { const active = document.activeElement; if (!isHTMLElement(active)) { return; } - const bar = getBarElement(barRef); - if (bar?.contains(active)) { + const bar = barRef.current; + if (isHTMLElement(bar) && bar.contains(active)) { const previous = lastFocusedOutsideRef.current; if (!previous?.isConnected) { active.blur(); From eb206976772a455e65939dedbb296c9beae69fd7 Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Tue, 29 Sep 2026 08:42:49 +0530 Subject: [PATCH 06/23] remove translations. Signed-off-by: krishna2323 --- src/languages/de.ts | 5 ----- src/languages/el.ts | 5 ----- src/languages/es.ts | 5 ----- src/languages/fr.ts | 5 ----- src/languages/it.ts | 5 ----- src/languages/ja.ts | 5 ----- src/languages/nl.ts | 5 ----- src/languages/pl.ts | 5 ----- src/languages/pt-BR.ts | 5 ----- src/languages/zh-hans.ts | 5 ----- 10 files changed, 50 deletions(-) diff --git a/src/languages/de.ts b/src/languages/de.ts index ce5939f0bfd8..1b8f0e88be85 100644 --- a/src/languages/de.ts +++ b/src/languages/de.ts @@ -34,11 +34,6 @@ type StateValue = { type States = Record; type AllCountries = Record; const translations: TranslationDeepObject = { - bulkActionBar: { - label: 'Massenaktionen', - clearSelection: 'Auswahl aufheben', - loadingSelection: 'Auswahl wird geladen', - }, common: { durationDays: ({count}: {count: number}) => ({ one: `1 Tag`, diff --git a/src/languages/el.ts b/src/languages/el.ts index 3e831bcf90d1..150e5abc1e95 100644 --- a/src/languages/el.ts +++ b/src/languages/el.ts @@ -31,11 +31,6 @@ type StateValue = { type States = Record; type AllCountries = Record; const translations: TranslationDeepObject = { - bulkActionBar: { - label: 'Μαζικές ενέργειες', - clearSelection: 'Εκκαθάριση επιλογής', - loadingSelection: 'Φόρτωση επιλογής', - }, common: { durationDays: ({count}: {count: number}) => ({ one: `1 ημέρα`, diff --git a/src/languages/es.ts b/src/languages/es.ts index cb7b7626afca..f8b9b288a7d6 100644 --- a/src/languages/es.ts +++ b/src/languages/es.ts @@ -20,11 +20,6 @@ import type en from './en'; import type {AllConnectionName, PolicyConnectionSyncStage} from './TranslationTypes'; import type {TranslationDeepObject} from './types'; const translations: TranslationDeepObject = { - bulkActionBar: { - label: 'Acciones masivas', - clearSelection: 'Borrar selección', - loadingSelection: 'Cargando selección', - }, common: { durationDays: ({count}: {count: number}) => ({ one: `1 día`, diff --git a/src/languages/fr.ts b/src/languages/fr.ts index 48b0b2be2b8f..25aa1b34cabc 100644 --- a/src/languages/fr.ts +++ b/src/languages/fr.ts @@ -34,11 +34,6 @@ type StateValue = { type States = Record; type AllCountries = Record; const translations: TranslationDeepObject = { - bulkActionBar: { - label: 'Actions groupées', - clearSelection: 'Effacer la sélection', - loadingSelection: 'Chargement de la sélection', - }, common: { durationDays: ({count}: {count: number}) => ({ one: `${count} jour`, diff --git a/src/languages/it.ts b/src/languages/it.ts index 89fdd54608fd..6064e9b5cc38 100644 --- a/src/languages/it.ts +++ b/src/languages/it.ts @@ -34,11 +34,6 @@ type StateValue = { type States = Record; type AllCountries = Record; const translations: TranslationDeepObject = { - bulkActionBar: { - label: 'Azioni in blocco', - clearSelection: 'Cancella selezione', - loadingSelection: 'Caricamento della selezione', - }, common: { durationDays: ({count}: {count: number}) => ({ one: `1 giorno`, diff --git a/src/languages/ja.ts b/src/languages/ja.ts index 47150e5a047d..042a7c0d4ab0 100644 --- a/src/languages/ja.ts +++ b/src/languages/ja.ts @@ -34,11 +34,6 @@ type StateValue = { type States = Record; type AllCountries = Record; const translations: TranslationDeepObject = { - bulkActionBar: { - label: '一括操作', - clearSelection: '選択を解除', - loadingSelection: '選択を読み込み中', - }, common: { durationDays: ({count}: {count: number}) => ({ one: `1 日`, diff --git a/src/languages/nl.ts b/src/languages/nl.ts index bf57b4a65385..eae278301c6c 100644 --- a/src/languages/nl.ts +++ b/src/languages/nl.ts @@ -34,11 +34,6 @@ type StateValue = { type States = Record; type AllCountries = Record; const translations: TranslationDeepObject = { - bulkActionBar: { - label: 'Bulkacties', - clearSelection: 'Selectie wissen', - loadingSelection: 'Selectie laden', - }, common: { durationDays: ({count}: {count: number}) => ({ one: `1 dag`, diff --git a/src/languages/pl.ts b/src/languages/pl.ts index c5bef799ab18..482f1f887b4a 100644 --- a/src/languages/pl.ts +++ b/src/languages/pl.ts @@ -34,11 +34,6 @@ type StateValue = { type States = Record; type AllCountries = Record; const translations: TranslationDeepObject = { - bulkActionBar: { - label: 'Akcje zbiorcze', - clearSelection: 'Wyczyść zaznaczenie', - loadingSelection: 'Ładowanie zaznaczenia', - }, common: { durationDays: ({count}: {count: number}) => ({ one: `1 dzień`, diff --git a/src/languages/pt-BR.ts b/src/languages/pt-BR.ts index dc01dd5533d3..ce56c5cad2bc 100644 --- a/src/languages/pt-BR.ts +++ b/src/languages/pt-BR.ts @@ -34,11 +34,6 @@ type StateValue = { type States = Record; type AllCountries = Record; const translations: TranslationDeepObject = { - bulkActionBar: { - label: 'Ações em massa', - clearSelection: 'Limpar seleção', - loadingSelection: 'Carregando seleção', - }, common: { durationDays: ({count}: {count: number}) => ({ one: `${count} dia`, diff --git a/src/languages/zh-hans.ts b/src/languages/zh-hans.ts index 68d0e80e395b..ba103eb5ca9e 100644 --- a/src/languages/zh-hans.ts +++ b/src/languages/zh-hans.ts @@ -34,11 +34,6 @@ type StateValue = { type States = Record; type AllCountries = Record; const translations: TranslationDeepObject = { - bulkActionBar: { - label: '批量操作', - clearSelection: '清除选择', - loadingSelection: '正在加载选择', - }, common: { durationDays: ({count}: {count: number}) => ({ one: `1 天`, From 98742ebb577d22bfa4c1aa44c9f209b293ee712d Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Tue, 29 Sep 2026 08:47:21 +0530 Subject: [PATCH 07/23] Drop the unused BulkActionBarFocus export Signed-off-by: krishna2323 --- src/components/BulkActionBar/useBulkActionBarFocus/types.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/BulkActionBar/useBulkActionBarFocus/types.ts b/src/components/BulkActionBar/useBulkActionBarFocus/types.ts index 43608c2e985b..7465051d085a 100644 --- a/src/components/BulkActionBar/useBulkActionBarFocus/types.ts +++ b/src/components/BulkActionBar/useBulkActionBarFocus/types.ts @@ -11,4 +11,3 @@ type BulkActionBarFocus = { type UseBulkActionBarFocus = (barRef: RefObject) => BulkActionBarFocus; export default UseBulkActionBarFocus; -export type {BulkActionBarFocus}; From 59fa50dea174c346d4c76c0664b1920b23471228 Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Tue, 29 Sep 2026 08:56:50 +0530 Subject: [PATCH 08/23] Keep Enter on a bar button from also opening the focused row Signed-off-by: krishna2323 --- src/components/BulkActionBar/index.tsx | 9 ++++++- .../useBulkActionBarFocus/index.native.ts | 2 +- .../useBulkActionBarFocus/index.ts | 25 ++++++++++++++++--- .../useBulkActionBarFocus/types.ts | 3 +++ 4 files changed, 33 insertions(+), 6 deletions(-) diff --git a/src/components/BulkActionBar/index.tsx b/src/components/BulkActionBar/index.tsx index 52b7204b110a..4eff14a2afc1 100644 --- a/src/components/BulkActionBar/index.tsx +++ b/src/components/BulkActionBar/index.tsx @@ -42,6 +42,8 @@ import BulkActionBarMenuTheme from './BulkActionBarMenuTheme'; import {defaultPopoverAnchorPosition, MORE_MENU_ANCHOR_ALIGNMENT} from './popoverPosition'; import useBulkActionBarFocus from './useBulkActionBarFocus'; +const noop = () => {}; + /** * The bar's contents. Everything here takes its colors from the theme it is rendered under, which `BulkActionBar` * inverts, so the surface, the buttons and the "More" menu all read as one layer without any of them being styled @@ -80,7 +82,7 @@ function BulkActionBarContent({ const {calculatePopoverPosition} = usePopoverPosition(); const barElementRef = useRef | null>(null); - const {handleFocusBeforeClose} = useBulkActionBarFocus(barElementRef); + const {handleFocusBeforeClose, isFocusInsideBar} = useBulkActionBarFocus(barElementRef); const moreAnchorRef = useRef | null>(null); const [isMoreMenuVisible, setIsMoreMenuVisible] = useState(false); @@ -107,6 +109,11 @@ function BulkActionBarContent({ // Esc inside a text field is how you leave the field, so leave the selection alone there. useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ESCAPE, clearSelection, {isActive: shouldClearSelectionOnEscape, captureOnInputs: false}); + // The lists this bar floats over run a global Enter shortcut off a keyboard cursor that outlives tabbing away from + // the rows. While one of the bar's own buttons is focused, claim Enter without bubbling so pressing it only works + // that button instead of also opening whichever row the cursor was left on. Same guard as SearchPageFooter's. + useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ENTER, noop, {isActive: isFocusInsideBar, shouldBubble: false, shouldPreventDefault: false}); + // Whichever Esc handler subscribed last runs first, so holding the pane back keeps it from closing out from under // a selection Esc was meant to clear. useEffect(() => { diff --git a/src/components/BulkActionBar/useBulkActionBarFocus/index.native.ts b/src/components/BulkActionBar/useBulkActionBarFocus/index.native.ts index 48d05d1cdef6..c441513a764b 100644 --- a/src/components/BulkActionBar/useBulkActionBarFocus/index.native.ts +++ b/src/components/BulkActionBar/useBulkActionBarFocus/index.native.ts @@ -3,6 +3,6 @@ import type UseBulkActionBarFocus from './types'; const noop = () => {}; /** Native has no tab order and no focus ring, so the bar's controls are reached through the screen reader instead. */ -const useBulkActionBarFocus: UseBulkActionBarFocus = () => ({handleFocusBeforeClose: noop}); +const useBulkActionBarFocus: UseBulkActionBarFocus = () => ({handleFocusBeforeClose: noop, isFocusInsideBar: false}); export default useBulkActionBarFocus; diff --git a/src/components/BulkActionBar/useBulkActionBarFocus/index.ts b/src/components/BulkActionBar/useBulkActionBarFocus/index.ts index b235f523ef79..45b1f8cbf0c4 100644 --- a/src/components/BulkActionBar/useBulkActionBarFocus/index.ts +++ b/src/components/BulkActionBar/useBulkActionBarFocus/index.ts @@ -1,7 +1,7 @@ import isHTMLElement from '@libs/isHTMLElement'; import markProgrammaticFocus from '@libs/programmaticFocus'; -import {useEffect, useRef} from 'react'; +import {useEffect, useRef, useState} from 'react'; import type UseBulkActionBarFocus from './types'; @@ -17,11 +17,13 @@ function isShowingFocusRing(element: HTMLElement): boolean { } } -/** Settles where focus goes when the selection is cleared and the bar that describes it disappears. */ +/** Tracks where focus is relative to the bar, and settles where it goes when the selection is cleared. */ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef) => { // Where focus was before the bar took it. The bar goes away with the selection, so focus has to be handed back. const lastFocusedOutsideRef = useRef(null); + const [isFocusInsideBar, setIsFocusInsideBar] = useState(false); + useEffect(() => { const handleFocusIn = (event: FocusEvent) => { const target = event.target; @@ -31,15 +33,30 @@ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef) => { const bar = barRef.current; if (isHTMLElement(bar) && bar.contains(target)) { + setIsFocusInsideBar(true); return; } + setIsFocusInsideBar(false); lastFocusedOutsideRef.current = target; }; + // Focus can also leave for nothing at all, such as an element being removed, which fires no matching focusin. + const handleFocusOut = (event: FocusEvent) => { + if (event.relatedTarget) { + return; + } + + setIsFocusInsideBar(false); + }; + document.addEventListener('focusin', handleFocusIn); + document.addEventListener('focusout', handleFocusOut); - return () => document.removeEventListener('focusin', handleFocusIn); + return () => { + document.removeEventListener('focusin', handleFocusIn); + document.removeEventListener('focusout', handleFocusOut); + }; }, [barRef]); const handleFocusBeforeClose = () => { @@ -69,7 +86,7 @@ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef) => { } }; - return {handleFocusBeforeClose}; + return {handleFocusBeforeClose, isFocusInsideBar}; }; export default useBulkActionBarFocus; diff --git a/src/components/BulkActionBar/useBulkActionBarFocus/types.ts b/src/components/BulkActionBar/useBulkActionBarFocus/types.ts index 7465051d085a..a41fb1a281cb 100644 --- a/src/components/BulkActionBar/useBulkActionBarFocus/types.ts +++ b/src/components/BulkActionBar/useBulkActionBarFocus/types.ts @@ -6,6 +6,9 @@ type BulkActionBarFocus = { * that clears the selection. */ handleFocusBeforeClose: () => void; + + /** Whether one of the bar's own controls currently holds focus. */ + isFocusInsideBar: boolean; }; type UseBulkActionBarFocus = (barRef: RefObject) => BulkActionBarFocus; From 0d76bfb79d1923adce1ae41916a81fca56042db5 Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Tue, 29 Sep 2026 09:10:46 +0530 Subject: [PATCH 09/23] Hand focus back whenever an action takes the bulk action bar away Signed-off-by: krishna2323 --- src/components/BulkActionBar/index.tsx | 18 ++++---- .../useBulkActionBarFocus/index.native.ts | 2 +- .../useBulkActionBarFocus/index.ts | 42 +++++++++++++------ .../useBulkActionBarFocus/types.ts | 6 +-- 4 files changed, 43 insertions(+), 25 deletions(-) diff --git a/src/components/BulkActionBar/index.tsx b/src/components/BulkActionBar/index.tsx index 4eff14a2afc1..288f592db72e 100644 --- a/src/components/BulkActionBar/index.tsx +++ b/src/components/BulkActionBar/index.tsx @@ -82,7 +82,7 @@ function BulkActionBarContent({ const {calculatePopoverPosition} = usePopoverPosition(); const barElementRef = useRef | null>(null); - const {handleFocusBeforeClose, isFocusInsideBar} = useBulkActionBarFocus(barElementRef); + const {suppressStrayFocusRing, isFocusInsideBar} = useBulkActionBarFocus(barElementRef); const moreAnchorRef = useRef | null>(null); const [isMoreMenuVisible, setIsMoreMenuVisible] = useState(false); @@ -101,13 +101,15 @@ function BulkActionBarContent({ const isCoveredByModal = !!modal?.isVisible && !(isFocused && modal?.type === CONST.MODAL.MODAL_TYPE.RIGHT_DOCKED); const shouldClearSelectionOnEscape = !modal?.willAlertModalBecomeVisible && !isCoveredByModal; - const clearSelection = () => { - handleFocusBeforeClose(); - onClearSelection(); - }; - // Esc inside a text field is how you leave the field, so leave the selection alone there. - useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ESCAPE, clearSelection, {isActive: shouldClearSelectionOnEscape, captureOnInputs: false}); + useKeyboardShortcut( + CONST.KEYBOARD_SHORTCUTS.ESCAPE, + () => { + suppressStrayFocusRing(); + onClearSelection(); + }, + {isActive: shouldClearSelectionOnEscape, captureOnInputs: false}, + ); // The lists this bar floats over run a global Enter shortcut off a keyboard cursor that outlives tabbing away from // the rows. While one of the bar's own buttons is focused, claim Enter without bubbling so pressing it only works @@ -220,7 +222,7 @@ function BulkActionBarContent({ )} {}; /** Native has no tab order and no focus ring, so the bar's controls are reached through the screen reader instead. */ -const useBulkActionBarFocus: UseBulkActionBarFocus = () => ({handleFocusBeforeClose: noop, isFocusInsideBar: false}); +const useBulkActionBarFocus: UseBulkActionBarFocus = () => ({suppressStrayFocusRing: noop, isFocusInsideBar: false}); export default useBulkActionBarFocus; diff --git a/src/components/BulkActionBar/useBulkActionBarFocus/index.ts b/src/components/BulkActionBar/useBulkActionBarFocus/index.ts index 45b1f8cbf0c4..3b025c391498 100644 --- a/src/components/BulkActionBar/useBulkActionBarFocus/index.ts +++ b/src/components/BulkActionBar/useBulkActionBarFocus/index.ts @@ -1,7 +1,7 @@ import isHTMLElement from '@libs/isHTMLElement'; import markProgrammaticFocus from '@libs/programmaticFocus'; -import {useEffect, useRef, useState} from 'react'; +import {useEffect, useLayoutEffect, useRef, useState} from 'react'; import type UseBulkActionBarFocus from './types'; @@ -17,7 +17,7 @@ function isShowingFocusRing(element: HTMLElement): boolean { } } -/** Tracks where focus is relative to the bar, and settles where it goes when the selection is cleared. */ +/** Tracks where focus is relative to the bar, and hands it back when the bar goes away holding it. */ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef) => { // Where focus was before the bar took it. The bar goes away with the selection, so focus has to be handed back. const lastFocusedOutsideRef = useRef(null); @@ -59,34 +59,50 @@ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef) => { }; }, [barRef]); - const handleFocusBeforeClose = () => { - const active = document.activeElement; - if (!isHTMLElement(active)) { - return; - } + // Most of the bar's actions clear the selection themselves, so the bar is torn out from under the button that was + // just pressed and focus would fall to the document. Handing it back here covers every one of those actions, + // rather than only the two the bar runs itself. A layout cleanup rather than a passive one, because this has to + // read the focus before the bar leaves the DOM. + useLayoutEffect( + () => () => { + const bar = barRef.current; + if (!isHTMLElement(bar) || !bar.contains(document.activeElement)) { + return; + } - const bar = barRef.current; - if (isHTMLElement(bar) && bar.contains(active)) { const previous = lastFocusedOutsideRef.current; if (!previous?.isConnected) { - active.blur(); return; } // Focus is being put back where the user left it rather than moved somewhere new, so it arrives without a ring. markProgrammaticFocus(previous); previous.focus(); + }, + [barRef], + ); + + const suppressStrayFocusRing = () => { + const active = document.activeElement; + if (!isHTMLElement(active)) { + return; + } + + const bar = barRef.current; + if (isHTMLElement(bar) && bar.contains(active)) { return; } // Esc makes whatever holds focus match `:focus-visible`, so a checkbox clicked with the mouse would light up // with a focus ring just as the selection it belongs to disappears. - if (!isShowingFocusRing(active)) { - markProgrammaticFocus(active); + if (isShowingFocusRing(active)) { + return; } + + markProgrammaticFocus(active); }; - return {handleFocusBeforeClose, isFocusInsideBar}; + return {suppressStrayFocusRing, isFocusInsideBar}; }; export default useBulkActionBarFocus; diff --git a/src/components/BulkActionBar/useBulkActionBarFocus/types.ts b/src/components/BulkActionBar/useBulkActionBarFocus/types.ts index a41fb1a281cb..196fcbf66a64 100644 --- a/src/components/BulkActionBar/useBulkActionBarFocus/types.ts +++ b/src/components/BulkActionBar/useBulkActionBarFocus/types.ts @@ -2,10 +2,10 @@ import type {RefObject} from 'react'; type BulkActionBarFocus = { /** - * Settles where focus should be before the selection goes away and takes the bar with it. Call it from every path - * that clears the selection. + * Keeps Esc from painting a focus ring on a control the pointer put focus on. Focus that is inside the bar is left + * alone, since the bar hands that back itself as it unmounts. */ - handleFocusBeforeClose: () => void; + suppressStrayFocusRing: () => void; /** Whether one of the bar's own controls currently holds focus. */ isFocusInsideBar: boolean; From df938d8d0d8ca599aafa9f45b9198e6b2799724b Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Thu, 1 Oct 2026 15:27:50 +0530 Subject: [PATCH 10/23] Consolidate member bulk role actions into a Change role selector Signed-off-by: krishna2323 --- src/CONST/index.ts | 5 +- src/ONYXKEYS.ts | 4 + src/ROUTES.ts | 4 + src/SCREENS.ts | 1 + src/components/WorkspaceMemberRoleList.tsx | 23 +- src/languages/en.ts | 1 + src/languages/es.ts | 1 + src/libs/ExportOnyxState/masking.ts | 1 + .../ModalStackNavigators/index.tsx | 1 + src/libs/Navigation/linkingConfig/config.ts | 3 + src/libs/Navigation/types.ts | 3 + src/libs/PolicyUtils.ts | 22 ++ src/libs/actions/Policy/Member.ts | 10 + src/pages/workspace/WorkspaceMembersPage.tsx | 138 ++----- .../members/WorkspaceMembersRolePage.tsx | 122 +++++++ src/setup/index.ts | 1 + tests/ui/WorkspaceMembersTest.tsx | 345 +++++------------- 17 files changed, 325 insertions(+), 360 deletions(-) create mode 100644 src/pages/workspace/members/WorkspaceMembersRolePage.tsx diff --git a/src/CONST/index.ts b/src/CONST/index.ts index d38e411c1beb..2d0f9217fd65 100644 --- a/src/CONST/index.ts +++ b/src/CONST/index.ts @@ -4607,10 +4607,7 @@ const CONST = { REMOVE: 'remove', MAKE_MEMBER: 'makeMember', MAKE_ADMIN: 'makeAdmin', - MAKE_AUDITOR: 'makeAuditor', - MAKE_CARD_ADMIN: 'makeCardAdmin', - MAKE_PEOPLE_ADMIN: 'makePeopleAdmin', - MAKE_PAYMENTS_ADMIN: 'makePaymentsAdmin', + CHANGE_ROLE: 'changeRole', }, BULK_ACTION_TYPES: { DELETE: 'delete', diff --git a/src/ONYXKEYS.ts b/src/ONYXKEYS.ts index 65112e36a7cc..8104d63bb31f 100755 --- a/src/ONYXKEYS.ts +++ b/src/ONYXKEYS.ts @@ -531,6 +531,9 @@ const ONYXKEYS = { /** Selected domain member account IDs for the move-to-group operation */ RAM_ONLY_DOMAIN_MEMBERS_SELECTED_FOR_MOVE: 'domainMembersSelectedForMove', + /** Logins of the workspace members selected for the bulk role change */ + RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE: 'workspaceMembersSelectedForRoleChange', + // The theme setting set by the user in preferences. // This can be either "light", "dark", "system", "light-contrast", "dark-contrast" or "system-contrast" PREFERRED_THEME: 'nvp_preferredTheme', @@ -1773,6 +1776,7 @@ type OnyxValuesMapping = { [ONYXKEYS.VIEWING_PUBLIC_ROOM_REPORT_ID]: string; [ONYXKEYS.MY_DOMAIN_SECURITY_GROUPS]: Record; [ONYXKEYS.RAM_ONLY_DOMAIN_MEMBERS_SELECTED_FOR_MOVE]: string[]; + [ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE]: string[]; [ONYXKEYS.VERIFY_3DS_SUBSCRIPTION]: string; [ONYXKEYS.PREFERRED_THEME]: ValueOf; [ONYXKEYS.SIGN_IN_HIGH_CONTRAST_INTENT]: boolean; diff --git a/src/ROUTES.ts b/src/ROUTES.ts index b622e2275fae..7dce94e1c684 100644 --- a/src/ROUTES.ts +++ b/src/ROUTES.ts @@ -3637,6 +3637,10 @@ const ROUTES = { route: 'workspaces/:policyID/taxes/settings/tax-name', getRoute: (policyID: string) => `workspaces/${policyID}/taxes/settings/tax-name` as const, }, + WORKSPACE_MEMBERS_ROLE: { + route: 'workspaces/:policyID/change-member-roles', + getRoute: (policyID: string) => `workspaces/${policyID}/change-member-roles` as const, + }, WORKSPACE_MEMBER_DETAILS: { route: 'workspaces/:policyID/members/:accountID', getRoute: (policyID: string, accountID: number) => `workspaces/${policyID}/members/${accountID}` as const, diff --git a/src/SCREENS.ts b/src/SCREENS.ts index 716357746b26..2028f3e0996f 100644 --- a/src/SCREENS.ts +++ b/src/SCREENS.ts @@ -945,6 +945,7 @@ const SCREENS = { MORE_FEATURES: 'Workspace_More_Features', MEMBER_DETAILS: 'Workspace_Member_Details', MEMBER_DETAILS_ROLE: 'Workspace_Member_Details_Role', + MEMBERS_ROLE: 'Workspace_Members_Role', MEMBER_WORK_ARRANGEMENT: 'Workspace_Member_Work_Arrangement', MEMBER_CUSTOM_FIELD: 'Workspace_Member_Custom_Field', MEMBER_NEW_CARD: 'Workspace_Member_NewCard', diff --git a/src/components/WorkspaceMemberRoleList.tsx b/src/components/WorkspaceMemberRoleList.tsx index b569350f1969..7f42a9c1ee2f 100644 --- a/src/components/WorkspaceMemberRoleList.tsx +++ b/src/components/WorkspaceMemberRoleList.tsx @@ -3,7 +3,7 @@ import useLocalize from '@hooks/useLocalize'; import useThemeStyles from '@hooks/useThemeStyles'; import Navigation from '@libs/Navigation/Navigation'; -import {canMemberAssignRole} from '@libs/PolicyUtils'; +import {getSelectableRoles} from '@libs/PolicyUtils'; import CONST from '@src/CONST'; import type {Route} from '@src/ROUTES'; @@ -15,7 +15,7 @@ import type {ValueOf} from 'type-fest'; import React from 'react'; import {View} from 'react-native'; -import type {ListItem} from './SelectionList/types'; +import type {ConfirmButtonOptions, ListItem} from './SelectionList/types'; import HeaderWithBackButton from './HeaderWithBackButton'; import SelectionList from './SelectionList'; @@ -37,9 +37,20 @@ type WorkspaceMemberRoleListProps = { /** When provided, restricts the selectable roles to this set (e.g. an Authorized Payer may only be an Admin or Payments Admin) */ allowedRoles?: Array>; + + /** When provided, the list confirms the pick with a button instead of applying it as soon as a row is pressed */ + confirmButtonOptions?: ConfirmButtonOptions; }; -function WorkspaceMemberRoleList({role, policy, navigateBackTo = undefined, isLoading = false, onSelectRole = () => {}, allowedRoles = undefined}: WorkspaceMemberRoleListProps) { +function WorkspaceMemberRoleList({ + role, + policy, + navigateBackTo = undefined, + isLoading = false, + onSelectRole = () => {}, + allowedRoles = undefined, + confirmButtonOptions = undefined, +}: WorkspaceMemberRoleListProps) { const {translate} = useLocalize(); const styles = useThemeStyles(); const {login: currentUserLogin = ''} = useCurrentUserPersonalDetails(); @@ -89,9 +100,8 @@ function WorkspaceMemberRoleList({role, policy, navigateBackTo = undefined, isLo }, ]; - const availableRoleItems: ListItemType[] = workspaceRoles.filter( - (item) => canMemberAssignRole(policy, currentUserLogin, item.value) && (!allowedRoles || allowedRoles.includes(item.value)), - ); + const selectableRoles = getSelectableRoles(policy, currentUserLogin, allowedRoles); + const availableRoleItems: ListItemType[] = workspaceRoles.filter((item) => selectableRoles.includes(item.value)); return ( <> @@ -105,6 +115,7 @@ function WorkspaceMemberRoleList({role, policy, navigateBackTo = undefined, isLo data={availableRoleItems} ListItem={SingleSelectListItem} onSelectRow={onSelectRole} + confirmButtonOptions={confirmButtonOptions} shouldSingleExecuteRowSelect initiallyFocusedItemKey={availableRoleItems.find((item) => item.isSelected)?.keyForList} addBottomSafeAreaPadding diff --git a/src/languages/en.ts b/src/languages/en.ts index 643062cfa658..24bba2f1e562 100644 --- a/src/languages/en.ts +++ b/src/languages/en.ts @@ -7390,6 +7390,7 @@ const translations = { removeMemberPrompt: (memberName: string) => `Are you sure you want to remove ${memberName}?`, removeMemberTitle: 'Remove member', transferOwner: 'Transfer owner', + changeRole: 'Change role', makeMember: () => ({ one: 'Make member', other: 'Make members', diff --git a/src/languages/es.ts b/src/languages/es.ts index 24d35d42ed5b..317cff5dba3d 100644 --- a/src/languages/es.ts +++ b/src/languages/es.ts @@ -6954,6 +6954,7 @@ El plan Controlar empieza en 9 $ por miembro activo al mes.`, removeMemberPrompt: (memberName) => `¿Estás seguro de que deseas eliminar a ${memberName}?`, removeMemberTitle: 'Eliminar miembro', transferOwner: 'Transferir la propiedad', + changeRole: 'Cambiar rol', makeMember: () => ({ one: 'Convertir en miembro', other: 'Convertir en miembros', diff --git a/src/libs/ExportOnyxState/masking.ts b/src/libs/ExportOnyxState/masking.ts index ab9b84b12f1c..8cc7a4199487 100644 --- a/src/libs/ExportOnyxState/masking.ts +++ b/src/libs/ExportOnyxState/masking.ts @@ -499,6 +499,7 @@ const onyxKeysToMaskFragileData = new Set([ ONYXKEYS.RAM_ONLY_SEARCH_TAG_FILTERS_PAGINATION, ONYXKEYS.RAM_ONLY_SEARCH_TAG_FILTERS_RESULTS, ONYXKEYS.RAM_ONLY_WALLET_ONFIDO, + ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE, ONYXKEYS.RECENTLY_USED_CURRENCIES, ONYXKEYS.RECENTLY_USED_REPORT_FIELDS, ONYXKEYS.RECENT_SEARCHES, diff --git a/src/libs/Navigation/AppNavigator/ModalStackNavigators/index.tsx b/src/libs/Navigation/AppNavigator/ModalStackNavigators/index.tsx index a0124ec2f372..266b3b4101fa 100644 --- a/src/libs/Navigation/AppNavigator/ModalStackNavigators/index.tsx +++ b/src/libs/Navigation/AppNavigator/ModalStackNavigators/index.tsx @@ -596,6 +596,7 @@ const SettingsModalStackNavigator = createModalStackNavigator require('../../../../pages/workspace/members/WorkspaceMemberDetailsPage').default, [SCREENS.WORKSPACE.ROOM_CREATE]: () => require('../../../../pages/workspace/rooms/WorkspaceRoomCreatePage').default, [SCREENS.WORKSPACE.MEMBER_DETAILS_ROLE]: () => require('../../../../pages/workspace/members/WorkspaceMemberDetailsRolePage').default, + [SCREENS.WORKSPACE.MEMBERS_ROLE]: () => require('../../../../pages/workspace/members/WorkspaceMembersRolePage').default, [SCREENS.WORKSPACE.MEMBER_WORK_ARRANGEMENT]: () => require('../../../../pages/workspace/members/WorkArrangementPage').default, [SCREENS.WORKSPACE.MEMBER_CUSTOM_FIELD]: () => require('../../../../pages/workspace/members/WorkspaceMemberCustomFieldPage').default, [SCREENS.WORKSPACE.DYNAMIC_OWNER_CHANGE_CHECK]: () => require('@pages/workspace/members/DynamicWorkspaceOwnerChangeWrapperPage').default, diff --git a/src/libs/Navigation/linkingConfig/config.ts b/src/libs/Navigation/linkingConfig/config.ts index a8598a47f1a3..b4bf237d8996 100644 --- a/src/libs/Navigation/linkingConfig/config.ts +++ b/src/libs/Navigation/linkingConfig/config.ts @@ -1144,6 +1144,9 @@ const config: LinkingOptions['config'] = { [SCREENS.WORKSPACE.MEMBER_DETAILS_ROLE]: { path: ROUTES.WORKSPACE_MEMBER_DETAILS_ROLE.route, }, + [SCREENS.WORKSPACE.MEMBERS_ROLE]: { + path: ROUTES.WORKSPACE_MEMBERS_ROLE.route, + }, [SCREENS.WORKSPACE.MEMBER_WORK_ARRANGEMENT]: { path: ROUTES.WORKSPACE_MEMBER_WORK_ARRANGEMENT.route, }, diff --git a/src/libs/Navigation/types.ts b/src/libs/Navigation/types.ts index 2a3238e5335b..7021346ea0e7 100644 --- a/src/libs/Navigation/types.ts +++ b/src/libs/Navigation/types.ts @@ -776,6 +776,9 @@ type SettingsNavigatorParamList = { policyID: string; accountID: string; }; + [SCREENS.WORKSPACE.MEMBERS_ROLE]: { + policyID: string; + }; [SCREENS.WORKSPACE.MEMBER_WORK_ARRANGEMENT]: { policyID: string; accountID: string; diff --git a/src/libs/PolicyUtils.ts b/src/libs/PolicyUtils.ts index 5f80ce8fcf89..481d6e4a6857 100644 --- a/src/libs/PolicyUtils.ts +++ b/src/libs/PolicyUtils.ts @@ -315,6 +315,27 @@ function canMemberAssignRole(policy: OnyxInputOrEntry, login: string, ro return isCorporatePolicy && canMemberWrite(policy, login, CONST.POLICY.POLICY_FEATURE.MEMBERS) && isNonElevatedRole; } +/** + * The roles a member can be moved to, in the order the role selector lists them. Owner and Editor are left out + * because neither is assignable through the role selector. + */ +const SELECTABLE_ROLES = [ + CONST.POLICY.ROLE.ADMIN, + CONST.POLICY.ROLE.AUDITOR, + CONST.POLICY.ROLE.CARD_ADMIN, + CONST.POLICY.ROLE.PEOPLE_ADMIN, + CONST.POLICY.ROLE.PAYMENTS_ADMIN, + CONST.POLICY.ROLE.USER, +] as const; + +/** + * The roles the given member may assign on this policy, optionally narrowed to a permitted set (for example an + * Authorized Payer may only hold a role that can pay). + */ +function getSelectableRoles(policy: OnyxInputOrEntry, login: string, allowedRoles?: Array>): Array> { + return SELECTABLE_ROLES.filter((role) => canMemberAssignRole(policy, login, role) && (!allowedRoles || allowedRoles.includes(role))); +} + // Whether the member can assign any elevated role: admins (via assignElevatedRoles) on any policy, or People Admins (up to auditor) on Control. function canMemberAssignElevatedRole(policy: OnyxInputOrEntry, login: string): boolean { return canMemberWrite(policy, login, CONST.POLICY.POLICY_FEATURE.ASSIGN_ELEVATED_ROLES) || canMemberAssignRole(policy, login, CONST.POLICY.ROLE.AUDITOR); @@ -3679,6 +3700,7 @@ export { canMemberRead, canMemberWrite, canMemberAssignRole, + getSelectableRoles, canMemberAssignElevatedRole, canMemberManageMemberWithRole, isGroupPolicy, diff --git a/src/libs/actions/Policy/Member.ts b/src/libs/actions/Policy/Member.ts index 28ce92be4fec..d8062dc3f799 100644 --- a/src/libs/actions/Policy/Member.ts +++ b/src/libs/actions/Policy/Member.ts @@ -1277,6 +1277,14 @@ function setWorkspaceInviteMembersDraft(policyID: string, invitedEmailsToAccount Onyx.set(`${ONYXKEYS.COLLECTION.WORKSPACE_INVITE_MEMBERS_DRAFT}${policyID}`, invitedEmailsToAccountIDs); } +function setMembersSelectedForRoleChange(memberLogins: string[]) { + Onyx.set(ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE, memberLogins); +} + +function clearMembersSelectedForRoleChange() { + Onyx.set(ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE, []); +} + function setWorkspaceInviteRoleDraft(policyID: string, role: ValueOf) { Onyx.set(`${ONYXKEYS.COLLECTION.WORKSPACE_INVITE_ROLE_DRAFT}${policyID}`, role); } @@ -1480,6 +1488,8 @@ export { buildRoomMembersOnyxData, openPolicyMemberProfilePage, setWorkspaceInviteRoleDraft, + setMembersSelectedForRoleChange, + clearMembersSelectedForRoleChange, clearWorkspaceInviteRoleDraft, setWorkspaceInviteApproverDraft, clearWorkspaceInviteApproverDraft, diff --git a/src/pages/workspace/WorkspaceMembersPage.tsx b/src/pages/workspace/WorkspaceMembersPage.tsx index 3fe9720c28c7..977c7e1e9909 100644 --- a/src/pages/workspace/WorkspaceMembersPage.tsx +++ b/src/pages/workspace/WorkspaceMembersPage.tsx @@ -41,7 +41,7 @@ import { downloadMembersCSV, openWorkspaceMembersPage, removeMembers, - updateWorkspaceMembersRole, + setMembersSelectedForRoleChange, } from '@libs/actions/Policy/Member'; import {removeApprovalWorkflow as removeApprovalWorkflowAction, updateApprovalWorkflow} from '@libs/actions/Workflow'; import {isRuleBotEnforcingRules} from '@libs/AgentRulesUtils'; @@ -56,12 +56,12 @@ import {isPersonalDetailsReady} from '@libs/OptionsListUtils'; import {getPersonalDetailsByID, temporaryGetDisplayNameOrDefault} from '@libs/PersonalDetailsUtils'; import { canEditWorkspaceSettings as canEditWorkspaceSettingsUtil, - canMemberAssignRole, canMemberManageMemberWithRole, canMemberWrite, getConnectionExporters, getMemberAccountIDsForWorkspace, getReimburserEmail, + getSelectableRoles, isControlPolicy, isDeletedPolicyEmployee, isExpensifyTeam, @@ -69,6 +69,7 @@ import { isPaidGroupPolicy, isPolicyApprover, isSubmitPolicy, + PAYER_ROLES, shouldFilterExpensifyTeam, } from '@libs/PolicyUtils'; import {getDisplayNameForParticipant, isApproverOfOutstandingPolicyReports} from '@libs/ReportUtils'; @@ -112,7 +113,7 @@ function invertObject(object: Record): Record { function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembersPageProps) { useWorkspaceDocumentTitle(policy?.name, 'common.members'); const tableRef = useRef>(null); - const icons = useMemoizedLazyExpensifyIcons(['Download', 'FallbackAvatar', 'MakeAdmin', 'Plus', 'RemoveMembers', 'Sync', 'Table', 'User', 'UserEye']); + const icons = useMemoizedLazyExpensifyIcons(['Download', 'FallbackAvatar', 'MakeAdmin', 'Plus', 'RemoveMembers', 'Sync', 'Table']); const employeePersonalDetails = usePersonalDetailsByLogins(Object.keys(policy?.employeeList ?? {})); const policyMemberEmailsToAccountIDs = useMemo( () => getMemberAccountIDsForWorkspace(policy?.employeeList, employeePersonalDetails, true), @@ -330,7 +331,6 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers ); const policyOwner = policy?.owner; - const canAssignElevatedRoles = canMemberWrite(policy, currentUserLogin ?? '', CONST.POLICY.POLICY_FEATURE.ASSIGN_ELEVATED_ROLES); const invitedPrimaryToSecondaryLogins = useMemo(() => invertObject(policy?.primaryLoginsInvited ?? {}), [policy?.primaryLoginsInvited]); const isControlPolicyWithWideLayout = !shouldUseNarrowLayout && isControlPolicy(policy); @@ -517,22 +517,18 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers onNavigationCallBack: () => Navigation.goBack(), }); - const hasSelectedRuleBot = selectedEmployees.some((email) => isRuleBotEnforcingRules(policyMemberEmailsToAccountIDs[email], policy)); + // The role change happens on its own screen, which reports the change back by emptying the shared selection. + // Backing out of that screen leaves the selection in place, so only a drop to zero clears the rows here. + const [membersSelectedForRoleChange] = useOnyx(ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE); + const membersSelectedForRoleChangeCount = membersSelectedForRoleChange?.length ?? 0; + const [lastMembersSelectedForRoleChangeCount, setLastMembersSelectedForRoleChangeCount] = useState(membersSelectedForRoleChangeCount); + if (lastMembersSelectedForRoleChangeCount !== membersSelectedForRoleChangeCount) { + setLastMembersSelectedForRoleChangeCount(membersSelectedForRoleChangeCount); - const changeUserRole = (role: ValueOf) => { - if (role !== CONST.POLICY.ROLE.ADMIN && hasSelectedRuleBot) { - showRuleBotGuardModal('changeRole', policyID); - return; + if (lastMembersSelectedForRoleChangeCount > 0 && membersSelectedForRoleChangeCount === 0) { + setSelectedEmployees([]); } - const loginsToUpdate = selectedEmployees.filter((login) => { - return policy?.employeeList?.[login]?.role !== role; - }); - - const accountIDsToUpdate = loginsToUpdate.map((login) => policyMemberEmailsToAccountIDs[login]).filter((id) => id !== undefined); - - setSelectedEmployees([]); - updateWorkspaceMembersRole(policy, loginsToUpdate, accountIDsToUpdate, role); - }; + } const getBulkActionsButtonOptions = () => { const selectedEmployeesRoles = selectedEmployees.map((email) => { @@ -541,6 +537,24 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers const canManageSelectedEmployees = selectedEmployeesRoles.every((role) => canMemberManageMemberWithRole(policy, currentUserLogin ?? '', role)); const options: Array> = []; + // The Authorized Payer (reimburser) must stay a valid payer, so a selection holding them may only be moved to a role that can pay. + const reimburserEmail = getReimburserEmail(policy); + const hasAtLeastOnePayer = !!reimburserEmail && selectedEmployees.includes(reimburserEmail); + const selectableRoles = getSelectableRoles(policy, currentUserLogin ?? '', hasAtLeastOnePayer ? [...PAYER_ROLES] : undefined); + + if (isPaidGroupPolicy(policy) && canManageSelectedEmployees && selectableRoles.length > 0) { + options.push({ + text: translate('workspace.people.changeRole'), + value: CONST.POLICY.MEMBERS_BULK_ACTION_TYPES.CHANGE_ROLE, + icon: icons.MakeAdmin, + shouldSkipFocusRestore: true, + onSelected: () => { + setMembersSelectedForRoleChange(selectedEmployees); + Navigation.navigate(ROUTES.WORKSPACE_MEMBERS_ROLE.getRoute(policyID)); + }, + }); + } + if (canManageSelectedEmployees) { options.push({ text: translate('workspace.people.removeMembersTitle', {count: selectedEmployees.length}), @@ -551,94 +565,6 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers }); } - if (!isPaidGroupPolicy(policy)) { - return options; - } - - const memberOption = { - text: translate('workspace.people.makeMember', {count: selectedEmployees.length}), - value: CONST.POLICY.MEMBERS_BULK_ACTION_TYPES.MAKE_MEMBER, - icon: icons.User, - shouldSkipFocusRestore: hasSelectedRuleBot, - onSelected: () => changeUserRole(CONST.POLICY.ROLE.USER), - }; - const adminOption = { - text: translate('workspace.people.makeAdmin', {count: selectedEmployees.length}), - value: CONST.POLICY.MEMBERS_BULK_ACTION_TYPES.MAKE_ADMIN, - icon: icons.MakeAdmin, - onSelected: () => changeUserRole(CONST.POLICY.ROLE.ADMIN), - }; - - const auditorOption = { - text: translate('workspace.people.makeAuditor', {count: selectedEmployees.length}), - value: CONST.POLICY.MEMBERS_BULK_ACTION_TYPES.MAKE_AUDITOR, - icon: icons.UserEye, - shouldSkipFocusRestore: hasSelectedRuleBot, - onSelected: () => changeUserRole(CONST.POLICY.ROLE.AUDITOR), - }; - const cardAdminOption = { - text: translate('workspace.people.makeCardAdmin', {count: selectedEmployees.length}), - value: CONST.POLICY.MEMBERS_BULK_ACTION_TYPES.MAKE_CARD_ADMIN, - icon: icons.MakeAdmin, - shouldSkipFocusRestore: hasSelectedRuleBot, - onSelected: () => changeUserRole(CONST.POLICY.ROLE.CARD_ADMIN), - }; - const peopleAdminOption = { - text: translate('workspace.people.makePeopleAdmin', {count: selectedEmployees.length}), - value: CONST.POLICY.MEMBERS_BULK_ACTION_TYPES.MAKE_PEOPLE_ADMIN, - icon: icons.MakeAdmin, - shouldSkipFocusRestore: hasSelectedRuleBot, - onSelected: () => changeUserRole(CONST.POLICY.ROLE.PEOPLE_ADMIN), - }; - const paymentsAdminOption = { - text: translate('workspace.people.makePaymentsAdmin', {count: selectedEmployees.length}), - value: CONST.POLICY.MEMBERS_BULK_ACTION_TYPES.MAKE_PAYMENTS_ADMIN, - icon: icons.MakeAdmin, - shouldSkipFocusRestore: hasSelectedRuleBot, - onSelected: () => changeUserRole(CONST.POLICY.ROLE.PAYMENTS_ADMIN), - }; - - const hasAtLeastOneNonAuditorRole = selectedEmployeesRoles.some((role) => role !== CONST.POLICY.ROLE.AUDITOR); - const hasAtLeastOneNonCardAdminRole = selectedEmployeesRoles.some((role) => role !== CONST.POLICY.ROLE.CARD_ADMIN); - const hasAtLeastOneNonPeopleAdminRole = selectedEmployeesRoles.some((role) => role !== CONST.POLICY.ROLE.PEOPLE_ADMIN); - const hasAtLeastOneNonPaymentsAdminRole = selectedEmployeesRoles.some((role) => role !== CONST.POLICY.ROLE.PAYMENTS_ADMIN); - const hasAtLeastOneNonMemberRole = selectedEmployeesRoles.some((role) => role !== CONST.POLICY.ROLE.USER); - const hasAtLeastOneNonAdminRole = selectedEmployeesRoles.some((role) => role !== CONST.POLICY.ROLE.ADMIN); - const reimburserEmail = getReimburserEmail(policy); - const hasAtLeastOnePayer = !!reimburserEmail && selectedEmployees.includes(reimburserEmail); - - if (hasAtLeastOneNonMemberRole && !hasAtLeastOnePayer && canManageSelectedEmployees && canMemberAssignRole(policy, currentUserLogin ?? '', CONST.POLICY.ROLE.USER)) { - options.push(memberOption); - } - - // Admin is a valid payer role, so the payer may be promoted to Admin (Admin and Payments Admin are the two roles that can pay). - if (hasAtLeastOneNonAdminRole && canAssignElevatedRoles) { - options.push(adminOption); - } - - if ( - hasAtLeastOneNonAuditorRole && - isControlPolicy(policy) && - !hasAtLeastOnePayer && - canManageSelectedEmployees && - canMemberAssignRole(policy, currentUserLogin ?? '', CONST.POLICY.ROLE.AUDITOR) - ) { - options.push(auditorOption); - } - - if (hasAtLeastOneNonCardAdminRole && isControlPolicy(policy) && !hasAtLeastOnePayer && canAssignElevatedRoles) { - options.push(cardAdminOption); - } - - if (hasAtLeastOneNonPeopleAdminRole && isControlPolicy(policy) && !hasAtLeastOnePayer && canAssignElevatedRoles) { - options.push(peopleAdminOption); - } - - // Payments Admin is a valid payer role, so the payer may be changed to Payments Admin (Admin and Payments Admin are the two roles that can pay). - if (hasAtLeastOneNonPaymentsAdminRole && isControlPolicy(policy) && canAssignElevatedRoles) { - options.push(paymentsAdminOption); - } - return options; }; diff --git a/src/pages/workspace/members/WorkspaceMembersRolePage.tsx b/src/pages/workspace/members/WorkspaceMembersRolePage.tsx new file mode 100644 index 000000000000..e44afd51fc7a --- /dev/null +++ b/src/pages/workspace/members/WorkspaceMembersRolePage.tsx @@ -0,0 +1,122 @@ +import ScreenWrapper from '@components/ScreenWrapper'; +import WorkspaceMemberRoleList from '@components/WorkspaceMemberRoleList'; +import type {ListItemType} from '@components/WorkspaceMemberRoleList'; + +import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails'; +import useLocalize from '@hooks/useLocalize'; +import useOnyx from '@hooks/useOnyx'; +import {usePersonalDetailsByLogins} from '@hooks/usePersonalDetailByLogin'; +import useRedirectSubmitWorkspaceFeatureUpgrade from '@hooks/useRedirectSubmitWorkspaceFeatureUpgrade'; +import useRuleBotGuardModal from '@hooks/useRuleBotGuardModal'; + +import {clearMembersSelectedForRoleChange, updateWorkspaceMembersRole} from '@libs/actions/Policy/Member'; +import {isRuleBotEnforcingRules} from '@libs/AgentRulesUtils'; +import Navigation from '@libs/Navigation/Navigation'; +import type {PlatformStackScreenProps} from '@libs/Navigation/PlatformStackNavigation/types'; +import type {SettingsNavigatorParamList} from '@libs/Navigation/types'; +import {canMemberManageMemberWithRole, getMemberAccountIDsForWorkspace, getReimburserEmail, PAYER_ROLES} from '@libs/PolicyUtils'; + +import AccessOrNotFoundWrapper from '@pages/workspace/AccessOrNotFoundWrapper'; +import withPolicyAndFullscreenLoading from '@pages/workspace/withPolicyAndFullscreenLoading'; +import type {WithPolicyAndFullscreenLoadingProps} from '@pages/workspace/withPolicyAndFullscreenLoading'; + +import CONST from '@src/CONST'; +import ONYXKEYS from '@src/ONYXKEYS'; +import ROUTES from '@src/ROUTES'; +import type SCREENS from '@src/SCREENS'; +import isLoadingOnyxValue from '@src/types/utils/isLoadingOnyxValue'; + +import type {ValueOf} from 'type-fest'; + +import React, {useEffect, useState} from 'react'; + +type WorkspaceMembersRolePageProps = Omit & PlatformStackScreenProps; + +function WorkspaceMembersRolePage({policy, route}: WorkspaceMembersRolePageProps) { + const policyID = route.params.policyID; + const {translate} = useLocalize(); + const showRuleBotGuardModal = useRuleBotGuardModal(); + const {login: currentUserLogin = ''} = useCurrentUserPersonalDetails(); + const [selectedLogins, selectedLoginsMetadata] = useOnyx(ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE); + const employeePersonalDetails = usePersonalDetailsByLogins(Object.keys(policy?.employeeList ?? {})); + const memberEmailsToAccountIDs = getMemberAccountIDsForWorkspace(policy?.employeeList, employeePersonalDetails, true); + + const memberLogins = selectedLogins ?? []; + const memberRoles = memberLogins.map((login) => policy?.employeeList?.[login]?.role); + const canManageSelectedMemberRoles = memberRoles.every((role) => canMemberManageMemberWithRole(policy, currentUserLogin, role)); + + // Only offer a role as already picked when the whole selection shares it, so a mixed selection starts with nothing + // selected and the member has to make a deliberate choice. + const sharedRole = memberRoles.every((role) => role === memberRoles.at(0)) ? memberRoles.at(0) : undefined; + const [draftRole, setDraftRole] = useState>(); + const selectedRole = draftRole ?? sharedRole; + + // The Authorized Payer (reimburser) must stay a valid payer, so restrict the whole selection to the roles that can pay once it includes them. + const reimburserEmail = getReimburserEmail(policy); + const hasReimburser = !!reimburserEmail && memberLogins.includes(reimburserEmail); + const allowedRoles = hasReimburser ? [...PAYER_ROLES] : undefined; + + useRedirectSubmitWorkspaceFeatureUpgrade({ + policy, + backTo: ROUTES.WORKSPACE_MEMBERS.getRoute(policyID), + upgradeFeatureAlias: CONST.UPGRADE_FEATURE_INTRO_MAPPING.roles.alias, + }); + + // The selection only lives in memory, so a reload or a deep link landing here directly has nothing to act on. + const isSelectionLoading = isLoadingOnyxValue(selectedLoginsMetadata); + const memberCount = memberLogins.length; + useEffect(() => { + if (isSelectionLoading || memberCount > 0) { + return; + } + Navigation.navigate(ROUTES.WORKSPACE_MEMBERS.getRoute(policyID)); + }, [isSelectionLoading, memberCount, policyID]); + + const saveAndGoBack = () => { + if (!selectedRole) { + return; + } + + if (selectedRole !== CONST.POLICY.ROLE.ADMIN && memberLogins.some((login) => isRuleBotEnforcingRules(memberEmailsToAccountIDs[login], policy))) { + showRuleBotGuardModal('changeRole', policyID); + return; + } + + const loginsToUpdate = memberLogins.filter((login) => policy?.employeeList?.[login]?.role !== selectedRole); + const accountIDsToUpdate = loginsToUpdate.map((login) => memberEmailsToAccountIDs[login]).filter((accountID) => accountID !== undefined); + + updateWorkspaceMembersRole(policy, loginsToUpdate, accountIDsToUpdate, selectedRole); + clearMembersSelectedForRoleChange(); + Navigation.goBack(ROUTES.WORKSPACE_MEMBERS.getRoute(policyID)); + }; + + return ( + 0 && !canManageSelectedMemberRoles} + > + + setDraftRole(value)} + allowedRoles={allowedRoles} + navigateBackTo={ROUTES.WORKSPACE_MEMBERS.getRoute(policyID)} + confirmButtonOptions={{ + showButton: true, + text: translate('common.save'), + onConfirm: saveAndGoBack, + isDisabled: !selectedRole || selectedRole === sharedRole, + }} + /> + + + ); +} + +export default withPolicyAndFullscreenLoading(WorkspaceMembersRolePage); diff --git a/src/setup/index.ts b/src/setup/index.ts index 15843b7d89ad..7b4aa0455084 100644 --- a/src/setup/index.ts +++ b/src/setup/index.ts @@ -96,6 +96,7 @@ export default function () { ONYXKEYS.RAM_ONLY_MERGE_HR_LINK_TOKEN, ONYXKEYS.COLLECTION.RAM_ONLY_ISSUE_NEW_EXPENSIFY_CARD, ONYXKEYS.RAM_ONLY_DOMAIN_MEMBERS_SELECTED_FOR_MOVE, + ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE, ONYXKEYS.RAM_ONLY_HAS_DISMISSED_CONCIERGE_NOTIFICATION_BANNER, ONYXKEYS.RAM_ONLY_CORPAY_PAY_MODAL, ], diff --git a/tests/ui/WorkspaceMembersTest.tsx b/tests/ui/WorkspaceMembersTest.tsx index b492b0b3c51a..d0b8c7655e7d 100644 --- a/tests/ui/WorkspaceMembersTest.tsx +++ b/tests/ui/WorkspaceMembersTest.tsx @@ -12,14 +12,17 @@ import type ResponsiveLayoutResult from '@hooks/useResponsiveLayout/types'; import createPlatformStackNavigator from '@libs/Navigation/PlatformStackNavigation/createPlatformStackNavigator'; -import type {WorkspaceSplitNavigatorParamList} from '@navigation/types'; +import type {SettingsNavigatorParamList, WorkspaceSplitNavigatorParamList} from '@navigation/types'; +import WorkspaceMembersRolePage from '@pages/workspace/members/WorkspaceMembersRolePage'; import WorkspaceMembersPage from '@pages/workspace/WorkspaceMembersPage'; import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; import SCREENS from '@src/SCREENS'; +import type {ValueOf} from 'type-fest'; + import {PortalProvider} from '@gorhom/portal'; import {NavigationContainer} from '@react-navigation/native'; import React from 'react'; @@ -54,6 +57,26 @@ const renderPage = (initialRouteName: typeof SCREENS.WORKSPACE.MEMBERS, initialP ); }; +const RoleStack = createPlatformStackNavigator(); + +const renderRolePage = (policyID: string) => { + return render( + + + + + + + + + , + ); +}; + /** * The wide layout offers the bulk actions in the floating bar over the table, which gives the first few an inline * button and moves the rest behind "More". Opening that menu puts every action on screen at once, whichever half of @@ -94,7 +117,6 @@ describe('WorkspaceMembers', () => { const selfAccountID = 1206; const selfEmail = 'test@example.com'; const ADMIN_OPTION = 'Admin User'; - const AUDITOR_OPTION = 'Auditor User'; const USER_OPTION = 'Member User'; const policy = { ...LHNTestUtils.getFakePolicy(), @@ -147,161 +169,34 @@ describe('WorkspaceMembers', () => { }); describe('Changing roles options', () => { - it('should show Make member/auditor/card admin when admin is selected', async () => { - const {unmount} = renderPage(SCREENS.WORKSPACE.MEMBERS, {policyID: policy.id}); - await waitForBatchedUpdatesWithAct(); - - // Wait for initial render and verify members are visible - await waitFor(() => { - expect(screen.getByText(ADMIN_OPTION)).toBeOnTheScreen(); - }); - - // Select admin option by clicking the checkbox - selectCheckboxByMemberName('Admin'); - - // Open the actions the bar moved behind "More", so every action the selection supports is on screen - await openBulkActions(); - - // Find and verify the "Make member" action - const makeMemberText = TestHelper.translateLocal('workspace.people.makeMember', {count: 1}); - expect(screen.getByText(makeMemberText)).toBeOnTheScreen(); - - // Find and verify the "Make auditor" action - const makeAuditorText = TestHelper.translateLocal('workspace.people.makeAuditor', {count: 1}); - expect(screen.getByText(makeAuditorText)).toBeOnTheScreen(); - - // Find and verify the "Make card admin" action - const makeCardAdminText = TestHelper.translateLocal('workspace.people.makeCardAdmin', {count: 1}); - expect(screen.getByText(makeCardAdminText)).toBeOnTheScreen(); - - // Find and verify the "Make admin" action is not present - const makeAdminText = TestHelper.translateLocal('workspace.people.makeAdmin', {count: 1}); - expect(screen.queryByText(makeAdminText)).not.toBeOnTheScreen(); - - unmount(); - await waitForBatchedUpdatesWithAct(); - }); - - it('should show Make admin/auditor/card admin when member is selected', async () => { - const {unmount} = renderPage(SCREENS.WORKSPACE.MEMBERS, {policyID: policy.id}); - await waitForBatchedUpdatesWithAct(); - - // Wait for initial render and verify members are visible - await waitFor(() => { - expect(screen.getByText(USER_OPTION)).toBeOnTheScreen(); - }); - - // Select member option by clicking the checkbox - selectCheckboxByMemberName('Member'); - - // Open the actions the bar moved behind "More", so every action the selection supports is on screen - await openBulkActions(); - - // Find and verify the "Make admin" action - const makeAdminText = TestHelper.translateLocal('workspace.people.makeAdmin', {count: 1}); - expect(screen.getByText(makeAdminText)).toBeOnTheScreen(); - - // Find and verify the "Make auditor" action - const makeAuditorText = TestHelper.translateLocal('workspace.people.makeAuditor', {count: 1}); - expect(screen.getByText(makeAuditorText)).toBeOnTheScreen(); - - // Find and verify the "Make card admin" action - const makeCardAdminText = TestHelper.translateLocal('workspace.people.makeCardAdmin', {count: 1}); - expect(screen.getByText(makeCardAdminText)).toBeOnTheScreen(); - - // Find and verify the "Make member" action is not present - const makeMemberText = TestHelper.translateLocal('workspace.people.makeMember', {count: 1}); - expect(screen.queryByText(makeMemberText)).not.toBeOnTheScreen(); - - unmount(); - await waitForBatchedUpdatesWithAct(); - }); - - it('should show Make member/admin/card admin when auditor is selected', async () => { - const {unmount} = renderPage(SCREENS.WORKSPACE.MEMBERS, {policyID: policy.id}); - await waitForBatchedUpdatesWithAct(); - - // Wait for initial render and verify members are visible - await waitFor(() => { - expect(screen.getByText(AUDITOR_OPTION)).toBeOnTheScreen(); - }); - - // Select auditor option by clicking the checkbox - selectCheckboxByMemberName('Auditor'); - - // Open the actions the bar moved behind "More", so every action the selection supports is on screen - await openBulkActions(); - - // Find and verify the "Make member" action - const makeMemberText = TestHelper.translateLocal('workspace.people.makeMember', {count: 1}); - expect(screen.getByText(makeMemberText)).toBeOnTheScreen(); - - // Find and verify the "Make admin" action - const makeAdminText = TestHelper.translateLocal('workspace.people.makeAdmin', {count: 1}); - expect(screen.getByText(makeAdminText)).toBeOnTheScreen(); - - // Find and verify the "Make card admin" action - const makeCardAdminText = TestHelper.translateLocal('workspace.people.makeCardAdmin', {count: 1}); - expect(screen.getByText(makeCardAdminText)).toBeOnTheScreen(); - - // Find and verify the "Make auditor" action is not present - const makeAuditorText = TestHelper.translateLocal('workspace.people.makeAuditor', {count: 1}); - expect(screen.queryByText(makeAuditorText)).not.toBeOnTheScreen(); - - unmount(); - await waitForBatchedUpdatesWithAct(); - }); - - it('should show Make member/admin/auditor/card admin when mix is selected', async () => { + it('should offer one Change role action instead of one action per role', async () => { + // Given the members page with an admin selected const {unmount} = renderPage(SCREENS.WORKSPACE.MEMBERS, {policyID: policy.id}); await waitForBatchedUpdatesWithAct(); - // Wait for initial render and verify members are visible - await waitFor(() => { - expect(screen.getByText(AUDITOR_OPTION)).toBeOnTheScreen(); - }); await waitFor(() => { expect(screen.getByText(ADMIN_OPTION)).toBeOnTheScreen(); }); - // Select options by clicking the checkboxes - selectCheckboxByMemberName('Auditor'); selectCheckboxByMemberName('Admin'); - // Open the actions the bar moved behind "More", so every action the selection supports is on screen + // When the actions the bar moved behind "More" are opened await openBulkActions(); - // Find and verify the "Make members" action (plural form for 2 selected items) - const makeMemberText = TestHelper.translateLocal('workspace.people.makeMember', {count: 2}); - expect(screen.getByText(makeMemberText)).toBeOnTheScreen(); - - // Find and verify the "Make admins" action (plural form for 2 selected items) - const makeAdminText = TestHelper.translateLocal('workspace.people.makeAdmin', {count: 2}); - expect(screen.getByText(makeAdminText)).toBeOnTheScreen(); - - // Find and verify the "Make auditors" action (plural form for 2 selected items) - const makeAuditorText = TestHelper.translateLocal('workspace.people.makeAuditor', {count: 2}); - expect(screen.getByText(makeAuditorText)).toBeOnTheScreen(); - - // Find and verify the "Make card admins" action (plural form for 2 selected items) - const makeCardAdminText = TestHelper.translateLocal('workspace.people.makeCardAdmin', {count: 2}); - expect(screen.getByText(makeCardAdminText)).toBeOnTheScreen(); + // Then the bar offers Change role alongside Remove, and none of the per-role actions it replaced + expect(screen.getByText(TestHelper.translateLocal('workspace.people.changeRole'))).toBeOnTheScreen(); + expect(screen.getByText(TestHelper.translateLocal('workspace.people.removeMembersTitle', {count: 1}))).toBeOnTheScreen(); + expect(screen.queryByText(TestHelper.translateLocal('workspace.people.makeMember', {count: 1}))).not.toBeOnTheScreen(); + expect(screen.queryByText(TestHelper.translateLocal('workspace.people.makeAdmin', {count: 1}))).not.toBeOnTheScreen(); unmount(); await waitForBatchedUpdatesWithAct(); }); - it('should only show member and auditor role actions for People Admin', async () => { - const peopleAdminPolicy = { - ...policy, - role: CONST.POLICY.ROLE.PEOPLE_ADMIN, - employeeList: { - ...policy.employeeList, - [selfEmail]: {email: selfEmail, role: CONST.POLICY.ROLE.PEOPLE_ADMIN}, - }, - }; + it('should not offer Change role on a workspace without paid roles', async () => { + // Given a Submit workspace, which has no roles to move a member between await act(async () => { - await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, peopleAdminPolicy); + await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, {type: CONST.POLICY.TYPE.SUBMIT}); }); const {unmount} = renderPage(SCREENS.WORKSPACE.MEMBERS, {policyID: policy.id}); @@ -311,148 +206,97 @@ describe('WorkspaceMembers', () => { expect(screen.getByText(USER_OPTION)).toBeOnTheScreen(); }); + // When a member is selected and the actions are opened selectCheckboxByMemberName('Member'); await openBulkActions(); - const removeText = TestHelper.translateLocal('workspace.people.removeMembersTitle', {count: 1}); - expect(screen.getByText(removeText)).toBeOnTheScreen(); - - const makeAuditorText = TestHelper.translateLocal('workspace.people.makeAuditor', {count: 1}); - expect(screen.getByText(makeAuditorText)).toBeOnTheScreen(); - - const makeAdminText = TestHelper.translateLocal('workspace.people.makeAdmin', {count: 1}); - expect(screen.queryByText(makeAdminText)).not.toBeOnTheScreen(); - - const makeCardAdminText = TestHelper.translateLocal('workspace.people.makeCardAdmin', {count: 1}); - expect(screen.queryByText(makeCardAdminText)).not.toBeOnTheScreen(); - - const makePeopleAdminText = TestHelper.translateLocal('workspace.people.makePeopleAdmin', {count: 1}); - expect(screen.queryByText(makePeopleAdminText)).not.toBeOnTheScreen(); + // Then only Remove is offered + expect(screen.getByText(TestHelper.translateLocal('workspace.people.removeMembersTitle', {count: 1}))).toBeOnTheScreen(); + expect(screen.queryByText(TestHelper.translateLocal('workspace.people.changeRole'))).not.toBeOnTheScreen(); unmount(); await waitForBatchedUpdatesWithAct(); }); + }); - it('should let People Admin make auditors members', async () => { - const peopleAdminPolicy = { - ...policy, - role: CONST.POLICY.ROLE.PEOPLE_ADMIN, - employeeList: { - ...policy.employeeList, - [selfEmail]: {email: selfEmail, role: CONST.POLICY.ROLE.PEOPLE_ADMIN}, - }, - }; + describe('Role selection screen', () => { + const roleName = (role: ValueOf) => TestHelper.translateLocal('workspace.common.roleName', role); + const memberRoleName = () => TestHelper.translateLocal('common.member'); + + const selectForRoleChange = async (logins: string[]) => { await act(async () => { - await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, peopleAdminPolicy); + await Onyx.set(ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE, logins); }); + }; - const {unmount} = renderPage(SCREENS.WORKSPACE.MEMBERS, {policyID: policy.id}); + it('should offer every role the current member may assign', async () => { + // Given an admin selected for a role change on a Control workspace + await selectForRoleChange([adminEmail]); + + const {unmount} = renderRolePage(policy.id); await waitForBatchedUpdatesWithAct(); + // Then the screen lists all of the Control roles, with the selection's shared role already picked await waitFor(() => { - expect(screen.getByText(AUDITOR_OPTION)).toBeOnTheScreen(); + expect(screen.getByText(roleName(CONST.POLICY.ROLE.ADMIN))).toBeOnTheScreen(); }); - - selectCheckboxByMemberName('Auditor'); - await openBulkActions(); - - const makeMemberText = TestHelper.translateLocal('workspace.people.makeMember', {count: 1}); - expect(screen.getByText(makeMemberText)).toBeOnTheScreen(); - - const makeAdminText = TestHelper.translateLocal('workspace.people.makeAdmin', {count: 1}); - expect(screen.queryByText(makeAdminText)).not.toBeOnTheScreen(); + expect(screen.getByText(roleName(CONST.POLICY.ROLE.AUDITOR))).toBeOnTheScreen(); + expect(screen.getByText(roleName(CONST.POLICY.ROLE.CARD_ADMIN))).toBeOnTheScreen(); + expect(screen.getByText(roleName(CONST.POLICY.ROLE.PEOPLE_ADMIN))).toBeOnTheScreen(); + expect(screen.getByText(roleName(CONST.POLICY.ROLE.PAYMENTS_ADMIN))).toBeOnTheScreen(); + expect(screen.getByText(memberRoleName())).toBeOnTheScreen(); unmount(); await waitForBatchedUpdatesWithAct(); }); - it('should hide demotions but offer Make payments admin when the selected member is the Authorized Payer resolved via policy.reimburser', async () => { - // Given a workspace whose Authorized Payer is an admin configured through policy.reimburser - // (the canonical resolution) rather than achAccount.reimburser. Demotions to roles that cannot - // pay must stay hidden, but changing to Payments Admin (the other valid payer role) must be offered. + it('should only offer member and auditor to a People Admin', async () => { + // Given a People Admin, who may assign no role above auditor await act(async () => { await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, { - reimbursementChoice: CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES, - reimburser: adminEmail, + role: CONST.POLICY.ROLE.PEOPLE_ADMIN, + employeeList: {[selfEmail]: {email: selfEmail, role: CONST.POLICY.ROLE.PEOPLE_ADMIN}}, }); }); + await selectForRoleChange([userEmail]); - const {unmount} = renderPage(SCREENS.WORKSPACE.MEMBERS, {policyID: policy.id}); + const {unmount} = renderRolePage(policy.id); await waitForBatchedUpdatesWithAct(); + // Then only the two roles they may assign are listed await waitFor(() => { - expect(screen.getByText(ADMIN_OPTION)).toBeOnTheScreen(); - }); - - // When that payer is bulk-selected and the actions dropdown is opened - selectCheckboxByMemberName('Admin'); - await openBulkActions(); - - // Then the Remove option is still available - const removeText = TestHelper.translateLocal('workspace.people.removeMembersTitle', {count: 1}); - await waitFor(() => { - expect(screen.getByText(removeText)).toBeOnTheScreen(); + expect(screen.getByText(roleName(CONST.POLICY.ROLE.AUDITOR))).toBeOnTheScreen(); }); - - // ...the demotions that would strip the payer of pay capability are hidden - const makeMemberText = TestHelper.translateLocal('workspace.people.makeMember', {count: 1}); - expect(screen.queryByText(makeMemberText)).not.toBeOnTheScreen(); - - const makeAuditorText = TestHelper.translateLocal('workspace.people.makeAuditor', {count: 1}); - expect(screen.queryByText(makeAuditorText)).not.toBeOnTheScreen(); - - const makeCardAdminText = TestHelper.translateLocal('workspace.people.makeCardAdmin', {count: 1}); - expect(screen.queryByText(makeCardAdminText)).not.toBeOnTheScreen(); - - // ...but Make payments admin IS offered, since Payments Admin is a valid payer role - const makePaymentsAdminText = TestHelper.translateLocal('workspace.people.makePaymentsAdmin', {count: 1}); - expect(screen.getByText(makePaymentsAdminText)).toBeOnTheScreen(); + expect(screen.getByText(memberRoleName())).toBeOnTheScreen(); + expect(screen.queryByText(roleName(CONST.POLICY.ROLE.ADMIN))).not.toBeOnTheScreen(); + expect(screen.queryByText(roleName(CONST.POLICY.ROLE.CARD_ADMIN))).not.toBeOnTheScreen(); + expect(screen.queryByText(roleName(CONST.POLICY.ROLE.PEOPLE_ADMIN))).not.toBeOnTheScreen(); unmount(); await waitForBatchedUpdatesWithAct(); }); - it('should offer Make workspace admin but hide demotions when the selected member is a Payments Admin who is the Authorized Payer', async () => { - // Given a Payments Admin who is also the Authorized Payer. Admin and Payments Admin are both valid - // payer roles, so promoting this payer to Admin keeps them a valid payer and must be offered. - // Every demotion to a role that cannot pay (Member, Auditor, Card Admin) stays gated on the payer. + it('should only offer the roles that can pay when the selection holds the Authorized Payer', async () => { + // Given a selection holding the Authorized Payer, who must stay a valid payer await act(async () => { await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, { reimbursementChoice: CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES, - reimburser: userEmail, - employeeList: { - [userEmail]: {email: userEmail, role: CONST.POLICY.ROLE.PAYMENTS_ADMIN}, - }, + reimburser: adminEmail, }); }); + await selectForRoleChange([adminEmail]); - const {unmount} = renderPage(SCREENS.WORKSPACE.MEMBERS, {policyID: policy.id}); + const {unmount} = renderRolePage(policy.id); await waitForBatchedUpdatesWithAct(); + // Then only Admin and Payments Admin are offered, and every role that cannot pay is withheld await waitFor(() => { - expect(screen.getByText(USER_OPTION)).toBeOnTheScreen(); + expect(screen.getByText(roleName(CONST.POLICY.ROLE.PAYMENTS_ADMIN))).toBeOnTheScreen(); }); - - // When that payer is bulk-selected and the actions dropdown is opened - selectCheckboxByMemberName('Member'); - await openBulkActions(); - - // Then the Remove option is still available - const removeText = TestHelper.translateLocal('workspace.people.removeMembersTitle', {count: 1}); - await waitFor(() => { - expect(screen.getByText(removeText)).toBeOnTheScreen(); - }); - - // ...and "Make workspace admin" IS offered, since Admin is a valid payer role - const makeAdminText = TestHelper.translateLocal('workspace.people.makeAdmin', {count: 1}); - expect(screen.getByText(makeAdminText)).toBeOnTheScreen(); - - // ...but the demotions that would strip the payer of pay capability stay hidden - const makeMemberText = TestHelper.translateLocal('workspace.people.makeMember', {count: 1}); - expect(screen.queryByText(makeMemberText)).not.toBeOnTheScreen(); - - const makeAuditorText = TestHelper.translateLocal('workspace.people.makeAuditor', {count: 1}); - expect(screen.queryByText(makeAuditorText)).not.toBeOnTheScreen(); + expect(screen.getByText(roleName(CONST.POLICY.ROLE.ADMIN))).toBeOnTheScreen(); + expect(screen.queryByText(memberRoleName())).not.toBeOnTheScreen(); + expect(screen.queryByText(roleName(CONST.POLICY.ROLE.AUDITOR))).not.toBeOnTheScreen(); + expect(screen.queryByText(roleName(CONST.POLICY.ROLE.CARD_ADMIN))).not.toBeOnTheScreen(); unmount(); await waitForBatchedUpdatesWithAct(); @@ -545,22 +389,35 @@ describe('WorkspaceMembers', () => { }); it('should show the unable-to-change-role modal when demoting a RuleBot enforcing agent rules', async () => { + // Given the RuleBot selected for a role change await makeAdminTheRuleBot(); + await act(async () => { + await Onyx.set(ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE, [adminEmail]); + }); - const {unmount} = renderPage(SCREENS.WORKSPACE.MEMBERS, {policyID: policy.id}); + const {unmount} = renderRolePage(policy.id); await waitForBatchedUpdatesWithAct(); - await selectAdminAndOpenBulkActions(); + // When a role below admin is picked and saved + const memberRow = await screen.findByText(TestHelper.translateLocal('common.member')); + fireEvent.press(memberRow, { + nativeEvent: {}, + type: 'press', + target: memberRow, + currentTarget: memberRow, + }); + await waitForBatchedUpdatesWithAct(); - const makeMemberMenuItem = screen.getByText(TestHelper.translateLocal('workspace.people.makeMember', {count: 1})); - fireEvent.press(makeMemberMenuItem, { + const saveButton = screen.getByText(TestHelper.translateLocal('common.save')); + fireEvent.press(saveButton, { nativeEvent: {}, type: 'press', - target: makeMemberMenuItem, - currentTarget: makeMemberMenuItem, + target: saveButton, + currentTarget: saveButton, }); await waitForBatchedUpdatesWithAct(); + // Then the role change is refused with the agent rules modal await waitFor(() => { expect(screen.getByText(TestHelper.translateLocal('workspace.rules.agentRules.unableToChangeRoleTitle'))).toBeOnTheScreen(); }); From 695f332284e4aef080c47771687d243706e150a0 Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Thu, 1 Oct 2026 15:32:25 +0530 Subject: [PATCH 11/23] Keep Save pressable on the bulk role selector and ask for a role instead Signed-off-by: krishna2323 --- src/components/WorkspaceMemberRoleList.tsx | 16 +++++++- .../members/WorkspaceMembersRolePage.tsx | 24 ++++++++---- tests/ui/WorkspaceMembersTest.tsx | 38 +++++++++++++++++++ 3 files changed, 69 insertions(+), 9 deletions(-) diff --git a/src/components/WorkspaceMemberRoleList.tsx b/src/components/WorkspaceMemberRoleList.tsx index 7f42a9c1ee2f..040db599bb1a 100644 --- a/src/components/WorkspaceMemberRoleList.tsx +++ b/src/components/WorkspaceMemberRoleList.tsx @@ -17,6 +17,7 @@ import {View} from 'react-native'; import type {ConfirmButtonOptions, ListItem} from './SelectionList/types'; +import FormHelpMessage from './FormHelpMessage'; import HeaderWithBackButton from './HeaderWithBackButton'; import SelectionList from './SelectionList'; import SingleSelectListItem from './SelectionList/ListItem/SingleSelectListItem'; @@ -40,6 +41,9 @@ type WorkspaceMemberRoleListProps = { /** When provided, the list confirms the pick with a button instead of applying it as soon as a row is pressed */ confirmButtonOptions?: ConfirmButtonOptions; + + /** Shown above the confirm button, for a confirmation pressed without a role picked */ + errorMessage?: string; }; function WorkspaceMemberRoleList({ @@ -50,6 +54,7 @@ function WorkspaceMemberRoleList({ onSelectRole = () => {}, allowedRoles = undefined, confirmButtonOptions = undefined, + errorMessage = '', }: WorkspaceMemberRoleListProps) { const {translate} = useLocalize(); const styles = useThemeStyles(); @@ -119,7 +124,16 @@ function WorkspaceMemberRoleList({ shouldSingleExecuteRowSelect initiallyFocusedItemKey={availableRoleItems.find((item) => item.isSelected)?.keyForList} addBottomSafeAreaPadding - /> + > + {!!errorMessage && ( + + + + )} + )} diff --git a/src/pages/workspace/members/WorkspaceMembersRolePage.tsx b/src/pages/workspace/members/WorkspaceMembersRolePage.tsx index e44afd51fc7a..45ea3bee2bb4 100644 --- a/src/pages/workspace/members/WorkspaceMembersRolePage.tsx +++ b/src/pages/workspace/members/WorkspaceMembersRolePage.tsx @@ -49,6 +49,7 @@ function WorkspaceMembersRolePage({policy, route}: WorkspaceMembersRolePageProps // selected and the member has to make a deliberate choice. const sharedRole = memberRoles.every((role) => role === memberRoles.at(0)) ? memberRoles.at(0) : undefined; const [draftRole, setDraftRole] = useState>(); + const [hasError, setHasError] = useState(false); const selectedRole = draftRole ?? sharedRole; // The Authorized Payer (reimburser) must stay a valid payer, so restrict the whole selection to the roles that can pay once it includes them. @@ -74,18 +75,22 @@ function WorkspaceMembersRolePage({policy, route}: WorkspaceMembersRolePageProps const saveAndGoBack = () => { if (!selectedRole) { - return; - } - - if (selectedRole !== CONST.POLICY.ROLE.ADMIN && memberLogins.some((login) => isRuleBotEnforcingRules(memberEmailsToAccountIDs[login], policy))) { - showRuleBotGuardModal('changeRole', policyID); + setHasError(true); return; } const loginsToUpdate = memberLogins.filter((login) => policy?.employeeList?.[login]?.role !== selectedRole); const accountIDsToUpdate = loginsToUpdate.map((login) => memberEmailsToAccountIDs[login]).filter((accountID) => accountID !== undefined); - updateWorkspaceMembersRole(policy, loginsToUpdate, accountIDsToUpdate, selectedRole); + if (loginsToUpdate.length > 0) { + if (selectedRole !== CONST.POLICY.ROLE.ADMIN && loginsToUpdate.some((login) => isRuleBotEnforcingRules(memberEmailsToAccountIDs[login], policy))) { + showRuleBotGuardModal('changeRole', policyID); + return; + } + + updateWorkspaceMembersRole(policy, loginsToUpdate, accountIDsToUpdate, selectedRole); + } + clearMembersSelectedForRoleChange(); Navigation.goBack(ROUTES.WORKSPACE_MEMBERS.getRoute(policyID)); }; @@ -104,15 +109,18 @@ function WorkspaceMembersRolePage({policy, route}: WorkspaceMembersRolePageProps setDraftRole(value)} + onSelectRole={({value}: ListItemType) => { + setDraftRole(value); + setHasError(false); + }} allowedRoles={allowedRoles} navigateBackTo={ROUTES.WORKSPACE_MEMBERS.getRoute(policyID)} confirmButtonOptions={{ showButton: true, text: translate('common.save'), onConfirm: saveAndGoBack, - isDisabled: !selectedRole || selectedRole === sharedRole, }} + errorMessage={hasError ? translate('common.error.pleaseSelectOne') : ''} /> diff --git a/tests/ui/WorkspaceMembersTest.tsx b/tests/ui/WorkspaceMembersTest.tsx index d0b8c7655e7d..719834c78d56 100644 --- a/tests/ui/WorkspaceMembersTest.tsx +++ b/tests/ui/WorkspaceMembersTest.tsx @@ -276,6 +276,44 @@ describe('WorkspaceMembers', () => { await waitForBatchedUpdatesWithAct(); }); + it('should ask for a role rather than disabling Save when the selection has no role in common', async () => { + // Given a mixed selection, which leaves the screen with nothing picked + await selectForRoleChange([adminEmail, userEmail]); + + const {unmount} = renderRolePage(policy.id); + await waitForBatchedUpdatesWithAct(); + + // When Save is pressed without picking a role + const saveButton = await screen.findByText(TestHelper.translateLocal('common.save')); + fireEvent.press(saveButton, { + nativeEvent: {}, + type: 'press', + target: saveButton, + currentTarget: saveButton, + }); + await waitForBatchedUpdatesWithAct(); + + // Then the screen asks for one instead of leaving the button dead + await waitFor(() => { + expect(screen.getByText(TestHelper.translateLocal('common.error.pleaseSelectOne'))).toBeOnTheScreen(); + }); + + // When a role is then picked, the message clears + const auditorRow = screen.getByText(roleName(CONST.POLICY.ROLE.AUDITOR)); + fireEvent.press(auditorRow, { + nativeEvent: {}, + type: 'press', + target: auditorRow, + currentTarget: auditorRow, + }); + await waitForBatchedUpdatesWithAct(); + + expect(screen.queryByText(TestHelper.translateLocal('common.error.pleaseSelectOne'))).not.toBeOnTheScreen(); + + unmount(); + await waitForBatchedUpdatesWithAct(); + }); + it('should only offer the roles that can pay when the selection holds the Authorized Payer', async () => { // Given a selection holding the Authorized Payer, who must stay a valid payer await act(async () => { From 7ad0d184662f9742273ecfb1fc5d705c7be3aed4 Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Mon, 5 Oct 2026 18:19:37 +0530 Subject: [PATCH 12/23] Use the user pencil icon for the bulk Change role action Signed-off-by: krishna2323 --- assets/images/user-pencil.svg | 1 + src/components/Icon/chunks/expensify-icons.chunk.ts | 2 ++ src/pages/workspace/WorkspaceMembersPage.tsx | 4 ++-- 3 files changed, 5 insertions(+), 2 deletions(-) create mode 100644 assets/images/user-pencil.svg diff --git a/assets/images/user-pencil.svg b/assets/images/user-pencil.svg new file mode 100644 index 000000000000..2ac194fb1129 --- /dev/null +++ b/assets/images/user-pencil.svg @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/src/components/Icon/chunks/expensify-icons.chunk.ts b/src/components/Icon/chunks/expensify-icons.chunk.ts index dbb862ca5000..0330c2252d10 100644 --- a/src/components/Icon/chunks/expensify-icons.chunk.ts +++ b/src/components/Icon/chunks/expensify-icons.chunk.ts @@ -280,6 +280,7 @@ import UserCheck from '@assets/images/user-check.svg'; import UserEye from '@assets/images/user-eye.svg'; import UserLock from '@assets/images/user-lock.svg'; import UserMinus from '@assets/images/user-minus.svg'; +import UserPencil from '@assets/images/user-pencil.svg'; import UserPlus from '@assets/images/user-plus.svg'; import UserSearch from '@assets/images/user-search.svg'; import UserShield from '@assets/images/user-shield.svg'; @@ -576,6 +577,7 @@ const Expensicons = { UserEye, UserSearch, CaretUpDown, + UserPencil, UserPlus, Feed, Table, diff --git a/src/pages/workspace/WorkspaceMembersPage.tsx b/src/pages/workspace/WorkspaceMembersPage.tsx index 2dac8b62bb42..e47a3690d67c 100644 --- a/src/pages/workspace/WorkspaceMembersPage.tsx +++ b/src/pages/workspace/WorkspaceMembersPage.tsx @@ -113,7 +113,7 @@ function invertObject(object: Record): Record { function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembersPageProps) { useWorkspaceDocumentTitle(policy?.name, 'common.members'); const tableRef = useRef>(null); - const icons = useMemoizedLazyExpensifyIcons(['Download', 'FallbackAvatar', 'MakeAdmin', 'Plus', 'RemoveMembers', 'Sync', 'Table']); + const icons = useMemoizedLazyExpensifyIcons(['Download', 'FallbackAvatar', 'Plus', 'RemoveMembers', 'Sync', 'Table', 'UserPencil']); const employeePersonalDetails = usePersonalDetailsByLogins(Object.keys(policy?.employeeList ?? {})); const policyMemberEmailsToAccountIDs = useMemo( () => getMemberAccountIDsForWorkspace(policy?.employeeList, employeePersonalDetails, true), @@ -549,7 +549,7 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers options.push({ text: translate('workspace.people.changeRole'), value: CONST.POLICY.MEMBERS_BULK_ACTION_TYPES.CHANGE_ROLE, - icon: icons.MakeAdmin, + icon: icons.UserPencil, shouldSkipFocusRestore: true, onSelected: () => { setMembersSelectedForRoleChange(selectedEmployees); From b847c52a15e34608d7d8f229d96e02a63c57b707 Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Mon, 5 Oct 2026 18:23:57 +0530 Subject: [PATCH 13/23] compress svg Signed-off-by: krishna2323 --- assets/images/user-pencil.svg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assets/images/user-pencil.svg b/assets/images/user-pencil.svg index 2ac194fb1129..7cc7518c5fe0 100644 --- a/assets/images/user-pencil.svg +++ b/assets/images/user-pencil.svg @@ -1 +1 @@ - \ No newline at end of file + \ No newline at end of file From 869c1118cb972557f45d5f0ec385eef39dd1356b Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Tue, 6 Oct 2026 02:52:22 +0530 Subject: [PATCH 14/23] update translations. Signed-off-by: krishna2323 --- src/languages/de.ts | 2 ++ src/languages/el.ts | 2 ++ src/languages/es.ts | 1 + src/languages/fr.ts | 2 ++ src/languages/it.ts | 2 ++ src/languages/ja.ts | 2 ++ src/languages/nl.ts | 2 ++ src/languages/pl.ts | 2 ++ src/languages/pt-BR.ts | 2 ++ src/languages/zh-hans.ts | 2 ++ 10 files changed, 19 insertions(+) diff --git a/src/languages/de.ts b/src/languages/de.ts index fd8e9d18cfa6..4691ad6a22bb 100644 --- a/src/languages/de.ts +++ b/src/languages/de.ts @@ -7164,6 +7164,7 @@ Der Control-Tarif beginnt bei 9 $ pro aktivem Mitglied und Monat.`, 'Das Mitglied arbeitet von zu Hause aus oder reist zwischen verschiedenen Standorten ohne festes Büro, daher gelten die Pendelregelungen nicht.', futureOnlyNote: 'Änderungen gelten nur für zukünftige Kilometerberechnungen. Bestehende Kilometerabrechnungen werden nicht neu berechnet.', }, + changeRole: 'Rolle ändern', }, card: { getStartedIssuing: 'Beginne, indem du deine erste virtuelle oder physische Karte ausstellst.', @@ -11720,5 +11721,6 @@ Hier ist ein *Testbeleg*, um dir zu zeigen, wie es funktioniert:`, confirmTitle: 'Verlängerung bestätigen', renew: 'Erneuern', }, + bulkActionBar: {label: 'Sammelaktionen', clearSelection: 'Auswahl aufheben', loadingSelection: 'Auswahl wird geladen'}, }; export default translations; diff --git a/src/languages/el.ts b/src/languages/el.ts index d7d7a7548e35..45aafadee650 100644 --- a/src/languages/el.ts +++ b/src/languages/el.ts @@ -7294,6 +7294,7 @@ _Για πιο αναλυτικές οδηγίες, [επισκεφθείτε τ optionNoRegularWorkspaceHelp: 'Το μέλος εργάζεται από το σπίτι ή ταξιδεύει μεταξύ τοποθεσιών χωρίς τακτικό γραφείο, επομένως οι κανόνες μετακίνησης δεν ισχύουν.', futureOnlyNote: 'Οι αλλαγές ισχύουν μόνο για μελλοντικούς υπολογισμούς χιλιομέτρων. Οι υπάρχουσες δαπάνες χιλιομέτρων δεν επανυπολογίζονται.', }, + changeRole: 'Αλλαγή ρόλου', }, card: { getStartedIssuing: 'Ξεκινήστε εκδίδοντας την πρώτη σας εικονική ή φυσική κάρτα.', @@ -11960,5 +11961,6 @@ ${reportName}`, confirmTitle: 'Επιβεβαίωση ανανέωσης', renew: 'Ανανέωση', }, + bulkActionBar: {label: 'Μαζικές ενέργειες', clearSelection: 'Καθαρισμός επιλογής', loadingSelection: 'Φόρτωση επιλογής'}, }; export default translations; diff --git a/src/languages/es.ts b/src/languages/es.ts index d2a1698f52a4..525544557aba 100644 --- a/src/languages/es.ts +++ b/src/languages/es.ts @@ -11988,5 +11988,6 @@ ${reportName}`, confirmTitle: 'Confirmar renovación', renew: 'Renovar', }, + bulkActionBar: {label: 'Acciones en bloque', clearSelection: 'Borrar selección', loadingSelection: 'Cargando selección'}, }; export default translations; diff --git a/src/languages/fr.ts b/src/languages/fr.ts index ef0e1e78c77e..a45f671756da 100644 --- a/src/languages/fr.ts +++ b/src/languages/fr.ts @@ -7181,6 +7181,7 @@ Le forfait Control commence à 9 $ par Membre actif et par mois.`, 'Le membre travaille à domicile ou se déplace entre plusieurs sites sans bureau fixe, donc les règles relatives aux trajets domicile-travail ne s’appliquent pas.', futureOnlyNote: 'Les modifications s’appliquent uniquement aux calculs de kilométrage futurs. Les dépenses de kilométrage existantes ne sont pas recalculées.', }, + changeRole: 'Modifier le rôle', }, card: { getStartedIssuing: 'Commencez par émettre votre première carte virtuelle ou physique.', @@ -11746,5 +11747,6 @@ Voici un *reçu test* pour vous montrer comment ça fonctionne :`, confirmTitle: 'Confirmer le renouvellement', renew: 'Renouveler', }, + bulkActionBar: {label: 'Actions groupées', clearSelection: 'Effacer la sélection', loadingSelection: 'Chargement de la sélection'}, }; export default translations; diff --git a/src/languages/it.ts b/src/languages/it.ts index 200743b63e56..5d126d3af95a 100644 --- a/src/languages/it.ts +++ b/src/languages/it.ts @@ -7122,6 +7122,7 @@ Il piano Control parte da 9 $ al mese per ogni membro attivo.`, 'Il membro lavora da casa o si sposta tra diverse sedi senza un ufficio fisso, quindi le regole relative al tragitto casa-lavoro non si applicano.', futureOnlyNote: 'Le modifiche si applicano solo ai calcoli chilometrici futuri. Le spese chilometriche esistenti non vengono ricalcolate.', }, + changeRole: 'Cambia ruolo', }, card: { getStartedIssuing: 'Inizia emettendo la tua prima carta virtuale o fisica.', @@ -11670,5 +11671,6 @@ Ecco una *ricevuta di prova* per mostrarti come funziona:`, confirmTitle: 'Conferma rinnovo', renew: 'Rinnova', }, + bulkActionBar: {label: 'Azioni collettive', clearSelection: 'Cancella selezione', loadingSelection: 'Caricamento selezione'}, }; export default translations; diff --git a/src/languages/ja.ts b/src/languages/ja.ts index 5686951a9547..b2bb80f72f16 100644 --- a/src/languages/ja.ts +++ b/src/languages/ja.ts @@ -7034,6 +7034,7 @@ Control プランは、アクティブメンバー1人あたり月額 $9 から optionNoRegularWorkspaceHelp: 'メンバーは在宅勤務、または決まったオフィスがなく拠点間を移動しているため、通勤規程は適用されません。', futureOnlyNote: '変更は今後の走行距離計算にのみ適用されます。既存の走行距離経費は再計算されません。', }, + changeRole: 'ロールを変更', }, card: { getStartedIssuing: 'まずは最初のバーチャルカードまたは物理カードを発行しましょう。', @@ -11501,5 +11502,6 @@ ${reportName}`, confirmTitle: '更新を確認', renew: '更新する', }, + bulkActionBar: {label: '一括操作', clearSelection: '選択をクリア', loadingSelection: '選択内容を読み込み中'}, }; export default translations; diff --git a/src/languages/nl.ts b/src/languages/nl.ts index b6b1bdd42149..b18803752314 100644 --- a/src/languages/nl.ts +++ b/src/languages/nl.ts @@ -7106,6 +7106,7 @@ Het Control-abonnement begint bij $9 per actieve deelnemer per maand.`, optionNoRegularWorkspaceHelp: 'Lid werkt thuis of reist tussen locaties zonder vaste kantoorlocatie, waardoor woon-werkregels niet van toepassing zijn.', futureOnlyNote: 'Wijzigingen zijn alleen van toepassing op toekomstige kilometerberekeningen. Bestaande kilometerdeclaraties worden niet opnieuw berekend.', }, + changeRole: 'Rol wijzigen', }, card: { getStartedIssuing: 'Begin met het uitgeven van je eerste virtuele of fysieke kaart.', @@ -11634,5 +11635,6 @@ Hier is een *proefbon* om je te laten zien hoe het werkt:`, confirmTitle: 'Verlenging bevestigen', renew: 'Verleng', }, + bulkActionBar: {label: 'Bulkacties', clearSelection: 'Selectie wissen', loadingSelection: 'Selectie laden'}, }; export default translations; diff --git a/src/languages/pl.ts b/src/languages/pl.ts index d0218ffe04fe..e97b6790e4e3 100644 --- a/src/languages/pl.ts +++ b/src/languages/pl.ts @@ -7131,6 +7131,7 @@ Plan Control zaczyna się od 9 USD za aktywnego członka miesięcznie.`, optionNoRegularWorkspaceHelp: 'Członek pracuje z domu lub podróżuje między lokalizacjami bez stałego biura, więc zasady dotyczące dojazdów do pracy nie mają zastosowania.', futureOnlyNote: 'Zmiany mają zastosowanie tylko do przyszłych obliczeń kilometrówki. Istniejące wydatki za przejazdy nie są przeliczane ponownie.', }, + changeRole: 'Zmień rolę', }, card: { getStartedIssuing: 'Zacznij od wydania swojej pierwszej wirtualnej lub fizycznej karty.', @@ -11649,5 +11650,6 @@ Oto *paragon testowy*, żeby pokazać Ci, jak to działa:`, confirmTitle: 'Potwierdź odnowienie', renew: 'Odnów', }, + bulkActionBar: {label: 'Akcje zbiorcze', clearSelection: 'Wyczyść wybór', loadingSelection: 'Wczytywanie zaznaczenia'}, }; export default translations; diff --git a/src/languages/pt-BR.ts b/src/languages/pt-BR.ts index c3a3dd80685c..8501ab8825ec 100644 --- a/src/languages/pt-BR.ts +++ b/src/languages/pt-BR.ts @@ -7108,6 +7108,7 @@ O plano Control começa em US$ 9 por membro ativo por mês.`, optionNoRegularWorkspaceHelp: 'O membro trabalha em casa ou viaja entre locais sem um escritório fixo, portanto as regras de deslocamento não se aplicam.', futureOnlyNote: 'As alterações se aplicam apenas aos cálculos de quilometragem futuros. As despesas de quilometragem existentes não são recalculadas.', }, + changeRole: 'Alterar função', }, card: { getStartedIssuing: 'Comece emitindo seu primeiro cartão virtual ou físico.', @@ -11626,5 +11627,6 @@ Aqui está um *comprovante de teste* para mostrar como funciona:`, confirmTitle: 'Confirmar renovação', renew: 'Renovar', }, + bulkActionBar: {label: 'Ações em massa', clearSelection: 'Limpar seleção', loadingSelection: 'Carregando seleção'}, }; export default translations; diff --git a/src/languages/zh-hans.ts b/src/languages/zh-hans.ts index 71de802346df..b48ee69608d4 100644 --- a/src/languages/zh-hans.ts +++ b/src/languages/zh-hans.ts @@ -6862,6 +6862,7 @@ _如需更详细的说明，请[访问我们的帮助网站](${CONST.NETSUITE_IM optionNoRegularWorkspaceHelp: '成员在家办公或在各地点之间出差，没有固定办公地点，因此通勤规则不适用。', futureOnlyNote: '更改仅适用于未来的里程计算，现有的里程报销将不会重新计算。', }, + changeRole: '更改角色', }, card: { getStartedIssuing: '从发放您的第一张虚拟卡或实体卡开始使用。', @@ -11210,5 +11211,6 @@ ${reportName}`, confirmTitle: '确认续订', renew: '续订', }, + bulkActionBar: {label: '批量操作', clearSelection: '清除选择', loadingSelection: '正在加载所选内容'}, }; export default translations; From 16e936846c268a1b21c41cab0ea3cfded8350104 Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Thu, 8 Oct 2026 18:37:49 +0530 Subject: [PATCH 15/23] Register the members role screen with navigation Signed-off-by: krishna2323 --- src/CONST/runtimeConfigured.ts | 1 + src/CONST/runtimeDefaults.ts | 1 + src/libs/Navigation/linkingConfig/RELATIONS/WORKSPACE_TO_RHP.ts | 1 + 3 files changed, 3 insertions(+) diff --git a/src/CONST/runtimeConfigured.ts b/src/CONST/runtimeConfigured.ts index b392e0db9024..f16cedfd8ba1 100644 --- a/src/CONST/runtimeConfigured.ts +++ b/src/CONST/runtimeConfigured.ts @@ -76,6 +76,7 @@ const CONST_RUNTIME: ConstRuntime = { SCREENS.AI_FEATURES_PROMO_MODAL.DYNAMIC_ROOT, SCREENS.MONEY_REQUEST.DYNAMIC_STEP_SCAN, SCREENS.DOMAIN.MEMBERS_MOVE_TO_GROUP, + SCREENS.WORKSPACE.MEMBERS_ROLE, SCREENS.PRE_MOUNT_BUFFER, ...Object.values(SCREENS.MULTIFACTOR_AUTHENTICATION), ], diff --git a/src/CONST/runtimeDefaults.ts b/src/CONST/runtimeDefaults.ts index 3385d9db55b5..c124bcdad185 100644 --- a/src/CONST/runtimeDefaults.ts +++ b/src/CONST/runtimeDefaults.ts @@ -99,6 +99,7 @@ const CONST_RUNTIME_DEFAULTS: ConstRuntime = { 'Dynamic_AIFeaturesPromoModal_Root', 'Money_Request_Step_Scan', 'Members_Move_To_Group', + 'Workspace_Members_Role', 'PreMountBuffer', 'Multifactor_Authentication_Validate_Code', 'Multifactor_Authentication_Outcome_Success', diff --git a/src/libs/Navigation/linkingConfig/RELATIONS/WORKSPACE_TO_RHP.ts b/src/libs/Navigation/linkingConfig/RELATIONS/WORKSPACE_TO_RHP.ts index e827c9827c03..e74f7ed1bfdd 100755 --- a/src/libs/Navigation/linkingConfig/RELATIONS/WORKSPACE_TO_RHP.ts +++ b/src/libs/Navigation/linkingConfig/RELATIONS/WORKSPACE_TO_RHP.ts @@ -18,6 +18,7 @@ const WORKSPACE_TO_RHP: Partial Date: Thu, 8 Oct 2026 19:58:49 +0530 Subject: [PATCH 16/23] Scope the stashed role-change selection to its workspace Signed-off-by: krishna2323 --- src/ONYXKEYS.ts | 4 +- src/libs/actions/Policy/Member.ts | 21 +++++-- src/pages/workspace/WorkspaceMembersPage.tsx | 14 ++--- .../members/WorkspaceMembersRolePage.tsx | 23 ++++++-- .../WorkspaceMembersSelectedForRoleChange.ts | 15 +++++ src/types/onyx/index.ts | 2 + tests/ui/WorkspaceMembersTest.tsx | 56 ++++++++++++++++++- 7 files changed, 116 insertions(+), 19 deletions(-) create mode 100644 src/types/onyx/WorkspaceMembersSelectedForRoleChange.ts diff --git a/src/ONYXKEYS.ts b/src/ONYXKEYS.ts index b7444c0b1119..0a66e279e539 100755 --- a/src/ONYXKEYS.ts +++ b/src/ONYXKEYS.ts @@ -550,7 +550,7 @@ const ONYXKEYS = { /** Selected domain member account IDs for the move-to-group operation */ RAM_ONLY_DOMAIN_MEMBERS_SELECTED_FOR_MOVE: 'domainMembersSelectedForMove', - /** Logins of the workspace members selected for the bulk role change */ + /** The workspace members selected for the bulk role change, with the workspace they were selected on */ RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE: 'workspaceMembersSelectedForRoleChange', // The theme setting set by the user in preferences. @@ -1834,7 +1834,7 @@ type OnyxValuesMapping = { [ONYXKEYS.VIEWING_PUBLIC_ROOM_REPORT_ID]: string; [ONYXKEYS.MY_DOMAIN_SECURITY_GROUPS]: Record; [ONYXKEYS.RAM_ONLY_DOMAIN_MEMBERS_SELECTED_FOR_MOVE]: string[]; - [ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE]: string[]; + [ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE]: OnyxTypes.WorkspaceMembersSelectedForRoleChange; [ONYXKEYS.VERIFY_3DS_SUBSCRIPTION]: string; [ONYXKEYS.PREFERRED_THEME]: ValueOf; [ONYXKEYS.SIGN_IN_HIGH_CONTRAST_INTENT]: boolean; diff --git a/src/libs/actions/Policy/Member.ts b/src/libs/actions/Policy/Member.ts index 0bfb8fb2cc66..14975bf3fce5 100644 --- a/src/libs/actions/Policy/Member.ts +++ b/src/libs/actions/Policy/Member.ts @@ -1303,12 +1303,24 @@ function setWorkspaceInviteMembersDraft(policyID: string, invitedEmailsToAccount Onyx.set(`${ONYXKEYS.COLLECTION.WORKSPACE_INVITE_MEMBERS_DRAFT}${policyID}`, invitedEmailsToAccountIDs); } -function setMembersSelectedForRoleChange(memberLogins: string[]) { - Onyx.set(ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE, memberLogins); +function setMembersSelectedForRoleChange(policyID: string, memberLogins: string[]) { + Onyx.set(ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE, {policyID, logins: memberLogins}); } -function clearMembersSelectedForRoleChange() { - Onyx.set(ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE, []); +/** + * Empties the selection in place, which is how the members table learns the change was saved and drops the rows it + * still has selected. + */ +function clearMembersSelectedForRoleChange(policyID: string) { + Onyx.set(ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE, {policyID, logins: []}); +} + +/** + * Drops the selection entirely, for leaving the role screen without saving. The members table keeps its rows selected, + * since an empty selection is reserved for a saved change. + */ +function discardMembersSelectedForRoleChange() { + Onyx.set(ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE, null); } function setWorkspaceInviteRoleDraft(policyID: string, role: ValueOf) { @@ -1516,6 +1528,7 @@ export { setWorkspaceInviteRoleDraft, setMembersSelectedForRoleChange, clearMembersSelectedForRoleChange, + discardMembersSelectedForRoleChange, clearWorkspaceInviteRoleDraft, setWorkspaceInviteApproverDraft, clearWorkspaceInviteApproverDraft, diff --git a/src/pages/workspace/WorkspaceMembersPage.tsx b/src/pages/workspace/WorkspaceMembersPage.tsx index 6857873d83d7..a5e11623e204 100644 --- a/src/pages/workspace/WorkspaceMembersPage.tsx +++ b/src/pages/workspace/WorkspaceMembersPage.tsx @@ -590,14 +590,14 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers }); // The role change happens on its own screen, which reports the change back by emptying the shared selection. - // Backing out of that screen leaves the selection in place, so only a drop to zero clears the rows here. + // Backing out of that screen drops the selection instead, so only a drop to zero clears the rows here. const [membersSelectedForRoleChange] = useOnyx(ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE); - const membersSelectedForRoleChangeCount = membersSelectedForRoleChange?.length ?? 0; - const [lastMembersSelectedForRoleChangeCount, setLastMembersSelectedForRoleChangeCount] = useState(membersSelectedForRoleChangeCount); - if (lastMembersSelectedForRoleChangeCount !== membersSelectedForRoleChangeCount) { - setLastMembersSelectedForRoleChangeCount(membersSelectedForRoleChangeCount); + const isRoleChangeSaved = membersSelectedForRoleChange?.policyID === policyID && membersSelectedForRoleChange.logins.length === 0; + const [wasRoleChangeSaved, setWasRoleChangeSaved] = useState(isRoleChangeSaved); + if (wasRoleChangeSaved !== isRoleChangeSaved) { + setWasRoleChangeSaved(isRoleChangeSaved); - if (lastMembersSelectedForRoleChangeCount > 0 && membersSelectedForRoleChangeCount === 0) { + if (isRoleChangeSaved) { setSelectedEmployees([]); } } @@ -621,7 +621,7 @@ function WorkspaceMembersPage({personalDetails, route, policy}: WorkspaceMembers icon: icons.UserPencil, shouldSkipFocusRestore: true, onSelected: () => { - setMembersSelectedForRoleChange(selectedEmployees); + setMembersSelectedForRoleChange(policyID, selectedEmployees); Navigation.navigate(ROUTES.WORKSPACE_MEMBERS_ROLE.getRoute(policyID)); }, }); diff --git a/src/pages/workspace/members/WorkspaceMembersRolePage.tsx b/src/pages/workspace/members/WorkspaceMembersRolePage.tsx index 380c5c2f974a..eee2a7bf4ff1 100644 --- a/src/pages/workspace/members/WorkspaceMembersRolePage.tsx +++ b/src/pages/workspace/members/WorkspaceMembersRolePage.tsx @@ -9,7 +9,7 @@ import {usePersonalDetailsByLogins} from '@hooks/usePersonalDetailByLogin'; import useRedirectSubmitWorkspaceFeatureUpgrade from '@hooks/useRedirectSubmitWorkspaceFeatureUpgrade'; import useRuleBotGuardModal from '@hooks/useRuleBotGuardModal'; -import {clearMembersSelectedForRoleChange, updateWorkspaceMembersRole} from '@libs/actions/Policy/Member'; +import {clearMembersSelectedForRoleChange, discardMembersSelectedForRoleChange, updateWorkspaceMembersRole} from '@libs/actions/Policy/Member'; import {isRuleBotEnforcingRules} from '@libs/AgentRulesUtils'; import Navigation from '@libs/Navigation/Navigation'; import type {PlatformStackScreenProps} from '@libs/Navigation/PlatformStackNavigation/types'; @@ -28,7 +28,7 @@ import isLoadingOnyxValue from '@src/types/utils/isLoadingOnyxValue'; import type {ValueOf} from 'type-fest'; -import React, {useEffect, useState} from 'react'; +import React, {useEffect, useRef, useState} from 'react'; type WorkspaceMembersRolePageProps = WithPolicyAndFullscreenLoadingProps & PlatformStackScreenProps; @@ -41,7 +41,8 @@ function WorkspaceMembersRolePage({policy, route}: WorkspaceMembersRolePageProps const employeePersonalDetails = usePersonalDetailsByLogins(Object.keys(policy?.employeeList ?? {})); const memberEmailsToAccountIDs = getMemberAccountIDsForWorkspace(policy?.employeeList, employeePersonalDetails, true); - const memberLogins = selectedLogins ?? []; + // A selection left behind by an earlier visit belongs to the workspace it was made on, so it is ignored here. + const memberLogins = selectedLogins?.policyID === policyID ? selectedLogins.logins : []; const memberRoles = memberLogins.map((login) => policy?.employeeList?.[login]?.role); const canManageSelectedMemberRoles = memberRoles.every((role) => canMemberManageMemberWithRole(policy, currentUserLogin, role)); @@ -73,6 +74,19 @@ function WorkspaceMembersRolePage({policy, route}: WorkspaceMembersRolePageProps Navigation.navigate(ROUTES.WORKSPACE_MEMBERS.getRoute(policyID)); }, [isSelectionLoading, memberCount, policyID]); + // Leaving without saving drops the selection, so coming back to this screen through history cannot apply a role to + // members the table no longer has selected. + const hasSaved = useRef(false); + useEffect( + () => () => { + if (hasSaved.current) { + return; + } + discardMembersSelectedForRoleChange(); + }, + [], + ); + const saveAndGoBack = () => { if (!selectedRole) { setHasError(true); @@ -91,7 +105,8 @@ function WorkspaceMembersRolePage({policy, route}: WorkspaceMembersRolePageProps updateWorkspaceMembersRole(policy, loginsToUpdate, accountIDsToUpdate, selectedRole); } - clearMembersSelectedForRoleChange(); + hasSaved.current = true; + clearMembersSelectedForRoleChange(policyID); Navigation.goBack(ROUTES.WORKSPACE_MEMBERS.getRoute(policyID)); }; diff --git a/src/types/onyx/WorkspaceMembersSelectedForRoleChange.ts b/src/types/onyx/WorkspaceMembersSelectedForRoleChange.ts new file mode 100644 index 000000000000..b46024cfb427 --- /dev/null +++ b/src/types/onyx/WorkspaceMembersSelectedForRoleChange.ts @@ -0,0 +1,15 @@ +/** + * The workspace members picked for a bulk role change, handed to the role screen that applies it. + * + * RAM-only, so the role screen has nothing to act on when it is reached by a reload or a deep link rather than from + * the members table. + */ +type WorkspaceMembersSelectedForRoleChange = { + /** The workspace the members were picked on, so a stash left behind cannot be applied to another one */ + policyID: string; + + /** Logins of the picked members. Empty once the change has been saved, which is what clears the rows behind it. */ + logins: string[]; +}; + +export default WorkspaceMembersSelectedForRoleChange; diff --git a/src/types/onyx/index.ts b/src/types/onyx/index.ts index fad7cf8b5ab5..a5191bf4db63 100644 --- a/src/types/onyx/index.ts +++ b/src/types/onyx/index.ts @@ -229,6 +229,7 @@ import type WalletOnfido from './WalletOnfido'; import type WalletStatement from './WalletStatement'; import type WalletTerms from './WalletTerms'; import type WalletTransfer from './WalletTransfer'; +import type WorkspaceMembersSelectedForRoleChange from './WorkspaceMembersSelectedForRoleChange'; export type { FileObject, @@ -397,6 +398,7 @@ export type { WalletStatement, WalletTerms, WalletTransfer, + WorkspaceMembersSelectedForRoleChange, SupportalPermissionDenied, PurchaseList, Purchases, diff --git a/tests/ui/WorkspaceMembersTest.tsx b/tests/ui/WorkspaceMembersTest.tsx index 8001ad1a1e0a..bb0d9ddea119 100644 --- a/tests/ui/WorkspaceMembersTest.tsx +++ b/tests/ui/WorkspaceMembersTest.tsx @@ -225,7 +225,7 @@ describe('WorkspaceMembers', () => { const selectForRoleChange = async (logins: string[]) => { await act(async () => { - await Onyx.set(ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE, logins); + await Onyx.set(ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE, {policyID: policy.id, logins}); }); }; @@ -314,6 +314,58 @@ describe('WorkspaceMembers', () => { await waitForBatchedUpdatesWithAct(); }); + it('should ignore a selection stashed on another workspace', async () => { + // Given an admin stashed for a role change on a different workspace, as backing out of this screen there would leave behind + await act(async () => { + await Onyx.set(ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE, {policyID: 'ANOTHER_WORKSPACE', logins: [adminEmail]}); + }); + + const {unmount} = renderRolePage(policy.id); + await waitForBatchedUpdatesWithAct(); + + // When Save is pressed + const saveButton = await screen.findByText(TestHelper.translateLocal('common.save')); + fireEvent.press(saveButton, { + nativeEvent: {}, + type: 'press', + target: saveButton, + currentTarget: saveButton, + }); + await waitForBatchedUpdatesWithAct(); + + // Then the stashed admin was never adopted, so the screen has nothing picked to save + await waitFor(() => { + expect(screen.getByText(TestHelper.translateLocal('common.error.pleaseSelectOne'))).toBeOnTheScreen(); + }); + + unmount(); + await waitForBatchedUpdatesWithAct(); + }); + + it('should drop the selection when the screen is left without saving', async () => { + // Given an admin selected for a role change + await selectForRoleChange([adminEmail]); + + const {unmount} = renderRolePage(policy.id); + await waitForBatchedUpdatesWithAct(); + + // When the screen is left without saving + unmount(); + await waitForBatchedUpdatesWithAct(); + + // Then nothing is left for a later visit to act on, and the empty selection that reports a saved change is not written + const stashedSelection = await new Promise((resolve) => { + const connection = Onyx.connect({ + key: ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE, + callback: (value) => { + Onyx.disconnect(connection); + resolve(value); + }, + }); + }); + expect(stashedSelection).toBeUndefined(); + }); + it('should only offer the roles that can pay when the selection holds the Authorized Payer', async () => { // Given a selection holding the Authorized Payer, who must stay a valid payer await act(async () => { @@ -430,7 +482,7 @@ describe('WorkspaceMembers', () => { // Given the RuleBot selected for a role change await makeAdminTheRuleBot(); await act(async () => { - await Onyx.set(ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE, [adminEmail]); + await Onyx.set(ONYXKEYS.RAM_ONLY_WORKSPACE_MEMBERS_SELECTED_FOR_ROLE_CHANGE, {policyID: policy.id, logins: [adminEmail]}); }); const {unmount} = renderRolePage(policy.id); From b7a71f558f1c7d2a10085d7dccca457a56ec8c3a Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Fri, 9 Oct 2026 01:04:38 +0530 Subject: [PATCH 17/23] Hand focus back when the bulk action bar goes away from another screen Signed-off-by: krishna2323 --- src/components/BulkActionBar/index.tsx | 5 +- .../useBulkActionBarFocus/index.ts | 26 +++-- .../useBulkActionBarFocus/types.ts | 6 +- tests/unit/useBulkActionBarFocusTest.ts | 99 +++++++++++++++++++ 4 files changed, 127 insertions(+), 9 deletions(-) create mode 100644 tests/unit/useBulkActionBarFocusTest.ts diff --git a/src/components/BulkActionBar/index.tsx b/src/components/BulkActionBar/index.tsx index 36fe938cae0e..cbddd6204445 100644 --- a/src/components/BulkActionBar/index.tsx +++ b/src/components/BulkActionBar/index.tsx @@ -82,8 +82,10 @@ function BulkActionBarContent({ const icons = useMemoizedLazyExpensifyIcons(['Close', 'DownArrow', 'UpArrow']); const {calculatePopoverPosition} = usePopoverPosition(); + const isFocused = useIsFocused(); + const barElementRef = useRef | null>(null); - const {suppressStrayFocusRing, isFocusInsideBar} = useBulkActionBarFocus(barElementRef); + const {suppressStrayFocusRing, isFocusInsideBar} = useBulkActionBarFocus(barElementRef, isFocused); const moreAnchorRef = useRef | null>(null); const [isMoreMenuVisible, setIsMoreMenuVisible] = useState(false); @@ -99,7 +101,6 @@ function BulkActionBarContent({ // reports itself visible too, and is told apart by the screen still being focused. The type is not worth reading // here, because a popover opened and closed over an RHP leaves it stale at `popover`. const [modal] = useOnyx(ONYXKEYS.MODAL); - const isFocused = useIsFocused(); const isCoveredByModal = !!modal?.isVisible && !isFocused; const canOwnEscape = !modal?.willAlertModalBecomeVisible && !isCoveredByModal; diff --git a/src/components/BulkActionBar/useBulkActionBarFocus/index.ts b/src/components/BulkActionBar/useBulkActionBarFocus/index.ts index 3b025c391498..cfa83abf07e0 100644 --- a/src/components/BulkActionBar/useBulkActionBarFocus/index.ts +++ b/src/components/BulkActionBar/useBulkActionBarFocus/index.ts @@ -1,5 +1,6 @@ import isHTMLElement from '@libs/isHTMLElement'; import markProgrammaticFocus from '@libs/programmaticFocus'; +import restoreFocusWithModality from '@libs/restoreFocusWithModality'; import {useEffect, useLayoutEffect, useRef, useState} from 'react'; @@ -18,7 +19,7 @@ function isShowingFocusRing(element: HTMLElement): boolean { } /** Tracks where focus is relative to the bar, and hands it back when the bar goes away holding it. */ -const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef) => { +const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef, isScreenFocused) => { // Where focus was before the bar took it. The bar goes away with the selection, so focus has to be handed back. const lastFocusedOutsideRef = useRef(null); @@ -38,6 +39,13 @@ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef) => { } setIsFocusInsideBar(false); + + // A screen opened over this one, such as the role screen, takes focus to controls that leave with it. + // Remembering one of those would leave the bar holding a target that is already gone when it needs one. + if (!isScreenFocused) { + return; + } + lastFocusedOutsideRef.current = target; }; @@ -57,7 +65,7 @@ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef) => { document.removeEventListener('focusin', handleFocusIn); document.removeEventListener('focusout', handleFocusOut); }; - }, [barRef]); + }, [barRef, isScreenFocused]); // Most of the bar's actions clear the selection themselves, so the bar is torn out from under the button that was // just pressed and focus would fall to the document. Handing it back here covers every one of those actions, @@ -66,7 +74,13 @@ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef) => { useLayoutEffect( () => () => { const bar = barRef.current; - if (!isHTMLElement(bar) || !bar.contains(document.activeElement)) { + const active = document.activeElement; + + // An action run from a screen opened over the table clears the selection from there, so the bar is gone + // before that screen hands its own focus back and there is nothing left for it to hand back to. Focus has + // already fallen to the document by then, which is the same loss as the bar being torn out from under it. + const hasFocusFallenToDocument = !active || active === document.body; + if (!hasFocusFallenToDocument && !(isHTMLElement(bar) && bar.contains(active))) { return; } @@ -75,9 +89,9 @@ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef) => { return; } - // Focus is being put back where the user left it rather than moved somewhere new, so it arrives without a ring. - markProgrammaticFocus(previous); - previous.focus(); + // The screen that cleared the selection may still be unwinding its focus trap, which would otherwise pull + // focus straight back into the container it is closing. + restoreFocusWithModality(previous); }, [barRef], ); diff --git a/src/components/BulkActionBar/useBulkActionBarFocus/types.ts b/src/components/BulkActionBar/useBulkActionBarFocus/types.ts index 196fcbf66a64..44300fe0e98e 100644 --- a/src/components/BulkActionBar/useBulkActionBarFocus/types.ts +++ b/src/components/BulkActionBar/useBulkActionBarFocus/types.ts @@ -11,6 +11,10 @@ type BulkActionBarFocus = { isFocusInsideBar: boolean; }; -type UseBulkActionBarFocus = (barRef: RefObject) => BulkActionBarFocus; +/** + * `isScreenFocused` is false while a screen is open over the one the bar is on, which is how focus taken by that + * screen is told apart from focus still on the table behind it. + */ +type UseBulkActionBarFocus = (barRef: RefObject, isScreenFocused: boolean) => BulkActionBarFocus; export default UseBulkActionBarFocus; diff --git a/tests/unit/useBulkActionBarFocusTest.ts b/tests/unit/useBulkActionBarFocusTest.ts new file mode 100644 index 000000000000..8caecc8e49f8 --- /dev/null +++ b/tests/unit/useBulkActionBarFocusTest.ts @@ -0,0 +1,99 @@ +/** + * Tests for useBulkActionBarFocus (web), the hand-back of focus when the bulk action bar unmounts with the + * selection it describes. Runs against real DOM nodes in jsdom rather than a rendered bar, because the hook only + * ever reads the bar through a ref. + */ + +import {act, renderHook} from '@testing-library/react-native'; + +import type {RefObject} from 'react'; + +// Jest resolves index.native.ts by default, and that one is a no-op. +const useBulkActionBarFocus = require<{ + default: (barRef: RefObject, isScreenFocused: boolean) => {suppressStrayFocusRing: () => void; isFocusInsideBar: boolean}; +}>('../../src/components/BulkActionBar/useBulkActionBarFocus/index.ts').default; + +function createButton(label: string) { + const button = document.createElement('button'); + button.textContent = label; + document.body.appendChild(button); + return button; +} + +/** jsdom fires focusin for `focus()`, which is what the hook listens on, so the real event is enough here. */ +function focus(element: HTMLElement) { + act(() => element.focus()); +} + +afterEach(() => { + document.body.innerHTML = ''; +}); + +describe('useBulkActionBarFocus', () => { + it('hands focus back to the table when the bar unmounts holding it', () => { + // Given a row that was focused before the bar appeared, and a bar that has since taken focus + const row = createButton('Row'); + const bar = document.createElement('div'); + const barButton = createButton('Change role'); + bar.appendChild(barButton); + document.body.appendChild(bar); + + const {unmount} = renderHook(() => useBulkActionBarFocus({current: bar}, true)); + + focus(row); + focus(barButton); + + // When the action clears the selection and the bar goes away under the focused button + unmount(); + bar.remove(); + + // Then focus is back on the row rather than lost to the document + expect(document.activeElement).toBe(row); + }); + + it('hands focus back when an action run from another screen has already dropped it to the document', () => { + // Given a row focused on the table, and a screen opened over it that takes focus to its own Save button + const row = createButton('Row'); + const bar = document.createElement('div'); + document.body.appendChild(bar); + const save = createButton('Save'); + + const {rerender, unmount} = renderHook(({isScreenFocused}) => useBulkActionBarFocus({current: bar}, isScreenFocused), {initialProps: {isScreenFocused: true}}); + + focus(row); + + rerender({isScreenFocused: false}); + focus(save); + + // When that screen saves and leaves, so its own controls are gone and focus has fallen to the document + // before the bar unmounts with the selection + act(() => save.remove()); + expect(document.activeElement).toBe(document.body); + + unmount(); + bar.remove(); + + // Then focus is back on the row, not left on the document + expect(document.activeElement).toBe(row); + }); + + it('leaves focus alone when the bar unmounts without holding it', () => { + // Given a bar that unmounts while focus sits on a control of the table behind it + const row = createButton('Row'); + const other = createButton('Other'); + const bar = document.createElement('div'); + document.body.appendChild(bar); + + const {unmount} = renderHook(() => useBulkActionBarFocus({current: bar}, true)); + + focus(row); + focus(other); + + // When the last row is deselected and the bar goes away + unmount(); + bar.remove(); + + // Then the control the user is actually on keeps focus + expect(document.activeElement).toBe(other); + }); +}); From c67ba73b9ee2712ebf115ec26339f988b1f893bd Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Fri, 9 Oct 2026 01:34:55 +0530 Subject: [PATCH 18/23] Make the bulk action bar and the role error answer the keyboard Signed-off-by: krishna2323 --- src/components/BulkActionBar/BulkActionBarButton.tsx | 1 + src/components/BulkActionBar/index.tsx | 5 +++++ src/components/Button/Button.tsx | 2 ++ src/components/Button/types.ts | 5 +++++ src/components/WorkspaceMemberRoleList.tsx | 7 +++++++ .../workspace/members/WorkspaceMembersRolePage.tsx | 10 ++++++---- tests/ui/WorkspaceMembersTest.tsx | 11 +++++++++++ 7 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/components/BulkActionBar/BulkActionBarButton.tsx b/src/components/BulkActionBar/BulkActionBarButton.tsx index 1b6b41b4e13d..6e12a5638216 100644 --- a/src/components/BulkActionBar/BulkActionBarButton.tsx +++ b/src/components/BulkActionBar/BulkActionBarButton.tsx @@ -76,6 +76,7 @@ function BulkActionBarButton({option, onSubItemSelected}: BulkAction isDisabled={option.disabled} accessibilityLabel={option.text} accessibilityState={hasSubMenu ? {expanded: isMenuVisible} : undefined} + accessibilityHasPopup={hasSubMenu ? CONST.ROLE.MENU : undefined} sentryLabel={option.sentryLabel} > {!!option.icon && ( diff --git a/src/components/BulkActionBar/index.tsx b/src/components/BulkActionBar/index.tsx index cbddd6204445..a66d08532978 100644 --- a/src/components/BulkActionBar/index.tsx +++ b/src/components/BulkActionBar/index.tsx @@ -21,6 +21,7 @@ import useThemeStyles from '@hooks/useThemeStyles'; import Accessibility from '@libs/Accessibility'; import mergeRefs from '@libs/mergeRefs'; +import moveMiniToolbarFocusWithArrowKey from '@libs/moveContextMenuFocusWithArrowKey'; import shouldPopoverUseScrollView from '@libs/shouldPopoverUseScrollView'; import {areAllModalsHidden, setDisableDismissOnEscape} from '@userActions/Modal'; @@ -178,6 +179,9 @@ function BulkActionBarContent({ style={styles.bulkActionBar} role={CONST.ROLE.TOOLBAR} accessibilityLabel={translate('bulkActionBar.label')} + // The toolbar role has screen readers promise Left and Right move between the buttons, so the bar has to + // answer them. Same handler as the report action mini toolbar, which is the same row of role=button children. + onKeyDown={moveMiniToolbarFocusWithArrowKey} onLayout={(event) => onBarLayout(event.nativeEvent.layout.width)} > {/* Sized for a three-digit count so the bar keeps still as the selection grows, and so swapping the @@ -208,6 +212,7 @@ function BulkActionBarContent({ onPress={() => setIsMoreMenuVisible((isVisible) => !isVisible)} accessibilityLabel={translate('common.more')} accessibilityState={{expanded: isMoreMenuVisible}} + accessibilityHasPopup={CONST.ROLE.MENU} sentryLabel={CONST.SENTRY_LABEL.BULK_ACTION_BAR.MORE} > {translate('common.more')} diff --git a/src/components/Button/Button.tsx b/src/components/Button/Button.tsx index 29e62ca71729..8563ba0635f3 100644 --- a/src/components/Button/Button.tsx +++ b/src/components/Button/Button.tsx @@ -51,6 +51,7 @@ function Button({ sentryLabel, ref, accessibilityState, + accessibilityHasPopup, }: ButtonProps) { const theme = useTheme(); const styles = useThemeStyles(); @@ -140,6 +141,7 @@ function Button({ testID={testID} accessibilityLabel={accessibilityLabel} accessibilityState={accessibilityState} + accessibilityHasPopup={accessibilityHasPopup} sentryLabel={sentryLabel} role={CONST.ROLE.BUTTON} isNested={isNested} diff --git a/src/components/Button/types.ts b/src/components/Button/types.ts index 6fad32beca96..634b4687d876 100644 --- a/src/components/Button/types.ts +++ b/src/components/Button/types.ts @@ -1,3 +1,5 @@ +import type {AccessibilityHasPopup} from '@components/Pressable/PressResponder/PressResponderContext'; + import type {ButtonVariant} from '@styles/utils/types'; import type CONST from '@src/CONST'; @@ -107,6 +109,9 @@ type BaseButtonProps = WithSentryLabel & /** Accessibility state to pass to the pressable */ accessibilityState?: AccessibilityState; + /** What the button opens, for a button that opens a menu or a dialog rather than acting straight away */ + accessibilityHasPopup?: AccessibilityHasPopup; + ref?: ForwardedRef>; }; diff --git a/src/components/WorkspaceMemberRoleList.tsx b/src/components/WorkspaceMemberRoleList.tsx index 2b82487b73c9..91d3d8e86772 100644 --- a/src/components/WorkspaceMemberRoleList.tsx +++ b/src/components/WorkspaceMemberRoleList.tsx @@ -45,6 +45,9 @@ type WorkspaceMemberRoleListProps = { /** Shown above the confirm button, for a confirmation pressed without a role picked */ errorMessage?: string; + + /** Bump to have the error read out again, since a repeated confirmation leaves the message itself unchanged */ + errorAnnouncementKey?: number; }; /** @@ -124,6 +127,7 @@ function WorkspaceMemberRoleList({ allowedRoles = undefined, confirmButtonOptions = undefined, errorMessage = '', + errorAnnouncementKey = 0, }: WorkspaceMemberRoleListProps) { const {translate} = useLocalize(); const styles = useThemeStyles(); @@ -150,7 +154,10 @@ function WorkspaceMemberRoleList({ > {!!errorMessage && ( + {/* Remounting is what makes the alert speak again. The list is not inside a `Form`, + so `FormHelpMessage` has no submit count of its own to re-announce from. */} diff --git a/src/pages/workspace/members/WorkspaceMembersRolePage.tsx b/src/pages/workspace/members/WorkspaceMembersRolePage.tsx index eee2a7bf4ff1..ecf82dc5a5b4 100644 --- a/src/pages/workspace/members/WorkspaceMembersRolePage.tsx +++ b/src/pages/workspace/members/WorkspaceMembersRolePage.tsx @@ -50,7 +50,8 @@ function WorkspaceMembersRolePage({policy, route}: WorkspaceMembersRolePageProps // selected and the member has to make a deliberate choice. const sharedRole = memberRoles.length > 0 ? Object.values(CONST.POLICY.ROLE).find((role) => memberRoles.every((memberRole) => memberRole === role)) : undefined; const [draftRole, setDraftRole] = useState>(); - const [hasError, setHasError] = useState(false); + // Counted rather than flagged, so a Save pressed again with still nothing picked reads the error out again. + const [rejectedSaveCount, setRejectedSaveCount] = useState(0); const selectedRole = draftRole ?? sharedRole; // The Authorized Payer (reimburser) must stay a valid payer, so restrict the whole selection to the roles that can pay once it includes them. @@ -89,7 +90,7 @@ function WorkspaceMembersRolePage({policy, route}: WorkspaceMembersRolePageProps const saveAndGoBack = () => { if (!selectedRole) { - setHasError(true); + setRejectedSaveCount((count) => count + 1); return; } @@ -126,7 +127,7 @@ function WorkspaceMembersRolePage({policy, route}: WorkspaceMembersRolePageProps policy={policy} onSelectRole={({value}: ListItemType) => { setDraftRole(value); - setHasError(false); + setRejectedSaveCount(0); }} allowedRoles={allowedRoles} navigateBackTo={ROUTES.WORKSPACE_MEMBERS.getRoute(policyID)} @@ -135,7 +136,8 @@ function WorkspaceMembersRolePage({policy, route}: WorkspaceMembersRolePageProps text: translate('common.save'), onConfirm: saveAndGoBack, }} - errorMessage={hasError ? translate('common.error.pleaseSelectOne') : ''} + errorMessage={rejectedSaveCount > 0 ? translate('common.error.pleaseSelectOne') : ''} + errorAnnouncementKey={rejectedSaveCount} /> diff --git a/tests/ui/WorkspaceMembersTest.tsx b/tests/ui/WorkspaceMembersTest.tsx index bb0d9ddea119..bb3bf36046ec 100644 --- a/tests/ui/WorkspaceMembersTest.tsx +++ b/tests/ui/WorkspaceMembersTest.tsx @@ -298,6 +298,17 @@ describe('WorkspaceMembers', () => { expect(screen.getByText(TestHelper.translateLocal('common.error.pleaseSelectOne'))).toBeOnTheScreen(); }); + // When Save is pressed again with still nothing picked, the ask stays up rather than being counted as handled + fireEvent.press(saveButton, { + nativeEvent: {}, + type: 'press', + target: saveButton, + currentTarget: saveButton, + }); + await waitForBatchedUpdatesWithAct(); + + expect(screen.getByText(TestHelper.translateLocal('common.error.pleaseSelectOne'))).toBeOnTheScreen(); + // When a role is then picked, the message clears const auditorRow = screen.getByText(roleName(CONST.POLICY.ROLE.AUDITOR)); fireEvent.press(auditorRow, { From 74205d943c4e2bdec82a0062e1d21896f6f4f337 Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Fri, 9 Oct 2026 17:40:26 +0530 Subject: [PATCH 19/23] Keep the bulk action bar's hand-back target on the table Signed-off-by: krishna2323 --- src/components/BulkActionBar/index.tsx | 14 ++++---- .../useBulkActionBarFocus/index.ts | 11 +++--- .../useBulkActionBarFocus/types.ts | 6 ++-- tests/unit/useBulkActionBarFocusTest.ts | 36 ++++++++++++++++--- 4 files changed, 47 insertions(+), 20 deletions(-) diff --git a/src/components/BulkActionBar/index.tsx b/src/components/BulkActionBar/index.tsx index a66d08532978..55cbd52fb2d3 100644 --- a/src/components/BulkActionBar/index.tsx +++ b/src/components/BulkActionBar/index.tsx @@ -85,8 +85,14 @@ function BulkActionBarContent({ const isFocused = useIsFocused(); + // Esc clears the selection, but not while something is open over the bar. An RHP the bar is rendered inside + // reports itself visible too, and is told apart by the screen still being focused. The type is not worth reading + // here, because a popover opened and closed over an RHP leaves it stale at `popover`. + const [modal] = useOnyx(ONYXKEYS.MODAL); + const isCoveredByModal = !!modal?.isVisible && !isFocused; + const barElementRef = useRef | null>(null); - const {suppressStrayFocusRing, isFocusInsideBar} = useBulkActionBarFocus(barElementRef, isFocused); + const {suppressStrayFocusRing, isFocusInsideBar} = useBulkActionBarFocus(barElementRef, !isFocused || !!modal?.isVisible); const moreAnchorRef = useRef | null>(null); const [isMoreMenuVisible, setIsMoreMenuVisible] = useState(false); @@ -98,12 +104,6 @@ function BulkActionBarContent({ const inlineOptions = hasMoreMenu ? options.slice(0, inlineActionCount) : options; const moreOptions = hasMoreMenu ? options.slice(inlineActionCount) : []; - // Esc clears the selection, but not while something is open over the bar. An RHP the bar is rendered inside - // reports itself visible too, and is told apart by the screen still being focused. The type is not worth reading - // here, because a popover opened and closed over an RHP leaves it stale at `popover`. - const [modal] = useOnyx(ONYXKEYS.MODAL); - const isCoveredByModal = !!modal?.isVisible && !isFocused; - const canOwnEscape = !modal?.willAlertModalBecomeVisible && !isCoveredByModal; // The Side Panel is not a `BaseModal`, so it is absent from both the modal state and the registry of open modals. diff --git a/src/components/BulkActionBar/useBulkActionBarFocus/index.ts b/src/components/BulkActionBar/useBulkActionBarFocus/index.ts index cfa83abf07e0..63fa3d24827c 100644 --- a/src/components/BulkActionBar/useBulkActionBarFocus/index.ts +++ b/src/components/BulkActionBar/useBulkActionBarFocus/index.ts @@ -19,7 +19,7 @@ function isShowingFocusRing(element: HTMLElement): boolean { } /** Tracks where focus is relative to the bar, and hands it back when the bar goes away holding it. */ -const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef, isScreenFocused) => { +const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef, isCoveredByOverlay) => { // Where focus was before the bar took it. The bar goes away with the selection, so focus has to be handed back. const lastFocusedOutsideRef = useRef(null); @@ -40,9 +40,10 @@ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef, isScreenFocused) = setIsFocusInsideBar(false); - // A screen opened over this one, such as the role screen, takes focus to controls that leave with it. - // Remembering one of those would leave the bar holding a target that is already gone when it needs one. - if (!isScreenFocused) { + // The role screen and the bar's own "More" menu both take focus to controls that leave with them, and the + // menu renders in a popover outside the bar rather than within it. Remembering either would leave the bar + // holding a target that is already gone when it needs one. + if (isCoveredByOverlay) { return; } @@ -65,7 +66,7 @@ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef, isScreenFocused) = document.removeEventListener('focusin', handleFocusIn); document.removeEventListener('focusout', handleFocusOut); }; - }, [barRef, isScreenFocused]); + }, [barRef, isCoveredByOverlay]); // Most of the bar's actions clear the selection themselves, so the bar is torn out from under the button that was // just pressed and focus would fall to the document. Handing it back here covers every one of those actions, diff --git a/src/components/BulkActionBar/useBulkActionBarFocus/types.ts b/src/components/BulkActionBar/useBulkActionBarFocus/types.ts index 44300fe0e98e..07262800e1f4 100644 --- a/src/components/BulkActionBar/useBulkActionBarFocus/types.ts +++ b/src/components/BulkActionBar/useBulkActionBarFocus/types.ts @@ -12,9 +12,9 @@ type BulkActionBarFocus = { }; /** - * `isScreenFocused` is false while a screen is open over the one the bar is on, which is how focus taken by that - * screen is told apart from focus still on the table behind it. + * `isCoveredByOverlay` is true while a screen or a popover is open over the bar. Focus landing outside the bar then + * belongs to that overlay rather than to the table, and leaves with it, so it is no use as a hand-back target. */ -type UseBulkActionBarFocus = (barRef: RefObject, isScreenFocused: boolean) => BulkActionBarFocus; +type UseBulkActionBarFocus = (barRef: RefObject, isCoveredByOverlay: boolean) => BulkActionBarFocus; export default UseBulkActionBarFocus; diff --git a/tests/unit/useBulkActionBarFocusTest.ts b/tests/unit/useBulkActionBarFocusTest.ts index 8caecc8e49f8..5a0bb2ffe370 100644 --- a/tests/unit/useBulkActionBarFocusTest.ts +++ b/tests/unit/useBulkActionBarFocusTest.ts @@ -10,7 +10,7 @@ import type {RefObject} from 'react'; // Jest resolves index.native.ts by default, and that one is a no-op. const useBulkActionBarFocus = require<{ - default: (barRef: RefObject, isScreenFocused: boolean) => {suppressStrayFocusRing: () => void; isFocusInsideBar: boolean}; + default: (barRef: RefObject, isCoveredByOverlay: boolean) => {suppressStrayFocusRing: () => void; isFocusInsideBar: boolean}; }>('../../src/components/BulkActionBar/useBulkActionBarFocus/index.ts').default; function createButton(label: string) { @@ -38,7 +38,7 @@ describe('useBulkActionBarFocus', () => { bar.appendChild(barButton); document.body.appendChild(bar); - const {unmount} = renderHook(() => useBulkActionBarFocus({current: bar}, true)); + const {unmount} = renderHook(() => useBulkActionBarFocus({current: bar}, false)); focus(row); focus(barButton); @@ -58,11 +58,11 @@ describe('useBulkActionBarFocus', () => { document.body.appendChild(bar); const save = createButton('Save'); - const {rerender, unmount} = renderHook(({isScreenFocused}) => useBulkActionBarFocus({current: bar}, isScreenFocused), {initialProps: {isScreenFocused: true}}); + const {rerender, unmount} = renderHook(({isCoveredByOverlay}) => useBulkActionBarFocus({current: bar}, isCoveredByOverlay), {initialProps: {isCoveredByOverlay: false}}); focus(row); - rerender({isScreenFocused: false}); + rerender({isCoveredByOverlay: true}); focus(save); // When that screen saves and leaves, so its own controls are gone and focus has fallen to the document @@ -84,7 +84,7 @@ describe('useBulkActionBarFocus', () => { const bar = document.createElement('div'); document.body.appendChild(bar); - const {unmount} = renderHook(() => useBulkActionBarFocus({current: bar}, true)); + const {unmount} = renderHook(() => useBulkActionBarFocus({current: bar}, false)); focus(row); focus(other); @@ -96,4 +96,30 @@ describe('useBulkActionBarFocus', () => { // Then the control the user is actually on keeps focus expect(document.activeElement).toBe(other); }); + + it('keeps the table target when focus passes through the bar\'s own "More" menu', () => { + // Given a row focused on the table, then the bar's "More" menu opened over it. The menu renders in a popover + // portalled to the body, so its items are outside the bar in the DOM but on the same screen. + const row = createButton('Row'); + const bar = document.createElement('div'); + document.body.appendChild(bar); + const menuItem = createButton('Remove members'); + + const {rerender, unmount} = renderHook(({isCoveredByOverlay}) => useBulkActionBarFocus({current: bar}, isCoveredByOverlay), {initialProps: {isCoveredByOverlay: false}}); + + focus(row); + + rerender({isCoveredByOverlay: true}); + focus(menuItem); + + // When the chosen action clears the selection, so the menu has closed and the bar unmounts with it + act(() => menuItem.remove()); + rerender({isCoveredByOverlay: false}); + + unmount(); + bar.remove(); + + // Then focus is back on the row, rather than lost because the menu item was remembered and is now gone + expect(document.activeElement).toBe(row); + }); }); From 669a14cdda5adeb74775e8b67024e72e68ebf5f8 Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Sat, 10 Oct 2026 01:18:13 +0530 Subject: [PATCH 20/23] Restore focus to the table after a bulk action navigates Signed-off-by: krishna2323 --- .../useBulkActionBarFocus/index.ts | 7 ++ .../NavigationFocusReturn/index.native.ts | 5 ++ src/libs/NavigationFocusReturn/index.ts | 17 +++++ tests/unit/NavigationFocusReturnTest.ts | 66 +++++++++++++++++++ 4 files changed, 95 insertions(+) diff --git a/src/components/BulkActionBar/useBulkActionBarFocus/index.ts b/src/components/BulkActionBar/useBulkActionBarFocus/index.ts index 63fa3d24827c..a58cb557736f 100644 --- a/src/components/BulkActionBar/useBulkActionBarFocus/index.ts +++ b/src/components/BulkActionBar/useBulkActionBarFocus/index.ts @@ -1,4 +1,5 @@ import isHTMLElement from '@libs/isHTMLElement'; +import {nominateTriggerFallback} from '@libs/NavigationFocusReturn'; import markProgrammaticFocus from '@libs/programmaticFocus'; import restoreFocusWithModality from '@libs/restoreFocusWithModality'; @@ -48,6 +49,10 @@ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef, isCoveredByOverlay } lastFocusedOutsideRef.current = target; + // An action that opens a screen is navigated back from, and the restore that follows aims at the bar + // button it was opened from, which the cleared selection has taken away by then. This is the survivor to + // aim at instead, and it is offered here rather than on unmount because the capture happens first. + nominateTriggerFallback(target); }; // Focus can also leave for nothing at all, such as an element being removed, which fires no matching focusin. @@ -74,6 +79,8 @@ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef, isCoveredByOverlay // read the focus before the bar leaves the DOM. useLayoutEffect( () => () => { + nominateTriggerFallback(null); + const bar = barRef.current; const active = document.activeElement; diff --git a/src/libs/NavigationFocusReturn/index.native.ts b/src/libs/NavigationFocusReturn/index.native.ts index 7db71f9dd4e5..28d41d2f79dd 100644 --- a/src/libs/NavigationFocusReturn/index.native.ts +++ b/src/libs/NavigationFocusReturn/index.native.ts @@ -329,6 +329,10 @@ function shouldSkipAutoFocusDueToExistingFocus(): boolean { return false; } +/** Web only. Native restores to a registered pressable rather than to a DOM element, so there is nothing to nominate. */ +// eslint-disable-next-line @typescript-eslint/no-unused-vars +function nominateTriggerFallback(_element: HTMLElement | null): void {} + function resetForTests(): void { teardownNavigationFocusReturn(); } @@ -352,6 +356,7 @@ export { teardownNavigationFocusReturn, handleStateChange, notifyPressedTrigger, + nominateTriggerFallback, registerPressable, notifyPushParamsForward, notifyPushParamsBackward, diff --git a/src/libs/NavigationFocusReturn/index.ts b/src/libs/NavigationFocusReturn/index.ts index ba7b9046d8dc..5ee9e6ba979e 100644 --- a/src/libs/NavigationFocusReturn/index.ts +++ b/src/libs/NavigationFocusReturn/index.ts @@ -38,6 +38,7 @@ let lastMouseTriggerAt = 0; let lastKeyboardTriggerElement: HTMLElement | null = null; let lastKeyboardTriggerTime = 0; let pendingActivationKey: 'Enter' | 'Space' | null = null; +let nominatedFallback: HTMLElement | null = null; function setTriggerEntry(routeKey: string, entry: TriggerEntry): void { setFifoEntry(triggerMap, routeKey, entry, TRIGGER_MAP_MAX); @@ -121,9 +122,23 @@ function captureTriggerForRoute(routeKey: string): void { if (!inner) { return; } + if (nominatedFallback && nominatedFallback !== inner) { + setTriggerEntry(routeKey, {primary: inner, fallback: nominatedFallback}); + return; + } setTriggerEntry(routeKey, {primary: inner}); } +/** + * Nominates a second choice to return to for the next route opened from here, for callers whose controls are removed + * while that route is open. A bulk action bar is the case this exists for. Its buttons go away with the selection the + * route cleared, so the captured trigger is detached by the time the route pops and focus falls to the document. + * Passing null withdraws the nomination, which callers do when they unmount. + */ +function nominateTriggerFallback(element: HTMLElement | null): void { + nominatedFallback = element; +} + /** Single-site latch reset so the three fields always clear together and future additions get one call site. */ function clearKeyboardLatch(): void { lastKeyboardTriggerElement = null; @@ -599,6 +614,7 @@ function resetForTests(): void { clearTransientCaptures(); lastRestoreTarget = null; skipNextRestore = false; + nominatedFallback = null; } function setLastInteractiveElementForTests(element: HTMLElement | null): void { @@ -690,6 +706,7 @@ export { teardownNavigationFocusReturn, handleStateChange, captureTriggerForRoute, + nominateTriggerFallback, restoreTriggerForRoute, notifyPushParamsForward, notifyPushParamsBackward, diff --git a/tests/unit/NavigationFocusReturnTest.ts b/tests/unit/NavigationFocusReturnTest.ts index df46623cd012..4395a1f61c68 100644 --- a/tests/unit/NavigationFocusReturnTest.ts +++ b/tests/unit/NavigationFocusReturnTest.ts @@ -40,6 +40,7 @@ const {resetForTests: resetHadTabNavigation, setupHadTabNavigation} = require<{ }>('../../src/libs/hadTabNavigation.ts'); const { captureTriggerForRoute, + nominateTriggerFallback, restoreTriggerForRoute, handleStateChange, resetForTests, @@ -55,6 +56,7 @@ const { teardownNavigationFocusReturn, } = require<{ captureTriggerForRoute: (routeKey: string) => void; + nominateTriggerFallback: (element: HTMLElement | null) => void; restoreTriggerForRoute: (routeKey: string) => boolean; handleStateChange: (state: unknown) => void; resetForTests: () => void; @@ -3083,3 +3085,67 @@ describe('teardown / setup lifecycle', () => { } }); }); + +describe('nominateTriggerFallback', () => { + beforeEach(() => { + simulateTab(); + }); + + it('should restore the nominated element when the captured trigger has been removed', () => { + // Given a row the user came from, and a bulk action bar button that opened the route. The bar goes away with + // the selection the route cleared, so by the time the route pops its button is detached. + const row = appendButton(); + const barButton = appendButton(); + row.focus(); + row.dispatchEvent(new FocusEvent('focusin', {bubbles: true})); + nominateTriggerFallback(row); + barButton.focus(); + setLastInteractiveElementForTests(barButton); + + captureTriggerForRoute('route-a'); + barButton.remove(); + + // When the route pops + const rowSpy = jest.spyOn(row, 'focus'); + + // Then focus returns to the row rather than being lost for want of a candidate + expect(restoreTriggerForRoute('route-a')).toBe(true); + expect(rowSpy).toHaveBeenCalled(); + }); + + it('should stop offering the element once the nomination is withdrawn', () => { + const row = appendButton(); + const barButton = appendButton(); + nominateTriggerFallback(row); + nominateTriggerFallback(null); + barButton.focus(); + setLastInteractiveElementForTests(barButton); + + captureTriggerForRoute('route-a'); + barButton.remove(); + + // Then there is nothing left to restore to, as before the nomination existed + expect(restoreTriggerForRoute('route-a')).toBe(false); + }); + + it('should leave an active launcher as the fallback', () => { + // Given a popover launcher, which is the caller the existing fallback slot belongs to + const row = appendButton(); + const launcher = appendButton(); + const menuItem = appendButton(); + nominateTriggerFallback(row); + setActivePopoverLauncher(launcher); + menuItem.focus(); + menuItem.dispatchEvent(new FocusEvent('focusin', {bubbles: true})); + + captureTriggerForRoute('route-a'); + menuItem.remove(); + + // Then the launcher is restored, and the nomination does not displace it + const launcherSpy = jest.spyOn(launcher, 'focus'); + const rowSpy = jest.spyOn(row, 'focus'); + expect(restoreTriggerForRoute('route-a')).toBe(true); + expect(launcherSpy).toHaveBeenCalled(); + expect(rowSpy).not.toHaveBeenCalled(); + }); +}); From e2f1301fc2549a65d0bfbd422b45a4db37c8d08e Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Sat, 10 Oct 2026 01:40:52 +0530 Subject: [PATCH 21/23] Point the removed-approver test at the bulk action bar Signed-off-by: krishna2323 --- tests/ui/WorkspaceMembersTest.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/ui/WorkspaceMembersTest.tsx b/tests/ui/WorkspaceMembersTest.tsx index 006f9e39fd92..d05434d71f01 100644 --- a/tests/ui/WorkspaceMembersTest.tsx +++ b/tests/ui/WorkspaceMembersTest.tsx @@ -466,8 +466,7 @@ describe('WorkspaceMembers', () => { // When the admin removes that approver const row = await screen.findByLabelText(new RegExp(`^${approverWithoutDetails}`)); fireEvent.press(within(row).getByLabelText(TestHelper.translateLocal('common.select'))); - fireEvent.press(await screen.findByTestId('WorkspaceMembersPage-header-dropdown-menu-button')); - await waitForBatchedUpdatesWithAct(); + await openBulkActions(); const removeMenuItem = screen.getByText(TestHelper.translateLocal('workspace.people.removeMembersTitle', {count: 1})); fireEvent.press(removeMenuItem, {nativeEvent: {}, type: 'press', target: removeMenuItem, currentTarget: removeMenuItem}); await waitForBatchedUpdatesWithAct(); From 149c56a407d1d95cc7db60259b2bd820d8c86794 Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Tue, 13 Oct 2026 20:13:57 +0530 Subject: [PATCH 22/23] Find the bar's own popover through its launcher rather than through modal state Signed-off-by: krishna2323 --- src/components/BulkActionBar/index.tsx | 2 +- .../useBulkActionBarFocus/index.ts | 18 +++++-- .../useBulkActionBarFocus/types.ts | 8 +-- tests/ui/WorkspaceMembersTest.tsx | 2 +- tests/unit/NavigationFocusReturnTest.ts | 4 ++ tests/unit/useBulkActionBarFocusTest.ts | 53 ++++++++++++++++--- 6 files changed, 69 insertions(+), 18 deletions(-) diff --git a/src/components/BulkActionBar/index.tsx b/src/components/BulkActionBar/index.tsx index 55cbd52fb2d3..bafe5f2c7c23 100644 --- a/src/components/BulkActionBar/index.tsx +++ b/src/components/BulkActionBar/index.tsx @@ -92,7 +92,7 @@ function BulkActionBarContent({ const isCoveredByModal = !!modal?.isVisible && !isFocused; const barElementRef = useRef | null>(null); - const {suppressStrayFocusRing, isFocusInsideBar} = useBulkActionBarFocus(barElementRef, !isFocused || !!modal?.isVisible); + const {suppressStrayFocusRing, isFocusInsideBar} = useBulkActionBarFocus(barElementRef, isCoveredByModal); const moreAnchorRef = useRef | null>(null); const [isMoreMenuVisible, setIsMoreMenuVisible] = useState(false); diff --git a/src/components/BulkActionBar/useBulkActionBarFocus/index.ts b/src/components/BulkActionBar/useBulkActionBarFocus/index.ts index a58cb557736f..f7cada3f8974 100644 --- a/src/components/BulkActionBar/useBulkActionBarFocus/index.ts +++ b/src/components/BulkActionBar/useBulkActionBarFocus/index.ts @@ -1,4 +1,5 @@ import isHTMLElement from '@libs/isHTMLElement'; +import {pickLauncher} from '@libs/LauncherStack'; import {nominateTriggerFallback} from '@libs/NavigationFocusReturn'; import markProgrammaticFocus from '@libs/programmaticFocus'; import restoreFocusWithModality from '@libs/restoreFocusWithModality'; @@ -20,7 +21,7 @@ function isShowingFocusRing(element: HTMLElement): boolean { } /** Tracks where focus is relative to the bar, and hands it back when the bar goes away holding it. */ -const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef, isCoveredByOverlay) => { +const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef, isCoveredByScreen) => { // Where focus was before the bar took it. The bar goes away with the selection, so focus has to be handed back. const lastFocusedOutsideRef = useRef(null); @@ -43,8 +44,9 @@ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef, isCoveredByOverlay // The role screen and the bar's own "More" menu both take focus to controls that leave with them, and the // menu renders in a popover outside the bar rather than within it. Remembering either would leave the bar - // holding a target that is already gone when it needs one. - if (isCoveredByOverlay) { + // holding a target that is already gone when it needs one. A popover is identified by the launcher it + // registered as it opened, which it does before it moves focus into itself. + if (isCoveredByScreen || pickLauncher()) { return; } @@ -71,7 +73,7 @@ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef, isCoveredByOverlay document.removeEventListener('focusin', handleFocusIn); document.removeEventListener('focusout', handleFocusOut); }; - }, [barRef, isCoveredByOverlay]); + }, [barRef, isCoveredByScreen]); // Most of the bar's actions clear the selection themselves, so the bar is torn out from under the button that was // just pressed and focus would fall to the document. Handing it back here covers every one of those actions, @@ -88,7 +90,13 @@ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef, isCoveredByOverlay // before that screen hands its own focus back and there is nothing left for it to hand back to. Focus has // already fallen to the document by then, which is the same loss as the bar being torn out from under it. const hasFocusFallenToDocument = !active || active === document.body; - if (!hasFocusFallenToDocument && !(isHTMLElement(bar) && bar.contains(active))) { + + // An action run from the bar's own "More" menu is pressed with focus inside a popover that renders outside + // the bar, so the bar does not contain the focus it nonetheless has to hand back. The popover that leaves + // with the bar is the one whose launcher the bar does contain. + const launcher = pickLauncher(); + const isFocusInsideBarPopover = isHTMLElement(bar) && !!launcher && bar.contains(launcher); + if (!hasFocusFallenToDocument && !isFocusInsideBarPopover && !(isHTMLElement(bar) && bar.contains(active))) { return; } diff --git a/src/components/BulkActionBar/useBulkActionBarFocus/types.ts b/src/components/BulkActionBar/useBulkActionBarFocus/types.ts index 07262800e1f4..376b5b2b8d98 100644 --- a/src/components/BulkActionBar/useBulkActionBarFocus/types.ts +++ b/src/components/BulkActionBar/useBulkActionBarFocus/types.ts @@ -12,9 +12,11 @@ type BulkActionBarFocus = { }; /** - * `isCoveredByOverlay` is true while a screen or a popover is open over the bar. Focus landing outside the bar then - * belongs to that overlay rather than to the table, and leaves with it, so it is no use as a hand-back target. + * `isCoveredByScreen` is true while another screen is open over the bar. Focus landing outside the bar then belongs to + * that screen rather than to the table, and leaves with it, so it is no use as a hand-back target. Popovers are left + * out of this flag, because an RHP the bar is rendered inside reports itself as a visible modal for as long as it is + * open. The bar tells those apart by the launcher they were opened from instead. */ -type UseBulkActionBarFocus = (barRef: RefObject, isCoveredByOverlay: boolean) => BulkActionBarFocus; +type UseBulkActionBarFocus = (barRef: RefObject, isCoveredByScreen: boolean) => BulkActionBarFocus; export default UseBulkActionBarFocus; diff --git a/tests/ui/WorkspaceMembersTest.tsx b/tests/ui/WorkspaceMembersTest.tsx index fb36762dc667..0be94fe9c8c8 100644 --- a/tests/ui/WorkspaceMembersTest.tsx +++ b/tests/ui/WorkspaceMembersTest.tsx @@ -64,7 +64,7 @@ const RoleStack = createPlatformStackNavigator(); const renderRolePage = (policyID: string) => { return render( - + diff --git a/tests/unit/NavigationFocusReturnTest.ts b/tests/unit/NavigationFocusReturnTest.ts index 4395a1f61c68..bc0c2ee529d3 100644 --- a/tests/unit/NavigationFocusReturnTest.ts +++ b/tests/unit/NavigationFocusReturnTest.ts @@ -3114,6 +3114,8 @@ describe('nominateTriggerFallback', () => { }); it('should stop offering the element once the nomination is withdrawn', () => { + // Given a row nominated as the fallback and then withdrawn, which is what the bulk action bar does as it + // unmounts, before a bar button opens the route const row = appendButton(); const barButton = appendButton(); nominateTriggerFallback(row); @@ -3124,6 +3126,7 @@ describe('nominateTriggerFallback', () => { captureTriggerForRoute('route-a'); barButton.remove(); + // When the route pops // Then there is nothing left to restore to, as before the nomination existed expect(restoreTriggerForRoute('route-a')).toBe(false); }); @@ -3141,6 +3144,7 @@ describe('nominateTriggerFallback', () => { captureTriggerForRoute('route-a'); menuItem.remove(); + // When the route pops // Then the launcher is restored, and the nomination does not displace it const launcherSpy = jest.spyOn(launcher, 'focus'); const rowSpy = jest.spyOn(row, 'focus'); diff --git a/tests/unit/useBulkActionBarFocusTest.ts b/tests/unit/useBulkActionBarFocusTest.ts index 5a0bb2ffe370..ebd4fc20ec37 100644 --- a/tests/unit/useBulkActionBarFocusTest.ts +++ b/tests/unit/useBulkActionBarFocusTest.ts @@ -10,8 +10,13 @@ import type {RefObject} from 'react'; // Jest resolves index.native.ts by default, and that one is a no-op. const useBulkActionBarFocus = require<{ - default: (barRef: RefObject, isCoveredByOverlay: boolean) => {suppressStrayFocusRing: () => void; isFocusInsideBar: boolean}; + default: (barRef: RefObject, isCoveredByScreen: boolean) => {suppressStrayFocusRing: () => void; isFocusInsideBar: boolean}; }>('../../src/components/BulkActionBar/useBulkActionBarFocus/index.ts').default; +const {setActivePopoverLauncher, markActivePopoverLauncherDeactivated, resetLauncherStackForTests} = require<{ + setActivePopoverLauncher: (element: HTMLElement) => void; + markActivePopoverLauncherDeactivated: (element?: HTMLElement) => void; + resetLauncherStackForTests: () => void; +}>('../../src/libs/LauncherStack.ts'); function createButton(label: string) { const button = document.createElement('button'); @@ -26,6 +31,7 @@ function focus(element: HTMLElement) { } afterEach(() => { + resetLauncherStackForTests(); document.body.innerHTML = ''; }); @@ -58,11 +64,11 @@ describe('useBulkActionBarFocus', () => { document.body.appendChild(bar); const save = createButton('Save'); - const {rerender, unmount} = renderHook(({isCoveredByOverlay}) => useBulkActionBarFocus({current: bar}, isCoveredByOverlay), {initialProps: {isCoveredByOverlay: false}}); + const {rerender, unmount} = renderHook(({isCoveredByScreen}) => useBulkActionBarFocus({current: bar}, isCoveredByScreen), {initialProps: {isCoveredByScreen: false}}); focus(row); - rerender({isCoveredByOverlay: true}); + rerender({isCoveredByScreen: true}); focus(save); // When that screen saves and leaves, so its own controls are gone and focus has fallen to the document @@ -103,18 +109,22 @@ describe('useBulkActionBarFocus', () => { const row = createButton('Row'); const bar = document.createElement('div'); document.body.appendChild(bar); + const moreButton = createButton('More'); + bar.appendChild(moreButton); const menuItem = createButton('Remove members'); - const {rerender, unmount} = renderHook(({isCoveredByOverlay}) => useBulkActionBarFocus({current: bar}, isCoveredByOverlay), {initialProps: {isCoveredByOverlay: false}}); + const {unmount} = renderHook(() => useBulkActionBarFocus({current: bar}, false)); focus(row); - - rerender({isCoveredByOverlay: true}); + focus(moreButton); + act(() => setActivePopoverLauncher(moreButton)); focus(menuItem); // When the chosen action clears the selection, so the menu has closed and the bar unmounts with it - act(() => menuItem.remove()); - rerender({isCoveredByOverlay: false}); + act(() => { + markActivePopoverLauncherDeactivated(moreButton); + menuItem.remove(); + }); unmount(); bar.remove(); @@ -122,4 +132,31 @@ describe('useBulkActionBarFocus', () => { // Then focus is back on the row, rather than lost because the menu item was remembered and is now gone expect(document.activeElement).toBe(row); }); + + it('hands focus back when the bar unmounts while its own "More" menu still holds focus', () => { + // Given a row focused on the table, then an action chosen from the bar's "More" menu. The action clears the + // selection from the menu item, so the bar unmounts before the popover has finished closing and focus is + // still on the item rather than back on the table. + const row = createButton('Row'); + const bar = document.createElement('div'); + document.body.appendChild(bar); + const moreButton = createButton('More'); + bar.appendChild(moreButton); + const menuItem = createButton('Duplicate expenses'); + + const {unmount} = renderHook(() => useBulkActionBarFocus({current: bar}, false)); + + focus(row); + focus(moreButton); + act(() => setActivePopoverLauncher(moreButton)); + focus(menuItem); + + // When the bar goes away with the selection, taking the launcher the popover would otherwise return to + unmount(); + bar.remove(); + act(() => menuItem.remove()); + + // Then focus is back on the row, rather than aimed at a "More" button that left with the bar + expect(document.activeElement).toBe(row); + }); }); From eb142943e4a1c2380ae3e83168dc04f54c3be7a7 Mon Sep 17 00:00:00 2001 From: krishna2323 Date: Tue, 20 Oct 2026 07:15:08 +0530 Subject: [PATCH 23/23] Remember the control that made the selection Signed-off-by: krishna2323 --- .../useBulkActionBarFocus/index.ts | 30 +++++++++++++++---- tests/unit/useBulkActionBarFocusTest.ts | 23 ++++++++++++++ 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/src/components/BulkActionBar/useBulkActionBarFocus/index.ts b/src/components/BulkActionBar/useBulkActionBarFocus/index.ts index f7cada3f8974..2a2bc210d3e7 100644 --- a/src/components/BulkActionBar/useBulkActionBarFocus/index.ts +++ b/src/components/BulkActionBar/useBulkActionBarFocus/index.ts @@ -28,20 +28,16 @@ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef, isCoveredByScreen) const [isFocusInsideBar, setIsFocusInsideBar] = useState(false); useEffect(() => { - const handleFocusIn = (event: FocusEvent) => { - const target = event.target; - if (!isHTMLElement(target)) { + const remember = (target: EventTarget | null) => { + if (!isHTMLElement(target) || target === document.body) { return; } const bar = barRef.current; if (isHTMLElement(bar) && bar.contains(target)) { - setIsFocusInsideBar(true); return; } - setIsFocusInsideBar(false); - // The role screen and the bar's own "More" menu both take focus to controls that leave with them, and the // menu renders in a popover outside the bar rather than within it. Remembering either would leave the bar // holding a target that is already gone when it needs one. A popover is identified by the launcher it @@ -57,6 +53,28 @@ const useBulkActionBarFocus: UseBulkActionBarFocus = (barRef, isCoveredByScreen) nominateTriggerFallback(target); }; + // The bar appears because of a selection, so the control that made it already holds focus and fires no + // focusin for the listener below to see. + if (!lastFocusedOutsideRef.current) { + remember(document.activeElement); + } + + const handleFocusIn = (event: FocusEvent) => { + const target = event.target; + if (!isHTMLElement(target)) { + return; + } + + const bar = barRef.current; + if (isHTMLElement(bar) && bar.contains(target)) { + setIsFocusInsideBar(true); + return; + } + + setIsFocusInsideBar(false); + remember(target); + }; + // Focus can also leave for nothing at all, such as an element being removed, which fires no matching focusin. const handleFocusOut = (event: FocusEvent) => { if (event.relatedTarget) { diff --git a/tests/unit/useBulkActionBarFocusTest.ts b/tests/unit/useBulkActionBarFocusTest.ts index ebd4fc20ec37..aa96bdef2d8a 100644 --- a/tests/unit/useBulkActionBarFocusTest.ts +++ b/tests/unit/useBulkActionBarFocusTest.ts @@ -57,6 +57,29 @@ describe('useBulkActionBarFocus', () => { expect(document.activeElement).toBe(row); }); + it('hands focus back to a checkbox that was already focused when the bar appeared', () => { + // Given the row checkbox that made the selection, which held focus before the bar existed and so fires no + // focusin the bar can see + const row = createButton('Row'); + focus(row); + + const bar = document.createElement('div'); + const barButton = createButton('Clear selection'); + bar.appendChild(barButton); + document.body.appendChild(bar); + + const {unmount} = renderHook(() => useBulkActionBarFocus({current: bar}, false)); + + focus(barButton); + + // When the action clears the selection and the bar goes away under the focused button + unmount(); + bar.remove(); + + // Then focus is back on the checkbox rather than lost to the document + expect(document.activeElement).toBe(row); + }); + it('hands focus back when an action run from another screen has already dropped it to the document', () => { // Given a row focused on the table, and a screen opened over it that takes focus to its own Save button const row = createButton('Row');