fix(animations): ensure inner :leave animations do not remove node when skipped (#19532)

matsko · chuckjaz · commit ac50bd678ee3 · 2017-10-10T12:41:47.000-07:00
PR Close #19532
diff --git a/packages/animations/browser/src/render/transition_animation_engine.ts b/packages/animations/browser/src/render/transition_animation_engine.ts
@@ -141,7 +141,7 @@ export class AnimationTransitionNamespace {
     if (!triggersWithStates.hasOwnProperty(name)) {
       addClass(element, NG_TRIGGER_CLASSNAME);
       addClass(element, NG_TRIGGER_CLASSNAME + '-' + name);
-      triggersWithStates[name] = null;
+      triggersWithStates[name] = DEFAULT_STATE_VALUE;
     }
 
     return () => {
@@ -305,47 +305,87 @@ export class AnimationTransitionNamespace {
     }
   }
 
-  private _destroyInnerNodes(rootElement: any, context: any, animate: boolean = false) {
+  private _signalRemovalForInnerTriggers(rootElement: any, context: any, animate: boolean = false) {
     // emulate a leave animation for all inner nodes within this node.
     // If there are no animations found for any of the nodes then clear the cache
     // for the element.
     this._engine.driver.query(rootElement, NG_TRIGGER_SELECTOR, true).forEach(elm => {
       const namespaces = this._engine.fetchNamespacesByElement(elm);
       if (namespaces.size) {
-        namespaces.forEach(ns => ns.removeNode(elm, context, true));
+        namespaces.forEach(ns => { ns.triggerLeaveAnimation(elm, context, false, true); });
       } else {
         this.clearElementCache(elm);
       }
     });
   }
 
-  removeNode(element: any, context: any, doNotRecurse?: boolean): void {
-    const engine = this._engine;
-
-    if (!doNotRecurse && element.childElementCount) {
-      this._destroyInnerNodes(element, context, true);
-    }
-
-    const triggerStates = engine.statesByElement.get(element);
+  triggerLeaveAnimation(
+      element: any, context: any, destroyAfterComplete?: boolean,
+      defaultToFallback?: boolean): boolean {
+    const triggerStates = this._engine.statesByElement.get(element);
     if (triggerStates) {
       const players: TransitionAnimationPlayer[] = [];
       Object.keys(triggerStates).forEach(triggerName => {
         // this check is here in the event that an element is removed
         // twice (both on the host level and the component level)
         if (this._triggers[triggerName]) {
-          const player = this.trigger(element, triggerName, VOID_VALUE, false);
+          const player = this.trigger(element, triggerName, VOID_VALUE, defaultToFallback);
           if (player) {
             players.push(player);
           }
         }
       });
 
       if (players.length) {
-        engine.markElementAsRemoved(this.id, element, true, context);
-        optimizeGroupPlayer(players).onDone(() => engine.processLeaveNode(element));
-        return;
+        this._engine.markElementAsRemoved(this.id, element, true, context);
+        if (destroyAfterComplete) {
+          optimizeGroupPlayer(players).onDone(() => this._engine.processLeaveNode(element));
+        }
+        return true;
       }
     }
+    return false;
+  }
+
+  prepareLeaveAnimationListeners(element: any) {
+    const listeners = this._elementListeners.get(element);
+    if (listeners) {
+      const visitedTriggers = new Set<string>();
+      listeners.forEach(listener => {
+        const triggerName = listener.name;
+        if (visitedTriggers.has(triggerName)) return;
+        visitedTriggers.add(triggerName);
+
+        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);
+
+        this._engine.totalQueuedPlayers++;
+        this._queue.push({
+          element,
+          triggerName,
+          transition,
+          fromState,
+          toState,
+          player,
+          isFallbackTransition: true
+        });
+      });
+    }
+  }
+
+  removeNode(element: any, context: any): void {
+    const engine = this._engine;
+
+    if (element.childElementCount) {
+      this._signalRemovalForInnerTriggers(element, context, true);
+    }
+
+    // this means that a * => VOID animation was detected and kicked off
+    if (this.triggerLeaveAnimation(element, context, true)) return;
 
     // find the player that is animating and make sure that the
     // removal is delayed until that player has completed
@@ -376,33 +416,7 @@ export class AnimationTransitionNamespace {
     // during flush or will be picked up by a parent query. Either way
     // we need to fire the listeners for this element when it DOES get
     // removed (once the query parent animation is done or after flush)
-    const listeners = this._elementListeners.get(element);
-    if (listeners) {
-      const visitedTriggers = new Set<string>();
-      listeners.forEach(listener => {
-        const triggerName = listener.name;
-        if (visitedTriggers.has(triggerName)) return;
-        visitedTriggers.add(triggerName);
-
-        const trigger = this._triggers[triggerName];
-        const transition = trigger.fallbackTransition;
-        const elementStates = 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);
-
-        this._engine.totalQueuedPlayers++;
-        this._queue.push({
-          element,
-          triggerName,
-          transition,
-          fromState,
-          toState,
-          player,
-          isFallbackTransition: true
-        });
-      });
-    }
+    this.prepareLeaveAnimationListeners(element);
 
     // whether or not a parent has an animation we need to delay the deferral of the leave
     // operation until we have more information (which we do after flush() has been called)
@@ -465,7 +479,7 @@ export class AnimationTransitionNamespace {
 
   destroy(context: any) {
     this.players.forEach(p => p.destroy());
-    this._destroyInnerNodes(this.hostElement, context);
+    this._signalRemovalForInnerTriggers(this.hostElement, context);
   }
 
   elementContainsData(element: any): boolean {
@@ -668,15 +682,15 @@ export class TransitionAnimationEngine {
     }
   }
 
-  removeNode(namespaceId: string, element: any, context: any, doNotRecurse?: boolean): void {
+  removeNode(namespaceId: string, element: any, context: any): void {
     if (!isElementNode(element)) {
       this._onRemovalComplete(element, context);
       return;
     }
 
     const ns = namespaceId ? this._fetchNamespace(namespaceId) : null;
     if (ns) {
-      ns.removeNode(element, context, doNotRecurse);
+      ns.removeNode(element, context);
     } else {
       this.markElementAsRemoved(namespaceId, element, false, context);
     }
@@ -708,37 +722,39 @@ export class TransitionAnimationEngine {
 
   destroyInnerAnimations(containerElement: any) {
     let elements = this.driver.query(containerElement, NG_TRIGGER_SELECTOR, true);
-    elements.forEach(element => {
-      const players = this.playersByElement.get(element);
-      if (players) {
-        players.forEach(player => {
-          // special case for when an element is set for destruction, but hasn't started.
-          // in this situation we want to delay the destruction until the flush occurs
-          // so that any event listeners attached to the player are triggered.
-          if (player.queued) {
-            player.markedForDestroy = true;
-          } else {
-            player.destroy();
-          }
-        });
-      }
-      const stateMap = this.statesByElement.get(element);
-      if (stateMap) {
-        Object.keys(stateMap).forEach(triggerName => stateMap[triggerName] = DELETED_STATE_VALUE);
-      }
-    });
+    elements.forEach(element => this.destroyActiveAnimationsForElement(element));
 
     if (this.playersByQueriedElement.size == 0) return;
 
     elements = this.driver.query(containerElement, NG_ANIMATING_SELECTOR, true);
-    if (elements.length) {
-      elements.forEach(element => {
-        const players = this.playersByQueriedElement.get(element);
-        if (players) {
-          players.forEach(player => player.finish());
+    elements.forEach(element => this.finishActiveQueriedAnimationOnElement(element));
+  }
+
+  destroyActiveAnimationsForElement(element: any) {
+    const players = this.playersByElement.get(element);
+    if (players) {
+      players.forEach(player => {
+        // special case for when an element is set for destruction, but hasn't started.
+        // in this situation we want to delay the destruction until the flush occurs
+        // so that any event listeners attached to the player are triggered.
+        if (player.queued) {
+          player.markedForDestroy = true;
+        } else {
+          player.destroy();
         }
       });
     }
+    const stateMap = this.statesByElement.get(element);
+    if (stateMap) {
+      Object.keys(stateMap).forEach(triggerName => stateMap[triggerName] = DELETED_STATE_VALUE);
+    }
+  }
+
+  finishActiveQueriedAnimationOnElement(element: any) {
+    const players = this.playersByQueriedElement.get(element);
+    if (players) {
+      players.forEach(player => player.finish());
+    }
   }
 
   whenRenderingDone(): Promise<any> {
diff --git a/packages/core/test/animation/animation_query_integration_spec.ts b/packages/core/test/animation/animation_query_integration_spec.ts
@@ -2380,6 +2380,62 @@ export function main() {
            ]);
          });
 
+      it('should not cause a removal of inner @trigger DOM nodes when a parent animation occurs',
+         fakeAsync(() => {
+           @Component({
+             selector: 'ani-cmp',
+             template: `
+            <div @parent *ngIf="exp" class="parent">
+              this <div @child>child</div>
+            </div>
+          `,
+             animations: [
+               trigger(
+                   'parent',
+                   [
+                     transition(
+                         ':leave',
+                         [
+                           style({opacity: 0}),
+                           animate('1s', style({opacity: 1})),
+                         ]),
+                   ]),
+               trigger(
+                   'child',
+                   [
+                     transition(
+                         '* => something',
+                         [
+                           style({opacity: 0}),
+                           animate('1s', style({opacity: 1})),
+                         ]),
+                   ]),
+             ]
+           })
+           class Cmp {
+             public exp: boolean = true;
+           }
+
+           TestBed.configureTestingModule({declarations: [Cmp]});
+
+           const fixture = TestBed.createComponent(Cmp);
+           const cmp = fixture.componentInstance;
+
+           cmp.exp = true;
+           fixture.detectChanges();
+           flushMicrotasks();
+
+           cmp.exp = false;
+           fixture.detectChanges();
+           flushMicrotasks();
+
+           const players = getLog();
+           expect(players.length).toEqual(1);
+
+           const element = players[0] !.element;
+           expect(element.innerText.trim()).toMatch(/this\s+child/mg);
+         }));
+
       it('should only mark outermost *directive nodes :enter and :leave when inserts and removals occur',
          () => {
            @Component({
@@ -2619,8 +2675,8 @@ export function main() {
            fixture.detectChanges();
            flushMicrotasks();
            expect(cmp.log).toEqual([
-             'c1-start', 'c1-done', 'c2-start', 'c2-done', 'p-start', 'p-done', 'c3-start',
-             'c3-done'
+             'c1-start', 'c1-done', 'c2-start', 'c2-done', 'p-start', 'c3-start', 'c3-done',
+             'p-done'
            ]);
          }));
 
