diff --git a/.changeset/stale-camels-fry.md b/.changeset/stale-camels-fry.md new file mode 100644 index 000000000..91cf48096 --- /dev/null +++ b/.changeset/stale-camels-fry.md @@ -0,0 +1,7 @@ +--- +'@lion/ui': minor +--- + +[overlays]: avoid interference of native dialog escape handler and escape handlers defined by OverlayController. +This is needed until we can configure `closedby="none"` on the native dialog for all browsers: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/dialog#closedby. +(We release this as a minor change, as we stop propagation of HTMLDialogElement 'cancel' and 'close' events, and some consumers might (ab)use them...) diff --git a/packages/ui/components/overlays/src/OverlayController.js b/packages/ui/components/overlays/src/OverlayController.js index ef785d839..f6eebe038 100644 --- a/packages/ui/components/overlays/src/OverlayController.js +++ b/packages/ui/components/overlays/src/OverlayController.js @@ -528,8 +528,6 @@ export class OverlayController extends EventTarget { this.__contentHasBeenInitialized = true; } - this.__wrappingDialogNode?.addEventListener('cancel', this.__cancelHandler); - // Reset all positioning styles (local, c.q. Popper) and classes (global) this.contentWrapperNode.removeAttribute('style'); this.contentWrapperNode.removeAttribute('class'); @@ -618,8 +616,9 @@ export class OverlayController extends EventTarget { contentWrapperNodeL2: this.contentWrapperNode, contentNodeL3: this.contentNode, }); - // @ts-ignore + // @ts-expect-error wrappingDialogElement.open = true; + if (this.isTooltip) { // needed to prevent tooltip getting focus in Safari and Firefox wrappingDialogElement.setAttribute('tabindex', '-1'); @@ -633,6 +632,35 @@ export class OverlayController extends EventTarget { // computed height of 0... this.contentNode.style.position = 'static'; } + + // Here we prevent any interference of the native element with the keyboard behavior + // as defined by the OverlayController. This is needed until we can configure `closedby="none"` + // on the native dialog for all browsers: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/dialog#closedby + const hasClosedBySupport = HTMLDialogElement && 'closedBy' in HTMLDialogElement.prototype; + if (hasClosedBySupport) { + // @ts-expect-error + wrappingDialogElement.closedBy = 'none'; + } else { + wrappingDialogElement.addEventListener( + 'keydown', + (/** @type {* & KeyboardEvent} */ event) => { + if (event.key === 'Escape') { + event.preventDefault(); + } + }, + ); + wrappingDialogElement.addEventListener('keyup', (/** @type {* & KeyboardEvent} */ event) => { + if (event.key === 'Escape') { + event.preventDefault(); + } + }); + wrappingDialogElement.addEventListener('cancel', event => { + event.stopPropagation(); + }); + wrappingDialogElement.addEventListener('close', event => { + event.stopPropagation(); + }); + } } /** @@ -1188,6 +1216,7 @@ export class OverlayController extends EventTarget { const hasPressedInside = event.composedPath().includes(this.contentNode) || deepContains(this.contentNode, /** @type {HTMLElement|ShadowRoot} */ (event.target)); + if (hasPressedInside) { this.hide(); // We could do event.stopPropagation() here, but we don't want to hide info for @@ -1384,8 +1413,6 @@ export class OverlayController extends EventTarget { teardown() { this.__handleOverlayStyles({ phase: 'teardown' }); this._handleFeatures({ phase: 'teardown' }); - this.__wrappingDialogNode?.removeEventListener('cancel', this.__cancelHandler); - if (this.#isRegisteredOnManager()) { this.manager.remove(this); } diff --git a/packages/ui/components/overlays/test-suites/OverlayMixin.suite.js b/packages/ui/components/overlays/test-suites/OverlayMixin.suite.js index 1882763c6..c0a4e96de 100644 --- a/packages/ui/components/overlays/test-suites/OverlayMixin.suite.js +++ b/packages/ui/components/overlays/test-suites/OverlayMixin.suite.js @@ -9,6 +9,7 @@ import { html, } from '@open-wc/testing'; import { overlays as overlaysManager, OverlayController } from '@lion/ui/overlays.js'; +import { sendKeys } from '@web/test-runner-commands'; import { browserDetection } from '@lion/ui/core.js'; import { cache } from 'lit/directives/cache.js'; import '@lion/ui/define/lion-dialog.js'; @@ -31,6 +32,28 @@ function resetOverlaysManager() { overlaysManager.list.forEach(overlayCtrl => overlaysManager.remove(overlayCtrl)); } +/** + * A small wrapper function that closely mimics an escape press from a user + * (prevents common mistakes like no bubbling or keydown) + * @param {HTMLElement|Document} element + */ +async function mimicEscapePress(element) { + // Make sure that the element inside the dialog is focusable (and cleanup after) + if (element instanceof HTMLElement) { + const { tabIndex: tabIndexBefore } = element; + // eslint-disable-next-line no-param-reassign + element.tabIndex = -1; + element.focus(); + // eslint-disable-next-line no-param-reassign + element.tabIndex = tabIndexBefore; // make sure element is focusable + } + + // Send the event + await sendKeys({ press: 'Escape' }); + // Wait for at least a microtask, so that possible property effects are performed + await aTimeout(0); +} + /** * @param {{tagString:string, tag: object, suffix?:string}} config */ @@ -367,6 +390,26 @@ export function runOverlayMixinSuite({ tagString, tag, suffix = '' }) { stub.restore(); }); + it('should not visually hide when hidesOnEsc / hidesOnOutsideEsc is not configured', async () => { + const el = /** @type {OverlayEl} */ ( + await fixture(html` + <${tag} .config="${{ hidesOnEsc: false, hidesOnOutsideEsc: false }}" opened> +
content of the overlay
+ + + `) + ); + + const dialogEl = /** @type {HTMLDialogElement} */ (el._overlayCtrl.__wrappingDialogNode); + + // @ts-expect-error [allow-protected-in-tests] + expect(dialogEl.checkVisibility()).to.be.true; + // @ts-expect-error [allow-protected-in-tests] + await mimicEscapePress(el._overlayContentNode); + // @ts-expect-error [allow-protected-in-tests] + expect(dialogEl.checkVisibility()).to.be.true; + }); + describe('Teardown', () => { it('does not teardown when moving dom nodes', async () => { const el = /** @type {OverlayEl} */ ( diff --git a/packages/ui/components/overlays/test/OverlayController.test.js b/packages/ui/components/overlays/test/OverlayController.test.js index 539455361..ff515f380 100644 --- a/packages/ui/components/overlays/test/OverlayController.test.js +++ b/packages/ui/components/overlays/test/OverlayController.test.js @@ -1,6 +1,7 @@ /* eslint-disable no-new */ import { OverlayController, overlays } from '@lion/ui/overlays.js'; import { mimicClick } from '@lion/ui/overlays-test-helpers.js'; +import { sendKeys } from '@web/test-runner-commands'; import sinon from 'sinon'; import { unsafeStatic, @@ -28,12 +29,22 @@ const wrappingDialogNodeStyle = 'display: none; z-index: 9999; padding: 0px;'; /** * A small wrapper function that closely mimics an escape press from a user * (prevents common mistakes like no bubbling or keydown) - * @param {Element|Document} element + * @param {HTMLElement|Document} element */ async function mimicEscapePress(element) { - element.dispatchEvent( - new KeyboardEvent('keyup', { key: 'Escape', bubbles: true, composed: true }), - ); + // Make sure that the element inside the dialog is focusable (and cleanup after) + if (element instanceof HTMLElement) { + const { tabIndex: tabIndexBefore } = element; + // eslint-disable-next-line no-param-reassign + element.tabIndex = -1; + element.focus(); + // eslint-disable-next-line no-param-reassign + element.tabIndex = tabIndexBefore; // make sure element is focusable + } + + // Send the event + await sendKeys({ press: 'Escape' }); + // Wait for at least a microtask, so that possible property effects are performed await aTimeout(0); } @@ -726,6 +737,7 @@ describe('OverlayController', () => { ) ); const { parentOverlay, childOverlay } = await createNestedEscControllers(parentContent); + await mimicEscapePress(childOverlay.contentNode); expect(parentOverlay.isShown).to.be.true;