fix: allow the popup dialog to close when it is setup by lit cache (#2563)

Author: okadurin

.changeset/six-ghosts-crash.md

@@ -0,0 +1,5 @@
+---
+'@lion/ui': patch
+---
+
+[overlays] allow the popup dialog to close when it is setup by `lit` `cache`

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

@@ -1,9 +1,20 @@
 /* eslint-disable lit-a11y/no-autofocus */
-import { expect, fixture as _fixture, html, unsafeStatic, aTimeout } from '@open-wc/testing';
+import {
+  expect,
+  fixture as _fixture,
+  html,
+  unsafeStatic,
+  aTimeout,
+  defineCE,
+  waitUntil,
+} from '@open-wc/testing';
+import { cache } from 'lit/directives/cache.js';
+import { LitElement, nothing } from 'lit';
 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';
+import '@lion/ui/define/lion-tabs.js';
 
 /**
  * @typedef {import('../src/LionDialog.js').LionDialog} LionDialog
@@ -296,5 +307,107 @@ describe('lion-dialog', () => {
       await dialogAfterRouteChange._overlayCtrl._showComplete;
       expect(dialogAfterRouteChange.opened).to.be.true;
     });
+
+    it('should close the popup dialog after rendered from cache', async () => {
+      /**
+       *
+       * @param {Event} e
+       * @returns
+       */
+      const closeButtonHandler = e =>
+        e.target?.dispatchEvent(new Event('close-overlay', { bubbles: true }));
+      const dialog = html` <lion-dialog>
+        <button slot="invoker" class="invoker-button">Click me to open dialog</button>
+        <div slot="content" class="demo-dialog-content">
+          Hello! You can close this dialog here:
+          <button class="close-button" @click="${closeButtonHandler}">⨯</button>
+        </div>
+      </lion-dialog>`;
+
+      /**
+       * Note, inactive tab content is **destroyed** on every tab switch.
+       */
+      class Wrapper extends LitElement {
+        static properties = {
+          ...super.properties,
+          activeTabIndex: { type: Number },
+        };
+
+        constructor() {
+          super();
+          this.activeTabIndex = 0;
+        }
+
+        /**
+         * @param {number} index
+         */
+        changeActiveTabIndex(index) {
+          this.activeTabIndex = index;
+        }
+
+        render() {
+          const changeActiveTabIndexRef = this.changeActiveTabIndex.bind(this);
+          return html`
+            <lion-tabs>
+              <button slot="tab" class="first-button" @click=${() => changeActiveTabIndexRef(0)}>
+                First
+              </button>
+              <p slot="panel">${cache(this.activeTabIndex === 0 ? dialog : nothing)}</p>
+              <button slot="tab" class="second-button" @click=${() => changeActiveTabIndexRef(1)}>
+                Second
+              </button>
+              <p slot="panel">Info page with lots of information about us.</p>
+            </lion-tabs>
+          `;
+        }
+      }
+
+      const wrapperFixture = /** @type {(arg: TemplateResult) => Promise<Wrapper>} */ (_fixture);
+      const tagString = defineCE(Wrapper);
+      const wrapperTag = unsafeStatic(tagString);
+      const wrapperElement = /** @type {Wrapper} */ (
+        await wrapperFixture(html`<${wrapperTag}></${wrapperTag}>`)
+      );
+      await wrapperElement.updateComplete;
+      const wrapperElementShadowRoot = wrapperElement.shadowRoot;
+      /**
+       * @returns { HTMLElement | null | undefined }
+       */
+      const getFirstButton = () => wrapperElementShadowRoot?.querySelector('.first-button');
+      /**
+       * @returns { HTMLElement | null | undefined }
+       */
+      const getSecondButton = () => wrapperElementShadowRoot?.querySelector('.second-button');
+      /**
+       * @returns { HTMLElement | null | undefined }
+       */
+      const getInvokerButton = () => wrapperElementShadowRoot?.querySelector('.invoker-button');
+      /**
+       * @returns { HTMLElement | null | undefined }
+       */
+      const getCloseButton = () => wrapperElementShadowRoot?.querySelector('.close-button');
+      /**
+       * @returns { Element | null | undefined }
+       */
+      const getDialog = () =>
+        wrapperElementShadowRoot?.querySelector('lion-dialog')?.shadowRoot?.querySelector('dialog');
+      // @ts-ignore
+      const isDialogVisible = () => getDialog()?.checkVisibility() === true;
+      const isDialogRendered = () =>
+        !!wrapperElement.shadowRoot?.querySelector('lion-dialog')?.shadowRoot?.childNodes.length;
+      getInvokerButton()?.click();
+      await waitUntil(isDialogVisible);
+      getCloseButton()?.click();
+      await waitUntil(() => !isDialogVisible());
+      getSecondButton()?.click();
+      await waitUntil(() => !isDialogRendered());
+      getFirstButton()?.click();
+      await waitUntil(isDialogRendered);
+      getInvokerButton()?.click();
+      await waitUntil(isDialogVisible);
+      getCloseButton()?.click();
+      await waitUntil(() => !isDialogVisible());
+      expect(isDialogVisible()).to.equal(false);
+    });
   });
 });

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

@@ -246,16 +246,23 @@ export const OverlayMixinImplementation = superclass =>
 
     /** @protected */
     _setupOverlayCtrl() {
-      if (this._overlayCtrl) return;
-
-      /** @type {OverlayController} */
-      this._overlayCtrl = this._defineOverlay({
+      if (this.#hasSetup) return;
+      const config = {
         contentNode: this._overlayContentNode,
         contentWrapperNode: this._overlayContentWrapperNode,
         invokerNode: this._overlayInvokerNode,
         referenceNode: this._overlayReferenceNode,
         backdropNode: this._overlayBackdropNode,
-      });
+      };
+
+      if (this._overlayCtrl) {
+        // when `lit` `cache` attaches node to the DOM, register the controller back in the OverlaysManager
+        this._overlayCtrl.updateConfig(config);
+      } else {
+        /** @type {OverlayController} */
+        this._overlayCtrl = this._defineOverlay(config);
+      }
+
       this.__syncToOverlayController();
       this.__setupSyncFromOverlayController();
       this._setupOpenCloseListeners();

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

@@ -533,8 +533,11 @@ export function runOverlayMixinSuite({ tagString, tag, suffix = '' }) {
 
         el.switched = false;
         await el.updateComplete;
-
         expect(setupSpy.callCount).to.equal(1);
+
+        const elCtrl = /** @type {OverlayEl} */ (el.overlayEl)?._overlayCtrl;
+        const isCtrlRegisteredAtOverlaysManager = elCtrl.manager.list.some(ctrl => elCtrl === ctrl);
+        expect(isCtrlRegisteredAtOverlaysManager).to.equal(true);
       });
 
       it('correctly removes event listeners when disconnected from dom', async () => {

packages/ui/components/overlays/test/OverlayController.test.js

@@ -153,6 +153,8 @@ async function createNestedEscControllers(parentContent) {
 
 afterEach(() => {
   overlays.teardown();
+  // clean document.body from the DOM nodes left by previous tests
+  document.body.innerHTML = '';
 });
 
 describe('OverlayController', () => {