From 9ac290eddc8c26f739bcc22d87b439af68740259 Mon Sep 17 00:00:00 2001 From: Thijs Louisse Date: Thu, 31 Oct 2024 17:04:17 +0100 Subject: [PATCH 1/2] fix(overlays): no hiding of nested overlays having `hideOnEsc` configured --- .changeset/rude-flies-reflect.md | 5 + .../overlays/src/OverlayController.js | 53 +++- .../overlays/test-helpers/mimicClick.js | 4 +- .../overlays/test/OverlayController.test.js | 263 ++++++++++++++++-- 4 files changed, 280 insertions(+), 45 deletions(-) create mode 100644 .changeset/rude-flies-reflect.md diff --git a/.changeset/rude-flies-reflect.md b/.changeset/rude-flies-reflect.md new file mode 100644 index 0000000000..6ca6c7fad5 --- /dev/null +++ b/.changeset/rude-flies-reflect.md @@ -0,0 +1,5 @@ +--- +'@lion/ui': patch +--- + +[overlays] no hiding of nested overlays having `hideOnEsc` configured diff --git a/packages/ui/components/overlays/src/OverlayController.js b/packages/ui/components/overlays/src/OverlayController.js index 39ffc26508..9879c49ef6 100644 --- a/packages/ui/components/overlays/src/OverlayController.js +++ b/packages/ui/components/overlays/src/OverlayController.js @@ -1,16 +1,17 @@ import { overlays } from './singleton.js'; import { containFocus } from './utils/contain-focus.js'; +import { deepContains } from './utils/deep-contains.js'; import { overlayShadowDomStyle } from './overlayShadowDomStyle.js'; import { _adoptStyleUtils } from './utils/adopt-styles.js'; /** - * @typedef {import('@lion/ui/types/overlays.js').OverlayConfig} OverlayConfig + * @typedef {'setup'|'init'|'teardown'|'before-show'|'show'|'hide'|'add'|'remove'} OverlayPhase * @typedef {import('@lion/ui/types/overlays.js').ViewportConfig} ViewportConfig - * @typedef {import('@popperjs/core').createPopper} Popper + * @typedef {import('@lion/ui/types/overlays.js').OverlayConfig} OverlayConfig * @typedef {import('@popperjs/core').Options} PopperOptions * @typedef {import('@popperjs/core').Placement} Placement + * @typedef {import('@popperjs/core').createPopper} Popper * @typedef {{ createPopper: Popper }} PopperModule - * @typedef {'setup'|'init'|'teardown'|'before-show'|'show'|'hide'|'add'|'remove'} OverlayPhase */ /** @@ -103,6 +104,8 @@ async function preloadPopper() { return /** @type {* & Promise} */ (import('@popperjs/core/dist/esm/popper.js')); } +const childDialogsClosedInEventLoopWeakmap = new WeakMap(); + /** * OverlayController is the fundament for every single type of overlay. With the right * configuration, it can be used to build (modal) dialogs, tooltips, dropdowns, popovers, @@ -498,9 +501,6 @@ export class OverlayController extends EventTarget { '[OverlayController] .isTooltip only takes effect when .handlesAccessibility is enabled', ); } - // if (newConfig.popperConfig.modifiers.arrow && !newConfig.contentWrapperNode) { - // throw new Error('You need to provide a .contentWrapperNode when Popper arrow is enabled'); - // } } /** @@ -1146,11 +1146,40 @@ export class OverlayController extends EventTarget { ev.preventDefault(); } - /** @private */ - __escKeyHandler(/** @type {KeyboardEvent} */ ev) { - return ev.key === 'Escape' && this.hide(); + /** + * @param {KeyboardEvent} event + * @returns {void} + */ + __escKeyHandler(event) { + if (event.key !== 'Escape' || childDialogsClosedInEventLoopWeakmap.has(event)) return; + + 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 + // the outside world about user interactions. Instead, we store the event in a WeakMap + // that will be garbage collected after the event loop. + childDialogsClosedInEventLoopWeakmap.set(event, this); + } } + /** + * @param {KeyboardEvent} event + * @returns {void} + */ + #outsideEscKeyHandler = event => { + if (event.key !== 'Escape') return; + + const hasPressedInside = + event.composedPath().includes(this.contentNode) || + deepContains(this.contentNode, /** @type {HTMLElement|ShadowRoot} */ (event.target)); + if (!hasPressedInside) { + this.hide(); + } + }; + /** * @param {{ phase: OverlayPhase }} config * @protected @@ -1175,11 +1204,9 @@ export class OverlayController extends EventTarget { */ _handleHidesOnOutsideEsc({ phase }) { if (phase === 'show') { - this.__escKeyHandler = (/** @type {KeyboardEvent} */ ev) => - ev.key === 'Escape' && this.hide(); - document.addEventListener('keyup', this.__escKeyHandler); + document.addEventListener('keyup', this.#outsideEscKeyHandler); } else if (phase === 'hide') { - document.removeEventListener('keyup', this.__escKeyHandler); + document.removeEventListener('keyup', this.#outsideEscKeyHandler); } } diff --git a/packages/ui/components/overlays/test-helpers/mimicClick.js b/packages/ui/components/overlays/test-helpers/mimicClick.js index f2a3e6e6f2..25cf2548a9 100644 --- a/packages/ui/components/overlays/test-helpers/mimicClick.js +++ b/packages/ui/components/overlays/test-helpers/mimicClick.js @@ -8,9 +8,9 @@ async function sleep(t = 0) { /** * @param {HTMLElement} el - * @param {{isAsync?:boolean, releaseElement?: HTMLElement}} config + * @param {{isAsync?:boolean, releaseElement?: HTMLElement}} config releaseElement can be different when the mouse is dragged before release */ -export async function mimicClick(el, { isAsync, releaseElement } = { isAsync: false }) { +export async function mimicClick(el, { isAsync = false, releaseElement } = {}) { const releaseEl = releaseElement || el; el.dispatchEvent(new MouseEvent('mousedown')); if (isAsync) { diff --git a/packages/ui/components/overlays/test/OverlayController.test.js b/packages/ui/components/overlays/test/OverlayController.test.js index 3978d2fe98..353374002c 100644 --- a/packages/ui/components/overlays/test/OverlayController.test.js +++ b/packages/ui/components/overlays/test/OverlayController.test.js @@ -1,28 +1,41 @@ /* eslint-disable no-new */ +import { OverlayController, overlays } from '@lion/ui/overlays.js'; +import { mimicClick } from '@lion/ui/overlays-test-helpers.js'; +import sinon from 'sinon'; import { - aTimeout, + unsafeStatic, + fixtureSync, defineCE, - expect, + aTimeout, fixture, + expect, html, - unsafeStatic, - fixtureSync, } from '@open-wc/testing'; -import sinon from 'sinon'; -import { OverlayController, overlays } from '@lion/ui/overlays.js'; -import { mimicClick } from '@lion/ui/overlays-test-helpers.js'; -import { keyCodes } from '../src/utils/key-codes.js'; -import { simulateTab } from '../src/utils/simulate-tab.js'; -import { _adoptStyleUtils } from '../src/utils/adopt-styles.js'; + import { createShadowHost } from '../test-helpers/createShadowHost.js'; +import { _adoptStyleUtils } from '../src/utils/adopt-styles.js'; +import { simulateTab } from '../src/utils/simulate-tab.js'; +import { keyCodes } from '../src/utils/key-codes.js'; /** - * @typedef {import('../types/OverlayConfig.js').OverlayConfig} OverlayConfig * @typedef {import('../types/OverlayConfig.js').ViewportPlacement} ViewportPlacement + * @typedef {import('../types/OverlayConfig.js').OverlayConfig} OverlayConfig */ 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 + */ +async function mimicEscapePress(element) { + element.dispatchEvent( + new KeyboardEvent('keyup', { key: 'Escape', bubbles: true, composed: true }), + ); + await aTimeout(0); +} + /** * Make sure that all browsers serialize html in a similar way * (Firefox tends to output empty style attrs) @@ -74,6 +87,50 @@ const withLocalTestConfig = () => ), }); +/** + * @param {HTMLDivElement} parentContent + * @returns {Promise<{parentOverlay: OverlayController; childOverlay: OverlayController}>} + */ +async function createNestedEscControllers(parentContent) { + const childContent = /** @type {HTMLDivElement} */ (parentContent.querySelector('div[id]')); + // Assert valid fixure + const isValid = + (parentContent.id === 'parent-overlay--hidesOnEsc' || + parentContent.id === 'parent-overlay--hidesOnOutsideEsc') && + (childContent.id === 'child-overlay--hidesOnEsc' || + childContent.id === 'child-overlay--hidesOnOutsideEsc'); + if (!isValid) { + throw new Error('Provide a valid fixture'); + } + + if (parentContent.hasAttribute('data-convert-to-shadow-root')) { + const shadowRootParent = parentContent.attachShadow({ mode: 'open' }); + shadowRootParent.appendChild(childContent); + } + + const parentHasOutsideOnEsc = parentContent.id === 'parent-overlay--hidesOnOutsideEsc'; + const childHasOutsideOnEsc = childContent.id === 'child-overlay--hidesOnOutsideEsc'; + + const parentConfig = parentHasOutsideOnEsc ? { hidesOnOutsideEsc: true } : { hidesOnEsc: true }; + const childConfig = childHasOutsideOnEsc ? { hidesOnOutsideEsc: true } : { hidesOnEsc: true }; + + const parentOverlay = new OverlayController({ + ...withGlobalTestConfig(), + contentNode: parentContent, + ...parentConfig, + }); + const childOverlay = new OverlayController({ + ...withGlobalTestConfig(), + contentNode: childContent, + ...childConfig, + }); + + await parentOverlay.show(); + await childOverlay.show(); + + return { parentOverlay, childOverlay }; +} + afterEach(() => { overlays.teardown(); }); @@ -574,74 +631,221 @@ describe('OverlayController', () => { }); describe('hidesOnEsc', () => { - it('hides when [escape] is pressed', async () => { + it('hides on [Escape] press', async () => { const ctrl = new OverlayController({ ...withGlobalTestConfig(), hidesOnEsc: true, }); await ctrl.show(); - ctrl.contentNode.dispatchEvent(new KeyboardEvent('keyup', { key: 'Escape' })); - await aTimeout(0); + await mimicEscapePress(ctrl.contentNode); expect(ctrl.isShown).to.be.false; }); - it("doesn't hide when [escape] is pressed and hidesOnEsc is set to false", async () => { + it('stays shown on [Escape] press with `.hidesOnEsc` set to false', async () => { const ctrl = new OverlayController({ ...withGlobalTestConfig(), hidesOnEsc: false, }); await ctrl.show(); - ctrl.contentNode.dispatchEvent(new KeyboardEvent('keyup', { key: 'Escape' })); - await aTimeout(0); + await mimicEscapePress(ctrl.contentNode); expect(ctrl.isShown).to.be.true; }); - it('stays shown when [escape] is pressed on outside element', async () => { + it('stays shown on [Escape] press on outside element', async () => { const ctrl = new OverlayController({ ...withGlobalTestConfig(), hidesOnEsc: true, }); await ctrl.show(); - document.dispatchEvent(new KeyboardEvent('keyup', { key: 'Escape' })); + await mimicEscapePress(document); expect(ctrl.isShown).to.be.true; }); - it('does not hide when [escape] is pressed with modal and "hidesOnEsc" is false', async () => { + it('stays shown on [Escape] press with modal and `.hidesOnEsc` is false', async () => { const ctrl = new OverlayController({ ...withGlobalTestConfig(), trapsKeyboardFocus: true, hidesOnEsc: false, }); await ctrl.show(); - ctrl.contentNode.dispatchEvent(new KeyboardEvent('keyup', { key: 'Escape' })); - await aTimeout(0); + await mimicEscapePress(ctrl.contentNode); expect(ctrl.isShown).to.be.true; }); + + it('parent stays shown on [Escape] press in a nested overlay', async () => { + const parentContent = /** @type {HTMLDivElement} */ ( + await fixture( + html` +
+
we press [Escape] here
+
`, + ) + ); + const { parentOverlay, childOverlay } = await createNestedEscControllers(parentContent); + await mimicEscapePress(childOverlay.contentNode); + + expect(parentOverlay.isShown).to.be.true; + expect(childOverlay.isShown).to.be.false; + }); }); describe('hidesOnOutsideEsc', () => { - it('hides when [escape] is pressed on outside element', async () => { + it('hides on [Escape] press on outside element', async () => { const ctrl = new OverlayController({ ...withGlobalTestConfig(), hidesOnOutsideEsc: true, }); await ctrl.show(); - document.dispatchEvent(new KeyboardEvent('keyup', { key: 'Escape' })); - await aTimeout(0); + await mimicEscapePress(document); expect(ctrl.isShown).to.be.false; }); - it('stays shown when [escape] is pressed on inside element', async () => { + it('stays shown on [Escape] press on inside element', async () => { const ctrl = new OverlayController({ ...withGlobalTestConfig(), hidesOnOutsideEsc: true, }); await ctrl.show(); - ctrl.contentNode.dispatchEvent(new KeyboardEvent('keyup', { key: 'Escape' })); + await mimicEscapePress(ctrl.contentNode); expect(ctrl.isShown).to.be.true; }); + + it('stays shown on [Escape] press in a nested overlay', async () => { + const parentContent = /** @type {HTMLDivElement} */ ( + await fixture( + html` +
+
we press [Escape] here
+
`, + ) + ); + const { parentOverlay, childOverlay } = await createNestedEscControllers(parentContent); + mimicEscapePress(childOverlay.contentNode); + + expect(parentOverlay.isShown).to.be.true; + expect(childOverlay.isShown).to.be.true; + }); }); + describe('Nested hidesOnEsc / hidesOnOutsideEsc', () => { + describe('Parent has hidesOnEsc and child has hidesOnOutsideEsc', () => { + it('on [Escape] press in child overlay: parent hides, child stays shown', async () => { + const parentContent = /** @type {HTMLDivElement} */ ( + await fixture( + html` +
+
we press [Escape] here
+
`, + ) + ); + const { parentOverlay, childOverlay } = await createNestedEscControllers(parentContent); + await mimicEscapePress(childOverlay.contentNode); + + expect(parentOverlay.isShown).to.be.false; + expect(childOverlay.isShown).to.be.true; + }); + + it('on [Escape] press outside overlays: parent stays shown, child hides', async () => { + const parentContent = /** @type {HTMLDivElement} */ ( + await fixture( + html` +
+
+
`, + ) + ); + const { parentOverlay, childOverlay } = await createNestedEscControllers(parentContent); + await mimicEscapePress(document); + + expect(parentOverlay.isShown).to.be.true; + expect(childOverlay.isShown).to.be.false; + }); + + it('on [Escape] press in parent overlay: parent is hidden, child is hidden', async () => { + const parentContent = /** @type {HTMLDivElement} */ ( + await fixture( + html` +
+ we press [Escape] here +
+
`, + ) + ); + const { parentOverlay, childOverlay } = await createNestedEscControllers(parentContent); + await mimicEscapePress(parentContent); + + expect(parentOverlay.isShown).to.be.false; + expect(childOverlay.isShown).to.be.false; + }); + }); + describe('Parent has hidesOnOutsideEsc and child has hidesOnEsc', () => { + it('on [Escape] press in child overlay: parent stays shown, child hides', async () => { + const parentContent = /** @type {HTMLDivElement} */ ( + await fixture( + html` +
+
we press [Escape] here
+
`, + ) + ); + const { parentOverlay, childOverlay } = await createNestedEscControllers(parentContent); + await mimicEscapePress(childOverlay.contentNode); + + expect(parentOverlay.isShown).to.be.true; + expect(childOverlay.isShown).to.be.false; + }); + + it('on [Escape] press outside overlays: parent hides, child stays shown', async () => { + const parentContent = /** @type {HTMLDivElement} */ ( + await fixture( + html` +
+
+
`, + ) + ); + const { parentOverlay, childOverlay } = await createNestedEscControllers(parentContent); + await mimicEscapePress(document); + + expect(parentOverlay.isShown).to.be.false; + expect(childOverlay.isShown).to.be.true; + }); + + it('on [Escape] press in parent overlay: parent hides, child stays shown', async () => { + const parentContent = /** @type {HTMLDivElement} */ ( + await fixture( + html` +
+
+
`, + ) + ); + const { parentOverlay, childOverlay } = await createNestedEscControllers(parentContent); + await mimicEscapePress(document); + + expect(parentOverlay.isShown).to.be.false; + expect(childOverlay.isShown).to.be.true; + }); + }); + + describe('With shadow dom', () => { + it('on [Escape] press in child overlay in shadow root: parent hides, child stays shown', async () => { + const parentContent = /** @type {HTMLDivElement} */ ( + await fixture( + html` +
+
we press [Escape] here
+
`, + ) + ); + const { parentOverlay, childOverlay } = await createNestedEscControllers(parentContent); + await mimicEscapePress(childOverlay.contentNode); + + expect(parentOverlay.isShown).to.be.false; + expect(childOverlay.isShown).to.be.true; + }); + }); + }); describe('hidesOnOutsideClick', () => { it('hides on outside click', async () => { const contentNode = /** @type {HTMLElement} */ (await fixture('
Content
')); @@ -684,9 +888,8 @@ describe('OverlayController', () => { expect(ctrl.isShown).to.be.true; // Don't hide on inside mousedown & outside mouseup - ctrl.contentNode.dispatchEvent(new MouseEvent('mousedown')); - await aTimeout(0); - document.body.dispatchEvent(new MouseEvent('mouseup')); + await mimicClick(ctrl.contentNode, { releaseElement: document.body, isAsync: true }); + await aTimeout(0); expect(ctrl.isShown).to.be.true; From c798ce523e88d8cecb30c6ad6ca53997669feb97 Mon Sep 17 00:00:00 2001 From: Thijs Louisse Date: Tue, 5 Nov 2024 16:54:34 +0100 Subject: [PATCH 2/2] fix: revert es version in tsconfig (as it was breaking types) --- .changeset/short-experts-mix.md | 5 +++++ tsconfig.json | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 .changeset/short-experts-mix.md diff --git a/.changeset/short-experts-mix.md b/.changeset/short-experts-mix.md new file mode 100644 index 0000000000..ea64275b8a --- /dev/null +++ b/.changeset/short-experts-mix.md @@ -0,0 +1,5 @@ +--- +'@lion/ui': patch +--- + +revert es version in tsconfig (as it was breaking types) diff --git a/tsconfig.json b/tsconfig.json index a42a71f423..3fa8febcfb 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -3,7 +3,7 @@ "target": "ESNext", "module": "ESNext", "moduleResolution": "NodeNext", - "lib": ["es2022", "dom"], + "lib": ["es2017", "dom"], "allowJs": true, "checkJs": true, "strict": true,