Skip to content

Commit dc7a416

Browse files
nielslyngsoemadsrasmussen
nielslyngsoe
and
madsrasmussen
authored
fix: selectable mixin only be selective when selectabletarget is this (#971)
* only care about the composedPath when selectableTarget is this. * correct none this select target logic * align table row selection * correction of selectableMixin for keyboard navigation * adjust table row * add checkbox import * Revert "align table row selection" This reverts commit 976b568. # Conflicts: # packages/uui-table/lib/uui-table-row.element.ts * optimize code * revert to none preventing approach * media card selectable solution * install dependency * add tests for uui-color-swatch selectable * expand stories * selection mixing is selective about targets when selectable target is not changed * clean up * fix * fix test * test keyboard interaction * focus for swatch * menu item select only correction * clean up tests * clean up * only set tabindex on this targets * prevent click-label when select only * story and tests * use send mouse * update title * wrap tests --------- Co-authored-by: Mads Rasmussen <madsr@hey.com>
1 parent 03fe69c commit dc7a416

File tree

14 files changed

+533
-301
lines changed

14 files changed

+533
-301
lines changed

‎package-lock.json‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@
8282
"@types/mocha": "^10.0.7",
8383
"@web/dev-server-esbuild": "0.4.3",
8484
"@web/test-runner": "0.18.2",
85+
"@web/test-runner-commands": "^0.9.0",
8586
"@web/test-runner-playwright": "0.11.0",
8687
"autoprefixer": "10.4.17",
8788
"babel-loader": "9.1.3",

‎packages/uui-base/lib/mixins/SelectableMixin.ts‎

Lines changed: 62 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -52,11 +52,16 @@ export const SelectableMixin = <T extends Constructor<LitElement>>(
5252
}
5353
set selectable(newVal) {
5454
const oldVal = this._selectable;
55+
if (oldVal === newVal) return;
5556
this._selectable = newVal;
57+
5658
// Potentially problematic as a component might need focus for another feature when not selectable:
57-
if (this.selectableTarget === this) {
59+
if (this.#selectableTarget === this) {
5860
// If the selectable target, then make it self selectable. (A different selectable target should be made focusable by the component itself)
59-
this.setAttribute('tabindex', `${newVal ? '0' : '-1'}`);
61+
this.#selectableTarget.setAttribute(
62+
'tabindex',
63+
`${newVal ? '0' : '-1'}`,
64+
);
6065
}
6166
this.requestUpdate('selectable', oldVal);
6267
}
@@ -71,7 +76,31 @@ export const SelectableMixin = <T extends Constructor<LitElement>>(
7176
@property({ type: Boolean, reflect: true })
7277
public selected = false;
7378

74-
protected selectableTarget: EventTarget = this;
79+
#selectableTarget: Element = this;
80+
protected get selectableTarget(): EventTarget {
81+
return this.#selectableTarget;
82+
}
83+
protected set selectableTarget(target: EventTarget) {
84+
const oldTarget = this.#selectableTarget;
85+
86+
oldTarget.removeAttribute('tabindex');
87+
oldTarget.removeEventListener('click', this.#onClick);
88+
oldTarget.removeEventListener(
89+
'keydown',
90+
this.#onKeydown as EventListener,
91+
);
92+
93+
this.#selectableTarget = target as Element;
94+
if (this.#selectableTarget === this) {
95+
// If the selectable target, then make it self selectable. (A different selectable target should be made focusable by the component itself)
96+
this.#selectableTarget.setAttribute(
97+
'tabindex',
98+
this._selectable ? '0' : '-1',
99+
);
100+
}
101+
target.addEventListener('click', this.#onClick);
102+
target.addEventListener('keydown', this.#onKeydown as EventListener);
103+
}
75104

76105
constructor(...args: any[]) {
77106
super(...args);
@@ -80,25 +109,41 @@ export const SelectableMixin = <T extends Constructor<LitElement>>(
80109
}
81110

82111
readonly #onKeydown = (e: KeyboardEvent) => {
83-
const composePath = e.composedPath();
84-
if (
85-
(this._selectable || (this.deselectable && this.selected)) &&
86-
composePath.indexOf(this.selectableTarget) === 0
87-
) {
88-
if (this.selectableTarget === this) {
89-
if (e.code !== 'Space' && e.code !== 'Enter') return;
90-
this.#toggleSelect();
91-
e.preventDefault();
92-
}
112+
if (e.code !== 'Space' && e.code !== 'Enter') return;
113+
if (e.composedPath().indexOf(this.#selectableTarget) === 0) {
114+
this.#onClick(e);
93115
}
94116
};
95117

96118
readonly #onClick = (e: Event) => {
119+
const isSelectable =
120+
this._selectable || (this.deselectable && this.selected);
121+
122+
if (isSelectable === false) return;
123+
97124
const composePath = e.composedPath();
98-
if (
99-
(this._selectable || (this.deselectable && this.selected)) &&
100-
composePath.indexOf(this.selectableTarget) === 0
101-
) {
125+
126+
if (this.#selectableTarget === this) {
127+
// the selectableTarget is not specified which means we need to be selective about what we accept events from.
128+
const isActionTag = composePath.some(el => {
129+
const elementTagName = (el as HTMLElement).tagName;
130+
return (
131+
elementTagName === 'A' ||
132+
elementTagName === 'BUTTON' ||
133+
elementTagName === 'INPUT' ||
134+
elementTagName === 'TEXTAREA' ||
135+
elementTagName === 'SELECT'
136+
);
137+
});
138+
139+
// never select when clicking on a link or button
140+
if (isActionTag) return;
141+
}
142+
143+
if (composePath.indexOf(this.#selectableTarget) !== -1) {
144+
if (e.type === 'keydown') {
145+
e.preventDefault(); // Do not want the space key to trigger a page scroll.
146+
}
102147
this.#toggleSelect();
103148
}
104149
};

‎packages/uui-card-media/lib/uui-card-media.story.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,20 @@ export const Actions: Story = {
6464
},
6565
};
6666

67+
export const Href: Story = {
68+
args: {
69+
'actions slot': html`<uui-button
70+
slot="actions"
71+
look="secondary"
72+
label="Remove"
73+
>Remove</uui-button
74+
>`,
75+
selectable: true,
76+
href: 'https://umbraco.com',
77+
target: '_blank',
78+
},
79+
};
80+
6781
export const Image: Story = {
6882
args: {
6983
slot: html`<img src="https://placedog.net/1447/?random" alt="" />`,

‎packages/uui-color-swatch/lib/uui-color-swatch.element.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { defineElement } from '@umbraco-ui/uui-base/lib/registration';
22
import { property } from 'lit/decorators.js';
33
import { css, html, LitElement, nothing } from 'lit';
4+
import { ref } from 'lit/directives/ref.js';
45
import { iconCheck } from '@umbraco-ui/uui-icon-registry-essential/lib/svgs';
56
import {
67
ActiveMixin,
@@ -109,10 +110,19 @@ export class UUIColorSwatchElement extends LabelMixin(
109110
}
110111
}
111112

113+
focus(options?: FocusOptions | undefined): void {
114+
(this.selectableTarget as HTMLElement | undefined)?.focus(options);
115+
}
116+
117+
#selectButtonChanged(button?: Element | undefined) {
118+
this.selectableTarget = button || this;
119+
}
120+
112121
render() {
113122
return html`
114123
<button
115124
id="swatch"
125+
${ref(this.#selectButtonChanged)}
116126
aria-label=${this.label}
117127
?disabled="${this.disabled}"
118128
title="${this.label}">

‎packages/uui-color-swatch/lib/uui-color-swatch.test.ts‎

Lines changed: 70 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1-
import { html, fixture, expect } from '@open-wc/testing';
1+
import { html, fixture, expect, elementUpdated } from '@open-wc/testing';
22
import { UUIColorSwatchElement } from './uui-color-swatch.element';
3+
import { sendMouse, sendKeys } from '@web/test-runner-commands';
34

45
describe('UUIColorSwatchElement', () => {
56
let element: UUIColorSwatchElement;
@@ -17,4 +18,72 @@ describe('UUIColorSwatchElement', () => {
1718
it('passes the a11y audit', async () => {
1819
await expect(element).shadowDom.to.be.accessible();
1920
});
21+
22+
describe('selectable', () => {
23+
beforeEach(async () => {
24+
element.selectable = true;
25+
});
26+
27+
it('can be selected when selectable', async () => {
28+
await elementUpdated(element);
29+
await sendMouse({
30+
type: 'click',
31+
position: [15, 15],
32+
button: 'left',
33+
});
34+
expect(element.selected).to.be.true;
35+
});
36+
37+
it('can not be selected when not selectable', async () => {
38+
element.selectable = false;
39+
await elementUpdated(element);
40+
await sendMouse({
41+
type: 'click',
42+
position: [15, 15],
43+
button: 'left',
44+
});
45+
expect(element.selected).to.be.false;
46+
});
47+
48+
it('cant be selected when disabled', async () => {
49+
element.disabled = true;
50+
await elementUpdated(element);
51+
await sendMouse({
52+
type: 'click',
53+
position: [15, 15],
54+
button: 'left',
55+
});
56+
expect(element.selected).to.be.false;
57+
});
58+
59+
it('can be selected with Space key', async () => {
60+
await sendKeys({
61+
press: 'Tab',
62+
});
63+
await sendKeys({
64+
press: 'Space',
65+
});
66+
expect(element.selected).to.be.true;
67+
68+
await sendKeys({
69+
press: 'Space',
70+
});
71+
expect(element.selected).to.be.false;
72+
});
73+
74+
it('can be selected with Enter key', async () => {
75+
await sendKeys({
76+
press: 'Tab',
77+
});
78+
await sendKeys({
79+
press: 'Enter',
80+
});
81+
expect(element.selected).to.be.true;
82+
83+
await sendKeys({
84+
press: 'Enter',
85+
});
86+
expect(element.selected).to.be.false;
87+
});
88+
});
2089
});

‎packages/uui-menu-item/lib/uui-menu-item.element.ts‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -163,10 +163,11 @@ export class UUIMenuItemElement extends SelectOnlyMixin(
163163
this.showChildren = !this.showChildren;
164164
};
165165

166-
private _onLabelClicked = () => {
166+
#onLabelClicked() {
167+
if (this.selectOnly) return;
167168
const event = new UUIMenuItemEvent(UUIMenuItemEvent.CLICK_LABEL);
168169
this.dispatchEvent(event);
169-
};
170+
}
170171

171172
private _renderLabelInside() {
172173
return html` <slot
@@ -195,7 +196,7 @@ export class UUIMenuItemElement extends SelectOnlyMixin(
195196
this.target === '_blank' ? 'noopener noreferrer' : undefined,
196197
),
197198
)}
198-
@click=${this._onLabelClicked}
199+
@click=${this.#onLabelClicked}
199200
?disabled=${this.disabled}
200201
aria-label="${this.label}">
201202
${this._renderLabelInside()}
@@ -206,7 +207,7 @@ export class UUIMenuItemElement extends SelectOnlyMixin(
206207
return html` <button
207208
id="label-button"
208209
${ref(this._labelButtonChanged)}
209-
@click=${this._onLabelClicked}
210+
@click=${this.#onLabelClicked}
210211
?disabled=${this.disabled}
211212
aria-label="${this.label}">
212213
${this._renderLabelInside()}
@@ -226,12 +227,12 @@ export class UUIMenuItemElement extends SelectOnlyMixin(
226227
?open=${this.showChildren}></uui-symbol-expand>
227228
</button>`
228229
: ''}
229-
${this.href ? this._renderLabelAsAnchor() : this._renderLabelAsButton()}
230+
${this.href && this.selectOnly !== true && this.selectable !== true
231+
? this._renderLabelAsAnchor()
232+
: this._renderLabelAsButton()}
230233
231234
<div id="label-button-background"></div>
232-
${this.selectOnly === false
233-
? html`<slot id="actions-container" name="actions"></slot>`
234-
: ''}
235+
<slot id="actions-container" name="actions"></slot>
235236
${this.loading
236237
? html`<uui-loader-bar id="loader"></uui-loader-bar>`
237238
: ''}

‎packages/uui-menu-item/lib/uui-menu-item.story.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,12 @@ export const Selectable: Story = {
163163
selectable: true,
164164
},
165165
};
166+
export const SelectOnly: Story = {
167+
args: {
168+
selectable: true,
169+
selectOnly: true,
170+
},
171+
};
166172

167173
export const Anchor: Story = {
168174
args: {

0 commit comments

Comments
 (0)