From 36b78e9502b224962557be4e746cdb02c5219938 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Thu, 23 Feb 2017 09:41:00 -0800 Subject: [PATCH] refactor(animations): defer the noop engine's event deregistration --- modules/@angular/core/src/view/services.ts | 3 - .../src/render/noop_animation_engine.ts | 18 ++++-- .../test/noop_animation_engine_spec.ts | 62 ++++++++++++------- .../noop_browser_animation_module_spec.ts | 43 +++++++++++++ 4 files changed, 96 insertions(+), 30 deletions(-) diff --git a/modules/@angular/core/src/view/services.ts b/modules/@angular/core/src/view/services.ts index c767d3d4c0..2db987597c 100644 --- a/modules/@angular/core/src/view/services.ts +++ b/modules/@angular/core/src/view/services.ts @@ -162,9 +162,6 @@ function debugSetCurrentNode(view: ViewData, nodeIndex: number) { } function debugHandleEvent(view: ViewData, nodeIndex: number, eventName: string, event: any) { - if (view.state & ViewState.Destroyed) { - throw viewDestroyedError(DebugAction[_currentAction]); - } debugSetCurrentNode(view, nodeIndex); return callWithDebugContext( DebugAction.handleEvent, view.def.handleEvent, null, [view, nodeIndex, eventName, event]); diff --git a/modules/@angular/platform-browser/animations/src/render/noop_animation_engine.ts b/modules/@angular/platform-browser/animations/src/render/noop_animation_engine.ts index b2107512e9..7ec0b1ead4 100644 --- a/modules/@angular/platform-browser/animations/src/render/noop_animation_engine.ts +++ b/modules/@angular/platform-browser/animations/src/render/noop_animation_engine.ts @@ -14,6 +14,7 @@ interface ListenerTuple { eventPhase: string; triggerName: string; callback: (event: any) => any; + doRemove?: boolean; } interface ChangeTuple { @@ -82,12 +83,7 @@ export class NoopAnimationEngine extends AnimationEngine { const tuple = {triggerName: eventName, eventPhase, callback}; listeners.push(tuple); - return () => { - const index = listeners.indexOf(tuple); - if (index >= 0) { - listeners.splice(index, 1); - } - }; + return () => tuple.doRemove = true; } flush(): void { @@ -134,6 +130,16 @@ export class NoopAnimationEngine extends AnimationEngine { } }); + // remove all the listeners after everything is complete + Array.from(this._listeners.keys()).forEach(element => { + const listenersToKeep = this._listeners.get(element).filter(l => !l.doRemove); + if (listenersToKeep.length) { + this._listeners.set(element, listenersToKeep); + } else { + this._listeners.delete(element); + } + }); + onStartCallbacks.forEach(fn => fn()); onDoneCallbacks.forEach(fn => fn()); this._flaggedRemovals.clear(); diff --git a/modules/@angular/platform-browser/animations/test/noop_animation_engine_spec.ts b/modules/@angular/platform-browser/animations/test/noop_animation_engine_spec.ts index fbe09d8df9..6b3371f07c 100644 --- a/modules/@angular/platform-browser/animations/test/noop_animation_engine_spec.ts +++ b/modules/@angular/platform-browser/animations/test/noop_animation_engine_spec.ts @@ -121,33 +121,53 @@ export function main() { expect(captures).toEqual([]); }); - it('should deregister a listener when the return function is called', () => { + it('should deregister a listener when the return function is called, but only after flush', + () => { + const engine = new NoopAnimationEngine(); + const elm = {}; + + const fn1 = engine.listen(elm, 'trig1', 'start', capture('trig1-start')); + const fn2 = engine.listen(elm, 'trig2', 'done', capture('trig2-done')); + + engine.setProperty(elm, 'trig1', 'value1'); + engine.setProperty(elm, 'trig2', 'value2'); + engine.flush(); + expect(captures).toEqual(['trig1-start', 'trig2-done']); + + captures = []; + engine.setProperty(elm, 'trig1', 'value3'); + engine.setProperty(elm, 'trig2', 'value4'); + + fn1(); + engine.flush(); + expect(captures).toEqual(['trig1-start', 'trig2-done']); + + captures = []; + engine.setProperty(elm, 'trig1', 'value5'); + engine.setProperty(elm, 'trig2', 'value6'); + + fn2(); + engine.flush(); + expect(captures).toEqual(['trig2-done']); + + captures = []; + engine.setProperty(elm, 'trig1', 'value7'); + engine.setProperty(elm, 'trig2', 'value8'); + engine.flush(); + expect(captures).toEqual([]); + }); + + it('should fire a removal listener even if the listener is deregistered prior to flush', () => { const engine = new NoopAnimationEngine(); const elm = {}; - const fn1 = engine.listen(elm, 'trig1', 'start', capture('trig1-start')); - const fn2 = engine.listen(elm, 'trig2', 'done', capture('trig2-done')); + const fn = engine.listen(elm, 'trig', 'start', capture('removal listener')); + fn(); - engine.setProperty(elm, 'trig1', 'value1'); - engine.setProperty(elm, 'trig2', 'value2'); + engine.onRemove(elm, capture('dom removal')); engine.flush(); - expect(captures).toEqual(['trig1-start', 'trig2-done']); - captures = []; - engine.setProperty(elm, 'trig1', 'value3'); - engine.setProperty(elm, 'trig2', 'value4'); - - fn1(); - engine.flush(); - expect(captures).toEqual(['trig2-done']); - - captures = []; - engine.setProperty(elm, 'trig1', 'value5'); - engine.setProperty(elm, 'trig2', 'value6'); - - fn2(); - engine.flush(); - expect(captures).toEqual([]); + expect(captures).toEqual(['dom removal', 'removal listener']); }); describe('styling', () => { diff --git a/modules/@angular/platform-browser/animations/test/noop_browser_animation_module_spec.ts b/modules/@angular/platform-browser/animations/test/noop_browser_animation_module_spec.ts index 6588178a28..1f6633de12 100644 --- a/modules/@angular/platform-browser/animations/test/noop_browser_animation_module_spec.ts +++ b/modules/@angular/platform-browser/animations/test/noop_browser_animation_module_spec.ts @@ -63,5 +63,48 @@ export function main() { async(); }); }); + + it('should handle leave animation callbacks even if the element is destroyed in the process', + (async) => { + @Component({ + selector: 'my-cmp', + template: + '
', + animations: [trigger( + 'myAnimation', + [transition( + ':leave', [style({'opacity': '0'}), animate(500, style({'opacity': '1'}))])])], + }) + class Cmp { + exp: any; + startEvent: any; + doneEvent: any; + onStart(event: any) { this.startEvent = event; } + onDone(event: any) { this.doneEvent = event; } + } + + TestBed.configureTestingModule({declarations: [Cmp]}); + const engine = TestBed.get(ɵAnimationEngine); + const fixture = TestBed.createComponent(Cmp); + const cmp = fixture.componentInstance; + + cmp.exp = true; + fixture.detectChanges(); + cmp.startEvent = null; + cmp.doneEvent = null; + + cmp.exp = false; + fixture.detectChanges(); + + fixture.whenStable().then(() => { + expect(cmp.startEvent.triggerName).toEqual('myAnimation'); + expect(cmp.startEvent.phaseName).toEqual('start'); + expect(cmp.startEvent.toState).toEqual('void'); + expect(cmp.doneEvent.triggerName).toEqual('myAnimation'); + expect(cmp.doneEvent.phaseName).toEqual('done'); + expect(cmp.doneEvent.toState).toEqual('void'); + async(); + }); + }); }); }