-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Toast focus management take 2 #6223
Changes from all commits
a685b76
db1f278
5d658cb
5c8bf5c
744bfb4
aae7f0f
6d52a1b
b78fd33
3f0ab0c
8a2eb3f
f718eaf
3d38fdd
344d3e0
abca7f9
731f371
2992e55
a23f010
a62255b
d619d72
98fdb0c
a025eb6
e22e319
66ff206
1752bf3
60692de
48198c6
0f25164
0ccee46
fd5ed10
3a65e51
5491f7d
73a2157
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
{ | ||
"close": "Close", | ||
"notifications": "Notifications" | ||
"notifications": "{count, plural, one {# notification} other {# notifications}}." | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,8 +15,8 @@ import {AriaLabelingProps, DOMAttributes, FocusableElement} from '@react-types/s | |
// @ts-ignore | ||
import intlMessages from '../intl/*.json'; | ||
import {QueuedToast, ToastState} from '@react-stately/toast'; | ||
import {RefObject, useEffect, useRef} from 'react'; | ||
import {useId, useLayoutEffect, useSlotId} from '@react-aria/utils'; | ||
import React, {RefObject, useEffect} from 'react'; | ||
import {useId, useSlotId} from '@react-aria/utils'; | ||
import {useLocalizedStringFormatter} from '@react-aria/i18n'; | ||
|
||
export interface AriaToastProps<T> extends AriaLabelingProps { | ||
|
@@ -25,8 +25,10 @@ export interface AriaToastProps<T> 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. */ | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we just remove the ref from the args even though it would be breaking? It is still in beta so might be ok? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
export function useToast<T>(props: AriaToastProps<T>, state: ToastState<T>, ref: RefObject<FocusableElement>): ToastAria { | ||
let { | ||
key, | ||
|
@@ -58,44 +61,35 @@ export function useToast<T>(props: AriaToastProps<T>, state: ToastState<T>, ref: | |
}; | ||
}, [timer, timeout]); | ||
|
||
// Restore focus to the toast container on unmount. | ||
// If there are no more toasts, the container will be unmounted | ||
// and will restore focus to wherever focus was before the user | ||
// focused the toast region. | ||
let focusOnUnmount = useRef(null); | ||
useLayoutEffect(() => { | ||
let container = ref.current.closest('[role=region]') as HTMLElement; | ||
return () => { | ||
if (container && container.contains(document.activeElement)) { | ||
// Focus must be delayed for focus ring to appear, but we can't wait | ||
// until useEffect cleanup to check if focus was inside the container. | ||
focusOnUnmount.current = container; | ||
} | ||
}; | ||
}, [ref]); | ||
|
||
// eslint-disable-next-line | ||
let [isEntered, setIsEntered] = React.useState(false); | ||
useEffect(() => { | ||
return () => { | ||
if (focusOnUnmount.current) { | ||
focusOnUnmount.current.focus(); | ||
} | ||
}; | ||
}, [ref]); | ||
if (animation === 'entering') { | ||
setIsEntered(true); | ||
} | ||
}, [animation]); | ||
|
||
let titleId = useId(); | ||
let descriptionId = useSlotId(); | ||
let stringFormatter = useLocalizedStringFormatter(intlMessages, '@react-aria/toast'); | ||
|
||
return { | ||
toastProps: { | ||
role: 'alert', | ||
role: 'alertdialog', | ||
'aria-modal': 'false', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I use
|
||
'aria-label': props['aria-label'], | ||
'aria-labelledby': props['aria-labelledby'] || titleId, | ||
'aria-describedby': props['aria-describedby'] || descriptionId, | ||
'aria-details': props['aria-details'], | ||
// Hide toasts that are animating out so VoiceOver doesn't announce them. | ||
'aria-hidden': animation === 'exiting' ? 'true' : undefined | ||
'aria-hidden': animation === 'exiting' ? 'true' : undefined, | ||
tabIndex: 0 | ||
}, | ||
contentProps: { | ||
role: 'alert', | ||
'aria-atomic': 'true', | ||
style: { | ||
visibility: isEntered || animation === null ? 'visible' : 'hidden' | ||
} | ||
}, | ||
titleProps: { | ||
id: titleId | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
import {AriaLabelingProps, DOMAttributes} from '@react-types/shared'; | ||
import {focusWithoutScrolling, mergeProps} from '@react-aria/utils'; | ||
import {focusWithoutScrolling, mergeProps, useLayoutEffect} from '@react-aria/utils'; | ||
import {getInteractionModality, useFocusWithin, useHover} from '@react-aria/interactions'; | ||
// @ts-ignore | ||
import intlMessages from '../intl/*.json'; | ||
|
@@ -29,14 +29,80 @@ export function useToastRegion<T>(props: AriaToastRegionProps, state: ToastState | |
let stringFormatter = useLocalizedStringFormatter(intlMessages, '@react-aria/toast'); | ||
let {landmarkProps} = useLandmark({ | ||
role: 'region', | ||
'aria-label': props['aria-label'] || stringFormatter.format('notifications') | ||
'aria-label': props['aria-label'] || stringFormatter.format('notifications', {count: state.visibleToasts.length}) | ||
}, ref); | ||
|
||
let {hoverProps} = useHover({ | ||
onHoverStart: state.pauseAll, | ||
onHoverEnd: state.resumeAll | ||
}); | ||
|
||
// Manage focus within the toast region. | ||
// If a focused containing toast is removed, move focus to the next toast, or the previous toast if there is no next toast. | ||
// We might be making an assumption with how this works if someone implements the priority queue differently, or | ||
// if they only show one toast at a time. | ||
let toasts = useRef([]); | ||
let prevVisibleToasts = useRef(state.visibleToasts); | ||
let focusedToast = useRef(null); | ||
useLayoutEffect(() => { | ||
// If no toast has focus, then don't do anything. | ||
if (focusedToast.current === -1 || state.visibleToasts.length === 0) { | ||
toasts.current = []; | ||
prevVisibleToasts.current = state.visibleToasts; | ||
return; | ||
} | ||
toasts.current = [...ref.current.querySelectorAll('[role="alertdialog"]')]; | ||
// If the visible toasts haven't changed, we don't need to do anything. | ||
if (prevVisibleToasts.current.length === state.visibleToasts.length | ||
&& state.visibleToasts.every((t, i) => t.key === prevVisibleToasts.current[i].key)) { | ||
prevVisibleToasts.current = state.visibleToasts; | ||
return; | ||
} | ||
// Get a list of all toasts by index and add info if they are removed. | ||
let allToasts = prevVisibleToasts.current | ||
.map((t, i) => ({ | ||
...t, | ||
i, | ||
isRemoved: !state.visibleToasts.some(t2 => t.key === t2.key) | ||
})); | ||
|
||
let removedFocusedToastIndex = allToasts.findIndex(t => t.i === focusedToast.current); | ||
|
||
// If the focused toast was removed, focus the next or previous toast. | ||
if (removedFocusedToastIndex > -1) { | ||
let i = 0; | ||
let nextToast; | ||
let prevToast; | ||
while (i <= removedFocusedToastIndex) { | ||
if (!allToasts[i].isRemoved) { | ||
prevToast = Math.max(0, i - 1); | ||
} | ||
i++; | ||
} | ||
while (i < allToasts.length) { | ||
if (!allToasts[i].isRemoved) { | ||
nextToast = i - 1; | ||
break; | ||
} | ||
i++; | ||
} | ||
|
||
// in the case where it's one toast at a time, both will be undefined, but we know the index must be 0 | ||
if (prevToast === undefined && nextToast === undefined) { | ||
prevToast = 0; | ||
} | ||
|
||
// prioritize going to newer toasts | ||
if (prevToast >= 0 && prevToast < toasts.current.length) { | ||
focusWithoutScrolling(toasts.current[prevToast]); | ||
} else if (nextToast >= 0 && nextToast < toasts.current.length) { | ||
focusWithoutScrolling(toasts.current[nextToast]); | ||
majornista marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
prevVisibleToasts.current = state.visibleToasts; | ||
}, [state.visibleToasts, ref]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
let lastFocused = useRef(null); | ||
let {focusWithinProps} = useFocusWithin({ | ||
onFocusWithin: (e) => { | ||
|
@@ -49,10 +115,22 @@ export function useToastRegion<T>(props: AriaToastRegionProps, state: ToastState | |
} | ||
}); | ||
|
||
// When the region unmounts, restore focus to the last element that had focus | ||
// before the user moved focus into the region. | ||
// TODO: handle when the element has unmounted like FocusScope does? | ||
// eslint-disable-next-line arrow-body-style | ||
// When the number of visible toasts becomes 0 or the region unmounts, | ||
// restore focus to the last element that had focus before the user moved focus | ||
// into the region. FocusScope restore focus doesn't update whenever the focus | ||
// moves in, it only happens once, so we correct it. | ||
// Because we're in a hook, we can't control if the user unmounts or not. | ||
useEffect(() => { | ||
if (state.visibleToasts.length === 0 && lastFocused.current && document.body.contains(lastFocused.current)) { | ||
if (getInteractionModality() === 'pointer') { | ||
focusWithoutScrolling(lastFocused.current); | ||
} else { | ||
lastFocused.current.focus(); | ||
} | ||
lastFocused.current = null; | ||
} | ||
}, [ref, state.visibleToasts.length]); | ||
|
||
useEffect(() => { | ||
return () => { | ||
if (lastFocused.current && document.body.contains(lastFocused.current)) { | ||
|
@@ -61,6 +139,7 @@ export function useToastRegion<T>(props: AriaToastRegionProps, state: ToastState | |
} else { | ||
lastFocused.current.focus(); | ||
} | ||
lastFocused.current = null; | ||
} | ||
}; | ||
}, [ref]); | ||
|
@@ -73,7 +152,16 @@ export function useToastRegion<T>(props: AriaToastRegionProps, state: ToastState | |
// - allows focus even outside a containing focus scope | ||
// - doesn’t dismiss overlays when clicking on it, even though it is outside | ||
// @ts-ignore | ||
'data-react-aria-top-layer': true | ||
'data-react-aria-top-layer': true, | ||
// listen to focus events separate from focuswithin because that will only fire once | ||
// and we need to follow all focus changes | ||
onFocus: (e) => { | ||
let target = e.target.closest('[role="alertdialog"]'); | ||
focusedToast.current = toasts.current.findIndex(t => t === target); | ||
}, | ||
onBlur: () => { | ||
focusedToast.current = -1; | ||
} | ||
}) | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see this was added to prevent double announcements, but where do we use Text in toasts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, i haven't noticed anything yet There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
{children} | ||
</span> | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need both contentProps and titleProps as separate elements or are they the same thing now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snowystinger I see. Thanks for the response!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@levrik and @snowystinger The new
contentProps
includes therole="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
andcontentProps
on the same element.