Skip to content

Commit 7ad98fa

Browse files
MartinCupelaMartinCupela
authored
fix: maintain topmost modal non-inert (#3206)
1 parent 4c057bb commit 7ad98fa

4 files changed

Lines changed: 155 additions & 3 deletions

File tree

‎src/components/Dialog/__tests__/DialogsManager.test.ts‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,22 @@ describe('DialogManager', () => {
222222
expect(Object.keys(dialogManager.state.getLatestValue().dialogsById)).toHaveLength(0);
223223
});
224224

225+
it('removes a dialog from the open stack as soon as it is marked for removal', () => {
226+
vi.useFakeTimers({ shouldAdvanceTime: true });
227+
228+
const dialogManager = new DialogManager();
229+
dialogManager.open({ id: 'underlying-dialog' });
230+
dialogManager.open({ id: dialogId });
231+
dialogManager.markForRemoval(dialogId);
232+
233+
expect(dialogManager.state.getLatestValue().openedDialogIds).toEqual([
234+
'underlying-dialog',
235+
]);
236+
expect(dialogManager.openDialogCount).toBe(1);
237+
238+
vi.runAllTimers();
239+
});
240+
225241
it('cancels dialog removal if it is referenced again quickly', () => {
226242
vi.useFakeTimers({ shouldAdvanceTime: true });
227243

@@ -236,4 +252,26 @@ describe('DialogManager', () => {
236252
expect(dialogManager.openDialogCount).toBe(1);
237253
expect(Object.keys(dialogManager.state.getLatestValue().dialogsById)).toHaveLength(1);
238254
});
255+
256+
it('restores an open dialog to the stack when pending removal is cancelled', () => {
257+
vi.useFakeTimers({ shouldAdvanceTime: true });
258+
259+
const dialogManager = new DialogManager();
260+
dialogManager.open({ id: 'underlying-dialog' });
261+
dialogManager.open({ id: dialogId });
262+
dialogManager.markForRemoval(dialogId);
263+
264+
expect(dialogManager.state.getLatestValue().openedDialogIds).toEqual([
265+
'underlying-dialog',
266+
]);
267+
268+
dialogManager.getOrCreate({ id: dialogId });
269+
270+
expect(dialogManager.state.getLatestValue().openedDialogIds).toEqual([
271+
'underlying-dialog',
272+
dialogId,
273+
]);
274+
275+
vi.runAllTimers();
276+
});
239277
});

‎src/components/Dialog/service/DialogManager.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,9 @@ export class DialogManager {
112112
removalTimeout: undefined,
113113
},
114114
},
115+
openedDialogIds: current.dialogsById[id]?.isOpen
116+
? [...current.openedDialogIds.filter((dialogId) => dialogId !== id), id]
117+
: current.openedDialogIds,
115118
}));
116119
}
117120

@@ -200,6 +203,7 @@ export class DialogManager {
200203
}, 16),
201204
},
202205
},
206+
openedDialogIds: current.openedDialogIds.filter((dialogId) => dialogId !== id),
203207
}));
204208
}
205209

@@ -221,6 +225,9 @@ export class DialogManager {
221225
removalTimeout: undefined,
222226
},
223227
},
228+
openedDialogIds: current.dialogsById[id]?.isOpen
229+
? [...current.openedDialogIds.filter((dialogId) => dialogId !== id), id]
230+
: current.openedDialogIds,
224231
}));
225232
}
226233
}

‎src/components/Modal/GlobalModal.tsx‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -126,11 +126,15 @@ export const GlobalModal = ({
126126
maybeClose('escape', event);
127127
};
128128

129-
// Sync open prop → dialog open. Don't close here (dialog ref changes after close → effect loop).
129+
// Sync open prop to dialog state.
130130
// closingRef blocks re-open when we just closed and parent hasn't set open=false yet.
131131
useEffect(() => {
132132
if (!open) {
133133
closingRef.current = false;
134+
if (isOpen) {
135+
dialog.close();
136+
}
137+
return;
134138
}
135139
if (open && !isOpen && !closingRef.current) {
136140
dialog.open();
@@ -162,7 +166,7 @@ export const GlobalModal = ({
162166
inert={isTopmost ? undefined : true}
163167
onKeyDown={handleDialogKeyDown}
164168
role={role}
165-
tabIndex={-1}
169+
tabIndex={isTopmost ? 0 : -1}
166170
>
167171
{children}
168172
</div>

‎src/components/Modal/__tests__/GlobalModal.test.tsx‎

Lines changed: 104 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,54 @@ const renderStackedModals = ({
7575
</ChatProvider>,
7676
);
7777

78+
const RemovableChildModalFixture = () => {
79+
const [showChild, setShowChild] = React.useState(true);
80+
81+
return (
82+
<ChatProvider value={mockChatContext({ theme: 'messaging light' })}>
83+
<ComponentProvider value={{ NotificationList: NoopNotificationList }}>
84+
<ModalDialogManagerProvider>
85+
<GlobalModal aria-label='Parent modal' open>
86+
<ModalContent text='Parent content' />
87+
{showChild && (
88+
<GlobalModal aria-label='Child modal' open role='alertdialog'>
89+
<ModalContent text='Child content'>
90+
<button onClick={() => setShowChild(false)} type='button'>
91+
Remove child modal
92+
</button>
93+
</ModalContent>
94+
</GlobalModal>
95+
)}
96+
</GlobalModal>
97+
</ModalDialogManagerProvider>
98+
</ComponentProvider>
99+
</ChatProvider>
100+
);
101+
};
102+
103+
const CloseChildModalFixture = () => {
104+
const [childOpen, setChildOpen] = React.useState(true);
105+
106+
return (
107+
<ChatProvider value={mockChatContext({ theme: 'messaging light' })}>
108+
<ComponentProvider value={{ NotificationList: NoopNotificationList }}>
109+
<ModalDialogManagerProvider>
110+
<GlobalModal aria-label='Parent modal' open>
111+
<ModalContent text='Parent content' />
112+
<GlobalModal aria-label='Child modal' open={childOpen} role='alertdialog'>
113+
<ModalContent text='Child content'>
114+
<button onClick={() => setChildOpen(false)} type='button'>
115+
Close child modal
116+
</button>
117+
</ModalContent>
118+
</GlobalModal>
119+
</GlobalModal>
120+
</ModalDialogManagerProvider>
121+
</ComponentProvider>
122+
</ChatProvider>
123+
);
124+
};
125+
78126
const OverlayCloseButton = React.forwardRef<
79127
HTMLButtonElement,
80128
React.ComponentProps<'button'>
@@ -302,7 +350,7 @@ describe('GlobalModal', () => {
302350
const dialog = screen.getByRole('dialog');
303351
expect(dialog).toHaveClass('str-chat__modal__dialog');
304352
expect(dialog).toHaveAttribute('aria-modal', 'true');
305-
expect(dialog).toHaveAttribute('tabindex', '-1');
353+
expect(dialog).toHaveAttribute('tabindex', '0');
306354
expect(dialog).toHaveAttribute('aria-labelledby', 'modal-title');
307355
expect(dialog).toHaveAttribute('aria-describedby', 'modal-description');
308356
});
@@ -390,9 +438,11 @@ describe('GlobalModal', () => {
390438
expect(parentModal).toBeInTheDocument();
391439
expect(parentModal).not.toHaveAttribute('aria-modal');
392440
expect(parentModal).toHaveAttribute('inert');
441+
expect(parentModal).toHaveAttribute('tabindex', '-1');
393442
expect(childModal).toBeInTheDocument();
394443
expect(childModal).toHaveAttribute('aria-modal', 'true');
395444
expect(childModal).not.toHaveAttribute('inert');
445+
expect(childModal).toHaveAttribute('tabindex', '0');
396446
});
397447

398448
it('only closes the topmost modal on Escape', () => {
@@ -413,6 +463,59 @@ describe('GlobalModal', () => {
413463
expect(parentOnClose).not.toHaveBeenCalled();
414464
});
415465

466+
it('restores interactivity to the underlying modal after the topmost modal closes', () => {
467+
const childOnClose = vi.fn();
468+
const parentOnClose = vi.fn();
469+
470+
renderStackedModals({ childOnClose, parentOnClose });
471+
472+
const parentModal = screen.getByRole('dialog', { name: 'Parent modal' });
473+
const childModal = screen.getByRole('alertdialog', { name: 'Child modal' });
474+
475+
fireEvent.keyDown(childModal, { key: 'Escape' });
476+
477+
expect(childOnClose).toHaveBeenCalledTimes(1);
478+
expect(parentModal).toHaveAttribute('aria-modal', 'true');
479+
expect(parentModal).not.toHaveAttribute('inert');
480+
expect(parentModal).toHaveAttribute('tabindex', '0');
481+
482+
fireEvent.keyDown(parentModal, { key: 'Escape' });
483+
484+
expect(parentOnClose).toHaveBeenCalledTimes(1);
485+
});
486+
487+
it('restores topmost state to the underlying modal after the topmost modal is removed', () => {
488+
render(<RemovableChildModalFixture />);
489+
490+
const parentModal = screen.getByRole('dialog', { name: 'Parent modal' });
491+
expect(parentModal).toHaveAttribute('tabindex', '-1');
492+
493+
fireEvent.click(screen.getByRole('button', { name: 'Remove child modal' }));
494+
495+
expect(
496+
screen.queryByRole('alertdialog', { name: 'Child modal' }),
497+
).not.toBeInTheDocument();
498+
expect(parentModal).toHaveAttribute('aria-modal', 'true');
499+
expect(parentModal).not.toHaveAttribute('inert');
500+
expect(parentModal).toHaveAttribute('tabindex', '0');
501+
});
502+
503+
it('restores topmost state to the underlying modal after the topmost modal open prop becomes false', () => {
504+
render(<CloseChildModalFixture />);
505+
506+
const parentModal = screen.getByRole('dialog', { name: 'Parent modal' });
507+
expect(parentModal).toHaveAttribute('tabindex', '-1');
508+
509+
fireEvent.click(screen.getByRole('button', { name: 'Close child modal' }));
510+
511+
expect(
512+
screen.queryByRole('alertdialog', { name: 'Child modal' }),
513+
).not.toBeInTheDocument();
514+
expect(parentModal).toHaveAttribute('aria-modal', 'true');
515+
expect(parentModal).not.toHaveAttribute('inert');
516+
expect(parentModal).toHaveAttribute('tabindex', '0');
517+
});
518+
416519
it('forwards alertdialog role when explicitly provided', () => {
417520
renderComponent({
418521
props: {

0 commit comments

Comments
 (0)