fix(animations): properly cleanup query artificats when animation construction fails

packages/animations/browser/src/dsl/animation_transition_factory.ts

@@ -29,12 +29,16 @@ export class AnimationTransitionFactory {
   build(
       driver: AnimationDriver, element: any, currentState: any, nextState: any,
       options?: AnimationOptions,
-      subInstructions?: ElementInstructionMap): AnimationTransitionInstruction|undefined {
+      subInstructions?: ElementInstructionMap): AnimationTransitionInstruction {
     const animationOptions = mergeAnimationOptions(this.ast.options || {}, options || {});
 
     const backupStateStyles = this._stateStyles['*'] || {};
     const currentStateStyles = this._stateStyles[currentState] || backupStateStyles;
     const nextStateStyles = this._stateStyles[nextState] || backupStateStyles;
+    const queriedElements = new Set<any>();
+    const preStyleMap = new Map<any, {[prop: string]: boolean}>();
+    const postStyleMap = new Map<any, {[prop: string]: boolean}>();
+    const isRemoval = nextState === 'void';
 
     const errors: any[] = [];
     const timelines = buildAnimationTimelines(
@@ -42,13 +46,11 @@ export class AnimationTransitionFactory {
         subInstructions, errors);
 
     if (errors.length) {
-      const errorMessage = `animation building failed:\n${errors.join("\n")}`;
-      throw new Error(errorMessage);
+      return createTransitionInstruction(
+          element, this._triggerName, currentState, nextState, isRemoval, currentStateStyles,
+          nextStateStyles, [], [], preStyleMap, postStyleMap, errors);
     }
 
-    const preStyleMap = new Map<any, {[prop: string]: boolean}>();
-    const postStyleMap = new Map<any, {[prop: string]: boolean}>();
-    const queriedElements = new Set<any>();
     timelines.forEach(tl => {
       const elm = tl.element;
       const preProps = getOrSetAsInMap(preStyleMap, elm, {});
@@ -64,9 +66,8 @@ export class AnimationTransitionFactory {
 
     const queriedElementsList = iteratorToArray(queriedElements.values());
     return createTransitionInstruction(
-        element, this._triggerName, currentState, nextState, nextState === 'void',
-        currentStateStyles, nextStateStyles, timelines, queriedElementsList, preStyleMap,
-        postStyleMap);
+        element, this._triggerName, currentState, nextState, isRemoval, currentStateStyles,
+        nextStateStyles, timelines, queriedElementsList, preStyleMap, postStyleMap);
   }
 }
 

packages/animations/browser/src/dsl/animation_transition_instruction.ts

@@ -21,6 +21,7 @@ export interface AnimationTransitionInstruction extends AnimationEngineInstructi
   queriedElements: any[];
   preStyleProps: Map<any, {[prop: string]: boolean}>;
   postStyleProps: Map<any, {[prop: string]: boolean}>;
+  errors?: any[];
 }
 
 export function createTransitionInstruction(
@@ -28,7 +29,8 @@ export function createTransitionInstruction(
     isRemovalTransition: boolean, fromStyles: ɵStyleData, toStyles: ɵStyleData,
     timelines: AnimationTimelineInstruction[], queriedElements: any[],
     preStyleProps: Map<any, {[prop: string]: boolean}>,
-    postStyleProps: Map<any, {[prop: string]: boolean}>): AnimationTransitionInstruction {
+    postStyleProps: Map<any, {[prop: string]: boolean}>,
+    errors?: any[]): AnimationTransitionInstruction {
   return {
     type: AnimationTransitionInstructionType.TransitionAnimation,
     element,
@@ -41,6 +43,7 @@ export function createTransitionInstruction(
     timelines,
     queriedElements,
     preStyleProps,
-    postStyleProps
+    postStyleProps,
+    errors
   };
 }

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

@@ -745,6 +745,7 @@ export class TransitionAnimationEngine {
     const queriedElements = new Map<any, TransitionAnimationPlayer[]>();
     const allPreStyleElements = new Map<any, Set<string>>();
     const allPostStyleElements = new Map<any, Set<string>>();
+    const cleanupFns: Function[] = [];
 
     const bodyNode = getBodyNode();
     const allEnterNodes: any[] = this.collectedEnterElements.length ?
@@ -772,10 +773,21 @@ export class TransitionAnimationEngine {
       }
     }
 
+    cleanupFns.push(() => {
+      allEnterNodes.forEach(element => removeClass(element, ENTER_CLASSNAME));
+      allLeaveNodes.forEach(element => {
+        removeClass(element, LEAVE_CLASSNAME);
+        this.processLeaveNode(element);
+      });
+    });
+
+    const allPlayers: TransitionAnimationPlayer[] = [];
+    const erroneousTransitions: AnimationTransitionInstruction[] = [];
     for (let i = this._namespaceList.length - 1; i >= 0; i--) {
       const ns = this._namespaceList[i];
       ns.drainQueuedTransitions(microtaskId).forEach(entry => {
         const player = entry.player;
+        allPlayers.push(player);
 
         const element = entry.element;
         if (!bodyNode || !this.driver.containsElement(bodyNode, element)) {
@@ -783,8 +795,11 @@ export class TransitionAnimationEngine {
           return;
         }
 
-        const instruction = this._buildInstruction(entry, subTimelines);
-        if (!instruction) return;
+        const instruction = this._buildInstruction(entry, subTimelines) !;
+        if (instruction.errors && instruction.errors.length) {
+          erroneousTransitions.push(instruction);
+          return;
+        }
 
         // if a unmatched transition is queued to go then it SHOULD NOT render
         // an animation and cancel the previously running animations.
@@ -833,6 +848,18 @@ export class TransitionAnimationEngine {
       });
     }
 
+    if (erroneousTransitions.length) {
+      let msg = `Unable to process animations due to the following failed trigger transitions\n`;
+      erroneousTransitions.forEach(instruction => {
+        msg += `@${instruction.triggerName} has failed due to:\n`;
+        instruction.errors !.forEach(error => { msg += `- ${error}\n`; });
+      });
+
+      cleanupFns.forEach(fn => fn());
+      allPlayers.forEach(player => player.destroy());
+      throw new Error(msg);
+    }
+
     // these can only be detected here since we have a map of all the elements
     // that have animations attached to them...
     const enterNodesWithoutAnimations: any[] = [];
@@ -937,6 +964,7 @@ export class TransitionAnimationEngine {
     for (let i = 0; i < allLeaveNodes.length; i++) {
       const element = allLeaveNodes[i];
       const details = element[REMOVAL_FLAG] as ElementAnimationState;
+      removeClass(element, LEAVE_CLASSNAME);
 
       // this means the element has a removal animation that is being
       // taken care of and therefore the inner elements will hang around
@@ -969,6 +997,9 @@ export class TransitionAnimationEngine {
       }
     }
 
+    // this is required so the cleanup method doesn't remove them
+    allLeaveNodes.length = 0;
+
     rootPlayers.forEach(player => {
       this.players.push(player);
       player.onDone(() => {
@@ -980,8 +1011,7 @@ export class TransitionAnimationEngine {
       player.play();
     });
 
-    allEnterNodes.forEach(element => removeClass(element, ENTER_CLASSNAME));
-
+    cleanupFns.forEach(fn => fn());
     return rootPlayers;
   }
 

packages/core/test/animation/animation_integration_spec.ts

@@ -1826,6 +1826,64 @@ export function main() {
               /only state\(\) and transition\(\) definitions can sit inside of a trigger\(\)/);
     });
 
+    it('should combine multiple errors together into one exception when an animation fails to be built',
+       () => {
+         @Component({
+           selector: 'if-cmp',
+           template: `
+          <div [@foo]="fooExp" [@bar]="barExp"></div>
+        `,
+           animations: [
+             trigger(
+                 'foo',
+                 [
+                   transition(':enter', []),
+                   transition(
+                       '* => *',
+                       [
+                         query('foo', animate(1000, style({background: 'red'}))),
+                       ]),
+                 ]),
+             trigger(
+                 'bar',
+                 [
+                   transition(':enter', []),
+                   transition(
+                       '* => *',
+                       [
+                         query('bar', animate(1000, style({background: 'blue'}))),
+                       ]),
+                 ]),
+           ]
+         })
+         class Cmp {
+           fooExp: any = false;
+           barExp: any = false;
+         }
+
+         TestBed.configureTestingModule({declarations: [Cmp]});
+
+         const engine = TestBed.get(ɵAnimationEngine);
+         const fixture = TestBed.createComponent(Cmp);
+         const cmp = fixture.componentInstance;
+         fixture.detectChanges();
+
+         cmp.fooExp = 'go';
+         cmp.barExp = 'go';
+
+         let errorMsg: string = '';
+         try {
+           fixture.detectChanges();
+         } catch (e) {
+           errorMsg = e.message;
+         }
+
+         expect(errorMsg).toMatch(/@foo has failed due to:/);
+         expect(errorMsg).toMatch(/`query\("foo"\)` returned zero elements/);
+         expect(errorMsg).toMatch(/@bar has failed due to:/);
+         expect(errorMsg).toMatch(/`query\("bar"\)` returned zero elements/);
+       });
+
     it('should not throw an error if styles overlap in separate transitions', () => {
       @Component({
         selector: 'if-cmp',

packages/core/test/animation/animation_query_integration_spec.ts

@@ -8,6 +8,7 @@
 import {AUTO_STYLE, AnimationPlayer, animate, animateChild, query, stagger, state, style, transition, trigger, ɵAnimationGroupPlayer as AnimationGroupPlayer} from '@angular/animations';
 import {AnimationDriver, ɵAnimationEngine} from '@angular/animations/browser';
 import {matchesElement} from '@angular/animations/browser/src/render/shared';
+import {ENTER_CLASSNAME, LEAVE_CLASSNAME} from '@angular/animations/browser/src/util';
 import {MockAnimationDriver, MockAnimationPlayer} from '@angular/animations/browser/testing';
 import {CommonModule} from '@angular/common';
 import {Component, HostBinding, ViewChild} from '@angular/core';
@@ -749,6 +750,69 @@ export function main() {
         });
       });
 
+      it('should cleanup :enter and :leave artifacts from nodes when any animation sequences fail to be built',
+         () => {
+           @Component({
+             selector: 'ani-cmp',
+             template: `
+            <div [@myAnimation]="items.length" class="parent" #container>
+              <div *ngFor="let item of items" class="child">
+                {{ item }}
+              </div>
+              <div *ngIf="items.length == 0" class="child">Leave!</div>
+            </div>
+          `,
+             animations: [
+               trigger(
+                   'myAnimation',
+                   [
+                     transition('* => 0', []),
+                     transition(
+                         '* => *',
+                         [
+                           query(
+                               '.child:enter',
+                               [
+                                 style({opacity: 0}),
+                                 animate(1000, style({opacity: 1})),
+                               ]),
+                           query(
+                               '.incorrect-child:leave',
+                               [
+                                 animate(1000, style({opacity: 0})),
+                               ]),
+                         ]),
+                   ]),
+             ]
+           })
+           class Cmp {
+             @ViewChild('container') public container: any;
+             public items: any[] = [];
+           }
+
+           TestBed.configureTestingModule({declarations: [Cmp]});
+
+           const engine = TestBed.get(ɵAnimationEngine);
+           const fixture = TestBed.createComponent(Cmp);
+           const cmp = fixture.componentInstance;
+
+           cmp.items = [];
+           fixture.detectChanges();
+
+           cmp.items = [0, 1, 2, 3, 4];
+
+           expect(() => { fixture.detectChanges(); }).toThrow();
+
+           const children = cmp.container.nativeElement.querySelectorAll('.child');
+           expect(children.length).toEqual(5);
+
+           for (let i = 0; i < children.length; i++) {
+             let child = children[i];
+             expect(child.classList.contains(ENTER_CLASSNAME)).toBe(false);
+             expect(child.classList.contains(LEAVE_CLASSNAME)).toBe(false);
+           }
+         });
+
       it('should find elements that have been removed via :leave', () => {
         @Component({
           selector: 'ani-cmp',