`Modal` - Revert fixes #2846 and #2902 #2964

didoo · 2025-06-20T10:26:24Z

📌 Summary

This PR reverts the fixes made in #2846 and #2902 that introduced an unexpected side-effect.

When the Modal was not closed via the "manual" dismiss (click outside, click dismiss button, esc key) or the F.Close yielded method, the code that cleans up the overflow: hidden from the <body> element is run inside the onDismiss function. When the modal is simply destroyed and removed from the DOM (eg. on submit) the onDismiss is not run, just the willDestroyNode method, which leaves the overflow applied to the body, making the page unscrollable.

We've decided that, even if we have other fixes to the bug (see #2961 and #2962), this is the safest and quickest route. After the release, we will discuss and decide how to reintroduce the original/intended changes (for better accessibility) in a safe and controlled way (that would likely include a refactoring of the existing code to make it easier to follow the logic flow and debug if necessary)

You can see examples of this bug in the linked Jira ticket.

🛠️ Detailed description

In this PR I have:

reverted Modal: fix issue with click outside to dismiss #2846
reverted Modal: remove click listener properly #2902 (but left the integration test that was added)

The updates to the showcase and the integration tests are done in a different PR (#2966) to keep this one very simple and straightforward.

📸 Screenshots

Modal:
https://github.com/user-attachments/assets/4a681e64-9f50-4a59-9001-b7cab3525f4c

Flyout:
https://github.com/user-attachments/assets/3c34dfe5-f503-4bf2-9cd6-694ccc06e692

🔗 External links

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

Related PRs:

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

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

vercel · 2025-06-20T10:26:29Z

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

Name	Status	Preview	Updated (UTC)
hds-showcase	✅ Ready (Inspect)	Visit Preview	Jun 20, 2025 1:40pm
hds-website	✅ Ready (Inspect)	Visit Preview	Jun 20, 2025 1:40pm

github-actions · 2025-06-20T10:28:08Z

📦 RC Packages Published

Latest commit: 48c8ecc

Published 2 packages

@hashicorp/design-system-components@4.20.2-rc-20250620133756

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

@hashicorp/flight-icons@3.11.1-rc-20250620133756

yarn up -C @hashicorp/flight-icons@rc

alex-ju · 2025-06-20T11:07:32Z

the original/intended changes (for better accessibility)

looking at #2846, it doesn't just improve accessibility, it fixes a functional issue with click outside the Modal not working properly when isDismissDisabled is set to true 🤔

so by simply reverting it we're bringing back a functional issue – @shleewhite may be able to keep me honest here – we could make the call of moving forward, but want to make sure it's a conscious decision

didoo · 2025-06-20T13:15:42Z

the original/intended changes (for better accessibility)

looking at #2846, it doesn't just improve accessibility, it fixes a functional issue with click outside the Modal not working properly when isDismissDisabled is set to true 🤔

so by simply reverting it we're bringing back a functional issue – @shleewhite may be able to keep me honest here – we could make the call of moving forward, but want to make sure it's a conscious decision

Per discussion amongst us (@alex-ju @shleewhite @didoo): we’ve decided that it would be safer/quicker to use the PR where we revert the changes introduced with the two previous fixes, because 1) we know that the code worked before and 2) the only issue, the original one that was being fixed, was reported by us and was a very specific edge case (click outside when the modal is not dismissible - see HDS-3972)

We’ve also decided to split in two the PRs, so we have a very simple PR for reverting things, and one for the showcase updates and the integration tests refactoring (see #2966).

This reverts commit 6f792ec.

This reverts commit 3dcdc17.

dchyun · 2025-06-20T13:41:03Z

This doesn't need to be a blocker for this PR, but for the examples deactivating on destroy and deactivating on form submit the focus is not getting reset back to the button after the modal is removed. In the on close example the focus is being reset.

This is because willDestroyNode is removing the close event listener before it gets called in the onDismiss with this._element.close().

Using the returnFocusTo argument makes the focus return correctly. It may be the case that if consumers use the modal in this way then they have to use returnFocusTo.

Overall the fix looks to be working great though. Didn't see any issues with scrolling being disabled.

alex-ju · 2025-06-20T14:10:06Z

This doesn't need to be a blocker for this PR, but for the examples deactivating on destroy and deactivating on form submit the focus is not getting reset back to the button after the modal is removed. In the on close example the focus is being reset.

This is because willDestroyNode is removing the close event listener before it gets called in the onDismiss with this._element.close().

Using the returnFocusTo argument makes the focus return correctly. It may be the case that if consumers use the modal in this way then they have to use returnFocusTo.

Overall the fix looks to be working great though. Didn't see any issues with scrolling being disabled.

I noticed that too @dchyun. The intention here is to provide a clean revert addressing the issue at hand, rather than a comprehensive fix – for now – and review the component for improvements in the coming days. We have a few potential ways to improve the component and clarify some of the actions.

didoo requested a review from alex-ju June 20, 2025 10:26

didoo added bug Something isn't working release-candidate Publishes release candidates to npm labels Jun 20, 2025

hashibot-hds added packages/components showcase labels Jun 20, 2025

didoo force-pushed the revert-modal-fixes-2846-2902 branch from c245493 to a79c86e Compare June 20, 2025 10:46

vercel bot deployed to Preview – hds-showcase June 20, 2025 10:47 View deployment

vercel bot deployed to Preview – hds-website June 20, 2025 10:50 View deployment

alex-ju requested a review from shleewhite June 20, 2025 11:16

didoo removed the release-candidate Publishes release candidates to npm label Jun 20, 2025

vercel bot deployed to Preview – hds-showcase June 20, 2025 12:00 View deployment

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

didoo force-pushed the revert-modal-fixes-2846-2902 branch from f3a2f5a to 815d901 Compare June 20, 2025 12:18

vercel bot deployed to Preview – hds-showcase June 20, 2025 12:20 View deployment

didoo force-pushed the revert-modal-fixes-2846-2902 branch from 815d901 to 61fbecf Compare June 20, 2025 12:22

shleewhite mentioned this pull request Jun 20, 2025

fix: move reset page logic back to onDismiss function #2965

Closed

5 tasks

didoo force-pushed the revert-modal-fixes-2846-2902 branch from 61fbecf to ba2ae3f Compare June 20, 2025 13:09

hashibot-hds removed the showcase label Jun 20, 2025

didoo mentioned this pull request Jun 20, 2025

Modal and Flyout - Improve test coverage (showcase and integration) #2966

Merged

4 tasks

didoo marked this pull request as ready for review June 20, 2025 13:16

didoo requested a review from a team as a code owner June 20, 2025 13:16

didoo added the release-candidate Publishes release candidates to npm label Jun 20, 2025

didoo added 2 commits June 20, 2025 14:21

Revert "Modal: remove click listener properly (#2902)"

a925809

This reverts commit 6f792ec.

Revert "Modal: fix issue with click outside to dismiss (#2846)"

7c9aef3

This reverts commit 3dcdc17.

didoo force-pushed the revert-modal-fixes-2846-2902 branch from ba2ae3f to 7c9aef3 Compare June 20, 2025 13:23

hashibot-hds added the showcase label Jun 20, 2025

vercel bot deployed to Preview – hds-showcase June 20, 2025 13:24 View deployment

vercel bot deployed to Preview – hds-website June 20, 2025 13:27 View deployment

didoo removed the release-candidate Publishes release candidates to npm label Jun 20, 2025

changeset

48c8ecc

didoo added the release-candidate Publishes release candidates to npm label Jun 20, 2025

vercel bot deployed to Preview – hds-showcase June 20, 2025 13:38 View deployment

vercel bot deployed to Preview – hds-website June 20, 2025 13:40 View deployment

didoo mentioned this pull request Jun 20, 2025

DO NOT MERGE - Smoke testing HDS 4.20.2 hashicorp/vault#31049

Closed

shleewhite approved these changes Jun 20, 2025

View reviewed changes

dchyun approved these changes Jun 20, 2025

View reviewed changes

alex-ju removed the release-candidate Publishes release candidates to npm label Jun 20, 2025

didoo mentioned this pull request Jun 20, 2025

DO NOT MERGE - Smoke testing HDS 4.20.2 hashicorp/boundary-ui#2894

Closed

didoo requested a review from shleewhite June 20, 2025 14:37

shleewhite approved these changes Jun 20, 2025

View reviewed changes

didoo merged commit 8660f5a into main Jun 20, 2025
17 checks passed

didoo deleted the revert-modal-fixes-2846-2902 branch June 20, 2025 14:46

hashibot-hds mentioned this pull request Jun 20, 2025

Version Packages #2945

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

`Modal` - Revert fixes #2846 and #2902 #2964

`Modal` - Revert fixes #2846 and #2902 #2964

Uh oh!

didoo commented Jun 20, 2025 •

edited by alex-ju

Loading

Uh oh!

vercel bot commented Jun 20, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Jun 20, 2025 •

edited

Loading

@hashicorp/design-system-components@4.20.2-rc-20250620133756

@hashicorp/flight-icons@3.11.1-rc-20250620133756

Uh oh!

alex-ju commented Jun 20, 2025

Uh oh!

didoo commented Jun 20, 2025 •

edited by atlassian bot

Loading

Uh oh!

dchyun commented Jun 20, 2025

Uh oh!

alex-ju commented Jun 20, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Modal - Revert fixes #2846 and #2902 #2964

Modal - Revert fixes #2846 and #2902 #2964

Uh oh!

Conversation

didoo commented Jun 20, 2025 • edited by alex-ju Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📌 Summary

🛠️ Detailed description

📸 Screenshots

🔗 External links

👀 Component checklist

Uh oh!

vercel bot commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📦 RC Packages Published

@hashicorp/design-system-components@4.20.2-rc-20250620133756

@hashicorp/flight-icons@3.11.1-rc-20250620133756

Uh oh!

alex-ju commented Jun 20, 2025

Uh oh!

didoo commented Jun 20, 2025 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dchyun commented Jun 20, 2025

Uh oh!

alex-ju commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

`Modal` - Revert fixes #2846 and #2902 #2964

`Modal` - Revert fixes #2846 and #2902 #2964

didoo commented Jun 20, 2025 •

edited by alex-ju

Loading

vercel bot commented Jun 20, 2025 •

edited

Loading

github-actions bot commented Jun 20, 2025 •

edited

Loading

didoo commented Jun 20, 2025 •

edited by atlassian bot

Loading

alex-ju commented Jun 20, 2025 •

edited

Loading