From b8bb026ecf2ccc4bdaa6773cbfd3d497f0aeecee Mon Sep 17 00:00:00 2001 From: Tomi Virkki Date: Wed, 25 Sep 2024 09:41:57 +0300 Subject: [PATCH 1/6] feat: handle focused dashboard widget removal --- packages/dashboard/src/vaadin-dashboard.js | 55 ++++++++++++ packages/dashboard/test/dashboard.test.ts | 99 +++++++++++++++++++++- 2 files changed, 153 insertions(+), 1 deletion(-) diff --git a/packages/dashboard/src/vaadin-dashboard.js b/packages/dashboard/src/vaadin-dashboard.js index 9eb832ea14..21a25e8103 100644 --- a/packages/dashboard/src/vaadin-dashboard.js +++ b/packages/dashboard/src/vaadin-dashboard.js @@ -191,6 +191,11 @@ class Dashboard extends ControllerMixin(DashboardLayoutMixin(ElementMixin(Themab let wrappers = [...hostElement.children].filter((el) => el.localName === WRAPPER_LOCAL_NAME); let previousWrapper = null; + const focusedWrapper = wrappers.find((wrapper) => wrapper.querySelector('[focused]')); + const focusedWrapperWillBeRemoved = focusedWrapper && !this.__isActiveWrapper(focusedWrapper); + const wrapperClosestToRemovedFocused = + focusedWrapperWillBeRemoved && this.__getClosestActiveWrapper(focusedWrapper); + items.forEach((item) => { // Find the wrapper for the item or create a new one const wrapper = wrappers.find((el) => el.__item === item) || this.__createWrapper(item); @@ -230,6 +235,56 @@ class Dashboard extends ControllerMixin(DashboardLayoutMixin(ElementMixin(Themab // Remove the unused wrappers wrappers.forEach((wrapper) => wrapper.remove()); + + if (focusedWrapperWillBeRemoved) { + // The wrapper containing the focused element was removed. Try to focus the element in the closest wrapper. + const targetWrapper = wrapperClosestToRemovedFocused || this.querySelector(WRAPPER_LOCAL_NAME); + if (targetWrapper) { + requestAnimationFrame(() => this.__focusWrapperContent(targetWrapper)); + } + } + } + + /** @private */ + __focusWrapperContent(wrapper) { + if (wrapper.firstElementChild) { + wrapper.firstElementChild.focus(); + } + } + + /** + * Checks if the wrapper represents an item that is part of the dashboard's items array + * @private + */ + __isActiveWrapper(wrapper) { + if (!wrapper || wrapper.localName !== WRAPPER_LOCAL_NAME) { + return false; + } + return getItemsArrayOfItem(getElementItem(wrapper), this.items); + } + + /** @private */ + __getClosestActiveWrapper(wrapper) { + if (!wrapper || this.__isActiveWrapper(wrapper)) { + return wrapper; + } + + // 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; + } + }; + + return ( + findSiblingWrapper(wrapper, 1) || + findSiblingWrapper(wrapper, -1) || + this.__getClosestActiveWrapper(wrapper.parentElement.closest(WRAPPER_LOCAL_NAME)) + ); } /** @private */ diff --git a/packages/dashboard/test/dashboard.test.ts b/packages/dashboard/test/dashboard.test.ts index 9f27f63507..e26103bf71 100644 --- a/packages/dashboard/test/dashboard.test.ts +++ b/packages/dashboard/test/dashboard.test.ts @@ -5,7 +5,7 @@ import '../vaadin-dashboard.js'; import type { CustomElementType } from '@vaadin/component-base/src/define.js'; import type { DashboardSection } from '../src/vaadin-dashboard-section.js'; import type { DashboardWidget } from '../src/vaadin-dashboard-widget.js'; -import type { Dashboard, DashboardItem } from '../vaadin-dashboard.js'; +import type { Dashboard, DashboardItem, DashboardSectionItem } from '../vaadin-dashboard.js'; import { getDraggable, getElementFromCell, @@ -563,5 +563,102 @@ describe('dashboard', () => { const removeButton = getRemoveButton(section); expect(removeButton.getBoundingClientRect().height).to.be.above(0); }); + + describe('focus restore on focused item removal', () => { + beforeEach(async () => { + dashboard.editable = true; + dashboard.items = [ + { id: 'Item 0' }, + { id: 'Item 1' }, + { title: 'Section', items: [{ id: 'Item 2' }, { id: 'Item 3' }] }, + { id: 'Item 4' }, + { id: 'Item 5' }, + ]; + await nextFrame(); + }); + + async function renderAndFocusRestore() { + // Wait for the updated wrapper layout to be rendered + await nextFrame(); + // Wait for the wrapper widgets to be rendered + await nextFrame(); + } + + it('should focus next widget on focused widget removal', async () => { + getElementFromCell(dashboard, 0, 0)!.focus(); + dashboard.items = dashboard.items.slice(1); + await renderAndFocusRestore(); + expect(document.activeElement).to.equal(getElementFromCell(dashboard, 0, 0)!); + }); + + it('should focus the previous widget on focused widget removal', async () => { + getElementFromCell(dashboard, 2, 1)!.focus(); + dashboard.items = dashboard.items.slice(0, 4); + await renderAndFocusRestore(); + expect(document.activeElement).to.equal(getElementFromCell(dashboard, 2, 0)!); + }); + + it('should focus the first widget on focused widget removal', async () => { + getElementFromCell(dashboard, 2, 0)!.focus(); + dashboard.items = [dashboard.items[0]]; + await renderAndFocusRestore(); + expect(document.activeElement).to.equal(getElementFromCell(dashboard, 0, 0)!); + }); + + it('should focus the last widget on focused widget removal', async () => { + getElementFromCell(dashboard, 0, 0)!.focus(); + dashboard.items = [dashboard.items[4]]; + await renderAndFocusRestore(); + expect(document.activeElement).to.equal(getElementFromCell(dashboard, 0, 0)!); + }); + + it('should focus a root level widget on focused section removal', async () => { + getElementFromCell(dashboard, 1, 0)!.focus(); + dashboard.items = [dashboard.items[0], dashboard.items[1], dashboard.items[3], dashboard.items[4]]; + await renderAndFocusRestore(); + expect(document.activeElement).to.equal(getElementFromCell(dashboard, 1, 0)!); + }); + + it('should focus the section on focused section widget removal', async () => { + const widget = getElementFromCell(dashboard, 1, 0)!; + const section = widget.closest('vaadin-dashboard-section') as DashboardSection; + widget.focus(); + (dashboard.items[2] as DashboardSectionItem).items = []; + dashboard.items = [...dashboard.items]; + + await renderAndFocusRestore(); + expect(document.activeElement).to.equal(section); + }); + + it('should focus the body on all items removal', async () => { + getElementFromCell(dashboard, 0, 0)!.focus(); + dashboard.items = []; + await renderAndFocusRestore(); + expect(document.activeElement).to.equal(document.body); + }); + + it('should focus a new widget when items are replaced', async () => { + getElementFromCell(dashboard, 0, 0)!.focus(); + await nextFrame(); + dashboard.items = [{ id: 'Item 100' }]; + await renderAndFocusRestore(); + expect(document.activeElement).to.equal(getElementFromCell(dashboard, 0, 0)!); + }); + + it('should not restore focus if no widget had focus', async () => { + dashboard.items = dashboard.items.slice(1); + await renderAndFocusRestore(); + expect(document.activeElement).to.equal(document.body); + }); + + it('should not try to focus an empty wrapper', async () => { + getElementFromCell(dashboard, 0, 0)!.focus(); + dashboard.renderer = (root) => { + root.textContent = ''; + }; + dashboard.items = dashboard.items.slice(1); + await renderAndFocusRestore(); + }); + }); }); }); From b1b174f6261d077a2ac87bc2e343f09894181c32 Mon Sep 17 00:00:00 2001 From: Tomi Virkki Date: Wed, 25 Sep 2024 13:05:57 +0300 Subject: [PATCH 2/6] avoid resize handle overflow --- packages/dashboard/src/vaadin-dashboard-widget.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/dashboard/src/vaadin-dashboard-widget.js b/packages/dashboard/src/vaadin-dashboard-widget.js index e98de48f43..6785a52cbf 100644 --- a/packages/dashboard/src/vaadin-dashboard-widget.js +++ b/packages/dashboard/src/vaadin-dashboard-widget.js @@ -65,6 +65,7 @@ class DashboardWidget extends DashboardItemMixin(ControllerMixin(ElementMixin(Po cursor: grab; line-height: 1; z-index: 1; + overflow: hidden; } #resize-handle::before { From d34bdbf99a72fed1678579b7114e2f201e5a2fd2 Mon Sep 17 00:00:00 2001 From: Tomi Virkki Date: Wed, 25 Sep 2024 13:48:33 +0300 Subject: [PATCH 3/6] testing build failures --- packages/dashboard/test/dashboard.test.ts | 11 +++++++++++ packages/dashboard/test/helpers.ts | 3 ++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/packages/dashboard/test/dashboard.test.ts b/packages/dashboard/test/dashboard.test.ts index e26103bf71..627dc950ef 100644 --- a/packages/dashboard/test/dashboard.test.ts +++ b/packages/dashboard/test/dashboard.test.ts @@ -7,6 +7,7 @@ import type { DashboardSection } from '../src/vaadin-dashboard-section.js'; import type { DashboardWidget } from '../src/vaadin-dashboard-widget.js'; import type { Dashboard, DashboardItem, DashboardSectionItem } from '../vaadin-dashboard.js'; import { + expectLayout, getDraggable, getElementFromCell, getRemoveButton, @@ -503,6 +504,7 @@ describe('dashboard', () => { if (!widget || widget.tabIndex !== 0) { root.textContent = ''; widget = document.createElement('vaadin-dashboard-widget'); + widget.id = model.item.id; widget.tabIndex = 0; root.appendChild(widget); } @@ -567,6 +569,8 @@ describe('dashboard', () => { describe('focus restore on focused item removal', () => { beforeEach(async () => { dashboard.editable = true; + await nextFrame(); + dashboard.items = [ { id: 'Item 0' }, { id: 'Item 1' }, @@ -575,6 +579,13 @@ describe('dashboard', () => { { id: 'Item 5' }, ]; await nextFrame(); + + /* prettier-ignore */ + expectLayout(dashboard, [ + [0, 1], + [2, 3], + [4, 5] + ]); }); async function renderAndFocusRestore() { diff --git a/packages/dashboard/test/helpers.ts b/packages/dashboard/test/helpers.ts index d40d040de7..a89013609d 100644 --- a/packages/dashboard/test/helpers.ts +++ b/packages/dashboard/test/helpers.ts @@ -165,7 +165,8 @@ export function expectLayout(dashboard: HTMLElement, layout: Array Date: Wed, 25 Sep 2024 14:22:42 +0300 Subject: [PATCH 4/6] test: increase bottom offset when getting a widget from coordinates --- packages/dashboard/test/helpers.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dashboard/test/helpers.ts b/packages/dashboard/test/helpers.ts index a89013609d..062bdd1690 100644 --- a/packages/dashboard/test/helpers.ts +++ b/packages/dashboard/test/helpers.ts @@ -45,7 +45,7 @@ function _getElementFromCell(dashboard: HTMLElement, rowIndex: number, columnInd const y = top + rowHeights.slice(0, rowIndex).reduce((sum, height) => sum + height, 0); return document - .elementsFromPoint(x + (columnWidths[columnIndex] / 2) * (rtl ? -1 : 1), y + rowHeights[rowIndex] - 1) + .elementsFromPoint(x + (columnWidths[columnIndex] / 2) * (rtl ? -1 : 1), y + rowHeights[rowIndex] - 10) .reverse() .find( (element) => From a1f5b82828df12ee64f724331cb2db5485a03a13 Mon Sep 17 00:00:00 2001 From: Tomi Virkki Date: Wed, 25 Sep 2024 15:00:39 +0300 Subject: [PATCH 5/6] use a two-row dashboard for testing --- packages/dashboard/test/dashboard.test.ts | 25 +++++++++++------------ packages/dashboard/test/helpers.ts | 4 ++-- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/packages/dashboard/test/dashboard.test.ts b/packages/dashboard/test/dashboard.test.ts index 627dc950ef..fa842cca10 100644 --- a/packages/dashboard/test/dashboard.test.ts +++ b/packages/dashboard/test/dashboard.test.ts @@ -10,6 +10,7 @@ import { expectLayout, getDraggable, getElementFromCell, + getParentSection, getRemoveButton, getResizeHandle, onceResized, @@ -575,16 +576,13 @@ describe('dashboard', () => { { id: 'Item 0' }, { id: 'Item 1' }, { title: 'Section', items: [{ id: 'Item 2' }, { id: 'Item 3' }] }, - { id: 'Item 4' }, - { id: 'Item 5' }, ]; await nextFrame(); /* prettier-ignore */ expectLayout(dashboard, [ [0, 1], - [2, 3], - [4, 5] + [2, 3] ]); }); @@ -603,14 +601,15 @@ describe('dashboard', () => { }); it('should focus the previous widget on focused widget removal', async () => { - getElementFromCell(dashboard, 2, 1)!.focus(); - dashboard.items = dashboard.items.slice(0, 4); + const sectionWidget = getElementFromCell(dashboard, 1, 1)!; + getParentSection(sectionWidget)!.focus(); + dashboard.items = dashboard.items.slice(0, 2); await renderAndFocusRestore(); - expect(document.activeElement).to.equal(getElementFromCell(dashboard, 2, 0)!); + expect(document.activeElement).to.equal(getElementFromCell(dashboard, 0, 1)!); }); it('should focus the first widget on focused widget removal', async () => { - getElementFromCell(dashboard, 2, 0)!.focus(); + getElementFromCell(dashboard, 1, 0)!.focus(); dashboard.items = [dashboard.items[0]]; await renderAndFocusRestore(); expect(document.activeElement).to.equal(getElementFromCell(dashboard, 0, 0)!); @@ -618,21 +617,21 @@ describe('dashboard', () => { it('should focus the last widget on focused widget removal', async () => { getElementFromCell(dashboard, 0, 0)!.focus(); - dashboard.items = [dashboard.items[4]]; + dashboard.items = [dashboard.items[2]]; await renderAndFocusRestore(); - expect(document.activeElement).to.equal(getElementFromCell(dashboard, 0, 0)!); + expect(document.activeElement).to.equal(getParentSection(getElementFromCell(dashboard, 0, 0))!); }); it('should focus a root level widget on focused section removal', async () => { getElementFromCell(dashboard, 1, 0)!.focus(); - dashboard.items = [dashboard.items[0], dashboard.items[1], dashboard.items[3], dashboard.items[4]]; + dashboard.items = dashboard.items.slice(0, 2); await renderAndFocusRestore(); - expect(document.activeElement).to.equal(getElementFromCell(dashboard, 1, 0)!); + expect(document.activeElement).to.equal(getElementFromCell(dashboard, 0, 1)!); }); it('should focus the section on focused section widget removal', async () => { const widget = getElementFromCell(dashboard, 1, 0)!; - const section = widget.closest('vaadin-dashboard-section') as DashboardSection; + const section = getParentSection(widget)!; widget.focus(); (dashboard.items[2] as DashboardSectionItem).items = []; dashboard.items = [...dashboard.items]; diff --git a/packages/dashboard/test/helpers.ts b/packages/dashboard/test/helpers.ts index 062bdd1690..7ed17af0f3 100644 --- a/packages/dashboard/test/helpers.ts +++ b/packages/dashboard/test/helpers.ts @@ -14,11 +14,11 @@ export function getScrollingContainer(dashboard: Element): Element { return getCssGrid(dashboard); } -export function getParentSection(element?: Element | null): Element | null { +export function getParentSection(element?: Element | null): DashboardSection | null { if (!element) { return null; } - return element.closest('vaadin-dashboard-section'); + return element.closest('vaadin-dashboard-section') as DashboardSection; } /** From 75306f5a30c03a59f94b5d2f3d2053dbbef3fed6 Mon Sep 17 00:00:00 2001 From: Tomi Virkki Date: Wed, 25 Sep 2024 15:03:01 +0300 Subject: [PATCH 6/6] cleanup --- packages/dashboard/src/vaadin-dashboard.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/dashboard/src/vaadin-dashboard.js b/packages/dashboard/src/vaadin-dashboard.js index 21a25e8103..e022f9d822 100644 --- a/packages/dashboard/src/vaadin-dashboard.js +++ b/packages/dashboard/src/vaadin-dashboard.js @@ -238,16 +238,15 @@ class Dashboard extends ControllerMixin(DashboardLayoutMixin(ElementMixin(Themab if (focusedWrapperWillBeRemoved) { // The wrapper containing the focused element was removed. Try to focus the element in the closest wrapper. - const targetWrapper = wrapperClosestToRemovedFocused || this.querySelector(WRAPPER_LOCAL_NAME); - if (targetWrapper) { - requestAnimationFrame(() => this.__focusWrapperContent(targetWrapper)); - } + requestAnimationFrame(() => + this.__focusWrapperContent(wrapperClosestToRemovedFocused || this.querySelector(WRAPPER_LOCAL_NAME)), + ); } } /** @private */ __focusWrapperContent(wrapper) { - if (wrapper.firstElementChild) { + if (wrapper && wrapper.firstElementChild) { wrapper.firstElementChild.focus(); } }