From 58c2a8accb62994f2bcd01986f5dfe9694db0e45 Mon Sep 17 00:00:00 2001 From: ShaneK Date: Thu, 16 Oct 2025 12:54:34 +0100 Subject: [PATCH 1/9] fix(accordion): skip initial animation --- .../accordion-group-interface.ts | 1 + .../accordion-group/accordion-group.tsx | 61 ++++++++++--------- core/src/components/accordion/accordion.tsx | 6 +- .../accordion/test/accordion.spec.ts | 36 +++++++++++ 4 files changed, 74 insertions(+), 30 deletions(-) diff --git a/core/src/components/accordion-group/accordion-group-interface.ts b/core/src/components/accordion-group/accordion-group-interface.ts index 7daae2982fd..4498ec42a6f 100644 --- a/core/src/components/accordion-group/accordion-group-interface.ts +++ b/core/src/components/accordion-group/accordion-group-interface.ts @@ -1,5 +1,6 @@ export interface AccordionGroupChangeEventDetail { value: T; + initial?: boolean; } export interface AccordionGroupCustomEvent extends CustomEvent { diff --git a/core/src/components/accordion-group/accordion-group.tsx b/core/src/components/accordion-group/accordion-group.tsx index ba5b110c8b6..00d40d33fc9 100644 --- a/core/src/components/accordion-group/accordion-group.tsx +++ b/core/src/components/accordion-group/accordion-group.tsx @@ -75,31 +75,7 @@ export class AccordionGroup implements ComponentInterface { @Watch('value') valueChanged() { - const { value, multiple } = this; - - if (!multiple && Array.isArray(value)) { - /** - * We do some processing on the `value` array so - * that it looks more like an array when logged to - * the console. - * Example given ['a', 'b'] - * Default toString() behavior: a,b - * Custom behavior: ['a', 'b'] - */ - printIonWarning( - `[ion-accordion-group] - An array of values was passed, but multiple is "false". This is incorrect usage and may result in unexpected behaviors. To dismiss this warning, pass a string to the "value" property when multiple="false". - - Value Passed: [${value.map((v) => `'${v}'`).join(', ')}] -`, - this.el - ); - } - - /** - * Do not use `value` here as that will be - * not account for the adjustment we make above. - */ - this.ionValueChange.emit({ value: this.value }); + this.emitValueChange(false); } @Watch('disabled') @@ -184,11 +160,10 @@ export class AccordionGroup implements ComponentInterface { * it is possible for the value to be set after the Web Component * initializes but before the value watcher is set up in Stencil. * As a result, the watcher callback may not be fired. - * We work around this by manually calling the watcher - * callback when the component has loaded and the watcher - * is configured. + * We work around this by manually emitting a value change when the component + * has loaded and the watcher is configured. */ - this.valueChanged(); + this.emitValueChange(true); } /** @@ -276,6 +251,34 @@ export class AccordionGroup implements ComponentInterface { return Array.from(this.el.querySelectorAll(':scope > ion-accordion')) as HTMLIonAccordionElement[]; } + private emitValueChange(initial: boolean) { + const { value, multiple } = this; + + if (!multiple && Array.isArray(value)) { + /** + * We do some processing on the `value` array so + * that it looks more like an array when logged to + * the console. + * Example given ['a', 'b'] + * Default toString() behavior: a,b + * Custom behavior: ['a', 'b'] + */ + printIonWarning( + `[ion-accordion-group] - An array of values was passed, but multiple is "false". This is incorrect usage and may result in unexpected behaviors. To dismiss this warning, pass a string to the "value" property when multiple="false". + + Value Passed: [${value.map((v) => `'${v}'`).join(', ')}] +`, + this.el + ); + } + + /** + * Do not use `value` here as that will not account + * for the adjustment we make above. + */ + this.ionValueChange.emit({ value: this.value, initial }); + } + render() { const { disabled, readonly, expand } = this; const mode = getIonMode(this); diff --git a/core/src/components/accordion/accordion.tsx b/core/src/components/accordion/accordion.tsx index 92d28848d20..ed0408e2126 100644 --- a/core/src/components/accordion/accordion.tsx +++ b/core/src/components/accordion/accordion.tsx @@ -5,6 +5,7 @@ import { chevronDown } from 'ionicons/icons'; import { config } from '../../global/config'; import { getIonMode } from '../../global/ionic-global'; +import type { AccordionGroupChangeEventDetail } from '../accordion-group/accordion-group-interface'; const enum AccordionState { Collapsed = 1 << 0, @@ -38,7 +39,10 @@ const enum AccordionState { }) export class Accordion implements ComponentInterface { private accordionGroupEl?: HTMLIonAccordionGroupElement | null; - private updateListener = () => this.updateState(false); + private updateListener = (ev: CustomEvent) => { + const initialUpdate = ev.detail?.initial ?? false; + this.updateState(initialUpdate); + }; private contentEl: HTMLDivElement | undefined; private contentElWrapper: HTMLDivElement | undefined; private headerEl: HTMLDivElement | undefined; diff --git a/core/src/components/accordion/test/accordion.spec.ts b/core/src/components/accordion/test/accordion.spec.ts index 54883002dbf..e37b62a9bff 100644 --- a/core/src/components/accordion/test/accordion.spec.ts +++ b/core/src/components/accordion/test/accordion.spec.ts @@ -1,5 +1,6 @@ import { newSpecPage } from '@stencil/core/testing'; +import type { AccordionGroupChangeEventDetail } from '../../accordion-group/accordion-group-interface'; import { AccordionGroup } from '../../accordion-group/accordion-group'; import { Item } from '../../item/item'; import { Accordion } from '../accordion'; @@ -200,6 +201,41 @@ it('should set default values if not provided', async () => { expect(accordion.classList.contains('accordion-collapsed')).toEqual(false); }); +it('should not animate when initial value is set before load', async () => { + const page = await newSpecPage({ + components: [Item, Accordion, AccordionGroup], + }); + + const accordionGroup = page.doc.createElement('ion-accordion-group'); + accordionGroup.innerHTML = ` + + Label +
Content
+
+ + Label +
Content
+
+ `; + + const details: AccordionGroupChangeEventDetail[] = []; + accordionGroup.addEventListener('ionValueChange', (event: CustomEvent) => { + details.push(event.detail); + }); + + accordionGroup.value = 'first'; + page.body.appendChild(accordionGroup); + + await page.waitForChanges(); + + expect(details[0]?.initial).toBe(true); + + const firstAccordion = accordionGroup.querySelector('ion-accordion[value="first"]')!; + + expect(firstAccordion.classList.contains('accordion-expanded')).toEqual(true); + expect(firstAccordion.classList.contains('accordion-expanding')).toEqual(false); +}); + // Verifies fix for https://github.com/ionic-team/ionic-framework/issues/27047 it('should not have animated class when animated="false"', async () => { const page = await newSpecPage({ From 761d06f1682ae9f3eebec0523c971898e7e519f4 Mon Sep 17 00:00:00 2001 From: ShaneK Date: Thu, 16 Oct 2025 13:29:58 +0100 Subject: [PATCH 2/9] fix(accordion-group): fixing animation in react --- .../accordion-group/accordion-group.tsx | 18 +++++++-- .../accordion/test/accordion.spec.ts | 37 +++++++++++++++++++ 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/core/src/components/accordion-group/accordion-group.tsx b/core/src/components/accordion-group/accordion-group.tsx index 00d40d33fc9..7c4e780d06f 100644 --- a/core/src/components/accordion-group/accordion-group.tsx +++ b/core/src/components/accordion-group/accordion-group.tsx @@ -73,6 +73,8 @@ export class AccordionGroup implements ComponentInterface { */ @Event() ionValueChange!: EventEmitter; + private hasEmittedInitialValue = false; + @Watch('value') valueChanged() { this.emitValueChange(false); @@ -163,7 +165,9 @@ export class AccordionGroup implements ComponentInterface { * We work around this by manually emitting a value change when the component * has loaded and the watcher is configured. */ - this.emitValueChange(true); + if (!this.hasEmittedInitialValue) { + this.emitValueChange(true); + } } /** @@ -273,10 +277,16 @@ export class AccordionGroup implements ComponentInterface { } /** - * Do not use `value` here as that will not account - * for the adjustment we make above. + * Track if this is the initial value update so accordions + * can skip transition animations when they first render. */ - this.ionValueChange.emit({ value: this.value, initial }); + const shouldMarkInitial = initial || (!this.hasEmittedInitialValue && value !== undefined); + + this.ionValueChange.emit({ value, initial: shouldMarkInitial }); + + if (value !== undefined) { + this.hasEmittedInitialValue = true; + } } render() { diff --git a/core/src/components/accordion/test/accordion.spec.ts b/core/src/components/accordion/test/accordion.spec.ts index e37b62a9bff..a132772e027 100644 --- a/core/src/components/accordion/test/accordion.spec.ts +++ b/core/src/components/accordion/test/accordion.spec.ts @@ -236,6 +236,43 @@ it('should not animate when initial value is set before load', async () => { expect(firstAccordion.classList.contains('accordion-expanding')).toEqual(false); }); +it('should not animate when initial value is set after load', async () => { + const page = await newSpecPage({ + components: [Item, Accordion, AccordionGroup], + }); + + const accordionGroup = page.doc.createElement('ion-accordion-group'); + accordionGroup.innerHTML = ` + + Label +
Content
+
+ + Label +
Content
+
+ `; + + const details: AccordionGroupChangeEventDetail[] = []; + accordionGroup.addEventListener('ionValueChange', (event: CustomEvent) => { + details.push(event.detail); + }); + + page.body.appendChild(accordionGroup); + await page.waitForChanges(); + + accordionGroup.value = 'first'; + await page.waitForChanges(); + + const firstDetail = details.find((detail) => detail.value === 'first'); + expect(firstDetail?.initial).toBe(true); + + const firstAccordion = accordionGroup.querySelector('ion-accordion[value="first"]')!; + + expect(firstAccordion.classList.contains('accordion-expanded')).toEqual(true); + expect(firstAccordion.classList.contains('accordion-expanding')).toEqual(false); +}); + // Verifies fix for https://github.com/ionic-team/ionic-framework/issues/27047 it('should not have animated class when animated="false"', async () => { const page = await newSpecPage({ From 2095f2f1c5d14ff910d36cf838fcb905d3f758c6 Mon Sep 17 00:00:00 2001 From: ShaneK Date: Mon, 20 Oct 2025 08:54:43 -0700 Subject: [PATCH 3/9] fix(accordion): animating open if the first opening is done by the user --- .../accordion-group/accordion-group.tsx | 9 ++++--- .../accordion/test/accordion.spec.ts | 27 +++++++++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/core/src/components/accordion-group/accordion-group.tsx b/core/src/components/accordion-group/accordion-group.tsx index 7c4e780d06f..ebe0e61a662 100644 --- a/core/src/components/accordion-group/accordion-group.tsx +++ b/core/src/components/accordion-group/accordion-group.tsx @@ -74,10 +74,12 @@ export class AccordionGroup implements ComponentInterface { @Event() ionValueChange!: EventEmitter; private hasEmittedInitialValue = false; + private isUserInitiatedChange = false; @Watch('value') valueChanged() { - this.emitValueChange(false); + this.emitValueChange(false, this.isUserInitiatedChange); + this.isUserInitiatedChange = false; } @Watch('disabled') @@ -179,6 +181,7 @@ export class AccordionGroup implements ComponentInterface { * accordion group to an array. */ private setValue(accordionValue: string | string[] | null | undefined) { + this.isUserInitiatedChange = true; const value = (this.value = accordionValue); this.ionChange.emit({ value }); } @@ -255,7 +258,7 @@ export class AccordionGroup implements ComponentInterface { return Array.from(this.el.querySelectorAll(':scope > ion-accordion')) as HTMLIonAccordionElement[]; } - private emitValueChange(initial: boolean) { + private emitValueChange(initial: boolean, isUserInitiated = false) { const { value, multiple } = this; if (!multiple && Array.isArray(value)) { @@ -280,7 +283,7 @@ export class AccordionGroup implements ComponentInterface { * Track if this is the initial value update so accordions * can skip transition animations when they first render. */ - const shouldMarkInitial = initial || (!this.hasEmittedInitialValue && value !== undefined); + const shouldMarkInitial = initial || (!this.hasEmittedInitialValue && value !== undefined && !isUserInitiated); this.ionValueChange.emit({ value, initial: shouldMarkInitial }); diff --git a/core/src/components/accordion/test/accordion.spec.ts b/core/src/components/accordion/test/accordion.spec.ts index a132772e027..436e03b7fb8 100644 --- a/core/src/components/accordion/test/accordion.spec.ts +++ b/core/src/components/accordion/test/accordion.spec.ts @@ -273,6 +273,33 @@ it('should not animate when initial value is set after load', async () => { expect(firstAccordion.classList.contains('accordion-expanding')).toEqual(false); }); +it('should animate when accordion is first opened by user', async () => { + const page = await newSpecPage({ + components: [Item, Accordion, AccordionGroup], + html: ` + + + Label +
Content
+
+
+ `, + }); + + const accordionGroup = page.body.querySelector('ion-accordion-group')!; + + const details: AccordionGroupChangeEventDetail[] = []; + accordionGroup.addEventListener('ionValueChange', (event: CustomEvent) => { + details.push(event.detail); + }); + + await accordionGroup.requestAccordionToggle('first', true); + await page.waitForChanges(); + + const lastDetail = details[details.length - 1]; + expect(lastDetail?.initial).toBe(false); +}); + // Verifies fix for https://github.com/ionic-team/ionic-framework/issues/27047 it('should not have animated class when animated="false"', async () => { const page = await newSpecPage({ From 63f32a53df88216db04f648f96a75081143dc014 Mon Sep 17 00:00:00 2001 From: ShaneK Date: Tue, 21 Oct 2025 08:34:57 -0700 Subject: [PATCH 4/9] fix(accordion-group): skipping initial arrow animation --- core/src/components/accordion/accordion.tsx | 18 ++++++++++++++++++ .../accordion/test/accordion.spec.ts | 5 +++++ 2 files changed, 23 insertions(+) diff --git a/core/src/components/accordion/accordion.tsx b/core/src/components/accordion/accordion.tsx index ed0408e2126..d7a5ea1d9d5 100644 --- a/core/src/components/accordion/accordion.tsx +++ b/core/src/components/accordion/accordion.tsx @@ -48,6 +48,7 @@ export class Accordion implements ComponentInterface { private headerEl: HTMLDivElement | undefined; private currentRaf: number | undefined; + private skipNextAnimation = false; @Element() el?: HTMLElement; @@ -295,6 +296,10 @@ export class Accordion implements ComponentInterface { * of what is set in the config. */ private shouldAnimate = () => { + if (this.skipNextAnimation) { + return false; + } + if (typeof (window as any) === 'undefined') { return false; } @@ -316,6 +321,14 @@ export class Accordion implements ComponentInterface { return true; }; + private disableAnimationTemporarily() { + this.skipNextAnimation = true; + } + + componentDidRender() { + this.skipNextAnimation = false; + } + private updateState = async (initialUpdate = false) => { const accordionGroup = this.accordionGroupEl; const accordionValue = this.value; @@ -327,6 +340,11 @@ export class Accordion implements ComponentInterface { const value = accordionGroup.value; const shouldExpand = Array.isArray(value) ? value.includes(accordionValue) : value === accordionValue; + const shouldDisableAnimation = initialUpdate && shouldExpand; + + if (shouldDisableAnimation) { + this.disableAnimationTemporarily(); + } if (shouldExpand) { this.expandAccordion(initialUpdate); diff --git a/core/src/components/accordion/test/accordion.spec.ts b/core/src/components/accordion/test/accordion.spec.ts index 436e03b7fb8..9350c91dfce 100644 --- a/core/src/components/accordion/test/accordion.spec.ts +++ b/core/src/components/accordion/test/accordion.spec.ts @@ -234,6 +234,7 @@ it('should not animate when initial value is set before load', async () => { expect(firstAccordion.classList.contains('accordion-expanded')).toEqual(true); expect(firstAccordion.classList.contains('accordion-expanding')).toEqual(false); + expect(firstAccordion.classList.contains('accordion-animated')).toEqual(false); }); it('should not animate when initial value is set after load', async () => { @@ -271,6 +272,7 @@ it('should not animate when initial value is set after load', async () => { expect(firstAccordion.classList.contains('accordion-expanded')).toEqual(true); expect(firstAccordion.classList.contains('accordion-expanding')).toEqual(false); + expect(firstAccordion.classList.contains('accordion-animated')).toEqual(false); }); it('should animate when accordion is first opened by user', async () => { @@ -298,6 +300,9 @@ it('should animate when accordion is first opened by user', async () => { const lastDetail = details[details.length - 1]; expect(lastDetail?.initial).toBe(false); + + const firstAccordion = accordionGroup.querySelector('ion-accordion[value="first"]')!; + expect(firstAccordion.classList.contains('accordion-animated')).toEqual(true); }); // Verifies fix for https://github.com/ionic-team/ionic-framework/issues/27047 From cc125e6a7e7052bb825cf5d2051517e9bc4c83f4 Mon Sep 17 00:00:00 2001 From: ShaneK Date: Tue, 21 Oct 2025 10:54:14 -0700 Subject: [PATCH 5/9] chore(accordion): adding comment explaining when we want to skip the next animation --- core/src/components/accordion/accordion.tsx | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/core/src/components/accordion/accordion.tsx b/core/src/components/accordion/accordion.tsx index d7a5ea1d9d5..328ed845bf2 100644 --- a/core/src/components/accordion/accordion.tsx +++ b/core/src/components/accordion/accordion.tsx @@ -48,6 +48,13 @@ export class Accordion implements ComponentInterface { private headerEl: HTMLDivElement | undefined; private currentRaf: number | undefined; + /** + * If true, the next animation will be skipped. + * We want to skip the animation when the accordion + * is expanded or collapsed on the initial load. + * This prevents the accordion from animating when + * it starts expanded or collapsed. + */ private skipNextAnimation = false; @Element() el?: HTMLElement; @@ -124,6 +131,10 @@ export class Accordion implements ComponentInterface { }); } + componentDidRender() { + this.skipNextAnimation = false; + } + private setItemDefaults = () => { const ionItem = this.getSlottedHeaderIonItem(); if (!ionItem) { @@ -321,14 +332,6 @@ export class Accordion implements ComponentInterface { return true; }; - private disableAnimationTemporarily() { - this.skipNextAnimation = true; - } - - componentDidRender() { - this.skipNextAnimation = false; - } - private updateState = async (initialUpdate = false) => { const accordionGroup = this.accordionGroupEl; const accordionValue = this.value; @@ -343,7 +346,7 @@ export class Accordion implements ComponentInterface { const shouldDisableAnimation = initialUpdate && shouldExpand; if (shouldDisableAnimation) { - this.disableAnimationTemporarily(); + this.skipNextAnimation = true; } if (shouldExpand) { From 90ebfba6d7079ddfc91b2985a41c094832374471 Mon Sep 17 00:00:00 2001 From: ShaneK Date: Wed, 22 Oct 2025 06:46:57 -0700 Subject: [PATCH 6/9] fix(accordion): re-rendering after intitial load to reset animation skip state and allow animations on first interaction --- core/src/components/accordion/accordion.tsx | 12 ++++++++++-- core/src/components/accordion/test/accordion.spec.ts | 2 -- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/core/src/components/accordion/accordion.tsx b/core/src/components/accordion/accordion.tsx index 328ed845bf2..3e6c835eb75 100644 --- a/core/src/components/accordion/accordion.tsx +++ b/core/src/components/accordion/accordion.tsx @@ -1,5 +1,5 @@ import type { ComponentInterface } from '@stencil/core'; -import { Component, Element, Host, Prop, State, Watch, h } from '@stencil/core'; +import { Component, Element, Host, Prop, State, Watch, forceUpdate, h } from '@stencil/core'; import { addEventListener, getElementRoot, raf, removeEventListener, transitionEndAsync } from '@utils/helpers'; import { chevronDown } from 'ionicons/icons'; @@ -132,7 +132,15 @@ export class Accordion implements ComponentInterface { } componentDidRender() { - this.skipNextAnimation = false; + if (this.skipNextAnimation) { + this.skipNextAnimation = false; + /** + * The initial render disables animations so framework-provided + * values do not cause the accordion to animate. Force a repaint + * so subsequent user interactions animate as expected. + */ + forceUpdate(this); + } } private setItemDefaults = () => { diff --git a/core/src/components/accordion/test/accordion.spec.ts b/core/src/components/accordion/test/accordion.spec.ts index 9350c91dfce..033ee00f8fb 100644 --- a/core/src/components/accordion/test/accordion.spec.ts +++ b/core/src/components/accordion/test/accordion.spec.ts @@ -234,7 +234,6 @@ it('should not animate when initial value is set before load', async () => { expect(firstAccordion.classList.contains('accordion-expanded')).toEqual(true); expect(firstAccordion.classList.contains('accordion-expanding')).toEqual(false); - expect(firstAccordion.classList.contains('accordion-animated')).toEqual(false); }); it('should not animate when initial value is set after load', async () => { @@ -272,7 +271,6 @@ it('should not animate when initial value is set after load', async () => { expect(firstAccordion.classList.contains('accordion-expanded')).toEqual(true); expect(firstAccordion.classList.contains('accordion-expanding')).toEqual(false); - expect(firstAccordion.classList.contains('accordion-animated')).toEqual(false); }); it('should animate when accordion is first opened by user', async () => { From e345efd50a615fe750d7c76f334cc8777e6e731f Mon Sep 17 00:00:00 2001 From: ShaneK Date: Tue, 28 Oct 2025 11:55:14 -0700 Subject: [PATCH 7/9] fix(accordion): defaulting to skip initial animation until first render --- core/src/components/accordion/accordion.tsx | 2 +- packages/react/test/base/src/App.tsx | 2 + .../test/base/src/pages/AccordionGroup.tsx | 54 +++++++++++++++++++ packages/react/test/base/src/pages/Main.tsx | 3 ++ 4 files changed, 60 insertions(+), 1 deletion(-) create mode 100644 packages/react/test/base/src/pages/AccordionGroup.tsx diff --git a/core/src/components/accordion/accordion.tsx b/core/src/components/accordion/accordion.tsx index 3e6c835eb75..179a94eed1c 100644 --- a/core/src/components/accordion/accordion.tsx +++ b/core/src/components/accordion/accordion.tsx @@ -55,7 +55,7 @@ export class Accordion implements ComponentInterface { * This prevents the accordion from animating when * it starts expanded or collapsed. */ - private skipNextAnimation = false; + private skipNextAnimation = true; @Element() el?: HTMLElement; diff --git a/packages/react/test/base/src/App.tsx b/packages/react/test/base/src/App.tsx index 2f7f4a63ded..634af89f075 100644 --- a/packages/react/test/base/src/App.tsx +++ b/packages/react/test/base/src/App.tsx @@ -37,6 +37,7 @@ import KeepContentsMounted from './pages/overlay-components/KeepContentsMounted' import OverlayComponents from './pages/overlay-components/OverlayComponents'; import OverlayHooks from './pages/overlay-hooks/OverlayHooks'; import ReorderGroup from './pages/ReorderGroup'; +import AccordionGroup from './pages/AccordionGroup'; setupIonicReact(); @@ -69,6 +70,7 @@ const App: React.FC = () => ( + diff --git a/packages/react/test/base/src/pages/AccordionGroup.tsx b/packages/react/test/base/src/pages/AccordionGroup.tsx new file mode 100644 index 00000000000..ffcfaca8bd2 --- /dev/null +++ b/packages/react/test/base/src/pages/AccordionGroup.tsx @@ -0,0 +1,54 @@ +import { IonHeader, IonTitle, IonToolbar, IonPage, IonContent, IonAccordionGroup, IonAccordion, IonItem, IonLabel } from '@ionic/react'; +import { useEffect, useRef } from 'react'; + +const AccordionGroup: React.FC = () => { + const accordionGroup = useRef(null); + + useEffect(() => { + if (!accordionGroup.current) { + return; + } + + accordionGroup.current.value = ['first', 'third']; + }, []); + + return ( + + + + Accordion Group + + + + + + + First Accordion + +
+ First Content +
+
+ + + Second Accordion + +
+ Second Content +
+
+ + + Third Accordion + +
+ Third Content +
+
+
+
+
+ ); +}; + +export default AccordionGroup; diff --git a/packages/react/test/base/src/pages/Main.tsx b/packages/react/test/base/src/pages/Main.tsx index dd87350d9be..3873cd3d5b5 100644 --- a/packages/react/test/base/src/pages/Main.tsx +++ b/packages/react/test/base/src/pages/Main.tsx @@ -22,6 +22,9 @@ const Main: React.FC = () => { + + Accordion Group + Overlay Hooks From f84c48447da8cf8b58a1e6becd234b4be0d66eca Mon Sep 17 00:00:00 2001 From: ShaneK Date: Thu, 30 Oct 2025 06:33:17 -0700 Subject: [PATCH 8/9] fix(accordion): moving to state-based re-rendering instead of calling forceUpdate to try to prevent race conditions --- core/src/components/accordion/accordion.tsx | 45 ++++++++++----------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/core/src/components/accordion/accordion.tsx b/core/src/components/accordion/accordion.tsx index 179a94eed1c..c7eaa791942 100644 --- a/core/src/components/accordion/accordion.tsx +++ b/core/src/components/accordion/accordion.tsx @@ -1,5 +1,5 @@ import type { ComponentInterface } from '@stencil/core'; -import { Component, Element, Host, Prop, State, Watch, forceUpdate, h } from '@stencil/core'; +import { Component, Element, Host, Prop, State, Watch, h } from '@stencil/core'; import { addEventListener, getElementRoot, raf, removeEventListener, transitionEndAsync } from '@utils/helpers'; import { chevronDown } from 'ionicons/icons'; @@ -48,20 +48,19 @@ export class Accordion implements ComponentInterface { private headerEl: HTMLDivElement | undefined; private currentRaf: number | undefined; - /** - * If true, the next animation will be skipped. - * We want to skip the animation when the accordion - * is expanded or collapsed on the initial load. - * This prevents the accordion from animating when - * it starts expanded or collapsed. - */ - private skipNextAnimation = true; @Element() el?: HTMLElement; @State() state: AccordionState = AccordionState.Collapsed; @State() isNext = false; @State() isPrevious = false; + /** + * Tracks whether the component has completed its initial render. + * Animations are disabled until after the first render completes. + * This prevents the accordion from animating when it starts + * expanded or collapsed on initial load. + */ + @State() hasRendered = false; /** * The value of the accordion. Defaults to an autogenerated @@ -132,14 +131,14 @@ export class Accordion implements ComponentInterface { } componentDidRender() { - if (this.skipNextAnimation) { - this.skipNextAnimation = false; - /** - * The initial render disables animations so framework-provided - * values do not cause the accordion to animate. Force a repaint - * so subsequent user interactions animate as expected. - */ - forceUpdate(this); + /** + * After the first render completes, mark that we've rendered. + * Setting this state property triggers a re-render, at which point + * animations will be enabled. This ensures animations are disabled + * only for the initial render, avoiding unwanted animations on load. + */ + if (!this.hasRendered) { + this.hasRendered = true; } } @@ -315,7 +314,12 @@ export class Accordion implements ComponentInterface { * of what is set in the config. */ private shouldAnimate = () => { - if (this.skipNextAnimation) { + /** + * Don't animate until after the first render cycle completes. + * This prevents animations on initial load when accordions + * start in an expanded or collapsed state. + */ + if (!this.hasRendered) { return false; } @@ -351,11 +355,6 @@ export class Accordion implements ComponentInterface { const value = accordionGroup.value; const shouldExpand = Array.isArray(value) ? value.includes(accordionValue) : value === accordionValue; - const shouldDisableAnimation = initialUpdate && shouldExpand; - - if (shouldDisableAnimation) { - this.skipNextAnimation = true; - } if (shouldExpand) { this.expandAccordion(initialUpdate); From 0ce05dc6e1eecad906bd15bc57246fdf39a8eff8 Mon Sep 17 00:00:00 2001 From: ShaneK Date: Thu, 30 Oct 2025 06:50:21 -0700 Subject: [PATCH 9/9] fix(accordion): trying to prevent animation based on interaction rather than render --- .../accordion-group-interface.ts | 1 - .../accordion-group/accordion-group.tsx | 69 ++++------ core/src/components/accordion/accordion.tsx | 120 +++++++++++++----- .../accordion/test/accordion.spec.ts | 34 +---- 4 files changed, 118 insertions(+), 106 deletions(-) diff --git a/core/src/components/accordion-group/accordion-group-interface.ts b/core/src/components/accordion-group/accordion-group-interface.ts index 4498ec42a6f..7daae2982fd 100644 --- a/core/src/components/accordion-group/accordion-group-interface.ts +++ b/core/src/components/accordion-group/accordion-group-interface.ts @@ -1,6 +1,5 @@ export interface AccordionGroupChangeEventDetail { value: T; - initial?: boolean; } export interface AccordionGroupCustomEvent extends CustomEvent { diff --git a/core/src/components/accordion-group/accordion-group.tsx b/core/src/components/accordion-group/accordion-group.tsx index ebe0e61a662..a2b2f82c700 100644 --- a/core/src/components/accordion-group/accordion-group.tsx +++ b/core/src/components/accordion-group/accordion-group.tsx @@ -73,13 +73,28 @@ export class AccordionGroup implements ComponentInterface { */ @Event() ionValueChange!: EventEmitter; - private hasEmittedInitialValue = false; - private isUserInitiatedChange = false; - @Watch('value') valueChanged() { - this.emitValueChange(false, this.isUserInitiatedChange); - this.isUserInitiatedChange = false; + const { value, multiple } = this; + + if (!multiple && Array.isArray(value)) { + /** + * We do some processing on the `value` array so + * that it looks more like an array when logged to + * the console. + * Example given ['a', 'b'] + * Default toString() behavior: a,b + * Custom behavior: ['a', 'b'] + */ + printIonWarning( + `[ion-accordion-group] - An array of values was passed, but multiple is "false". This is incorrect usage and may result in unexpected behaviors. To dismiss this warning, pass a string to the "value" property when multiple="false". + Value Passed: [${value.map((v) => `'${v}'`).join(', ')}] +`, + this.el + ); + } + + this.ionValueChange.emit({ value: this.value }); } @Watch('disabled') @@ -164,12 +179,11 @@ export class AccordionGroup implements ComponentInterface { * it is possible for the value to be set after the Web Component * initializes but before the value watcher is set up in Stencil. * As a result, the watcher callback may not be fired. - * We work around this by manually emitting a value change when the component - * has loaded and the watcher is configured. + * We work around this by manually calling the watcher + * callback when the component has loaded and the watcher + * is configured. */ - if (!this.hasEmittedInitialValue) { - this.emitValueChange(true); - } + this.valueChanged(); } /** @@ -181,7 +195,6 @@ export class AccordionGroup implements ComponentInterface { * accordion group to an array. */ private setValue(accordionValue: string | string[] | null | undefined) { - this.isUserInitiatedChange = true; const value = (this.value = accordionValue); this.ionChange.emit({ value }); } @@ -258,40 +271,6 @@ export class AccordionGroup implements ComponentInterface { return Array.from(this.el.querySelectorAll(':scope > ion-accordion')) as HTMLIonAccordionElement[]; } - private emitValueChange(initial: boolean, isUserInitiated = false) { - const { value, multiple } = this; - - if (!multiple && Array.isArray(value)) { - /** - * We do some processing on the `value` array so - * that it looks more like an array when logged to - * the console. - * Example given ['a', 'b'] - * Default toString() behavior: a,b - * Custom behavior: ['a', 'b'] - */ - printIonWarning( - `[ion-accordion-group] - An array of values was passed, but multiple is "false". This is incorrect usage and may result in unexpected behaviors. To dismiss this warning, pass a string to the "value" property when multiple="false". - - Value Passed: [${value.map((v) => `'${v}'`).join(', ')}] -`, - this.el - ); - } - - /** - * Track if this is the initial value update so accordions - * can skip transition animations when they first render. - */ - const shouldMarkInitial = initial || (!this.hasEmittedInitialValue && value !== undefined && !isUserInitiated); - - this.ionValueChange.emit({ value, initial: shouldMarkInitial }); - - if (value !== undefined) { - this.hasEmittedInitialValue = true; - } - } - render() { const { disabled, readonly, expand } = this; const mode = getIonMode(this); diff --git a/core/src/components/accordion/accordion.tsx b/core/src/components/accordion/accordion.tsx index c7eaa791942..249f8a260cb 100644 --- a/core/src/components/accordion/accordion.tsx +++ b/core/src/components/accordion/accordion.tsx @@ -5,7 +5,6 @@ import { chevronDown } from 'ionicons/icons'; import { config } from '../../global/config'; import { getIonMode } from '../../global/ionic-global'; -import type { AccordionGroupChangeEventDetail } from '../accordion-group/accordion-group-interface'; const enum AccordionState { Collapsed = 1 << 0, @@ -39,9 +38,39 @@ const enum AccordionState { }) export class Accordion implements ComponentInterface { private accordionGroupEl?: HTMLIonAccordionGroupElement | null; - private updateListener = (ev: CustomEvent) => { - const initialUpdate = ev.detail?.initial ?? false; - this.updateState(initialUpdate); + private updateListener = () => { + /** + * Determine if this update will cause an actual state change. + * We only want to mark as "interacted" if the state is changing. + */ + const accordionGroup = this.accordionGroupEl; + if (accordionGroup) { + const value = accordionGroup.value; + const accordionValue = this.value; + const shouldExpand = Array.isArray(value) ? value.includes(accordionValue) : value === accordionValue; + const isExpanded = this.state === AccordionState.Expanded || this.state === AccordionState.Expanding; + const stateWillChange = shouldExpand !== isExpanded; + + /** + * Only mark as interacted if: + * 1. This is not the first update we've received with a defined value + * 2. The state is actually changing (prevents redundant updates from enabling animations) + */ + if (this.hasReceivedFirstUpdate && stateWillChange) { + this.hasInteracted = true; + } + + /** + * Only count this as the first update if the group value is defined. + * This prevents the initial undefined value from the group's componentDidLoad + * from being treated as the first real update. + */ + if (value !== undefined) { + this.hasReceivedFirstUpdate = true; + } + } + + this.updateState(); }; private contentEl: HTMLDivElement | undefined; private contentElWrapper: HTMLDivElement | undefined; @@ -55,12 +84,24 @@ export class Accordion implements ComponentInterface { @State() isNext = false; @State() isPrevious = false; /** - * Tracks whether the component has completed its initial render. - * Animations are disabled until after the first render completes. - * This prevents the accordion from animating when it starts - * expanded or collapsed on initial load. + * Tracks whether a user-initiated interaction has occurred. + * Animations are disabled until the first interaction happens. + * This prevents the accordion from animating when it's programmatically + * set to an expanded or collapsed state on initial load. */ - @State() hasRendered = false; + @State() hasInteracted = false; + + /** + * Tracks if this accordion has ever been expanded. + * Used to prevent the first expansion from animating. + */ + private hasEverBeenExpanded = false; + + /** + * Tracks if this accordion has received its first update from the group. + * Used to distinguish initial programmatic sets from user interactions. + */ + private hasReceivedFirstUpdate = false; /** * The value of the accordion. Defaults to an autogenerated @@ -99,7 +140,7 @@ export class Accordion implements ComponentInterface { connectedCallback() { const accordionGroupEl = (this.accordionGroupEl = this.el?.closest('ion-accordion-group')); if (accordionGroupEl) { - this.updateState(true); + this.updateState(); addEventListener(accordionGroupEl, 'ionValueChange', this.updateListener); } } @@ -130,18 +171,6 @@ export class Accordion implements ComponentInterface { }); } - componentDidRender() { - /** - * After the first render completes, mark that we've rendered. - * Setting this state property triggers a re-render, at which point - * animations will be enabled. This ensures animations are disabled - * only for the initial render, avoiding unwanted animations on load. - */ - if (!this.hasRendered) { - this.hasRendered = true; - } - } - private setItemDefaults = () => { const ionItem = this.getSlottedHeaderIonItem(); if (!ionItem) { @@ -235,10 +264,16 @@ export class Accordion implements ComponentInterface { ionItem.appendChild(iconEl); }; - private expandAccordion = (initialUpdate = false) => { + private expandAccordion = () => { const { contentEl, contentElWrapper } = this; - if (initialUpdate || contentEl === undefined || contentElWrapper === undefined) { + + /** + * If the content elements aren't available yet, just set the state. + * This happens on initial render before the DOM is ready. + */ + if (contentEl === undefined || contentElWrapper === undefined) { this.state = AccordionState.Expanded; + this.hasEverBeenExpanded = true; return; } @@ -250,6 +285,12 @@ export class Accordion implements ComponentInterface { cancelAnimationFrame(this.currentRaf); } + /** + * Mark that this accordion has been expanded at least once. + * This allows subsequent expansions to animate. + */ + this.hasEverBeenExpanded = true; + if (this.shouldAnimate()) { raf(() => { this.state = AccordionState.Expanding; @@ -270,9 +311,14 @@ export class Accordion implements ComponentInterface { } }; - private collapseAccordion = (initialUpdate = false) => { + private collapseAccordion = () => { const { contentEl } = this; - if (initialUpdate || contentEl === undefined) { + + /** + * If the content element isn't available yet, just set the state. + * This happens on initial render before the DOM is ready. + */ + if (contentEl === undefined) { this.state = AccordionState.Collapsed; return; } @@ -315,11 +361,15 @@ export class Accordion implements ComponentInterface { */ private shouldAnimate = () => { /** - * Don't animate until after the first render cycle completes. + * Don't animate until after the first user interaction. * This prevents animations on initial load when accordions - * start in an expanded or collapsed state. + * start in an expanded or collapsed state programmatically. + * + * Additionally, don't animate the very first expansion even if + * hasInteracted is true. This handles edge cases like React StrictMode + * where effects run twice and might incorrectly mark as interacted. */ - if (!this.hasRendered) { + if (!this.hasInteracted || !this.hasEverBeenExpanded) { return false; } @@ -344,7 +394,7 @@ export class Accordion implements ComponentInterface { return true; }; - private updateState = async (initialUpdate = false) => { + private updateState = async () => { const accordionGroup = this.accordionGroupEl; const accordionValue = this.value; @@ -357,10 +407,10 @@ export class Accordion implements ComponentInterface { const shouldExpand = Array.isArray(value) ? value.includes(accordionValue) : value === accordionValue; if (shouldExpand) { - this.expandAccordion(initialUpdate); + this.expandAccordion(); this.isNext = this.isPrevious = false; } else { - this.collapseAccordion(initialUpdate); + this.collapseAccordion(); /** * When using popout or inset, @@ -418,6 +468,12 @@ export class Accordion implements ComponentInterface { if (disabled || readonly) return; + /** + * Mark that the user has interacted with the accordion. + * This enables animations for all future state changes. + */ + this.hasInteracted = true; + if (accordionGroupEl) { /** * Because the accordion group may or may diff --git a/core/src/components/accordion/test/accordion.spec.ts b/core/src/components/accordion/test/accordion.spec.ts index 033ee00f8fb..e10fdc9d279 100644 --- a/core/src/components/accordion/test/accordion.spec.ts +++ b/core/src/components/accordion/test/accordion.spec.ts @@ -1,6 +1,5 @@ import { newSpecPage } from '@stencil/core/testing'; -import type { AccordionGroupChangeEventDetail } from '../../accordion-group/accordion-group-interface'; import { AccordionGroup } from '../../accordion-group/accordion-group'; import { Item } from '../../item/item'; import { Accordion } from '../accordion'; @@ -218,18 +217,11 @@ it('should not animate when initial value is set before load', async () => { `; - const details: AccordionGroupChangeEventDetail[] = []; - accordionGroup.addEventListener('ionValueChange', (event: CustomEvent) => { - details.push(event.detail); - }); - accordionGroup.value = 'first'; page.body.appendChild(accordionGroup); await page.waitForChanges(); - expect(details[0]?.initial).toBe(true); - const firstAccordion = accordionGroup.querySelector('ion-accordion[value="first"]')!; expect(firstAccordion.classList.contains('accordion-expanded')).toEqual(true); @@ -253,27 +245,19 @@ it('should not animate when initial value is set after load', async () => { `; - const details: AccordionGroupChangeEventDetail[] = []; - accordionGroup.addEventListener('ionValueChange', (event: CustomEvent) => { - details.push(event.detail); - }); - page.body.appendChild(accordionGroup); await page.waitForChanges(); accordionGroup.value = 'first'; await page.waitForChanges(); - const firstDetail = details.find((detail) => detail.value === 'first'); - expect(firstDetail?.initial).toBe(true); - const firstAccordion = accordionGroup.querySelector('ion-accordion[value="first"]')!; expect(firstAccordion.classList.contains('accordion-expanded')).toEqual(true); expect(firstAccordion.classList.contains('accordion-expanding')).toEqual(false); }); -it('should animate when accordion is first opened by user', async () => { +it('should not have animated class on first expansion', async () => { const page = await newSpecPage({ components: [Item, Accordion, AccordionGroup], html: ` @@ -287,20 +271,14 @@ it('should animate when accordion is first opened by user', async () => { }); const accordionGroup = page.body.querySelector('ion-accordion-group')!; + const firstAccordion = page.body.querySelector('ion-accordion[value="first"]')!; - const details: AccordionGroupChangeEventDetail[] = []; - accordionGroup.addEventListener('ionValueChange', (event: CustomEvent) => { - details.push(event.detail); - }); - - await accordionGroup.requestAccordionToggle('first', true); + // First expansion should not have the animated class + accordionGroup.value = 'first'; await page.waitForChanges(); - const lastDetail = details[details.length - 1]; - expect(lastDetail?.initial).toBe(false); - - const firstAccordion = accordionGroup.querySelector('ion-accordion[value="first"]')!; - expect(firstAccordion.classList.contains('accordion-animated')).toEqual(true); + expect(firstAccordion.classList.contains('accordion-animated')).toEqual(false); + expect(firstAccordion.classList.contains('accordion-expanded')).toEqual(true); }); // Verifies fix for https://github.com/ionic-team/ionic-framework/issues/27047