From de0f70eefce770d349568a4b6fb5eae47ed66df3 Mon Sep 17 00:00:00 2001 From: Roland Date: Sat, 1 Aug 2026 14:58:21 -0600 Subject: [PATCH] ui: restore the clickable toast the migration dropped No, it did not still work. Semantic's toast took an `onClick` in its options and made the whole toast activate it. The migrated helper destructured four keys and dropped the rest, silently, so every toast that carried one became a dead toast. `Events.js` still passes one -- it did before the migration and was never changed -- for the three events that arrive with a URL: * `user_notify_message`, a page shared from the browser extension * `upload_archive` * `screenshot_generated` The first renders the description "Click here to view the shared page", which it was not. Nothing threw and nothing looked wrong; the toast simply stopped being a way to reach the thing it was announcing. `onClick` is passed through again, and a toast carrying one gets a pointer cursor so it looks like what it is. Clicks on the dismiss button do not follow it -- the button is inside the toast, so its click reaches the root, and closing a notification is not the same as following it. It is also reachable from the keyboard now, which it never was: WROLPi runs on devices with a keyboard and no mouse, and a toast only a pointer can follow is a feature half the deployments cannot use. No `role`, because a `button` or `link` role may not contain interactive content and the dismiss button is in there. Mantine also leaves that dismiss button unnamed, so a screen reader read it as "button". It has a label now, which is how the test for the dismiss-does-not-follow case can find it. Five tests, all red first. The gallery gets a Clickable button; following the toast raises a second one, so it is obvious whether the handler ran. --- app/src/components/ThemeSamplePage.js | 8 +++ app/src/components/ui/toast.test.js | 74 +++++++++++++++++++++++++++ app/src/components/ui/toast.ts | 44 +++++++++++++++- 3 files changed, 124 insertions(+), 2 deletions(-) diff --git a/app/src/components/ThemeSamplePage.js b/app/src/components/ThemeSamplePage.js index 7d2dabc6..39fb8396 100644 --- a/app/src/components/ThemeSamplePage.js +++ b/app/src/components/ThemeSamplePage.js @@ -276,6 +276,14 @@ export function ThemeSamplePage() { + 'keeps/going/for/a/while.html: the remote server closed the ' + 'connection after sending part of the response body.', })}>Long text + {/* Events.js sends three of these, each opening a URL. Click the toast + body, or Tab to it and press Enter; the dismiss button does not follow. */} + diff --git a/app/src/components/ui/toast.test.js b/app/src/components/ui/toast.test.js index 6e94768a..92c04bb1 100644 --- a/app/src/components/ui/toast.test.js +++ b/app/src/components/ui/toast.test.js @@ -1,5 +1,6 @@ import React from 'react'; import {act, render, screen, waitFor} from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; import {MantineProvider} from '@mantine/core'; import {Notifications} from '@mantine/notifications'; import {cssVariablesResolver, mantineTheme} from '../../themes/mantine'; @@ -152,6 +153,79 @@ describe('toast', () => { }); }); +describe('a toast that can be clicked', () => { + /* + * Semantic's toast took an `onClick` in its options and made the whole toast activate it. + * `Events.js` still passes one -- it did before the migration and was never changed -- for + * three events that each open a URL: a shared page pushed from the browser extension, a + * finished archive upload, and a generated screenshot. The migrated helper destructured + * four keys and dropped the rest, so all three became dead toasts. The first of them + * renders the description "Click here to view the shared page", which it was not. + */ + + it('runs the handler when the toast is clicked', async () => { + const onClick = jest.fn(); + withToasts(); + + showToast({title: 'Archive Uploaded', description: 'example.com', onClick}); + await waitFor(() => expect(visibleToasts()).toHaveLength(1)); + await userEvent.click(screen.getByText('Archive Uploaded')); + + expect(onClick).toHaveBeenCalledTimes(1); + }); + + it('does not run the handler when the toast is dismissed', async () => { + // The close button sits inside the toast, so its click reaches the root too. Dismissing + // a notification is not the same as following it, and for these three that difference is + // a page navigation the user did not ask for. + const onClick = jest.fn(); + withToasts(); + + showToast({title: 'Archive Uploaded', onClick}); + await waitFor(() => expect(visibleToasts()).toHaveLength(1)); + await userEvent.click(screen.getByRole('button', {name: 'Dismiss notification'})); + + expect(onClick).not.toHaveBeenCalled(); + }); + + it('can be reached and activated from the keyboard', async () => { + // WROLPi runs on devices with a keyboard and no mouse. A toast that only a pointer can + // follow is a feature half the deployments cannot use. + const onClick = jest.fn(); + withToasts(); + + showToast({title: 'Screenshot Generated', onClick}); + await waitFor(() => expect(visibleToasts()).toHaveLength(1)); + visibleToasts()[0].focus(); + expect(visibleToasts()[0]).toHaveFocus(); + await userEvent.keyboard('{Enter}'); + + expect(onClick).toHaveBeenCalledTimes(1); + }); + + it('looks clickable only when it is', async () => { + withToasts(); + + showToast({title: 'Plain'}); + await waitFor(() => expect(visibleToasts()).toHaveLength(1)); + + const plain = visibleToasts()[0]; + expect(plain.style.cursor).not.toBe('pointer'); + expect(plain).not.toHaveAttribute('tabindex'); + }); + + it('marks a clickable toast as clickable', async () => { + const onClick = jest.fn(); + withToasts(); + + showToast({title: 'Follow me', onClick}); + await waitFor(() => expect(visibleToasts()).toHaveLength(1)); + + expect(visibleToasts()[0].style.cursor).toBe('pointer'); + expect(visibleToasts()[0]).toHaveAttribute('tabindex', '0'); + }); +}); + describe('how long a toast stays up', () => { /* * Real timers, with short durations chosen by the caller. diff --git a/app/src/components/ui/toast.ts b/app/src/components/ui/toast.ts index 195458b0..961b8629 100644 --- a/app/src/components/ui/toast.ts +++ b/app/src/components/ui/toast.ts @@ -1,3 +1,4 @@ +import React from 'react'; import {notifications} from '@mantine/notifications'; /* @@ -16,6 +17,12 @@ export interface ToastOptions { description?: string; /** Milliseconds before auto-dismiss. 0 keeps it until dismissed. */ time?: number; + /** + * Makes the whole toast activate this — Semantic's behaviour, which `Events.js` still + * relies on for the three events that carry a URL: a page shared from the browser + * extension, a finished archive upload, and a generated screenshot. + */ + onClick?: () => void; } const toastColors: Record = { @@ -25,14 +32,47 @@ const toastColors: Record = { error: 'red', }; -export function toast({type = 'info', title, description, time = 5000}: ToastOptions = {}) { +/** + * The props that make a toast followable. + * + * `tabIndex` and the key handler are not decoration: WROLPi runs on devices with a keyboard + * and no mouse, and a toast only a pointer can follow is a feature half the deployments + * cannot use. There is deliberately no `role` — a `button` or `link` role may not contain + * interactive content, and the dismiss button lives inside. + */ +const followable = (onClick: () => void) => ({ + style: {cursor: 'pointer'}, + tabIndex: 0, + onClick: (event: React.MouseEvent) => { + // The dismiss button is inside the toast, so its click reaches here too. Closing a + // notification is not following it, and for these three that would be a navigation + // the user did not ask for. + if ((event.target as HTMLElement).closest('button')) { + return; + } + onClick(); + }, + onKeyDown: (event: React.KeyboardEvent) => { + if (event.key === 'Enter' || event.key === ' ') { + event.preventDefault(); + onClick(); + } + }, +}); + +export function toast({type = 'info', title, description, time = 5000, onClick}: ToastOptions = {}) { notifications.show({ title, message: description, color: toastColors[type], - // Semantic treated 0 as "stay up"; Mantine wants false for that. + // Semantic treated 0 as "stay up"; Mantine wants false for that. Its `getAutoClose` + // returns any number as given and the container then calls `setTimeout(hide, 0)`, so + // passing the 0 through dismisses the toast on the next tick. autoClose: time === 0 ? false : time, withBorder: true, + // Mantine leaves its close button unnamed, which a screen reader reads as "button". + closeButtonProps: {'aria-label': 'Dismiss notification'}, + ...(onClick ? followable(onClick) : {}), }); }