Toast focus management take 2 #6223

snowystinger · 2024-04-17T20:43:21Z

Closes

In association with #6011

There are still some known issues, they are listed in our GA ticket https://github.com/orgs/adobe/projects/19/views/4?pane=issue&itemId=55618129

✅ 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:

Test instructions in #6011 description

🧢 Your Project:

packages/@react-aria/toast/src/useToastRegion.ts

rspbot · 2024-04-23T13:59:55Z

Build successful! 🎉

rspbot · 2024-04-29T14:58:52Z

Build successful! 🎉

rspbot · 2024-04-30T15:07:17Z

Build successful! 🎉

rspbot · 2024-05-02T19:55:01Z

Build successful! 🎉

rspbot · 2024-05-03T01:16:26Z

Build successful! 🎉

rspbot · 2024-05-03T18:35:32Z

Build successful! 🎉

…into toast-focus-take-2

rspbot · 2024-05-08T07:01:34Z

Build successful! 🎉

majornista · 2024-05-09T13:43:23Z

@snowystinger With recent changes, focus is getting lost to the document.body when the top toast, the last item in the list gets removed and other toasts remain.

rspbot · 2024-05-09T15:32:06Z

Build successful! 🎉

rspbot · 2024-05-20T08:09:18Z

Build successful! 🎉

rspbot · 2024-05-20T08:55:30Z

Build successful! 🎉

rspbot · 2024-05-20T23:24:33Z

Build successful! 🎉

rspbot · 2024-05-21T00:57:42Z

Build successful! 🎉

majornista

I tested this change using NVDA in Firefox, Chrome and Edge, and the Toast messages are being announced.

rspbot · 2024-05-21T20:15:27Z

Build successful! 🎉

rspbot · 2024-05-22T23:02:42Z

Build successful! 🎉

LFDanLu · 2024-05-23T23:08:11Z

packages/@react-aria/toast/src/useToast.ts


  let titleId = useId();
  let descriptionId = useSlotId();
  let stringFormatter = useLocalizedStringFormatter(intlMessages, '@react-aria/toast');

  return {
    toastProps: {
-      role: 'alert',
+      role: 'alertdialog',
+      'aria-modal': 'false',


non-action, just interested that this is specifically set to false. I'm just used to omitting the attribute but I'm guessing this explicit false made a difference in the screen reader behavior?

@majornista correct me if I'm wrong, but this is just to possibly future proof? since alertdialog would be considered a modal by default, and so we use aria-modal to communicate that it's not, in fact, a modal

aria-modal="false" doesn't really make a difference for screen reader behavior; the default is for a dialog to be non-modal. However, the APG design pattern describes an alertdialog as modal, so I thought it best to be explicit:

An alert dialog is a modal dialog that interrupts the user's workflow to communicate an important message and acquire a response. Examples include action confirmation prompts and error message confirmations. The alertdialog role enables assistive technologies and browsers to distinguish alert dialogs from other dialogs so they have the option of giving alert dialogs special treatment, such as playing a system alert sound.

I use role="alertdialog", as a container for the toast message content, which has role="alert" to announce as a live region, and the button to dismiss the Toast, for a few reasons.

A Toast is kind of a tiny non-modal dialog, so it didn't seem like much of a stretch.

Using a dialog as a container helps disambiguate between the Dismiss buttons when more than one Toast is displayed.

When we close a Toast, unlike the Dismiss button for an adjacent Toast, the dialog provides a uniquely labeled widget element to receive focus, and is easy to style using the React-Spectrum focus ring.

Depending on the assistive technology being used, an alert dialog will open with an earcon, which I see as nice feature to distinguish a Toast from other content the screen reader may be announcing.

LFDanLu · 2024-05-23T23:10:34Z

packages/@react-aria/toast/src/useToast.ts

@@ -39,6 +41,7 @@ export interface ToastAria {
 * Provides the behavior and accessibility implementation for a toast component.
 * Toasts display brief, temporary notifications of actions, errors, or other events in an application.
 */
+// eslint-disable-next-line @typescript-eslint/no-unused-vars


Can we just remove the ref from the args even though it would be breaking? It is still in beta so might be ok?

it's more of a, we've needed to add it back in later in other cases. I'd prefer to know we'll have access to it

LFDanLu · 2024-05-23T23:30:50Z

packages/@react-spectrum/text/src/Text.tsx

@@ -26,7 +26,7 @@ function Text(props: TextProps, ref: DOMRef) {
  let domRef = useDOMRef(ref);

  return (
-    <span {...filterDOMProps(otherProps)} {...styleProps} ref={domRef}>
+    <span role="none" {...filterDOMProps(otherProps)} {...styleProps} ref={domRef}>


I see this was added to prevent double announcements, but where do we use Text in toasts?

Ah I see, the Button for the toast action. Will have to double check everywhere we use Text to see if there are any changes to the screen reader performance

yeah, i haven't noticed anything yet

Yeah, this was something I noticed in testing with VoiceOver. When a Toast included an action button, the button text was announced twice. This has to do with how live regions work, announcing both element "additions" and "text." VoiceOver seem to interpret the Text component's span element child of the Button as an addition, and announces the text more than once.

LFDanLu · 2024-05-24T00:06:03Z

packages/@react-aria/toast/src/useToastRegion.ts

+    if (removedToastIndex > -1) {
+      let i = 0;
+      let nextToast;
+      let prevToast;
+      while (i <= removedToastIndex) {
+        if (!removedToasts[i].isRemoved) {
+          prevToast = Math.max(0, i - 1);
+        }
+        i++;
+      }
+      while (i < removedToasts.length) {
+        if (!removedToasts[i].isRemoved) {
+          nextToast = i - 1;
+          break;
+        }
+        i++;
+      }


bit of a nit, but I found this a bit hard to follow, could we instead just slice the removedToasts list into two (one going from the 0 -> removedToastIndex and the other going from removedToastIndex -> end) and use findLastIndex with the same !removedToasts.isRemoved condition on the first slice to get the index of the prevToast and findIndex + the removedToastIndex on the second slice to get the nextToast index?

Thinking on this, it might just be the "removedToasts" naming that is tripping me up since that list contains all the toasts but just adds extra data to mark the removed ones.

yeah, it was hard to write too
I've clarified with renames and some more comments

rspbot · 2024-05-24T02:51:13Z

Build successful! 🎉

rspbot · 2024-05-24T02:54:04Z

Build successful! 🎉

LFDanLu

LGTM

devongovett

Do we need to update the aria docs example?

devongovett · 2024-05-30T03:13:21Z

packages/@react-aria/toast/src/useToast.ts

  toastProps: DOMAttributes,
+  /** Props for the toast content alert message. */
+  contentProps: DOMAttributes,


Do we still need both contentProps and titleProps as separate elements or are they the same thing now?

yeah, it still provides a useful distinction for the aria-labelledby. You can put things like the icon on the outside of the title so it isn't read off all the time

Is the new contentProps required to be used? The documentation isn't really clear about this. I currently don't have a wrapper element around my toast title and description as they can be arranged differently in layout with CSS Grid.

It probably depends a bit on how complex your layout is. It can be helpful to target the live announcer to a more succinct announcement. So I'd say good practice, but possibly not strictly necessary. You'll have to test it out with a screen reader to see if not including it is sufficient.

@snowystinger I see. Thanks for the response!

@levrik and @snowystinger The new contentProps includes the role="alert" attribute, which prompts live region announcement when the Toast appears, so I would say these props are necessary.

If need be, I believe you can spread both titleProps and contentProps on the same element.

<div {...toastProps} ref={ref} style={{margin: 20, display: 'flex', gap: 5}}> <div {...mergeProps(titleProps, contentProps)}>{props.toast.content}</div> <button {...buttonProps} ref={buttonRef}>x</button> </div>

devongovett · 2024-05-30T03:16:14Z

packages/@react-aria/toast/src/useToastRegion.ts

+    }
+
+    prevVisibleToasts.current = state.visibleToasts;
+  }, [state.visibleToasts, ref]);


This logic is quite similar to what we have for other collection components when removing rows. Wonder if we could centralize this at some point.

snowystinger · 2024-05-30T05:36:41Z

Yeah, we should update the hooks docs, followup https://github.com/orgs/adobe/projects/19/views/4?pane=issue&itemId=65286699
Starting branch here: #6468

rspbot · 2024-05-30T05:48:01Z

Build successful! 🎉

rspbot · 2024-05-30T05:48:02Z

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

@react-aria/toast

ToastAria

 ToastAria {
   closeButtonProps: AriaButtonProps
+  contentProps: DOMAttributes
   descriptionProps: DOMAttributes
   titleProps: DOMAttributes
   toastProps: DOMAttributes
 }

it changed:

useToast

Toast focus management

a685b76

majornista reviewed Apr 19, 2024

View reviewed changes

packages/@react-aria/toast/src/useToastRegion.ts Show resolved Hide resolved

Merge branch 'main' into toast-focus-take-2

db1f278

majornista added 2 commits April 26, 2024 12:52

Merge branch 'main' into toast-focus-take-2

5d658cb

lint fix: remove duplicated import

5c8bf5c

Merge branch 'main' into toast-focus-take-2

744bfb4

Merge branch 'main' into toast-focus-take-2

aae7f0f

snowystinger added 3 commits May 2, 2024 15:57

Merge branch 'main' into toast-focus-take-2

6d52a1b

prevent a double announcement

b78fd33

fix test

3f0ab0c

snowystinger marked this pull request as ready for review May 3, 2024 18:23

Merge branch 'main' into toast-focus-take-2

8a2eb3f

snowystinger added 6 commits May 3, 2024 15:41

Merge branch 'main' into toast-focus-take-2

f718eaf

Correct the tab order

3d38fdd

Merge branch 'toast-focus-take-2' of github.com:adobe/react-spectrum …

344d3e0

…into toast-focus-take-2

cleanup

abca7f9

Merge branch 'main' into toast-focus-take-2

731f371

fix logic

2992e55

fix logic when removing first or last Toast

a23f010

snowystinger added 2 commits May 10, 2024 12:19

Merge branch 'main' into toast-focus-take-2

a62255b

add test

d619d72

Merge branch 'main' into toast-focus-take-2

66ff206

make tests more realistic, fix already rendered

1752bf3

Always show toasts contents when switching toast containers

60692de

snowystinger added 2 commits May 21, 2024 10:20

move hidden logic to hook

48198c6

Merge branch 'main' into toast-focus-take-2

0f25164

majornista previously approved these changes May 21, 2024

View reviewed changes

Merge branch 'main' into toast-focus-take-2

0ccee46

snowystinger mentioned this pull request May 22, 2024

Toast focus improvements #6011

Closed

5 tasks

Merge branch 'main' into toast-focus-take-2

fd5ed10

LFDanLu reviewed May 24, 2024

View reviewed changes

clarify difficult code

3a65e51

snowystinger dismissed majornista’s stale review via 3a65e51 May 24, 2024 02:34

Merge branch 'main' into toast-focus-take-2

5491f7d

LFDanLu approved these changes May 24, 2024

View reviewed changes

devongovett approved these changes May 30, 2024

View reviewed changes

Merge branch 'main' into toast-focus-take-2

73a2157

snowystinger merged commit ad6e240 into main May 30, 2024
26 checks passed

snowystinger deleted the toast-focus-take-2 branch May 30, 2024 05:49

devongovett mentioned this pull request Jan 14, 2025

Probably we should use status and alert role instead of alertdialog role for useToast and Toast component #7318

Closed

Toast focus management take 2 #6223

Toast focus management take 2 #6223

Conversation

snowystinger commented Apr 17, 2024 • edited Loading

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

rspbot commented Apr 23, 2024

rspbot commented Apr 29, 2024

rspbot commented Apr 30, 2024

rspbot commented May 2, 2024

rspbot commented May 3, 2024

rspbot commented May 3, 2024

rspbot commented May 8, 2024

majornista commented May 9, 2024

rspbot commented May 9, 2024

rspbot commented May 20, 2024

rspbot commented May 20, 2024

rspbot commented May 20, 2024

rspbot commented May 21, 2024

majornista left a comment

Choose a reason for hiding this comment

rspbot commented May 21, 2024

rspbot commented May 22, 2024

Choose a reason for hiding this comment

snowystinger May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rspbot commented May 24, 2024

rspbot commented May 24, 2024

LFDanLu left a comment

Choose a reason for hiding this comment

devongovett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majornista Aug 1, 2024 • edited by snowystinger Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowystinger commented May 30, 2024

rspbot commented May 30, 2024

rspbot commented May 30, 2024

@react-aria/toast

ToastAria

snowystinger commented Apr 17, 2024 •

edited

Loading

snowystinger May 24, 2024 •

edited

Loading

majornista Aug 1, 2024 •

edited by snowystinger

Loading