Skip to content

AppSideNav: fix scrolling issue and being able to focus hidden links issue #2869

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

Merged
merged 17 commits into from
May 15, 2025
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/honest-experts-battle.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@hashicorp/design-system-components": patch
---

`AppSideNav` - Fixed bug where scrolling was blocked when the `AppSideNav` was expanded on desktop views. Also fixed bug which allowed user to focus links that were visually hidden.
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
/>
{{/if}}

<div class="hds-app-side-nav__wrapper-body">
<div class="hds-app-side-nav__wrapper-body" {{did-insert this.didInsertNavWrapperBody}}>
{{~yield~}}
</div>
</div>
Expand Down
40 changes: 24 additions & 16 deletions packages/components/src/components/hds/app-side-nav/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export default class HdsAppSideNav extends Component<HdsAppSideNavSignature> {
private _body!: HTMLElement;
private _bodyInitialOverflowValue = '';
private _desktopMQ: MediaQueryList;
private _containersToHide!: NodeListOf<Element>;
private _navWrapperBody!: HTMLElement;

// we use the `lg` breakpoint for `desktop` viewports, but consumers can override its value
private _desktopMQVal = this.args.breakpoint ?? hdsBreakpoints['lg'].px;
Expand Down Expand Up @@ -123,13 +123,11 @@ export default class HdsAppSideNav extends Component<HdsAppSideNavSignature> {
}

synchronizeInert(): void {
this._containersToHide?.forEach((element): void => {
if (this._isMinimized) {
element.setAttribute('inert', '');
} else {
element.removeAttribute('inert');
}
});
if (this._isMinimized) {
this._navWrapperBody?.setAttribute('inert', '');
} else {
this._navWrapperBody?.removeAttribute('inert');
}
}

lockBodyScroll(): void {
Expand Down Expand Up @@ -161,6 +159,7 @@ export default class HdsAppSideNav extends Component<HdsAppSideNavSignature> {
if (event.key === 'Escape' && !this._isMinimized && !this._isDesktop) {
this._isMinimized = true;
this.synchronizeInert();
this.unlockBodyScroll();
}
}

Expand All @@ -175,24 +174,28 @@ export default class HdsAppSideNav extends Component<HdsAppSideNavSignature> {
onToggleMinimizedStatus(this._isMinimized);
}

if (this._isMinimized) {
this.unlockBodyScroll();
} else {
this.lockBodyScroll();
if (!this._isDesktop) {
if (this._isMinimized) {
this.unlockBodyScroll();
} else {
this.lockBodyScroll();
}
}
}

@action
didInsert(element: HTMLElement): void {
this._containersToHide = element.querySelectorAll(
'.hds-app-side-nav-hide-when-minimized'
);
didInsert(): void {
this._body = document.body;
// Store the initial `overflow` value of `<body>` so we can reset to it
this._bodyInitialOverflowValue =
this._body.style.getPropertyValue('overflow');
}

@action
didInsertNavWrapperBody(element: HTMLElement): void {
this._navWrapperBody = element;
}

@action
setTransition(phase: string, event: TransitionEvent): void {
// we only want to respond to `width` animation/transitions
Expand All @@ -215,6 +218,11 @@ export default class HdsAppSideNav extends Component<HdsAppSideNavSignature> {

this.synchronizeInert();

if (this._isDesktop) {
// make sure scrolling is enabled if the user resizes the window from mobile to desktop
this.unlockBodyScroll();
}

const { onDesktopViewportChange } = this.args;

if (typeof onDesktopViewportChange === 'function') {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
@multiple={{true}}
@onChange={{this.panelsChanged}}
@name={{if @targetName @targetName "hds-app-side-nav-portal-target"}}
class="hds-app-side-nav__content-panels hds-app-side-nav-hide-when-minimized"
class="hds-app-side-nav__content-panels"
{{did-update this.didUpdateSubnav this._numSubnavs}}
/>
</div>
Original file line number Diff line number Diff line change
Expand Up @@ -102,12 +102,7 @@
overflow-y: auto;
}

// "HIDE-WHEN-MINIMIZED" SPECIAL CLASS
// this is a special class that is used to control which elements of the sidenav need to be:
// - hidden (immediately) when the sidenav is "minimized"
// - shown (transitioning their opacity) when the sidenav is "expanded"

.hds-app-side-nav-hide-when-minimized {
.hds-app-side-nav__wrapper-body {
.hds-app-side-nav--is-minimized & {
visibility: hidden !important; // we need `!important` here to override the inline style applied to the single panels
opacity: 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
resetOnerror,
settled,
triggerKeyEvent,
tab,
} from '@ember/test-helpers';
import { hbs } from 'ember-cli-htmlbars';

Expand Down Expand Up @@ -137,19 +138,24 @@ module('Integration | Component | hds/app-side-nav/index', function (hooks) {
hbs`<Hds::AppSideNav id='test-app-side-nav' @breakpoint='10000px' />`
);
assert.dom('#test-app-side-nav').hasClass('hds-app-side-nav--is-minimized');
assert.dom('body', document).doesNotHaveStyle('overflow');

await click('.hds-app-side-nav__toggle-button');
assert
.dom('#test-app-side-nav')
.hasClass('hds-app-side-nav--is-not-minimized');
assert.dom('body', document).hasStyle({
overflow: 'hidden',
});

await click('.hds-app-side-nav__toggle-button');
assert.dom('#test-app-side-nav').hasClass('hds-app-side-nav--is-minimized');
assert.dom('body', document).doesNotHaveStyle('overflow');
});

test('it collapses when the ESC key is pressed on narrow viewports', async function (assert) {
await render(hbs`<Hds::AppSideNav id='test-app-side-nav' @breakpoint='10000px'>
<span id='test-app-side-nav-body' />
<span class='hds-app-side-nav-hide-when-minimized' />
</Hds::AppSideNav>`);
assert.dom('#test-app-side-nav').hasClass('hds-app-side-nav--is-minimized');
await click('.hds-app-side-nav__toggle-button');
Expand All @@ -159,7 +165,7 @@ module('Integration | Component | hds/app-side-nav/index', function (hooks) {

await triggerKeyEvent('#test-app-side-nav', 'keydown', 'Escape');
assert.dom('#test-app-side-nav').hasClass('hds-app-side-nav--is-minimized');
assert.dom('.hds-app-side-nav-hide-when-minimized').hasAttribute('inert');
assert.dom('.hds-app-side-nav__wrapper-body').hasAttribute('inert');
});

// COLLAPSIBLE
Expand All @@ -184,7 +190,6 @@ module('Integration | Component | hds/app-side-nav/index', function (hooks) {
test('the "non-minimized" and "minimized" states have impact on its internal properties', async function (assert) {
await render(hbs`<Hds::AppSideNav @isCollapsible={{true}} id='test-app-side-nav'>
<span id='test-app-side-nav-body' />
<span class='hds-app-side-nav-hide-when-minimized' />
</Hds::AppSideNav>`);
assert
.dom('#test-app-side-nav')
Expand All @@ -195,10 +200,9 @@ module('Integration | Component | hds/app-side-nav/index', function (hooks) {
assert
.dom('.hds-app-side-nav__toggle-button .hds-icon')
.hasClass('hds-icon-chevrons-left');
assert
.dom('.hds-app-side-nav-hide-when-minimized')
.doesNotHaveAttribute('inert');
assert.dom('.hds-app-side-nav__wrapper-body').doesNotHaveAttribute('inert');
assert.dom('#test-app-side-nav-body').doesNotHaveAttribute('inert');
assert.dom('body', document).doesNotHaveStyle('overflow');

await click('.hds-app-side-nav__toggle-button');

Expand All @@ -209,8 +213,8 @@ module('Integration | Component | hds/app-side-nav/index', function (hooks) {
assert
.dom('.hds-app-side-nav__toggle-button .hds-icon')
.hasClass('hds-icon-chevrons-right');
assert.dom('.hds-app-side-nav-hide-when-minimized').hasAttribute('inert');
assert.dom('#test-app-side-nav-body').doesNotHaveAttribute('inert');
assert.dom('.hds-app-side-nav__wrapper-body').hasAttribute('inert');
assert.dom('body', document).doesNotHaveStyle('overflow');
});

test('when the viewport changes from desktop to mobile, it automatically collapses and becomes inert', async function (assert) {
Expand All @@ -226,7 +230,6 @@ module('Integration | Component | hds/app-side-nav/index', function (hooks) {
@onDesktopViewportChange={{this.onDesktopViewportChange}}
>
<span id='test-app-side-nav-body' />
<span class='hds-app-side-nav-hide-when-minimized' />
</Hds::AppSideNav>`);

assert.strictEqual(calls.length, 1, 'called with initial viewport');
Expand All @@ -238,7 +241,7 @@ module('Integration | Component | hds/app-side-nav/index', function (hooks) {
'resizing to mobile triggers a false event'
);

assert.dom('.hds-app-side-nav-hide-when-minimized').hasAttribute('inert');
assert.dom('.hds-app-side-nav__wrapper-body').hasAttribute('inert');
});

test('when collapsed and the viewport changes from mobile to desktop, it automatically expands and is no longer inert', async function (assert) {
Expand All @@ -254,29 +257,81 @@ module('Integration | Component | hds/app-side-nav/index', function (hooks) {
@onDesktopViewportChange={{this.onDesktopViewportChange}}
>
<span id='test-app-side-nav-body' />
<span class='hds-app-side-nav-hide-when-minimized' />
</Hds::AppSideNav>`);

await click('.hds-app-side-nav__toggle-button');
assert.dom('.hds-app-side-nav-hide-when-minimized').hasAttribute('inert');
assert.dom('.hds-app-side-nav__wrapper-body').hasAttribute('inert');

await this.changeBrowserSize(false);
assert.deepEqual(
calls[1],
[false],
'resizing to mobile triggers a false event'
);
assert.dom('.hds-app-side-nav-hide-when-minimized').hasAttribute('inert');
assert.dom('.hds-app-side-nav__wrapper-body').hasAttribute('inert');

await this.changeBrowserSize(true);
assert.deepEqual(
calls[2],
[true],
'resizing to desktop triggers a true event'
);
assert
.dom('.hds-app-side-nav-hide-when-minimized')
.doesNotHaveAttribute('inert');
assert.dom('.hds-app-side-nav__wrapper-body').doesNotHaveAttribute('inert');
assert.dom('body', document).doesNotHaveStyle('overflow');
});

test('when collapsed and the viewport changes from mobile to desktop and is expanded, scrolling is enabled', async function (assert) {
this.mockMedia();

let calls = [];
this.setProperties({
onDesktopViewportChange: (...args) => calls.push(args),
});

await render(hbs`<Hds::AppSideNav
@isCollapsible={{true}}
@onDesktopViewportChange={{this.onDesktopViewportChange}}
>
<span id='test-app-side-nav-body' />
</Hds::AppSideNav>`);
await this.changeBrowserSize(false);
assert.deepEqual(
calls[1],
[false],
'resizing to mobile triggers a false event'
);

await click('.hds-app-side-nav__toggle-button');

assert.dom('body', document).hasStyle({
overflow: 'hidden',
});

await this.changeBrowserSize(true);
assert.deepEqual(
calls[2],
[true],
'resizing to desktop triggers a true event'
);

assert.dom('body', document).doesNotHaveStyle('overflow');
});

test('when collapsed, the content in the AppSideNav is not focusable', async function (assert) {
await render(hbs`<Hds::AppSideNav
id='test-app-side-nav'
@isCollapsible={{true}}
@onDesktopViewportChange={{this.onDesktopViewportChange}}
>
<span id='test-app-side-nav-body' />
</Hds::AppSideNav><button id='button-2'>Click</button>`);

await click('.hds-app-side-nav__toggle-button');
assert.dom('#test-app-side-nav').hasClass('hds-app-side-nav--is-minimized');
assert.dom('.hds-app-side-nav__toggle-button').isFocused();

await tab();
assert.dom('#button-2').isFocused();
});

// CALLBACKS
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* SPDX-License-Identifier: MPL-2.0
*/

import { module, test, skip } from 'qunit';
import { module, test } from 'qunit';
import { setupRenderingTest } from 'ember-qunit';
import {
click,
Expand Down Expand Up @@ -231,8 +231,7 @@ module('Integration | Component | hds/modal/index', function (hooks) {
assert.dom('#test-button').isFocused();
});

// not sure how to reach the `body` element, it says "body is not a valid root element"
skip('it returns focus to the `body` element, if the one that initiated the open event not anymore in the DOM', async function (assert) {
test('it returns focus to the `body` element, if the one that initiated the open event not anymore in the DOM', async function (assert) {
await render(
hbs`<Hds::Dropdown as |D|>
<D.ToggleButton id="test-toggle" @text="open modal" />
Expand All @@ -249,7 +248,7 @@ module('Integration | Component | hds/modal/index', function (hooks) {
await click('#test-interactive');
assert.true(this.showModal);
await click('button.hds-modal__dismiss');
assert.dom('body', 'body').isFocused();
assert.dom('body', document).isFocused();
});

test('it returns focus to a specific element if provided via`@returnFocusTo`', async function (assert) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,19 +201,15 @@ Each one of these states has CSS class names associated, and they’re used by t

The App Side Nav component **automatically**:

- fades in/out the “actions” block in the header and the content injected in the body via “portals”.
- fades in/out the content in the navigation
- swaps the toggle button icon from “menu” to “close” and moves it from one position to another

Any other content in the App Side Nav needs to be **explicitly handled** by the consumers (in this way they have full control of the content they add, and they can customize the transition as they want/need).

One possible way to do it is to use the **`hds-app-side-nav-hide-when-minimized` class**. This is a special class that can be applied to a DOM element so that it **automatically** fades in/out when the App Side Nav changes its “minimization” state.

More specifically:
More specifically, the animation is:

- `minimized → maximized` transition: the content appears with a fade-in effect, when the width animation is already completed (the width is maximized)
- `maximized → minimized` transition: the content disappears at once with no transition, before the width animation starts

Another option is to use the **`isMinimized` parameter**, which is useful in those cases where the content is so custom/specialized that it can’t just be faded in/out but needs to have a different kind of transition (eg. remain visible but change layout or respond to the width of the container). This value is passed down by the `<:header/body/footer>` named blocks as parameters, and can be used to build custom logic on the consumers’ side.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The component doesn't return named blocks or the isMinimized state to the consumer, so this is not true.

Any other content in the App Side Nav needs to be **explicitly handled** by the consumers (in this way they have full control of the content they add, and they can customize the transition as they want/need).

#### Advanced customization

Expand Down