From 5583b5d6f08675fa53c959f960ed106fc795ac0d Mon Sep 17 00:00:00 2001 From: Tomi Virkki Date: Thu, 19 Dec 2024 16:59:03 +0200 Subject: [PATCH] fix: slot dashboard widgets instead of dom reordering (#8359) --- .../dashboard/src/vaadin-dashboard-section.js | 9 ++++ packages/dashboard/src/vaadin-dashboard.js | 54 +++++++++++-------- .../dashboard/test/dashboard-keyboard.test.ts | 5 ++ .../test/dashboard-widget-reordering.test.ts | 32 +++++++++++ packages/dashboard/test/dashboard.test.ts | 16 ++++++ 5 files changed, 95 insertions(+), 21 deletions(-) diff --git a/packages/dashboard/src/vaadin-dashboard-section.js b/packages/dashboard/src/vaadin-dashboard-section.js index c4410a3267..356421eafa 100644 --- a/packages/dashboard/src/vaadin-dashboard-section.js +++ b/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 + + + ${[...Array(this.__childCount)].map((_, index) => html``)} `; } diff --git a/packages/dashboard/src/vaadin-dashboard.js b/packages/dashboard/src/vaadin-dashboard.js index b211204081..83a8312631 100644 --- a/packages/dashboard/src/vaadin-dashboard.js +++ b/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`
`; + return html`
+ ${[...Array(this.__childCount)].map((_, index) => html``)} +
`; } /** @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,12 +365,26 @@ 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) => { @@ -381,7 +392,8 @@ class Dashboard extends DashboardLayoutMixin(ElementMixin(ThemableMixin(PolylitM 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]; } }; diff --git a/packages/dashboard/test/dashboard-keyboard.test.ts b/packages/dashboard/test/dashboard-keyboard.test.ts index 112b21efd5..a91d583865 100644 --- a/packages/dashboard/test/dashboard-keyboard.test.ts +++ b/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); diff --git a/packages/dashboard/test/dashboard-widget-reordering.test.ts b/packages/dashboard/test/dashboard-widget-reordering.test.ts index ade35e4724..70c9ea05e3 100644 --- a/packages/dashboard/test/dashboard-widget-reordering.test.ts +++ b/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 = ` + + `; + }; + 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; + }); }); diff --git a/packages/dashboard/test/dashboard.test.ts b/packages/dashboard/test/dashboard.test.ts index d93bbc40fa..57d3776112 100644 --- a/packages/dashboard/test/dashboard.test.ts +++ b/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();