radix-ui / primitives

Radix Primitives is an open-source UI component library for building high-quality, accessible design systems and web apps. Maintained by @workos.
https://radix-ui.com/primitives
MIT License
14.93k stars 729 forks source link

Toast doesn't dismiss because of pause/resume issue #2233

Open jvzaniolo opened 1 year ago

jvzaniolo commented 1 year ago

Pause/Resume issue on Toast when changing durations

Context

I created a way to handle multiple toasts inspired by Shadcn UI and react-hot-toast.

One of the toasts is the toast.promise which displays a loading, success, or error toast depending on the Promise status. It uses an update function to merge the success and error props with the loading toast (example in the CodeSandbox).

I'm using Infinity as the loading Toast duration so it doesn't close until the Promise finishes.

Issue

The issue happens when the duration is paused (either by focusing on another window, focusing on the Toast itself, or hovering over it)

When I do that, the Toast doesn't close automatically.

I can still swipe and close it or click the <Toast.Close /> button.

Behavior

I noticed that the Toast doesn't call the onOpenChange function when I trigger the pause and resume functions.

Example

Here's a CodeSandbox with the example:

https://codesandbox.io/s/radix-toast-pause-resume-issue-q6ghfn?file=/src/App.tsx

Related issues

jvzaniolo commented 1 year ago

I forked the project and ran the Storybook. This issue also happens in your Promise Toast.

The toast automatically closes if I don't trigger the pause/resume functions, but it doesn't work correctly when I do.

CleanShot 2023-06-29 at 18 39 12

jvzaniolo commented 1 year ago

I think the issue is here:

https://github.com/radix-ui/primitives/blob/eca6babd188df465f64f23f3584738b85dba610e/packages/react/toast/src/Toast.tsx#L491

The value of closeTimerRemainingTimeRef.current doesn't change from Infinity to the new duration when the component rerenders.

And on this line

https://github.com/radix-ui/primitives/blob/eca6babd188df465f64f23f3584738b85dba610e/packages/react/toast/src/Toast.tsx#L521

the Math is actually closeTimerRemainingTimeRef.current = Infinity - elapsedTime;

Resetting the ref's value in a useEffect between rerenders fixes the issue.

I'll submit a PR

paul-sachs commented 12 months ago

I second this, I recently ran into this when trying to create a notification whose duration is dynamic (based on when a promise resolves).

joepetrillo commented 11 months ago

Any updates?

gdfreitas commented 11 months ago

We're also facing this issue. Would anyone kindly give us an status update?

richard-hunter-hpe commented 10 months ago

Tangential but related (not sure if it is worth it's own issue), Infinity can be used as a value for Toast.Root prop duration to essentially disable duration completely, but I don't see it documented. Hopefully any fix to this issue will not alter that behavior.

joepetrillo commented 9 months ago

Any updates?

hongkiulam commented 9 months ago

My workaround in my codebase looks like this

duration={toast.delay === Infinity ? false : toast.delay}

Will need to do ts-ignore in typescript codebases tho

misha-erm commented 9 months ago

+1 for this issue. Currently it stops us from adapting Radix toasts. Some of our toasts have ToastClose with infinite duration and what I noticed is that timer stops after I press and is not resumed for new toasts until pointermove or similar event

It can be easily reproduced on shadcn preview page. Just click "Add to calendar" then click on close icon and click "Add to calendar" again (https://ui.shadcn.com/docs/components/toast)

EandrewJones commented 9 months ago

+1 Here as well. It's these types of bugs that perpetually push my team away from adopting a UI library and back to own our implementations.

We were having problems relying solely on duration=Infinity.

Instead, I found a workaround using useRef instead of useState to manage the open state.

const isOpen = useRef<boolean>(false)
const handleOpen = (open: boolean) => (isOpen.current = open)

[promise-based life-cycle callbacks here]

return <Toast.Root open={isOpen.current} onOpenChange={handleOpen} duration={Infinity} />...</Toast.Root>

Admittedly, the above isn't perfect since often the API call or whatever async function you're waiting on is called from somewhere else and you want to synchronize isOpen.current with its resolution. We're using a pub-sub architecture so it's trivial to subscribe to a toast topic and handle the life cycle callbacks inside the custom toast component.

I realize for normal API calls it may be more convoluted.

misha-erm commented 8 months ago

Okay, seems like I realized the root cause for the issue about stopped timers after clicking on ToastClose

The main reason is that viewport gets focused after you press ToastClose. Once you understand the rationale behind it the behavior becomes quite logical :D But our toast queues are limited to the size of 1, thus we don't have something like Notification Center, and focusing whole viewport after closing seems a bit redundant.

If possible I'd like to either opt-out of this behavior, or switch to per-toast timers rather than one global viewport timer.

Here is my patch that seems to solve the issue for me, I just commented out a call to viewport.focus():

diff --git a/dist/index.mjs b/dist/index.mjs
index 1366d141dd80a8491c9635239950080c1633f8e2..6d902049ae94869bafefbe62af05c2d0100e6083 100644
--- a/dist/index.mjs
+++ b/dist/index.mjs
@@ -343,11 +343,15 @@ const $054eb8030ebde76e$var$ToastImpl = /*#__PURE__*/ $eyrYI$forwardRef((props,
     const closeTimerRef = $eyrYI$useRef(0);
     const { onToastAdd: onToastAdd , onToastRemove: onToastRemove  } = context;
     const handleClose = $eyrYI$useCallbackRef(()=>{
-        var _context$viewport2;
-        // focus viewport if focus is within toast to read the remaining toast
-        // count to SR users and ensure focus isn't lost
-        const isFocusInToast = node1 === null || node1 === void 0 ? void 0 : node1.contains(document.activeElement);
-        if (isFocusInToast) (_context$viewport2 = context.viewport) === null || _context$viewport2 === void 0 || _context$viewport2.focus();
+        // As a workaround for https://github.com/radix-ui/primitives/issues/2268 we do not focus viewport on close
+        // since our toast queue is limited to one toast usually there is nothing else to announce after close 
+        /* 
+            // var _context$viewport2;
+            // focus viewport if focus is within toast to read the remaining toast
+            // count to SR users and ensure focus isn't lost
+            const isFocusInToast = node1 === null || node1 === void 0 ? void 0 : node1.contains(document.activeElement);
+            if (isFocusInToast) (_context$viewport2 = context.viewport) === null || _context$viewport2 === void 0 || _context$viewport2.focus(); 
+        */
         onClose();
     });
     const startTimer = $eyrYI$useCallback((duration)=>{
oliveira05 commented 7 months ago

I've managed to find a solution if the queue is limited only to one toast, and with the help of @MikeYermolayev reply to this issue.

If you've implemented this component - <Toast.Viewport /> - on your Toast Component, such as the example given on Radix's website, give it an ID.

<Toast.Viewport id="toast-vp" />

This is the component that has the element that gets focused in the handleClose callback, present in @MikeYermolayev reply. It is an ordered list <ol tabindex="-1" id="toast-vp"></ol>.

With this, in the onOpenChange prop, we could do as such:

<Toast.Root open={open}
                onOpenChange={(open) => {
                    if (!open)
                        document.getElementById("toast-vp")?.blur();

                }}> {children}</Toast.Root>

That is, we get the element that we gave an ID and we simly remove the focus from it with blur().

I don't know if this the best solution, but it is the simpler I've found. There are probably costs of doing this, but it is working fine for my use case.