Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/toast-dismiss-without-row.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@astryxdesign/core': patch
---

[fix] A toast dismissed while it had no row on screen — hidden past `maxVisible`, or dismissed before it first rendered — now leaves the stack. Before, it stayed as an empty, invisible row that took a visible slot when it came back into view and kept the "Notifications" landmark (and its F6 stop) present after every real toast was gone.

@vjeux
137 changes: 137 additions & 0 deletions packages/core/src/Toast/ToastViewport.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,143 @@ describe('ToastViewport visible limit', () => {
});
});

describe('dismissing a toast that has no mounted row', () => {
function ShowAndKeepDismiss({
options,
label,
onShown,
}: {
options: ToastOptions;
label: string;
onShown: (dismiss: () => void) => void;
}) {
const toast = useToast();
return (
<button type="button" onClick={() => onShown(toast(options))}>
{label}
</button>
);
}

it('reports a hidden toast once when it is dismissed twice before the viewport renders again', () => {
const onHideA = vi.fn();
let dismissA: (() => void) | null = null;
render(
<ToastViewport isTopLayer={false} maxVisible={1}>
<ShowAndKeepDismiss
options={{body: 'Toast A', isAutoHide: false, onHide: onHideA}}
label="Show A"
onShown={dismiss => {
dismissA = dismiss;
}}
/>
<ShowToastButton
options={{body: 'Toast B', isAutoHide: false}}
triggerLabel="Show B"
/>
</ToastViewport>,
);
act(() => {
fireEvent.click(screen.getByText('Show A'));
fireEvent.click(screen.getByText('Show B'));
});
expect(screen.queryByText('Toast A')).toBeNull();

// Both calls land in one batch, before the list renders without A.
act(() => {
dismissA?.();
dismissA?.();
});
expect(onHideA).toHaveBeenCalledTimes(1);
});

it('frees the slot of a toast dismissed while it was hidden past maxVisible', () => {
const onHideA = vi.fn();
let dismissA: (() => void) | null = null;
render(
<ToastViewport isTopLayer={false} maxVisible={2}>
<ShowAndKeepDismiss
options={{body: 'Toast A', isAutoHide: false, onHide: onHideA}}
label="Show A"
onShown={dismiss => {
dismissA = dismiss;
}}
/>
<ShowToastButton
options={{body: 'Toast B', isAutoHide: false}}
triggerLabel="Show B"
/>
<ShowToastButton
options={{body: 'Toast C', isAutoHide: false}}
triggerLabel="Show C"
/>
</ToastViewport>,
);
act(() => {
fireEvent.click(screen.getByText('Show A'));
fireEvent.click(screen.getByText('Show B'));
fireEvent.click(screen.getByText('Show C'));
});
// Only the newest two are shown: A is hidden past the limit.
expect(screen.queryByText('Toast A')).toBeNull();

act(() => {
dismissA?.();
});
expect(onHideA).toHaveBeenCalledTimes(1);
expect(onHideA).toHaveBeenCalledWith('manual');

// C leaves. A was dismissed, so it must not come back to hold the slot
// as an empty row: B is the only toast left.
const toastC = getToastWrapperByText('Toast C');
act(() => {
fireEvent.click(
within(toastC).getByRole('button', {name: 'Dismiss notification'}),
);
});
act(() => {
completeExit(toastC.dataset.toastId ?? '');
});
expect(document.querySelectorAll('[data-toast-id]')).toHaveLength(1);
expect(screen.getByText('Toast B')).toBeInTheDocument();

// Once B leaves too, nothing is left: no row, no empty landmark.
const toastB = getToastWrapperByText('Toast B');
act(() => {
fireEvent.click(
within(toastB).getByRole('button', {name: 'Dismiss notification'}),
);
});
act(() => {
completeExit(toastB.dataset.toastId ?? '');
});
expect(document.querySelectorAll('[data-toast-id]')).toHaveLength(0);
expect(screen.queryByRole('region', {name: 'Notifications'})).toBeNull();
expect(onHideA).toHaveBeenCalledTimes(1);
});

it('never shows a toast dismissed before it rendered, and reports it once', () => {
const onHide = vi.fn();
render(
<ToastViewport isTopLayer={false}>
<ShowAndKeepDismiss
options={{body: 'Toast gone', isAutoHide: false, onHide}}
label="Show and dismiss"
onShown={dismiss => dismiss()}
/>
</ToastViewport>,
);
act(() => {
fireEvent.click(screen.getByText('Show and dismiss'));
});

expect(document.querySelectorAll('[data-toast-id]')).toHaveLength(0);
expect(screen.queryByRole('region', {name: 'Notifications'})).toBeNull();
expect(onHide).toHaveBeenCalledTimes(1);
expect(onHide).toHaveBeenCalledWith('manual');
});
});

describe('ToastViewport keyboard reach + focus', () => {
it('F6 moves focus into the newest toast', () => {
renderViewport(
Expand Down
40 changes: 36 additions & 4 deletions packages/core/src/Toast/ToastViewport.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,9 @@ export function ToastViewport({
// Toast ids whose exit has begun — guards onHide from double-firing (see
// removeToast). Mirrors exitingIds state, readable synchronously.
const exitingIdsRef = useRef<Set<string>>(new Set());
// Toasts dispatched since the last commit, by id: a dismiss() called
// before the toast first renders still finds it (and its onHide).
Comment on lines +351 to +352

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional, non-blocking: with the commit-time effect removed, nothing prunes this map at commit. An entry now leaves only through the uniqueID loop in addToast, the no-row branch in removeToast, or handleExited, so the map holds every live toast, committed or not. Something like // Every toast dispatched and not yet removed, by id: a dismiss() called before the toast first renders still finds it (and its onHide). would match the code.

const pendingEntriesRef = useRef<Map<string, ToastEntry>>(new Map());
// The element that was focused before the user jumped into the viewport
// (via F6). Used to restore focus once all toasts are dismissed so focus
// never falls to <body>.
Expand Down Expand Up @@ -423,6 +426,16 @@ export function ToastViewport({
// else to the polite region (role="status") — mirrors Toast.tsx.
announce(text, entry.options.type === 'error' ? 'assertive' : 'polite');
}
const pending = pendingEntriesRef.current;
if (uniqueID) {
// An overwrite replaces a toast that has not rendered yet too.
for (const [pendingId, pendingEntry] of pending) {
if (pendingEntry.options.uniqueID === uniqueID) {
pending.delete(pendingId);
}
}
}
pending.set(entry.id, entry);
// An overwrite swaps a new entry, with a new id, into the replaced
// toast's place, so the old row leaves the DOM without ever being
// dismissed. Its settled id is dropped by the prune below, which covers
Expand All @@ -449,11 +462,15 @@ export function ToastViewport({
if (exitingIdsRef.current.has(id)) {
return;
}
exitingIdsRef.current.add(id);
const entry = toastsRef.current.find(t => t.id === id);
if (entry) {
entry.options.onHide?.(reason);
const entry =
toastsRef.current.find(t => t.id === id) ??
pendingEntriesRef.current.get(id);
if (entry == null) {
// Already gone: nothing to hide or report.
return;
}
exitingIdsRef.current.add(id);
entry.options.onHide?.(reason);
// If focus currently lives inside the toast being dismissed, remember
// that its removal must hand focus off to a neighbor (or the element
// focused before the user entered the viewport) rather than <body>.
Expand All @@ -480,6 +497,20 @@ export function ToastViewport({
pendingFocusRef.current = 'restore';
}
}
if (dismissedNode == null) {
// No row is mounted for it: it is hidden past `maxVisible`, or it has
// not rendered yet. Nothing animates out, so it leaves the list now.
// Waiting for its row to finish exiting would keep it forever: when it
// surfaced, its row would mount already collapsed, with no transition
// left to end, and hold a visible slot as an empty row.
// It leaves the committed list now too, so a second dismissal before
// the next render finds nothing to report.
exitingIdsRef.current.delete(id);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Releasing the guard here while toastsRef.current still lists the toast (it is only reassigned on the next render) lets a second dismiss() in the same batch pass the check at line 462 and call onHide again. With maxVisible={1} and two dismissA() calls in one act(), onHide runs twice on this branch and once on main. Dropping the entry from toastsRef.current here, or keeping the id in exitingIdsRef until the list commits, fixes it; a regression test for the double call would pin it.

pendingEntriesRef.current.delete(id);
toastsRef.current = toastsRef.current.filter(t => t.id !== id);
setToasts(prev => prev.filter(t => t.id !== id));
return;
}
setExitingIds(prev => {
if (prev.has(id)) {
return prev;
Expand All @@ -490,6 +521,7 @@ export function ToastViewport({

const handleExited = useCallback((id: string) => {
exitingIdsRef.current.delete(id);
pendingEntriesRef.current.delete(id);
setExitingIds(withoutId(id));
setToasts(prev => prev.filter(t => t.id !== id));
}, []);
Expand Down
Loading