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 <[email protected]>
umbraco · Nov 27, 2024 · dc7a416 · dc7a416
1 parent 03fe69c
commit dc7a416
Show file tree

Hide file tree

Showing 14 changed files with 533 additions and 301 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -82,6 +82,7 @@
     "@types/mocha": "^10.0.7",
     "@web/dev-server-esbuild": "0.4.3",
     "@web/test-runner": "0.18.2",
+    "@web/test-runner-commands": "^0.9.0",
     "@web/test-runner-playwright": "0.11.0",
     "autoprefixer": "10.4.17",
     "babel-loader": "9.1.3",

diff --git a/packages/uui-base/lib/mixins/SelectableMixin.ts b/packages/uui-base/lib/mixins/SelectableMixin.ts
@@ -52,11 +52,16 @@ export const SelectableMixin = <T extends Constructor<LitElement>>(
     }
     set selectable(newVal) {
       const oldVal = this._selectable;
+      if (oldVal === newVal) return;
       this._selectable = newVal;
+
       // Potentially problematic as a component might need focus for another feature when not selectable:
-      if (this.selectableTarget === this) {
+      if (this.#selectableTarget === this) {
         // If the selectable target, then make it self selectable. (A different selectable target should be made focusable by the component itself)
-        this.setAttribute('tabindex', `${newVal ? '0' : '-1'}`);
+        this.#selectableTarget.setAttribute(
+          'tabindex',
+          `${newVal ? '0' : '-1'}`,
+        );
       }
       this.requestUpdate('selectable', oldVal);
     }
@@ -71,7 +76,31 @@ export const SelectableMixin = <T extends Constructor<LitElement>>(
     @property({ type: Boolean, reflect: true })
     public selected = false;
 
-    protected selectableTarget: EventTarget = this;
+    #selectableTarget: Element = this;
+    protected get selectableTarget(): EventTarget {
+      return this.#selectableTarget;
+    }
+    protected set selectableTarget(target: EventTarget) {
+      const oldTarget = this.#selectableTarget;
+
+      oldTarget.removeAttribute('tabindex');
+      oldTarget.removeEventListener('click', this.#onClick);
+      oldTarget.removeEventListener(
+        'keydown',
+        this.#onKeydown as EventListener,
+      );
+
+      this.#selectableTarget = target as Element;
+      if (this.#selectableTarget === this) {
+        // If the selectable target, then make it self selectable. (A different selectable target should be made focusable by the component itself)
+        this.#selectableTarget.setAttribute(
+          'tabindex',
+          this._selectable ? '0' : '-1',
+        );
+      }
+      target.addEventListener('click', this.#onClick);
+      target.addEventListener('keydown', this.#onKeydown as EventListener);
+    }
 
     constructor(...args: any[]) {
       super(...args);
@@ -80,25 +109,41 @@ export const SelectableMixin = <T extends Constructor<LitElement>>(
     }
 
     readonly #onKeydown = (e: KeyboardEvent) => {
-      const composePath = e.composedPath();
-      if (
-        (this._selectable || (this.deselectable && this.selected)) &&
-        composePath.indexOf(this.selectableTarget) === 0
-      ) {
-        if (this.selectableTarget === this) {
-          if (e.code !== 'Space' && e.code !== 'Enter') return;
-          this.#toggleSelect();
-          e.preventDefault();
-        }
+      if (e.code !== 'Space' && e.code !== 'Enter') return;
+      if (e.composedPath().indexOf(this.#selectableTarget) === 0) {
+        this.#onClick(e);
       }
     };
 
     readonly #onClick = (e: Event) => {
+      const isSelectable =
+        this._selectable || (this.deselectable && this.selected);
+
+      if (isSelectable === false) return;
+
       const composePath = e.composedPath();
-      if (
-        (this._selectable || (this.deselectable && this.selected)) &&
-        composePath.indexOf(this.selectableTarget) === 0
-      ) {
+
+      if (this.#selectableTarget === this) {
+        // the selectableTarget is not specified which means we need to be selective about what we accept events from.
+        const isActionTag = composePath.some(el => {
+          const elementTagName = (el as HTMLElement).tagName;
+          return (
+            elementTagName === 'A' ||
+            elementTagName === 'BUTTON' ||
+            elementTagName === 'INPUT' ||
+            elementTagName === 'TEXTAREA' ||
+            elementTagName === 'SELECT'
+          );
+        });
+
+        // never select when clicking on a link or button
+        if (isActionTag) return;
+      }
+
+      if (composePath.indexOf(this.#selectableTarget) !== -1) {
+        if (e.type === 'keydown') {
+          e.preventDefault(); // Do not want the space key to trigger a page scroll.
+        }
         this.#toggleSelect();
       }
     };

diff --git a/packages/uui-card-media/lib/uui-card-media.story.ts b/packages/uui-card-media/lib/uui-card-media.story.ts
@@ -64,6 +64,20 @@ export const Actions: Story = {
   },
 };
 
+export const Href: Story = {
+  args: {
+    'actions slot': html`<uui-button
+      slot="actions"
+      look="secondary"
+      label="Remove"
+      >Remove</uui-button
+    >`,
+    selectable: true,
+    href: 'https://umbraco.com',
+    target: '_blank',
+  },
+};
+
 export const Image: Story = {
   args: {
     slot: html`<img src="https://placedog.net/1447/?random" alt="" />`,

diff --git a/packages/uui-color-swatch/lib/uui-color-swatch.element.ts b/packages/uui-color-swatch/lib/uui-color-swatch.element.ts
@@ -1,6 +1,7 @@
 import { defineElement } from '@umbraco-ui/uui-base/lib/registration';
 import { property } from 'lit/decorators.js';
 import { css, html, LitElement, nothing } from 'lit';
+import { ref } from 'lit/directives/ref.js';
 import { iconCheck } from '@umbraco-ui/uui-icon-registry-essential/lib/svgs';
 import {
   ActiveMixin,
@@ -109,10 +110,19 @@ export class UUIColorSwatchElement extends LabelMixin(
     }
   }
 
+  focus(options?: FocusOptions | undefined): void {
+    (this.selectableTarget as HTMLElement | undefined)?.focus(options);
+  }
+
+  #selectButtonChanged(button?: Element | undefined) {
+    this.selectableTarget = button || this;
+  }
+
   render() {
     return html`
       <button
         id="swatch"
+        ${ref(this.#selectButtonChanged)}
         aria-label=${this.label}
         ?disabled="${this.disabled}"
         title="${this.label}">

diff --git a/packages/uui-color-swatch/lib/uui-color-swatch.test.ts b/packages/uui-color-swatch/lib/uui-color-swatch.test.ts
@@ -1,5 +1,6 @@
-import { html, fixture, expect } from '@open-wc/testing';
+import { html, fixture, expect, elementUpdated } from '@open-wc/testing';
 import { UUIColorSwatchElement } from './uui-color-swatch.element';
+import { sendMouse, sendKeys } from '@web/test-runner-commands';
 
 describe('UUIColorSwatchElement', () => {
   let element: UUIColorSwatchElement;
@@ -17,4 +18,72 @@ describe('UUIColorSwatchElement', () => {
   it('passes the a11y audit', async () => {
     await expect(element).shadowDom.to.be.accessible();
   });
+
+  describe('selectable', () => {
+    beforeEach(async () => {
+      element.selectable = true;
+    });
+
+    it('can be selected when selectable', async () => {
+      await elementUpdated(element);
+      await sendMouse({
+        type: 'click',
+        position: [15, 15],
+        button: 'left',
+      });
+      expect(element.selected).to.be.true;
+    });
+
+    it('can not be selected when not selectable', async () => {
+      element.selectable = false;
+      await elementUpdated(element);
+      await sendMouse({
+        type: 'click',
+        position: [15, 15],
+        button: 'left',
+      });
+      expect(element.selected).to.be.false;
+    });
+
+    it('cant be selected when disabled', async () => {
+      element.disabled = true;
+      await elementUpdated(element);
+      await sendMouse({
+        type: 'click',
+        position: [15, 15],
+        button: 'left',
+      });
+      expect(element.selected).to.be.false;
+    });
+
+    it('can be selected with Space key', async () => {
+      await sendKeys({
+        press: 'Tab',
+      });
+      await sendKeys({
+        press: 'Space',
+      });
+      expect(element.selected).to.be.true;
+
+      await sendKeys({
+        press: 'Space',
+      });
+      expect(element.selected).to.be.false;
+    });
+
+    it('can be selected with Enter key', async () => {
+      await sendKeys({
+        press: 'Tab',
+      });
+      await sendKeys({
+        press: 'Enter',
+      });
+      expect(element.selected).to.be.true;
+
+      await sendKeys({
+        press: 'Enter',
+      });
+      expect(element.selected).to.be.false;
+    });
+  });
 });
diff --git a/packages/uui-menu-item/lib/uui-menu-item.element.ts b/packages/uui-menu-item/lib/uui-menu-item.element.ts
@@ -163,10 +163,11 @@ export class UUIMenuItemElement extends SelectOnlyMixin(
     this.showChildren = !this.showChildren;
   };
 
-  private _onLabelClicked = () => {
+  #onLabelClicked() {
+    if (this.selectOnly) return;
     const event = new UUIMenuItemEvent(UUIMenuItemEvent.CLICK_LABEL);
     this.dispatchEvent(event);
-  };
+  }
 
   private _renderLabelInside() {
     return html` <slot
@@ -195,7 +196,7 @@ export class UUIMenuItemElement extends SelectOnlyMixin(
             this.target === '_blank' ? 'noopener noreferrer' : undefined,
           ),
       )}
-      @click=${this._onLabelClicked}
+      @click=${this.#onLabelClicked}
       ?disabled=${this.disabled}
       aria-label="${this.label}">
       ${this._renderLabelInside()}
@@ -206,7 +207,7 @@ export class UUIMenuItemElement extends SelectOnlyMixin(
     return html` <button
       id="label-button"
       ${ref(this._labelButtonChanged)}
-      @click=${this._onLabelClicked}
+      @click=${this.#onLabelClicked}
       ?disabled=${this.disabled}
       aria-label="${this.label}">
       ${this._renderLabelInside()}
@@ -226,12 +227,12 @@ export class UUIMenuItemElement extends SelectOnlyMixin(
                 ?open=${this.showChildren}></uui-symbol-expand>
             </button>`
           : ''}
-        ${this.href ? this._renderLabelAsAnchor() : this._renderLabelAsButton()}
+        ${this.href && this.selectOnly !== true && this.selectable !== true
+          ? this._renderLabelAsAnchor()
+          : this._renderLabelAsButton()}
 
         <div id="label-button-background"></div>
-        ${this.selectOnly === false
-          ? html`<slot id="actions-container" name="actions"></slot>`
-          : ''}
+        <slot id="actions-container" name="actions"></slot>
         ${this.loading
           ? html`<uui-loader-bar id="loader"></uui-loader-bar>`
           : ''}

diff --git a/packages/uui-menu-item/lib/uui-menu-item.story.ts b/packages/uui-menu-item/lib/uui-menu-item.story.ts
@@ -163,6 +163,12 @@ export const Selectable: Story = {
     selectable: true,
   },
 };
+export const SelectOnly: Story = {
+  args: {
+    selectable: true,
+    selectOnly: true,
+  },
+};
 
 export const Anchor: Story = {
   args: {