diff --git a/packages/core/src/render3/i18n.ts b/packages/core/src/render3/i18n.ts index 33061675b7..fe2081ba1a 100644 --- a/packages/core/src/render3/i18n.ts +++ b/packages/core/src/render3/i18n.ts @@ -19,7 +19,7 @@ import {RComment, RElement} from './interfaces/renderer'; import {SanitizerFn} from './interfaces/sanitization'; import {StylingContext} from './interfaces/styling'; import {BINDING_INDEX, HEADER_OFFSET, HOST_NODE, LView, RENDERER, TVIEW, TView} from './interfaces/view'; -import {appendChild, createTextNode, removeChild} from './node_manipulation'; +import {appendChild, createTextNode, removeNode as removeRNode} from './node_manipulation'; import {getIsParent, getLView, getPreviousOrParentTNode, setIsParent, setPreviousOrParentTNode} from './state'; import {NO_CHANGE} from './tokens'; import {addAllToArray, getNativeByIndex, getNativeByTNode, getTNode, isLContainer, renderStringify} from './util'; @@ -830,7 +830,7 @@ function removeNode(index: number, viewData: LView) { const removedPhTNode = getTNode(index, viewData); const removedPhRNode = getNativeByIndex(index, viewData); if (removedPhRNode) { - removeChild(removedPhTNode, removedPhRNode, viewData); + removeRNode(removedPhTNode, removedPhRNode, viewData); } removedPhTNode.detached = true; @@ -840,7 +840,7 @@ function removeNode(index: number, viewData: LView) { if (isLContainer(slotValue)) { const lContainer = slotValue as LContainer; if (removedPhTNode.type !== TNodeType.Container) { - removeChild(removedPhTNode, lContainer[NATIVE], viewData); + removeRNode(removedPhTNode, lContainer[NATIVE], viewData); } } } diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index dce6ae1d66..3ff7c10172 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -181,7 +181,7 @@ function executeNodeAction( if (action === WalkTNodeTreeAction.Insert) { nativeInsertBefore(renderer, parent !, node, beforeNode || null); } else if (action === WalkTNodeTreeAction.Detach) { - nativeRemoveChild(renderer, parent !, node, isComponent(tNode)); + nativeRemoveChild(renderer, node, isComponent(tNode)); } else if (action === WalkTNodeTreeAction.Destroy) { ngDevMode && ngDevMode.rendererDestroyNode++; (renderer as ProceduralRenderer3).destroyNode !(node); @@ -593,13 +593,19 @@ function nativeAppendOrInsertBefore( } } -/** - * Removes a native child node from a given native parent node. - */ +/** Removes a node from the DOM. */ export function nativeRemoveChild( - renderer: Renderer3, parent: RElement, child: RNode, isHostElement?: boolean): void { - isProceduralRenderer(renderer) ? renderer.removeChild(parent as RElement, child, isHostElement) : - parent.removeChild(child); + renderer: Renderer3, child: RNode, isHostElement?: boolean): void { + if (isProceduralRenderer(renderer)) { + const renderParent = renderer.parentNode(child); + if (renderParent) { + renderer.removeChild(renderParent, child, isHostElement); + } + } else { + // We intentionally don't use the given parent node since it may no longer + // match the state of the DOM (if the child node has been manually moved). + child.parentNode && child.parentNode.removeChild(child); + } } /** @@ -692,14 +698,9 @@ export function getBeforeNodeForView(index: number, views: LView[], containerNat * @param childTNode The TNode of the child to remove * @param childEl The child that should be removed * @param currentView The current LView - * @returns Whether or not the child was removed */ -export function removeChild(childTNode: TNode, childEl: RNode, currentView: LView): void { - const parentNative = getRenderParent(childTNode, currentView); - // We only remove the element if it already has a render parent. - if (parentNative) { - nativeRemoveChild(currentView[RENDERER], parentNative, childEl); - } +export function removeNode(childTNode: TNode, childEl: RNode, currentView: LView): void { + nativeRemoveChild(currentView[RENDERER], childEl); } /** diff --git a/packages/core/test/linker/view_injector_integration_spec.ts b/packages/core/test/linker/view_injector_integration_spec.ts index fd3a205b74..7c952e931b 100644 --- a/packages/core/test/linker/view_injector_integration_spec.ts +++ b/packages/core/test/linker/view_injector_integration_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {Attribute, ChangeDetectionStrategy, ChangeDetectorRef, Component, ComponentFactoryResolver, DebugElement, Directive, ElementRef, Host, Inject, InjectionToken, Injector, Input, NgModule, Optional, Pipe, PipeTransform, Provider, Self, SkipSelf, TemplateRef, Type, ViewContainerRef} from '@angular/core'; +import {Attribute, ChangeDetectionStrategy, ChangeDetectorRef, Component, ComponentFactoryResolver, DebugElement, Directive, ElementRef, EmbeddedViewRef, Host, Inject, InjectionToken, Injector, Input, NgModule, Optional, Pipe, PipeTransform, Provider, Self, SkipSelf, TemplateRef, Type, ViewContainerRef} from '@angular/core'; import {ComponentFixture, TestBed, fakeAsync} from '@angular/core/testing'; import {getDOM} from '@angular/platform-browser/src/dom/dom_adapter'; import {expect} from '@angular/platform-browser/testing/src/matchers'; @@ -1021,6 +1021,58 @@ class TestComp { expect(impurePipe4).not.toBe(impurePipe1); }); }); + + describe('view destruction', () => { + @Component({selector: 'some-component', template: ''}) + class SomeComponent { + } + + @NgModule({ + declarations: [SomeComponent], + exports: [SomeComponent], + entryComponents: [SomeComponent] + }) + class SomeModule { + } + + @Component({selector: 'listener-and-on-destroy', template: ''}) + class ComponentThatLoadsAnotherComponentThenMovesIt { + constructor( + private viewContainerRef: ViewContainerRef, + private componentFactoryResolver: ComponentFactoryResolver) {} + + + ngOnInit() { + // Dynamically load some component. + const componentFactory = + this.componentFactoryResolver.resolveComponentFactory(SomeComponent); + const componentRef = + this.viewContainerRef.createComponent(componentFactory, this.viewContainerRef.length); + + // Manually move the loaded component to some arbitrary DOM node. + const componentRootNode = + (componentRef.hostView as EmbeddedViewRef).rootNodes[0] as HTMLElement; + document.createElement('div').appendChild(componentRootNode); + + // Destroy the component we just moved to ensure that it does not error during + // destruction. + componentRef.destroy(); + } + } + + it('should not error when destroying a component that has been moved in the DOM', () => { + TestBed.configureTestingModule({ + imports: [SomeModule], + declarations: [ComponentThatLoadsAnotherComponentThenMovesIt], + }); + const fixture = + createComponentFixture(``); + fixture.detectChanges(); + + // This test will fail if the ngOnInit of ComponentThatLoadsAnotherComponentThenMovesIt + // throws an error. + }); + }); }); })(); diff --git a/packages/core/test/render3/view_container_ref_spec.ts b/packages/core/test/render3/view_container_ref_spec.ts index 0a58cf77cc..db9f6b497c 100644 --- a/packages/core/test/render3/view_container_ref_spec.ts +++ b/packages/core/test/render3/view_container_ref_spec.ts @@ -6,9 +6,9 @@ * found in the LICENSE file at https://angular.io/license */ -import {ChangeDetectorRef, Component as _Component, ComponentFactoryResolver, ElementRef, EmbeddedViewRef, NgModuleRef, Pipe, PipeTransform, QueryList, RendererFactory2, TemplateRef, ViewContainerRef, ViewRef, defineInjector, ɵAPP_ROOT as APP_ROOT, ɵNgModuleDef as NgModuleDef} from '../../src/core'; +import {ChangeDetectorRef, Component as _Component, ComponentFactoryResolver, ComponentRef, defineInjector, ElementRef, EmbeddedViewRef, NgModuleRef, Pipe, PipeTransform, QueryList, RendererFactory2, TemplateRef, ViewContainerRef, ViewRef, ɵAPP_ROOT as APP_ROOT, ɵNgModuleDef as NgModuleDef,} from '../../src/core'; +import {createInjector} from '../../src/di/r3_injector'; import {ViewEncapsulation} from '../../src/metadata'; - import {AttributeMarker, defineComponent, defineDirective, definePipe, injectComponentFactoryResolver, listener, loadViewQuery, NgOnChangesFeature, queryRefresh, viewQuery,} from '../../src/render3/index'; import {allocHostVars, bind, container, containerRefreshEnd, containerRefreshStart, directiveInject, element, elementEnd, elementHostAttrs, elementProperty, elementStart, embeddedViewEnd, embeddedViewStart, interpolation1, interpolation3, nextContext, projection, projectionDef, reference, template, text, textBinding,} from '../../src/render3/instructions'; @@ -18,7 +18,6 @@ import {NgModuleFactory} from '../../src/render3/ng_module_ref'; import {pipe, pipeBind1} from '../../src/render3/pipe'; import {getLView} from '../../src/render3/state'; import {getNativeByIndex} from '../../src/render3/util'; -import {createInjector} from '../../src/di/r3_injector'; import {templateRefExtractor} from '../../src/render3/view_engine_compatibility_prebound'; import {NgForOf} from '../../test/render3/common_with_def'; diff --git a/packages/platform-browser/test/dom/dom_renderer_spec.ts b/packages/platform-browser/test/dom/dom_renderer_spec.ts index ae26bce267..ef47b6b14d 100644 --- a/packages/platform-browser/test/dom/dom_renderer_spec.ts +++ b/packages/platform-browser/test/dom/dom_renderer_spec.ts @@ -87,6 +87,17 @@ import {expect} from '@angular/platform-browser/testing/src/matchers'; }); }); + describe('removeChild', () => { + it('should not error when removing a child with a different parent than given', () => { + const savedParent = document.createElement('div'); + const realParent = document.createElement('div'); + const child = document.createElement('div'); + + realParent.appendChild(child); + renderer.removeChild(savedParent, child); + }); + }); + if (browserDetection.supportsDeprecatedShadowDomV0) { it('should allow to style components with emulated encapsulation and no encapsulation inside of components with shadow DOM', () => {