fix(select): emit single ionChange event for popover option selection

core/src/components/checkbox/checkbox.tsx

@@ -289,7 +289,9 @@ Example: <ion-checkbox>Label</ion-checkbox>
 
 For checkboxes that do not have a visible label, developers should use "aria-label" so screen readers can announce the purpose of the checkbox.
 
-For checkboxes that do not render the label immediately next to the checkbox, developers may continue to use "ion-label" but must manually associate the label with the checkbox by using "aria-labelledby".`,
+For checkboxes that do not render the label immediately next to the checkbox, developers may continue to use "ion-label" but must manually associate the label with the checkbox by using "aria-labelledby".
+
+Developers can use the "legacy" property to continue using the legacy form markup. This property will be removed in an upcoming major release of Ionic where this form control will use the modern form markup.`,
         this.el
       );
 

core/src/components/select-popover/select-popover.tsx

@@ -1,14 +1,14 @@
 import type { ComponentInterface } from '@stencil/core';
-import { Component, Host, Listen, Prop, h } from '@stencil/core';
+import { Element, Component, Host, Prop, h } from '@stencil/core';
 
 import { getIonMode } from '../../global/ionic-global';
 import { safeCall } from '../../utils/overlays';
 import { getClassMap } from '../../utils/theme';
+import type { CheckboxCustomEvent } from '../checkbox/checkbox-interface';
+import type { RadioGroupCustomEvent } from '../radio-group/radio-group-interface';
 
 import type { SelectPopoverOption } from './select-popover-interface';
 
-// TODO(FW-2832): types
-
 /**
  * @internal
  */
@@ -21,6 +21,7 @@ import type { SelectPopoverOption } from './select-popover-interface';
   scoped: true,
 })
 export class SelectPopover implements ComponentInterface {
+  @Element() el!: HTMLIonSelectPopoverElement;
   /**
    * The header text of the popover
    */
@@ -46,13 +47,7 @@ export class SelectPopover implements ComponentInterface {
    */
   @Prop() options: SelectPopoverOption[] = [];
 
-  @Listen('ionChange')
-  onSelect(ev: any) {
-    this.setChecked(ev);
-    this.callOptionHandler(ev);
-  }
-
-  private findOptionFromEvent(ev: any) {
+  private findOptionFromEvent(ev: CheckboxCustomEvent | RadioGroupCustomEvent) {
     const { options } = this;
     return options.find((o) => o.value === ev.target.value);
   }
@@ -62,7 +57,7 @@ export class SelectPopover implements ComponentInterface {
    * of the selected option(s) and return it in the option
    * handler
    */
-  private callOptionHandler(ev: any) {
+  private callOptionHandler(ev: CheckboxCustomEvent | RadioGroupCustomEvent) {
     const option = this.findOptionFromEvent(ev);
     const values = this.getValues(ev);
 
@@ -72,15 +67,17 @@ export class SelectPopover implements ComponentInterface {
   }
 
   /**
-   * This is required when selecting a radio that is already
-   * selected because it will not trigger the ionChange event
-   * but we still want to close the popover
+   * Dismisses the host popover that the `ion-select-popover`
+   * is rendered within.
    */
-  private rbClick(ev: any) {
-    this.callOptionHandler(ev);
+  private dismissParentPopover() {
+    const popover = this.el.closest('ion-popover');
+    if (popover) {
+      popover.dismiss();
+    }
   }
 
-  private setChecked(ev: any): void {
+  private setChecked(ev: CheckboxCustomEvent): void {
     const { multiple } = this;
     const option = this.findOptionFromEvent(ev);
 
@@ -91,7 +88,7 @@ export class SelectPopover implements ComponentInterface {
     }
   }
 
-  private getValues(ev: any): any | any[] | null {
+  private getValues(ev: CheckboxCustomEvent | RadioGroupCustomEvent): string | string[] | undefined {
     const { multiple, options } = this;
 
     if (multiple) {
@@ -125,6 +122,11 @@ export class SelectPopover implements ComponentInterface {
           value={option.value}
           disabled={option.disabled}
           checked={option.checked}
+          legacy={true}
+          onIonChange={(ev) => {
+            this.setChecked(ev);
+            this.callOptionHandler(ev);
+          }}
         ></ion-checkbox>
         <ion-label>{option.text}</ion-label>
       </ion-item>
@@ -135,11 +137,26 @@ export class SelectPopover implements ComponentInterface {
     const checked = options.filter((o) => o.checked).map((o) => o.value)[0];
 
     return (
-      <ion-radio-group value={checked}>
+      <ion-radio-group value={checked} onIonChange={(ev) => this.callOptionHandler(ev)}>
         {options.map((option) => (
           <ion-item class={getClassMap(option.cssClass)}>
             <ion-label>{option.text}</ion-label>
-            <ion-radio value={option.value} disabled={option.disabled} onClick={(ev) => this.rbClick(ev)}></ion-radio>
+            <ion-radio
+              value={option.value}
+              disabled={option.disabled}
+              legacy={true}
+              onClick={() => this.dismissParentPopover()}
+              onKeyUp={(ev) => {
+                if (ev.key === ' ') {
+                  /**
+                   * Selecting a radio option with keyboard navigation,
+                   * either through the Enter or Space keys, should
+                   * dismiss the popover.
+                   */
+                  this.dismissParentPopover();
+                }
+              }}
+            ></ion-radio>
           </ion-item>
         ))}
       </ion-radio-group>

core/src/components/select-popover/test/basic/index.html

@@ -0,0 +1,40 @@
+<!DOCTYPE html>
+<html lang="en" dir="ltr">
+  <head>
+    <meta charset="UTF-8" />
+    <title>Select - Popover</title>
+    <meta
+      name="viewport"
+      content="width=device-width, initial-scale=1.0, minimum-scale=1.0, maximum-scale=1.0, user-scalable=no"
+    />
+    <link href="../../../../../css/ionic.bundle.css" rel="stylesheet" />
+    <link href="../../../../../scripts/testing/styles.css" rel="stylesheet" />
+    <script src="../../../../../scripts/testing/scripts.js"></script>
+    <script nomodule src="../../../../../dist/ionic/ionic.js"></script>
+    <script type="module" src="../../../../../dist/ionic/ionic.esm.js"></script>
+  </head>
+
+  <body>
+    <ion-app>
+      <ion-header>
+        <ion-toolbar>
+          <ion-title>Select Popover - Basic</ion-title>
+        </ion-toolbar>
+      </ion-header>
+
+      <ion-content>
+        <ion-popover class="select-popover" is-open="true">
+          <ion-select-popover multiple="false"></ion-select-popover>
+        </ion-popover>
+      </ion-content>
+    </ion-app>
+
+    <script>
+      const selectPopover = document.querySelector('ion-select-popover');
+      selectPopover.options = [
+        { value: 'apple', text: 'Apple', disabled: false, checked: true },
+        { value: 'banana', text: 'Banana', disabled: false, checked: false },
+      ];
+    </script>
+  </body>
+</html>

core/src/components/select-popover/test/basic/select-popover.e2e.ts

@@ -0,0 +1,65 @@
+import { expect } from '@playwright/test';
+import { test } from '@utils/test/playwright';
+
+import type { SelectPopoverOption } from '../../select-popover-interface';
+import { SelectPopoverPage } from '../fixtures';
+
+const options: SelectPopoverOption[] = [
+  { value: 'apple', text: 'Apple', disabled: false, checked: false },
+  { value: 'banana', text: 'Banana', disabled: false, checked: false },
+];
+
+const checkedOptions: SelectPopoverOption[] = [
+  { value: 'apple', text: 'Apple', disabled: false, checked: true },
+  { value: 'banana', text: 'Banana', disabled: false, checked: false },
+];
+
+test.describe('select-popover: basic', () => {
+  test.beforeEach(({ skip, browserName }) => {
+    skip.rtl();
+    skip.mode('ios', 'Consistent behavior across modes');
+    test.skip(browserName === 'webkit', 'https://ionic-cloud.atlassian.net/browse/FW-2979');
+  });
+
+  test.describe('single selection', () => {
+    let selectPopoverPage: SelectPopoverPage;
+
+    test.beforeEach(async ({ page }) => {
+      selectPopoverPage = new SelectPopoverPage(page);
+    });
+
+    test('clicking an unselected option should dismiss the popover', async () => {
+      await selectPopoverPage.setup(options, false);
+
+      await selectPopoverPage.clickOption('apple');
+      await selectPopoverPage.ionPopoverDidDismiss.next();
+      await expect(selectPopoverPage.popover).not.toBeVisible();
+    });
+
+    test('clicking a selected option should dismiss the popover', async () => {
+      await selectPopoverPage.setup(checkedOptions, false);
+
+      await selectPopoverPage.clickOption('apple');
+      await selectPopoverPage.ionPopoverDidDismiss.next();
+      await expect(selectPopoverPage.popover).not.toBeVisible();
+    });
+
+    test('pressing Space on an unselected option should dismiss the popover', async () => {
+      await selectPopoverPage.setup(options, false);
+
+      await selectPopoverPage.pressSpaceOnOption('apple');
+      await selectPopoverPage.ionPopoverDidDismiss.next();
+      await expect(selectPopoverPage.popover).not.toBeVisible();
+    });
+
+    test('pressing Space on a selected option should dismiss the popover', async ({ browserName }) => {
+      test.skip(browserName === 'firefox', 'Same behavior as https://ionic-cloud.atlassian.net/browse/FW-2979');
+
+      await selectPopoverPage.setup(checkedOptions, false);
+
+      await selectPopoverPage.pressSpaceOnOption('apple');
+      await selectPopoverPage.ionPopoverDidDismiss.next();
+      await expect(selectPopoverPage.popover).not.toBeVisible();
+    });
+  });
+});

core/src/components/select-popover/test/fixtures.ts

@@ -0,0 +1,65 @@
+import type { E2EPage, E2ELocator, EventSpy } from '@utils/test/playwright';
+
+import type { SelectPopoverOption } from '../select-popover-interface';
+
+export class SelectPopoverPage {
+  private page: E2EPage;
+  private multiple?: boolean;
+  private options: SelectPopoverOption[] = [];
+
+  // Locators
+  popover!: E2ELocator;
+  selectPopover!: E2ELocator;
+
+  // Event spies
+  ionPopoverDidDismiss!: EventSpy;
+
+  constructor(page: E2EPage) {
+    this.page = page;
+  }
+
+  async setup(options: SelectPopoverOption[], multiple = false) {
+    const { page } = this;
+
+    await page.setContent(`
+    <ion-popover>
+      <ion-select-popover></ion-select-popover>
+    </ion-popover>
+    <script>
+      const selectPopover = document.querySelector('ion-select-popover');
+      selectPopover.options = ${JSON.stringify(options)};
+      selectPopover.multiple = ${multiple};
+    </script>
+    `);
+
+    const ionPopoverDidPresent = await page.spyOnEvent('ionPopoverDidPresent');
+    this.ionPopoverDidDismiss = await page.spyOnEvent('ionPopoverDidDismiss');
+
+    this.popover = page.locator('ion-popover');
+    this.selectPopover = page.locator('ion-select-popover');
+    this.multiple = multiple;
+    this.options = options;
+
+    await this.popover.evaluate((popover: HTMLIonPopoverElement) => popover.present());
+
+    await ionPopoverDidPresent.next();
+  }
+
+  async clickOption(value: string) {
+    const option = this.getOption(value);
+    await option.click();
+  }
+
+  async pressSpaceOnOption(value: string) {
+    const option = this.getOption(value);
+    await option.press('Space');
+  }
+
+  private getOption(value: string) {
+    const { multiple, selectPopover } = this;
+    const selector = multiple ? 'ion-checkbox' : 'ion-radio';
+    const index = this.options.findIndex((o) => o.value === value);
+
+    return selectPopover.locator(selector).nth(index);
+  }
+}

core/src/components/select/test/basic/select.e2e.ts

@@ -1,5 +1,6 @@
 import { expect } from '@playwright/test';
 import { test } from '@utils/test/playwright';
+import type { E2ELocator } from '@utils/test/playwright';
 
 test.describe('select: basic', () => {
   test.beforeEach(async ({ page }) => {
@@ -130,6 +131,7 @@ test.describe('select: ionChange', () => {
 
     await ionChange.next();
     expect(ionChange).toHaveReceivedEventDetail({ value: 'apple' });
+    expect(ionChange).toHaveReceivedEventTimes(1);
   });
 
   test('should fire ionChange when confirming a value from a popover', async ({ page }) => {
@@ -141,8 +143,8 @@ test.describe('select: ionChange', () => {
     `);
 
     const ionPopoverDidPresent = await page.spyOnEvent('ionPopoverDidPresent');
-    const ionChange = await page.spyOnEvent('ionChange');
-    const select = page.locator('ion-select');
+    const select = page.locator('ion-select') as E2ELocator;
+    const ionChange = await select.spyOnEvent('ionChange');
 
     await select.click();
     await ionPopoverDidPresent.next();
@@ -153,7 +155,39 @@ test.describe('select: ionChange', () => {
     await radioButtons.nth(0).click();
 
     await ionChange.next();
-    expect(ionChange).toHaveReceivedEventDetail({ value: 'apple', event: { isTrusted: true } });
+    expect(ionChange).toHaveReceivedEventDetail({ value: 'apple' });
+    expect(ionChange).toHaveReceivedEventTimes(1);
+  });
+
+  test('should fire ionChange when confirming multiple values from a popover', async ({ page }) => {
+    await page.setContent(`
+      <ion-select aria-label="Fruit" interface="popover" multiple="true">
+        <ion-select-option value="apple">Apple</ion-select-option>
+        <ion-select-option value="banana">Banana</ion-select-option>
+      </ion-select>
+    `);
+
+    const ionPopoverDidPresent = await page.spyOnEvent('ionPopoverDidPresent');
+    const select = page.locator('ion-select') as E2ELocator;
+    const ionChange = await select.spyOnEvent('ionChange');
+
+    await select.click();
+    await ionPopoverDidPresent.next();
+
+    const popover = page.locator('ion-popover');
+    const checkboxes = popover.locator('ion-checkbox');
+
+    await checkboxes.nth(0).click();
+    await ionChange.next();
+
+    expect(ionChange).toHaveReceivedEventDetail({ value: ['apple'] });
+    expect(ionChange).toHaveReceivedEventTimes(1);
+
+    await checkboxes.nth(1).click();
+    await ionChange.next();
+
+    expect(ionChange).toHaveReceivedEventDetail({ value: ['apple', 'banana'] });
+    expect(ionChange).toHaveReceivedEventTimes(2);
   });
 
   test('should fire ionChange when confirming a value from an action sheet', async ({ page }) => {
@@ -178,6 +212,7 @@ test.describe('select: ionChange', () => {
 
     await ionChange.next();
     expect(ionChange).toHaveReceivedEventDetail({ value: 'apple' });
+    expect(ionChange).toHaveReceivedEventTimes(1);
   });
 
   test('should not fire when programmatically setting a valid value', async ({ page }) => {

core/src/utils/test/playwright/index.ts

@@ -1,6 +1,7 @@
 export * from './playwright-page';
 export * from './playwright-declarations';
 export * from './page/event-spy';
+export * from './page/utils';
 export * from './drag-element';
 export * from './matchers';
 export * from './viewports';