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

fix(NotificationsPanel): remove focus trap #6829

Conversation

paul-balchin-ibm
Copy link
Contributor

@paul-balchin-ibm paul-balchin-ibm commented Jan 31, 2025

Closes #6432

For accessibility reasons: remove the built-in focus trap.

After detailed discussions (see also the refinement comment), the following changes were approved:

  1. Remove the focus trap entirely.
  2. Solve why the Panel "flickers" when it's open and a new notification is added.
  3. Keep the feature: "on click outside / tap escape key → close the Panel."

What did you change?

packages/ibm-products-styles/src/components/NotificationsPanel/_notifications-panel.scss
packages/ibm-products/src/components/NotificationsPanel/NotificationsPanel.stories.jsx
packages/ibm-products/src/components/NotificationsPanel/NotificationsPanel.tsx
packages/ibm-products/src/components/NotificationsPanel/NotificationsPanel_data.js
packages/ibm-products/src/components/NotificationsPanel/_storybook-styles.scss

How did you test and verify your work?

  • Storybook
  • Coverage / testing

@paul-balchin-ibm paul-balchin-ibm requested a review from a team as a code owner January 31, 2025 19:14
@paul-balchin-ibm paul-balchin-ibm requested review from AlexanderMelox and amal-k-joy and removed request for a team January 31, 2025 19:14
@paul-balchin-ibm paul-balchin-ibm marked this pull request as draft January 31, 2025 19:14
Copy link

netlify bot commented Jan 31, 2025

Deploy Preview for ibm-products-web-components ready!

Name Link
🔨 Latest commit 115f84f
🔍 Latest deploy log https://app.netlify.com/sites/ibm-products-web-components/deploys/67a518ea71e1630008fe7962
😎 Deploy Preview https://deploy-preview-6829--ibm-products-web-components.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jan 31, 2025

Deploy Preview for carbon-for-ibm-products ready!

Name Link
🔨 Latest commit 115f84f
🔍 Latest deploy log https://app.netlify.com/sites/carbon-for-ibm-products/deploys/67a518ea65d6fa0008e9f903
😎 Deploy Preview https://deploy-preview-6829--carbon-for-ibm-products.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Jan 31, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.29%. Comparing base (44a206d) to head (115f84f).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6829      +/-   ##
==========================================
- Coverage   81.31%   81.29%   -0.02%     
==========================================
  Files         397      397              
  Lines       12959    12945      -14     
  Branches     4261     4255       -6     
==========================================
- Hits        10537    10524      -13     
+ Misses       2422     2421       -1     
Components Coverage Δ
ibm-products ∅ <ø> (∅)
ibm-products-web-components ∅ <ø> (∅)

Comment on lines 128 to 129
{/* <HeaderContainer
render={() => ( */}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temp fix for draft review.

Removing the HeaderContainer:

  1. Solves the problem of re-rendering all children every time there's a state change.
  2. Solves the problem of tabbing order by allowing the Panel to be rendered inline with the Shell's notification icon button.

Keeping the HeaderContainer:

  1. Requires the Panel to be rendered outside the Shell, but it will lose the expected tabbing order.

Copy link
Member

@matthewgallo matthewgallo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few questions for you, I noticed that the exit animation of the panel is gone. Is that something you can check out? Also, triggerButtonRef isn't used at all anymore. Thoughts on this? This is used across modals, panels in Carbon, but since we removed focus trap, suppose we might be able to deprecate it?

@paul-balchin-ibm
Copy link
Contributor Author

paul-balchin-ibm commented Feb 5, 2025

...the triggerButtonRef is gone.

Oops. That was probably part of my aggressive "delete one thing at a time until I see a change in behavior" tactic; and then forgot to put them back in. (All for nothing, it was the UI Shell that was the source of the issues.)

...the exit animation is gone.

If you mean onAnimationEnd, it hasn't been removed.

Thanks, @matthewgallo.

@matthewgallo
Copy link
Member

The actual exit animation of the panel is missing now. This is what is currently looks like with the latest changes from main.

Screen.Recording.2025-02-06.at.9.17.38.AM.mov

@paul-balchin-ibm paul-balchin-ibm marked this pull request as ready for review February 6, 2025 19:14
@paul-balchin-ibm
Copy link
Contributor Author

The actual exit animation of the panel is missing now

Fixed with latest commit.

Removed tests that confirmed focus trapping.
Copy link
Contributor

@elycheea elycheea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @paul-balchin-ibm! Those alignment fixes you snuck in and Storybook cleanup have long overdue. 🚀

@elycheea elycheea added this pull request to the merge queue Feb 7, 2025
Merged via the queue into carbon-design-system:main with commit ccd0926 Feb 7, 2025
33 checks passed
@devadula-nandan
Copy link
Contributor

devadula-nandan commented Feb 7, 2025

hi @paul-balchin-ibm,
i remember fixing the notification panel flicker issue
#4771

but looks like it got resurrected from the grave after moving them to useIsomorphicEffect as part of fixing csp violations by removing inline css. #6340

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Focus trap in NotificationPanel prevents user from navigating to next item in UIShell header
4 participants