refactor!: use slotted div element as a dialog renderer root (#10367)

web-padawan · web-flow · commit b9f16748358d · 2025-10-24T12:05:04.000+03:00
diff --git a/packages/context-menu/src/vaadin-menu-overlay-mixin.js b/packages/context-menu/src/vaadin-menu-overlay-mixin.js
@@ -37,11 +37,20 @@ export const MenuOverlayMixin = (superClass) =>
     }
 
     /**
-     * Override method from OverlayFocusMixin to use slotted div as the content root.
+     * Override method from OverlayFocusMixin to use slotted div as content root.
      * @protected
      * @override
      */
     get _contentRoot() {
+      return this._rendererRoot;
+    }
+
+    /**
+     * Override method from OverlayMixin to use slotted div as the renderer root.
+     * @protected
+     * @override
+     */
+    get _rendererRoot() {
       if (!this.__savedRoot) {
         const root = document.createElement('div');
         root.setAttribute('slot', 'overlay');
diff --git a/packages/dialog/src/vaadin-dialog-overlay-mixin.js b/packages/dialog/src/vaadin-dialog-overlay-mixin.js
@@ -53,6 +53,22 @@ export const DialogOverlayMixin = (superClass) =>
       return this.owner;
     }
 
+    /**
+     * Override method from OverlayMixin to use slotted div as a renderer root.
+     * @protected
+     * @override
+     */
+    get _rendererRoot() {
+      if (!this.__savedRoot) {
+        const root = document.createElement('div');
+        root.style.display = 'contents';
+        this.owner.appendChild(root);
+        this.__savedRoot = root;
+      }
+
+      return this.__savedRoot;
+    }
+
     /** @protected */
     ready() {
       super.ready();
@@ -98,8 +114,9 @@ export const DialogOverlayMixin = (superClass) =>
         // Reset existing container in case if a new renderer is set.
         this.__clearContainer(container);
       } else {
-        // Create the container, but wait to append it until requestContentUpdate is called.
+        // Create the container and append it to the dialog element.
         container = this.__createContainer(slot);
+        this.owner.appendChild(container);
       }
       return container;
     }
@@ -181,28 +198,12 @@ export const DialogOverlayMixin = (superClass) =>
     requestContentUpdate() {
       super.requestContentUpdate();
 
-      if (this.headerContainer) {
-        // If a new renderer has been set, make sure to reattach the container
-        if (!this.headerContainer.parentElement) {
-          this.owner.appendChild(this.headerContainer);
-        }
-
-        if (this.headerRenderer) {
-          // Only call header renderer after the container has been initialized
-          this.headerRenderer.call(this.owner, this.headerContainer, this.owner);
-        }
+      if (this.headerContainer && this.headerRenderer) {
+        this.headerRenderer.call(this.owner, this.headerContainer, this.owner);
       }
 
-      if (this.footerContainer) {
-        // If a new renderer has been set, make sure to reattach the container
-        if (!this.footerContainer.parentElement) {
-          this.owner.appendChild(this.footerContainer);
-        }
-
-        if (this.footerRenderer) {
-          // Only call header renderer after the container has been initialized
-          this.footerRenderer.call(this.owner, this.footerContainer, this.owner);
-        }
+      if (this.footerContainer && this.footerRenderer) {
+        this.footerRenderer.call(this.owner, this.footerContainer, this.owner);
       }
 
       this._headerTitleRenderer();
diff --git a/packages/dialog/test/dom/__snapshots__/dialog.test.snap.js b/packages/dialog/test/dom/__snapshots__/dialog.test.snap.js
@@ -9,7 +9,9 @@ snapshots["vaadin-dialog host"] =
   tabindex="0"
   with-backdrop=""
 >
-  content
+  <div>
+    content
+  </div>
 </vaadin-dialog>
 `;
 /* end snapshot vaadin-dialog host */
diff --git a/packages/dialog/test/draggable-resizable.test.js b/packages/dialog/test/draggable-resizable.test.js
@@ -235,20 +235,21 @@ describe('resizable', () => {
   });
 
   it('should support scrollable full size content', () => {
-    const container = dialog.firstElementChild;
-    container.style.height = '100%';
-    container.style.width = '100%';
-    container.style.overflow = 'auto';
-    container.textContent = new Array(10000).fill('foo').join(' ');
+    const contentElement = dialog.$.overlay._rendererRoot.firstElementChild;
+    contentElement.style.height = '100%';
+    contentElement.style.width = '100%';
+    contentElement.style.overflow = 'auto';
+    contentElement.textContent = new Array(10000).fill('foo').join(' ');
 
     const resizeContainer = dialog.$.overlay.$.resizerContainer;
-    expect(container.offsetHeight).to.equal(resizeContainer.offsetHeight);
+    expect(contentElement.offsetHeight).to.equal(resizeContainer.offsetHeight);
   });
 
   it('should scroll if the content overflows', async () => {
     // Fill the content with a lot of text so that it overflows the viewport
-    dialog.firstElementChild.textContent = new Array(10000).fill('foo').join(' ');
-    await nextResize(dialog.firstElementChild);
+    const contentElement = dialog.$.overlay._rendererRoot.firstElementChild;
+    contentElement.textContent = new Array(10000).fill('foo').join(' ');
+    await nextResize(contentElement);
     await nextRender();
 
     const content = dialog.$.overlay.$.content;
@@ -260,9 +261,10 @@ describe('resizable', () => {
     resize(overlayPart.querySelector('.s'), 0, 10);
 
     // Set the dialog content to have 100% height
-    dialog.firstElementChild.style.height = '100%';
+    const contentElement = dialog.$.overlay._rendererRoot.firstElementChild;
+    contentElement.style.height = '100%';
 
-    const contentBounds = dialog.firstElementChild.getBoundingClientRect();
+    const contentBounds = contentElement.getBoundingClientRect();
     const overlayBounds = overlayPart.getBoundingClientRect();
     expect(Math.floor(contentBounds.height)).to.equal(Math.floor(overlayBounds.height));
   });
@@ -579,7 +581,7 @@ describe('draggable', () => {
     dialog.height = '100px';
     await nextRender();
 
-    const contentElement = dialog.firstElementChild;
+    const contentElement = dialog.$.overlay._rendererRoot.firstElementChild;
     contentElement.style.minHeight = '200px';
     await nextResize(contentElement);
     await nextRender();
diff --git a/packages/dialog/test/renderer.test.js b/packages/dialog/test/renderer.test.js
@@ -4,7 +4,7 @@ import sinon from 'sinon';
 import '../src/vaadin-dialog.js';
 
 describe('vaadin-dialog renderer', () => {
-  let dialog;
+  let dialog, rendererRoot;
 
   beforeEach(async () => {
     dialog = fixtureSync('<vaadin-dialog></vaadin-dialog>');
@@ -20,7 +20,8 @@ describe('vaadin-dialog renderer', () => {
     dialog.opened = true;
     await nextRender();
 
-    expect(dialog.textContent).to.include('The content of the dialog');
+    rendererRoot = dialog.firstElementChild;
+    expect(rendererRoot.textContent).to.include('The content of the dialog');
   });
 
   it('should run renderers when requesting content update', async () => {
@@ -48,11 +49,13 @@ describe('vaadin-dialog renderer', () => {
     dialog.opened = true;
     await nextRender();
 
-    expect(dialog.textContent).to.equal('foo');
+    rendererRoot = dialog.firstElementChild;
+
+    expect(rendererRoot.textContent).to.equal('foo');
 
     dialog.renderer = null;
     await nextUpdate(dialog);
 
-    expect(dialog.textContent).to.equal('');
+    expect(rendererRoot.textContent).to.equal('');
   });
 });
diff --git a/packages/overlay/src/vaadin-overlay-mixin.js b/packages/overlay/src/vaadin-overlay-mixin.js
@@ -102,6 +102,15 @@ export const OverlayMixin = (superClass) =>
       return ['_rendererOrDataChanged(renderer, owner, model, opened)'];
     }
 
+    /**
+     * Override to specify another element used as a renderer root,
+     * e.g. slotted into the overlay, rather than overlay itself.
+     * @protected
+     */
+    get _rendererRoot() {
+      return this;
+    }
+
     constructor() {
       super();
 
@@ -175,7 +184,7 @@ export const OverlayMixin = (superClass) =>
      */
     requestContentUpdate() {
       if (this.renderer) {
-        this.renderer.call(this.owner, this._contentRoot, this.owner, this.model);
+        this.renderer.call(this.owner, this._rendererRoot, this.owner, this.model);
       }
     }
 
@@ -295,11 +304,11 @@ export const OverlayMixin = (superClass) =>
       this._oldOpened = opened;
 
       if (rendererChanged && hasOldRenderer) {
-        this._contentRoot.innerHTML = '';
+        this._rendererRoot.innerHTML = '';
         // Whenever a Lit-based renderer is used, it assigns a Lit part to the node it was rendered into.
         // When clearing the rendered content, this part needs to be manually disposed of.
         // Otherwise, using a Lit-based renderer on the same node will throw an exception or render nothing afterward.
-        delete this._contentRoot._$litPart$;
+        delete this._rendererRoot._$litPart$;
       }
 
       if (opened && renderer && (rendererChanged || openedChanged || ownerOrModelChanged)) {
diff --git a/packages/overlay/test/renderer.test.js b/packages/overlay/test/renderer.test.js
@@ -158,7 +158,7 @@ describe('renderer', () => {
     customElements.define(
       'custom-root-overlay',
       class extends Overlay {
-        get _contentRoot() {
+        get _rendererRoot() {
           return this.__customRoot;
         }
 
@@ -174,7 +174,7 @@ describe('renderer', () => {
     beforeEach(async () => {
       overlay = fixtureSync('<custom-root-overlay></custom-root-overlay>');
       await nextRender();
-      root = overlay._contentRoot;
+      root = overlay._rendererRoot;
       content = document.createElement('p');
       content.textContent = 'renderer-content';
     });
diff --git a/packages/overlay/test/restore-focus.test.js b/packages/overlay/test/restore-focus.test.js
@@ -193,6 +193,10 @@ describe('custom content root', () => {
       get _contentRoot() {
         return this.owner;
       }
+
+      get _rendererRoot() {
+        return this.owner;
+      }
     },
   );
 
diff --git a/packages/popover/src/vaadin-popover-overlay.js b/packages/popover/src/vaadin-popover-overlay.js
@@ -74,6 +74,14 @@ class PopoverOverlay extends PopoverOverlayMixin(
     return this.owner;
   }
 
+  /**
+   * @override
+   * @protected
+   */
+  get _rendererRoot() {
+    return this.owner;
+  }
+
   /**
    * Override method from OverlayFocusMixin to use owner as focus trap root
    * @protected
diff --git a/packages/select/src/vaadin-select-overlay-mixin.js b/packages/select/src/vaadin-select-overlay-mixin.js
@@ -27,10 +27,20 @@ export const SelectOverlayMixin = (superClass) =>
     }
 
     /**
-     * @override
+     * Override method from OverlayFocusMixin to use slotted div as content root.
      * @protected
+     * @override
      */
     get _contentRoot() {
+      return this._rendererRoot;
+    }
+
+    /**
+     * Override method from OverlayMixin to use slotted div as a renderer root.
+     * @protected
+     * @override
+     */
+    get _rendererRoot() {
       if (!this.__savedRoot) {
         const root = document.createElement('div');
         root.setAttribute('slot', 'overlay');
@@ -66,7 +76,7 @@ export const SelectOverlayMixin = (superClass) =>
 
     /** @protected */
     _getMenuElement() {
-      return Array.from(this._contentRoot.children).find((el) => el.localName !== 'style');
+      return Array.from(this._rendererRoot.children).find((el) => el.localName !== 'style');
     }
 
     /** @private */
diff --git a/packages/select/test/keyboard.test.js b/packages/select/test/keyboard.test.js
@@ -1,6 +1,6 @@
 import { expect } from '@vaadin/chai-plugins';
-import { sendKeys } from '@vaadin/test-runner-commands';
-import { aTimeout, fixtureSync, nextRender, nextUpdate, outsideClick } from '@vaadin/testing-helpers';
+import { resetMouse, sendKeys, sendMouseToElement } from '@vaadin/test-runner-commands';
+import { aTimeout, fixtureSync, nextRender, nextUpdate } from '@vaadin/testing-helpers';
 import sinon from 'sinon';
 import './not-animated-styles.js';
 import '../src/vaadin-select.js';
@@ -91,9 +91,10 @@ describe('keyboard', () => {
 
       const focusedSpy = sinon.spy(valueButton, 'focus');
 
-      outsideClick();
+      await sendMouseToElement({ type: 'click', element: document.body });
       await nextUpdate(select);
       await aTimeout(0);
+      await resetMouse();
 
       expect(focusedSpy.calledOnce).to.be.true;
     });
diff --git a/packages/select/test/lit-renderer-directives.test.js b/packages/select/test/lit-renderer-directives.test.js
@@ -39,12 +39,12 @@ describe('lit renderer directives', () => {
       });
 
       it('should render the content with the renderer', () => {
-        expect(overlay._contentRoot.textContent.trim()).to.equal('Content');
+        expect(overlay._rendererRoot.textContent.trim()).to.equal('Content');
       });
 
       it('should re-render the content when a renderer dependency changes', async () => {
         await renderOpenedSelect(container, { content: 'New Content' });
-        expect(overlay._contentRoot.textContent.trim()).to.equal('New Content');
+        expect(overlay._rendererRoot.textContent.trim()).to.equal('New Content');
       });
     });
 
diff --git a/packages/select/test/select.test.js b/packages/select/test/select.test.js
@@ -80,7 +80,7 @@ describe('vaadin-select', () => {
       await nextUpdate(select);
       const listBox = select._menuElement;
       expect(listBox.isConnected).to.be.true;
-      expect(listBox.parentNode).to.equal(select._overlayElement._contentRoot);
+      expect(listBox.parentNode).to.equal(select._overlayElement._rendererRoot);
     });
 
     it('should have position set to relative', () => {
