From fe35bc34f64ea69e02ba2cb09e157ef69720bd72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Tue, 8 Nov 2016 16:21:28 -0800 Subject: [PATCH] fix(animations): allow animations to be destroyed manually (#12719) Closes #12456 Closes #12719 --- .../src/animation/animation_compiler.ts | 23 +++-- .../src/animation/animation_group_player.ts | 18 ++-- .../animation/animation_sequence_player.ts | 22 +++-- .../animation/animation_group_player_spec.ts | 87 ++++++++++--------- .../animation_sequence_player_spec.ts | 85 +++++++++--------- .../core/testing/mock_animation_player.ts | 13 +-- .../src/dom/web_animations_player.ts | 29 ++++--- .../test/dom/web_animations_player_spec.ts | 19 +++- 8 files changed, 175 insertions(+), 121 deletions(-) diff --git a/modules/@angular/compiler/src/animation/animation_compiler.ts b/modules/@angular/compiler/src/animation/animation_compiler.ts index 0ad4501dd1..633aa47a1e 100644 --- a/modules/@angular/compiler/src/animation/animation_compiler.ts +++ b/modules/@angular/compiler/src/animation/animation_compiler.ts @@ -251,17 +251,22 @@ class _AnimationBuilder implements AnimationAstVisitor { _ANIMATION_PLAYER_VAR .callMethod( 'onDone', - [o.fn( - [], - [RENDER_STYLES_FN - .callFn([ - _ANIMATION_FACTORY_ELEMENT_VAR, _ANIMATION_FACTORY_RENDERER_VAR, - o.importExpr(resolveIdentifier(Identifiers.prepareFinalAnimationStyles)) + [o + .fn([], + [ + _ANIMATION_PLAYER_VAR.callMethod('destroy', []).toStmt(), + RENDER_STYLES_FN .callFn([ - _ANIMATION_START_STATE_STYLES_VAR, _ANIMATION_END_STATE_STYLES_VAR + _ANIMATION_FACTORY_ELEMENT_VAR, _ANIMATION_FACTORY_RENDERER_VAR, + o.importExpr( + resolveIdentifier(Identifiers.prepareFinalAnimationStyles)) + .callFn([ + _ANIMATION_START_STATE_STYLES_VAR, + _ANIMATION_END_STATE_STYLES_VAR + ]) ]) - ]) - .toStmt()])]) + .toStmt() + ])]) .toStmt()); statements.push(_ANIMATION_FACTORY_VIEW_CONTEXT diff --git a/modules/@angular/core/src/animation/animation_group_player.ts b/modules/@angular/core/src/animation/animation_group_player.ts index 9145d4abed..8c04706f71 100644 --- a/modules/@angular/core/src/animation/animation_group_player.ts +++ b/modules/@angular/core/src/animation/animation_group_player.ts @@ -15,6 +15,7 @@ export class AnimationGroupPlayer implements AnimationPlayer { private _onStartFns: Function[] = []; private _finished = false; private _started = false; + private _destroyed = false; public parentPlayer: AnimationPlayer = null; @@ -38,9 +39,6 @@ export class AnimationGroupPlayer implements AnimationPlayer { private _onFinish() { if (!this._finished) { this._finished = true; - if (!isPresent(this.parentPlayer)) { - this.destroy(); - } this._onDoneFns.forEach(fn => fn()); this._onDoneFns = []; } @@ -76,11 +74,19 @@ export class AnimationGroupPlayer implements AnimationPlayer { } destroy(): void { - this._onFinish(); - this._players.forEach(player => player.destroy()); + if (!this._destroyed) { + this._onFinish(); + this._players.forEach(player => player.destroy()); + this._destroyed = true; + } } - reset(): void { this._players.forEach(player => player.reset()); } + reset(): void { + this._players.forEach(player => player.reset()); + this._destroyed = false; + this._finished = false; + this._started = false; + } setPosition(p: any /** TODO #9100 */): void { this._players.forEach(player => { player.setPosition(p); }); diff --git a/modules/@angular/core/src/animation/animation_sequence_player.ts b/modules/@angular/core/src/animation/animation_sequence_player.ts index 28becdf9bb..2e99711af2 100644 --- a/modules/@angular/core/src/animation/animation_sequence_player.ts +++ b/modules/@angular/core/src/animation/animation_sequence_player.ts @@ -16,7 +16,8 @@ export class AnimationSequencePlayer implements AnimationPlayer { private _onDoneFns: Function[] = []; private _onStartFns: Function[] = []; private _finished = false; - private _started: boolean = false; + private _started = false; + private _destroyed = false; public parentPlayer: AnimationPlayer = null; @@ -48,9 +49,6 @@ export class AnimationSequencePlayer implements AnimationPlayer { private _onFinish() { if (!this._finished) { this._finished = true; - if (!isPresent(this.parentPlayer)) { - this.destroy(); - } this._onDoneFns.forEach(fn => fn()); this._onDoneFns = []; } @@ -79,13 +77,18 @@ export class AnimationSequencePlayer implements AnimationPlayer { pause(): void { this._activePlayer.pause(); } restart(): void { + this.reset(); if (this._players.length > 0) { - this.reset(); this._players[0].restart(); } } - reset(): void { this._players.forEach(player => player.reset()); } + reset(): void { + this._players.forEach(player => player.reset()); + this._destroyed = false; + this._finished = false; + this._started = false; + } finish(): void { this._onFinish(); @@ -93,8 +96,11 @@ export class AnimationSequencePlayer implements AnimationPlayer { } destroy(): void { - this._onFinish(); - this._players.forEach(player => player.destroy()); + if (!this._destroyed) { + this._onFinish(); + this._players.forEach(player => player.destroy()); + this._destroyed = true; + } } setPosition(p: any /** TODO #9100 */): void { this._players[0].setPosition(p); } diff --git a/modules/@angular/core/test/animation/animation_group_player_spec.ts b/modules/@angular/core/test/animation/animation_group_player_spec.ts index bb259cea59..9269e594c1 100644 --- a/modules/@angular/core/test/animation/animation_group_player_spec.ts +++ b/modules/@angular/core/test/animation/animation_group_player_spec.ts @@ -95,7 +95,7 @@ export function main() { assertLastStatus(players[2], 'restart', true); }); - it('should finish all the players', () => { + it('should not destroy the inner the players when finished', () => { var group = new AnimationGroupPlayer(players); var completed = false; @@ -113,55 +113,36 @@ export function main() { group.finish(); - assertLastStatus(players[0], 'finish', true, -1); - assertLastStatus(players[1], 'finish', true, -1); - assertLastStatus(players[2], 'finish', true, -1); - - assertLastStatus(players[0], 'destroy', true); - assertLastStatus(players[1], 'destroy', true); - assertLastStatus(players[2], 'destroy', true); + assertLastStatus(players[0], 'finish', true); + assertLastStatus(players[1], 'finish', true); + assertLastStatus(players[2], 'finish', true); expect(completed).toBeTruthy(); }); - it('should call destroy automatically when finished if no parent player is present', () => { - var group = new AnimationGroupPlayer(players); + it('should not call destroy automatically when finished even if a parent player finishes', + () => { + var group = new AnimationGroupPlayer(players); + var parent = new AnimationGroupPlayer([group, new MockAnimationPlayer()]); - group.play(); + group.play(); - assertLastStatus(players[0], 'destroy', false); - assertLastStatus(players[1], 'destroy', false); - assertLastStatus(players[2], 'destroy', false); + assertLastStatus(players[0], 'destroy', false); + assertLastStatus(players[1], 'destroy', false); + assertLastStatus(players[2], 'destroy', false); - group.finish(); + group.finish(); - assertLastStatus(players[0], 'destroy', true); - assertLastStatus(players[1], 'destroy', true); - assertLastStatus(players[2], 'destroy', true); - }); + assertLastStatus(players[0], 'destroy', false); + assertLastStatus(players[1], 'destroy', false); + assertLastStatus(players[2], 'destroy', false); - it('should not call destroy automatically when finished if a parent player is present', () => { - var group = new AnimationGroupPlayer(players); - var parent = new AnimationGroupPlayer([group, new MockAnimationPlayer()]); + parent.finish(); - group.play(); - - assertLastStatus(players[0], 'destroy', false); - assertLastStatus(players[1], 'destroy', false); - assertLastStatus(players[2], 'destroy', false); - - group.finish(); - - assertLastStatus(players[0], 'destroy', false); - assertLastStatus(players[1], 'destroy', false); - assertLastStatus(players[2], 'destroy', false); - - parent.finish(); - - assertLastStatus(players[0], 'destroy', true); - assertLastStatus(players[1], 'destroy', true); - assertLastStatus(players[2], 'destroy', true); - }); + assertLastStatus(players[0], 'destroy', false); + assertLastStatus(players[1], 'destroy', false); + assertLastStatus(players[2], 'destroy', false); + }); it('should function without any players', () => { var group = new AnimationGroupPlayer([]); @@ -193,5 +174,31 @@ export function main() { flushMicrotasks(); expect(completed).toEqual(true); })); + + it('should not allow the player to be destroyed if it already has been destroyed unless reset', + fakeAsync(() => { + var p1 = new MockAnimationPlayer(); + var p2 = new MockAnimationPlayer(); + var innerPlayers = [p1, p2]; + + var groupPlayer = new AnimationGroupPlayer(innerPlayers); + expect(p1.log[p1.log.length - 1]).not.toContain('destroy'); + expect(p2.log[p2.log.length - 1]).not.toContain('destroy'); + + groupPlayer.destroy(); + expect(p1.log[p1.log.length - 1]).toContain('destroy'); + expect(p2.log[p2.log.length - 1]).toContain('destroy'); + + p1.log = p2.log = []; + + groupPlayer.destroy(); + expect(p1.log[p1.log.length - 1]).not.toContain('destroy'); + expect(p2.log[p2.log.length - 1]).not.toContain('destroy'); + + groupPlayer.reset(); + groupPlayer.destroy(); + expect(p1.log[p1.log.length - 1]).toContain('destroy'); + expect(p2.log[p2.log.length - 1]).toContain('destroy'); + })); }); } diff --git a/modules/@angular/core/test/animation/animation_sequence_player_spec.ts b/modules/@angular/core/test/animation/animation_sequence_player_spec.ts index 5a90f991ed..7b0cf380db 100644 --- a/modules/@angular/core/test/animation/animation_sequence_player_spec.ts +++ b/modules/@angular/core/test/animation/animation_sequence_player_spec.ts @@ -135,55 +135,36 @@ export function main() { sequence.finish(); - assertLastStatus(players[0], 'finish', true, -1); - assertLastStatus(players[1], 'finish', true, -1); - assertLastStatus(players[2], 'finish', true, -1); - - assertLastStatus(players[0], 'destroy', true); - assertLastStatus(players[1], 'destroy', true); - assertLastStatus(players[2], 'destroy', true); + assertLastStatus(players[0], 'finish', true); + assertLastStatus(players[1], 'finish', true); + assertLastStatus(players[2], 'finish', true); expect(completed).toBeTruthy(); }); - it('should call destroy automatically when finished if no parent player is present', () => { - var sequence = new AnimationSequencePlayer(players); + it('should not call destroy automatically when finished even if a parent player is present', + () => { + var sequence = new AnimationSequencePlayer(players); + var parent = new AnimationSequencePlayer([sequence, new MockAnimationPlayer()]); - sequence.play(); + sequence.play(); - assertLastStatus(players[0], 'destroy', false); - assertLastStatus(players[1], 'destroy', false); - assertLastStatus(players[2], 'destroy', false); + assertLastStatus(players[0], 'destroy', false); + assertLastStatus(players[1], 'destroy', false); + assertLastStatus(players[2], 'destroy', false); - sequence.finish(); + sequence.finish(); - assertLastStatus(players[0], 'destroy', true); - assertLastStatus(players[1], 'destroy', true); - assertLastStatus(players[2], 'destroy', true); - }); + assertLastStatus(players[0], 'destroy', false); + assertLastStatus(players[1], 'destroy', false); + assertLastStatus(players[2], 'destroy', false); - it('should not call destroy automatically when finished if a parent player is present', () => { - var sequence = new AnimationSequencePlayer(players); - var parent = new AnimationSequencePlayer([sequence, new MockAnimationPlayer()]); + parent.finish(); - sequence.play(); - - assertLastStatus(players[0], 'destroy', false); - assertLastStatus(players[1], 'destroy', false); - assertLastStatus(players[2], 'destroy', false); - - sequence.finish(); - - assertLastStatus(players[0], 'destroy', false); - assertLastStatus(players[1], 'destroy', false); - assertLastStatus(players[2], 'destroy', false); - - parent.finish(); - - assertLastStatus(players[0], 'destroy', true); - assertLastStatus(players[1], 'destroy', true); - assertLastStatus(players[2], 'destroy', true); - }); + assertLastStatus(players[0], 'destroy', false); + assertLastStatus(players[1], 'destroy', false); + assertLastStatus(players[2], 'destroy', false); + }); it('should function without any players', () => { var sequence = new AnimationSequencePlayer([]); @@ -215,5 +196,31 @@ export function main() { flushMicrotasks(); expect(completed).toEqual(true); })); + + it('should not allow the player to be destroyed if it already has been destroyed unless reset', + fakeAsync(() => { + var p1 = new MockAnimationPlayer(); + var p2 = new MockAnimationPlayer(); + var innerPlayers = [p1, p2]; + + var sequencePlayer = new AnimationSequencePlayer(innerPlayers); + expect(p1.log[p1.log.length - 1]).not.toContain('destroy'); + expect(p2.log[p2.log.length - 1]).not.toContain('destroy'); + + sequencePlayer.destroy(); + expect(p1.log[p1.log.length - 1]).toContain('destroy'); + expect(p2.log[p2.log.length - 1]).toContain('destroy'); + + p1.log = p2.log = []; + + sequencePlayer.destroy(); + expect(p1.log[p1.log.length - 1]).not.toContain('destroy'); + expect(p2.log[p2.log.length - 1]).not.toContain('destroy'); + + sequencePlayer.reset(); + sequencePlayer.destroy(); + expect(p1.log[p1.log.length - 1]).toContain('destroy'); + expect(p2.log[p2.log.length - 1]).toContain('destroy'); + })); }); } diff --git a/modules/@angular/core/testing/mock_animation_player.ts b/modules/@angular/core/testing/mock_animation_player.ts index 65b1bff4ce..7eb777ca61 100644 --- a/modules/@angular/core/testing/mock_animation_player.ts +++ b/modules/@angular/core/testing/mock_animation_player.ts @@ -7,14 +7,13 @@ */ import {AnimationPlayer} from '@angular/core'; -import {isPresent} from './facade/lang'; export class MockAnimationPlayer implements AnimationPlayer { private _onDoneFns: Function[] = []; private _onStartFns: Function[] = []; private _finished = false; private _destroyed = false; - private _started: boolean = false; + private _started = false; public parentPlayer: AnimationPlayer = null; @@ -27,9 +26,6 @@ export class MockAnimationPlayer implements AnimationPlayer { this._onDoneFns.forEach(fn => fn()); this._onDoneFns = []; - if (!isPresent(this.parentPlayer)) { - this.destroy(); - } } } @@ -56,7 +52,12 @@ export class MockAnimationPlayer implements AnimationPlayer { finish(): void { this._onFinish(); } - reset(): void { this.log.push('reset'); } + reset(): void { + this.log.push('reset'); + this._destroyed = false; + this._finished = false; + this._started = false; + } destroy(): void { if (!this._destroyed) { diff --git a/modules/@angular/platform-browser/src/dom/web_animations_player.ts b/modules/@angular/platform-browser/src/dom/web_animations_player.ts index db1eb4495a..a89c050185 100644 --- a/modules/@angular/platform-browser/src/dom/web_animations_player.ts +++ b/modules/@angular/platform-browser/src/dom/web_animations_player.ts @@ -7,7 +7,6 @@ */ import {AUTO_STYLE} from '@angular/core'; -import {isPresent} from '../facade/lang'; import {AnimationPlayer} from '../private_import_core'; import {getDOM} from './dom_adapter'; @@ -16,11 +15,12 @@ import {DomAnimatePlayer} from './dom_animate_player'; export class WebAnimationsPlayer implements AnimationPlayer { private _onDoneFns: Function[] = []; private _onStartFns: Function[] = []; - private _finished = false; - private _initialized = false; private _player: DomAnimatePlayer; - private _started: boolean = false; private _duration: number; + private _initialized = false; + private _finished = false; + private _started = false; + private _destroyed = false; public parentPlayer: AnimationPlayer = null; @@ -33,9 +33,6 @@ export class WebAnimationsPlayer implements AnimationPlayer { private _onFinish() { if (!this._finished) { this._finished = true; - if (!isPresent(this.parentPlayer)) { - this.destroy(); - } this._onDoneFns.forEach(fn => fn()); this._onDoneFns = []; } @@ -57,7 +54,7 @@ export class WebAnimationsPlayer implements AnimationPlayer { this._player = this._triggerWebAnimation(this.element, keyframes, this.options); // this is required so that the player doesn't start to animate right away - this.reset(); + this._resetDomPlayerState(); this._player.onfinish = () => this._onFinish(); } @@ -91,7 +88,14 @@ export class WebAnimationsPlayer implements AnimationPlayer { this._player.finish(); } - reset(): void { this._player.cancel(); } + reset(): void { + this._resetDomPlayerState(); + this._destroyed = false; + this._finished = false; + this._started = false; + } + + private _resetDomPlayerState() { this._player.cancel(); } restart(): void { this.reset(); @@ -101,8 +105,11 @@ export class WebAnimationsPlayer implements AnimationPlayer { hasStarted(): boolean { return this._started; } destroy(): void { - this.reset(); - this._onFinish(); + if (!this._destroyed) { + this._resetDomPlayerState(); + this._onFinish(); + this._destroyed = true; + } } get totalTime(): number { return this._duration; } diff --git a/modules/@angular/platform-browser/test/dom/web_animations_player_spec.ts b/modules/@angular/platform-browser/test/dom/web_animations_player_spec.ts index c89c1622cb..1ef487e25c 100644 --- a/modules/@angular/platform-browser/test/dom/web_animations_player_spec.ts +++ b/modules/@angular/platform-browser/test/dom/web_animations_player_spec.ts @@ -110,12 +110,12 @@ export function main() { expect(count).toEqual(2); }); - it('should destroy itself automatically if a parent player is not present', () => { + it('should not destroy itself automatically if a parent player is not present', () => { captures['cancel'] = []; player.finish(); expect(captures['finish'].length).toEqual(1); - expect(captures['cancel'].length).toEqual(1); + expect(captures['cancel'].length).toEqual(0); var next = makePlayer(); var player2 = next['player']; @@ -139,5 +139,20 @@ export function main() { player.play(); expect(calls).toEqual(1); }); + + it('should not allow the player to be cancelled via destroy if it has already been destroyed unless reset', + () => { + captures['cancel'] = []; + expect(captures['cancel'].length).toBe(0); + player.destroy(); + expect(captures['cancel'].length).toBe(1); + captures['cancel'] = []; + player.destroy(); + expect(captures['cancel'].length).toBe(0); + player.reset(); + captures['cancel'] = []; + player.destroy(); + expect(captures['cancel'].length).toBe(1); + }); }); }