From d035175cdb7e053be56acb05852a714d94321d33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Thu, 12 Oct 2017 15:19:21 -0700 Subject: [PATCH] fix(animations): ensure inner :leave animations do not remove node when skipped (#19532) (#19693) PR Close #19693 --- .../src/render/transition_animation_engine.ts | 154 ++++++++++-------- .../animation_query_integration_spec.ts | 60 ++++++- 2 files changed, 143 insertions(+), 71 deletions(-) diff --git a/packages/animations/browser/src/render/transition_animation_engine.ts b/packages/animations/browser/src/render/transition_animation_engine.ts index ce0ca4231f..a217254453 100644 --- 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,35 +305,31 @@ 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); } @@ -341,11 +337,55 @@ export class AnimationTransitionNamespace { }); 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(); + 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(); - 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,7 +682,7 @@ 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; @@ -676,7 +690,7 @@ export class TransitionAnimationEngine { 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 { diff --git a/packages/core/test/animation/animation_query_integration_spec.ts b/packages/core/test/animation/animation_query_integration_spec.ts index 08ab6edb46..c694c2afe5 100644 --- 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: ` +
+ this
child
+
+ `, + 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' ]); }));