Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main-5.1.0
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
175a047
feat: improve lifecycle management by removing ember-render-modifiers
Sep 30, 2025
bc4cfad
fix: bug with click outside to dismiss for modal
Sep 30, 2025
7d53259
chore: reenable click outside test
Oct 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .changeset/rich-zoos-type.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 -->
3 changes: 1 addition & 2 deletions packages/components/src/components/hds/flyout/index.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)}}
>
Expand Down
83 changes: 34 additions & 49 deletions packages/components/src/components/hds/flyout/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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;

Expand All @@ -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'];

Expand All @@ -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;
Expand All @@ -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 {
Expand All @@ -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 = () => {
Expand All @@ -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();
}
}
}
}
5 changes: 2 additions & 3 deletions packages/components/src/components/hds/modal/index.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
90 changes: 57 additions & 33 deletions packages/components/src/components/hds/modal/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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 {
Expand All @@ -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 = () => {
Expand All @@ -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();
}
}
}
}
18 changes: 8 additions & 10 deletions showcase/app/templates/page-components/flyout.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}} />
Expand All @@ -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
Expand Down Expand Up @@ -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>
Expand Down
Loading