From 5a582a8afdd8c8e1567abef72623871164f9ca42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Mon, 14 Jan 2019 15:36:08 -0800 Subject: [PATCH] fix(ivy): ensure element removal triggers host removal animations (#28162) Prior to this fix Ivy would not execute any animation triggers that exist as host bindings on an element if it is removed by the parent template. PR Close #28162 --- .../src/render/animation_engine_next.ts | 4 +- .../src/render/transition_animation_engine.ts | 24 ++- .../transition_animation_engine_spec.ts | 2 +- packages/core/src/render/api.ts | 4 +- .../core/src/render3/interfaces/renderer.ts | 2 +- .../core/src/render3/node_manipulation.ts | 18 +- .../animation/animation_integration_spec.ts | 189 +++++++++--------- .../animation_query_integration_spec.ts | 2 +- .../animations/src/animation_renderer.ts | 4 +- tools/public_api_guard/core/core.d.ts | 2 +- 10 files changed, 127 insertions(+), 124 deletions(-) diff --git a/packages/animations/browser/src/render/animation_engine_next.ts b/packages/animations/browser/src/render/animation_engine_next.ts index 753d8b52ae..fe458b7611 100644 --- a/packages/animations/browser/src/render/animation_engine_next.ts +++ b/packages/animations/browser/src/render/animation_engine_next.ts @@ -66,8 +66,8 @@ export class AnimationEngine { this._transitionEngine.insertNode(namespaceId, element, parent, insertBefore); } - onRemove(namespaceId: string, element: any, context: any): void { - this._transitionEngine.removeNode(namespaceId, element, context); + onRemove(namespaceId: string, element: any, context: any, isHostElement?: boolean): void { + this._transitionEngine.removeNode(namespaceId, element, isHostElement || false, context); } disableAnimations(element: any, disable: boolean) { diff --git a/packages/animations/browser/src/render/transition_animation_engine.ts b/packages/animations/browser/src/render/transition_animation_engine.ts index 0feacd5de4..b44f54446b 100644 --- a/packages/animations/browser/src/render/transition_animation_engine.ts +++ b/packages/animations/browser/src/render/transition_animation_engine.ts @@ -708,17 +708,23 @@ export class TransitionAnimationEngine { } } - removeNode(namespaceId: string, element: any, context: any): void { - if (!isElementNode(element)) { - this._onRemovalComplete(element, context); - return; - } + removeNode(namespaceId: string, element: any, isHostElement: boolean, context: any): void { + if (isElementNode(element)) { + const ns = namespaceId ? this._fetchNamespace(namespaceId) : null; + if (ns) { + ns.removeNode(element, context); + } else { + this.markElementAsRemoved(namespaceId, element, false, context); + } - const ns = namespaceId ? this._fetchNamespace(namespaceId) : null; - if (ns) { - ns.removeNode(element, context); + if (isHostElement) { + const hostNS = this.namespacesByHostElement.get(element); + if (hostNS && hostNS.id !== namespaceId) { + hostNS.removeNode(element, context); + } + } } else { - this.markElementAsRemoved(namespaceId, element, false, context); + this._onRemovalComplete(element, context); } } diff --git a/packages/animations/browser/test/render/transition_animation_engine_spec.ts b/packages/animations/browser/test/render/transition_animation_engine_spec.ts index 8090f81e00..1ad40b001f 100644 --- a/packages/animations/browser/test/render/transition_animation_engine_spec.ts +++ b/packages/animations/browser/test/render/transition_animation_engine_spec.ts @@ -111,7 +111,7 @@ const DEFAULT_NAMESPACE_ID = 'id'; expect(engine.elementContainsData(DEFAULT_NAMESPACE_ID, element)).toBeTruthy(); - engine.removeNode(DEFAULT_NAMESPACE_ID, element, true); + engine.removeNode(DEFAULT_NAMESPACE_ID, element, true, true); engine.flush(); expect(engine.elementContainsData(DEFAULT_NAMESPACE_ID, element)).toBeTruthy(); diff --git a/packages/core/src/render/api.ts b/packages/core/src/render/api.ts index 8bc0147e78..0eb61f7d9b 100644 --- a/packages/core/src/render/api.ts +++ b/packages/core/src/render/api.ts @@ -267,8 +267,10 @@ export abstract class Renderer2 { * Implement this callback to remove a child node from the host element's DOM. * @param parent The parent node. * @param oldChild The child node to remove. + * @param isHostElement Optionally signal to the renderer whether this element is a host element + * or not */ - abstract removeChild(parent: any, oldChild: any): void; + abstract removeChild(parent: any, oldChild: any, isHostElement?: boolean): void; /** * Implement this callback to prepare an element to be bootstrapped * as a root element, and return the element instance. diff --git a/packages/core/src/render3/interfaces/renderer.ts b/packages/core/src/render3/interfaces/renderer.ts index 071fd958cf..a247204bc6 100644 --- a/packages/core/src/render3/interfaces/renderer.ts +++ b/packages/core/src/render3/interfaces/renderer.ts @@ -74,7 +74,7 @@ export interface ProceduralRenderer3 { destroyNode?: ((node: RNode) => void)|null; appendChild(parent: RElement, newChild: RNode): void; insertBefore(parent: RNode, newChild: RNode, refChild: RNode|null): void; - removeChild(parent: RElement, oldChild: RNode): void; + removeChild(parent: RElement, oldChild: RNode, isHostElement?: boolean): void; selectRootElement(selectorOrNode: string|any): RElement; parentNode(node: RNode): RElement|null; diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index f96f6e0638..62be5b49b3 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -14,7 +14,7 @@ import {unusedValueExportToPlacateAjd as unused3} from './interfaces/projection' import {ProceduralRenderer3, RComment, RElement, RNode, RText, Renderer3, isProceduralRenderer, unusedValueExportToPlacateAjd as unused4} from './interfaces/renderer'; import {CLEANUP, CONTAINER_INDEX, FLAGS, HEADER_OFFSET, HOST_NODE, HookData, LView, LViewFlags, NEXT, PARENT, QUERIES, RENDERER, TVIEW, unusedValueExportToPlacateAjd as unused5} from './interfaces/view'; import {assertNodeType} from './node_assert'; -import {findComponentView, getNativeByTNode, isLContainer, isRootView, readElementValue, renderStringify} from './util'; +import {findComponentView, getNativeByTNode, isComponent, isLContainer, isRootView, readElementValue, renderStringify} from './util'; const unusedValueToPlacateAjd = unused1 + unused2 + unused3 + unused4 + unused5; @@ -84,15 +84,16 @@ function walkTNodeTree( let nextTNode: TNode|null = null; if (tNode.type === TNodeType.Element) { executeNodeAction( - action, renderer, renderParent, getNativeByTNode(tNode, currentView), beforeNode); + action, renderer, renderParent, getNativeByTNode(tNode, currentView), tNode, beforeNode); const nodeOrContainer = currentView[tNode.index]; if (isLContainer(nodeOrContainer)) { // This element has an LContainer, and its comment needs to be handled - executeNodeAction(action, renderer, renderParent, nodeOrContainer[NATIVE], beforeNode); + executeNodeAction( + action, renderer, renderParent, nodeOrContainer[NATIVE], tNode, beforeNode); } } else if (tNode.type === TNodeType.Container) { const lContainer = currentView ![tNode.index] as LContainer; - executeNodeAction(action, renderer, renderParent, lContainer[NATIVE], beforeNode); + executeNodeAction(action, renderer, renderParent, lContainer[NATIVE], tNode, beforeNode); if (lContainer[VIEWS].length) { currentView = lContainer[VIEWS][0]; @@ -166,11 +167,11 @@ function walkTNodeTree( */ function executeNodeAction( action: WalkTNodeTreeAction, renderer: Renderer3, parent: RElement | null, - node: RComment | RElement | RText, beforeNode?: RNode | null) { + node: RComment | RElement | RText, tNode: TNode, beforeNode?: RNode | null) { if (action === WalkTNodeTreeAction.Insert) { nativeInsertBefore(renderer, parent !, node, beforeNode || null); } else if (action === WalkTNodeTreeAction.Detach) { - nativeRemoveChild(renderer, parent !, node); + nativeRemoveChild(renderer, parent !, node, isComponent(tNode)); } else if (action === WalkTNodeTreeAction.Destroy) { ngDevMode && ngDevMode.rendererDestroyNode++; (renderer as ProceduralRenderer3).destroyNode !(node); @@ -550,8 +551,9 @@ export function nativeInsertBefore( /** * Removes a native child node from a given native parent node. */ -export function nativeRemoveChild(renderer: Renderer3, parent: RElement, child: RNode): void { - isProceduralRenderer(renderer) ? renderer.removeChild(parent as RElement, child) : +export function nativeRemoveChild( + renderer: Renderer3, parent: RElement, child: RNode, isHostElement?: boolean): void { + isProceduralRenderer(renderer) ? renderer.removeChild(parent as RElement, child, isHostElement) : parent.removeChild(child); } diff --git a/packages/core/test/animation/animation_integration_spec.ts b/packages/core/test/animation/animation_integration_spec.ts index e86b7be998..035df4a223 100644 --- a/packages/core/test/animation/animation_integration_spec.ts +++ b/packages/core/test/animation/animation_integration_spec.ts @@ -852,58 +852,55 @@ const DEFAULT_COMPONENT_ID = '1'; expect(data.keyframes).toEqual([{offset: 0, opacity: '0'}, {offset: 1, opacity: '1'}]); })); - // nonAnimationRenderer => animationRenderer - fixmeIvy( - 'FW-943 - elements are removed in the wrong renderer so far as host animation @triggers are concerned') - .it('should trigger a leave animation when the inner components host binding updates', - fakeAsync(() => { - @Component({ - selector: 'parent-cmp', - template: ` + it('should trigger a leave animation when the inner components host binding updates', + fakeAsync(() => { + @Component({ + selector: 'parent-cmp', + template: ` ` - }) - class ParentCmp { - public exp = true; - } + }) + class ParentCmp { + public exp = true; + } - @Component({ - selector: 'child-cmp', - template: '...', - animations: [trigger( - 'host', [transition( - ':leave', - [style({opacity: 1}), animate(1000, style({opacity: 0}))])])] - }) - class ChildCmp { - @HostBinding('@host') public hostAnimation = true; - } + @Component({ + selector: 'child-cmp', + template: '...', + animations: [trigger( + 'host', + [transition( + ':leave', [style({opacity: 1}), animate(1000, style({opacity: 0}))])])] + }) + class ChildCmp { + @HostBinding('@host') public hostAnimation = true; + } - TestBed.configureTestingModule({declarations: [ParentCmp, ChildCmp]}); + TestBed.configureTestingModule({declarations: [ParentCmp, ChildCmp]}); - const engine = TestBed.get(ɵAnimationEngine); - const fixture = TestBed.createComponent(ParentCmp); - const cmp = fixture.componentInstance; - fixture.detectChanges(); - engine.flush(); - expect(getLog().length).toEqual(0); + const engine = TestBed.get(ɵAnimationEngine); + const fixture = TestBed.createComponent(ParentCmp); + const cmp = fixture.componentInstance; + fixture.detectChanges(); + engine.flush(); + expect(getLog().length).toEqual(0); - cmp.exp = false; - fixture.detectChanges(); - expect(fixture.debugElement.nativeElement.children.length).toBe(1); + cmp.exp = false; + fixture.detectChanges(); + expect(fixture.debugElement.nativeElement.children.length).toBe(1); - engine.flush(); - expect(getLog().length).toEqual(1); + engine.flush(); + expect(getLog().length).toEqual(1); - const [player] = getLog(); - expect(player.keyframes).toEqual([ - {opacity: '1', offset: 0}, - {opacity: '0', offset: 1}, - ]); + const [player] = getLog(); + expect(player.keyframes).toEqual([ + {opacity: '1', offset: 0}, + {opacity: '0', offset: 1}, + ]); - player.finish(); - expect(fixture.debugElement.nativeElement.children.length).toBe(0); - })); + player.finish(); + expect(fixture.debugElement.nativeElement.children.length).toBe(0); + })); // animationRenderer => nonAnimationRenderer it('should trigger a leave animation when the outer components element binding updates on the host component element', @@ -956,71 +953,67 @@ const DEFAULT_COMPONENT_ID = '1'; expect(fixture.debugElement.nativeElement.children.length).toBe(0); })); - // animationRenderer => animationRenderer - fixmeIvy( - 'FW-943 - elements are removed in the wrong renderer so far as host animation @triggers are concerned') - .it('should trigger a leave animation when both the inner and outer components trigger on the same element', - fakeAsync(() => { - @Component({ - selector: 'parent-cmp', - animations: [trigger( - 'host', - [transition( - ':leave', - [style({height: '100px'}), animate(1000, style({height: '0px'}))])])], - template: ` + it('should trigger a leave animation when both the inner and outer components trigger on the same element', + fakeAsync(() => { + @Component({ + selector: 'parent-cmp', + animations: [trigger( + 'host', + [transition( + ':leave', + [style({height: '100px'}), animate(1000, style({height: '0px'}))])])], + template: ` ` - }) - class ParentCmp { - public exp = true; - } + }) + class ParentCmp { + public exp = true; + } - @Component({ - selector: 'child-cmp', - template: '...', - animations: [trigger( - 'host', - [transition( - ':leave', - [style({width: '100px'}), animate(1000, style({width: '0px'}))])])] - }) - class ChildCmp { - @HostBinding('@host') public hostAnimation = true; - } + @Component({ + selector: 'child-cmp', + template: '...', + animations: [trigger( + 'host', [transition( + ':leave', + [style({width: '100px'}), animate(1000, style({width: '0px'}))])])] + }) + class ChildCmp { + @HostBinding('@host') public hostAnimation = true; + } - TestBed.configureTestingModule({declarations: [ParentCmp, ChildCmp]}); + TestBed.configureTestingModule({declarations: [ParentCmp, ChildCmp]}); - const engine = TestBed.get(ɵAnimationEngine); - const fixture = TestBed.createComponent(ParentCmp); - const cmp = fixture.componentInstance; - fixture.detectChanges(); - engine.flush(); - expect(getLog().length).toEqual(0); + const engine = TestBed.get(ɵAnimationEngine); + const fixture = TestBed.createComponent(ParentCmp); + const cmp = fixture.componentInstance; + fixture.detectChanges(); + engine.flush(); + expect(getLog().length).toEqual(0); - cmp.exp = false; - fixture.detectChanges(); - expect(fixture.debugElement.nativeElement.children.length).toBe(1); + cmp.exp = false; + fixture.detectChanges(); + expect(fixture.debugElement.nativeElement.children.length).toBe(1); - engine.flush(); - expect(getLog().length).toEqual(2); + engine.flush(); + expect(getLog().length).toEqual(2); - const [p1, p2] = getLog(); - expect(p1.keyframes).toEqual([ - {width: '100px', offset: 0}, - {width: '0px', offset: 1}, - ]); + const [p1, p2] = getLog(); + expect(p1.keyframes).toEqual([ + {width: '100px', offset: 0}, + {width: '0px', offset: 1}, + ]); - expect(p2.keyframes).toEqual([ - {height: '100px', offset: 0}, - {height: '0px', offset: 1}, - ]); + expect(p2.keyframes).toEqual([ + {height: '100px', offset: 0}, + {height: '0px', offset: 1}, + ]); - p1.finish(); - p2.finish(); - flushMicrotasks(); - expect(fixture.debugElement.nativeElement.children.length).toBe(0); - })); + p1.finish(); + p2.finish(); + flushMicrotasks(); + expect(fixture.debugElement.nativeElement.children.length).toBe(0); + })); it('should not throw when the host element is removed and no animation triggers', fakeAsync(() => { diff --git a/packages/core/test/animation/animation_query_integration_spec.ts b/packages/core/test/animation/animation_query_integration_spec.ts index 4969036d82..d3de042917 100644 --- a/packages/core/test/animation/animation_query_integration_spec.ts +++ b/packages/core/test/animation/animation_query_integration_spec.ts @@ -2239,7 +2239,7 @@ import {HostListener} from '../../src/metadata/directives'; }); fixmeIvy( - 'FW-943 - elements are removed in the wrong renderer so far as host animation @triggers are concerned') + 'FW-943 - Fix final `unknown` issue in `animation_query_integration_spec.ts` once #28162 lands') .it('should emulate a leave animation on the nearest sub host elements when a parent is removed', fakeAsync(() => { @Component({ diff --git a/packages/platform-browser/animations/src/animation_renderer.ts b/packages/platform-browser/animations/src/animation_renderer.ts index 3b089ced3a..59b4f48268 100644 --- a/packages/platform-browser/animations/src/animation_renderer.ts +++ b/packages/platform-browser/animations/src/animation_renderer.ts @@ -148,8 +148,8 @@ export class BaseAnimationRenderer implements Renderer2 { this.engine.onInsert(this.namespaceId, newChild, parent, true); } - removeChild(parent: any, oldChild: any): void { - this.engine.onRemove(this.namespaceId, oldChild, this.delegate); + removeChild(parent: any, oldChild: any, isHostElement: boolean): void { + this.engine.onRemove(this.namespaceId, oldChild, this.delegate, isHostElement); } selectRootElement(selectorOrNode: any, preserveContent?: boolean) { diff --git a/tools/public_api_guard/core/core.d.ts b/tools/public_api_guard/core/core.d.ts index c6e810ca68..e94f6a31c7 100644 --- a/tools/public_api_guard/core/core.d.ts +++ b/tools/public_api_guard/core/core.d.ts @@ -770,7 +770,7 @@ export declare abstract class Renderer2 { abstract nextSibling(node: any): any; abstract parentNode(node: any): any; abstract removeAttribute(el: any, name: string, namespace?: string | null): void; - abstract removeChild(parent: any, oldChild: any): void; + abstract removeChild(parent: any, oldChild: any, isHostElement?: boolean): void; abstract removeClass(el: any, name: string): void; abstract removeStyle(el: any, style: string, flags?: RendererStyleFlags2): void; abstract selectRootElement(selectorOrNode: string | any, preserveContent?: boolean): any;