Skip to content

fix(toast): resume timers if pointer leaves region due to toast being removed #7771

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 20, 2025

Conversation

reidbarber
Copy link
Member

@reidbarber reidbarber commented Feb 13, 2025

Closes #7024. Alternative to #7681

After a toast has been removed, we check the pointer position on the next pointermove event. If it is now outside the toast region, we resume timers.

For discussion:

  • The timers won't resume until the next pointermove after closing. This behavior seems good IMO. It gives the user time to process the new state and decide whether to move their pointer elsewhere or to the next toast (would cause another pause).
  • There is some related but different behavior we may want to discuss: if you close a toast via touch, currently the next toast's remove button gets focused, causing the timer to pause. If the user scrolls or interacts with anything else outside the region, the timers resume. This behavior seems fine IMO.

Also, note that the original issue is no longer reproducible in RSP Toast after #7631, so I've updated the useToast story for testing this.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

In the useToast story with a timeout enabled, open two or more toasts
Close the top one and keep your mouse where it is
Confirm that the remaining toast(s) close after a few seconds due to the timeout. Previously they would not due to the timer not resuming, due to no pointerleave event being fired in this case.

🧢 Your Project:

@rspbot
Copy link

rspbot commented Feb 13, 2025

@yihuiliao
Copy link
Member

it still seems like when i focus on a toast, it'll still auto dismiss even though the timer should have paused

Screen.Recording.2025-02-14.at.4.32.08.PM.mov

@reidbarber
Copy link
Member Author

@yihuiliao I'm having trouble reproducing this. Is this still easy to reproduce for you?

@yihuiliao
Copy link
Member

so i click on the "low priority" toasts three times, and then i tab to focus on the toast. after that point, i just wait and then it'll auto dismiss

@reidbarber
Copy link
Member Author

toast-timout.mov

That is strange, I still can't reproduce.

@yihuiliao
Copy link
Member

hm okay slightly revised instructions. what if you try opening up three "low priority" toasts, and then dismissing them by clicking the clear button. then opening three more "low priority" toasts, pressing tab, and waiting for the auto dismiss

it seems like when it's the "low priority 1" i can't reproduce the issue, but when it's a different number i can

@reidbarber
Copy link
Member Author

Looks like this is pre-existing. I'll work on a fix in this PR.

Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

small nitpicks

};

if (state.visibleToasts.length < prevToastCount.current && state.visibleToasts.length > 0) {
document.addEventListener('pointermove', onPointerMove);
Copy link
Member

Choose a reason for hiding this comment

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

if we use once, then it simplifies some of the cleanup

Suggested change
document.addEventListener('pointermove', onPointerMove);
document.addEventListener('pointermove', onPointerMove, {once: true});

return () => {
document.removeEventListener('pointermove', onPointerMove);
};
}, [state.visibleToasts, ref, state]);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}, [state.visibleToasts, ref, state]);
}, [state.visibleToasts, state.resumeAll, ref]);

@snowystinger snowystinger added this pull request to the merge queue Feb 20, 2025
Merged via the queue into main with commit 6cd19f0 Feb 20, 2025
33 checks passed
@snowystinger snowystinger deleted the toast-resume-timers-pointer branch February 20, 2025 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple auto-dismiss toasts
5 participants