Skip to content

[🧪] Testing possible fix for Modal issues #2961

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

Closed
wants to merge 5 commits into from
Closed

[🧪] Testing possible fix for Modal issues #2961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f836cb9
added detailed logging to the `modal` backing class
didoo Jun 19, 2025
e47da3d
updated showcase page for `Modal` component to reproduce bug
didoo Jun 20, 2025
9a47d2e
updated showcase page for `Flyout` component to make sure there is no…
didoo Jun 20, 2025
cd154a0
refactoring of `Modal` component to identify possible fix
didoo Jun 19, 2025
a412e2a
removed unnecessary `onDeactivate=this.onManualDismiss` for the `focu…
didoo Jun 19, 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
8 changes: 5 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 @@ -9,15 +9,15 @@
{{did-insert this.didInsert}}
{{will-destroy this.willDestroyNode}}
{{! @glint-expect-error - https://github.com/josemarluedke/ember-focus-trap/issues/86 }}
{{focus-trap isActive=this._isOpen focusTrapOptions=(hash onDeactivate=this.onDismiss)}}
{{focus-trap isActive=this._isOpen}}
>
<:header>
{{yield
(hash
Header=(component
"hds/dialog-primitive/header"
id=this.id
onDismiss=this.onDismiss
onDismiss=this.onManualDismiss
contextualClassPrefix="hds-modal"
titleTag="h1"
)
Expand All @@ -30,7 +30,9 @@
<:footer>
{{yield
(hash
Footer=(component "hds/dialog-primitive/footer" onDismiss=this.onDismiss contextualClass="hds-modal__footer")
Footer=(component
"hds/dialog-primitive/footer" onDismiss=this.onManualDismiss contextualClass="hds-modal__footer"
)
)
}}
</:footer>
Expand Down
245 changes: 161 additions & 84 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,10 +9,8 @@ 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 { registerDestructor } from '@ember/destroyable';

import type { WithBoundArgs } from '@glint/template';
import type Owner from '@ember/owner';
import type { HdsModalSizes, HdsModalColors } from './types.ts';

import HdsDialogPrimitiveHeaderComponent from '../dialog-primitive/header.ts';
Expand Down Expand Up @@ -63,15 +61,7 @@ export default class HdsModal extends Component<HdsModalSignature> {
private _element!: HTMLDialogElement;
private _body!: HTMLElement;
private _bodyInitialOverflowValue = '';
private _clickHandler!: (event: MouseEvent) => void;

constructor(owner: Owner, args: HdsModalSignature['Args']) {
super(owner, args);

registerDestructor(this, (): void => {
document.removeEventListener('click', this._clickHandler, true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't follow all of the changes here, but why not do the style cleanup here and remove the use of will-destroy render modifier?
(the destructor will run when the component is cleaned up / unrendered)

});
}
private _documentClickHandler!: (event: MouseEvent) => void;

get isDismissDisabled(): boolean {
return this.args.isDismissDisabled ?? false;
Expand Down Expand Up @@ -119,130 +109,217 @@ export default class HdsModal extends Component<HdsModalSignature> {
return classes.join(' ');
}

@action registerOnCloseCallback(event: Event): void {
if (
!this.isDismissDisabled &&
this.args.onClose &&
typeof this.args.onClose === 'function'
) {
this.args.onClose(event);
}

// If the dismissal of the modal is disabled, we keep the modal open/visible otherwise we mark it as closed
if (this.isDismissDisabled) {
// If, in a chain of events, the element is not attached to the DOM, the `showModal` would fail
// so we add this safeguard condition that checks for the `<dialog>` to have a parent
if (this._element.parentElement) {
// As there is no way to `preventDefault` on `close` events, we call the `showModal` function
// preserving the state of the modal dialog
this._element.showModal();
}
} 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();
}
}
}
}
// INSERT / OPEN

@action
didInsert(element: HTMLDialogElement): void {
console.group('@action didInsert() invoked');
// Store references of `<dialog>` and `<body>` elements
this._element = element;
this._body = document.body;

if (this._body) {
// Store the initial `overflow` value of `<body>` so we can reset to it
this._bodyInitialOverflowValue =
this._body.style.getPropertyValue('overflow');
}

// Register "onClose" callback function to be called when a native 'close' event is dispatched
// eslint-disable-next-line @typescript-eslint/unbound-method
this._element.addEventListener('close', this.registerOnCloseCallback, true);

// If the modal dialog is not already open
if (!this._element.open) {
this.open();
this.openModalProgrammatically();
console.log(
'opening programmatically the Dialog element via `this.openModalProgrammatically();`'
);

this.setupXyx();
console.log('setup performed via `this.setupXyx()`');
}

this._clickHandler = (event: MouseEvent) => {
console.groupEnd();
}

@action
setupXyx(): void {
console.group('@action setupXyx() invoked');

// define event listener and assign it to the `document` (for click outside dismiss)
this._documentClickHandler = (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) {
void this.onDismiss();
void this.onManualDismiss();
console.log(
'executed `this.onManualDismiss()` inside `this._documentClickHandler`'
);
}
}
};
console.log('defined `this._documentClickHandler` callback');

document.addEventListener('click', this._clickHandler, {
document.addEventListener('click', this._documentClickHandler, {
capture: true,
passive: false,
});
}
console.log(
'added event listener to `document` for `click` and assigned `this._documentClickHandler` callback'
);

@action
willDestroyNode(): void {
if (this._element) {
this._element.removeEventListener(
'close',
// eslint-disable-next-line @typescript-eslint/unbound-method
this.registerOnCloseCallback,
true
// register "onDialogNativeClose" callback function to be called when a native 'close' event is dispatched
// eslint-disable-next-line @typescript-eslint/unbound-method
this._element.addEventListener('close', this.onDialogNativeClose, true);
console.log(
'added event listener for `close` applied to this._element with `this.onDialogNativeClose`'
);

// prevent page from scrolling when the dialog is open
if (this._body) {
// Store the initial `overflow` value of `<body>` so we can reset to it
this._bodyInitialOverflowValue =
this._body.style.getPropertyValue('overflow');
console.log(
'stored initial `overflow` value for `body` as',
this._bodyInitialOverflowValue
);

this._body.style.setProperty('overflow', 'hidden');
console.log('set `overflow: hidden on `body` element');
}

console.groupEnd();
}

@action
open(): void {
openModalProgrammatically(): void {
console.group('@action open() invoked');
// Make modal dialog visible using the native `showModal` method
this._element.showModal();
console.log(
'opened Dialog element via native call `this._element.showModal();`'
);
this._isOpen = true;

// Prevent page from scrolling when the dialog is open
if (this._body) this._body.style.setProperty('overflow', 'hidden');
console.log('set `this._isOpen = true`');

// Call "onOpen" callback function
if (this.args.onOpen && typeof this.args.onOpen === 'function') {
this.args.onOpen();
}
console.groupEnd();
}

// DISMISS / CLOSE

@action
// eslint-disable-next-line @typescript-eslint/require-await
async onDismiss(): Promise<void> {
async onManualDismiss(): Promise<void> {
console.group('async @action onManualDismiss() invoked');
// allow ember test helpers to be aware of when the `close` event fires
// when using `click` or other helpers from '@ember/test-helpers'
if (this._element.open) {
const token = waiter.beginAsync();
const listener = () => {
waiter.endAsync(token);
this._element.removeEventListener('close', listener);
console.log('removed event listener for `close` with `listener`');
};
this._element.addEventListener('close', listener);
console.log('added back event listener for `close` with `listener`');
}

// Make modal dialog invisible using the native `close` method
this._element.close();
console.log(
'closed Dialog element via native call `this._element.close();`'
);
console.groupEnd();
}

@action
onDialogNativeClose(event: Event): void {
console.group('@action onDialogNativeClose() invoked');

// If the dismissal of the modal is disabled, we keep the modal open/visible otherwise we mark it as closed
if (this.isDismissDisabled) {
// If, in a chain of events, the element is not attached to the DOM, the `showModal` would fail
// so we add this safeguard condition that checks for the `<dialog>` to have a parent
if (this._element.parentElement) {
// As there is no way to `preventDefault` on `close` events, we call the `showModal` function
// preserving the state of the modal dialog
this._element.showModal();
console.log(
're-opened the modal with `this._element.showModal()` when dismiss is disabled'
);
}
} else {
this._isOpen = false;
console.log('close the modal setting `this._isOpen = false`');

if (this.args.onClose && typeof this.args.onClose === 'function') {
this.args.onClose(event);
console.log('this.args.onClose() invoked');
}
}
console.groupEnd();
}

@action
willDestroyNode(): void {
console.group('@action willDestroyNode() invoked');

this.cleanupXyz();
console.log('cleanup performed via `this.cleanupXyz()`');

console.groupEnd();
}

@action
cleanupXyz(): void {
console.group('@action cleanupXyz() invoked');

// remove event listener assigned to the `document` (for click outside dismiss)
document.removeEventListener('click', this._documentClickHandler, true);
console.log('removed `click` event listener');

// remove event listener assigned to the `dialog` element
if (this._element) {
this._element.removeEventListener(
'close',
// eslint-disable-next-line @typescript-eslint/unbound-method
this.onDialogNativeClose,
true
);
console.log(
'removed event listener for `close` applied to this._element with `this.onDialogNativeClose`'
);
}

// reset page `overflow` property
if (this._body) {
this._body.style.removeProperty('overflow');
console.log('remove property `overflow` that we assigned before');
if (this._bodyInitialOverflowValue === '') {
console.log('initial overflow value was empty string');
if (this._body.style.length === 0) {
this._body.removeAttribute('style');
console.log(
'entirely removed the `style` attribute because its lenght is 0'
);
}
} else {
this._body.style.setProperty(
'overflow',
this._bodyInitialOverflowValue
);
console.log(
'set back initial overflow value that was stored as',
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();
console.log(
'return focus to element declared as `this.args.returnFocusTo`'
);
}
}
console.groupEnd();
}
}
18 changes: 18 additions & 0 deletions showcase/app/controllers/components/flyout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ export default class FlyoutController extends Controller {
@tracked largeFlyoutActive = false;
@tracked dropdownInitiatedFlyoutActive = false;
@tracked dropdownInitiatedWithReturnedFocusFlyoutActive = false;
@tracked deactivateFlyoutOnCloseActive = false;
@tracked deactivateFlyoutOnDestroyActive = false;
@tracked deactivateFlyoutOnSubmitActive = false;
@tracked deactivateFlyoutOnSubmitValidationError = false;

@action
activateFlyout(Flyout) {
Expand All @@ -22,4 +26,18 @@ export default class FlyoutController extends Controller {
deactivateFlyout(Flyout) {
this[Flyout] = false;
}

@action
deactivateFlyoutOnSubmit(event) {
event.preventDefault(); // Prevent page reload
const formData = new FormData(event.target);
const value = formData.get('deactivate-flyout-on-submit__input');

if (!value) {
this.deactivateFlyoutOnSubmitValidationError = true;
} else {
this.deactivateFlyoutOnSubmitValidationError = false;
this.deactivateFlyoutOnSubmitActive = false;
}
}
}
Loading
Loading