fix(core): not inserting ViewContainerRef nodes when inside root of a…

… component (#39599) When a `ViewContainerRef` is injected, we dynamically create a comment node next to the host so that it can be used as an anchor point for inserting views. The comment node is inserted through the `appendChild` helper from `node_manipulation.ts` in most cases. The problem with using `appendChild` here is that it has some extra logic which doesn't return a parent `RNode` if an element is at the root of a component. I __think__ that this is a performance optimization which is used to avoid inserting an element in one place in the DOM and then moving it a bit later when it is projected. This can break down in some cases when creating a `ViewContainerRef` for a non-component node at the root of another component like the following: ``` <root> <div #viewContainerRef></div> </root> ``` In this case the `#viewContainerRef` node is at the root of a component so we intentionally don't insert it, but since its anchor element was created manually, it'll never be projected. This will prevent any views added through the `ViewContainerRef` from being inserted into the DOM. These changes resolve the issue by not going through `appendChild` at all when creating a comment node for `ViewContainerRef`. This should work identically since `appendChild` doesn't really do anything with the T structures anyway, it only uses them to reach the relevant DOM nodes. Fixes #39556. PR Close #39599
angular · Nov 12, 2020 · 20db90a · 20db90a
1 parent 050cea9
commit 20db90a
Show file tree

Hide file tree

Showing 6 changed files with 60 additions and 45 deletions.
diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts
@@ -557,9 +557,8 @@ export function getClosestRElement(tView: TView, tNode: TNode|null, lView: LView
     ngDevMode && assertTNodeType(parentTNode, TNodeType.AnyRNode | TNodeType.Container);
     if (parentTNode.flags & TNodeFlags.isComponentHost) {
       ngDevMode && assertTNodeForLView(parentTNode, lView);
-      const tData = tView.data;
-      const tNode = tData[parentTNode.index] as TNode;
-      const encapsulation = (tData[tNode.directiveStart] as ComponentDef<any>).encapsulation;
+      const encapsulation =
+          (tView.data[parentTNode.directiveStart] as ComponentDef<unknown>).encapsulation;
       // We've got a parent which is an element in the current view. We just need to verify if the
       // parent element is not a component. Component's content nodes are not inserted immediately
       // because they will be projected, and so doing insert at this point would be wasteful.

diff --git a/packages/core/src/render3/view_engine_compatibility.ts b/packages/core/src/render3/view_engine_compatibility.ts
@@ -20,15 +20,15 @@ import {assertDefined, assertEqual, assertGreaterThan, assertLessThan} from '../
 
 import {assertLContainer, assertNodeInjector} from './assert';
 import {getParentInjectorLocation, NodeInjector} from './di';
-import {addToViewTree, createLContainer, createLView, createTNode, renderView} from './instructions/shared';
+import {addToViewTree, createLContainer, createLView, renderView} from './instructions/shared';
 import {CONTAINER_HEADER_OFFSET, LContainer, NATIVE, VIEW_REFS} from './interfaces/container';
 import {NodeInjectorOffset} from './interfaces/injector';
 import {TContainerNode, TDirectiveHostNode, TElementContainerNode, TElementNode, TNode, TNodeType} from './interfaces/node';
 import {isProceduralRenderer, RComment, RElement} from './interfaces/renderer';
-import {isComponentHost, isLContainer, isLView, isRootView} from './interfaces/type_checks';
+import {isComponentHost, isLContainer, isLView} from './interfaces/type_checks';
 import {DECLARATION_COMPONENT_VIEW, DECLARATION_LCONTAINER, LView, LViewFlags, PARENT, QUERIES, RENDERER, T_HOST, TVIEW, TView} from './interfaces/view';
 import {assertTNodeType} from './node_assert';
-import {addViewToContainer, appendChild, destroyLView, detachView, getBeforeNodeForView, insertView, nativeInsertBefore, nativeNextSibling, nativeParentNode} from './node_manipulation';
+import {addViewToContainer, destroyLView, detachView, getBeforeNodeForView, insertView, nativeInsertBefore, nativeNextSibling, nativeParentNode} from './node_manipulation';
 import {getCurrentTNode, getLView} from './state';
 import {getParentInjectorIndex, getParentInjectorView, hasParentInjector} from './util/injector_utils';
 import {getComponentLViewByIndex, getNativeByTNode, unwrapRNode, viewAttachedToContainer} from './util/view_utils';
@@ -373,28 +373,18 @@ export function createContainerRef(
     if (hostTNode.type & TNodeType.ElementContainer) {
       commentNode = unwrapRNode(slotValue) as RComment;
     } else {
+      // If the host is a regular element, we have to insert a comment node manually which will
+      // be used as an anchor when inserting elements. In this specific case we use low-level DOM
+      // manipulation to insert it.
+      const renderer = hostView[RENDERER];
       ngDevMode && ngDevMode.rendererCreateComment++;
-      commentNode = hostView[RENDERER].createComment(ngDevMode ? 'container' : '');
-
-      // A `ViewContainerRef` can be injected by the root (topmost / bootstrapped) component. In
-      // this case we can't use TView / TNode data structures to insert container's marker node
-      // (both a parent of a comment node and the comment node itself are not part of any view). In
-      // this specific case we use low-level DOM manipulation to insert container's marker (comment)
-      // node.
-      if (isRootView(hostView)) {
-        const renderer = hostView[RENDERER];
-        const hostNative = getNativeByTNode(hostTNode, hostView)!;
-        const parentOfHostNative = nativeParentNode(renderer, hostNative);
-        nativeInsertBefore(
-            renderer, parentOfHostNative!, commentNode, nativeNextSibling(renderer, hostNative),
-            false);
-      } else {
-        // The TNode created here is bogus, in that it is not added to the TView. It is only created
-        // to allow us to create a dynamic Comment node.
-        const commentTNode =
-            createTNode(hostView[TVIEW], hostTNode.parent, TNodeType.Container, 0, null, null);
-        appendChild(hostView[TVIEW], hostView, commentNode, commentTNode);
-      }
+      commentNode = renderer.createComment(ngDevMode ? 'container' : '');
+
+      const hostNative = getNativeByTNode(hostTNode, hostView)!;
+      const parentOfHostNative = nativeParentNode(renderer, hostNative);
+      nativeInsertBefore(
+          renderer, parentOfHostNative!, commentNode, nativeNextSibling(renderer, hostNative),
+          false);
     }
 
     hostView[hostTNode.index] = lContainer =

diff --git a/packages/core/test/acceptance/component_spec.ts b/packages/core/test/acceptance/component_spec.ts
@@ -96,6 +96,50 @@ describe('component', () => {
     expect(fixture.nativeElement).toHaveText('foo|bar');
   });
 
+  it('should be able to dynamically insert a component into a view container at the root of a component',
+     () => {
+       @Component({template: 'hello'})
+       class HelloComponent {
+       }
+
+       // TODO: This module is only used to declare the `entryComponets` since
+       //  `configureTestingModule` doesn't support it. The module can be removed
+       // once ViewEngine is removed.
+       @NgModule({
+         declarations: [HelloComponent],
+         exports: [HelloComponent],
+         entryComponents: [HelloComponent]
+       })
+       class HelloModule {
+       }
+
+       @Component({selector: 'wrapper', template: '<ng-content></ng-content>'})
+       class Wrapper {
+       }
+
+       @Component({
+         template: `
+            <wrapper>
+              <div #insertionPoint></div>
+            </wrapper>
+          `
+       })
+       class App {
+         @ViewChild('insertionPoint', {read: ViewContainerRef}) viewContainerRef!: ViewContainerRef;
+         constructor(public componentFactoryResolver: ComponentFactoryResolver) {}
+       }
+
+       TestBed.configureTestingModule({declarations: [App, Wrapper], imports: [HelloModule]});
+       const fixture = TestBed.createComponent(App);
+       fixture.detectChanges();
+
+       const instance = fixture.componentInstance;
+       const factory = instance.componentFactoryResolver.resolveComponentFactory(HelloComponent);
+       instance.viewContainerRef.createComponent(factory);
+
+       expect(fixture.nativeElement.textContent.trim()).toBe('hello');
+     });
+
   // TODO: add tests with Native once tests run in real browser (domino doesn't support shadow root)
   describe('encapsulation', () => {
     @Component({

diff --git a/packages/core/test/bundling/forms/bundle.golden_symbols.json b/packages/core/test/bundling/forms/bundle.golden_symbols.json
@@ -824,9 +824,6 @@
   {
     "name": "createPlatformFactory"
   },
-  {
-    "name": "createTNode"
-  },
   {
     "name": "createTView"
   },
@@ -1244,9 +1241,6 @@
   {
     "name": "isPropertyUpdated"
   },
-  {
-    "name": "isRootView"
-  },
   {
     "name": "isScheduler"
   },

diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json
@@ -1070,9 +1070,6 @@
   {
     "name": "createRouterScroller"
   },
-  {
-    "name": "createTNode"
-  },
   {
     "name": "createTView"
   },
@@ -1568,9 +1565,6 @@
   {
     "name": "isPromise"
   },
-  {
-    "name": "isRootView"
-  },
   {
     "name": "isScheduler"
   },

diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json
@@ -260,9 +260,6 @@
   {
     "name": "createLView"
   },
-  {
-    "name": "createTNode"
-  },
   {
     "name": "createTView"
   },
@@ -518,9 +515,6 @@
   {
     "name": "isProceduralRenderer"
   },
-  {
-    "name": "isRootView"
-  },
   {
     "name": "isStylingMatch"
   },