fix: slot dashboard widgets instead of dom reordering (#8359)

Author: tomivirkki

packages/dashboard/src/vaadin-dashboard-section.js

@@ -173,6 +173,12 @@ class DashboardSection extends DashboardItemMixin(ElementMixin(ThemableMixin(Pol
         type: String,
         value: '',
       },
+
+      /** @private */
+      __childCount: {
+        type: Number,
+        value: 0,
+      },
     };
   }
 
@@ -191,7 +197,10 @@ class DashboardSection extends DashboardItemMixin(ElementMixin(ThemableMixin(Pol
         </header>
       </div>
 
+      <!-- Default slot is used by <vaadin-dashboard-layout> -->
       <slot></slot>
+      <!-- Named slots are used by <vaadin-dashboard> -->
+      ${[...Array(this.__childCount)].map((_, index) => html`<slot name="slot-${index}"></slot>`)}
     `;
   }
 

packages/dashboard/src/vaadin-dashboard.js

@@ -195,6 +195,12 @@ class Dashboard extends DashboardLayoutMixin(ElementMixin(ThemableMixin(PolylitM
           };
         },
       },
+
+      /** @private */
+      __childCount: {
+        type: Number,
+        value: 0,
+      },
     };
   }
 
@@ -221,11 +227,14 @@ class Dashboard extends DashboardLayoutMixin(ElementMixin(ThemableMixin(PolylitM
 
   /** @protected */
   render() {
-    return html`<div id="grid"><slot></slot></div>`;
+    return html`<div id="grid">
+      ${[...Array(this.__childCount)].map((_, index) => html`<slot name="slot-${index}"></slot>`)}
+    </div>`;
   }
 
   /** @private */
   __itemsOrRendererChanged(items, renderer) {
+    this.__childCount = items ? items.length : 0;
     this.__renderItemWrappers(items || []);
 
     this.querySelectorAll(WRAPPER_LOCAL_NAME).forEach((wrapper) => {
@@ -257,38 +266,25 @@ class Dashboard extends DashboardLayoutMixin(ElementMixin(ThemableMixin(PolylitM
   __renderItemWrappers(items, hostElement = this) {
     // Get all the wrappers in the host element
     let wrappers = [...hostElement.children].filter((el) => el.localName === WRAPPER_LOCAL_NAME);
-    let previousWrapper = null;
 
     const focusedWrapper = wrappers.find((wrapper) => wrapper.querySelector(':focus'));
     const focusedWrapperWillBeRemoved = focusedWrapper && !this.__isActiveWrapper(focusedWrapper);
     const wrapperClosestToRemovedFocused =
       focusedWrapperWillBeRemoved && this.__getClosestActiveWrapper(focusedWrapper);
 
-    items.forEach((item) => {
+    items.forEach((item, index) => {
       // Find the wrapper for the item or create a new one
       const wrapper = wrappers.find((el) => itemsEqual(getElementItem(el), item)) || this.__createWrapper(item);
       wrappers = wrappers.filter((el) => el !== wrapper);
+      if (!wrapper.isConnected) {
+        hostElement.appendChild(wrapper);
+      }
 
       // Update the wrapper style
       this.__updateWrapper(wrapper, item);
 
-      if (wrapper !== focusedWrapper) {
-        if (previousWrapper) {
-          // Append the wrapper after the previous one if it's not already there
-          if (wrapper.previousElementSibling !== previousWrapper) {
-            previousWrapper.after(wrapper);
-          }
-        } else if (hostElement.firstChild) {
-          // Insert the wrapper as the first child of the host element if it's not already there
-          if (wrapper !== hostElement.firstChild) {
-            hostElement.insertBefore(wrapper, hostElement.firstChild);
-          }
-        } else {
-          // Append the wrapper to the empty host element
-          hostElement.appendChild(wrapper);
-        }
-      }
-      previousWrapper = wrapper;
+      // Update the wrapper slot
+      wrapper.slot = `slot-${index}`;
 
       // Render section if the item has subitems
       if (item.items) {
@@ -307,6 +303,7 @@ class Dashboard extends DashboardLayoutMixin(ElementMixin(ThemableMixin(PolylitM
         section.__i18n = this.i18n;
 
         // Render the subitems
+        section.__childCount = item.items.length;
         this.__renderItemWrappers(item.items, section);
       }
     });
@@ -368,20 +365,35 @@ class Dashboard extends DashboardLayoutMixin(ElementMixin(ThemableMixin(PolylitM
     return getItemsArrayOfItem(getElementItem(wrapper), this.items);
   }
 
+  /**
+   * Parses the slot name to get the index of the item in the dashboard
+   * For example, slot name "slot-12" will return 12
+   * @private
+   */
+  __parseSlotIndex(slotName) {
+    return parseInt(slotName.split('-')[1]);
+  }
+
   /** @private */
   __getClosestActiveWrapper(wrapper) {
     if (!wrapper || this.__isActiveWrapper(wrapper)) {
       return wrapper;
     }
 
+    // Sibling wrappers sorted by their slot name
+    const siblingWrappers = [...wrapper.parentElement.children].sort((a, b) => {
+      return this.__parseSlotIndex(a.slot) - this.__parseSlotIndex(b.slot);
+    });
+
     // Starting from the given wrapper element, iterates through the siblings in the given direction
     // to find the closest wrapper that represents an item in the dashboard's items array
     const findSiblingWrapper = (wrapper, dir) => {
       while (wrapper) {
         if (this.__isActiveWrapper(wrapper)) {
           return wrapper;
         }
-        wrapper = dir === 1 ? wrapper.nextElementSibling : wrapper.previousElementSibling;
+        const currentIndex = siblingWrappers.indexOf(wrapper);
+        wrapper = dir === 1 ? siblingWrappers[currentIndex + 1] : siblingWrappers[currentIndex - 1];
       }
     };
 

packages/dashboard/test/dashboard-keyboard.test.ts

@@ -23,6 +23,7 @@ import {
   setMinimumColumnWidth,
   setMinimumRowHeight,
   setSpacing,
+  updateComplete,
 } from './helpers.js';
 
 type TestDashboardItem = DashboardItem & { id: number };
@@ -209,6 +210,7 @@ describe('dashboard - keyboard interaction', () => {
 
     it('should move the widget backwards on arrow up', async () => {
       await sendKeys({ press: 'ArrowDown' });
+      await updateComplete(dashboard);
       await sendKeys({ press: 'ArrowUp' });
       expect(dashboard.items).to.eql([{ id: 0 }, { id: 1 }, { items: [{ id: 2 }, { id: 3 }] }]);
     });
@@ -245,6 +247,7 @@ describe('dashboard - keyboard interaction', () => {
       setMinimumRowHeight(dashboard, 100);
       await sendKeys({ down: 'Shift' });
       await sendKeys({ press: 'ArrowDown' });
+      await updateComplete(dashboard);
       await sendKeys({ press: 'ArrowUp' });
       await sendKeys({ up: 'Shift' });
       expect((dashboard.items[0] as DashboardItem).rowspan).to.equal(1);
@@ -363,6 +366,7 @@ describe('dashboard - keyboard interaction', () => {
 
       it('should move the widget backwards on arrow backwards', async () => {
         await sendKeys({ press: arrowForwards });
+        await updateComplete(dashboard);
         await sendKeys({ press: arrowBackwards });
         expect(dashboard.items).to.eql([{ id: 0 }, { id: 1 }, { items: [{ id: 2 }, { id: 3 }] }]);
       });
@@ -377,6 +381,7 @@ describe('dashboard - keyboard interaction', () => {
       it('should decrease the widget column span on shift + arrow backwards', async () => {
         await sendKeys({ down: 'Shift' });
         await sendKeys({ press: arrowForwards });
+        await updateComplete(dashboard);
         await sendKeys({ press: arrowBackwards });
         await sendKeys({ up: 'Shift' });
         expect((dashboard.items[0] as DashboardItem).colspan).to.equal(1);

packages/dashboard/test/dashboard-widget-reordering.test.ts

@@ -21,6 +21,7 @@ import {
   setMinimumColumnWidth,
   setMinimumRowHeight,
   setSpacing,
+  updateComplete,
 } from './helpers.js';
 
 type TestDashboardItem = DashboardItem & { id: number };
@@ -613,4 +614,35 @@ describe('dashboard - widget reordering', () => {
       });
     });
   });
+
+  it('should not disconnect widgets when reordering', async () => {
+    // Define a test element that spies on disconnectedCallback
+    const disconnectSpy = sinon.spy();
+    customElements.define(
+      'disconnect-test-element',
+      class extends HTMLElement {
+        disconnectedCallback() {
+          disconnectSpy();
+        }
+      },
+    );
+
+    // Assign a renderer that uses the test element
+    dashboard.renderer = (root, _, model) => {
+      if (root.querySelector('disconnect-test-element')) {
+        return;
+      }
+      root.innerHTML = `<vaadin-dashboard-widget id="${model.item.id}" widget-title="${model.item.id} title">
+          <disconnect-test-element></disconnect-test-element>
+        </vaadin-dashboard-widget>`;
+    };
+    await updateComplete(dashboard);
+
+    // Reorder the items
+    dashboard.items = [dashboard.items[1], dashboard.items[0]];
+    await updateComplete(dashboard);
+
+    // Expect no test element to have been disconnected
+    expect(disconnectSpy).to.not.have.been.called;
+  });
 });

packages/dashboard/test/dashboard.test.ts

@@ -681,6 +681,22 @@ describe('dashboard', () => {
         expect(document.activeElement).to.equal(getElementFromCell(dashboard, 0, 0)!);
       });
 
+      it('should focus next widget on focused widget removal after reordering', async () => {
+        getElementFromCell(dashboard, 0, 0)!.focus();
+        // Reorder items (the focused widget is moved to the end)
+        dashboard.items = [dashboard.items[1], dashboard.items[2], dashboard.items[0]];
+        await updateComplete(dashboard);
+
+        // Remove the focused widget
+        dashboard.items = [dashboard.items[0], dashboard.items[1]];
+        await updateComplete(dashboard);
+
+        // Expect the section to be focused
+        const sectionWidget = getElementFromCell(dashboard, 1, 0)!;
+        const section = getParentSection(sectionWidget)!;
+        expect(document.activeElement).to.equal(section);
+      });
+
       it('should focus the previous widget on focused widget removal', async () => {
         const sectionWidget = getElementFromCell(dashboard, 1, 1)!;
         getParentSection(sectionWidget)!.focus();