feat(accordion): ionChange will only emit from user committed changes (…

…#25922) BREAKING CHANGE: `ionChange` is no longer emitted when the `value` of `ion-accordion-group` is modified externally. `ionChange` is only emitted from user committed changes, such as clicking or tapping the accordion header.
ionic-team · Sep 13, 2022 · 4eea9fa · 4eea9fa
1 parent 89b880e
commit 4eea9fa
Show file tree

Hide file tree

Showing 7 changed files with 154 additions and 13 deletions.
diff --git a/BREAKING.md b/BREAKING.md
@@ -14,6 +14,7 @@ This is a comprehensive list of the breaking changes introduced in the major ver
 
 - [Browser and Platform Support](#version-7x-browser-platform-support)
 - [Components](#version-7x-components)
+  - [Accordion Group](#version-7x-accordion-group)
   - [Input](#version-7x-input)
   - [Overlays](#version-7x-overlays)
   - [Range](#version-7x-range)
@@ -51,6 +52,10 @@ This section details the desktop browser, JavaScript framework, and mobile platf
 
 <h2 id="version-7x-components">Components</h2>
 
+<h4 id="version-7x-accordion-group">Accordion Group</h4>
+
+`ionChange` is no longer emitted when the `value` of `ion-accordion-group` is modified externally. `ionChange` is only emitted from user committed changes, such as clicking or tapping the accordion header.
+
 <h4 id="version-7x-input">Input</h4>
 
 `ionChange` is no longer emitted when the `value` of `ion-input` is modified externally. `ionChange` is only emitted from user committed changes, such as typing in the input and the input losing focus or from clicking the clear action within the input.

diff --git a/angular/src/directives/proxies.ts b/angular/src/directives/proxies.ts
@@ -31,7 +31,10 @@ export class IonAccordion {
 import type { AccordionGroupChangeEventDetail as IAccordionGroupAccordionGroupChangeEventDetail } from '@ionic/core';
 export declare interface IonAccordionGroup extends Components.IonAccordionGroup {
   /**
-   * Emitted when the value property has changed. 
+   * Emitted when the value property has changed
+as a result of a user action such as a click.
+This event will not emit when programmatically setting
+the value property. 
    */
   ionChange: EventEmitter<CustomEvent<IAccordionGroupAccordionGroupChangeEventDetail>>;
 

diff --git a/core/src/components.d.ts b/core/src/components.d.ts
@@ -68,6 +68,9 @@ export namespace Components {
           * If `true`, the accordion group cannot be interacted with, but does not alter the opacity.
          */
         "readonly": boolean;
+        /**
+          * This method is used to ensure that the value of ion-accordion-group is being set in a valid way. This method should only be called in response to a user generated action.
+         */
         "requestAccordionToggle": (accordionValue: string | undefined, accordionExpand: boolean) => Promise<void>;
         /**
           * The value of the accordion group.
@@ -3804,9 +3807,13 @@ declare namespace LocalJSX {
          */
         "multiple"?: boolean;
         /**
-          * Emitted when the value property has changed.
+          * Emitted when the value property has changed as a result of a user action such as a click. This event will not emit when programmatically setting the value property.
          */
         "onIonChange"?: (event: IonAccordionGroupCustomEvent<AccordionGroupChangeEventDetail>) => void;
+        /**
+          * Emitted when the value property has changed. This is used to ensure that ion-accordion can respond to any value property changes.
+         */
+        "onIonValueChange"?: (event: IonAccordionGroupCustomEvent<AccordionGroupChangeEventDetail>) => void;
         /**
           * If `true`, the accordion group cannot be interacted with, but does not alter the opacity.
          */

diff --git a/core/src/components/accordion-group/accordion-group.tsx b/core/src/components/accordion-group/accordion-group.tsx
@@ -55,10 +55,21 @@ export class AccordionGroup implements ComponentInterface {
   @Prop() expand: 'compact' | 'inset' = 'compact';
 
   /**
-   * Emitted when the value property has changed.
+   * Emitted when the value property has changed
+   * as a result of a user action such as a click.
+   * This event will not emit when programmatically setting
+   * the value property.
    */
   @Event() ionChange!: EventEmitter<AccordionGroupChangeEventDetail>;
 
+  /**
+   * Emitted when the value property has changed.
+   * This is used to ensure that ion-accordion can respond
+   * to any value property changes.
+   * @internal
+   */
+  @Event() ionValueChange!: EventEmitter<AccordionGroupChangeEventDetail>;
+
   @Watch('value')
   valueChanged() {
     const { value, multiple } = this;
@@ -68,12 +79,18 @@ export class AccordionGroup implements ComponentInterface {
      * let multiple accordions be open
      * at once, but user passes an array
      * just grab the first value.
+     * This should emit ionChange because
+     * we are updating the value internally.
      */
     if (!multiple && Array.isArray(value)) {
-      this.value = value[0];
-    } else {
-      this.ionChange.emit({ value: this.value });
+      this.setValue(value[0]);
     }
+
+    /**
+     * Do not use `value` here as that will be
+     * not account for the adjustment we make above.
+     */
+    this.ionValueChange.emit({ value: this.value });
   }
 
   @Watch('disabled')
@@ -156,6 +173,23 @@ export class AccordionGroup implements ComponentInterface {
   }
 
   /**
+   * Sets the value property and emits ionChange.
+   * This should only be called when the user interacts
+   * with the accordion and not for any update
+   * to the value property. The exception is when
+   * the app sets the value of a single-select
+   * accordion group to an array.
+   */
+  private setValue(accordionValue: string | string[] | null | undefined) {
+    const value = (this.value = accordionValue);
+    this.ionChange.emit({ value });
+  }
+
+  /**
+   * This method is used to ensure that the value
+   * of ion-accordion-group is being set in a valid
+   * way. This method should only be called in
+   * response to a user generated action.
    * @internal
    */
   @Method()
@@ -177,10 +211,10 @@ export class AccordionGroup implements ComponentInterface {
         const processedValue = Array.isArray(groupValue) ? groupValue : [groupValue];
         const valueExists = processedValue.find((v) => v === accordionValue);
         if (valueExists === undefined && accordionValue !== undefined) {
-          this.value = [...processedValue, accordionValue];
+          this.setValue([...processedValue, accordionValue]);
         }
       } else {
-        this.value = accordionValue;
+        this.setValue(accordionValue);
       }
     } else {
       /**
@@ -190,9 +224,9 @@ export class AccordionGroup implements ComponentInterface {
       if (multiple) {
         const groupValue = value || [];
         const processedValue = Array.isArray(groupValue) ? groupValue : [groupValue];
-        this.value = processedValue.filter((v) => v !== accordionValue);
+        this.setValue(processedValue.filter((v) => v !== accordionValue));
       } else {
-        this.value = undefined;
+        this.setValue(undefined);
       }
     }
   }

diff --git a/core/src/components/accordion/accordion.tsx b/core/src/components/accordion/accordion.tsx
@@ -85,14 +85,14 @@ export class Accordion implements ComponentInterface {
     const accordionGroupEl = (this.accordionGroupEl = this.el?.closest('ion-accordion-group'));
     if (accordionGroupEl) {
       this.updateState(true);
-      addEventListener(accordionGroupEl, 'ionChange', this.updateListener);
+      addEventListener(accordionGroupEl, 'ionValueChange', this.updateListener);
     }
   }
 
   disconnectedCallback() {
     const accordionGroupEl = this.accordionGroupEl;
     if (accordionGroupEl) {
-      removeEventListener(accordionGroupEl, 'ionChange', this.updateListener);
+      removeEventListener(accordionGroupEl, 'ionValueChange', this.updateListener);
     }
   }
 

diff --git a/core/src/components/accordion/test/basic/accordion.e2e.ts b/core/src/components/accordion/test/basic/accordion.e2e.ts
@@ -10,3 +10,94 @@ test.describe('accordion: basic', () => {
     expect(await page.screenshot()).toMatchSnapshot(`accordion-basic-${page.getSnapshotSettings()}.png`);
   });
 });
+
+test.describe('accordion: ionChange', () => {
+  test.beforeEach(({ skip }) => {
+    skip.rtl();
+  });
+  test('should fire ionChange when interacting with accordions', async ({ page }) => {
+    await page.setContent(`
+      <ion-accordion-group value="second">
+        <ion-accordion value="first">
+          <button slot="header">Header</button>
+          <div slot="content">First Content</div>
+        </ion-accordion>
+        <ion-accordion value="second">
+          <button slot="header">Header</button>
+          <div slot="content">Second Content</div>
+        </ion-accordion>
+        <ion-accordion value="third">
+          <button slot="header">Header</button>
+          <div slot="content">Third Content</div>
+        </ion-accordion>
+      </ion-accordion-group>
+    `);
+
+    const ionChange = await page.spyOnEvent('ionChange');
+    const accordionHeaders = page.locator('button[slot="header"]');
+
+    await accordionHeaders.nth(0).click();
+    await expect(ionChange).toHaveReceivedEventDetail({ value: 'first' });
+
+    await accordionHeaders.nth(1).click();
+    await expect(ionChange).toHaveReceivedEventDetail({ value: 'second' });
+
+    await accordionHeaders.nth(1).click();
+    await expect(ionChange).toHaveReceivedEventDetail({ value: undefined });
+
+    await accordionHeaders.nth(2).click();
+    await expect(ionChange).toHaveReceivedEventDetail({ value: 'third' });
+  });
+
+  test('should not fire when programmatically setting a valid value', async ({ page }) => {
+    await page.setContent(`
+      <ion-accordion-group>
+        <ion-accordion value="first">
+          <button slot="header">Header</button>
+          <div slot="content">First Content</div>
+        </ion-accordion>
+        <ion-accordion value="second">
+          <button slot="header">Header</button>
+          <div slot="content">Second Content</div>
+        </ion-accordion>
+        <ion-accordion value="third">
+          <button slot="header">Header</button>
+          <div slot="content">Third Content</div>
+        </ion-accordion>
+      </ion-accordion-group>
+
+    `);
+
+    const ionChange = await page.spyOnEvent('ionChange');
+    const accordionGroup = page.locator('ion-accordion-group');
+
+    await accordionGroup.evaluate((el: HTMLIonAccordionGroupElement) => (el.value = 'second'));
+    await expect(ionChange).not.toHaveReceivedEvent();
+  });
+
+  test('should fire ionChange setting array of values on a single selection accordion', async ({ page }) => {
+    await page.setContent(`
+      <ion-accordion-group>
+        <ion-accordion value="first">
+          <button slot="header">Header</button>
+          <div slot="content">First Content</div>
+        </ion-accordion>
+        <ion-accordion value="second">
+          <button slot="header">Header</button>
+          <div slot="content">Second Content</div>
+        </ion-accordion>
+        <ion-accordion value="third">
+          <button slot="header">Header</button>
+          <div slot="content">Third Content</div>
+        </ion-accordion>
+      </ion-accordion-group>
+
+    `);
+
+    const ionChange = await page.spyOnEvent('ionChange');
+    const accordionGroup = page.locator('ion-accordion-group');
+
+    await accordionGroup.evaluate((el: HTMLIonAccordionGroupElement) => (el.value = ['second', 'third']));
+    await expect(ionChange).toHaveReceivedEventDetail({ value: 'second' });
+  });
+});
diff --git a/packages/vue/src/proxies.ts b/packages/vue/src/proxies.ts
@@ -92,7 +92,8 @@ export const IonAccordionGroup = /*@__PURE__*/ defineContainer<JSX.IonAccordionG
   'disabled',
   'readonly',
   'expand',
-  'ionChange'
+  'ionChange',
+  'ionValueChange'
 ],
 'value', 'v-ion-change', 'ionChange');