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

Focus trap in NotificationPanel prevents user from navigating to next item in UIShell header #6432

Closed
anurima9 opened this issue Nov 14, 2024 · 5 comments · Fixed by #6829
Closed
Assignees
Labels
component: NotificationsPanel role: dev type: enhancement 💡 New feature or request UX: consistency Issues leading to inconsistencies within one or across multiple experiences UX: issue Any type of experience feedback that once addressed improves or enhances the experience UX: usability Issues that impact task completion (prevents, slows down, introduces errors)
Milestone

Comments

@anurima9
Copy link

I am trying to work only with the keyboard strokes, and on click of Tab I go to the notifications panel, click Enter to open it, and Tab again to bring focus to the different Tabbable elements in the panel. But it takes us into a loop and not able to exit from the panel after we reach the end of the panel. While we do have Esc key to exit and close the panel, we want to have other options to exit the panel so as to bring consistency among our other header action components fhttps://github.com/user-attachments/assets/a923b755-ea36-4d07-976d-4a39da52a4b7
https://github.com/user-attachments/assets/cd8f696d-22a0-4be0-8592-b941ad0e2615
or the ease of the user. Attached is one screen recording of the same behavior.

@arinclementibm
Copy link

This was discussed during Carbon office hours on Nov 12, 2024. [email protected] led the discussion :)

@elycheea elycheea added type: enhancement 💡 New feature or request component: NotificationsPanel UX: issue Any type of experience feedback that once addressed improves or enhances the experience UX: usability Issues that impact task completion (prevents, slows down, introduces errors) UX: consistency Issues leading to inconsistencies within one or across multiple experiences and removed status: needs triage 🕵️‍♀️ labels Nov 19, 2024
@elycheea elycheea moved this from Needs triage 🧐 to Needs refinement 🤓 in Carbon for IBM Products Nov 19, 2024
@anurima9
Copy link
Author

anurima9 commented Dec 6, 2024

@elycheea Is there any update on this issue? Could you please help here?
@arinclementibm

@elycheea
Copy link
Contributor

elycheea commented Dec 6, 2024

@arinclementibm @anurima9 Ahh, it sounds like you shared this in the System Squad office hours since we didn’t see you in our IBM Products office hours that day!

@RichKummer reached out to Michael Gower about the current behavior in our notifications panel which does fall inline with current expectations.

We could consider allowing teams to determine whether the notifications panel should be modal or non-modal in the meantime. @RichKummer maybe we can have some cross-squad discussion on it next refinement.

@arinclementibm @anurima9 if you’re still able to join our office hours, you can find our team in another teams room — https://ibm.biz/join-carbon-dev-ibm-products-office-hours! It’ll help us guide the conversation better in our next refinement.

@arinclementibm
Copy link

Thank you for reviewing the notification panel today in office hours!
The link to our SIT environment to see the experience is: https://awfe-test.wdc1a.ciocloud.nonprod.intranet.ibm.com/homepage
We are expecting to have UAT in January that includes the UI shell experience, with a goal of launching our MVP1 in Feb 2025.

@matthewgallo
Copy link
Member

matthewgallo commented Dec 11, 2024

Analysis from @paul-balchin-ibm

Apples and Oranges:

  • Apples: The current Story renders the Panel at the bottom of the DOM. Even if focusTrapping is disabled, the tabbing order will not move to the next icon in the Header. Instead it will return to the top of the DOM/page.
  • Oranges: The live site renders the same Panel directly beneath the button icon in the DOM. If focusTrapping is disabled, the tabbing order should close the existing Panel and tab to the next icon in the header. This needs to be tested in Storybook.
  • Technically, the Panel is a standalone component and how and where it is rendered in the DOM is outside of our scope .

Refinement notes

The original Panel as integrated with the UI Shell worked as expected. However, removing focus trapping created many unintended side effects.

  1. The Shell's notification button was grabbing focus regardless of whatever else you clicked on.
  2. Because the Notification panel was being rendered at the bottom of the DOM, the Shell buttons and Panel tabbing order was now out of order.
  3. Etc.

After chatting with @elycheea and @matthewgallo, we settled on the following fixes:

  • Fix how the Shell and Panel "flicker" when a new notification is added while the Panel is open. For purposes of the Storybook demo and respecting tabbing order, the Notification panel is rendered inline with the Shell's notification panel. However, the "Add notification" button passes through the Shell and we can now see a re-render of the Shell and Notification panel. This was not an issue previously because you could only add a notification if the Notification panel was closed.
    • Test the Panel without the Shell, Panel focus trap disabled. Tested: the Panel does not flicker.
    • Test the Panel with the Shell (Panel sits outside of the Shell), Panel focus trap disabled: Tested: the Panel does not flicker.
    • Test the Panel with the Shell (Panel sits inside the Shell to respect keyboard tabbing order), Panel focus trap disabled: Problem found; see below.
    • If all the above fail, try using React Context to minimize prop drilling and affecting both Shell and Panel.
  • Remove focus trapping altogether. Issue 5448 suggested adding focus trapping to the Notification panel and was added, but without a follow up deep dive discussion as part of the typical process when considering such a complex issue. (Not pointing fingers, just skipped a process by mistake)
  • Keep onClickOutside() functionality to close the panel if the user clicks anywhere outside the boundaries of the Notification panel component.

Test results resolving the "flicker" issue.

The UI Shell has a top-level <HeaderContainer render={ () => ( child Shell components + your content ) }>.

Image

Any state changes inside <HeaderContainer> forces a re-render. This was never noticed before because there was no UI Shell demo using dynamic data.

The NotificationsPanel has an animated intro. This means any update to state (i.e. add/remove/etc notification), and:

  1. the Shell erases all of the child Shell components + your content,
  2. everything is re-rendered from scratch,
  3. the NotificationsPanel's animated intro runs again.
with.header.container.mov

Since the NotificationsPanel is always being re-rendered "as new", there's no way to add logic to the Panel such as "animate on open, don't animate on update".

Solutions

None of these suggestions completely fix the problem.

  1. Render the NotificationsPanel outside of the UI Shell. But, when tabbing away from the last element of the Panel, the next focused element will be the top of the page instead of the next icon in the Shell.

  2. Do not use the <HeaderContainer>. It's only purpose seems to be to manage the state of the side panel and it doesn't render HTML to the page (HeaderContainer.tsx on GitHub).

without.header.container.mov

@RichKummer RichKummer added this to the 2025 Q1 milestone Dec 11, 2024
@elycheea elycheea changed the title While using keyboard strokes, using Tab key brings focus to all contents inside the panel, but it loops through the contents. There is no way to get out of the loop and move to the next navigation option in the header. Focus trap in NotificationPanel prevents user from navigating to next item in UIShell header Jan 21, 2025
@elycheea elycheea added the Sev 3 Visible or noticeable to users but does not impede functionality. Has a workaround. label Jan 21, 2025
@ljcarot ljcarot moved this from Needs refinement 🤓 to Backlog 🌋 in Carbon for IBM Products Jan 24, 2025
@ljcarot ljcarot moved this from Backlog 🌋 to In progress in Carbon for IBM Products Jan 29, 2025
@ljcarot ljcarot removed the Sev 3 Visible or noticeable to users but does not impede functionality. Has a workaround. label Jan 29, 2025
@paul-balchin-ibm paul-balchin-ibm moved this from In progress to Needs review 👋 in Carbon for IBM Products Feb 6, 2025
@github-project-automation github-project-automation bot moved this from Needs review 👋 to Done 🚀 in Carbon for IBM Products Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: NotificationsPanel role: dev type: enhancement 💡 New feature or request UX: consistency Issues leading to inconsistencies within one or across multiple experiences UX: issue Any type of experience feedback that once addressed improves or enhances the experience UX: usability Issues that impact task completion (prevents, slows down, introduces errors)
Projects
Status: Done 🚀
Development

Successfully merging a pull request may close this issue.

7 participants