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

shleewhite · 2025-05-12T18:26:21Z

📌 Summary

If merged, this PR would do some cleanup of the AppSideNav component

only lock scrolling if the user is on mobile (when on desktop, scrolling shouldnt be prevented because the user can still interact with the rest of the page)
reset ability to scroll when close the AppSideNav with the escape key
prevent the user from tabbing to the body of the AppSideNav when it is minimized

There are also updates to tests to check for the correct behavior (+ 1 bonus fix for a Modal test):

add test to check the user cannot tab to the content when the AppSideNav is minimized
add checks to existing tests that overflow: hidden is set appropriately
add test that when a user resizes from mobile -> desktop, ability to scroll is returned
re-enable test for Modal because I figured out how to target the body element in tests

🧪 Tested in Atlas:

🔗 External links

Jira ticket: HDS-4827
Jira ticket: HDS-4858

👀 Component checklist

Percy was checked for any visual regression
A changelog entry was added via Changesets if needed (see templates here)

💬 Please consider using conventional comments when reviewing this PR.

vercel · 2025-05-12T18:26:25Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
hds-showcase	✅ Ready (Inspect)	Visit Preview	May 15, 2025 0:36am
hds-website	✅ Ready (Inspect)	Visit Preview	May 15, 2025 0:36am

shleewhite · 2025-05-12T18:28:45Z

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

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

-    <div class="hds-app-side-nav__wrapper-body">
+    <div class="hds-app-side-nav__wrapper-body hds-app-side-nav-hide-when-minimized">


Note: I had to add this class because this is what the component is checking to decide what to add inert to. Previously, this was only getting added to the target for the portal (link).

Now it gets added when the consumer isn't using Portals too.

This seems a little confusing since the 2nd class is always present. Could you just use the existing hds-app-side-nav__wrapper-body class name instead?

(Also BEM format would be "hds-app-side-nav--hide-when-minimized")

Yeah.. I think this comes from SideNav. I can update it to be BEM format.

When the AppSideNav is inserted, it looks for elements with this class name to decide where to put inert.

I didn't want to change how it works for Portals, which is why I kept using the existing class.

The class is also used a lot in tests.

It still seems a bit confusing to add an extra class that's always there instead of just using the existing class. Would be curious to know what other engineers think though. @hashicorp/hds-engineering @didoo

Maybe you could at least add a comment to explain why the extra class is there?

I'll have a look tomorrow

github-actions · 2025-05-13T14:21:21Z

📦 RC Packages Published

Latest commit: 3dd5918

Published 1 packages

@hashicorp/[email protected]

yarn up -C @hashicorp/design-system-components@rc

.changeset/honest-experts-battle.md

shleewhite · 2025-05-13T18:11:35Z

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


 More specifically:

 - `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.


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

KristinLBradley · 2025-05-13T18:34:00Z

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

@@ -206,15 +206,13 @@ The App Side Nav component **automatically**:

 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.
+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.


But this class is always present now, isn't it?

Yes... Ok I have looked into it more and I believe we can safely simplify this logic. I will push up a refactor shortly.

KristinLBradley

Looking good overall but I had a few questions still (see comments)

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

Co-authored-by: Kristin Bradley <[email protected]>

…ide-nav--hide-when-minimized

didoo

@shleewhite left a bunch of comments/questions, but overall the PR is so much better and clean with your latest changes! 🙌

didoo · 2025-05-14T21:08:01Z

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

@@ -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}}>


[suggestion] my understanding is that we're moving away from ember-render-modifiers in favour of custom modifiers (see Dylan's PR https://github.com/hashicorp/design-system/pull/2856/files for example); maybe something we can do here as well?

or is it because you didn't want to have to do also the migration of the did-insert applied to the root element of this component? in which case makes sense, and I have no objection to keep it "old school" for now :)

Hahah I can update to use the new modifier pattern! Might as well since I'm touching the file.

didoo · 2025-05-14T21:14:21Z

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

@@ -213,6 +216,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


[praise] 👏 👏 👏

didoo · 2025-05-14T21:22:53Z

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

-        element.removeAttribute('inert');
-      }
-    });
+    if (this._isMinimized) {


[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)

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

didoo · 2025-05-14T21:29:18Z

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

@@ -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();


didoo · 2025-05-14T21:38:14Z

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

+    await render(hbs`<Hds::AppSideNav
+    id='test-app-side-nav'
+  @isCollapsible={{true}}
+  @onDesktopViewportChange={{this.onDesktopViewportChange}}


[question] could it be that this.onDesktopViewportChange is undefined for this test, and you have to add its definition like in the other tests above? or I'm missing something?

Ah, I can remove it. It is unneeded for this test.

didoo · 2025-05-14T21:43:25Z

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

+  @isCollapsible={{true}}
+  @onDesktopViewportChange={{this.onDesktopViewportChange}}
+>
+  <span id='test-app-side-nav-body' />


[question] what is this span#test-app-side-nav-body used for? 🤔

(I see other places where is not used in the tests assertions/declarations; maybe there's a reason for that? otherwise maybe we can remove them?)

I can most of remove them... They were initially used in tests along with <span id='hds-app-side-nav-hide-when-minimized' /> to show that inert was only added to the correct elements.

Im leaving ones where we test that the content in the yielded block is rendered correctly and a test that makes sure that the content in the block is hidden when the nav is minimized.

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

didoo · 2025-05-14T21:52:54Z

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

+    assert.dom('body', document).doesNotHaveStyle('overflow');
+  });
+
+  test('when collapsed, the content in the AppSideNav is not focusable', async function (assert) {


[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? 🤔

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

…est.js Co-authored-by: Cristiano Rastelli <[email protected]>

hashibot-hds added packages/components showcase labels May 12, 2025

vercel bot deployed to Preview – hds-showcase May 12, 2025 18:28 View deployment

shleewhite commented May 12, 2025

View reviewed changes

vercel bot deployed to Preview – hds-website May 12, 2025 18:30 View deployment

shleewhite marked this pull request as ready for review May 13, 2025 11:44

shleewhite requested a review from a team as a code owner May 13, 2025 11:44

shleewhite changed the title ~~AppSideNav: fix scrolling issue,~~ AppSideNav: fix scrolling issue and being able to focus hidden links issue May 13, 2025

vercel bot deployed to Preview – hds-showcase May 13, 2025 12:01 View deployment

vercel bot deployed to Preview – hds-website May 13, 2025 12:03 View deployment

vercel bot deployed to Preview – hds-showcase May 13, 2025 13:56 View deployment

vercel bot deployed to Preview – hds-website May 13, 2025 13:59 View deployment

shleewhite added the release-candidate Publishes release candidates to npm label May 13, 2025

shleewhite removed the release-candidate Publishes release candidates to npm label May 13, 2025

KristinLBradley reviewed May 13, 2025

View reviewed changes

.changeset/honest-experts-battle.md Outdated Show resolved Hide resolved

vercel bot deployed to Preview – hds-showcase May 13, 2025 18:04 View deployment

vercel bot deployed to Preview – hds-website May 13, 2025 18:06 View deployment

shleewhite requested a review from a team as a code owner May 13, 2025 18:10

hashibot-hds added the docs-website Content updates to the documentation website label May 13, 2025

shleewhite commented May 13, 2025

View reviewed changes

vercel bot deployed to Preview – hds-showcase May 13, 2025 18:12 View deployment

vercel bot deployed to Preview – hds-website May 13, 2025 18:14 View deployment

KristinLBradley reviewed May 13, 2025

View reviewed changes

shleewhite added this to the [email protected] milestone May 14, 2025

shleewhite requested a review from a team May 14, 2025 16:39

didoo reviewed May 14, 2025

View reviewed changes

packages/components/src/components/hds/app-side-nav/index.hbs Outdated Show resolved Hide resolved

didoo mentioned this pull request May 14, 2025

Showcase - Add a frameless example for the AppSideNav #2876

Merged

1 task

vercel bot deployed to Preview – hds-showcase May 14, 2025 21:03 View deployment

vercel bot deployed to Preview – hds-website May 14, 2025 21:06 View deployment

shleewhite and others added 12 commits May 14, 2025 14:14

fix: dont lock scrolling on desktop

5405641

chore: changeset

f99562e

fix: re-enable body test for Modal

696be6f

fix: prevent users from tabbing to hidden links if dont use portal

d5c7db5

chore: add tests for the bugs fixed

2df7b3e

chore: update changeset

c6e5edd

fix: issue where pressing escape doesnt reset ability to scroll

6af08bc

fix: failing test

6b4ba9c

Update .changeset/honest-experts-battle.md

1adbcbd

Co-authored-by: Kristin Bradley <[email protected]>

chore: rename class hds-app-side-nav-hide-when-minimized -> hds-app-s…

800bdb9

…ide-nav--hide-when-minimized

chore: remove content from docs that isnt true

8dd5c5b

chore: refactor how content is marked as inert

ffb7563

shleewhite force-pushed the hds-4827/app-side-nav-overflow-bug branch from 1a2188c to ffb7563 Compare May 14, 2025 21:14

chore: typo

d0f4983

vercel bot deployed to Preview – hds-showcase May 14, 2025 21:17 View deployment

shleewhite requested review from didoo and KristinLBradley May 14, 2025 21:22

vercel bot deployed to Preview – hds-website May 14, 2025 21:23 View deployment

didoo reviewed May 14, 2025

View reviewed changes

Update showcase/tests/integration/components/hds/app-side-nav/index-t…

45d098d

…est.js Co-authored-by: Cristiano Rastelli <[email protected]>

vercel bot deployed to Preview – hds-showcase May 14, 2025 23:31 View deployment

vercel bot deployed to Preview – hds-website May 14, 2025 23:33 View deployment

shleewhite added 3 commits May 14, 2025 17:18

chore: remove unneeded content inside AppSideNav tests

f479157

chore: remove unneeded arg in test

0e325c6

fix: use custom modifiers instead of did-insert

345c550

vercel bot deployed to Preview – hds-showcase May 15, 2025 00:34 View deployment

shleewhite requested a review from didoo May 15, 2025 00:35

vercel bot deployed to Preview – hds-website May 15, 2025 00:36 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

shleewhite commented May 12, 2025 •

edited

Loading

vercel bot commented May 12, 2025 •

edited

Loading

shleewhite May 12, 2025 •

edited

Loading

KristinLBradley May 13, 2025

KristinLBradley May 13, 2025

shleewhite May 13, 2025 •

edited

Loading

KristinLBradley May 13, 2025 •

edited

Loading

didoo May 13, 2025

github-actions bot commented May 13, 2025

@hashicorp/[email protected]

shleewhite May 13, 2025

KristinLBradley May 13, 2025

shleewhite May 14, 2025

KristinLBradley left a comment

didoo left a comment

didoo May 14, 2025

didoo May 14, 2025

shleewhite May 15, 2025

didoo May 14, 2025

didoo May 14, 2025

shleewhite May 15, 2025

didoo May 14, 2025

didoo May 14, 2025 •

edited

Loading

shleewhite May 15, 2025

didoo May 14, 2025

shleewhite May 15, 2025 •

edited

Loading

didoo May 14, 2025

shleewhite May 14, 2025

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

Are you sure you want to change the base?

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

Conversation

shleewhite commented May 12, 2025 • edited Loading

📌 Summary

🔗 External links

👀 Component checklist

vercel bot commented May 12, 2025 • edited Loading

shleewhite May 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shleewhite May 13, 2025 • edited Loading

Choose a reason for hiding this comment

KristinLBradley May 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 13, 2025

📦 RC Packages Published

@hashicorp/[email protected]

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KristinLBradley left a comment

Choose a reason for hiding this comment

didoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

didoo May 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shleewhite May 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

shleewhite commented May 12, 2025 •

edited

Loading

vercel bot commented May 12, 2025 •

edited

Loading

shleewhite May 12, 2025 •

edited

Loading

shleewhite May 13, 2025 •

edited

Loading

KristinLBradley May 13, 2025 •

edited

Loading

didoo May 14, 2025 •

edited

Loading

shleewhite May 15, 2025 •

edited

Loading