From 4ce44eac3389f2c36972b0d8ce341e59042a5fd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Fri, 7 Feb 2020 14:57:10 -0500 Subject: [PATCH] fix(animations): properly track listeners for a removed element (#40712) Prior to this patch, if an element was removed multiple times (due to the nature of parent/child elements), the leave listeners may have been fired for an element that was already removed. This patch adds a guard within the animations code to prevent this. PR Close #40712 --- .../src/render/transition_animation_engine.ts | 8 +- .../core/test/acceptance/integration_spec.ts | 220 +++++++++++++++++- .../animation/animation_integration_spec.ts | 1 - 3 files changed, 224 insertions(+), 5 deletions(-) diff --git a/packages/animations/browser/src/render/transition_animation_engine.ts b/packages/animations/browser/src/render/transition_animation_engine.ts index 79703bc234..d6f7547991 100644 --- a/packages/animations/browser/src/render/transition_animation_engine.ts +++ b/packages/animations/browser/src/render/transition_animation_engine.ts @@ -370,7 +370,11 @@ export class AnimationTransitionNamespace { prepareLeaveAnimationListeners(element: any) { const listeners = this._elementListeners.get(element); - if (listeners) { + const elementStates = this._engine.statesByElement.get(element); + + // if this statement fails then it means that the element was picked up + // by an earlier flush (or there are no listeners at all to track the leave). + if (listeners && elementStates) { const visitedTriggers = new Set(); listeners.forEach(listener => { const triggerName = listener.name; @@ -379,7 +383,6 @@ export class AnimationTransitionNamespace { const trigger = this._triggers[triggerName]; const transition = trigger.fallbackTransition; - const elementStates = this._engine.statesByElement.get(element)!; const fromState = elementStates[triggerName] || DEFAULT_STATE_VALUE; const toState = new StateValue(VOID_VALUE); const player = new TransitionAnimationPlayer(this.id, triggerName, element); @@ -400,7 +403,6 @@ export class AnimationTransitionNamespace { removeNode(element: any, context: any): void { const engine = this._engine; - if (element.childElementCount) { this._signalRemovalForInnerTriggers(element, context); } diff --git a/packages/core/test/acceptance/integration_spec.ts b/packages/core/test/acceptance/integration_spec.ts index 21e0c0000a..bc4e911b9e 100644 --- a/packages/core/test/acceptance/integration_spec.ts +++ b/packages/core/test/acceptance/integration_spec.ts @@ -5,12 +5,16 @@ * Use of this source code is governed by an MIT-style license that can be * found in the LICENSE file at https://angular.io/license */ +import {animate, AnimationEvent, state, style, transition, trigger} from '@angular/animations'; +import {AnimationDriver} from '@angular/animations/browser'; +import {MockAnimationDriver, MockAnimationPlayer} from '@angular/animations/browser/testing'; import {CommonModule} from '@angular/common'; import {Component, ContentChild, Directive, ElementRef, EventEmitter, HostBinding, HostListener, Input, NgModule, OnInit, Output, Pipe, QueryList, TemplateRef, ViewChild, ViewChildren, ViewContainerRef} from '@angular/core'; +import {Inject} from '@angular/core/src/di'; import {TVIEW} from '@angular/core/src/render3/interfaces/view'; import {getLView} from '@angular/core/src/render3/state'; import {ngDevModeResetPerfCounters} from '@angular/core/src/util/ng_dev_mode'; -import {TestBed} from '@angular/core/testing'; +import {fakeAsync, flushMicrotasks, TestBed} from '@angular/core/testing'; import {By} from '@angular/platform-browser'; import {expect} from '@angular/platform-browser/testing/src/matchers'; import {ivyEnabled, onlyInIvy} from '@angular/private/testing'; @@ -2017,4 +2021,218 @@ describe('acceptance integration tests', () => { assertAttrValues(elements[2], 'post-update-pass'); }); }); + + describe('animations', () => { + it('should apply triggers for a list of items when they are sorted and reSorted', + fakeAsync(() => { + interface Item { + value: any; + id: number; + } + + @Component({ + template: ` +
+ Nooo! +
+ + + + {{ item.value }} + + + ` + }) + class Cmp { + showWarningMessage = false; + + items: Item[] = [ + {value: 1, id: 1}, + {value: 2, id: 2}, + {value: 3, id: 3}, + {value: 4, id: 4}, + {value: 5, id: 5}, + ]; + + itemTrackFn(value: Item) { + return value.id; + } + } + + @Component({ + selector: 'animation-comp', + animations: [ + trigger( + 'host', + [ + state('void', style({height: '0px'})), + transition( + '* => *', + [ + animate('1s'), + ]), + ]), + ], + template: ` + + ` + }) + class AnimationComp { + @HostBinding('@host') public hostState = ''; + + @HostListener('@host.start', ['$event']) + onLeaveStart(event: AnimationEvent) { + // we just want to register the listener + } + } + + TestBed.configureTestingModule({ + declarations: [Cmp, AnimationComp], + providers: [{provide: AnimationDriver, useClass: MockAnimationDriver}], + }); + const fixture = TestBed.createComponent(Cmp); + fixture.detectChanges(); + + let elements = queryAll(fixture.nativeElement, 'animation-comp'); + expect(elements.length).toEqual(5); + expect(elements.map(e => e.textContent?.trim())).toEqual(['1', '2', '3', '4', '5']); + + const items = fixture.componentInstance.items; + arraySwap(items, 2, 0); // 3 2 1 4 5 + arraySwap(items, 2, 1); // 3 1 2 4 5 + const first = items.shift()!; + items.push(first); // 1 2 4 5 3 + fixture.detectChanges(); + + elements = queryAll(fixture.nativeElement, 'animation-comp'); + expect(elements.length).toEqual(5); + expect(elements.map(e => e.textContent?.trim())).toEqual(['1', '2', '4', '5', '3']); + completeAnimations(); + + fixture.componentInstance.showWarningMessage = true; + fixture.detectChanges(); + completeAnimations(); + + elements = queryAll(fixture.nativeElement, 'animation-comp'); + expect(elements.length).toEqual(0); + expect(fixture.nativeElement.textContent.trim()).toEqual('Nooo!'); + + fixture.componentInstance.showWarningMessage = false; + fixture.detectChanges(); + + elements = queryAll(fixture.nativeElement, 'animation-comp'); + expect(elements.length).toEqual(5); + })); + + it('should insert and remove views in the correct order when animations are present', + fakeAsync(() => { + @Component({ + animations: [ + trigger('root', [transition('* => *', [])]), + trigger('outer', [transition('* => *', [])]), + trigger('inner', [transition('* => *', [])]), + ], + template: ` +
+
+ Nooo! +
+ + + + {{ item.value }} + + +
+ ` + }) + class Cmp { + showRoot = true; + showIfContents = true; + items = [1]; + log: string[] = []; + + track(name: string, event: AnimationEvent) { + this.log.push(name); + } + } + + @Component({ + selector: 'inner-comp', + animations: [ + trigger('host', [transition('* => *', [])]), + ], + template: ` + + ` + }) + class InnerComp { + @HostBinding('@host') public hostState = ''; + + constructor(@Inject(Cmp) private parent: Cmp) {} + + @HostListener('@host.start', ['$event']) + onLeaveStart(event: AnimationEvent) { + this.parent.log.push('host'); + } + } + + TestBed.configureTestingModule({ + declarations: [Cmp, InnerComp], + providers: [{provide: AnimationDriver, useClass: MockAnimationDriver}], + }); + const fixture = TestBed.createComponent(Cmp); + fixture.detectChanges(); + completeAnimations(); + + const comp = fixture.componentInstance; + expect(comp.log).toEqual([ + 'root', // insertion of the inner-comp content + 'outer', // insertion of the default ngIf + ]); + + comp.log = []; + comp.showIfContents = false; + fixture.detectChanges(); + completeAnimations(); + + expect(comp.log).toEqual([ + 'host', // insertion of the inner-comp content + 'outer', // insertion of the template into the ngIf + 'inner' // insertion of the inner comp element + ]); + + comp.log = []; + comp.showRoot = false; + fixture.detectChanges(); + completeAnimations(); + + expect(comp.log).toEqual([ + 'root', // removal the root div container + 'host', // removal of the inner-comp content + 'inner' // removal of the inner comp element + ]); + })); + }); }); + +function completeAnimations() { + flushMicrotasks(); + const log = MockAnimationDriver.log as MockAnimationPlayer[]; + log.forEach(player => player.finish()); + flushMicrotasks(); +} + +function arraySwap(arr: any[], indexA: number, indexB: number): void { + const item = arr[indexA]; + arr[indexA] = arr[indexB]; + arr[indexB] = item; +} + +/** + * Queries the provided `root` element for sub elements by the selector and casts the result as an + * array of elements + */ +function queryAll(root: HTMLElement, selector: string): HTMLElement[] { + return Array.from(root.querySelectorAll(selector)); +} diff --git a/packages/core/test/animation/animation_integration_spec.ts b/packages/core/test/animation/animation_integration_spec.ts index 02f5704682..a73d153977 100644 --- a/packages/core/test/animation/animation_integration_spec.ts +++ b/packages/core/test/animation/animation_integration_spec.ts @@ -8,7 +8,6 @@ import {animate, animateChild, AnimationEvent, AnimationOptions, AUTO_STYLE, group, keyframes, query, state, style, transition, trigger, ɵPRE_STYLE as PRE_STYLE} from '@angular/animations'; import {AnimationDriver, ɵAnimationEngine, ɵNoopAnimationDriver as NoopAnimationDriver} from '@angular/animations/browser'; import {MockAnimationDriver, MockAnimationPlayer} from '@angular/animations/browser/testing'; -import {ɵgetDOM as getDOM} from '@angular/common'; import {ChangeDetectorRef, Component, HostBinding, HostListener, Inject, RendererFactory2, ViewChild} from '@angular/core'; import {fakeAsync, flushMicrotasks, TestBed} from '@angular/core/testing'; import {ɵDomRendererFactory2} from '@angular/platform-browser';