Skip to content
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

[Feature Request]: Add a "disableFocusTrap" prop to the ActionableNotification component #17392

Open
1 task done
TazmanianDI opened this issue Sep 9, 2024 · 10 comments
Open
1 task done
Labels
proposal: accepted This request has gone through triaging and we are accepting PR's against it. role: dev 🤖 type: enhancement 💡

Comments

@TazmanianDI
Copy link

The problem

The ActionableNotification component is currently implemented to require a user to dismiss it before they can interact with the rest of the application. When an actionable notification is opened, it will immediately grab focus and then trap focus inside the notification until the notification is dismissed.

We have a use case where the notification is used for information purposes about asynchronous processes. The user triggers some action that completes later and the notification is used to inform the user the action is complete and it includes a button to take the user to that action to see the results. When an action is complete, is should be informational to the user and should not forcibly move or trap focus. There may even be multiple notifications. We know the current behavior is as designed and we would like a prop to allow us to control the behavior.

There is currently a hasFocus prop on the component now that disables the initial grabbing of focus but this prop is deprecated and even with that prop, the notification will still trap focus if focus is moved inside of it.

Here are a few related issues discussing this problem:
#15319
#16231

The solution

Add something like a disableFocusTrap prop to the component that will disable both the initial grabbing of focus and the focus trap that occurs when elements inside the notification get focus.

Examples

Here's a video showing a simplified example. The user is interacting with a chat bot that starts an asynchronous process. When the process complete, focus is moved away from the field while the user is in the middle of typing. Just recording this video was hard because I kept pressing the space bar as I was typing which just dismissed the notification immediately when it got focus.

Screen.Recording.2024-09-09.at.10.24.36.AM.mov

Application/PAL

No response

Business priority

None

Available extra resources

No response

Code of Conduct

Copy link
Contributor

github-actions bot commented Sep 9, 2024

Thank you for submitting a feature request. Your proposal is open and will soon be triaged by the Carbon team.

If your proposal is accepted and the Carbon team has bandwidth they will take on the issue, or else request you or other volunteers from the community to work on this issue.

@tay1orjones
Copy link
Member

Thanks for opening this issue and showing your use-case so clearly!

Add ... disableFocusTrap ... that will disable both the initial grabbing of focus and the focus trap that occurs when elements inside the notification get focus.

ActionableNotification is an alertdialog, and to conform to the spec it must grab and wrap focus.

As you saw in the related issues you linked, the initial addition of StaticNotification (soon to be renamed Callout) aimed to target this type of interaction. Since then we've determined that to ensure these are fully accessible they need to be rendered in the initial DOM and not added later as the result of a user/system action.

This is a challenging design problem. @carbon-design-system/design are there other options for this type of interaction?

@TazmanianDI
Copy link
Author

ActionableNotification is an alertdialog, and to conform to the spec it must grab and wrap focus.

As you saw in the related issues you linked, the initial addition of StaticNotification (soon to be renamed Callout) aimed to target this type of interaction. Since then we've determined that to ensure these are fully accessible they need to be rendered in the initial DOM and not added later as the result of a user/system action.

This is a challenging design problem. @carbon-design-system/design are there other options for this type of interaction?

Ah, I didn't notice the references to the experimental StaticNotification. I do agree that alertdialog requires the focus trap but dialog does not. It is a problem that StaticNotification doesn't trigger any announcements though. Rendering them in the DOM before will not work in our use case because there is not a fixed number of them. It could be a list of notifications.

Is the problem because the screen reader won't announce the initial appearance of a live region? The trick we've used in the past is to render the element in a live region that's empty and then use a setTimeout to add the content on a second browser pass which then does allow a screen reader to catch it. Would that help? Although if it is not required that a dialog be announced then I suppose StaticNotification would be doing the right thing. Our app does have a mechanism for explicitly triggering the reading of content so maybe we can just rely on that ourselves.

@TazmanianDI
Copy link
Author

@tay1orjones This is a bit of a tangent but is there a reason that StaticNotification doesn't appear to have an onClose prop like ActionableNotification?

@TazmanianDI
Copy link
Author

I think maybe that StaticNotification component is not quite ready to go. Looks like there's some styling issues with it. I'm seeing the title and message on the same line and the alignment of the action button is off. There should be some space to the left. And I guess there's no "x" button to dismiss the notification without taking the action?
image

Here's the ActionableNotification for comparison.
image

@tay1orjones
Copy link
Member

Yeah, when a notification with interactive elements/actions is added to the DOM as a result of a user/system action, it needs to be both announced and able to be easily navigated to by a keyboard/screenreader user.

The first part, announcement, is easy to do as you mention with your approach of an existing live region.

The second piece, easily navigating to the notification, can be very difficult to solve and implement properly. Aiming to provide an accessible experience out of the box, the only way to do this right now is through an ActionableNotification alertdialog. That said, I do think we need to continue to push further and provide a way to accomplish what you're describing. There's only two approaches I'm aware of:

  1. Rendering notifications into an existing region that can immediately be navigated to via a hotkey. Once the user navigates there and takes an action or gets to the last focusable element, focus returns from the region to the element previously focused before they entered the region.
  2. Collecting notifications in a persistent place in the app (a "drawer" or "side panel" type of thing) and announcing to the user how they can navigate to that place to review all notifications and optionally take action on it.

There is not a way for you to take on that complexity right now with ActionableNotification being locked to alertdialog. If you're wanting to though, we could consider potentially opening up the role to be configurable with some type of dev warning to explain some of this.


The StaticNotification/Callout intentionally does not have a close button because the design spec calls for it to not be dismissable.

@TazmanianDI
Copy link
Author

Yeah, letting us control the role somehow along with the focus behavior is essentially what we want.

it needs to be both announced

I'm not sure if this is actually required but I'm not an a11y expert. What little I've read suggests there's just no hard rule here and I wonder if perhaps that means Carbon should just delegate this responsibility to the application. If the application wants to display a non-modal dialog then it needs to decide if it should be announced and if it does, the application can put it inside a live region. The application could also be responsible for putting the modal in a region to make it easier to navigate to. I don't know if Carbon wants to take a more specific enforcement opinion on these questions or be more out-of-the-box as you put it.

I think we'll probably be okay with any of the region/announcement options as long as we can get a non-modal dialog that doesn't steal or trap focus.

@sstrubberg
Copy link
Member

Waiting to chat with @mbgower in a future CAG (Carbon Accessibility Guild) call.

@mbgower
Copy link

mbgower commented Oct 15, 2024

Yeah, I think this can use a bit more discussion.
We have an accessibility guild call scheduled for 7am Pacific next Monday, Oct 21. @sstrubberg and @TazmanianDI are you able to attend and we can discuss?

@tay1orjones
Copy link
Member

tay1orjones commented Nov 4, 2024

We chatted about this today on the CAG call. Plan of action we came up with is below:

  • Update ActionableNotification to allow role to be configurable
  • If role is configured:
    • provide console warning in dev w/ a link to documentation
    • if role !== alertdialog, turn off the focus trap/wrap
  • Add documentation on what needs to be done in application if you reconfigure role to be something other than alertdialog (Rendering notifications into an existing region, etc)
    • Put a link to this in the dev console warning
  • Encourage the use of Callout instead of ActionableNotification, where possible (if interaction isn't needed)
    • Can alert|log|status contain interactive elements? Our existing understanding is no, but this may have shifted since we made this decision for v11. If it still cannot contain interactive elements, this limits the ability to use a Callout instead of ActionableNotification

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal: accepted This request has gone through triaging and we are accepting PR's against it. role: dev 🤖 type: enhancement 💡
Projects
Status: Next ➡
Development

No branches or pull requests

4 participants