From 175a04739d7eafd03e1059a279bf93dd691a3714 Mon Sep 17 00:00:00 2001 From: shleewhite Date: Tue, 30 Sep 2025 12:11:55 -0400 Subject: [PATCH 1/3] feat: improve lifecycle management by removing ember-render-modifiers --- .changeset/rich-zoos-type.md | 11 +++ .../src/components/hds/flyout/index.hbs | 3 +- .../src/components/hds/flyout/index.ts | 83 ++++++++----------- .../src/components/hds/modal/index.hbs | 3 +- .../src/components/hds/modal/index.ts | 67 +++++++-------- .../app/templates/page-components/flyout.hbs | 18 ++-- .../app/templates/page-components/modal.hbs | 18 ++-- 7 files changed, 97 insertions(+), 106 deletions(-) create mode 100644 .changeset/rich-zoos-type.md diff --git a/.changeset/rich-zoos-type.md b/.changeset/rich-zoos-type.md new file mode 100644 index 00000000000..b0170b669ed --- /dev/null +++ b/.changeset/rich-zoos-type.md @@ -0,0 +1,11 @@ +--- +"@hashicorp/design-system-components": patch +--- + + +`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. + + + +`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. + \ No newline at end of file diff --git a/packages/components/src/components/hds/flyout/index.hbs b/packages/components/src/components/hds/flyout/index.hbs index 949ca208a42..8c15b2b1f35 100644 --- 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 index 4861fe385c6..772dc5da816 100644 --- 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 { 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 { 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 { } 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 `` and `` elements this._element = element; this._body = document.body; @@ -135,19 +142,21 @@ export default class HdsFlyout extends Component { if (!this._element.open) { this.open(); } - } - @action - willDestroyNode(): void { - if (this._element) { - this._element.removeEventListener( + return () => { + // if the 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 { async onDismiss(): Promise { // 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 { // 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 index ea32f64eb5b..71b076256b1 100644 --- a/packages/components/src/components/hds/modal/index.hbs +++ b/packages/components/src/components/hds/modal/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/modal/index.ts b/packages/components/src/components/hds/modal/index.ts index 33d3e27ee63..550997bd40d 100644 --- 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'; @@ -128,11 +129,33 @@ export default class HdsModal extends Component { } } 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 `` and `` elements this._element = element; this._body = document.body; @@ -151,19 +174,21 @@ export default class HdsModal extends Component { if (!this._element.open) { this.open(); } - } - @action - willDestroyNode(): void { - if (this._element) { - this._element.removeEventListener( + return () => { + // if the 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 { @@ -185,7 +210,6 @@ export default class HdsModal extends Component { async onDismiss(): Promise { // 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 +221,5 @@ export default class HdsModal extends Component { // 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 index b741e623913..25404adddc5 100644 --- a/showcase/app/templates/page-components/flyout.hbs +++ b/showcase/app/templates/page-components/flyout.hbs @@ -332,14 +332,12 @@ method.

This is equivalent to a manual dismiss (Esc - 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 close() method of the Dialog - HTML element, who then will cause the - willDestroyNode - action to execute.

+ HTML element.

@@ -363,9 +361,9 @@ flyout from the DOM.

This is not equivalent to a manual dismiss (Esc - key, click outsite, click dismiss button) because it will trigger directly the - willDestroyNode - action.

+ key, click outside, click dismiss button) because it will trigger directly the + _registerDialog + modifier's cleanup function.

This is not equivalent to a manual dismiss (Esc - key, click outsite, click dismiss button) because it will trigger directly the - willDestroyNode - action.

+ key, click outside, click dismiss button) because it will directly trigger the + _registerDialog + modifier's cleanup function.

Fill in this input diff --git a/showcase/app/templates/page-components/modal.hbs b/showcase/app/templates/page-components/modal.hbs index 6979cb0e726..cb27e4dfe37 100644 --- a/showcase/app/templates/page-components/modal.hbs +++ b/showcase/app/templates/page-components/modal.hbs @@ -585,14 +585,12 @@ method.

This is equivalent to a manual dismiss (Esc - 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 close() method of the Dialog - HTML element, who then will cause the - willDestroyNode - action to execute.

+ HTML element.

@@ -616,9 +614,9 @@ modal from the DOM.

This is not equivalent to a manual dismiss (Esc - key, click outsite, click dismiss button) because it will trigger directly the - willDestroyNode - action.

+ key, click outside, click dismiss button) because it will directly trigger the + _registerDialog + modifier's cleanup function.

This is not equivalent to a manual dismiss (Esc - key, click outsite, click dismiss button) because it will trigger directly the - willDestroyNode - action.

+ key, click outside, click dismiss button) because it will directly trigger the + _registerDialog + modifier's cleanup function.

Fill in this input From bc4cfad3a044c3f51105a298bcb729450d19f836 Mon Sep 17 00:00:00 2001 From: shleewhite Date: Tue, 30 Sep 2025 12:32:11 -0400 Subject: [PATCH 2/3] fix: bug with click outside to dismiss for modal --- .../src/components/hds/modal/index.hbs | 2 +- .../src/components/hds/modal/index.ts | 23 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/packages/components/src/components/hds/modal/index.hbs b/packages/components/src/components/hds/modal/index.hbs index 71b076256b1..92859957f5a 100644 --- a/packages/components/src/components/hds/modal/index.hbs +++ b/packages/components/src/components/hds/modal/index.hbs @@ -8,7 +8,7 @@ aria-labelledby={{this.id}} {{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 index 550997bd40d..ee6f1a2df29 100644 --- a/packages/components/src/components/hds/modal/index.ts +++ b/packages/components/src/components/hds/modal/index.ts @@ -62,6 +62,7 @@ export default class HdsModal extends Component { private _element!: HTMLDialogElement; private _body!: HTMLElement; private _bodyInitialOverflowValue = ''; + private _clickOutsideToDismissHandler!: (event: MouseEvent) => void; get isDismissDisabled(): boolean { return this.args.isDismissDisabled ?? false; @@ -175,6 +176,22 @@ export default class HdsModal extends Component { this.open(); } + // 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 is removed from the dom while open we emulate the close event if (this._isOpen) { @@ -187,6 +204,12 @@ export default class HdsModal extends Component { this.registerOnCloseCallback, true ); + + document.removeEventListener( + 'click', + this._clickOutsideToDismissHandler, + true + ); }; }); From 7d53259f6d94fdb673886cafa97a50b22917d274 Mon Sep 17 00:00:00 2001 From: shleewhite Date: Wed, 1 Oct 2025 09:20:21 -0400 Subject: [PATCH 3/3] chore: reenable click outside test --- .../tests/integration/components/hds/modal/index-test.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/showcase/tests/integration/components/hds/modal/index-test.js b/showcase/tests/integration/components/hds/modal/index-test.js index 256673965b9..ea3e5afbddd 100644 --- a/showcase/tests/integration/components/hds/modal/index-test.js +++ b/showcase/tests/integration/components/hds/modal/index-test.js @@ -151,8 +151,8 @@ module('Integration | Component | hds/modal/index', function (hooks) { await triggerKeyEvent('.hds-modal', 'keydown', 'Escape'); assert.dom('#test-modal').isNotVisible(); }); - // TODO! while we decide what to do about the original bug - skip('it should close the modal when clicking outside', async function (assert) { + + test('it should close the modal when clicking outside', async function (assert) { await render( hbs`Title`, ); @@ -160,6 +160,7 @@ module('Integration | Component | hds/modal/index', function (hooks) { await click('.hds-modal__overlay'); assert.dom('#test-modal').isNotVisible(); }); + test('it should not close the modal when `@isDismissDisabled` is `true`', async function (assert) { this.set('isDismissDisabled', true); await render(