Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: restore focus on dashboard widget removal #7860

Merged
merged 6 commits into from
Sep 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/dashboard/src/vaadin-dashboard-widget.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ class DashboardWidget extends DashboardItemMixin(ControllerMixin(ElementMixin(Po
cursor: grab;
line-height: 1;
z-index: 1;
overflow: hidden;
}

#resize-handle::before {
Expand Down
54 changes: 54 additions & 0 deletions packages/dashboard/src/vaadin-dashboard.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -230,6 +235,55 @@ 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.
requestAnimationFrame(() =>
this.__focusWrapperContent(wrapperClosestToRemovedFocused || this.querySelector(WRAPPER_LOCAL_NAME)),
);
}
}

/** @private */
__focusWrapperContent(wrapper) {
if (wrapper && 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 */
Expand Down
109 changes: 108 additions & 1 deletion packages/dashboard/test/dashboard.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@ 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 {
expectLayout,
getDraggable,
getElementFromCell,
getParentSection,
getRemoveButton,
getResizeHandle,
onceResized,
Expand Down Expand Up @@ -503,6 +505,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);
}
Expand Down Expand Up @@ -563,5 +566,109 @@ 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;
await nextFrame();

dashboard.items = [
{ id: 'Item 0' },
{ id: 'Item 1' },
{ title: 'Section', items: [{ id: 'Item 2' }, { id: 'Item 3' }] },
];
await nextFrame();

/* prettier-ignore */
expectLayout(dashboard, [
[0, 1],
[2, 3]
]);
});

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 () => {
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, 0, 1)!);
});

it('should focus the first widget on focused widget removal', async () => {
getElementFromCell(dashboard, 1, 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[2]];
await renderAndFocusRestore();
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.slice(0, 2);
await renderAndFocusRestore();
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 = getParentSection(widget)!;
widget.focus();
(dashboard.items[2] as DashboardSectionItem<TestDashboardItem>).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();
});
});
});
});
9 changes: 5 additions & 4 deletions packages/dashboard/test/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand All @@ -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) =>
Expand Down Expand Up @@ -165,7 +165,8 @@ export function expectLayout(dashboard: HTMLElement, layout: Array<Array<number
if (!element) {
actualRow.push(null);
} else {
actualRow.push(parseInt(element.id.replace('item-', '')));
// TODO: Just use a number for all test item IDs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add this to an improvements list or create an issue for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a small chore, shouldn't need an issue

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed here 274c8d9

actualRow.push(parseInt(element.id.replace('item-', '').replace('Item ', ''), 10));
}
});
});
Expand Down
Loading