From e14f7bb01d016ac07de8b96237f028d4e0820607 Mon Sep 17 00:00:00 2001 From: Michael Jordan Date: Wed, 20 Mar 2024 12:56:30 -0400 Subject: [PATCH] refactor with suggestions from code review 1. Use role="alertdialog" for each Toast. 2. useToast returns contentProps with role="alert" for container around the icon and the title 3. Fix live region announcement when an Action button is present, by adding aria-relevant="additions" and aria-atomic="true" to contentProps returned by useToast. 4. ToastContainer now renders using an ordered list in reverse order. 5. Fix focus ring rendering around Toast. 6. Fix focus management when the last item in the list is removed, by adding `from` and `accept` to the focusManagerf.ocusPrevious/Next method calls. --- packages/@react-aria/toast/src/useToast.ts | 17 +++-- .../@react-aria/toast/test/useToast.test.js | 5 +- packages/@react-spectrum/toast/package.json | 1 + packages/@react-spectrum/toast/src/Toast.tsx | 51 +++++++++------ .../toast/src/ToastContainer.tsx | 21 ++++-- .../toast/src/toastContainer.css | 20 ++++++ .../toast/test/ToastContainer.test.js | 64 +++++++++++-------- 7 files changed, 122 insertions(+), 57 deletions(-) diff --git a/packages/@react-aria/toast/src/useToast.ts b/packages/@react-aria/toast/src/useToast.ts index a3e79bfbb94..b237c87f2e7 100644 --- a/packages/@react-aria/toast/src/useToast.ts +++ b/packages/@react-aria/toast/src/useToast.ts @@ -26,8 +26,10 @@ export interface AriaToastProps extends AriaLabelingProps { } export interface ToastAria { - /** Props for the toast container element. */ + /** Props for the toast container, non-modal dialog element. */ toastProps: DOMAttributes, + /** Props for the toast content alert message. */ + contentProps: DOMAttributes, /** Props for the toast title element. */ titleProps: DOMAttributes, /** Props for the toast description element, if any. */ @@ -94,16 +96,18 @@ export function useToast(props: AriaToastProps, state: ToastState, ref: // Only move focus when there is another Toast. At this point, // state.visibleToasts still includes Toast being removed. if (state.visibleToasts?.length > 1) { - let nextItemFocused = focusManager.focusNext(); + const from = document.activeElement?.closest('[role="alertdialog"]') || document.activeElement; + const accept = (node:Element) => node.getAttribute('role') === 'alertdialog'; + let nextItemFocused = focusManager.focusNext({from, accept}); if (!nextItemFocused || Object.keys(nextItemFocused).length === 0) { - focusManager.focusPrevious(); + focusManager.focusPrevious({from, accept}); } } }; return { toastProps: { - role: 'alert', + role: 'alertdialog', 'aria-label': props['aria-label'], 'aria-labelledby': props['aria-labelledby'] || titleId, 'aria-describedby': props['aria-describedby'] || descriptionId, @@ -112,6 +116,11 @@ export function useToast(props: AriaToastProps, state: ToastState, ref: 'aria-hidden': animation === 'exiting' ? 'true' : undefined, tabIndex: 0 }, + contentProps: { + role: 'alert', + 'aria-atomic': 'true', + 'aria-relevant': 'additions' + }, titleProps: { id: titleId }, diff --git a/packages/@react-aria/toast/test/useToast.test.js b/packages/@react-aria/toast/test/useToast.test.js index 81ade7cdba9..1370cbfe889 100644 --- a/packages/@react-aria/toast/test/useToast.test.js +++ b/packages/@react-aria/toast/test/useToast.test.js @@ -27,9 +27,10 @@ describe('useToast', () => { }; it('handles defaults', function () { - let {closeButtonProps, toastProps, titleProps} = renderToastHook({}, {close}); + let {closeButtonProps, toastProps, contentProps, titleProps} = renderToastHook({}, {close}); - expect(toastProps.role).toBe('alert'); + expect(toastProps.role).toBe('alertdialog'); + expect(contentProps.role).toBe('alert'); expect(closeButtonProps['aria-label']).toBe('Close'); expect(typeof closeButtonProps.onPress).toBe('function'); expect(titleProps.id).toEqual(toastProps['aria-labelledby']); diff --git a/packages/@react-spectrum/toast/package.json b/packages/@react-spectrum/toast/package.json index 78fc2380f1c..1bcc62efd61 100644 --- a/packages/@react-spectrum/toast/package.json +++ b/packages/@react-spectrum/toast/package.json @@ -46,6 +46,7 @@ "@react-types/shared": "^3.22.1", "@spectrum-icons/ui": "^3.6.5", "@swc/helpers": "^0.5.0", + "react-aria": "^3.32.1", "use-sync-external-store": "^1.2.0" }, "devDependencies": { diff --git a/packages/@react-spectrum/toast/src/Toast.tsx b/packages/@react-spectrum/toast/src/Toast.tsx index cb789bdc3d3..878902cdb0d 100644 --- a/packages/@react-spectrum/toast/src/Toast.tsx +++ b/packages/@react-spectrum/toast/src/Toast.tsx @@ -19,11 +19,12 @@ import InfoMedium from '@spectrum-icons/ui/InfoMedium'; // @ts-ignore import intlMessages from '../intl/*.json'; import {QueuedToast, ToastState} from '@react-stately/toast'; -import React, {useContext} from 'react'; +import React from 'react'; import styles from '@adobe/spectrum-css-temp/components/toast/vars.css'; import SuccessMedium from '@spectrum-icons/ui/SuccessMedium'; import toastContainerStyles from './toastContainer.css'; -import {ToasterContext} from './Toaster'; +import {useFocusRing} from '@react-aria/focus'; +import {useId} from '@react-aria/utils'; import {useLocalizedStringFormatter} from '@react-aria/i18n'; import {useToast} from '@react-aria/toast'; @@ -66,6 +67,7 @@ function Toast(props: SpectrumToastProps, ref: DOMRef) { let domRef = useDOMRef(ref); let { closeButtonProps, + contentProps, titleProps, toastProps } = useToast(props, state, domRef); @@ -74,7 +76,7 @@ function Toast(props: SpectrumToastProps, ref: DOMRef) { let stringFormatter = useLocalizedStringFormatter(intlMessages, '@react-spectrum/toast'); let iconLabel = variant && variant !== 'neutral' ? stringFormatter.format(variant) : null; let Icon = ICONS[variant]; - let isFocusVisible = useContext(ToasterContext); + let {isFocusVisible, focusProps} = useFocusRing(); const handleAction = () => { if (onAction) { @@ -86,10 +88,16 @@ function Toast(props: SpectrumToastProps, ref: DOMRef) { } }; + const iconId = useId(); + if (Icon) { + toastProps['aria-labelledby'] = `${iconId} ${titleProps.id}`; + } + return (
) { classNames( toastContainerStyles, 'spectrum-Toast', - {'focus-ring': props.toast.key === state.visibleToasts[0]?.key && isFocusVisible} + {'focus-ring': isFocusVisible} ) )} style={{ @@ -111,24 +119,27 @@ function Toast(props: SpectrumToastProps, ref: DOMRef) { state.remove(key); } }}> - {Icon && - - } -
-
{children}
- {actionLabel && - +
+ {Icon && + } +
+
{children}
+ {actionLabel && + + } +
-
+
diff --git a/packages/@react-spectrum/toast/src/ToastContainer.tsx b/packages/@react-spectrum/toast/src/ToastContainer.tsx index 5b2d17dfd73..48252dc028f 100644 --- a/packages/@react-spectrum/toast/src/ToastContainer.tsx +++ b/packages/@react-spectrum/toast/src/ToastContainer.tsx @@ -11,8 +11,10 @@ */ import {AriaToastRegionProps} from '@react-aria/toast'; +import {classNames} from '@react-spectrum/utils'; import React, {ReactElement, useEffect, useRef} from 'react'; import {SpectrumToastValue, Toast} from './Toast'; +import toastContainerStyles from './toastContainer.css'; import {Toaster} from './Toaster'; import {ToastOptions, ToastQueue, useToastQueue} from '@react-stately/toast'; import {useSyncExternalStore} from 'use-sync-external-store/shim/index.js'; @@ -106,12 +108,19 @@ export function ToastContainer(props: SpectrumToastContainerProps): ReactElement if (ref === activeToastContainer && state.visibleToasts.length > 0) { return ( - {state.visibleToasts.map((toast) => ( - - ))} +
    + {state.visibleToasts.map((toast) => ( +
  1. + +
    2. + ))} +
); } diff --git a/packages/@react-spectrum/toast/src/toastContainer.css b/packages/@react-spectrum/toast/src/toastContainer.css index e0a82e27079..1c0ed8d5487 100644 --- a/packages/@react-spectrum/toast/src/toastContainer.css +++ b/packages/@react-spectrum/toast/src/toastContainer.css @@ -72,6 +72,9 @@ --spectrum-focus-ring-gap: var(--spectrum-alias-focus-ring-gap); --spectrum-focus-ring-size: var(--spectrum-alias-focus-ring-size); + position: relative; + outline: none; + &[data-animation=entering] { animation: slide-in 300ms; } @@ -80,6 +83,23 @@ animation: fade-out 300ms forwards; } } +.spectrum-Toast-list { + display: contents; + list-style-type: none; + margin-block-start: 0; + margin-block-end: 0; + margin-inline-start: 0; + margin-inline-end: 0; + padding-inline-start: 0; +} + +.spectrum-Toast-listitem { + display: inline-block; +} + +.spectrum-Toast-contentWrapper { + display: contents; +} @keyframes slide-in { from { diff --git a/packages/@react-spectrum/toast/test/ToastContainer.test.js b/packages/@react-spectrum/toast/test/ToastContainer.test.js index b9bfd2d74da..bed3124a025 100644 --- a/packages/@react-spectrum/toast/test/ToastContainer.test.js +++ b/packages/@react-spectrum/toast/test/ToastContainer.test.js @@ -64,20 +64,24 @@ describe('Toast Provider and Container', function () { let {getByRole, queryByRole} = renderComponent(); let button = getByRole('button'); + expect(queryByRole('alertdialog')).toBeNull(); expect(queryByRole('alert')).toBeNull(); triggerPress(button); let region = getByRole('region'); expect(region).toHaveAttribute('aria-label', 'Notifications'); - let alert = getByRole('alert'); + let toast = getByRole('alertdialog'); + let alert = within(toast).getByRole('alert'); + expect(toast).toBeVisible(); expect(alert).toBeVisible(); - button = within(alert).getByRole('button'); + button = within(toast).getByRole('button'); expect(button).toHaveAttribute('aria-label', 'Close'); triggerPress(button); fireAnimationEnd(alert); + expect(queryByRole('alertdialog')).toBeNull(); expect(queryByRole('alert')).toBeNull(); }); @@ -86,9 +90,12 @@ describe('Toast Provider and Container', function () { let button = getByRole('button'); triggerPress(button); - let alert = getByRole('alert'); + let toast = getByRole('alertdialog'); + let alert = within(toast).getByRole('alert'); let icon = within(alert).getByRole('img'); expect(icon).toHaveAttribute('aria-label', 'Success'); + let title = within(alert).getByText('Toast is default'); + expect(toast).toHaveAttribute('aria-labelledby', `${icon.id} ${title.id}`); }); it('removes a toast via timeout', () => { @@ -97,7 +104,7 @@ describe('Toast Provider and Container', function () { triggerPress(button); - let toast = getByRole('alert'); + let toast = getByRole('alertdialog'); expect(toast).toBeVisible(); act(() => jest.advanceTimersByTime(1000)); @@ -107,7 +114,7 @@ describe('Toast Provider and Container', function () { expect(toast).toHaveAttribute('data-animation', 'exiting'); fireAnimationEnd(toast); - expect(queryByRole('alert')).toBeNull(); + expect(queryByRole('alertdialog')).toBeNull(); }); it('pauses timers when hovering', async () => { @@ -116,7 +123,7 @@ describe('Toast Provider and Container', function () { triggerPress(button); - let toast = getByRole('alert'); + let toast = getByRole('alertdialog'); expect(toast).toBeVisible(); act(() => jest.advanceTimersByTime(1000)); @@ -131,7 +138,7 @@ describe('Toast Provider and Container', function () { expect(toast).toHaveAttribute('data-animation', 'exiting'); fireAnimationEnd(toast); - expect(queryByRole('alert')).toBeNull(); + expect(queryByRole('alertdialog')).toBeNull(); }); it('pauses timers when focusing', () => { @@ -140,7 +147,7 @@ describe('Toast Provider and Container', function () { triggerPress(button); - let toast = getByRole('alert'); + let toast = getByRole('alertdialog'); expect(toast).toBeVisible(); act(() => jest.advanceTimersByTime(1000)); @@ -155,7 +162,7 @@ describe('Toast Provider and Container', function () { expect(toast).toHaveAttribute('data-animation', 'exiting'); fireAnimationEnd(toast); - expect(queryByRole('alert')).toBeNull(); + expect(queryByRole('alertdialog')).toBeNull(); }); it('renders a toast with an action', () => { @@ -164,13 +171,15 @@ describe('Toast Provider and Container', function () { let {getByRole, queryByRole} = renderComponent(); let button = getByRole('button'); - expect(queryByRole('alert')).toBeNull(); + expect(queryByRole('alertdialog')).toBeNull(); triggerPress(button); - let alert = getByRole('alert'); + let toast = getByRole('alertdialog'); + let alert = within(toast).getByRole('alert'); + expect(toast).toBeVisible(); expect(alert).toBeVisible(); - let buttons = within(alert).getAllByRole('button'); + let buttons = within(toast).getAllByRole('button'); expect(buttons[0]).toHaveTextContent('Action'); triggerPress(buttons[0]); @@ -184,22 +193,24 @@ describe('Toast Provider and Container', function () { let {getByRole, queryByRole} = renderComponent(); let button = getByRole('button'); - expect(queryByRole('alert')).toBeNull(); + expect(queryByRole('alertdialog')).toBeNull(); triggerPress(button); - let alert = getByRole('alert'); + let toast = getByRole('alertdialog'); + let alert = within(toast).getByRole('alert'); + expect(toast).toBeVisible(); expect(alert).toBeVisible(); - let buttons = within(alert).getAllByRole('button'); + let buttons = within(toast).getAllByRole('button'); expect(buttons[0]).toHaveTextContent('Action'); triggerPress(buttons[0]); expect(onAction).toHaveBeenCalledTimes(1); expect(onClose).toHaveBeenCalledTimes(1); - expect(alert).toHaveAttribute('data-animation', 'exiting'); - fireAnimationEnd(alert); - expect(queryByRole('alert')).toBeNull(); + expect(toast).toHaveAttribute('data-animation', 'exiting'); + fireAnimationEnd(toast); + expect(queryByRole('alertdialog')).toBeNull(); }); it('can focus toast region using F6', () => { @@ -208,7 +219,7 @@ describe('Toast Provider and Container', function () { triggerPress(button); - let toast = getByRole('alert'); + let toast = getByRole('alertdialog'); expect(toast).toBeVisible(); expect(document.activeElement).toBe(button); @@ -225,13 +236,13 @@ describe('Toast Provider and Container', function () { triggerPress(button); - let toast = getByRole('alert'); + let toast = getByRole('alertdialog'); let closeButton = within(toast).getByRole('button'); act(() => closeButton.focus()); triggerPress(closeButton); fireAnimationEnd(toast); - expect(queryByRole('alert')).toBeNull(); + expect(queryByRole('alertdialog')).toBeNull(); expect(document.activeElement).toBe(button); }); @@ -242,19 +253,19 @@ describe('Toast Provider and Container', function () { triggerPress(button); triggerPress(button); - let toast = getAllByRole('alert')[0]; + let toast = getAllByRole('alertdialog')[0]; let closeButton = within(toast).getByRole('button'); triggerPress(closeButton); fireAnimationEnd(toast); - toast = getByRole('alert'); + toast = getByRole('alertdialog'); expect(document.activeElement).toBe(toast); closeButton = within(toast).getByRole('button'); triggerPress(closeButton); fireAnimationEnd(toast); - expect(queryByRole('alert')).toBeNull(); + expect(queryByRole('alertdialog')).toBeNull(); expect(document.activeElement).toBe(button); }); @@ -283,11 +294,14 @@ describe('Toast Provider and Container', function () { triggerPress(button); - let toast = getByRole('alert'); + let toast = getByRole('alertdialog'); + let alert = within(toast).getByRole('alert'); expect(toast).toBeVisible(); + expect(alert).toBeVisible(); triggerPress(button); fireAnimationEnd(toast); + expect(queryByRole('alertdialog')).toBeNull(); expect(queryByRole('alert')).toBeNull(); });