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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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 @@ -10,7 +10,7 @@
{{on "transitionend" (fn this.setTransition "end")}}
{{! @glint-expect-error - https://github.com/josemarluedke/ember-focus-trap/issues/86 }}
{{focus-trap isActive=this.shouldTrapFocus}}
{{did-insert this.didInsert}}
{{this._setUpBodyElement}}
>
<h2 class="sr-only" id="hds-app-side-nav-header">Application local navigation</h2>

Expand All @@ -27,7 +27,7 @@
/>
{{/if}}

<div class="hds-app-side-nav__wrapper-body">
<div class="hds-app-side-nav__wrapper-body" {{this._setUpNavWrapperBody}}>
{{~yield~}}
</div>
</div>
Expand Down
53 changes: 30 additions & 23 deletions packages/components/src/components/hds/app-side-nav/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { tracked } from '@glimmer/tracking';
import { action } from '@ember/object';
import { registerDestructor } from '@ember/destroyable';
import type Owner from '@ember/owner';
import { modifier } from 'ember-modifier';

import { hdsBreakpoints } from '@hashicorp/design-system-components/utils/hds-breakpoints';

Expand All @@ -34,7 +35,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 All @@ -49,6 +50,17 @@ export default class HdsAppSideNav extends Component<HdsAppSideNavSignature> {
});
}

private _setUpBodyElement = modifier(() => {
this._body = document.body;
// Store the initial `overflow` value of `<body>` so we can reset to it
this._bodyInitialOverflowValue =
this._body.style.getPropertyValue('overflow');
});

private _setUpNavWrapperBody = modifier((element: HTMLElement) => {
this._navWrapperBody = element;
});

addEventListeners(): void {
// eslint-disable-next-line @typescript-eslint/unbound-method
document.addEventListener('keydown', this.escapePress, true);
Expand Down Expand Up @@ -123,13 +135,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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[question/suggestion?] MDN says that "The inert [...] attribute is a Boolean attribute". Does it mean this could be converted to this._navWrapperBody?.setAttribute('inert', this._isMinimized);? (in case, test that it works, I didn't)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested in chrome and it treated inert="false" as inert still, will leave it as is for now.

this._navWrapperBody?.setAttribute('inert', '');
} else {
this._navWrapperBody?.removeAttribute('inert');
}
}

lockBodyScroll(): void {
Expand Down Expand Up @@ -161,6 +171,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 +186,15 @@ 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'
);
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
setTransition(phase: string, event: TransitionEvent): void {
// we only want to respond to `width` animation/transitions
Expand All @@ -215,6 +217,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
Copy link
Contributor

Choose a reason for hiding this comment

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

[praise] 👏 👏 👏

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,20 +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>`);
await render(hbs`<Hds::AppSideNav id='test-app-side-nav' @breakpoint='10000px'
/>`);
assert.dom('#test-app-side-nav').hasClass('hds-app-side-nav--is-minimized');
await click('.hds-app-side-nav__toggle-button');
assert
Expand All @@ -159,7 +164,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 +189,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 +199,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 +212,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 @@ -224,10 +227,7 @@ module('Integration | Component | hds/app-side-nav/index', function (hooks) {
await render(hbs`<Hds::AppSideNav
@isCollapsible={{true}}
@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 +238,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 @@ -252,31 +252,78 @@ module('Integration | Component | hds/app-side-nav/index', function (hooks) {
await render(hbs`<Hds::AppSideNav
@isCollapsible={{true}}
@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}}
/>`);
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[question] I don't understand this test: how do you know if the content in the AppSideNav is focusable or not, if there's nothing focusable (like a button) inside it? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh my gosh, that is right. I will update.

await render(hbs`<Hds::AppSideNav
id='test-app-side-nav'
@isCollapsible={{true}}
>
<button id='button-inside'>Click</button>
</Hds::AppSideNav><button id='button-outside'>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-outside').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();
Copy link
Contributor

Choose a reason for hiding this comment

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

😮

});

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