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 all 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
44 changes: 6 additions & 38 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 @@ -10,7 +10,6 @@
import { LitElement, html } from 'lit';
import { property } from 'lit/decorators.js';
import { classMap } from 'lit/directives/class-map.js';
import on from '../../globals/mixins/on';
import { prefix } from '../../globals/settings';
import HostListenerMixin from '../../globals/mixins/host-listener';
import HostListener from '../../globals/decorators/host-listener';
Expand Down Expand Up @@ -43,18 +42,6 @@ class CDSSideNav extends HostListenerMixin(LitElement) {
*/
private _hTransition: Handle | null = null;

/**
* A promise that is resolved when the transition is complete.
*/
private _transitionPromise = Promise.resolve();

/**
* A promise that is resolved when the transition upon proprety update is complete.
*/
private get _updateAndTransitionPromise() {
return this.updateComplete.then(() => this._transitionPromise);
}

/**
* Cleans the handle for `transitionend` event listener.
*/
Expand All @@ -69,19 +56,8 @@ class CDSSideNav extends HostListenerMixin(LitElement) {
*/
@HostListener('parentRoot:eventButtonToggle')
// @ts-ignore: The decorator refers to this method but TS thinks this method is not referred to
private _handleButtonToggle = async (event: CustomEvent) => {
protected _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 @@ -138,19 +114,6 @@ class CDSSideNav extends HostListenerMixin(LitElement) {
super.disconnectedCallback();
}

shouldUpdate(changedProperties) {
if (changedProperties.has('expanded')) {
this._transitionPromise = new Promise((resolve) => {
this._cleanHTransition();
this._hTransition = on(this, 'transitionend', () => {
this._cleanHTransition();
resolve();
});
});
}
return true;
}

updated(changedProperties) {
const doc = this.getRootNode() as Document;
if (changedProperties.has('collapseMode')) {
Expand Down Expand Up @@ -179,6 +142,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
1 change: 0 additions & 1 deletion packages/web-components/src/components/masthead/left-nav-menu-item.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ class C4DLeftNavMenuItem extends CDSSideNavMenuItem {
return href
? html`
<a
tabindex="-1"
part="link"
class="${linkClasses}"
href="${href}"
Expand Down
47 changes: 0 additions & 47 deletions packages/web-components/src/components/masthead/left-nav-menu-section.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import ChevronLeft16 from '../../internal/vendor/@carbon/web-components/icons/ch
import FocusMixin from '../../internal/vendor/@carbon/web-components/globals/mixins/focus.js';
import { selectorTabbable } from '../../internal/vendor/@carbon/web-components/globals/settings.js';
import settings from '../../internal/vendor/@carbon/ibmdotcom-utilities/utilities/settings/settings';
import { forEach } from '../../globals/internal/collection-helpers';
import styles from './masthead.scss';
import C4DLeftNav from './left-nav';
import { carbonElement as customElement } from '../../internal/vendor/@carbon/web-components/globals/decorators/carbon-element.js';
Expand Down Expand Up @@ -177,36 +176,12 @@ class C4DLeftNavMenuSection extends HostListenerMixin(FocusMixin(LitElement)) {
}

async updated(changedProperties) {
// make sure leftNavMenuSection updates before setting the tabIndex's per item
await this._requestLeftNavMenuSectionUpdate();

if (changedProperties.has('expanded')) {
const { selectorNavMenu, selectorNavItem } = this
.constructor as typeof C4DLeftNavMenuSection;
const { expanded, isSubmenu } = this;

if (expanded) {
if (isSubmenu) {
const backBtn = this.shadowRoot?.querySelector('button');
if (backBtn) {
backBtn.tabIndex = 0;
}
}
forEach(this.querySelectorAll(selectorNavMenu), (elem) => {
const item = (elem as HTMLElement).shadowRoot?.querySelector(
'button'
);
if (item) {
item.tabIndex = 0;
}
});
forEach(this.querySelectorAll(selectorNavItem), (elem) => {
const item = (elem as HTMLElement).shadowRoot?.querySelector('a');
if (item) {
item.tabIndex = 0;
}
});

// set focus to first element of menu panel to allow for tabbing through the menu
let tabbable;
if (isSubmenu) {
Expand All @@ -228,27 +203,6 @@ class C4DLeftNavMenuSection extends HostListenerMixin(FocusMixin(LitElement)) {
{ once: true }
);
}
} else {
forEach(this.querySelectorAll(selectorNavMenu), (elem) => {
const item = (elem as HTMLElement).shadowRoot?.querySelector(
'button'
);
if (item) {
item.tabIndex = -1;
}
});
forEach(this.querySelectorAll(selectorNavItem), (elem) => {
const item = (elem as HTMLElement).shadowRoot?.querySelector('a');
if (item) {
item.tabIndex = -1;
}
});
if (isSubmenu) {
const backBtn = this.shadowRoot?.querySelector('button');
if (backBtn) {
backBtn.tabIndex = -1;
}
}
}
}
}
Expand Down Expand Up @@ -276,7 +230,6 @@ class C4DLeftNavMenuSection extends HostListenerMixin(FocusMixin(LitElement)) {
role="none">
<button
class="${prefix}--side-nav__link"
tabindex="-1"
@click="${handleClickBack}">
<span class="${prefix}--side-nav__link-text"
>${ChevronLeft16()}${backButtonText}</span
Expand Down
1 change: 0 additions & 1 deletion packages/web-components/src/components/masthead/left-nav-menu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,6 @@ class C4DLeftNavMenu extends FocusMixin(LitElement) {
<button
type="button"
aria-haspopup="true"
tabindex="-1"
aria-expanded="${expanded}"
class="${buttonClasses}"
@click=${handleClickExpando}
Expand Down
37 changes: 24 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,31 @@ class C4DLeftNav extends StableSelectorMixin(CDSSideNav) {
private _importedSideNav = false;

/**
* Handles `c4d-request-focus-wrap` event on the document.
* Handles `${prefix}-header-menu-button-toggle` event on the document.
*/
@HostListener('parentRoot:eventButtonToggle')
// @ts-ignore: The decorator refers to this method but TS thinks this method is not referred to
protected _handleButtonToggle = async (event: CustomEvent) => {
if (!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'),
]);
this._importedSideNav = true;
}
this.expanded = event.detail.active;
};

/**
* 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 +241,7 @@ class C4DLeftNav extends StableSelectorMixin(CDSSideNav) {
document.addEventListener('click', this._handleClickOut.bind(this));
}

updated(changedProperties) {
async updated(changedProperties) {
super.updated(changedProperties);
const { usageMode } = this;
if (
Expand Down Expand Up @@ -256,15 +276,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 +323,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
2 changes: 0 additions & 2 deletions packages/web-components/tests/snapshots/c4d-left-nav-menu.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
data-attribute1="headerNav"
data-attribute2="L0"
data-attribute3=""
tabindex="-1"
type="button"
>
<div class="cds--side-nav__submenu-content">
Expand All @@ -38,7 +37,6 @@
data-attribute1="headerNav"
data-attribute2="L0"
data-attribute3="title-foo"
tabindex="-1"
type="button"
>
<div class="cds--side-nav__submenu-content">
Expand Down