Skip to content

Commit cbdfa0f

Browse files
authored
fix(dialog): keepOpenOnEscape should prevent dialog from closing on Escape key press (#1986)
Chromium based browsers have a default behavior of firing a `close` event when the Escape key is repeatedly pressed. Since that event cannot be cancelled, keep the dialog open when `keepOpenOnEscape` is set to true. A better fix would be to use the `closedby` property but unfortunately that is not supported in Safari. Closes #1983
1 parent 66dfc34 commit cbdfa0f

File tree

2 files changed

+32
-7
lines changed

2 files changed

+32
-7
lines changed

src/components/dialog/dialog.spec.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,17 @@ describe('Dialog', () => {
266266
await waitUntil(() => eventSpy.calledWith('igcClosed'));
267267
expect(dialog.open).to.be.false;
268268
});
269+
270+
it('issue 1983 - does not close the dialog when keepOpenOnEscape is true and a non-cancelable close event is fired', async () => {
271+
dialog.keepOpenOnEscape = true;
272+
await dialog.show();
273+
274+
nativeDialog.dispatchEvent(new Event('close'));
275+
await elementUpdated(dialog);
276+
277+
expect(dialog.open).to.be.true;
278+
expect(nativeDialog.open).to.be.true;
279+
});
269280
});
270281

271282
describe('Form', () => {

src/components/dialog/dialog.ts

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import { registerComponent } from '../common/definitions/register.js';
1111
import type { Constructor } from '../common/mixins/constructor.js';
1212
import { EventEmitterMixin } from '../common/mixins/event-emitter.js';
1313
import { partMap } from '../common/part-map.js';
14-
import { numberInRangeInclusive } from '../common/util.js';
14+
import { bindIf, numberInRangeInclusive } from '../common/util.js';
1515
import { styles } from './themes/dialog.base.css.js';
1616
import { styles as shared } from './themes/shared/dialog.common.css.js';
1717
import { all } from './themes/themes.js';
@@ -163,6 +163,10 @@ export default class IgcDialogComponent extends EventEmitterMixin<
163163
return true;
164164
}
165165

166+
private _closeWithEvent(): void {
167+
this._hide(true);
168+
}
169+
166170
protected _handleFormSubmit(event: SubmitEvent): void {
167171
const form = event.target as HTMLFormElement;
168172

@@ -172,7 +176,7 @@ export default class IgcDialogComponent extends EventEmitterMixin<
172176
}
173177

174178
if (!event.defaultPrevented) {
175-
this._hide(true);
179+
this._closeWithEvent();
176180
}
177181
}
178182
}
@@ -181,7 +185,16 @@ export default class IgcDialogComponent extends EventEmitterMixin<
181185
event.preventDefault();
182186

183187
if (!this.keepOpenOnEscape) {
184-
this._hide(true);
188+
this._closeWithEvent();
189+
}
190+
}
191+
192+
private _handleClose(): void {
193+
// When a non-cancelable close event is fired (e.g., from repeated Escape presses),
194+
// reopen the dialog to prevent the broken state with visible backdrop.
195+
// Note that this handler is invoked only when `keepOpenOnEscape` is true.
196+
if (this.open) {
197+
this._dialog.showModal();
185198
}
186199
}
187200

@@ -192,7 +205,7 @@ export default class IgcDialogComponent extends EventEmitterMixin<
192205
const inY = numberInRangeInclusive(clientY, rect.top, rect.bottom);
193206

194207
if (!(inX && inY)) {
195-
this._hide(true);
208+
this._closeWithEvent();
196209
}
197210
}
198211
}
@@ -262,7 +275,7 @@ export default class IgcDialogComponent extends EventEmitterMixin<
262275
${this.hideDefaultAction
263276
? nothing
264277
: html`
265-
<igc-button variant="flat" @click=${() => this._hide(true)}>
278+
<igc-button variant="flat" @click=${this._closeWithEvent}>
266279
OK
267280
</igc-button>
268281
`}
@@ -283,10 +296,11 @@ export default class IgcDialogComponent extends EventEmitterMixin<
283296
${ref(this._dialogRef)}
284297
part=${partMap({ base: true, titled: hasTitle, footed: hasFooter })}
285298
role="dialog"
286-
aria-label=${this.ariaLabel || nothing}
287-
aria-labelledby=${labelledBy || nothing}
299+
aria-label=${bindIf(this.ariaLabel, this.ariaLabel)}
300+
aria-labelledby=${bindIf(labelledBy, labelledBy)}
288301
@click=${this._handleClick}
289302
@cancel=${this._handleCancel}
303+
@close=${bindIf(this.keepOpenOnEscape, this._handleClose)}
290304
>
291305
${this._renderHeader()} ${this._renderContent()} ${this._renderFooter()}
292306
</dialog>

0 commit comments

Comments
 (0)