-
Notifications
You must be signed in to change notification settings - Fork 158
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(left-nav): fix focus trapping #11842
fix(left-nav): fix focus trapping #11842
Conversation
) as HTMLElement | ||
)?.focus(); | ||
} | ||
} |
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.
While we change the value of this.expanded
here, we then immediately wait for the resolution of this.update
from the component's update/render cycle.
this.update
is already resolved from the last run of the component lifecycle and our current function hasn't been off the main thread to let the component start a new update.
Since we're awaiting a resolved promise, we immediate move on to try to focus on elements that are not yet in the DOM.
Deploy preview created for package Built with commit: ab93ccadbc7c75ccf5ec43e87736da1ddc012400 |
this.querySelector( | ||
(this.constructor as typeof CDSSideNav).selectorNavItems | ||
) as HTMLElement | ||
)?.focus(); |
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.
Moving the logic out of the event listener and into the update lifecycle ensures we focus after the seletorNavItems are added to the DOM.
await Promise.all([ | ||
import('./left-nav-name'), | ||
import('./left-nav-menu'), | ||
import('./left-nav-menu-section'), | ||
import('./left-nav-menu-item'), | ||
import('./left-nav-menu-category-heading'), | ||
import('./left-nav-overlay') | ||
]); |
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 don't want to await each of these in series, we should see a slight perf boost by awaiting in parallel.
import('./left-nav-menu-category-heading'); | ||
import('./left-nav-overlay'); | ||
this._importedSideNav = true; | ||
} |
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.
super.updated()
, called at the top of this function, is where we try to set focus. We have to make sure all our components are loaded before that, so this section is moved to willUpdate()
to prioritize loading in the components we need.
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.
Code changes are looking good, but CI and deploy previews are failing.
Deploy preview created for package Built with commit: ab93ccadbc7c75ccf5ec43e87736da1ddc012400 |
Deploy preview created for package Built with commit: ab93ccadbc7c75ccf5ec43e87736da1ddc012400 |
Deploy preview created for package Built with commit: ab93ccadbc7c75ccf5ec43e87736da1ddc012400 |
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.
Works beautifully! +1 from me
This is on hold pending a successful test of #11839's deploy preview CDN on ibm.com |
Deploy preview created for package Built with commit: ab93ccadbc7c75ccf5ec43e87736da1ddc012400 |
This is ready now. I tested successfully by doing a chrome devtools override and replacing |
5f824ec
into
carbon-design-system:main
Hey there! This issue/pull request was referenced in recently released v2.11.0. |
1 similar comment
Hey there! This issue/pull request was referenced in recently released v2.11.0. |
https://jsw.ibm.com/browse/ADCMS-5191 Fixes the focus trapping on the left-nav component **Changed** - Fixes the focus trapping on the left-nav component - Open the masthead story - Reduce viewport width to get the mobile version of the nav - Open the left-nav with the keyboard - Ensure focus moves to the first item in the left-nav on open - Ensure focus is trapped in the left-nav, cycling between the close button & last link/button element in both directions.
Related Ticket(s)
https://jsw.ibm.com/browse/ADCMS-5191
Description
Fixes the focus trapping on the left-nav component
Changelog
Changed
Testing Instructions