Skip to content

Conversation

didoo
Copy link
Contributor

@didoo didoo commented Jul 18, 2025

📌 Summary

This is a follow-up of #3026.

I've noticed that the focused state of the AppSideNav::List:Link items was covered by the following item in the list:

screenshot_5184

The reason is that in #3026 I have applied an explicit background to it, the same color as the backdrop of their container.

This issue didn't happen in the previous implementation (SideNav::List:Link):
screenshot_5185

The reason for this difference is that before the focus was applied via a pseudo-element, so the z-index would take care of the stacking, while in the new implementation of the side navigation the focus is applied as box shadow.

This PR fixes the issue.

🛠️ Detailed description

In this PR I have:

  • updated the showcase for the status of AppSideNav::List:Link to make the problem visible
  • set a transparent background color for .hds-app-side-nav__list-item-link

👉 👉 👉 Preview: https://hds-showcase-git-sidenav-list-link-fix-backgro-cf8dad-hashicorp.vercel.app/components/app-side-nav#states

📸 Screenshots

Before:
screenshot_5186

After:
screenshot_5187

🔗 External links

Jira ticket: https://hashicorp.atlassian.net/browse/HDS-5153


👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

📋 PCI review checklist
  • If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
  • If applicable, I've worked with GRC to document the impact of any changes to security controls.
    Examples of changes to controls include access controls, encryption, logging, etc.
  • If applicable, I've worked with GRC to ensure compliance due to a significant change to the in-scope PCI environment.
    Examples include changes to operating systems, ports, protocols, services, cryptography-related components, PII processing code, etc.

@didoo didoo requested a review from a team as a code owner July 18, 2025 10:54
Copy link

vercel bot commented Jul 18, 2025

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

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Jul 22, 2025 11:23am
hds-website ✅ Ready (Inspect) Visit Preview Jul 22, 2025 11:23am

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a background/z-index stacking issue in AppSideNav::List::Link where focused states were being covered by subsequent list items. The fix changes the background from an explicit color to transparent, allowing proper stacking of focus states that are now implemented as box shadows.

Key changes:

  • Changed background color from explicit surface color to transparent for link items
  • Updated showcase to demonstrate the fix with multiple links in lists
  • Added margin reset for better visual consistency in showcase

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
packages/components/src/styles/components/app-side-nav/content.scss Sets transparent background for link items to fix focus state stacking
showcase/app/templates/page-components/app-side-nav/index.hbs Updates showcase to use multiple links and proper list structure to demonstrate the fix
showcase/app/styles/showcase-pages/app-side-nav.scss Adds margin reset for showcase list wrapper styling

Copy link
Contributor

@heatherlarsen heatherlarsen left a comment

Choose a reason for hiding this comment

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

Great improvement!

I found a couple of instances where the focus ring is still being cut off:

At the bottom of the list...not in all cases, though 🤔
Screenshot 2025-07-18 at 8 32 14 AM

When focus is visible on the list item above an active list item
Screenshot 2025-07-18 at 8 30 40 AM

@didoo
Copy link
Contributor Author

didoo commented Jul 21, 2025

I found a couple of instances where the focus ring is still being cut off:
At the bottom of the list...not in all cases, though

@heatherlarsen yes, this is a known issue. While we know what a possible fix may be for it, this PR is only focusing on the background issue

When focus is visible on the list item above an active list item

yeah, nice catch! there are so many combinations and permutations that it's hard to test them all! I've added more use cases to the showcase page for the AppSideNav, related to this, and I'll move the PR to draft so that @KristinLBradley can have a proper look at it and see if the fixes to this issue are correct, if there are better alternatives, and expecially if we're covering all the use possible cases (there may be more that I have forgotten).

@didoo didoo marked this pull request as draft July 21, 2025 10:31
@KristinLBradley
Copy link
Contributor

KristinLBradley commented Jul 21, 2025

From Heather:

I found a couple of instances where the focus ring is still being cut off:

At the bottom of the list...not in all cases, though 🤔

When focus is visible on the list item above an active list item

@heatherlarsen @didoo

I've been investigating the remaining focus ring issues. For the first one, when panels are used there is an overflow:hidden on the panel that causes the focus ring to be cut off. This was part of a bug fix with scrolling: Merged PR, Jira ticket

The safest way to fix this bug (I think) would be to add a bit of top and bottom padding to the panel container to give the focus ring room. This would alter the layout from the AppSideNav's not using panels though. (It would probably be possible to add some tricky CSS to change the overall visual padding dependent of the AppSideNav when it contains a panel vs. not to keep the visual spacing consistent but it would be a touch messy.)

It may also be possible to delete the overflow:hidden style on the panel but that risks re-introducing bugs with scrolling the AppSideNav content and is difficult to test for to account for all possible interactions related to navigating between links.

For the 2nd issue, with active list item backgrounds overlapping the focus ring, I wasn't able to duplicate it so it seems that this issue may already be fixed in this PR?

@didoo
Copy link
Contributor Author

didoo commented Jul 22, 2025

It may also be possible to delete the overflow:hidden style on the panel but that risks re-introducing bugs with scrolling the AppSideNav content and is difficult to test for to account for all possible interactions related to navigating between links.

@KristinLBradley yes, that was I was hoping you could do or have more context about: do you remember why this fix was introduced, and how to replicate the issue that made us make the fix, so we can see if it's still applicable?

For the 2nd issue, with active list item backgrounds overlapping the focus ring, I wasn't able to duplicate it so it seems that this issue may already be fixed in this PR?

Yes, I would assume this is fixed with this PR, but I need you to double check the proposed solution, because it involves a bit of refactoring of position and z-index values, so I may have not considered something.

@KristinLBradley
Copy link
Contributor

KristinLBradley commented Jul 22, 2025

It may also be possible to delete the overflow:hidden style on the panel but that risks re-introducing bugs with scrolling the AppSideNav content and is difficult to test for to account for all possible interactions related to navigating between links.

@KristinLBradley yes, that was I was hoping you could do or have more context about: do you remember why this fix was introduced, and how to replicate the issue that made us make the fix, so we can see if it's still applicable?

For the 2nd issue, with active list item backgrounds overlapping the focus ring, I wasn't able to duplicate it so it seems that this issue may already be fixed in this PR?

Yes, I would assume this is fixed with this PR, but I need you to double check the proposed solution, because it involves a bit of refactoring of position and z-index values, so I may have not considered something.

@didoo I checked out this PR branch locally and also removed the overflow: hidden style from .hds-side-nav__content-panel then linked to the local files and tried testing within both cloud-ui & atlas. I navigated around and tried focusing various links. I wasn't able to reproduce the original issue so as far as I can tell it's not needed. Terraform/atlas has a custom overflow: hidden style on an outer ".sidebar-scrollbar" container however which still cuts off the focus ring on focused links at the top & bottom of the AppSideNav. (I tried also commenting out this custom style and then navigated around & focused the AppSideNav links and wasn't able to detect issues.)

I'll continue testing your other solution for the 2nd issue to see if I encounter issues but so far I'm not seeing any.

@@ -130,6 +131,7 @@
// Important: This element does not doing anything when interacted with so should not change color according to state
&.active,
&.active:hover:focus {
position: relative; // for the "active" indicator bar to be correctly positioned
Copy link
Contributor

Choose a reason for hiding this comment

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

So far, I haven't been able to uncover issues related to this change and the z-index above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice!

@didoo didoo marked this pull request as ready for review July 23, 2025 09:31
@didoo
Copy link
Contributor Author

didoo commented Jul 23, 2025

@KristinLBradley I have moved the PR out of draft, please review.

Note: this PR fixes only the background issue; for the overflow issue you should open a distinct PR so we have better traceability of the changes we're introducing. As soon as it's open, I can review it.

@didoo didoo merged commit 4ffea83 into main Jul 23, 2025
16 checks passed
@didoo didoo deleted the sidenav-list-link-fix-background-issue branch July 23, 2025 16:41
@hashibot-hds hashibot-hds mentioned this pull request Jul 23, 2025
@KristinLBradley
Copy link
Contributor

@KristinLBradley I have moved the PR out of draft, please review.

Note: this PR fixes only the background issue; for the overflow issue you should open a distinct PR so we have better traceability of the changes we're introducing. As soon as it's open, I can review it.

@didoo Should I create a Jira ticket or is it ok to open a PR directly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants