fix(accordion): trying to prevent animation based on interaction rather than render

This commit is contained in:
ShaneK
2025-10-30 06:50:21 -07:00
parent f84c48447d
commit 0ce05dc6e1
4 changed files with 118 additions and 106 deletions

View File

@ -1,6 +1,5 @@
export interface AccordionGroupChangeEventDetail<T = any> { export interface AccordionGroupChangeEventDetail<T = any> {
value: T; value: T;
initial?: boolean;
} }
export interface AccordionGroupCustomEvent<T = any> extends CustomEvent { export interface AccordionGroupCustomEvent<T = any> extends CustomEvent {

View File

@ -73,13 +73,28 @@ export class AccordionGroup implements ComponentInterface {
*/ */
@Event() ionValueChange!: EventEmitter<AccordionGroupChangeEventDetail>; @Event() ionValueChange!: EventEmitter<AccordionGroupChangeEventDetail>;
private hasEmittedInitialValue = false;
private isUserInitiatedChange = false;
@Watch('value') @Watch('value')
valueChanged() { valueChanged() {
this.emitValueChange(false, this.isUserInitiatedChange); const { value, multiple } = this;
this.isUserInitiatedChange = false;
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') @Watch('disabled')
@ -164,12 +179,11 @@ export class AccordionGroup implements ComponentInterface {
* it is possible for the value to be set after the Web Component * it is possible for the value to be set after the Web Component
* initializes but before the value watcher is set up in Stencil. * initializes but before the value watcher is set up in Stencil.
* As a result, the watcher callback may not be fired. * As a result, the watcher callback may not be fired.
* We work around this by manually emitting a value change when the component * We work around this by manually calling the watcher
* has loaded and the watcher is configured. * callback when the component has loaded and the watcher
* is configured.
*/ */
if (!this.hasEmittedInitialValue) { this.valueChanged();
this.emitValueChange(true);
}
} }
/** /**
@ -181,7 +195,6 @@ export class AccordionGroup implements ComponentInterface {
* accordion group to an array. * accordion group to an array.
*/ */
private setValue(accordionValue: string | string[] | null | undefined) { private setValue(accordionValue: string | string[] | null | undefined) {
this.isUserInitiatedChange = true;
const value = (this.value = accordionValue); const value = (this.value = accordionValue);
this.ionChange.emit({ value }); this.ionChange.emit({ value });
} }
@ -258,40 +271,6 @@ export class AccordionGroup implements ComponentInterface {
return Array.from(this.el.querySelectorAll(':scope > ion-accordion')) as HTMLIonAccordionElement[]; 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() { render() {
const { disabled, readonly, expand } = this; const { disabled, readonly, expand } = this;
const mode = getIonMode(this); const mode = getIonMode(this);

View File

@ -5,7 +5,6 @@ import { chevronDown } from 'ionicons/icons';
import { config } from '../../global/config'; import { config } from '../../global/config';
import { getIonMode } from '../../global/ionic-global'; import { getIonMode } from '../../global/ionic-global';
import type { AccordionGroupChangeEventDetail } from '../accordion-group/accordion-group-interface';
const enum AccordionState { const enum AccordionState {
Collapsed = 1 << 0, Collapsed = 1 << 0,
@ -39,9 +38,39 @@ const enum AccordionState {
}) })
export class Accordion implements ComponentInterface { export class Accordion implements ComponentInterface {
private accordionGroupEl?: HTMLIonAccordionGroupElement | null; private accordionGroupEl?: HTMLIonAccordionGroupElement | null;
private updateListener = (ev: CustomEvent<AccordionGroupChangeEventDetail>) => { private updateListener = () => {
const initialUpdate = ev.detail?.initial ?? false; /**
this.updateState(initialUpdate); * 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 contentEl: HTMLDivElement | undefined;
private contentElWrapper: HTMLDivElement | undefined; private contentElWrapper: HTMLDivElement | undefined;
@ -55,12 +84,24 @@ export class Accordion implements ComponentInterface {
@State() isNext = false; @State() isNext = false;
@State() isPrevious = false; @State() isPrevious = false;
/** /**
* Tracks whether the component has completed its initial render. * Tracks whether a user-initiated interaction has occurred.
* Animations are disabled until after the first render completes. * Animations are disabled until the first interaction happens.
* This prevents the accordion from animating when it starts * This prevents the accordion from animating when it's programmatically
* expanded or collapsed on initial load. * 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 * The value of the accordion. Defaults to an autogenerated
@ -99,7 +140,7 @@ export class Accordion implements ComponentInterface {
connectedCallback() { connectedCallback() {
const accordionGroupEl = (this.accordionGroupEl = this.el?.closest('ion-accordion-group')); const accordionGroupEl = (this.accordionGroupEl = this.el?.closest('ion-accordion-group'));
if (accordionGroupEl) { if (accordionGroupEl) {
this.updateState(true); this.updateState();
addEventListener(accordionGroupEl, 'ionValueChange', this.updateListener); 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 = () => { private setItemDefaults = () => {
const ionItem = this.getSlottedHeaderIonItem(); const ionItem = this.getSlottedHeaderIonItem();
if (!ionItem) { if (!ionItem) {
@ -235,10 +264,16 @@ export class Accordion implements ComponentInterface {
ionItem.appendChild(iconEl); ionItem.appendChild(iconEl);
}; };
private expandAccordion = (initialUpdate = false) => { private expandAccordion = () => {
const { contentEl, contentElWrapper } = this; 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.state = AccordionState.Expanded;
this.hasEverBeenExpanded = true;
return; return;
} }
@ -250,6 +285,12 @@ export class Accordion implements ComponentInterface {
cancelAnimationFrame(this.currentRaf); 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()) { if (this.shouldAnimate()) {
raf(() => { raf(() => {
this.state = AccordionState.Expanding; this.state = AccordionState.Expanding;
@ -270,9 +311,14 @@ export class Accordion implements ComponentInterface {
} }
}; };
private collapseAccordion = (initialUpdate = false) => { private collapseAccordion = () => {
const { contentEl } = this; 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; this.state = AccordionState.Collapsed;
return; return;
} }
@ -315,11 +361,15 @@ export class Accordion implements ComponentInterface {
*/ */
private shouldAnimate = () => { 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 * 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; return false;
} }
@ -344,7 +394,7 @@ export class Accordion implements ComponentInterface {
return true; return true;
}; };
private updateState = async (initialUpdate = false) => { private updateState = async () => {
const accordionGroup = this.accordionGroupEl; const accordionGroup = this.accordionGroupEl;
const accordionValue = this.value; const accordionValue = this.value;
@ -357,10 +407,10 @@ export class Accordion implements ComponentInterface {
const shouldExpand = Array.isArray(value) ? value.includes(accordionValue) : value === accordionValue; const shouldExpand = Array.isArray(value) ? value.includes(accordionValue) : value === accordionValue;
if (shouldExpand) { if (shouldExpand) {
this.expandAccordion(initialUpdate); this.expandAccordion();
this.isNext = this.isPrevious = false; this.isNext = this.isPrevious = false;
} else { } else {
this.collapseAccordion(initialUpdate); this.collapseAccordion();
/** /**
* When using popout or inset, * When using popout or inset,
@ -418,6 +468,12 @@ export class Accordion implements ComponentInterface {
if (disabled || readonly) return; 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) { if (accordionGroupEl) {
/** /**
* Because the accordion group may or may * Because the accordion group may or may

View File

@ -1,6 +1,5 @@
import { newSpecPage } from '@stencil/core/testing'; import { newSpecPage } from '@stencil/core/testing';
import type { AccordionGroupChangeEventDetail } from '../../accordion-group/accordion-group-interface';
import { AccordionGroup } from '../../accordion-group/accordion-group'; import { AccordionGroup } from '../../accordion-group/accordion-group';
import { Item } from '../../item/item'; import { Item } from '../../item/item';
import { Accordion } from '../accordion'; import { Accordion } from '../accordion';
@ -218,18 +217,11 @@ it('should not animate when initial value is set before load', async () => {
</ion-accordion> </ion-accordion>
`; `;
const details: AccordionGroupChangeEventDetail[] = [];
accordionGroup.addEventListener('ionValueChange', (event: CustomEvent<AccordionGroupChangeEventDetail>) => {
details.push(event.detail);
});
accordionGroup.value = 'first'; accordionGroup.value = 'first';
page.body.appendChild(accordionGroup); page.body.appendChild(accordionGroup);
await page.waitForChanges(); await page.waitForChanges();
expect(details[0]?.initial).toBe(true);
const firstAccordion = accordionGroup.querySelector('ion-accordion[value="first"]')!; const firstAccordion = accordionGroup.querySelector('ion-accordion[value="first"]')!;
expect(firstAccordion.classList.contains('accordion-expanded')).toEqual(true); expect(firstAccordion.classList.contains('accordion-expanded')).toEqual(true);
@ -253,27 +245,19 @@ it('should not animate when initial value is set after load', async () => {
</ion-accordion> </ion-accordion>
`; `;
const details: AccordionGroupChangeEventDetail[] = [];
accordionGroup.addEventListener('ionValueChange', (event: CustomEvent<AccordionGroupChangeEventDetail>) => {
details.push(event.detail);
});
page.body.appendChild(accordionGroup); page.body.appendChild(accordionGroup);
await page.waitForChanges(); await page.waitForChanges();
accordionGroup.value = 'first'; accordionGroup.value = 'first';
await page.waitForChanges(); await page.waitForChanges();
const firstDetail = details.find((detail) => detail.value === 'first');
expect(firstDetail?.initial).toBe(true);
const firstAccordion = accordionGroup.querySelector('ion-accordion[value="first"]')!; const firstAccordion = accordionGroup.querySelector('ion-accordion[value="first"]')!;
expect(firstAccordion.classList.contains('accordion-expanded')).toEqual(true); expect(firstAccordion.classList.contains('accordion-expanded')).toEqual(true);
expect(firstAccordion.classList.contains('accordion-expanding')).toEqual(false); 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({ const page = await newSpecPage({
components: [Item, Accordion, AccordionGroup], components: [Item, Accordion, AccordionGroup],
html: ` 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 accordionGroup = page.body.querySelector('ion-accordion-group')!;
const firstAccordion = page.body.querySelector('ion-accordion[value="first"]')!;
const details: AccordionGroupChangeEventDetail[] = []; // First expansion should not have the animated class
accordionGroup.addEventListener('ionValueChange', (event: CustomEvent<AccordionGroupChangeEventDetail>) => { accordionGroup.value = 'first';
details.push(event.detail);
});
await accordionGroup.requestAccordionToggle('first', true);
await page.waitForChanges(); await page.waitForChanges();
const lastDetail = details[details.length - 1]; expect(firstAccordion.classList.contains('accordion-animated')).toEqual(false);
expect(lastDetail?.initial).toBe(false); expect(firstAccordion.classList.contains('accordion-expanded')).toEqual(true);
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 // Verifies fix for https://github.com/ionic-team/ionic-framework/issues/27047