fix(animations): always fire callbacks even for noop animations

packages/animations/browser/src/render/dom_animation_engine.ts

@@ -43,6 +43,8 @@ export class DomAnimationEngine {
   private _triggers: {[triggerName: string]: AnimationTrigger} = Object.create(null);
   private _triggerListeners = new Map<any, TriggerListenerTuple[]>();
 
+  private _pendingListenerRemovals = new Map<any, TriggerListenerTuple[]>();
+
   constructor(private _driver: AnimationDriver, private _normalizer: AnimationStyleNormalizer) {}
 
   get queuedPlayers(): AnimationPlayer[] {
@@ -83,6 +85,13 @@ export class DomAnimationEngine {
         return;
       }
     }
+
+    // this means that there are no animations to take on this
+    // leave operation therefore we should fire the done|start callbacks
+    if (this._triggerListeners.has(element)) {
+      element[MARKED_FOR_REMOVAL] = true;
+      this._queuedRemovals.set(element, () => {});
+    }
     domFn();
   }
 
@@ -128,13 +137,27 @@ export class DomAnimationEngine {
     const tuple = <TriggerListenerTuple>{triggerName: eventName, phase: eventPhase, callback};
     elementListeners.push(tuple);
     return () => {
-      const index = elementListeners.indexOf(tuple);
-      if (index >= 0) {
-        elementListeners.splice(index, 1);
-      }
+      // this is queued up in the event that a removal animation is set
+      // to fire on the element (the listeners need to be set during flush)
+      getOrSetAsInMap(this._pendingListenerRemovals, element, []).push(tuple);
     };
   }
 
+  private _clearPendingListenerRemovals() {
+    this._pendingListenerRemovals.forEach((tuples: TriggerListenerTuple[], element: any) => {
+      const elementListeners = this._triggerListeners.get(element);
+      if (elementListeners) {
+        tuples.forEach(tuple => {
+          const index = elementListeners.indexOf(tuple);
+          if (index >= 0) {
+            elementListeners.splice(index, 1);
+          }
+        });
+      }
+    });
+    this._pendingListenerRemovals.clear();
+  }
+
   private _onRemovalTransition(element: any): AnimationPlayer[] {
     // when a parent animation is set to trigger a removal we want to
     // find all of the children that are currently animating and clear
@@ -293,13 +316,6 @@ export class DomAnimationEngine {
         if (parent[MARKED_FOR_REMOVAL]) continue parentLoop;
       }
 
-      // if a removal exists for the given element then we need cancel
-      // all the queued players so that a proper removal animation can go
-      if (this._queuedRemovals.has(element)) {
-        player.destroy();
-        continue;
-      }
-
       const listeners = this._triggerListeners.get(element);
       if (listeners) {
         listeners.forEach(tuple => {
@@ -309,6 +325,13 @@ export class DomAnimationEngine {
         });
       }
 
+      // if a removal exists for the given element then we need cancel
+      // all the queued players so that a proper removal animation can go
+      if (this._queuedRemovals.has(element)) {
+        player.destroy();
+        continue;
+      }
+
       this._markPlayerAsActive(element, player);
 
       // in the event that an animation throws an error then we do
@@ -321,6 +344,18 @@ export class DomAnimationEngine {
   }
 
   flush() {
+    const leaveListeners = new Map<any, TriggerListenerTuple[]>();
+    this._queuedRemovals.forEach((callback, element) => {
+      const tuple = this._pendingListenerRemovals.get(element);
+      if (tuple) {
+        leaveListeners.set(element, tuple);
+        this._pendingListenerRemovals.delete(element);
+      }
+    });
+
+    this._clearPendingListenerRemovals();
+    this._pendingListenerRemovals = leaveListeners;
+
     this._flushQueuedAnimations();
 
     let flushAgain = false;
@@ -355,11 +390,15 @@ export class DomAnimationEngine {
         const stateDetails = this._elementTriggerStates.get(element);
         if (stateDetails) {
           Object.keys(stateDetails).forEach(triggerName => {
+            flushAgain = true;
             const oldValue = stateDetails[triggerName];
             const instruction = this._triggers[triggerName].matchTransition(oldValue, 'void');
             if (instruction) {
               players.push(this.animateTransition(element, instruction));
-              flushAgain = true;
+            } else {
+              const event = makeAnimationEvent(element, triggerName, oldValue, 'void', '', 0);
+              const player = new NoopAnimationPlayer();
+              this._queuePlayer(element, triggerName, player, event);
             }
           });
         }
@@ -378,6 +417,7 @@ export class DomAnimationEngine {
     // this means that one or more leave animations were detected
     if (flushAgain) {
       this._flushQueuedAnimations();
+      this._clearPendingListenerRemovals();
     }
   }
 }

packages/animations/src/players/animation_player.ts

@@ -39,7 +39,7 @@ export class NoopAnimationPlayer implements AnimationPlayer {
   private _destroyed = false;
   private _finished = false;
   public parentPlayer: AnimationPlayer = null;
-  constructor() { scheduleMicroTask(() => this._onFinish()); }
+  constructor() {}
   private _onFinish() {
     if (!this._finished) {
       this._finished = true;
@@ -54,17 +54,24 @@ export class NoopAnimationPlayer implements AnimationPlayer {
   init(): void {}
   play(): void {
     if (!this.hasStarted()) {
-      this._onStartFns.forEach(fn => fn());
-      this._onStartFns = [];
+      scheduleMicroTask(() => this._onFinish());
+      this._onStart();
     }
     this._started = true;
   }
+  private _onStart() {
+    this._onStartFns.forEach(fn => fn());
+    this._onStartFns = [];
+  }
   pause(): void {}
   restart(): void {}
   finish(): void { this._onFinish(); }
   destroy(): void {
     if (!this._destroyed) {
       this._destroyed = true;
+      if (!this.hasStarted()) {
+        this._onStart();
+      }
       this.finish();
       this._onDestroyFns.forEach(fn => fn());
       this._onDestroyFns = [];

packages/animations/test/animation_player_spec.ts

@@ -0,0 +1,44 @@
+/**
+ * @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 {fakeAsync} from '@angular/core/testing';
+
+import {flushMicrotasks} from '../../core/testing/src/fake_async';
+import {NoopAnimationPlayer} from '../src/players/animation_player';
+
+export function main() {
+  describe('NoopAnimationPlayer', function() {
+    it('should finish after the next microtask once started', fakeAsync(() => {
+         const log: string[] = [];
+
+         const player = new NoopAnimationPlayer();
+         player.onStart(() => log.push('started'));
+         player.onDone(() => log.push('done'));
+         flushMicrotasks();
+
+         expect(log).toEqual([]);
+         player.play();
+         expect(log).toEqual(['started']);
+
+         flushMicrotasks();
+         expect(log).toEqual(['started', 'done']);
+       }));
+
+    it('should fire all callbacks when destroyed', () => {
+      const log: string[] = [];
+
+      const player = new NoopAnimationPlayer();
+      player.onStart(() => log.push('started'));
+      player.onDone(() => log.push('done'));
+      player.onDestroy(() => log.push('destroy'));
+      expect(log).toEqual([]);
+
+      player.destroy();
+      expect(log).toEqual(['started', 'done', 'destroy']);
+    });
+  });
+}

packages/core/test/animation/animation_integration_spec.ts

@@ -6,14 +6,14 @@
  * found in the LICENSE file at https://angular.io/license
  */
 import {AUTO_STYLE, AnimationEvent, animate, keyframes, state, style, transition, trigger} from '@angular/animations';
-import {AnimationDriver, ɵAnimationEngine} from '@angular/animations/browser';
+import {AnimationDriver, ɵAnimationEngine, ɵNoopAnimationDriver} from '@angular/animations/browser';
 import {MockAnimationDriver, MockAnimationPlayer} from '@angular/animations/browser/testing';
 import {Component, HostBinding, HostListener, RendererFactory2, ViewChild} from '@angular/core';
 import {ɵDomRendererFactory2} from '@angular/platform-browser';
 import {BrowserAnimationsModule} from '@angular/platform-browser/animations';
 import {getDOM} from '@angular/platform-browser/src/dom/dom_adapter';
 
-import {TestBed} from '../../testing';
+import {TestBed, fakeAsync, flushMicrotasks} from '../../testing';
 
 export function main() {
   // these tests are only mean't to be run within the DOM (for now)
@@ -551,6 +551,80 @@ export function main() {
            expect(cmp.event.fromState).toEqual('void');
            expect(cmp.event.toState).toEqual('TRUE');
          });
+
+      it('should always fire callbacks even when a transition is not detected', fakeAsync(() => {
+           @Component({
+             selector: 'my-cmp',
+             template: `
+              <div [@myAnimation]="exp" (@myAnimation.start)="callback($event)" (@myAnimation.done)="callback($event)"></div>
+            `,
+             animations: [trigger('myAnimation', [])]
+           })
+           class Cmp {
+             exp: string;
+             log: any[] = [];
+             callback = (event: any) => { this.log.push(`${event.phaseName} => ${event.toState}`); }
+           }
+
+           TestBed.configureTestingModule({
+             providers: [{provide: AnimationDriver, useClass: ɵNoopAnimationDriver}],
+             declarations: [Cmp]
+           });
+
+           const fixture = TestBed.createComponent(Cmp);
+           const cmp = fixture.componentInstance;
+
+           cmp.exp = 'a';
+           fixture.detectChanges();
+           flushMicrotasks();
+           expect(cmp.log).toEqual(['start => a', 'done => a']);
+
+           cmp.log = [];
+           cmp.exp = 'b';
+           fixture.detectChanges();
+           flushMicrotasks();
+
+           expect(cmp.log).toEqual(['start => b', 'done => b']);
+         }));
+
+      it('should fire callback events for leave animations', fakeAsync(() => {
+           @Component({
+             selector: 'my-cmp',
+             template: `
+              <div *ngIf="exp" @myAnimation (@myAnimation.start)="callback($event)" (@myAnimation.done)="callback($event)"></div>
+            `,
+             animations: [trigger('myAnimation', [])]
+           })
+           class Cmp {
+             exp: boolean = false;
+             log: any[] = [];
+             callback = (event: any) => {
+               const state = event.toState || '_default_';
+               this.log.push(`${event.phaseName} => ${state}`);
+             }
+           }
+
+           TestBed.configureTestingModule({
+             providers: [{provide: AnimationDriver, useClass: ɵNoopAnimationDriver}],
+             declarations: [Cmp]
+           });
+
+           const fixture = TestBed.createComponent(Cmp);
+           const cmp = fixture.componentInstance;
+
+           cmp.exp = true;
+           fixture.detectChanges();
+           flushMicrotasks();
+           expect(cmp.log).toEqual(['start => _default_', 'done => _default_']);
+
+           cmp.log = [];
+
+           cmp.exp = false;
+           fixture.detectChanges();
+           flushMicrotasks();
+
+           expect(cmp.log).toEqual(['start => void', 'done => void']);
+         }));
     });
 
     describe('errors for not using the animation module', () => {