From f0b0762f4aac6b150f0e9cf260311ea5f2c55cfc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Fri, 9 Dec 2016 10:45:10 -0800 Subject: [PATCH] fix(animations): always cleanup players after they have finished internally (#13334) Closes #13333 Closes #13334 --- .../src/animation/animation_compiler.ts | 3 +- .../core/src/animation/view_animation_map.ts | 14 +++-- .../core/src/linker/animation_view_context.ts | 12 ++-- .../animation/animation_integration_spec.ts | 37 +++++++++++ .../linker/animation_view_context_spec.ts | 61 +++++++++++++++++++ 5 files changed, 114 insertions(+), 13 deletions(-) create mode 100644 modules/@angular/core/test/linker/animation_view_context_spec.ts diff --git a/modules/@angular/compiler/src/animation/animation_compiler.ts b/modules/@angular/compiler/src/animation/animation_compiler.ts index 02a3057b29..5e48d7623d 100644 --- a/modules/@angular/compiler/src/animation/animation_compiler.ts +++ b/modules/@angular/compiler/src/animation/animation_compiler.ts @@ -199,8 +199,9 @@ class _AnimationBuilder implements AnimationAstVisitor { .set(_ANIMATION_FACTORY_VIEW_CONTEXT.callMethod( 'getAnimationPlayers', [ - _ANIMATION_FACTORY_ELEMENT_VAR, o.literal(this.animationName), + _ANIMATION_FACTORY_ELEMENT_VAR, _ANIMATION_NEXT_STATE_VAR.equals(o.literal(EMPTY_STATE)) + .conditional(o.NULL_EXPR, o.literal(this.animationName)) ])) .toDeclStmt()); diff --git a/modules/@angular/core/src/animation/view_animation_map.ts b/modules/@angular/core/src/animation/view_animation_map.ts index 874a3a50e2..c22dae819c 100644 --- a/modules/@angular/core/src/animation/view_animation_map.ts +++ b/modules/@angular/core/src/animation/view_animation_map.ts @@ -44,16 +44,18 @@ export class ViewAnimationMap { getAllPlayers(): AnimationPlayer[] { return this._allPlayers; } - remove(element: any, animationName: string): void { + remove(element: any, animationName: string, targetPlayer: AnimationPlayer = null): void { const playersByAnimation = this._map.get(element); if (playersByAnimation) { const player = playersByAnimation[animationName]; - delete playersByAnimation[animationName]; - const index = this._allPlayers.indexOf(player); - this._allPlayers.splice(index, 1); + if (!targetPlayer || player === targetPlayer) { + delete playersByAnimation[animationName]; + const index = this._allPlayers.indexOf(player); + this._allPlayers.splice(index, 1); - if (Object.keys(playersByAnimation).length === 0) { - this._map.delete(element); + if (Object.keys(playersByAnimation).length === 0) { + this._map.delete(element); + } } } } diff --git a/modules/@angular/core/src/linker/animation_view_context.ts b/modules/@angular/core/src/linker/animation_view_context.ts index 77a294e792..51aa2b1e51 100644 --- a/modules/@angular/core/src/linker/animation_view_context.ts +++ b/modules/@angular/core/src/linker/animation_view_context.ts @@ -29,19 +29,19 @@ export class AnimationViewContext { queueAnimation(element: any, animationName: string, player: AnimationPlayer): void { queueAnimationGlobally(player); this._players.set(element, animationName, player); + player.onDone(() => this._players.remove(element, animationName, player)); } - getAnimationPlayers(element: any, animationName: string, removeAllAnimations: boolean = false): - AnimationPlayer[] { + getAnimationPlayers(element: any, animationName: string = null): AnimationPlayer[] { const players: AnimationPlayer[] = []; - if (removeAllAnimations) { - this._players.findAllPlayersByElement(element).forEach( - player => { _recursePlayers(player, players); }); - } else { + if (animationName) { const currentPlayer = this._players.find(element, animationName); if (currentPlayer) { _recursePlayers(currentPlayer, players); } + } else { + this._players.findAllPlayersByElement(element).forEach( + player => _recursePlayers(player, players)); } return players; } diff --git a/modules/@angular/core/test/animation/animation_integration_spec.ts b/modules/@angular/core/test/animation/animation_integration_spec.ts index bfd13c9964..97cacd2c6b 100644 --- a/modules/@angular/core/test/animation/animation_integration_spec.ts +++ b/modules/@angular/core/test/animation/animation_integration_spec.ts @@ -2121,6 +2121,43 @@ function declareTests({useJit}: {useJit: boolean}) { expect(kf[1]).toEqual([1, {'height': '333px', 'opacity': AUTO_STYLE, 'width': '200px'}]); }); }); + + it('should not use the previous animation\'s styling if the previous animation has already finished', + fakeAsync(() => { + TestBed.overrideComponent(DummyIfCmp, { + set: { + template: ` +
+ `, + animations: [trigger( + 'myAnimation', + [ + state('a', style({color: 'red'})), state('b', style({color: 'red'})), + transition('* => *', animate(1000)) + ])] + } + }); + + const driver = TestBed.get(AnimationDriver) as MockAnimationDriver; + const fixture = TestBed.createComponent(DummyIfCmp); + const cmp = fixture.componentInstance; + + cmp.exp = 'a'; + fixture.detectChanges(); + flushMicrotasks(); + + const animation1 = driver.log.shift(); + expect(animation1['previousStyles']).toEqual({}); + + animation1['player'].finish(); + + cmp.exp = 'b'; + fixture.detectChanges(); + flushMicrotasks(); + + const animation2 = driver.log.shift(); + expect(animation2['previousStyles']).toEqual({}); + })); }); describe('full animation integration tests', () => { diff --git a/modules/@angular/core/test/linker/animation_view_context_spec.ts b/modules/@angular/core/test/linker/animation_view_context_spec.ts new file mode 100644 index 0000000000..c9e87a12c4 --- /dev/null +++ b/modules/@angular/core/test/linker/animation_view_context_spec.ts @@ -0,0 +1,61 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * 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 {el} from '@angular/platform-browser/testing/browser_util'; + +import {NoOpAnimationPlayer} from '../../src/animation/animation_player'; +import {AnimationViewContext} from '../../src/linker/animation_view_context'; +import {fakeAsync, flushMicrotasks} from '../../testing'; +import {describe, expect, iit, it} from '../../testing/testing_internal'; + +export function main() { + describe('AnimationViewContext', function() { + let viewContext: AnimationViewContext; + let elm: any; + beforeEach(() => { + viewContext = new AnimationViewContext(); + elm = el('
'); + }); + + function getPlayers() { return viewContext.getAnimationPlayers(elm); } + + it('should remove the player from the registry once the animation is complete', + fakeAsync(() => { + const player = new NoOpAnimationPlayer(); + + expect(getPlayers().length).toEqual(0); + viewContext.queueAnimation(elm, 'someAnimation', player); + expect(getPlayers().length).toEqual(1); + player.finish(); + expect(getPlayers().length).toEqual(0); + })); + + it('should not remove a follow-up player from the registry if another player is queued', + fakeAsync(() => { + const player1 = new NoOpAnimationPlayer(); + const player2 = new NoOpAnimationPlayer(); + + viewContext.queueAnimation(elm, 'someAnimation', player1); + expect(getPlayers().length).toBe(1); + expect(getPlayers()[0]).toBe(player1); + + viewContext.queueAnimation(elm, 'someAnimation', player2); + expect(getPlayers().length).toBe(1); + expect(getPlayers()[0]).toBe(player2); + + player1.finish(); + + expect(getPlayers().length).toBe(1); + expect(getPlayers()[0]).toBe(player2); + + player2.finish(); + + expect(getPlayers().length).toBe(0); + })); + }); +}