feat: improve logic for modal/flyout lifecycle handling (#3215)

shleewhite · web-flow · commit 4cddac065f0a · 2025-10-08T15:14:24.000-04:00
diff --git a/.changeset/rich-zoos-type.md b/.changeset/rich-zoos-type.md
@@ -0,0 +1,11 @@
+---
+"@hashicorp/design-system-components": patch
+---
+
+<!-- START components/modal -->
+`Modal` - Refactored the component to not use `ember-render-modifiers` which fixes issues where the DOM may not be cleaned up when the Modal is closed. 
+<!-- END -->
+
+<!-- START components/flyout -->
+`Flyout` - Refactored the component to not use `ember-render-modifiers` which fixes issues where the DOM may not be cleaned up when the Flyout is closed. 
+<!-- END -->
diff --git a/packages/components/src/components/hds/flyout/index.hbs b/packages/components/src/components/hds/flyout/index.hbs
@@ -6,8 +6,7 @@
   class={{this.classNames}}
   ...attributes
   aria-labelledby={{this.id}}
-  {{did-insert this.didInsert}}
-  {{will-destroy this.willDestroyNode}}
+  {{this._registerDialog}}
   {{! @glint-expect-error - https://github.com/josemarluedke/ember-focus-trap/issues/86 }}
   {{focus-trap isActive=this._isOpen focusTrapOptions=(hash onDeactivate=this.onDismiss clickOutsideDeactivates=true)}}
 >
diff --git a/packages/components/src/components/hds/flyout/index.ts b/packages/components/src/components/hds/flyout/index.ts
@@ -10,6 +10,7 @@ import { assert } from '@ember/debug';
 import { getElementId } from '../../../utils/hds-get-element-id.ts';
 import { buildWaiter } from '@ember/test-waiters';
 import type { WithBoundArgs } from '@glint/template';
+import { modifier } from 'ember-modifier';
 
 import type { HdsFlyoutSizes } from './types.ts';
 
@@ -65,14 +66,6 @@ export default class HdsFlyout extends Component<HdsFlyoutSignature> {
   private _body!: HTMLElement;
   private _bodyInitialOverflowValue = '';
 
-  /**
-   * Sets the size of the flyout
-   * Accepted values: medium, large
-   *
-   * @param size
-   * @type {string}
-   * @default 'medium'
-   */
   get size(): HdsFlyoutSizes {
     const { size = DEFAULT_SIZE } = this.args;
 
@@ -86,18 +79,10 @@ export default class HdsFlyout extends Component<HdsFlyoutSignature> {
     return size;
   }
 
-  /**
-   * Calculates the unique ID to assign to the title
-   */
   get id(): string {
     return getElementId(this);
   }
 
-  /**
-   * Get the class names to apply to the component.
-   * @method classNames
-   * @return {string} The "class" attribute to apply to the component.
-   */
   get classNames(): string {
     const classes = ['hds-flyout'];
 
@@ -113,10 +98,32 @@ export default class HdsFlyout extends Component<HdsFlyoutSignature> {
     }
 
     this._isOpen = false;
+
+    // Reset page `overflow` property
+    if (this._body) {
+      this._body.style.removeProperty('overflow');
+      if (this._bodyInitialOverflowValue === '') {
+        if (this._body.style.length === 0) {
+          this._body.removeAttribute('style');
+        }
+      } else {
+        this._body.style.setProperty(
+          'overflow',
+          this._bodyInitialOverflowValue
+        );
+      }
+    }
+
+    // Return focus to a specific element (if provided)
+    if (this.args.returnFocusTo) {
+      const initiator = document.getElementById(this.args.returnFocusTo);
+      if (initiator) {
+        initiator.focus();
+      }
+    }
   }
 
-  @action
-  didInsert(element: HTMLDialogElement): void {
+  private _registerDialog = modifier((element: HTMLDialogElement) => {
     // Store references of `<dialog>` and `<body>` elements
     this._element = element;
     this._body = document.body;
@@ -135,19 +142,21 @@ export default class HdsFlyout extends Component<HdsFlyoutSignature> {
     if (!this._element.open) {
       this.open();
     }
-  }
 
-  @action
-  willDestroyNode(): void {
-    if (this._element) {
-      this._element.removeEventListener(
+    return () => {
+      // if the <dialog> is removed from the dom while open we emulate the close event
+      if (this._isOpen) {
+        this._element?.dispatchEvent(new Event('close'));
+      }
+
+      this._element?.removeEventListener(
         'close',
         // eslint-disable-next-line @typescript-eslint/unbound-method
         this.registerOnCloseCallback,
         true
       );
-    }
-  }
+    };
+  });
 
   @action
   open(): void {
@@ -169,7 +178,6 @@ export default class HdsFlyout extends Component<HdsFlyoutSignature> {
   async onDismiss(): Promise<void> {
     // allow ember test helpers to be aware of when the `close` event fires
     // when using `click` or other helpers from '@ember/test-helpers'
-    // Notice: this code will get stripped out in production builds (DEBUG evaluates to `true` in dev/test builds, but `false` in prod builds)
     if (this._element.open) {
       const token = waiter.beginAsync();
       const listener = () => {
@@ -181,28 +189,5 @@ export default class HdsFlyout extends Component<HdsFlyoutSignature> {
 
     // Make flyout dialog invisible using the native `close` method
     this._element.close();
-
-    // Reset page `overflow` property
-    if (this._body) {
-      this._body.style.removeProperty('overflow');
-      if (this._bodyInitialOverflowValue === '') {
-        if (this._body.style.length === 0) {
-          this._body.removeAttribute('style');
-        }
-      } else {
-        this._body.style.setProperty(
-          'overflow',
-          this._bodyInitialOverflowValue
-        );
-      }
-    }
-
-    // Return focus to a specific element (if provided)
-    if (this.args.returnFocusTo) {
-      const initiator = document.getElementById(this.args.returnFocusTo);
-      if (initiator) {
-        initiator.focus();
-      }
-    }
   }
 }
diff --git a/packages/components/src/components/hds/modal/index.hbs b/packages/components/src/components/hds/modal/index.hbs
@@ -6,10 +6,9 @@
   class={{this.classNames}}
   ...attributes
   aria-labelledby={{this.id}}
-  {{did-insert this.didInsert}}
-  {{will-destroy this.willDestroyNode}}
+  {{this._registerDialog}}
   {{! @glint-expect-error - https://github.com/josemarluedke/ember-focus-trap/issues/86 }}
-  {{focus-trap isActive=this._isOpen focusTrapOptions=(hash onDeactivate=this.onDismiss clickOutsideDeactivates=true)}}
+  {{focus-trap isActive=this._isOpen focusTrapOptions=(hash onDeactivate=this.onDismiss)}}
 >
   <:header>
     {{yield
diff --git a/packages/components/src/components/hds/modal/index.ts b/packages/components/src/components/hds/modal/index.ts
@@ -9,6 +9,7 @@ import { action } from '@ember/object';
 import { assert } from '@ember/debug';
 import { getElementId } from '../../../utils/hds-get-element-id.ts';
 import { buildWaiter } from '@ember/test-waiters';
+import { modifier } from 'ember-modifier';
 
 import type { WithBoundArgs } from '@glint/template';
 import type { HdsModalSizes, HdsModalColors } from './types.ts';
@@ -61,6 +62,7 @@ export default class HdsModal extends Component<HdsModalSignature> {
   private _element!: HTMLDialogElement;
   private _body!: HTMLElement;
   private _bodyInitialOverflowValue = '';
+  private _clickOutsideToDismissHandler!: (event: MouseEvent) => void;
 
   get isDismissDisabled(): boolean {
     return this.args.isDismissDisabled ?? false;
@@ -128,11 +130,33 @@ export default class HdsModal extends Component<HdsModalSignature> {
       }
     } else {
       this._isOpen = false;
+
+      // Reset page `overflow` property
+      if (this._body) {
+        this._body.style.removeProperty('overflow');
+        if (this._bodyInitialOverflowValue === '') {
+          if (this._body.style.length === 0) {
+            this._body.removeAttribute('style');
+          }
+        } else {
+          this._body.style.setProperty(
+            'overflow',
+            this._bodyInitialOverflowValue
+          );
+        }
+      }
+
+      // Return focus to a specific element (if provided)
+      if (this.args.returnFocusTo) {
+        const initiator = document.getElementById(this.args.returnFocusTo);
+        if (initiator) {
+          initiator.focus();
+        }
+      }
     }
   }
 
-  @action
-  didInsert(element: HTMLDialogElement): void {
+  private _registerDialog = modifier((element: HTMLDialogElement) => {
     // Store references of `<dialog>` and `<body>` elements
     this._element = element;
     this._body = document.body;
@@ -151,19 +175,43 @@ export default class HdsModal extends Component<HdsModalSignature> {
     if (!this._element.open) {
       this.open();
     }
-  }
 
-  @action
-  willDestroyNode(): void {
-    if (this._element) {
-      this._element.removeEventListener(
+    // Note: because the Modal has the `@isDismissedDisabled` argument, we need to add our own click outside to dismiss logic. This is because `ember-focus-trap` treats the `focusTrapOptions` as static, so we can't update it dynamically if `@isDismissDisabled` changes.
+    this._clickOutsideToDismissHandler = (event: MouseEvent) => {
+      // check if the click is outside the modal and the modal is open
+      if (!this._element.contains(event.target as Node) && this._isOpen) {
+        if (!this.isDismissDisabled) {
+          //  here we use `void` because `onDismiss` is an async function, but in reality we don't need to handle the result or wait for its completion
+          void this.onDismiss();
+        }
+      }
+    };
+
+    document.addEventListener('click', this._clickOutsideToDismissHandler, {
+      capture: true,
+      passive: false,
+    });
+
+    return () => {
+      // if the <dialog> is removed from the dom while open we emulate the close event
+      if (this._isOpen) {
+        this._element?.dispatchEvent(new Event('close'));
+      }
+
+      this._element?.removeEventListener(
         'close',
         // eslint-disable-next-line @typescript-eslint/unbound-method
         this.registerOnCloseCallback,
         true
       );
-    }
-  }
+
+      document.removeEventListener(
+        'click',
+        this._clickOutsideToDismissHandler,
+        true
+      );
+    };
+  });
 
   @action
   open(): void {
@@ -185,7 +233,6 @@ export default class HdsModal extends Component<HdsModalSignature> {
   async onDismiss(): Promise<void> {
     // allow ember test helpers to be aware of when the `close` event fires
     // when using `click` or other helpers from '@ember/test-helpers'
-    // Notice: this code will get stripped out in production builds (DEBUG evaluates to `true` in dev/test builds, but `false` in prod builds)
     if (this._element.open) {
       const token = waiter.beginAsync();
       const listener = () => {
@@ -197,28 +244,5 @@ export default class HdsModal extends Component<HdsModalSignature> {
 
     // Make modal dialog invisible using the native `close` method
     this._element.close();
-
-    // Reset page `overflow` property
-    if (this._body) {
-      this._body.style.removeProperty('overflow');
-      if (this._bodyInitialOverflowValue === '') {
-        if (this._body.style.length === 0) {
-          this._body.removeAttribute('style');
-        }
-      } else {
-        this._body.style.setProperty(
-          'overflow',
-          this._bodyInitialOverflowValue
-        );
-      }
-    }
-
-    // Return focus to a specific element (if provided)
-    if (this.args.returnFocusTo) {
-      const initiator = document.getElementById(this.args.returnFocusTo);
-      if (initiator) {
-        initiator.focus();
-      }
-    }
   }
 }
diff --git a/showcase/app/templates/page-components/flyout.hbs b/showcase/app/templates/page-components/flyout.hbs
@@ -332,14 +332,12 @@
           method.</p>
         <p class="hds-typography-body-200 hds-foreground-primary" {{style margin-top="12px"}}>This is equivalent to a
           manual dismiss (<code>Esc</code>
-          key, click outsite, click dismiss button) because they're all calling the same function, which invokes the
+          key, click outside, click dismiss button) because they're all calling the same function, which invokes the
           native
           <code>close()</code>
           method of the
           <code>Dialog</code>
-          HTML element, who then will cause the
-          <code>willDestroyNode</code>
-          action to execute.</p>
+          HTML element.</p>
       </F.Body>
       <F.Footer as |F|>
         <Hds::Button type="button" @text="Confirm" {{on "click" F.close}} />
@@ -363,9 +361,9 @@
           flyout from the DOM.</p>
         <p class="hds-typography-body-200 hds-foreground-primary" {{style margin-top="12px"}}>This is not equivalent to
           a manual dismiss (<code>Esc</code>
-          key, click outsite, click dismiss button) because it will trigger directly the
-          <code>willDestroyNode</code>
-          action.</p>
+          key, click outside, click dismiss button) because it will trigger directly the
+          <code>_registerDialog</code>
+          modifier's cleanup function.</p>
       </F.Body>
       <F.Footer>
         <Hds::Button
@@ -394,9 +392,9 @@
           the associated action will remove the flyout from the DOM.</p>
         <p class="hds-typography-body-200 hds-foreground-primary" {{style margin="12px 0 32px"}}>This is not equivalent
           to a manual dismiss (<code>Esc</code>
-          key, click outsite, click dismiss button) because it will trigger directly the
-          <code>willDestroyNode</code>
-          action.</p>
+          key, click outside, click dismiss button) because it will directly trigger the
+          <code>_registerDialog</code>
+          modifier's cleanup function.</p>
         <form id="deactivate-flyout-on-submit__form" {{on "submit" this.deactivateFlyoutOnSubmit}}>
           <Hds::Form::TextInput::Field name="deactivate-flyout-on-submit__input" as |F|>
             <F.Label>Fill in this input</F.Label>
diff --git a/showcase/app/templates/page-components/modal.hbs b/showcase/app/templates/page-components/modal.hbs
diff --git a/showcase/tests/integration/components/hds/modal/index-test.js b/showcase/tests/integration/components/hds/modal/index-test.js
