-
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 improvements #6011
Toast focus improvements #6011
Conversation
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
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.
If I remove/clear the top most toast in a stack of > 1, focus is lost to the body.
If I use the storybook controls to set shouldCloseOnAction
true, and then use an action on a toast, the focus is lost to the body.
Open question, what should happen with timeout and focus on a particular toast, vs on the container, vs on a different toast? Right now, they all freeze and none of them dismiss no matter which toast you have focused.
@@ -1,4 +1,5 @@ | |||
{ | |||
"close": "Close", | |||
"notifications": "Notifications" | |||
"notifications": "Notifications", | |||
"countAnnouncement": "{count, plural, one {# notification} other {# notifications}}." |
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.
I'm never seeing this announcement, when should it be happening?
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.
F6 to switch landmark regions is the easiest way.
Regarding your general comment it looks like the addition of focusing Toasts broke the focus management improvements. Waiting for feedback from Michael before fixing.
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.
I'm not seeing this announcement in the latest built version on Safari. @majornista how do I get this notification?
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.
I think I may have fixed this announcement.
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.
I see this now, though it does get read 2x in Safari. And in Chrome it reads that there is a list 2 items long, this announcement never seems to appear.
7909022
to
d724773
Compare
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
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.
Toasts no longer announce when they appear?
@@ -1,4 +1,5 @@ | |||
{ | |||
"close": "Close", | |||
"notifications": "Notifications" | |||
"notifications": "Notifications", | |||
"countAnnouncement": "{count, plural, one {# notification} other {# notifications}}." |
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.
I'm not seeing this announcement in the latest built version on Safari. @majornista how do I get this notification?
Build successful! 🎉 |
Michael, to confirm the testing strategy, can we write up a specification for what we're trying to implement? Below are the cases I'm aware of. Can you please update and/or fill in the blanks? This is to help with testing. Toast announcement
Toast landmark region
Use cases
Questions
|
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.
Seems like duplicate messages when triggering a Toast because it announces the Toast (message, button, and status) then announces the Toast status and message.
When I navigated to the landmark region where I've previously focused a toast, it landed on a toast, anounced the toast, then announced the region (number of notifications) twice.
I noticed we have list 2 items
and 2 notifications
being announced together, seems redundant.
'aria-hidden': animation === 'exiting' ? 'true' : undefined, | ||
tabIndex: 0 | ||
}, | ||
contentProps: { |
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.
we'll need to keep this in mind for announcement purposes, it might be argued that it's a breaking change
Build successful! 🎉 |
@@ -1,4 +1,5 @@ | |||
{ | |||
"close": "Close", | |||
"notifications": "Notifications" | |||
"notifications": "Notifications", | |||
"countAnnouncement": "{count, plural, one {# notification} other {# notifications}}." |
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.
I see this now, though it does get read 2x in Safari. And in Chrome it reads that there is a list 2 items long, this announcement never seems to appear.
useLayoutEffect(() => { | ||
let timeoutId:ReturnType<typeof setTimeout>; | ||
if (isBusy && ref.current) { | ||
timeoutId = setTimeout(() => setIsBusy(false), 50); |
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.
where'd 50 come from? does it match an animation duration?
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.
50 is based on allowing a couple keyframes for the Toast content to render before removing aria-hidden
so that the Toast will announce.
clearTimeout(timeoutId); | ||
timeoutId = undefined; | ||
} | ||
timeoutId = setTimeout(() => announce(announcement, 'polite'), 750); |
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.
where'd this timeout value from from?
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.
Probably trial and error in an effort to allow ToastRegion to announce on focus before the number of notifications within it.
} | ||
<div className={classNames(styles, 'spectrum-Toast-body')} role="presentation"> | ||
<div className={classNames(styles, 'spectrum-Toast-content')} role="presentation" {...titleProps}>{children}</div> | ||
{actionLabel && |
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.
should the action button actually be outside the aria-atomic region? I guess it makes sense if you want someone to know that the alert has something you can do with it
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.
I think it is important to know action as part of the announcement.
Build successful! 🎉 |
Build successful! 🎉 |
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.
- Improve Toast alert announcement - Improve countAnnouncement - Restore focus ring on Toaster container element - fix shouldCloseOnAction behavior to include focus management logic
Use aria-hidden instead of aria-busy.
This prevents VoiceOver from announcing the Text node within a Button as a node addition within a live region, which results in double voicing.
Build successful! 🎉 |
## API Changes
unknown top level export { type: 'any' } @react-aria/toastToastAria ToastAria {
closeButtonProps: AriaButtonProps
+ contentProps: DOMAttributes
descriptionProps: DOMAttributes
titleProps: DOMAttributes
toastProps: DOMAttributes
} it changed:
|
closing in favor of #6223 |
Focus issues found during testing
This behaves slightly better than v2 Toasts. When removing Toasts it behaves the same as far as not focusing another toast, but staying within the container. With the last one we now restore focus to the last focused item outside the ToastContainer.
This isn't doing what the display one Toast at a time did, which is focus Toasts loading from the queue or focusing the container when the last toast is removed.
If we like this, I'll remove all the focus code from the container and focusing an actual Toast not the close button.
✅ Pull Request Checklist:
📝 Test Instructions:
Add and remove several Toasts and see where focus goes.
In Toast storybook example at: https://reactspectrum.blob.core.windows.net/reactspectrum/b849db005f61cfeed14f599878c5085e14d1d154/storybook/index.html?path=/story/toast--with-action&args=shouldCloseOnAction:true&providerSwitcher-express=false
With a screen reader running:
Toast live region announcement should include,
Toast live region announcement should not include the close button.
The Toast landmark region should labelled "Notification," by default and should politely announce the number of notifications when focus moves into the landmark region, “# notifications.”
Focus management use cases:
Toast are organized within an ordered list within the landmark region, we should not need to announce the number of notifications except when moving focus to the landmark region. Depending on the screen reader, the new count for the number of items in the list might be announced automatically when the list count changes.
Focus can move to the Notifications landmark region:
Regardless of how focus entered the Notifications landmark, F6 should eventually cycle back to the last focused element in the main landmark, and focus management when a Toast is removed should behave the same regardless of the manner in which the Toast received focus. For example, if I were to remove a Toast using a mouse click on the close button, focus should move to the adjacent Toast, rather than be lost to the document.body.
🧢 Your Project:
RSP