Skip to content

Commit 9912ac3

Browse files
fix(mosaic): address pending action menu review feedback
1 parent 50c40d4 commit 9912ac3

14 files changed

Lines changed: 138 additions & 68 deletions

File tree

‎packages/mosaic/docs/controllers.md‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -104,13 +104,14 @@ section. The dialog opens once `visible` is true, so it skips the loading card,
104104
dismissal only while the controller has a cancel callback. Return focus with the hook's
105105
`lastKey`, which outlives the run, unlike `pendingKey`.
106106

107-
For a direct action in `ActionMenu`, set `pending` while its request runs and
108-
no dialog is open yet, and `false` otherwise so its row keeps room for the
109-
spinner. Set it in the same event as the click, since the menu
110-
closes on the next render unless an action is pending. The menu keeps its
111-
trailing spinner visible while pending, disables its actions, and blocks
112-
dismissal. It closes once `pending` turns false, whether a dialog opened or the
113-
request settled. Ordinary failures use the section's existing error.
107+
For a direct action in `ActionMenu`, opt in with `status: 'idle'`. Set it to
108+
`'pending'` while its request runs and no dialog is open yet. Update the status
109+
in the click event, since the menu closes on the next render unless an action
110+
is pending. The menu reserves room for a spinner, disables its actions, and
111+
blocks dismissal while pending. It closes when the status returns to `'idle'`,
112+
whether a dialog opened or the request settled. Leave `status` undefined for
113+
ordinary actions that close through the menu's native click behavior. Ordinary
114+
failures use the section's existing error.
114115

115116
A single open boolean earns nothing. A two-state machine with one event is a
116117
boolean spelled long. `useUserProfileEditUsernameController` holds `isOpen` in

‎packages/mosaic/src/components/action-menu/action-menu.test.tsx‎

Lines changed: 71 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1-
import { render, screen, waitFor } from '@testing-library/react';
1+
import { act, fireEvent, render, screen, waitFor, within } from '@testing-library/react';
22
import userEvent from '@testing-library/user-event';
3+
import { useState } from 'react';
34
import { describe, expect, it, vi } from 'vitest';
45

56
import { Pen } from '../../icons/glyphs/pen';
@@ -17,6 +18,27 @@ function renderMenu(actions: Parameters<typeof ActionMenu>[0]['actions']) {
1718
);
1819
}
1920

21+
function AsyncMenu({ onRemove = () => undefined }: { onRemove?: () => void }) {
22+
const [status, setStatus] = useState<'idle' | 'pending'>('idle');
23+
return (
24+
<MosaicProvider>
25+
<ActionMenu
26+
label='Manage passkey'
27+
actions={[
28+
{ label: 'Regenerate', status, onClick: () => setStatus('pending') },
29+
{ label: 'Remove', onClick: onRemove },
30+
]}
31+
/>
32+
<button
33+
type='button'
34+
onClick={() => setStatus('idle')}
35+
>
36+
Finish request
37+
</button>
38+
</MosaicProvider>
39+
);
40+
}
41+
2042
describe('Mosaic ActionMenu', () => {
2143
it('renders nothing when there are no actions', () => {
2244
renderMenu([]);
@@ -49,6 +71,54 @@ describe('Mosaic ActionMenu', () => {
4971
await waitFor(() => expect(screen.queryByRole('menu')).not.toBeInTheDocument());
5072
});
5173

74+
it('keeps an opted-in action open while pending and closes immediately when it settles', async () => {
75+
const user = userEvent.setup();
76+
const onRemove = vi.fn();
77+
render(<AsyncMenu onRemove={onRemove} />);
78+
await user.click(screen.getByRole('button', { name: 'Manage passkey' }));
79+
await user.click(await screen.findByRole('menuitem', { name: 'Regenerate' }));
80+
const menu = screen.getByRole('menu');
81+
expect(within(menu).getByRole('menuitem', { name: 'Regenerate' })).toHaveAttribute('aria-busy', 'true');
82+
expect(within(menu).getByRole('menuitem', { name: 'Remove' })).toHaveAttribute('aria-disabled', 'true');
83+
await user.click(within(menu).getByRole('menuitem', { name: 'Remove' }));
84+
expect(onRemove).not.toHaveBeenCalled();
85+
await user.keyboard('{Escape}');
86+
expect(screen.getByRole('menu')).toBeInTheDocument();
87+
await user.click(document.body);
88+
expect(screen.getByRole('menu')).toBeInTheDocument();
89+
await user.click(screen.getByRole('button', { name: 'Finish request' }));
90+
await waitFor(() => expect(screen.queryByRole('menu')).not.toBeInTheDocument());
91+
});
92+
93+
it('reserves an indicator immediately and delays its named spinner', async () => {
94+
const user = userEvent.setup();
95+
render(<AsyncMenu />);
96+
await user.click(screen.getByRole('button', { name: 'Manage passkey' }));
97+
const item = await screen.findByRole('menuitem', { name: 'Regenerate' });
98+
expect(item.querySelector('.cl-menu-indicator')).toBeInTheDocument();
99+
vi.useFakeTimers();
100+
try {
101+
fireEvent.click(item);
102+
expect(screen.queryByRole('progressbar', { name: 'Regenerate' })).not.toBeInTheDocument();
103+
await act(() => vi.advanceTimersByTime(149));
104+
expect(screen.queryByRole('progressbar', { name: 'Regenerate' })).not.toBeInTheDocument();
105+
await act(() => vi.advanceTimersByTime(1));
106+
expect(screen.getByRole('progressbar', { name: 'Regenerate' })).toBeInTheDocument();
107+
fireEvent.click(screen.getByRole('button', { name: 'Finish request' }));
108+
expect(screen.queryByRole('menu')).not.toBeInTheDocument();
109+
} finally {
110+
vi.useRealTimers();
111+
}
112+
});
113+
114+
it('closes an opted-in action that hands off before becoming pending', async () => {
115+
const user = userEvent.setup();
116+
renderMenu([{ label: 'Open dialog', status: 'idle', onClick: () => undefined }]);
117+
await user.click(screen.getByRole('button', { name: 'Manage passkey' }));
118+
await user.click(await screen.findByRole('menuitem', { name: 'Open dialog' }));
119+
await waitFor(() => expect(screen.queryByRole('menu')).not.toBeInTheDocument());
120+
});
121+
52122
it('keeps a disabled trigger focusable without allowing pointer or keyboard activation', async () => {
53123
const onClick = vi.fn();
54124
render(

‎packages/mosaic/src/components/action-menu/action-menu.tsx‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ export interface ActionMenuAction {
1111
color?: 'neutral' | 'negative';
1212
icon?: IconComponent;
1313
onClick: () => void;
14-
pending?: boolean;
14+
status?: 'idle' | 'pending';
1515
}
1616

1717
export interface ActionMenuProps {
@@ -25,8 +25,8 @@ export interface ActionMenuProps {
2525
export function ActionMenu({ label, actions, triggerRef, disabled, focusableWhenDisabled }: ActionMenuProps) {
2626
const [open, setOpen] = useState(false);
2727
const [activated, setActivated] = useState(false);
28-
const pending = actions.some(action => action.pending);
29-
const showSpinner = useSpinDelay(pending ? true : null);
28+
const pending = actions.some(action => action.status === 'pending');
29+
const showSpinner = useSpinDelay(pending ? true : null, { minDuration: 0 });
3030

3131
if (activated && !pending) {
3232
setActivated(false);
@@ -60,11 +60,13 @@ export function ActionMenu({ label, actions, triggerRef, disabled, focusableWhen
6060
color={action.color}
6161
label={action.label}
6262
onClick={() => {
63-
setActivated(true);
63+
if (action.status !== undefined) {
64+
setActivated(true);
65+
}
6466
action.onClick();
6567
}}
66-
closeOnClick={false}
67-
aria-busy={action.pending || undefined}
68+
closeOnClick={action.status === undefined}
69+
aria-busy={action.status === 'pending' || undefined}
6870
disabled={disabled || pending}
6971
>
7072
{action.icon ? (
@@ -73,14 +75,12 @@ export function ActionMenu({ label, actions, triggerRef, disabled, focusableWhen
7375
</Menu.Media>
7476
) : null}
7577
<Menu.Label>{action.label}</Menu.Label>
76-
{action.pending !== undefined ? (
78+
{action.status !== undefined ? (
7779
<Menu.Indicator>
78-
{action.pending && showSpinner ? (
80+
{action.status === 'pending' && showSpinner ? (
7981
<Spinner
8082
size='sm'
81-
role='progressbar'
82-
aria-hidden={undefined}
83-
aria-label={action.label}
83+
label={action.label}
8484
/>
8585
) : null}
8686
</Menu.Indicator>

‎packages/mosaic/src/components/button/submit-button.tsx‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -96,15 +96,7 @@ export const SubmitButton = React.forwardRef<HTMLButtonElement, SubmitButtonProp
9696
</span>
9797
{isPending || showPending ? (
9898
<Spinner
99-
// The indicator has to be in the accessibility tree the moment the button goes
100-
// pending, so it opts out of `Spinner`'s decorative default and carries an
101-
// indeterminate progressbar role. It is named in its own right rather than folded
102-
// into the button's name: `progressbar` is a range role, so the name computation
103-
// takes its value — absent, since it is indeterminate — over its label, and a
104-
// descendant one contributes nothing to the button above it.
105-
role='progressbar'
106-
aria-hidden={undefined}
107-
aria-label={pendingLabel}
99+
label={pendingLabel}
108100
size={spinnerSizes[size]}
109101
xstyle={[styles.spinner, !showPending && styles.spinnerHidden]}
110102
/>

‎packages/mosaic/src/components/spinner/spinner.test.tsx‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import * as stylex from '@stylexjs/stylex';
2-
import { render } from '@testing-library/react';
2+
import { render, screen } from '@testing-library/react';
33
import React from 'react';
44
import { describe, expect, it } from 'vitest';
55

@@ -20,6 +20,13 @@ describe('Mosaic Spinner', () => {
2020
expect(el).toHaveAttribute('aria-hidden', 'true');
2121
});
2222

23+
it('announces a labeled spinner as an indeterminate progressbar', () => {
24+
render(<Spinner label='Regenerating backup codes' />);
25+
const progress = screen.getByRole('progressbar', { name: 'Regenerating backup codes' });
26+
expect(progress).not.toHaveAttribute('aria-hidden');
27+
expect(progress).not.toHaveAttribute('aria-valuenow');
28+
});
29+
2330
it('applies the default size when none is passed', () => {
2431
const { container } = render(<Spinner />);
2532
expect(spinner(container)).toHaveAttribute('data-size', 'md');

‎packages/mosaic/src/components/spinner/spinner.tsx‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,11 @@ import { sizes, styles } from './spinner.styles';
77

88
export type SpinnerProps = MosaicElementProps<'span'> & {
99
size?: 'sm' | 'md';
10+
label?: string;
1011
};
1112

1213
/**
13-
* Indeterminate loading spinner. Decorative (`aria-hidden`) — pair it with a disabled control or an
14-
* `aria-busy` container so assistive tech is informed of the pending state.
14+
* Indeterminate loading spinner. Decorative by default; pass `label` to announce progress.
1515
*
1616
* @example
1717
* // Default (md), sized to match a `md` Icon
@@ -22,13 +22,15 @@ export type SpinnerProps = MosaicElementProps<'span'> & {
2222
* <Spinner size='sm' />
2323
*/
2424
export const Spinner = React.forwardRef<HTMLSpanElement, SpinnerProps>(function MosaicSpinner(
25-
{ size = 'md', xstyle, ...rest },
25+
{ size = 'md', label, xstyle, ...rest },
2626
ref,
2727
) {
2828
return (
2929
<span
3030
ref={ref}
31-
aria-hidden
31+
role={label ? 'progressbar' : undefined}
32+
aria-label={label}
33+
aria-hidden={label ? undefined : true}
3234
{...mergeStyleProps(themeProps('spinner', { size }), stylex.props(styles.base, sizes[size], xstyle), rest)}
3335
/>
3436
);

‎packages/mosaic/src/features/switcher/switcher.view.tsx‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -197,9 +197,7 @@ function PendingSpinner() {
197197
const m = useSwitcherMessages();
198198
return (
199199
<Spinner
200-
role='progressbar'
201-
aria-hidden={undefined}
202-
aria-label={m.organizations.pending}
200+
label={m.organizations.pending}
203201
size='sm'
204202
/>
205203
);

‎packages/mosaic/src/features/user-profile/__tests__/user-profile-security-panel.view.test.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,7 @@ describe('UserProfileSecurityPanelView', () => {
268268
const backupOnlyView = renderView(
269269
<UserProfileMfaSectionView
270270
methods={[backupCodes]}
271-
onRegenerateBackupCodes={onRegenerateBackupCodes}
271+
regenerate={{ onClick: onRegenerateBackupCodes, status: 'idle' }}
272272
onRemove={onRemove}
273273
/>,
274274
);
@@ -280,7 +280,7 @@ describe('UserProfileSecurityPanelView', () => {
280280
renderView(
281281
<UserProfileMfaSectionView
282282
methods={[{ id: 'sms_1', type: 'sms' }, backupCodes]}
283-
onRegenerateBackupCodes={onRegenerateBackupCodes}
283+
regenerate={{ onClick: onRegenerateBackupCodes, status: 'idle' }}
284284
onRemove={onRemove}
285285
/>,
286286
);

‎packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-row.view.tsx‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,8 @@ export function UserProfileMfaRowView({
1414
triggerRef,
1515
onRemove,
1616
onSetDefault,
17-
onRegenerateBackupCodes,
18-
regeneratePending,
19-
}: Pick<UserProfileMfaSectionViewProps, 'onRegenerateBackupCodes' | 'regeneratePending'> & {
17+
regenerate,
18+
}: Pick<UserProfileMfaSectionViewProps, 'regenerate'> & {
2019
method: UserProfileMfaMethod;
2120
triggerRef?: Ref<HTMLButtonElement>;
2221
onRemove?: () => void;
@@ -37,8 +36,8 @@ export function UserProfileMfaRowView({
3736
}
3837

3938
if (method.type === 'backup-codes') {
40-
if (onRegenerateBackupCodes) {
41-
actions.push({ label: m.regenerate, onClick: onRegenerateBackupCodes, pending: regeneratePending });
39+
if (regenerate) {
40+
actions.push({ label: m.regenerate, onClick: regenerate.onClick, status: regenerate.status });
4241
}
4342
} else if (onRemove && method.canRemove !== false) {
4443
actions.push({ label: m.remove, color: 'negative', onClick: onRemove });

‎packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.controller.ts‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -587,9 +587,13 @@ export function useUserProfileMfaController(model: ReadyModel) {
587587
const sectionProps: UserProfileMfaSectionViewProps = {
588588
methods: [...model.methods],
589589
addableMethods: model.addableMethods,
590-
onRegenerateBackupCodes: model.generateBackupCodes ? generateBackupCodes : undefined,
591-
regeneratePending: context.pending === 'generate' && !dialogOpen,
592-
regenerateError: flow.kind === 'closed' && flow.error ? errorText(flow.error) : undefined,
590+
regenerate: model.generateBackupCodes
591+
? {
592+
onClick: generateBackupCodes,
593+
status: context.pending === 'generate' && !dialogOpen ? 'pending' : 'idle',
594+
error: flow.kind === 'closed' && flow.error ? errorText(flow.error) : undefined,
595+
}
596+
: undefined,
593597
onRemove: model.remove,
594598
onSetDefault: model.setDefault,
595599
removeReverification: model.removeReverification,

0 commit comments

Comments
 (0)