From 5b7e422dc0bfd4d58fb31f62715af47e62dabb57 Mon Sep 17 00:00:00 2001
From: Sean Perkins <13732623+sean-perkins@users.noreply.github.com>
Date: Tue, 19 Sep 2023 23:27:36 -0400
Subject: [PATCH] fix(radio,toggle,checkbox,select): padded space is clickable
 in items (#28136)

Issue number: Resolves #27169

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

Clicking the padded space within an `ion-item` will not pass the click
event to the slotted `ion-radio`, `ion-checkbox`, `ion-select` or
`ion-toggle`.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- The padded space at the start of `.item-native` and at the end of
`.item-inner` is clickable to activate a control.
- When the item is clicked, we check if the event is a result of
clicking the control or clicking the item's padded space. If the click
event is on the control, we don't need to do anything and let the
default behavior occur. If the click event is on the padded space, we
manually call the `.click()` method for the interactive element.
- The cursor pointer displays when hovering over the padded space when a
slotted interactive control is present.


## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->
---
 core/src/components/checkbox/checkbox.tsx     | 11 +++-
 .../checkbox/test/basic/checkbox.e2e.ts       | 23 ++++++++
 .../test/legacy/basic/checkbox.e2e.ts         | 23 ++++++++
 core/src/components/item/item.scss            |  8 +++
 core/src/components/item/item.tsx             | 58 ++++++++++++++++---
 .../components/radio/test/item/radio.e2e.ts   | 29 ++++++++++
 .../select/test/basic/select.e2e.ts           | 28 +++++++++
 .../components/toggle/test/item/toggle.e2e.ts | 34 +++++++++++
 core/src/components/toggle/toggle.tsx         |  2 +-
 9 files changed, 205 insertions(+), 11 deletions(-)

diff --git a/core/src/components/checkbox/checkbox.tsx b/core/src/components/checkbox/checkbox.tsx
index cf463bfbf55..a90ed53c458 100644
--- a/core/src/components/checkbox/checkbox.tsx
+++ b/core/src/components/checkbox/checkbox.tsx
@@ -140,9 +140,8 @@ export class Checkbox implements ComponentInterface {
    */
   @Event() ionStyle!: EventEmitter<StyleEventDetail>;
 
-  // TODO(FW-3100): remove this
   connectedCallback() {
-    this.legacyFormController = createLegacyFormController(this.el);
+    this.legacyFormController = createLegacyFormController(this.el); // TODO(FW-3100): remove this
   }
 
   componentWillLoad() {
@@ -195,7 +194,7 @@ export class Checkbox implements ComponentInterface {
     });
   };
 
-  private toggleChecked = (ev: any) => {
+  private toggleChecked = (ev: Event) => {
     ev.preventDefault();
 
     this.setFocus();
@@ -211,6 +210,10 @@ export class Checkbox implements ComponentInterface {
     this.ionBlur.emit();
   };
 
+  private onClick = (ev: MouseEvent) => {
+    this.toggleChecked(ev);
+  };
+
   // TODO(FW-3100): run contents of renderCheckbox directly instead
   render() {
     const { legacyFormController } = this;
@@ -252,6 +255,7 @@ export class Checkbox implements ComponentInterface {
           [`checkbox-alignment-${alignment}`]: true,
           [`checkbox-label-placement-${labelPlacement}`]: true,
         })}
+        onClick={this.onClick}
       >
         <label class="checkbox-wrapper">
           {/*
@@ -333,6 +337,7 @@ Developers can dismiss this warning by removing their usage of the "legacy" prop
           'legacy-checkbox': true,
           interactive: true,
         })}
+        onClick={this.onClick}
       >
         <svg class="checkbox-icon" viewBox="0 0 24 24" part="container">
           {path}
diff --git a/core/src/components/checkbox/test/basic/checkbox.e2e.ts b/core/src/components/checkbox/test/basic/checkbox.e2e.ts
index 83d9c2858ea..e06367f6887 100644
--- a/core/src/components/checkbox/test/basic/checkbox.e2e.ts
+++ b/core/src/components/checkbox/test/basic/checkbox.e2e.ts
@@ -83,5 +83,28 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
       await checkbox.evaluate((el: HTMLIonCheckboxElement) => (el.checked = true));
       expect(ionChange).not.toHaveReceivedEvent();
     });
+
+    test('clicking padded space within item should click the checkbox', async ({ page }) => {
+      await page.setContent(
+        `
+        <ion-item>
+          <ion-checkbox>Size</ion-checkbox>
+        </ion-item>
+      `,
+        config
+      );
+      const itemNative = page.locator('.item-native');
+      const ionChange = await page.spyOnEvent('ionChange');
+
+      // Clicks the padded space within the item
+      await itemNative.click({
+        position: {
+          x: 5,
+          y: 5,
+        },
+      });
+
+      expect(ionChange).toHaveReceivedEvent();
+    });
   });
 });
diff --git a/core/src/components/checkbox/test/legacy/basic/checkbox.e2e.ts b/core/src/components/checkbox/test/legacy/basic/checkbox.e2e.ts
index 2bcbbed58cb..873fa15f2f9 100644
--- a/core/src/components/checkbox/test/legacy/basic/checkbox.e2e.ts
+++ b/core/src/components/checkbox/test/legacy/basic/checkbox.e2e.ts
@@ -67,5 +67,28 @@ configs({ directions: ['ltr'] }).forEach(({ title, config }) => {
       await checkbox.evaluate((el: HTMLIonCheckboxElement) => (el.checked = true));
       await expect(ionChange).not.toHaveReceivedEvent();
     });
+
+    test('clicking padded space within item should click the checkbox', async ({ page }) => {
+      await page.setContent(
+        `
+        <ion-item>
+          <ion-checkbox legacy="true" value="my-checkbox"></ion-checkbox>
+        </ion-item>
+      `,
+        config
+      );
+      const itemNative = page.locator('.item-native');
+      const ionChange = await page.spyOnEvent('ionChange');
+
+      // Clicks the padded space within the item
+      await itemNative.click({
+        position: {
+          x: 5,
+          y: 5,
+        },
+      });
+
+      expect(ionChange).toHaveReceivedEvent();
+    });
   });
 });
diff --git a/core/src/components/item/item.scss b/core/src/components/item/item.scss
index 46e2cafe052..eb71271dd0f 100644
--- a/core/src/components/item/item.scss
+++ b/core/src/components/item/item.scss
@@ -173,6 +173,13 @@
   }
 }
 
+// Item: Interactive
+// --------------------------------------------------
+
+:host(.item-has-interactive-control) {
+  cursor: pointer;
+}
+
 
 // Item: Disabled
 // --------------------------------------------------
@@ -189,6 +196,7 @@
 }
 
 
+
 // Native Item
 // --------------------------------------------------
 
diff --git a/core/src/components/item/item.tsx b/core/src/components/item/item.tsx
index 6e2d48186da..8fe7d50ec97 100644
--- a/core/src/components/item/item.tsx
+++ b/core/src/components/item/item.tsx
@@ -1,5 +1,5 @@
 import type { ComponentInterface } from '@stencil/core';
-import { Component, Element, Host, Listen, Prop, State, Watch, forceUpdate, h } from '@stencil/core';
+import { Build, Component, Element, Host, Listen, Prop, State, Watch, forceUpdate, h } from '@stencil/core';
 import type { AnchorInterface, ButtonInterface } from '@utils/element-interface';
 import type { Attributes } from '@utils/helpers';
 import { inheritAttributes, raf } from '@utils/helpers';
@@ -350,6 +350,22 @@ export class Item implements ComponentInterface, AnchorInterface, ButtonInterfac
     }
   }
 
+  private getFirstInteractive() {
+    if (Build.isTesting) {
+      /**
+       * Pseudo selectors can't be tested in unit tests.
+       * It will cause an error when running the tests.
+       *
+       * TODO: FW-5205 - Remove the build conditional when this is fixed in Stencil
+       */
+      return undefined;
+    }
+    const controls = this.el.querySelectorAll<HTMLElement>(
+      'ion-toggle:not([disabled]), ion-checkbox:not([disabled]), ion-radio:not([disabled]), ion-select:not([disabled])'
+    );
+    return controls[0];
+  }
+
   render() {
     const {
       counterString,
@@ -367,6 +383,7 @@ export class Item implements ComponentInterface, AnchorInterface, ButtonInterfac
       routerAnimation,
       routerDirection,
       inheritedAriaAttributes,
+      multipleInputs,
     } = this;
     const childStyles = {} as StyleEventDetail;
     const mode = getIonMode(this);
@@ -383,15 +400,41 @@ export class Item implements ComponentInterface, AnchorInterface, ButtonInterfac
             rel,
             target,
           };
+
+    let clickFn = {};
+
+    const firstInteractive = this.getFirstInteractive();
+
     // Only set onClick if the item is clickable to prevent screen
     // readers from reading all items as clickable
-    const clickFn = clickable
-      ? {
-          onClick: (ev: Event) => {
+    if (clickable || (firstInteractive !== undefined && !multipleInputs)) {
+      clickFn = {
+        onClick: (ev: MouseEvent) => {
+          if (clickable) {
             openURL(href, ev, routerDirection, routerAnimation);
-          },
-        }
-      : {};
+          }
+          if (firstInteractive !== undefined && !multipleInputs) {
+            const path = ev.composedPath();
+            const target = path[0] as HTMLElement;
+            if (ev.isTrusted) {
+              /**
+               * Dispatches a click event to the first interactive element,
+               * when it is the result of a user clicking on the item.
+               *
+               * We check if the click target is in the shadow root,
+               * which means the user clicked on the .item-native or
+               * .item-inner padding.
+               */
+              const clickedWithinShadowRoot = this.el.shadowRoot!.contains(target);
+              if (clickedWithinShadowRoot) {
+                firstInteractive.click();
+              }
+            }
+          }
+        },
+      };
+    }
+
     const showDetail = detail !== undefined ? detail : mode === 'ios' && clickable;
     this.itemStyles.forEach((value) => {
       Object.assign(childStyles, value);
@@ -413,6 +456,7 @@ export class Item implements ComponentInterface, AnchorInterface, ButtonInterfac
             [`item-lines-${lines}`]: lines !== undefined,
             [`item-fill-${fillValue}`]: true,
             [`item-shape-${shape}`]: shape !== undefined,
+            'item-has-interactive-control': firstInteractive !== undefined,
             'item-disabled': disabled,
             'in-list': inList,
             'item-multiple-inputs': this.multipleInputs,
diff --git a/core/src/components/radio/test/item/radio.e2e.ts b/core/src/components/radio/test/item/radio.e2e.ts
index e83e755dab3..4fdf34bd4e1 100644
--- a/core/src/components/radio/test/item/radio.e2e.ts
+++ b/core/src/components/radio/test/item/radio.e2e.ts
@@ -78,4 +78,33 @@ configs({ directions: ['ltr'], modes: ['md'] }).forEach(({ title, screenshot, co
       await expect(list).toHaveScreenshot(screenshot(`radio-stacked-label-in-item`));
     });
   });
+
+  test.describe(title('radio: ionChange'), () => {
+    test('clicking padded space within item should click the radio', async ({ page }) => {
+      await page.setContent(
+        `
+        <ion-radio-group>
+          <ion-item>
+            <ion-radio>
+              <ion-label>Enable Notifications</ion-label>
+            </ion-radio>
+          </ion-item>
+        </ion-radio-group>
+      `,
+        config
+      );
+      const itemNative = page.locator('.item-native');
+      const ionChange = await page.spyOnEvent('ionChange');
+
+      // Clicks the padded space within the item
+      await itemNative.click({
+        position: {
+          x: 5,
+          y: 5,
+        },
+      });
+
+      expect(ionChange).toHaveReceivedEvent();
+    });
+  });
 });
diff --git a/core/src/components/select/test/basic/select.e2e.ts b/core/src/components/select/test/basic/select.e2e.ts
index f288df62b09..44dd7dba25e 100644
--- a/core/src/components/select/test/basic/select.e2e.ts
+++ b/core/src/components/select/test/basic/select.e2e.ts
@@ -234,5 +234,33 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => {
       await select.evaluate((el: HTMLIonSelectElement) => (el.value = 'banana'));
       await expect(ionChange).not.toHaveReceivedEvent();
     });
+
+    test('clicking padded space within item should click the select', async ({ page }) => {
+      await page.setContent(
+        `
+        <ion-item>
+          <ion-select label="Fruit" interface="action-sheet">
+            <ion-select-option value="apple">Apple</ion-select-option>
+            <ion-select-option value="banana">Banana</ion-select-option>
+          </ion-select>
+        </ion-item>
+      `,
+        config
+      );
+      const itemNative = page.locator('.item-native');
+      const ionActionSheetDidPresent = await page.spyOnEvent('ionActionSheetDidPresent');
+
+      // Clicks the padded space within the item
+      await itemNative.click({
+        position: {
+          x: 5,
+          y: 5,
+        },
+      });
+
+      await ionActionSheetDidPresent.next();
+
+      expect(ionActionSheetDidPresent).toHaveReceivedEvent();
+    });
   });
 });
diff --git a/core/src/components/toggle/test/item/toggle.e2e.ts b/core/src/components/toggle/test/item/toggle.e2e.ts
index 58d4802d94f..1dff98c31be 100644
--- a/core/src/components/toggle/test/item/toggle.e2e.ts
+++ b/core/src/components/toggle/test/item/toggle.e2e.ts
@@ -89,4 +89,38 @@ configs({ directions: ['ltr'], modes: ['md'] }).forEach(({ title, screenshot, co
       await expect(list).toHaveScreenshot(screenshot(`toggle-stacked-label-in-item`));
     });
   });
+
+  test.describe(title('toggle: ionChange'), () => {
+    test('clicking padded space within item should click the toggle', async ({ page }) => {
+      await page.setContent(
+        `
+        <ion-item>
+          <ion-toggle>Size</ion-toggle>
+        </ion-item>
+      `,
+        config
+      );
+      const itemNative = page.locator('.item-native');
+      const ionChange = await page.spyOnEvent('ionChange');
+
+      /**
+       * Clicks the padded space within the item.
+       *
+       * We intentionally activate the toggle control when clicking either
+       * the label or padded space. This is different than native iOS,
+       * but we do it for three reasons:
+       * 1. Clicking a label connected to a control is standard behavior for web controls.
+       * 2. iOS is inconsistent in their implementation and other controls can be activated by clicking the label.
+       * 3. MD is consistent in their implementation and activates controls by clicking the label.
+       */
+      await itemNative.click({
+        position: {
+          x: 5,
+          y: 5,
+        },
+      });
+
+      expect(ionChange).toHaveReceivedEvent();
+    });
+  });
 });
diff --git a/core/src/components/toggle/toggle.tsx b/core/src/components/toggle/toggle.tsx
index 934133cb8b4..8ccfd9975f1 100644
--- a/core/src/components/toggle/toggle.tsx
+++ b/core/src/components/toggle/toggle.tsx
@@ -258,7 +258,7 @@ export class Toggle implements ComponentInterface {
     }
   }
 
-  private onClick = (ev: Event) => {
+  private onClick = (ev: MouseEvent) => {
     ev.preventDefault();
 
     if (this.lastDrag + 300 < Date.now()) {