From 0ce05dc6e1eecad906bd15bc57246fdf39a8eff8 Mon Sep 17 00:00:00 2001 From: ShaneK Date: Thu, 30 Oct 2025 06:50:21 -0700 Subject: [PATCH] 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 4498ec42a6..7daae2982f 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 ebe0e61a66..a2b2f82c70 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 c7eaa79194..249f8a260c 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 033ee00f8f..e10fdc9d27 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