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

[Bug]: Keyboard Navigation Stuck Within Actionable Notification Component #15319

Closed
2 tasks done
ellamathewsonibm opened this issue Dec 4, 2023 · 17 comments
Closed
2 tasks done
Labels
proposal: not pursuing This has gone through triaging and because of priorities or mission, we will not be pursuing this. type: bug 🐛

Comments

@ellamathewsonibm
Copy link

Package

@carbon/react

Browser

Chrome

Package version

v1.42.1

React version

v18.2.0

Description

When using keyboard navigation on Actionable notification, tab focus cannot escape the alert and instead cycles through the action button and close button indefinitely. When the notification only has either the action or close buttons, the tab focus does not move from the button.

Screen.Recording.2023-12-04.at.4.36.58.PM.mov

Reproduction/example

https://react.carbondesignsystem.com/?path=/story/components-notifications-actionable--playground&args=kind:info;lowContrast:!true;statusIconDescription:in;inline:!true

Steps to reproduce

Go to Notifications -> Actionable -> Playground. Press Tab on keyboard to navigate a couple times. See focus remains switching between the x close icon button and the action button, never leaving the component.

Suggested Severity

Severity 2 = User cannot complete task, and/or no workaround within the user experience of a given component.

Application/PAL

w3 Carbon

Code of Conduct

@kuri-sun
Copy link
Contributor

kuri-sun commented Dec 5, 2023

Hello! I love to work on this issue! May I?

@ellamathewsonibm
Copy link
Author

Hello! I love to work on this issue! May I?

Yes please! Thank you

@kuri-sun
Copy link
Contributor

kuri-sun commented Dec 5, 2023

Hi @ellamathewsonibm and everyone!
After investigating this issue, this trap focus behavior is an intentional feature implemented by this PR.
In terms of trap focus, I did not even know that and I found a great article about that. I will put the article here for a knowledge exchange! here is the article.
For maintainers, If this understanding is correct, I request to close this issue.
Thank you so much!

@lluisrojass
Copy link
Contributor

lluisrojass commented Dec 5, 2023

@kuri-sun the behavior seems unintentionally aggressive. When you render an actionable notification without a close button and turn closeOnEscape to false, focus is trapped indefinitely.

@lluisrojass
Copy link
Contributor

@kuri-sun

from: https://carbondesignsystem.com/components/notification/usage/#actionable-notifications

Actionable notifications allow for interactive elements within a notification styled like an inline or toast notification. Actionable notifications, since they require user interaction, take focus when triggered and can be highly disruptive to screen readers and keyboard users. With actionable notifications, only one action is allowed per notification. This action frequently takes users to a flow or page related to the message, where they can resolve the notification

from the PR: #14877

This enhancement ensures that when a user receives a notification with the hasFocus prop set to true, the focus is correctly directed to the component.

I think when rendered with the default hasFocus=true behavior, the intention is for the notification to hijack focus, and coerce a response from the user. However, a persisted actionable notifications also feels like a valid us case, and plays a different role where it feels this behavior is not appropriate.

@juanencalada
Copy link

Hey @ellamathewsonibm and @lluisrojass, thanks for sharing your notes here. Reaching out to Michael Gower about this as well from Carbon's accessibility team

@kuri-sun
Copy link
Contributor

kuri-sun commented Dec 5, 2023

Thank you for the explanation @lluisrojass!
Here is my understanding right now.
When hasFocus=true, trap the user's focus. (but never loop. just once.)
When hasFocus=false, it never intercepts the user's focus. (do nothing)
Please correct me if I've mistaken something.
If these are correct, I would love to work on this issue! 😃
Thank you!

@lluisrojass
Copy link
Contributor

My understanding is that hasFocus=true means grab the user's focus and trap it until the notification is dismissed or the action is triggered (loops until action taken) and hasFocus=false won't grab the user's focus, and in my opinion it should not trap either.

@guidari are you able to provide an opinion?

@guidari
Copy link
Contributor

guidari commented Dec 6, 2023

Hello guys.
@kuri-sun is right this is an intentional feature that was implemented. But I see the point that @lluisrojass is making by saying that hasFocus=false won't grab the user focus (as it does now) and also shouldn't trap the user focus when tabbing.

I'll bring that issue to the team so we can all discuss together and I'll came back to you guys as soon as possible.

@kuri-sun
Copy link
Contributor

kuri-sun commented Dec 6, 2023

Thanks for your opinion @guidari!

@guidari
Copy link
Contributor

guidari commented Dec 11, 2023

Hey! @lluisrojass

I talked with the team and we are technically already allowing to go off-spec by providing the hasFocus prop to disable the component from taking focus when it's rendered. The spec says that the focus should be trapped in all cases.

It can be implemented ways for the user to close the ActionableNotification with a close button or ESC key.
The recommend action is to set hasFocus to true.

Thank you!

@guidari guidari closed this as completed Dec 11, 2023
@guidari guidari added proposal: not pursuing This has gone through triaging and because of priorities or mission, we will not be pursuing this. and removed status: waiting for maintainer response 💬 labels Dec 11, 2023
Copy link
Contributor

Thank you for submitting your proposal. Unfortunately, it doesn't align with our roadmap or philosophy at this time. We value your contribution and encourage you to keep engaging with the community.

@alisonjoseph alisonjoseph closed this as not planned Won't fix, can't repro, duplicate, stale Dec 11, 2023
@finken2
Copy link
Contributor

finken2 commented Jan 4, 2024

@guidari @tay1orjones Can we reopen this issue? I ran into it today and would like to have the history here instead of opening a new issue.
The actionable notification is trapping focus even when used inline, making the other items on the page unusable. Here's an example below. You can't click in the text fields or tab to them.
image

@guidari
Copy link
Contributor

guidari commented Jan 5, 2024

The actionable notification is trapping focus even when used inline, making the other items on the page unusable. Here's an example below. You can't click in the text fields or tab to them.

Hey @finken2
This is an intended behavior in the Actionable variant.

I checked the component usage in the carbon website and you can use the InlineNotification variant to insert a "Learn More" link, like is being done in the website.

@finken2
Copy link
Contributor

finken2 commented Jan 5, 2024

@guidari In v11, we aren't supposed to have actions in the InlineNotification variant. We can discuss on Slack, but we should not have ActionableNotification as a focus trap that renders the rest of the page unusable.

@tay1orjones
Copy link
Member

tay1orjones commented Jan 5, 2024

@finken2 ActionableNotification should be used when the user must take action. I wouldn't use it for the use case you outline above because the link appears to be optional.

InlineNotification accepts children for this limited use case, and it's not documented because the link is somewhat inaccessible due to the role being one of 'alert' | 'log' | 'status'. These roles don't explicitly state children are only presentational, but screenreaders will usually strip semantics from the contents. The link will still be discoverable in the tab order though.

We do this on our website for similar use cases:

image

@finken2
Copy link
Contributor

finken2 commented Jan 5, 2024

@tay1orjones I'll ping you to discuss, but we need to do some clarification or code changes somewhere. We've changed a ton of our InlineNotifications to be Actionable based on the v11 guidance and i'm sure others have done the same. Regardless of that, the current implementation i showed in my screenshot should not act like it does. You can't even click in the other text fields on the page; it's not just a tabbing issue. Without some kind of glasspane/modal indication, there's no way the user would expect that behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal: not pursuing This has gone through triaging and because of priorities or mission, we will not be pursuing this. type: bug 🐛
Projects
Archived in project
Development

No branches or pull requests

8 participants