From e13f2588724d09123e9bdf5b9f268685b0739b94 Mon Sep 17 00:00:00 2001
From: JC Franco <jfranco@esri.com>
Date: Fri, 1 Oct 2021 16:35:55 -0700
Subject: [PATCH] fix(label): fix issue where clicking on a wrapped labelable
 component would not update its value correctly

---
 .../calcite-checkbox/calcite-checkbox.tsx     | 19 ++++++------
 .../calcite-label/calcite-label.tsx           | 10 ++++--
 .../calcite-radio-button.tsx                  | 24 +++++++-------
 .../calcite-switch/calcite-switch.tsx         |  4 ---
 src/tests/commonTests.ts                      | 10 +++++-
 src/utils/label.ts                            | 31 +++++++++++++++++--
 6 files changed, 64 insertions(+), 34 deletions(-)

diff --git a/src/components/calcite-checkbox/calcite-checkbox.tsx b/src/components/calcite-checkbox/calcite-checkbox.tsx
index 63b06bbe285..fb5144e22e2 100644
--- a/src/components/calcite-checkbox/calcite-checkbox.tsx
+++ b/src/components/calcite-checkbox/calcite-checkbox.tsx
@@ -4,6 +4,7 @@ import {
   Event,
   EventEmitter,
   h,
+  Host,
   Listen,
   Method,
   Prop,
@@ -145,10 +146,6 @@ export class CalciteCheckbox implements LabelableComponent {
   };
 
   private clickHandler = (): void => {
-    if (this.labelEl) {
-      return;
-    }
-
     this.toggle();
   };
 
@@ -257,12 +254,14 @@ export class CalciteCheckbox implements LabelableComponent {
 
   render(): VNode {
     return (
-      <div class={{ focused: this.focused }} onClick={this.clickHandler}>
-        <svg class="check-svg" viewBox="0 0 16 16">
-          <path d={this.getPath()} />
-        </svg>
-        <slot />
-      </div>
+      <Host onClick={this.clickHandler}>
+        <div class={{ focused: this.focused, test: true }}>
+          <svg class="check-svg" viewBox="0 0 16 16">
+            <path d={this.getPath()} />
+          </svg>
+          <slot />
+        </div>
+      </Host>
     );
   }
 }
diff --git a/src/components/calcite-label/calcite-label.tsx b/src/components/calcite-label/calcite-label.tsx
index eb1b813c6d7..ab65cb0362a 100644
--- a/src/components/calcite-label/calcite-label.tsx
+++ b/src/components/calcite-label/calcite-label.tsx
@@ -57,7 +57,9 @@ export class CalciteLabel {
   /**
    * @internal
    */
-  @Event({ bubbles: false }) calciteInternalLabelClick: EventEmitter<void>;
+  @Event({ bubbles: false }) calciteInternalLabelClick: EventEmitter<{
+    sourceEvent: MouseEvent;
+  }>;
 
   //--------------------------------------------------------------------------
   //
@@ -65,8 +67,10 @@ export class CalciteLabel {
   //
   //--------------------------------------------------------------------------
 
-  labelClickHandler = (): void => {
-    this.calciteInternalLabelClick.emit();
+  labelClickHandler = (event: MouseEvent): void => {
+    this.calciteInternalLabelClick.emit({
+      sourceEvent: event
+    });
   };
 
   //--------------------------------------------------------------------------
diff --git a/src/components/calcite-radio-button/calcite-radio-button.tsx b/src/components/calcite-radio-button/calcite-radio-button.tsx
index 5eb84be5c11..6dbb37583ae 100644
--- a/src/components/calcite-radio-button/calcite-radio-button.tsx
+++ b/src/components/calcite-radio-button/calcite-radio-button.tsx
@@ -182,10 +182,6 @@ export class CalciteRadioButton implements LabelableComponent {
   };
 
   private clickHandler = (): void => {
-    if (this.labelEl) {
-      return;
-    }
-
     this.toggle();
   };
 
@@ -193,15 +189,17 @@ export class CalciteRadioButton implements LabelableComponent {
     if (!this.disabled && !this.hidden) {
       this.uncheckOtherRadioButtonsInGroup();
       const label = event.currentTarget as HTMLCalciteLabelElement;
-      const firstButton = this.rootNode.querySelector<HTMLCalciteRadioButtonElement>(
-        label.for
-          ? `calcite-radio-button[id="${label.for}"]`
-          : `calcite-radio-button[name="${this.name}"]`
-      );
-
-      if (firstButton) {
-        firstButton.checked = true;
-        firstButton.focused = true;
+      const radioButton = label.for
+        ? this.rootNode.querySelector<HTMLCalciteRadioButtonElement>(
+            `calcite-radio-button[id="${label.for}"]`
+          )
+        : label.querySelector<HTMLCalciteRadioButtonElement>(
+            `calcite-radio-button[name="${this.name}"]`
+          );
+
+      if (radioButton) {
+        radioButton.checked = true;
+        radioButton.focused = true;
       }
 
       this.calciteRadioButtonChange.emit();
diff --git a/src/components/calcite-switch/calcite-switch.tsx b/src/components/calcite-switch/calcite-switch.tsx
index a683ade8262..66f6a5dc7bd 100644
--- a/src/components/calcite-switch/calcite-switch.tsx
+++ b/src/components/calcite-switch/calcite-switch.tsx
@@ -141,10 +141,6 @@ export class CalciteSwitch implements LabelableComponent {
   }
 
   private clickHandler = (): void => {
-    if (this.labelEl) {
-      return;
-    }
-
     this.toggle();
   };
 
diff --git a/src/tests/commonTests.ts b/src/tests/commonTests.ts
index f4a20dd901b..180850ce909 100644
--- a/src/tests/commonTests.ts
+++ b/src/tests/commonTests.ts
@@ -267,7 +267,15 @@ async function assertLabelable({
   }
 
   if (propertyToToggle) {
-    expect(await component.getProperty(propertyToToggle)).toBe(!initialPropertyValue);
+    const toggledPropertyValue = !initialPropertyValue;
+    expect(await component.getProperty(propertyToToggle)).toBe(toggledPropertyValue);
+
+    // assert that direct clicks on component toggle property correctly
+    await component.setProperty(propertyToToggle, initialPropertyValue); // we reset as not all components toggle when clicked
+    await page.waitForChanges();
+    await component.click();
+    await page.waitForChanges();
+    expect(await component.getProperty(propertyToToggle)).toBe(toggledPropertyValue);
   }
 }
 
diff --git a/src/utils/label.ts b/src/utils/label.ts
index 36f51a90bbf..7c41a722692 100644
--- a/src/utils/label.ts
+++ b/src/utils/label.ts
@@ -24,6 +24,7 @@ export interface LabelableComponent {
 
 const labelTagName = "calcite-label";
 const labelClickEvent = "calciteInternalLabelClick";
+const onLabelClickMap = new WeakMap<HTMLElement, typeof onLabelClick>();
 
 const findLabelForComponent = (componentEl: HTMLElement): HTMLCalciteLabelElement | null => {
   const id = componentEl.id;
@@ -37,15 +38,29 @@ const findLabelForComponent = (componentEl: HTMLElement): HTMLCalciteLabelElemen
  * Helper to set up label interactions on connectedCallback.
  */
 export function connectLabel(component: LabelableComponent): void {
-  component.labelEl = findLabelForComponent(component.el);
-  component.labelEl?.addEventListener(labelClickEvent, component.onLabelClick);
+  const labelEl = findLabelForComponent(component.el);
+
+  if (!labelEl) {
+    return;
+  }
+
+  component.labelEl = labelEl;
+  const boundOnLabelClick = onLabelClick.bind(component);
+  onLabelClickMap.set(component.el, boundOnLabelClick);
+  component.labelEl.addEventListener(labelClickEvent, boundOnLabelClick);
 }
 
 /**
  * Helper to tear down label interactions on disconnectedCallback.
  */
 export function disconnectLabel(component: LabelableComponent): void {
-  component.labelEl?.removeEventListener(labelClickEvent, component.onLabelClick);
+  if (!component.labelEl) {
+    return;
+  }
+
+  const boundOnLabelClick = onLabelClickMap.get(component.el);
+  component.labelEl.removeEventListener(labelClickEvent, boundOnLabelClick);
+  onLabelClickMap.delete(component.el);
 }
 
 /**
@@ -54,3 +69,13 @@ export function disconnectLabel(component: LabelableComponent): void {
 export function getLabelText(component: LabelableComponent): string {
   return component.label || component.labelEl?.textContent?.trim() || "";
 }
+
+function onLabelClick(this: LabelableComponent, event: CustomEvent<{ sourceEvent: MouseEvent }>): void {
+  const containedLabelableChildClicked = this.el.contains(event.detail.sourceEvent.target as HTMLElement);
+
+  if (containedLabelableChildClicked) {
+    return;
+  }
+
+  this.onLabelClick(event);
+}