From abcc6fdd6949397ce4690551f453d238c2802edb Mon Sep 17 00:00:00 2001 From: Oleksii Kadurin Date: Thu, 30 Oct 2025 12:47:53 +0100 Subject: [PATCH] fix: hidesOnEsc for nested overlays --- .changeset/yellow-hotels-sell.md | 5 + .../test/lion-dialog.integration.test.js | 170 ++++++++++++++++++ .../overlays/src/OverlayController.js | 41 ++++- .../test-suites/OverlayMixin.suite.js | 57 ++++++ .../overlays/test/OverlayMixin.test.js | 4 +- 5 files changed, 268 insertions(+), 9 deletions(-) create mode 100644 .changeset/yellow-hotels-sell.md create mode 100644 packages/ui/components/dialog/test/lion-dialog.integration.test.js diff --git a/.changeset/yellow-hotels-sell.md b/.changeset/yellow-hotels-sell.md new file mode 100644 index 000000000..17b6ddad7 --- /dev/null +++ b/.changeset/yellow-hotels-sell.md @@ -0,0 +1,5 @@ +--- +'@lion/ui': patch +--- + +fix hidesOnEsc for nested overlays diff --git a/packages/ui/components/dialog/test/lion-dialog.integration.test.js b/packages/ui/components/dialog/test/lion-dialog.integration.test.js new file mode 100644 index 000000000..35e547f9e --- /dev/null +++ b/packages/ui/components/dialog/test/lion-dialog.integration.test.js @@ -0,0 +1,170 @@ +/* eslint-disable lit-a11y/no-autofocus */ +import { expect, fixture as _fixture, html, waitUntil } from '@open-wc/testing'; +import { sendKeys } from '@web/test-runner-commands'; + +import '@lion/ui/define/lion-button.js'; +import '@lion/ui/define/lion-combobox.js'; +import '@lion/ui/define/lion-select-rich.js'; +import '@lion/ui/define/lion-option.js'; +import '@lion/ui/define/lion-dialog.js'; +import '@lion/ui/define/lion-tooltip.js'; + +import { mimicUserTyping } from '@lion/ui/combobox-test-helpers.js'; +import sinon from 'sinon'; + +/** + * @typedef {import('../src/LionDialog.js').LionDialog} LionDialog + * @typedef {import('lit').TemplateResult} TemplateResult + */ +const fixture = /** @type {(arg: TemplateResult) => Promise} */ (_fixture); + +const dropDownEntries = ['Apple', 'Banana']; + +describe('lion-dialog', () => { + describe('lion-combobox integration', () => { + it('should close lion-combobox dropdown on Escape and should not close the parent lion-dialog', async () => { + const el = await fixture(html` + + Open Dialog +
Combobox example
+
+ + ${dropDownEntries.map( + (/** @type { string } */ entry) => + html` ${entry}`, + )} + +
+ + `); + const dialogInvoker = /** @type {HTMLElement} */ (el.querySelector('[slot="invoker"]')); + dialogInvoker.click(); + const combobox = /** @type {HTMLElement} */ el.querySelector('lion-combobox'); + const isComboboxRendered = () => !!combobox?.shadowRoot?.childNodes.length; + await waitUntil(isComboboxRendered); + // @ts-ignore + await mimicUserTyping(combobox, 'a'); + const firstOption = /** @type { HTMLElement | undefined } */ ( + el.querySelectorAll('lion-option')?.[0] + ); + // @ts-ignore + const isComboboxFirstOptionInDropdownVisible = () => firstOption?.checkVisibility(); + await waitUntil(isComboboxFirstOptionInDropdownVisible); + const comboboxDialog = combobox?.shadowRoot?.querySelector('dialog'); + // Note, do not remove `console.log` down below. There is a bug in test engine. + // Referring dialog.close from the test environement fixes the bug + console.log(sinon.spy(comboboxDialog?.close)); + const comboboxInput = combobox?.querySelector('input'); + comboboxInput?.focus(); + const dropdownDialog = combobox?.shadowRoot?.querySelector('dialog'); + // @ts-ignore + const dropdownDialogCloseSpy = sinon.spy(dropdownDialog, 'close'); + await sendKeys({ + press: 'Escape', + }); + // @ts-ignore + const isDialogVisible = () => el?.shadowRoot?.querySelector('dialog')?.checkVisibility(); + await waitUntil(() => dropdownDialogCloseSpy.called); + expect(isDialogVisible()).to.equal(true); + }); + }); + + describe('lion-select-rich integration', () => { + it('should close lion-select-rich dropdown on Escape and should not close the parent lion-dialog', async () => { + const el = await fixture(html` + + Open Dialog +
Select rich example
+
+ + ${dropDownEntries.map( + (/** @type { string } */ entry) => + html` ${entry}`, + )} + +
+ + `); + const dialogInvoker = /** @type {HTMLElement} */ (el.querySelector('[slot="invoker"]')); + dialogInvoker.click(); + const selectRichInvoker = /** @type {HTMLElement} */ ( + el.querySelector('lion-select-invoker') + ); + const isSelectRichInvokerRendered = () => !!selectRichInvoker?.shadowRoot?.childNodes.length; + await waitUntil(isSelectRichInvokerRendered); + const selectRich = el?.querySelector('lion-select-rich'); + const selectRichDialog = selectRich?.shadowRoot?.querySelector('dialog'); + // Note, do not remove `console.log` down below. There is a bug in test engine. + // Referring dialog.close from the test environement fixes the bug + console.log(selectRichDialog?.close); + + selectRichInvoker?.click(); + const dropdownDialog = el + ?.querySelector('lion-select-rich') + ?.shadowRoot?.querySelector('dialog'); + // @ts-ignore + const dropdownDialogCloseSpy = sinon.spy(dropdownDialog, 'close'); + const isDropdownVisible = () => + el + ?.querySelector('lion-select-rich') + ?.shadowRoot?.querySelector('dialog') + // @ts-ignore + ?.checkVisibility(); + await waitUntil(isDropdownVisible); + const lionOptions = /** @type {HTMLElement} */ (el.querySelector('lion-options')); + lionOptions?.focus(); + await sendKeys({ + press: 'Escape', + }); + // @ts-ignore + const isDialogVisible = () => el?.shadowRoot?.querySelector('dialog')?.checkVisibility(); + await waitUntil(() => dropdownDialogCloseSpy.called); + expect(isDialogVisible()).to.equal(true); + }); + }); + + describe('lion-tooltip integration', () => { + it('should close lion-tooltip on first Escape and should close the parent lion-dialog on the second Escape', async () => { + const el = await fixture(html` + + Open Dialog +
Tooltip example
+
+ + +
This is a tooltip
+ +
+ + `); + + await el.updateComplete; + const dialogInvoker = /** @type {HTMLElement} */ (el.querySelector('.dialog-invoker')); + dialogInvoker.click(); + const tooltip = /** @type {HTMLElement} */ el.querySelector('lion-tooltip'); + const isTooltipRendered = () => !!tooltip?.shadowRoot?.childNodes.length; + await waitUntil(isTooltipRendered); + const tooltipButton = /** @type {HTMLElement} */ (el.querySelector('.demo-tooltip-invoker')); + tooltipButton?.focus(); + const getTooltipContent = () => + el + .querySelector('lion-tooltip') + ?.shadowRoot?.querySelector('#overlay-content-node-wrapper'); + // @ts-ignore + const isTooltipContentVisible = () => getTooltipContent()?.checkVisibility(); + await waitUntil(isTooltipContentVisible); + await sendKeys({ + press: 'Escape', + }); + // @ts-ignore + const isDialogVisible = () => el?.shadowRoot?.querySelector('dialog')?.checkVisibility(); + expect(isTooltipContentVisible()).to.equal(false); + expect(isDialogVisible()).to.equal(true); + await sendKeys({ + press: 'Escape', + }); + expect(isTooltipContentVisible()).to.equal(false); + expect(isDialogVisible()).to.equal(false); + }); + }); +}); diff --git a/packages/ui/components/overlays/src/OverlayController.js b/packages/ui/components/overlays/src/OverlayController.js index f6eebe038..050274973 100644 --- a/packages/ui/components/overlays/src/OverlayController.js +++ b/packages/ui/components/overlays/src/OverlayController.js @@ -197,15 +197,24 @@ export class OverlayController extends EventTarget { this._contentId = `overlay-content--${Math.random().toString(36).slice(2, 10)}`; /** @private */ this.__originalAttrs = new Map(); + /** @private */ + this.__escKeyHandler = this.__escKeyHandler.bind(this); this.updateConfig(config); /** @private */ this.__hasActiveTrapsKeyboardFocus = false; /** @private */ this.__hasActiveBackdrop = true; /** @private */ - this.__escKeyHandler = this.__escKeyHandler.bind(this); - /** @private */ this.__cancelHandler = this.__cancelHandler.bind(this); + /** + * The property is used to skip `__escKeyHandler` handler for overlays that have been closed + * by `__escKeyHandlerCalled` previously. + * `__escKeyHandlerCalled` is set to `false` right before overlay show up + * `__escKeyHandlerCalled` is set to `true` when `__escKeyHandler` is called at least one time + * after each 'show' phase + * @private + */ + this.__escKeyHandlerCalled = false; } /** @@ -1211,13 +1220,21 @@ export class OverlayController extends EventTarget { * @returns {void} */ __escKeyHandler(event) { - if (event.key !== 'Escape' || childDialogsClosedInEventLoopWeakmap.has(event)) return; + if ( + event.key !== 'Escape' || + childDialogsClosedInEventLoopWeakmap.has(event) || + (!this.isShown && this.__escKeyHandlerCalled) + ) { + return; + } const hasPressedInside = event.composedPath().includes(this.contentNode) || + (this.invokerNode && event.composedPath().includes(this.invokerNode)) || deepContains(this.contentNode, /** @type {HTMLElement|ShadowRoot} */ (event.target)); if (hasPressedInside) { + this.__escKeyHandlerCalled = true; this.hide(); // We could do event.stopPropagation() here, but we don't want to hide info for // the outside world about user interactions. Instead, we store the event in a WeakMap @@ -1245,12 +1262,21 @@ export class OverlayController extends EventTarget { * @protected */ _handleHidesOnEsc({ phase }) { - if (phase === 'show') { + if (phase === 'init') { + // we remove previously added (if any) event listener to guarantee + // there is only one Escape handler added here. + // Note `init` phase triggered on every `updateConfig` call and that + // could happen multiple times during the component life cycle + this.contentNode.removeEventListener('keyup', this.__escKeyHandler); this.contentNode.addEventListener('keyup', this.__escKeyHandler); if (this.invokerNode) { this.invokerNode.addEventListener('keyup', this.__escKeyHandler); } - } else if (phase === 'hide' || phase === 'teardown') { + } + if (phase === 'show') { + this.__escKeyHandlerCalled = false; + } + if (phase === 'teardown') { this.contentNode.removeEventListener('keyup', this.__escKeyHandler); if (this.invokerNode) { this.invokerNode.removeEventListener('keyup', this.__escKeyHandler); @@ -1263,9 +1289,10 @@ export class OverlayController extends EventTarget { * @protected */ _handleHidesOnOutsideEsc({ phase }) { - if (phase === 'show') { + if (phase === 'init') { + document.removeEventListener('keyup', this.#outsideEscKeyHandler); document.addEventListener('keyup', this.#outsideEscKeyHandler); - } else if (phase === 'hide' || phase === 'teardown') { + } else if (phase === 'teardown') { document.removeEventListener('keyup', this.#outsideEscKeyHandler); } } diff --git a/packages/ui/components/overlays/test-suites/OverlayMixin.suite.js b/packages/ui/components/overlays/test-suites/OverlayMixin.suite.js index 983f55cb7..9918a0a2e 100644 --- a/packages/ui/components/overlays/test-suites/OverlayMixin.suite.js +++ b/packages/ui/components/overlays/test-suites/OverlayMixin.suite.js @@ -111,6 +111,63 @@ export function runOverlayMixinSuite({ tagString, tag, suffix = '' }) { expect(el.opened).to.be.false; }); + /** + * In case OverlayMixin has a handler for Escape which sets `opened` to `false`, + * make sure it does not conflict with the Escape handler in OverlayController. + * */ + it('syncs opened between overlayController and OverlayMixin on Escape', async () => { + const config = { hidesOnEsc: true }; + const el = /** @type {OverlayEl} */ ( + await fixture(html` + <${tag} id="main-dialog" .config="${config}"> +
+ open nested overlay: + <${tag} id="sub-dialog" .config="${config}"> +
+ +
+ + +
+ + + `) + ); + await el.updateComplete; + const subDialog = /** @type {OverlayEl} */ (el.querySelector('#sub-dialog')); + const nestedContentInput = /** @type {OverlayEl} */ (el.querySelector('#nestedContentInput')); + + // Note, OverlayController has Escape key listener added to "#nestedContent". + // And the sub-dialog adds the Escape key listener to "#nestedContentInput" which is a child evelement of "#nestedContent" + // In this case when "#nestedContentInput" is focused and Escape is pressed, the listener of "#nestedContentInput" + // is always called before the listener set for "#nestedContent" inside OverlayController. + // This way we check the scenario when OverlayController's `hide()` is called first from + // The OverlayMixin and it does not prevent OverlayController `hidesOnEsc` handler from being called + nestedContentInput?.addEventListener('keyup', (/** @type {* & KeyboardEvent} */ event) => { + if (event.key === 'Escape') { + subDialog.opened = false; + } + }); + + el.opened = true; + await el.updateComplete; + await el._overlayCtrl._showComplete; + expect(el.opened).to.be.true; + expect(el._overlayCtrl.isShown).to.be.true; + + subDialog.opened = true; + await subDialog.updateComplete; + await subDialog._overlayCtrl._showComplete; + expect(subDialog.opened).to.be.true; + expect(subDialog._overlayCtrl.isShown).to.be.true; + + await mimicEscapePress(nestedContentInput); + expect(subDialog.opened).to.be.false; + expect(subDialog._overlayCtrl.isShown).to.be.false; + expect(el.opened).to.be.true; + expect(el._overlayCtrl.isShown).to.be.true; + }); + // TODO: put this tests in OverlayController.test.js instead? it('does not change the body size when opened', async () => { const parentNode = document.createElement('div'); diff --git a/packages/ui/components/overlays/test/OverlayMixin.test.js b/packages/ui/components/overlays/test/OverlayMixin.test.js index 0232fac18..20a719b5d 100644 --- a/packages/ui/components/overlays/test/OverlayMixin.test.js +++ b/packages/ui/components/overlays/test/OverlayMixin.test.js @@ -7,9 +7,9 @@ const tagString = defineCE( class extends OverlayMixin(LitElement) { render() { return html` - +
-
content of the overlay
+
`; }