Skip to content

Commit 50afd67

Browse files
shleewhiteKristinLBradleydidoo
authored
AppSideNav: fix scrolling issue and being able to focus hidden links issue (#2869)
Co-authored-by: Kristin Bradley <[email protected]> Co-authored-by: Cristiano Rastelli <[email protected]>
1 parent f474b19 commit 50afd67

File tree

8 files changed

+117
-68
lines changed

8 files changed

+117
-68
lines changed

‎.changeset/honest-experts-battle.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@hashicorp/design-system-components": patch
3+
---
4+
5+
`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.

‎packages/components/src/components/hds/app-side-nav/index.hbs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
{{on "transitionend" (fn this.setTransition "end")}}
1111
{{! @glint-expect-error - https://github.com/josemarluedke/ember-focus-trap/issues/86 }}
1212
{{focus-trap isActive=this.shouldTrapFocus}}
13-
{{did-insert this.didInsert}}
13+
{{this._setUpBodyElement}}
1414
>
1515
<h2 class="sr-only" id="hds-app-side-nav-header">Application local navigation</h2>
1616

@@ -27,7 +27,7 @@
2727
/>
2828
{{/if}}
2929

30-
<div class="hds-app-side-nav__wrapper-body">
30+
<div class="hds-app-side-nav__wrapper-body" {{this._setUpNavWrapperBody}}>
3131
{{~yield~}}
3232
</div>
3333
</div>

‎packages/components/src/components/hds/app-side-nav/index.ts‎

Lines changed: 30 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { tracked } from '@glimmer/tracking';
88
import { action } from '@ember/object';
99
import { registerDestructor } from '@ember/destroyable';
1010
import type Owner from '@ember/owner';
11+
import { modifier } from 'ember-modifier';
1112

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

@@ -34,7 +35,7 @@ export default class HdsAppSideNav extends Component<HdsAppSideNavSignature> {
3435
private _body!: HTMLElement;
3536
private _bodyInitialOverflowValue = '';
3637
private _desktopMQ: MediaQueryList;
37-
private _containersToHide!: NodeListOf<Element>;
38+
private _navWrapperBody!: HTMLElement;
3839

3940
// we use the `lg` breakpoint for `desktop` viewports, but consumers can override its value
4041
private _desktopMQVal = this.args.breakpoint ?? hdsBreakpoints['lg'].px;
@@ -49,6 +50,17 @@ export default class HdsAppSideNav extends Component<HdsAppSideNavSignature> {
4950
});
5051
}
5152

53+
private _setUpBodyElement = modifier(() => {
54+
this._body = document.body;
55+
// Store the initial `overflow` value of `<body>` so we can reset to it
56+
this._bodyInitialOverflowValue =
57+
this._body.style.getPropertyValue('overflow');
58+
});
59+
60+
private _setUpNavWrapperBody = modifier((element: HTMLElement) => {
61+
this._navWrapperBody = element;
62+
});
63+
5264
addEventListeners(): void {
5365
// eslint-disable-next-line @typescript-eslint/unbound-method
5466
document.addEventListener('keydown', this.escapePress, true);
@@ -123,13 +135,11 @@ export default class HdsAppSideNav extends Component<HdsAppSideNavSignature> {
123135
}
124136

125137
synchronizeInert(): void {
126-
this._containersToHide?.forEach((element): void => {
127-
if (this._isMinimized) {
128-
element.setAttribute('inert', '');
129-
} else {
130-
element.removeAttribute('inert');
131-
}
132-
});
138+
if (this._isMinimized) {
139+
this._navWrapperBody?.setAttribute('inert', '');
140+
} else {
141+
this._navWrapperBody?.removeAttribute('inert');
142+
}
133143
}
134144

135145
lockBodyScroll(): void {
@@ -161,6 +171,7 @@ export default class HdsAppSideNav extends Component<HdsAppSideNavSignature> {
161171
if (event.key === 'Escape' && !this._isMinimized && !this._isDesktop) {
162172
this._isMinimized = true;
163173
this.synchronizeInert();
174+
this.unlockBodyScroll();
164175
}
165176
}
166177

@@ -175,24 +186,15 @@ export default class HdsAppSideNav extends Component<HdsAppSideNavSignature> {
175186
onToggleMinimizedStatus(this._isMinimized);
176187
}
177188

178-
if (this._isMinimized) {
179-
this.unlockBodyScroll();
180-
} else {
181-
this.lockBodyScroll();
189+
if (!this._isDesktop) {
190+
if (this._isMinimized) {
191+
this.unlockBodyScroll();
192+
} else {
193+
this.lockBodyScroll();
194+
}
182195
}
183196
}
184197

185-
@action
186-
didInsert(element: HTMLElement): void {
187-
this._containersToHide = element.querySelectorAll(
188-
'.hds-app-side-nav-hide-when-minimized'
189-
);
190-
this._body = document.body;
191-
// Store the initial `overflow` value of `<body>` so we can reset to it
192-
this._bodyInitialOverflowValue =
193-
this._body.style.getPropertyValue('overflow');
194-
}
195-
196198
@action
197199
setTransition(phase: string, event: TransitionEvent): void {
198200
// we only want to respond to `width` animation/transitions
@@ -215,6 +217,11 @@ export default class HdsAppSideNav extends Component<HdsAppSideNavSignature> {
215217

216218
this.synchronizeInert();
217219

220+
if (this._isDesktop) {
221+
// make sure scrolling is enabled if the user resizes the window from mobile to desktop
222+
this.unlockBodyScroll();
223+
}
224+
218225
const { onDesktopViewportChange } = this.args;
219226

220227
if (typeof onDesktopViewportChange === 'function') {

‎packages/components/src/components/hds/app-side-nav/portal/target.hbs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
@multiple={{true}}
99
@onChange={{this.panelsChanged}}
1010
@name={{if @targetName @targetName "hds-app-side-nav-portal-target"}}
11-
class="hds-app-side-nav__content-panels hds-app-side-nav-hide-when-minimized"
11+
class="hds-app-side-nav__content-panels"
1212
{{did-update this.didUpdateSubnav this._numSubnavs}}
1313
/>
1414
</div>

‎packages/components/src/styles/components/app-side-nav/main.scss‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -102,12 +102,7 @@
102102
overflow-y: auto;
103103
}
104104

105-
// "HIDE-WHEN-MINIMIZED" SPECIAL CLASS
106-
// this is a special class that is used to control which elements of the sidenav need to be:
107-
// - hidden (immediately) when the sidenav is "minimized"
108-
// - shown (transitioning their opacity) when the sidenav is "expanded"
109-
110-
.hds-app-side-nav-hide-when-minimized {
105+
.hds-app-side-nav__wrapper-body {
111106
.hds-app-side-nav--is-minimized & {
112107
visibility: hidden !important; // we need `!important` here to override the inline style applied to the single panels
113108
opacity: 0;

‎showcase/tests/integration/components/hds/app-side-nav/index-test.js‎

Lines changed: 72 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
resetOnerror,
1212
settled,
1313
triggerKeyEvent,
14+
tab,
1415
} from '@ember/test-helpers';
1516
import { hbs } from 'ember-cli-htmlbars';
1617

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

141143
await click('.hds-app-side-nav__toggle-button');
142144
assert
143145
.dom('#test-app-side-nav')
144146
.hasClass('hds-app-side-nav--is-not-minimized');
147+
assert.dom('body', document).hasStyle({
148+
overflow: 'hidden',
149+
});
150+
145151
await click('.hds-app-side-nav__toggle-button');
146152
assert.dom('#test-app-side-nav').hasClass('hds-app-side-nav--is-minimized');
153+
assert.dom('body', document).doesNotHaveStyle('overflow');
147154
});
148155

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

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

165170
// COLLAPSIBLE
@@ -184,7 +189,6 @@ module('Integration | Component | hds/app-side-nav/index', function (hooks) {
184189
test('the "non-minimized" and "minimized" states have impact on its internal properties', async function (assert) {
185190
await render(hbs`<Hds::AppSideNav @isCollapsible={{true}} id='test-app-side-nav'>
186191
<span id='test-app-side-nav-body' />
187-
<span class='hds-app-side-nav-hide-when-minimized' />
188192
</Hds::AppSideNav>`);
189193
assert
190194
.dom('#test-app-side-nav')
@@ -195,10 +199,9 @@ module('Integration | Component | hds/app-side-nav/index', function (hooks) {
195199
assert
196200
.dom('.hds-app-side-nav__toggle-button .hds-icon')
197201
.hasClass('hds-icon-chevrons-left');
198-
assert
199-
.dom('.hds-app-side-nav-hide-when-minimized')
200-
.doesNotHaveAttribute('inert');
202+
assert.dom('.hds-app-side-nav__wrapper-body').doesNotHaveAttribute('inert');
201203
assert.dom('#test-app-side-nav-body').doesNotHaveAttribute('inert');
204+
assert.dom('body', document).doesNotHaveStyle('overflow');
202205

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

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

216219
test('when the viewport changes from desktop to mobile, it automatically collapses and becomes inert', async function (assert) {
@@ -224,10 +227,7 @@ module('Integration | Component | hds/app-side-nav/index', function (hooks) {
224227
await render(hbs`<Hds::AppSideNav
225228
@isCollapsible={{true}}
226229
@onDesktopViewportChange={{this.onDesktopViewportChange}}
227-
>
228-
<span id='test-app-side-nav-body' />
229-
<span class='hds-app-side-nav-hide-when-minimized' />
230-
</Hds::AppSideNav>`);
230+
/>`);
231231

232232
assert.strictEqual(calls.length, 1, 'called with initial viewport');
233233

@@ -238,7 +238,7 @@ module('Integration | Component | hds/app-side-nav/index', function (hooks) {
238238
'resizing to mobile triggers a false event'
239239
);
240240

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

244244
test('when collapsed and the viewport changes from mobile to desktop, it automatically expands and is no longer inert', async function (assert) {
@@ -252,31 +252,78 @@ module('Integration | Component | hds/app-side-nav/index', function (hooks) {
252252
await render(hbs`<Hds::AppSideNav
253253
@isCollapsible={{true}}
254254
@onDesktopViewportChange={{this.onDesktopViewportChange}}
255-
>
256-
<span id='test-app-side-nav-body' />
257-
<span class='hds-app-side-nav-hide-when-minimized' />
258-
</Hds::AppSideNav>`);
255+
/>`);
259256

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

263260
await this.changeBrowserSize(false);
264261
assert.deepEqual(
265262
calls[1],
266263
[false],
267264
'resizing to mobile triggers a false event'
268265
);
269-
assert.dom('.hds-app-side-nav-hide-when-minimized').hasAttribute('inert');
266+
assert.dom('.hds-app-side-nav__wrapper-body').hasAttribute('inert');
270267

271268
await this.changeBrowserSize(true);
272269
assert.deepEqual(
273270
calls[2],
274271
[true],
275272
'resizing to desktop triggers a true event'
276273
);
277-
assert
278-
.dom('.hds-app-side-nav-hide-when-minimized')
279-
.doesNotHaveAttribute('inert');
274+
assert.dom('.hds-app-side-nav__wrapper-body').doesNotHaveAttribute('inert');
275+
assert.dom('body', document).doesNotHaveStyle('overflow');
276+
});
277+
278+
test('when collapsed and the viewport changes from mobile to desktop and is expanded, scrolling is enabled', async function (assert) {
279+
this.mockMedia();
280+
281+
let calls = [];
282+
this.setProperties({
283+
onDesktopViewportChange: (...args) => calls.push(args),
284+
});
285+
286+
await render(hbs`<Hds::AppSideNav
287+
@isCollapsible={{true}}
288+
@onDesktopViewportChange={{this.onDesktopViewportChange}}
289+
/>`);
290+
await this.changeBrowserSize(false);
291+
assert.deepEqual(
292+
calls[1],
293+
[false],
294+
'resizing to mobile triggers a false event'
295+
);
296+
297+
await click('.hds-app-side-nav__toggle-button');
298+
299+
assert.dom('body', document).hasStyle({
300+
overflow: 'hidden',
301+
});
302+
303+
await this.changeBrowserSize(true);
304+
assert.deepEqual(
305+
calls[2],
306+
[true],
307+
'resizing to desktop triggers a true event'
308+
);
309+
310+
assert.dom('body', document).doesNotHaveStyle('overflow');
311+
});
312+
313+
test('when collapsed, the content in the AppSideNav is not focusable', async function (assert) {
314+
await render(hbs`<Hds::AppSideNav
315+
id='test-app-side-nav'
316+
@isCollapsible={{true}}
317+
>
318+
<button id='button-inside'>Click</button>
319+
</Hds::AppSideNav><button id='button-outside'>Click</button>`);
320+
321+
await click('.hds-app-side-nav__toggle-button');
322+
assert.dom('#test-app-side-nav').hasClass('hds-app-side-nav--is-minimized');
323+
assert.dom('.hds-app-side-nav__toggle-button').isFocused();
324+
325+
await tab();
326+
assert.dom('#button-outside').isFocused();
280327
});
281328

282329
// CALLBACKS

‎showcase/tests/integration/components/hds/modal/index-test.js‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* SPDX-License-Identifier: MPL-2.0
44
*/
55

6-
import { module, test, skip } from 'qunit';
6+
import { module, test } from 'qunit';
77
import { setupRenderingTest } from 'ember-qunit';
88
import {
99
click,
@@ -231,8 +231,7 @@ module('Integration | Component | hds/modal/index', function (hooks) {
231231
assert.dom('#test-button').isFocused();
232232
});
233233

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

255254
test('it returns focus to a specific element if provided via`@returnFocusTo`', async function (assert) {

‎website/docs/components/app-side-nav/partials/code/how-to-use.md‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -201,19 +201,15 @@ Each one of these states has CSS class names associated, and they’re used by t
201201

202202
The App Side Nav component **automatically**:
203203

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

207-
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).
208-
209-
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.
210-
211-
More specifically:
207+
More specifically, the animation is:
212208

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

216-
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.
212+
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).
217213

218214
#### Advanced customization
219215

0 commit comments

Comments
 (0)