Skip to content

Commit

Permalink
refactor: remove widget focused on selection (#8199)
Browse files Browse the repository at this point in the history
  • Loading branch information
tomivirkki authored Nov 26, 2024
1 parent f83bfc2 commit 89e79cb
Show file tree
Hide file tree
Showing 8 changed files with 55 additions and 26 deletions.
8 changes: 0 additions & 8 deletions packages/dashboard/src/keyboard-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ export class KeyboardController {
this.host = host;

host.addEventListener('focusout', (e) => this.__focusout(e));
host.addEventListener('focusin', (e) => this.__focusin(e));
host.addEventListener('keydown', (e) => this.__keydown(e));
}

Expand All @@ -36,13 +35,6 @@ export class KeyboardController {
}
}

/** @private */
__focusin(e) {
if (e.target === this.host) {
this.host.__focused = true;
}
}

/** @private */
__keydown(e) {
if (e.metaKey || e.ctrlKey || !this.host.__selected) {
Expand Down
13 changes: 12 additions & 1 deletion packages/dashboard/src/vaadin-dashboard-item-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,19 @@ export const DashboardItemMixin = (superClass) =>
aria-label=${this.__i18n[i18nSelectTitleForEditingProperty]}
aria-describedby="title"
aria-pressed="${!!this.__selected}"
@focus="${() => {
this.__focused = true;
}}"
@blur="${() => {
this.__focused = false;
}}"
@click="${() => {
this.__selected = true;
const wasSelected = this.__selected;
this.__selected = !wasSelected;
this.__focused = wasSelected;
if (this.__selected) {
this.$['drag-handle'].focus();
}
}}"
></button>
</label>`;
Expand Down
4 changes: 3 additions & 1 deletion packages/dashboard/src/vaadin-dashboard-section.js
Original file line number Diff line number Diff line change
Expand Up @@ -179,9 +179,11 @@ class DashboardSection extends DashboardItemMixin(ElementMixin(ThemableMixin(Pol
/** @protected */
render() {
return html`
${this.__renderFocusButton('selectSection')} ${this.__renderMoveControls()}
${this.__renderMoveControls()}
<div id="focustrap">
${this.__renderFocusButton('selectSection')}
<header part="header">
${this.__renderDragHandle()}
<h2 id="title" part="title">${this.sectionTitle}</h2>
Expand Down
4 changes: 3 additions & 1 deletion packages/dashboard/src/vaadin-dashboard-widget.js
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,11 @@ class DashboardWidget extends DashboardItemMixin(ElementMixin(ThemableMixin(Poly
/** @protected */
render() {
return html`
${this.__renderFocusButton('selectWidget')} ${this.__renderMoveControls()} ${this.__renderResizeControls()}
${this.__renderMoveControls()} ${this.__renderResizeControls()}
<div id="focustrap">
${this.__renderFocusButton('selectWidget')}
<header part="header">
${this.__renderDragHandle()}
${this.__nestedHeadingLevel
Expand Down
4 changes: 2 additions & 2 deletions packages/dashboard/src/vaadin-dashboard.js
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ class Dashboard extends DashboardLayoutMixin(ElementMixin(ThemableMixin(PolylitM
let wrappers = [...hostElement.children].filter((el) => el.localName === WRAPPER_LOCAL_NAME);
let previousWrapper = null;

const focusedWrapper = wrappers.find((wrapper) => wrapper.querySelector('[focused]'));
const focusedWrapper = wrappers.find((wrapper) => wrapper.querySelector(':focus'));
const focusedWrapperWillBeRemoved = focusedWrapper && !this.__isActiveWrapper(focusedWrapper);
const wrapperClosestToRemovedFocused =
focusedWrapperWillBeRemoved && this.__getClosestActiveWrapper(focusedWrapper);
Expand Down Expand Up @@ -320,7 +320,7 @@ class Dashboard extends DashboardLayoutMixin(ElementMixin(ThemableMixin(PolylitM
this.__focusWrapperContent(wrapperClosestToRemovedFocused || this.querySelector(WRAPPER_LOCAL_NAME));
}

const focusedItem = this.querySelector('[focused]');
const focusedItem = this.querySelector(':focus');
if (focusedItem && this.__outsideViewport(focusedItem)) {
// If the focused wrapper is not in the viewport, scroll it into view
focusedItem.scrollIntoView();
Expand Down
42 changes: 30 additions & 12 deletions packages/dashboard/test/dashboard-keyboard.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,29 @@ describe('dashboard - keyboard interaction', () => {
const widget = getElementFromCell(dashboard, 0, 0)!;
await sendKeys({ press: 'Tab' });
await sendKeys({ press: 'Space' });
expect(widget.hasAttribute('focused')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.false;
expect(widget.hasAttribute('selected')).to.be.true;
expect(dashboard.hasAttribute('item-selected')).to.be.true;
});

it('should unselect the widget on focus button click', async () => {
const widget = getElementFromCell(dashboard, 0, 0)!;
await sendKeys({ press: 'Tab' });
await sendKeys({ press: 'Space' });

// Focus the focus-button with shift + tab
await sendKeys({ down: 'Shift' });
await sendKeys({ press: 'Tab' });
await sendKeys({ up: 'Shift' });

// Click the focus-button
await sendKeys({ press: 'Space' });

expect(widget.hasAttribute('focused')).to.be.true;
expect(widget.hasAttribute('selected')).to.be.false;
expect(dashboard.hasAttribute('item-selected')).to.be.false;
});

it('should dispatch a selection event', async () => {
const spy = sinon.spy();
dashboard.addEventListener('dashboard-item-selected-changed', spy);
Expand Down Expand Up @@ -448,7 +466,7 @@ describe('dashboard - keyboard interaction', () => {
// Enter move mode
await sendKeys({ press: 'Space' });

expect(widget.hasAttribute('focused')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.false;
expect(widget.hasAttribute('selected')).to.be.true;
expect(widget.hasAttribute('move-mode')).to.be.true;
});
Expand All @@ -471,7 +489,7 @@ describe('dashboard - keyboard interaction', () => {
(getDraggable(widget) as HTMLElement).click();
await nextFrame();

expect(widget.hasAttribute('focused')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.false;
expect(widget.hasAttribute('selected')).to.be.true;
expect(widget.hasAttribute('move-mode')).to.be.true;
});
Expand Down Expand Up @@ -501,7 +519,7 @@ describe('dashboard - keyboard interaction', () => {
await sendKeys({ press: 'Escape' });
expect(widget.hasAttribute('move-mode')).to.be.false;
expect(widget.hasAttribute('selected')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.false;
});

it('should dispatch a move mode event', async () => {
Expand All @@ -519,7 +537,7 @@ describe('dashboard - keyboard interaction', () => {
await sendKeys({ press: 'Space' });
expect(widget.hasAttribute('move-mode')).to.be.false;
expect(widget.hasAttribute('selected')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.false;
});

it('should focus drag handle on exit move mode', async () => {
Expand Down Expand Up @@ -594,7 +612,7 @@ describe('dashboard - keyboard interaction', () => {

expect(widget.hasAttribute('move-mode')).to.be.true;
expect(widget.hasAttribute('selected')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.false;
});

it('should not lose move mode when the focused forward button is hidden', async () => {
Expand Down Expand Up @@ -624,7 +642,7 @@ describe('dashboard - keyboard interaction', () => {

expect(widget.hasAttribute('move-mode')).to.be.true;
expect(widget.hasAttribute('selected')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.false;
});

it('should deselect the section on blur', async () => {
Expand Down Expand Up @@ -678,7 +696,7 @@ describe('dashboard - keyboard interaction', () => {
const widget = getElementFromCell(dashboard, 0, 0)!;
expect(widget.contains(document.activeElement)).to.be.true;
expect(widget.hasAttribute('selected')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.false;
dashboard.editable = false;
await nextFrame();
expect(widget.contains(document.activeElement)).to.be.false;
Expand All @@ -697,7 +715,7 @@ describe('dashboard - keyboard interaction', () => {
await sendKeys({ press: 'Tab' });
await sendKeys({ press: 'Space' });

expect(widget.hasAttribute('focused')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.false;
expect(widget.hasAttribute('selected')).to.be.true;
expect(widget.hasAttribute('resize-mode')).to.be.true;
});
Expand All @@ -707,7 +725,7 @@ describe('dashboard - keyboard interaction', () => {
(getResizeHandle(widget) as HTMLElement).click();
await nextFrame();

expect(widget.hasAttribute('focused')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.false;
expect(widget.hasAttribute('selected')).to.be.true;
expect(widget.hasAttribute('resize-mode')).to.be.true;
});
Expand Down Expand Up @@ -739,7 +757,7 @@ describe('dashboard - keyboard interaction', () => {
await sendKeys({ press: 'Escape' });
expect(widget.hasAttribute('resize-mode')).to.be.false;
expect(widget.hasAttribute('selected')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.false;
});

it('should dispatch a resize mode event', async () => {
Expand All @@ -756,7 +774,7 @@ describe('dashboard - keyboard interaction', () => {
await sendKeys({ press: 'Space' });
expect(widget.hasAttribute('resize-mode')).to.be.false;
expect(widget.hasAttribute('selected')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.false;
});

it('should focus resize handle on exit resize mode', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ describe('dashboard - widget reordering', () => {
(getDraggable(widget) as HTMLElement).click();
await nextFrame();
expect(widget.hasAttribute('selected')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.true;
expect(widget.hasAttribute('focused')).to.be.false;
expect(widget.hasAttribute('move-mode')).to.be.true;

fireDragStart(widget);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ const dashboardWidgetAndSection = css`
margin: calc(var(--_focus-ring-spacing-offset) * -1);
}
:host([selected])::before {
outline: 1px solid var(--_focus-ring-color);
}
:host([focused])::before {
outline: var(--_focus-ring-width) solid var(--_focus-ring-color);
}
Expand Down

0 comments on commit 89e79cb

Please sign in to comment.