Skip to content

fix(left-nav): fix focus trapping #11842

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.

Sign up for GitHub

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

Merged
kodiakhq merged 9 commits into from
Jun 10, 2024
Merged

fix(left-nav): fix focus trapping #11842

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
887f078
fix(left-nav): wait for component to load, then focus in
andy-blum May 30, 2024
a840177
fix(left-nav): watch for correct event
andy-blum May 31, 2024
f295746
fix(left-nav): format & lint
andy-blum May 31, 2024
bf99f81
fix(left-nav): move import logic
andy-blum May 31, 2024
a8a09d9
fix(left-nav): fix focus trapping
andy-blum Jun 6, 2024
28464bb
Merge branch 'main' into fix/masthead-focus-trap-v2
andy-blum Jun 6, 2024
3f7ec94
Merge branch 'main' into fix/masthead-focus-trap-v2
andy-blum Jun 10, 2024
e54bfca
Merge branch 'main' into fix/masthead-focus-trap-v2
kennylam Jun 10, 2024
ab93cca
Merge branch 'main' into fix/masthead-focus-trap-v2
kennylam Jun 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 5 additions & 11 deletions packages/carbon-web-components/src/components/ui-shell/side-nav.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,17 +71,6 @@ class CDSSideNav extends HostListenerMixin(LitElement) {
// @ts-ignore: The decorator refers to this method but TS thinks this method is not referred to
private _handleButtonToggle = async (event: CustomEvent) => {
this.expanded = event.detail.active;
if (this.expanded) {
await this._updateAndTransitionPromise;
// Checks if the side nav is not collapsed during the animation
if (this.expanded) {
(
this.querySelector(
(this.constructor as typeof CDSSideNav).selectorNavItems
) as HTMLElement
)?.focus();
}
}
Copy link
Copy Markdown
Contributor Author

@andy-blum andy-blum May 31, 2024

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.

};

/**
Expand Down Expand Up @@ -179,6 +168,11 @@ class CDSSideNav extends HostListenerMixin(LitElement) {
forEach(headerItems, (item) => {
item.setAttribute('tabindex', '-1');
});
(
this.querySelector(
(this.constructor as typeof CDSSideNav).selectorNavItems
) as HTMLElement
)?.focus();
Copy link
Copy Markdown
Contributor Author

@andy-blum andy-blum May 31, 2024

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.

} else {
forEach(headerItems, (item) => {
item.removeAttribute('tabindex');
Expand Down
31 changes: 18 additions & 13 deletions packages/web-components/src/components/masthead/left-nav.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,11 @@ class C4DLeftNav extends StableSelectorMixin(CDSSideNav) {
private _importedSideNav = false;

/**
* Handles `c4d-request-focus-wrap` event on the document.
* Handles `cds-request-focus-wrap` event on the document dispatched from focuswrap.
*
* @param event The event.
*/
@HostListener('document:c4d-request-focus-wrap')
@HostListener('document:cds-request-focus-wrap')
// @ts-ignore: The decorator refers to this method but TS thinks this method is not referred to
private _handleRequestMenuButtonFocusWrap = (event: CustomEvent) => {
const { selectorButtonToggle } = this.constructor as typeof C4DLeftNav;
Expand Down Expand Up @@ -221,7 +221,21 @@ class C4DLeftNav extends StableSelectorMixin(CDSSideNav) {
document.addEventListener('click', this._handleClickOut.bind(this));
}

updated(changedProperties) {
protected async willUpdate(changedProperties) {
if (changedProperties.has('expanded') && !this._importedSideNav) {
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')
]);
Copy link
Copy Markdown
Contributor Author

@andy-blum andy-blum May 31, 2024

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.

this._importedSideNav = true;
}
}

async updated(changedProperties) {
super.updated(changedProperties);
const { usageMode } = this;
if (
Expand Down Expand Up @@ -256,15 +270,6 @@ class C4DLeftNav extends StableSelectorMixin(CDSSideNav) {
${c4dPrefix}-masthead-composite`
)
?.querySelector(`${c4dPrefix}-masthead`);
if (expanded && !this._importedSideNav) {
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');
this._importedSideNav = true;
}
Copy link
Copy Markdown
Contributor Author

@andy-blum andy-blum May 31, 2024

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.

if (expanded && masthead) {
this._hFocusWrap = focuswrap(this.shadowRoot!, [
startSentinelNode,
Expand Down Expand Up @@ -312,7 +317,7 @@ class C4DLeftNav extends StableSelectorMixin(CDSSideNav) {
}
}

private _renderSentinel = (side: String) => {
private _renderSentinel = (side: string) => {
return html`
<button
id="${side}-sentinel"
Expand Down
22 changes: 12 additions & 10 deletions packages/web-components/src/components/masthead/megamenu-top-nav-menu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,16 +179,18 @@ class C4DMegaMenuTopNavMenu extends C4DTopNavMenu {
if (this.expanded) {
// Import needed subcomponents on first expansion
if (!(this.parentElement as C4DTopNav)?.importedMegamenu) {
await import('./megamenu-left-navigation');
await import('./megamenu-category-link');
await import('./megamenu-category-link-group');
await import('./megamenu-category-group');
await import('./megamenu-category-group-copy');
await import('./megamenu-category-heading');
await import('./megamenu-link-with-icon');
await import('./megamenu-overlay');
await import('./megamenu-tab');
await import('./megamenu-tabs');
await Promise.all([
import('./megamenu-left-navigation'),
import('./megamenu-category-link'),
import('./megamenu-category-link-group'),
import('./megamenu-category-group'),
import('./megamenu-category-group-copy'),
import('./megamenu-category-heading'),
import('./megamenu-link-with-icon'),
import('./megamenu-overlay'),
import('./megamenu-tab'),
import('./megamenu-tabs')
]);
(this.parentElement as C4DTopNav).importedMegamenu = true;
}

Expand Down