Merge pull request #963 from umbraco/v1/bugfix/do-not-select-when-opening

Fix: do not select when opening href on selectable elements

Author: nielslyngsoe

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

@@ -75,25 +75,45 @@ export const SelectableMixin = <T extends Constructor<LitElement>>(
 
     constructor(...args: any[]) {
       super(...args);
-      this.addEventListener('click', this._handleClick);
-      this.addEventListener('keydown', this.handleSelectKeydown);
+      this.addEventListener('click', this.#onClick);
+      this.addEventListener('keydown', this.#onKeydown);
     }
 
-    private handleSelectKeydown = (e: KeyboardEvent) => {
+    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();
+          this.#toggleSelect();
           e.preventDefault();
         }
       }
     };
 
-    private _select() {
+    readonly #onClick = (e: Event) => {
+      const composePath = e.composedPath();
+      if (
+        (this._selectable || (this.deselectable && this.selected)) &&
+        composePath.indexOf(this.selectableTarget) === 0
+      ) {
+        this.#toggleSelect();
+      }
+    };
+
+    #toggleSelect() {
+      // Only allow for select-interaction if selectable is true. Deselectable is ignored in this case, we do not want a DX where only deselection is a possibility..
+      if (!this.selectable) return;
+      if (this.deselectable === false) {
+        this.#select();
+      } else {
+        this.selected ? this.#deselect() : this.#select();
+      }
+    }
+
+    #select() {
       if (!this.selectable) return;
       const selectEvent = new UUISelectableEvent(UUISelectableEvent.SELECTED);
       this.dispatchEvent(selectEvent);
@@ -102,30 +122,14 @@ export const SelectableMixin = <T extends Constructor<LitElement>>(
       this.selected = true;
     }
 
-    private _deselect() {
+    #deselect() {
       if (!this.deselectable) return;
       const selectEvent = new UUISelectableEvent(UUISelectableEvent.DESELECTED);
       this.dispatchEvent(selectEvent);
       if (selectEvent.defaultPrevented) return;
 
       this.selected = false;
     }
-
-    private _handleClick(e: Event) {
-      if (e.composedPath().indexOf(this.selectableTarget) !== -1) {
-        this._toggleSelect();
-      }
-    }
-
-    private _toggleSelect() {
-      // Only allow for select-interaction if selectable is true. Deselectable is ignored in this case, we do not want a DX where only deselection is a possibility..
-      if (!this.selectable) return;
-      if (this.deselectable === false) {
-        this._select();
-      } else {
-        this.selected ? this._deselect() : this._select();
-      }
-    }
   }
   // prettier-ignore
   return (SelectableMixinClass as unknown) as Constructor<SelectableMixinInterface> & T;

packages/uui-card-media/lib/uui-card-media.test.ts

@@ -99,6 +99,35 @@ describe('UUICardMediaElement', () => {
         expect(event.type).to.equal(UUISelectableEvent.SELECTED);
         expect(element.selected).to.be.true;
       });
+      it('do react to a click event on performed in this way', async () => {
+        element.selectable = true;
+        await elementUpdated(element);
+        element.click();
+        await Promise.resolve();
+        expect(element.selected).to.be.true;
+      });
+      it('do not react to a click event on other parts', async () => {
+        element.selectable = true;
+        await elementUpdated(element);
+        const listener = oneEvent(element, UUICardEvent.OPEN);
+        element
+          .shadowRoot!.querySelector<HTMLAnchorElement>('#open-part')!
+          .click();
+        const event = await listener;
+        expect(event).to.exist;
+        expect(event.type).to.equal(UUICardEvent.OPEN);
+        expect(element.selected).to.be.false;
+      });
+      it('do not react to a click event on other parts as href', async () => {
+        element.selectable = true;
+        element.href = '#hello';
+        await elementUpdated(element);
+        element
+          .shadowRoot!.querySelector<HTMLAnchorElement>('#open-part')!
+          .click();
+        await Promise.resolve();
+        expect(element.selected).to.be.false;
+      });
       it('can be selected with keyboard', async () => {
         element.selectable = true;
         await elementUpdated(element);