fix(overlays): rework setup and teardown logic of OverlayMixin

.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)

.changeset/tricky-suns-change.md

@@ -0,0 +1,5 @@
+---
+'@lion/ui': patch
+---
+
+[overlays]: allow Subclassers to teardown at the right moment

.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)

packages/ui/components/combobox/test/lion-combobox.test.js

@@ -229,6 +229,8 @@ describe('lion-combobox', () => {
             </lion-combobox>
           `)
         );
+        await el.updateComplete;
+
         const { _inputNode } = getComboboxMembers(el);
 
         _inputNode.dispatchEvent(new KeyboardEvent('keyup', { key: 'Escape' }));

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();
   };
 

packages/ui/components/overlays/src/OverlayMixin.js

@@ -173,22 +173,22 @@ export const OverlayMixinImplementation = superclass =>
       }
     }
 
-    /**
-     * @param {import('lit-element').PropertyValues } changedProperties
-     */
-    firstUpdated(changedProperties) {
-      super.firstUpdated(changedProperties);
+    connectedCallback() {
+      super.connectedCallback();
 
-      this._setupOverlayCtrl();
+      this.updateComplete.then(() => {
+        this._setupOverlayCtrl();
+      });
     }
 
     async disconnectedCallback() {
       super.disconnectedCallback();
 
       if (!this._overlayCtrl) return;
-      if (await this.#isMovingInDom()) return;
 
-      this._teardownOverlayCtrl();
+      if (await this._isPermanentlyDisconnected()) {
+        this._teardownOverlayCtrl();
+      }
     }
 
     /**
@@ -238,6 +238,8 @@ export const OverlayMixinImplementation = superclass =>
 
     /** @protected */
     _setupOverlayCtrl() {
+      if (this._overlayCtrl) return;
+
       /** @type {OverlayController} */
       this._overlayCtrl = this._defineOverlay({
         contentNode: this._overlayContentNode,
@@ -253,11 +255,13 @@ export const OverlayMixinImplementation = superclass =>
 
     /** @protected */
     _teardownOverlayCtrl() {
+      if (!this._overlayCtrl) return;
+
       this._teardownOpenCloseListeners();
       this.__teardownSyncFromOverlayController();
 
-      /** @type {OverlayController} */
-      (this._overlayCtrl).teardown();
+      /** @type {OverlayController} */ (this._overlayCtrl).teardown();
+      this._overlayCtrl = undefined;
     }
 
     /**
@@ -396,9 +400,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<boolean>}
      */
-    async #isMovingInDom() {
+    async _isPermanentlyDisconnected() {
       await this.updateComplete;
-      return this.isConnected;
+      return !this.isConnected;
     }
   };
 export const OverlayMixin = dedupeMixin(OverlayMixinImplementation);

packages/ui/components/overlays/test-suites/OverlayMixin.suite.js

@@ -331,6 +331,67 @@ export function runOverlayMixinSuite({ tagString, tag, suffix = '' }) {
       }).to.not.throw;
       stub.restore();
     });
+
+    it('does not teardown when moving dom nodes', async () => {
+      const el = /** @type {OverlayEl} */ (
+        await fixture(html`
+        <${tag}>
+          <div slot="content">content</div>
+          <button slot="invoker">invoker button</button>
+        </${tag}>
+      `)
+      );
+      // @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);
+      await aTimeout(0);
+
+      expect(spyDisconnected.callCount).to.equal(1);
+      expect(teardownSpy.callCount).to.equal(0);
+
+      // simulate a permanent disconnect
+      newParent.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}>
+          <div slot="content">content</div>
+          <button slot="invoker">invoker button</button>
+        </${tag}>
+      `)
+      );
+      // @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');
+
+      // move around in dom
+      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);
+    });
   });
 
   describe(`OverlayMixin${suffix} nested`, () => {
@@ -388,6 +449,7 @@ export function runOverlayMixinSuite({ tagString, tag, suffix = '' }) {
         const moveTarget = /** @type {OverlayEl} */ (await fixture('<div id="target"></div>'));
         moveTarget.appendChild(el);
         await el.updateComplete;
+
         expect(getGlobalOverlayCtrls().length).to.equal(1);
       }
     });

packages/ui/components/select-rich/src/LionSelectRich.js

@@ -423,6 +423,8 @@ export class LionSelectRich extends SlotMixin(ScopedElementsMixin(OverlayMixin(L
    */
   _teardownOverlayCtrl() {
     super._teardownOverlayCtrl();
+    if (!this._overlayCtrl) return;
+
     this._overlayCtrl.removeEventListener('show', this.__overlayOnShow);
     this._overlayCtrl.removeEventListener('before-show', this.__overlayBeforeShow);
     this._overlayCtrl.removeEventListener('hide', this.__overlayOnHide);
@@ -434,11 +436,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;