From 8377e8d17d54ce902a87e3a8db158554833d2fde Mon Sep 17 00:00:00 2001 From: Thijs Louisse Date: Mon, 6 Jan 2025 14:40:12 +0100 Subject: [PATCH] fix(overlays): rework setup and teardown logic of OverlayMixin Add a failing test chore: broken test fix chore: harden overlay teardown tests and cleanup select-rich --- .changeset/smooth-boats-argue.md | 2 +- .changeset/tricky-suns-change.md | 5 + .changeset/tricky-suns-changerz.md | 5 + .../combobox/test/lion-combobox.test.js | 2 + .../dialog/test-helpers/test-router.js | 43 ++++ .../dialog/test/lion-dialog.test.js | 54 +++++ .../overlays/src/OverlayController.js | 21 +- .../components/overlays/src/OverlayMixin.js | 34 +-- .../test-suites/OverlayMixin.suite.js | 193 +++++++++++++++++- .../select-rich/src/LionSelectRich.js | 4 +- .../select-rich/test/lion-select-rich.test.js | 44 ++++ 11 files changed, 371 insertions(+), 36 deletions(-) create mode 100644 .changeset/tricky-suns-change.md create mode 100644 .changeset/tricky-suns-changerz.md create mode 100644 packages/ui/components/dialog/test-helpers/test-router.js diff --git a/.changeset/smooth-boats-argue.md b/.changeset/smooth-boats-argue.md index 6a93afc4c..1b3bcadf9 100644 --- a/.changeset/smooth-boats-argue.md +++ b/.changeset/smooth-boats-argue.md @@ -2,4 +2,4 @@ '@lion/ui': patch --- -overlayController teardown cleanup (removes logs in test files) +[overlays]: overlayController teardown cleanup (removes logs in test files) diff --git a/.changeset/tricky-suns-change.md b/.changeset/tricky-suns-change.md new file mode 100644 index 000000000..4775be4ad --- /dev/null +++ b/.changeset/tricky-suns-change.md @@ -0,0 +1,5 @@ +--- +'@lion/ui': patch +--- + +[overlays]: allow Subclassers to teardown at the right moment diff --git a/.changeset/tricky-suns-changerz.md b/.changeset/tricky-suns-changerz.md new file mode 100644 index 000000000..0cb9ea690 --- /dev/null +++ b/.changeset/tricky-suns-changerz.md @@ -0,0 +1,5 @@ +--- +'@lion/ui': minor +--- + +[overlays]: rework setup and teardown logic of OverlayMixin (allow to reconnect offline dom nodes; allow moving dom nodes in performant way) diff --git a/packages/ui/components/combobox/test/lion-combobox.test.js b/packages/ui/components/combobox/test/lion-combobox.test.js index 410d5e929..34682ae55 100644 --- a/packages/ui/components/combobox/test/lion-combobox.test.js +++ b/packages/ui/components/combobox/test/lion-combobox.test.js @@ -229,6 +229,8 @@ describe('lion-combobox', () => { `) ); + await el.updateComplete; + const { _inputNode } = getComboboxMembers(el); _inputNode.dispatchEvent(new KeyboardEvent('keyup', { key: 'Escape' })); diff --git a/packages/ui/components/dialog/test-helpers/test-router.js b/packages/ui/components/dialog/test-helpers/test-router.js new file mode 100644 index 000000000..9b102b48c --- /dev/null +++ b/packages/ui/components/dialog/test-helpers/test-router.js @@ -0,0 +1,43 @@ +import { html, LitElement } from 'lit'; + +class TestRouter extends LitElement { + static properties = { + routingMap: { + type: Object, + attribute: false, + }, + path: { type: String }, + }; + + constructor() { + super(); + + /** @type {{ [path: string]: HTMLElement }} */ + this.routingMap = {}; + /** @type {string} */ + this.path = ''; + } + + render() { + return html` +
+
+ ${Object.keys(this.routingMap).map( + path => + html``, + )} +
+
${this.routingMap[this.path]}
+
+ `; + } +} + +customElements.define('test-router', TestRouter); diff --git a/packages/ui/components/dialog/test/lion-dialog.test.js b/packages/ui/components/dialog/test/lion-dialog.test.js index 3838b6703..35074be09 100644 --- a/packages/ui/components/dialog/test/lion-dialog.test.js +++ b/packages/ui/components/dialog/test/lion-dialog.test.js @@ -2,6 +2,7 @@ import { expect, fixture as _fixture, html, unsafeStatic, aTimeout } from '@open-wc/testing'; import { runOverlayMixinSuite } from '../../overlays/test-suites/OverlayMixin.suite.js'; import { isActiveElement } from '../../core/test-helpers/isActiveElement.js'; +import '../test-helpers/test-router.js'; import '@lion/ui/define/lion-dialog.js'; /** @@ -187,4 +188,57 @@ describe('lion-dialog', () => { expect(invokerButton.getAttribute('aria-expanded')).to.equal(null); }); }); + + describe('Edge cases', () => { + it('does not lose click event handler when was detached and reattached', async () => { + const el = await fixture(html` + Hey there + + + `, + b: html`
B
`, + }}" + path="a" + > + `); + + const getDialog = () => + /** @type {LionDialog} */ (el.shadowRoot?.querySelector('lion-dialog')); + const getInvoker = () => + /** @type {HTMLElement} */ (getDialog().querySelector('[slot="invoker"]')); + + getInvoker().click(); + const dialog = getDialog(); + // @ts-expect-error [allow-protected-in-tests] + await dialog._overlayCtrl._showComplete; + expect(dialog.opened).to.be.true; + + dialog.close(); + // @ts-expect-error [allow-protected-in-tests] + await dialog._overlayCtrl._hideComplete; + expect(dialog.opened).to.be.false; + + const buttonA = /** @type {HTMLElement} */ (el.shadowRoot?.querySelector('#path-a')); + const buttonB = /** @type {HTMLElement} */ (el.shadowRoot?.querySelector('#path-b')); + + buttonB.click(); + await el.updateComplete; + buttonA.click(); + await el.updateComplete; + await el.updateComplete; + + getInvoker().click(); + + const dialogAfterRouteChange = getDialog(); + // @ts-expect-error [allow-protected-in-tests] + expect(dialogAfterRouteChange._overlayCtrl).not.to.be.undefined; + // @ts-expect-error [allow-protected-in-tests] + await dialogAfterRouteChange._overlayCtrl._showComplete; + expect(dialogAfterRouteChange.opened).to.be.true; + }); + }); }); diff --git a/packages/ui/components/overlays/src/OverlayController.js b/packages/ui/components/overlays/src/OverlayController.js index 9ff3e8295..17f15a20a 100644 --- a/packages/ui/components/overlays/src/OverlayController.js +++ b/packages/ui/components/overlays/src/OverlayController.js @@ -1003,16 +1003,16 @@ export class OverlayController extends EventTarget { * @param {{ phase: OverlayPhase }} config */ _handleVisibilityTriggers({ phase }) { - if (typeof this.visibilityTriggerFunction !== 'function') return; - - if (phase === 'init') { - this.__visibilityTriggerHandler = this.visibilityTriggerFunction({ - phase, - controller: this, - }); - } - if (this.__visibilityTriggerHandler[phase]) { - this.__visibilityTriggerHandler[phase](); + if (typeof this.visibilityTriggerFunction === 'function') { + if (phase === 'init') { + this.__visibilityTriggerHandler = this.visibilityTriggerFunction({ + phase, + controller: this, + }); + } + if (this.__visibilityTriggerHandler[phase]) { + this.__visibilityTriggerHandler[phase](); + } } } @@ -1192,7 +1192,6 @@ export class OverlayController extends EventTarget { event.composedPath().includes(this.contentNode) || deepContains(this.contentNode, /** @type {HTMLElement|ShadowRoot} */ (event.target)); if (hasPressedInside) return; - this.hide(); }; diff --git a/packages/ui/components/overlays/src/OverlayMixin.js b/packages/ui/components/overlays/src/OverlayMixin.js index 39466f83b..6b2bce481 100644 --- a/packages/ui/components/overlays/src/OverlayMixin.js +++ b/packages/ui/components/overlays/src/OverlayMixin.js @@ -24,6 +24,8 @@ export const OverlayMixinImplementation = superclass => }; } + #hasSetup = false; + constructor() { super(); /** @@ -173,22 +175,23 @@ export const OverlayMixinImplementation = superclass => } } - /** - * @param {import('lit-element').PropertyValues } changedProperties - */ - firstUpdated(changedProperties) { - super.firstUpdated(changedProperties); + connectedCallback() { + super.connectedCallback(); - this._setupOverlayCtrl(); + this.updateComplete.then(() => { + if (this.#hasSetup) return; + this._setupOverlayCtrl(); + this.#hasSetup = true; + }); } async disconnectedCallback() { super.disconnectedCallback(); - if (!this._overlayCtrl) return; - if (await this.#isMovingInDom()) return; - - this._teardownOverlayCtrl(); + if (await this._isPermanentlyDisconnected()) { + this._teardownOverlayCtrl(); + this.#hasSetup = false; + } } /** @@ -238,6 +241,8 @@ export const OverlayMixinImplementation = superclass => /** @protected */ _setupOverlayCtrl() { + if (this._overlayCtrl) return; + /** @type {OverlayController} */ this._overlayCtrl = this._defineOverlay({ contentNode: this._overlayContentNode, @@ -253,11 +258,12 @@ export const OverlayMixinImplementation = superclass => /** @protected */ _teardownOverlayCtrl() { + if (!this._overlayCtrl) return; + this._teardownOpenCloseListeners(); this.__teardownSyncFromOverlayController(); - /** @type {OverlayController} */ - (this._overlayCtrl).teardown(); + /** @type {OverlayController} */ (this._overlayCtrl).teardown(); } /** @@ -396,9 +402,9 @@ export const OverlayMixinImplementation = superclass => * Before we decide to teardown, let's wait to see if we were not just moving nodes around. * @return {Promise} */ - async #isMovingInDom() { + async _isPermanentlyDisconnected() { await this.updateComplete; - return this.isConnected; + return !this.isConnected; } }; export const OverlayMixin = dedupeMixin(OverlayMixinImplementation); diff --git a/packages/ui/components/overlays/test-suites/OverlayMixin.suite.js b/packages/ui/components/overlays/test-suites/OverlayMixin.suite.js index 912078952..00a71a252 100644 --- a/packages/ui/components/overlays/test-suites/OverlayMixin.suite.js +++ b/packages/ui/components/overlays/test-suites/OverlayMixin.suite.js @@ -1,18 +1,25 @@ -import { expect, fixture, html, nextFrame, aTimeout } from '@open-wc/testing'; - -import sinon from 'sinon'; +import { + unsafeStatic, + nextFrame, + aTimeout, + defineCE, + fixture, + expect, + html, +} from '@open-wc/testing'; import { overlays as overlaysManager, OverlayController } from '@lion/ui/overlays.js'; - -import '@lion/ui/define/lion-dialog.js'; import { browserDetection } from '@lion/ui/core.js'; +import { cache } from 'lit/directives/cache.js'; +import '@lion/ui/define/lion-dialog.js'; +import { LitElement } from 'lit'; +import sinon from 'sinon'; /** - * @typedef {import('../types/OverlayConfig.js').OverlayConfig} OverlayConfig * @typedef {import('../types/OverlayMixinTypes.js').DefineOverlayConfig} DefineOverlayConfig - * @typedef {import('../types/OverlayMixinTypes.js').OverlayHost} OverlayHost - * @typedef {import('../types/OverlayMixinTypes.js').OverlayMixin} OverlayMixin - * @typedef {import('lit').LitElement} LitElement * @typedef {LitElement & OverlayHost & {_overlayCtrl:OverlayController}} OverlayEl + * @typedef {import('../types/OverlayMixinTypes.js').OverlayMixin} OverlayMixin + * @typedef {import('../types/OverlayMixinTypes.js').OverlayHost} OverlayHost + * @typedef {import('../types/OverlayConfig.js').OverlayConfig} OverlayConfig */ function getGlobalOverlayCtrls() { @@ -331,6 +338,173 @@ export function runOverlayMixinSuite({ tagString, tag, suffix = '' }) { }).to.not.throw; stub.restore(); }); + + describe('Teardown', () => { + it('does not teardown when moving dom nodes', async () => { + const el = /** @type {OverlayEl} */ ( + await fixture(html` + <${tag}> +
content
+ + + `) + ); + // @ts-expect-error [allow-protected] in tests + const teardownSpy = sinon.spy(el, '_teardownOverlayCtrl'); + // @ts-expect-error [allow-protected] in tests + const spyDisconnected = sinon.spy(el, '_isPermanentlyDisconnected'); + + // move around in dom + const newParent = document.createElement('div'); + document.body.appendChild(newParent); + newParent.appendChild(el); + expect(spyDisconnected.callCount).to.equal(1); + + const otherParent = document.createElement('div'); + document.body.appendChild(otherParent); + otherParent.appendChild(el); + await aTimeout(0); + + expect(spyDisconnected.callCount).to.equal(2); + expect(teardownSpy.callCount).to.equal(0); + + // simulate a permanent disconnect + otherParent.removeChild(el); + await aTimeout(0); + + expect(teardownSpy.callCount).to.equal(1); + }); + + it('allows to disconnect and reconnect later', async () => { + const el = /** @type {OverlayEl} */ ( + await fixture(html` + <${tag}> +
content
+ + + `) + ); + // @ts-expect-error [allow-protected] in tests + const teardownSpy = sinon.spy(el, '_teardownOverlayCtrl'); + // @ts-expect-error [allow-protected] in tests + const spyDisconnected = sinon.spy(el, '_isPermanentlyDisconnected'); + // @ts-expect-error [allow-protected] in tests + const setupSpy = sinon.spy(el, '_setupOverlayCtrl'); + + // 'permanently' disconnect (although we will reconnect later) + const offlineParent = document.createElement('div'); + offlineParent.appendChild(el); + await aTimeout(0); + + expect(spyDisconnected.callCount).to.equal(1); + expect(teardownSpy.callCount).to.equal(1); + + // reconnect + document.body.appendChild(offlineParent); + await aTimeout(0); + + expect(setupSpy.callCount).to.equal(1); + }); + + it('works with cache directive when reconnected', async () => { + class CachingContext extends LitElement { + static properties = { + switched: { type: Boolean }, + }; + + constructor() { + super(); + + this.switched = false; + } + + get overlayEl() { + return this.shadowRoot?.querySelector('#myOverlay'); + } + + render() { + return html`${cache( + this.switched + ? html`something else` + : html`<${tag} id="myOverlay"> +
content b
+ + `, + )}`; + } + } + + const cachingTagName = defineCE(CachingContext); + const cachingTag = unsafeStatic(cachingTagName); + + const el = /** @type {CachingContext} */ ( + await fixture(html` + <${cachingTag}> + `) + ); + + // @ts-expect-error [allow-protected] in tests + const teardownSpy = sinon.spy(el.overlayEl, '_teardownOverlayCtrl'); + // @ts-expect-error [allow-protected] in tests + const spyDisconnected = sinon.spy(el.overlayEl, '_isPermanentlyDisconnected'); + // @ts-expect-error [allow-protected] in tests + const setupSpy = sinon.spy(el.overlayEl, '_setupOverlayCtrl'); + + el.switched = true; + // render the new content + await el.updateComplete; + // wait for the teardown to complete + await el.updateComplete; + + expect(spyDisconnected.callCount).to.equal(1); + expect(teardownSpy.callCount).to.equal(1); + expect(setupSpy.callCount).to.equal(0); + + el.switched = false; + await el.updateComplete; + + expect(setupSpy.callCount).to.equal(1); + }); + + it('correctly removes event listeners when disconnected from dom', async () => { + const el = /** @type {OverlayEl} */ ( + await fixture(html` + <${tag}> +
content
+ + + `) + ); + + const eventRemoveSpy = sinon.spy(el._overlayCtrl, 'removeEventListener'); + el.parentNode?.removeChild(el); + await el.updateComplete; + + expect(eventRemoveSpy.callCount).to.equal(0); + + await el.updateComplete; + const hasOneInstanceFor = ( + /** @type {string} */ eventName, + /** @type {any[][]} */ eventsToSearch, + /** @type {string} */ methodToRemove, + ) => + Boolean( + eventsToSearch.filter( + ([eventToSearch, methodToSearch]) => + eventToSearch === eventName && methodToSearch === methodToRemove, + ).length, + ); + + // @ts-expect-error [allow-private] in tests + expect(hasOneInstanceFor('show', eventRemoveSpy.args, el.__onOverlayCtrlShow)).to.be.true; + // @ts-expect-error [allow-private] in tests + expect(hasOneInstanceFor('hide', eventRemoveSpy.args, el.__onOverlayCtrlHide)).to.be.true; + // @ts-expect-error [allow-private] in tests + expect(hasOneInstanceFor('before-show', eventRemoveSpy.args, el.__onBeforeShow)).to.be.true; + // @ts-expect-error [allow-private] in tests + expect(hasOneInstanceFor('before-hide', eventRemoveSpy.args, el.__onBeforeHide)).to.be.true; + }); + }); }); describe(`OverlayMixin${suffix} nested`, () => { @@ -388,6 +562,7 @@ export function runOverlayMixinSuite({ tagString, tag, suffix = '' }) { const moveTarget = /** @type {OverlayEl} */ (await fixture('
')); moveTarget.appendChild(el); await el.updateComplete; + expect(getGlobalOverlayCtrls().length).to.equal(1); } }); diff --git a/packages/ui/components/select-rich/src/LionSelectRich.js b/packages/ui/components/select-rich/src/LionSelectRich.js index a8dc0df8b..06d798d5c 100644 --- a/packages/ui/components/select-rich/src/LionSelectRich.js +++ b/packages/ui/components/select-rich/src/LionSelectRich.js @@ -423,6 +423,7 @@ export class LionSelectRich extends SlotMixin(ScopedElementsMixin(OverlayMixin(L */ _teardownOverlayCtrl() { super._teardownOverlayCtrl(); + this._overlayCtrl.removeEventListener('show', this.__overlayOnShow); this._overlayCtrl.removeEventListener('before-show', this.__overlayBeforeShow); this._overlayCtrl.removeEventListener('hide', this.__overlayOnHide); @@ -434,11 +435,12 @@ export class LionSelectRich extends SlotMixin(ScopedElementsMixin(OverlayMixin(L * @protected */ async _alignInvokerWidth() { + await this.updateComplete; + if (!this._overlayCtrl?.content) { return; } - await this.updateComplete; const initContentDisplay = this._overlayCtrl.content.style.display; const initContentMinWidth = this._overlayCtrl.contentWrapperNode.style.minWidth; const initContentWidth = this._overlayCtrl.contentWrapperNode.style.width; diff --git a/packages/ui/components/select-rich/test/lion-select-rich.test.js b/packages/ui/components/select-rich/test/lion-select-rich.test.js index e131bc2fb..7bc4cc232 100644 --- a/packages/ui/components/select-rich/test/lion-select-rich.test.js +++ b/packages/ui/components/select-rich/test/lion-select-rich.test.js @@ -7,6 +7,7 @@ import '@lion/ui/define/lion-select-rich.js'; import '@lion/ui/define/lion-listbox.js'; import '@lion/ui/define/lion-option.js'; import { LitElement } from 'lit'; +import sinon from 'sinon'; import { fixture as _fixture, unsafeStatic, @@ -711,6 +712,49 @@ describe('lion-select-rich', () => { }); }); + describe('Teardown', () => { + it('correctly removes event listeners when disconnected from dom', async () => { + const el = await fixture(html` + + Item 1 + Item 2 + + `); + + const { _overlayCtrl } = getSelectRichMembers(el); + const eventRemoveSpy = sinon.spy(_overlayCtrl, 'removeEventListener'); + + el.parentNode?.removeChild(el); + + expect(eventRemoveSpy.callCount).to.equal(0); + + await el.updateComplete; + expect(eventRemoveSpy.callCount).to.equal(0); + + await el.updateComplete; + + const hasOneInstanceFor = ( + /** @type {string} */ eventName, + /** @type {any[][]} */ eventsToSearch, + /** @type {string} */ methodToRemove, + ) => + Boolean( + eventsToSearch.filter( + ([eventToSearch, methodToSearch]) => + eventToSearch === eventName && methodToSearch === methodToRemove, + ).length, + ); + + // @ts-expect-error [allow-private] in tests + expect(hasOneInstanceFor('show', eventRemoveSpy.args, el.__overlayOnShow)).to.be.true; + // @ts-expect-error [allow-private] in tests + expect(hasOneInstanceFor('before-show', eventRemoveSpy.args, el.__overlayBeforeShow)).to.be + .true; + // @ts-expect-error [allow-private] in tests + expect(hasOneInstanceFor('hide', eventRemoveSpy.args, el.__overlayOnHide)).to.be.true; + }); + }); + describe('Subclassers', () => { it('allows to override the type of overlay', async () => { const mySelectTagString = defineCE(