fix(left-nav): fix focus trapping#11842
fix(left-nav): fix focus trapping#11842kodiakhq[bot] merged 9 commits intocarbon-design-system:mainfrom
Conversation
andy-blum
requested review from
IgnacioBecerra and
sangeethababu9223
and removed request for
a team
| ) as HTMLElement | ||
| )?.focus(); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
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.
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.
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.
andy-blum
changed the title
jkaeser
left a comment
jkaeser
left a comment
There was a problem hiding this comment.
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 |
jkaeser
left a comment
jkaeser
left a comment
There was a problem hiding this comment.
Works beautifully! +1 from me
|
This is on hold pending a successful test of #11839's deploy preview CDN on ibm.com |
andy-blum
added
the
test: CDN preview
|
Deploy preview created for package Built with commit: ab93ccadbc7c75ccf5ec43e87736da1ddc012400 |
|
This is ready now. I tested successfully by doing a chrome devtools override and replacing |
kennylam
added
the
Ready to merge
|
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.
4 participants
Related Ticket(s)
https://jsw.ibm.com/browse/ADCMS-5191
Description
Fixes the focus trapping on the left-nav component
Changelog
Changed
Testing Instructions