fix(ivy): properly find RNode (angular#23193)

As we no longer create native (RNode) comment nodes for containers,
we need to execute logic for finding a next sibiling node with RNode
when inserting a view.

The mentioned logic need to be updated for the case of dynamically
created containers (LContainerNode). Indeed, we need to be able to
descend into dynamically inserted views while looking for a RNode.
To achieve this we need to have a pointer from a host LNode to a
dynamically created LContainerNode).

PR Close angular#23193

Author: pkozlowski-opensource

packages/core/src/render3/di.ts

@@ -556,10 +556,12 @@ export function getOrCreateContainerRef(di: LInjector): viewEngine_ViewContainer
 
     ngDevMode && assertNodeOfPossibleTypes(vcRefHost, LNodeType.Container, LNodeType.Element);
 
-    const lContainer = createLContainer(vcRefHost.parent !, vcRefHost.view, undefined, vcRefHost);
+    const lContainer = createLContainer(vcRefHost.parent !, vcRefHost.view);
     const lContainerNode: LContainerNode = createLNodeObject(
         LNodeType.Container, vcRefHost.view, vcRefHost.parent !, undefined, lContainer, null);
 
+    vcRefHost.dynamicLContainerNode = lContainerNode;
+
     addToViewTree(vcRefHost.view, lContainer);
 
     di.viewContainerRef = new ViewContainerRef(lContainerNode);
@@ -608,6 +610,10 @@ class ViewContainerRef implements viewEngine_ViewContainerRef {
     const adjustedIdx = this._adjustAndAssertIndex(index);
 
     insertView(this._lContainerNode, lViewNode, adjustedIdx);
+    // invalidate cache of next sibling RNode (we do similar operation in the containerRefreshEnd
+    // instruction)
+    this._lContainerNode.native = undefined;
+
     this._viewRefs.splice(adjustedIdx, 0, viewRef);
 
     (lViewNode as{parent: LNode}).parent = this._lContainerNode;

packages/core/src/render3/instructions.ts

@@ -318,7 +318,8 @@ export function createLNodeObject(
     data: state,
     queries: queries,
     tNode: null,
-    pNextOrParent: null
+    pNextOrParent: null,
+    dynamicLContainerNode: null
   };
 }
 
@@ -386,6 +387,9 @@ export function createLNode(
                        previousOrParentNode.next,
                        `previousOrParentNode's next property should not have been set ${index}.`);
       previousOrParentNode.next = node;
+      if (previousOrParentNode.dynamicLContainerNode) {
+        previousOrParentNode.dynamicLContainerNode.next = node;
+      }
     }
   }
   previousOrParentNode = node;
@@ -452,9 +456,10 @@ export function renderEmbeddedTemplate<T>(
       const directives = currentView && currentView.tView.directiveRegistry;
       const pipes = currentView && currentView.tView.pipeRegistry;
 
-      const view = createLView(
-          -1, renderer, createTView(directives, pipes), template, context, LViewFlags.CheckAlways);
-      viewNode = createLNode(null, LNodeType.View, null, view);
+      const tView = getOrCreateTView(template, directives, pipes);
+      const lView = createLView(-1, renderer, tView, template, context, LViewFlags.CheckAlways);
+
+      viewNode = createLNode(null, LNodeType.View, null, lView);
       cm = true;
     }
     oldView = enterView(viewNode.data, viewNode);
@@ -1311,8 +1316,7 @@ function generateInitialInputs(
 
 
 export function createLContainer(
-    parentLNode: LNode, currentView: LView, template?: ComponentTemplate<any>,
-    host?: LContainerNode | LElementNode): LContainer {
+    parentLNode: LNode, currentView: LView, template?: ComponentTemplate<any>): LContainer {
   ngDevMode && assertNotNull(parentLNode, 'containers should have a parent');
   return <LContainer>{
     views: [],
@@ -1324,8 +1328,7 @@ export function createLContainer(
     next: null,
     parent: currentView,
     dynamicViewCount: 0,
-    queries: null,
-    host: host == null ? null : host
+    queries: null
   };
 }
 

packages/core/src/render3/interfaces/container.ts

@@ -80,13 +80,6 @@ export interface LContainer {
    * this container are reported to queries referenced here.
    */
   queries: LQueries|null;
-
-  /**
-   * If a LContainer is created dynamically (by a directive requesting ViewContainerRef) this fields
-   * keeps a reference to a node on which a ViewContainerRef was requested. We need to store this
-   * information to find a next render sibling node.
-   */
-  host: LContainerNode|LElementNode|null;
 }
 
 /**

packages/core/src/render3/interfaces/node.ts

@@ -130,6 +130,11 @@ export interface LNode {
    * data about this node.
    */
   tNode: TNode|null;
+
+  /**
+   * A pointer to a LContainerNode created by directives requesting ViewContainerRef
+   */
+  dynamicLContainerNode: LContainerNode|null;
 }
 
 
@@ -158,6 +163,7 @@ export interface LTextNode extends LNode {
   /** LTextNodes can be inside LElementNodes or inside LViewNodes. */
   readonly parent: LElementNode|LViewNode;
   readonly data: null;
+  dynamicLContainerNode: null;
 }
 
 /** Abstract node which contains root nodes of a view. */
@@ -169,6 +175,7 @@ export interface LViewNode extends LNode {
   /**  LViewNodes can only be added to LContainerNodes. */
   readonly parent: LContainerNode|null;
   readonly data: LView;
+  dynamicLContainerNode: null;
 }
 
 /** Abstract node container which contains other views. */
@@ -199,6 +206,7 @@ export interface LProjectionNode extends LNode {
 
   /** Projections can be added to elements or views. */
   readonly parent: LElementNode|LViewNode;
+  dynamicLContainerNode: null;
 }
 
 /**

packages/core/src/render3/node_manipulation.ts

@@ -125,8 +125,10 @@ function findFirstRNode(rootNode: LNode): RElement|RText|null {
       // A LElementNode has a matching RNode in LElementNode.native
       return (node as LElementNode).native;
     } else if (node.type === LNodeType.Container) {
-      // For container look at the first node of the view next
-      const childContainerData: LContainer = (node as LContainerNode).data;
+      const lContainerNode: LContainerNode = (node as LContainerNode);
+      const childContainerData: LContainer = lContainerNode.dynamicLContainerNode ?
+          lContainerNode.dynamicLContainerNode.data :
+          lContainerNode.data;
       nextNode = childContainerData.views.length ? childContainerData.views[0].child : null;
     } else if (node.type === LNodeType.Projection) {
       // For Projection look at the first projected node
@@ -281,10 +283,7 @@ export function insertView(
     if (!beforeNode) {
       let containerNextNativeNode = container.native;
       if (containerNextNativeNode === undefined) {
-        // TODO(pk): this is probably too simplistic, add more tests for various host placements
-        // (dynamic view, projection, ...)
-        containerNextNativeNode = container.native =
-            findNextRNodeSibling(container.data.host ? container.data.host : container, null);
+        containerNextNativeNode = container.native = findNextRNodeSibling(container, null);
       }
       beforeNode = containerNextNativeNode;
     }

packages/core/test/render3/view_container_ref_spec.ts

@@ -9,7 +9,7 @@
 import {TemplateRef, ViewContainerRef} from '../../src/core';
 import {getOrCreateNodeInjectorForNode, getOrCreateTemplateRef} from '../../src/render3/di';
 import {defineComponent, defineDirective, injectTemplateRef, injectViewContainerRef} from '../../src/render3/index';
-import {bind, container, elementEnd, elementProperty, elementStart, interpolation1, load, loadDirective, text, textBinding} from '../../src/render3/instructions';
+import {bind, container, containerRefreshEnd, containerRefreshStart, elementEnd, elementProperty, elementStart, embeddedViewEnd, embeddedViewStart, interpolation1, load, loadDirective, text, textBinding} from '../../src/render3/instructions';
 
 import {ComponentFixture} from './render_util';
 
@@ -190,4 +190,161 @@ describe('ViewContainerRef', () => {
     expect(fixture.html).toEqual('before||after');
   });
 
+  it('should add embedded views at the right position in the DOM tree (ng-template next to other ng-template)',
+     () => {
+       let directiveInstances: TestDirective[] = [];
+
+       class TestDirective {
+         static ngDirectiveDef = defineDirective({
+           type: TestDirective,
+           selectors: [['', 'testdir', '']],
+           factory: () => {
+             const instance = new TestDirective(injectViewContainerRef(), injectTemplateRef());
+
+             directiveInstances.push(instance);
+
+             return instance;
+           }
+         });
+
+         constructor(private _vcRef: ViewContainerRef, private _tplRef: TemplateRef<{}>) {}
+
+         insertTpl(ctx: {}) { this._vcRef.createEmbeddedView(this._tplRef, ctx); }
+
+         remove(index?: number) { this._vcRef.remove(index); }
+       }
+
+       function EmbeddedTemplateA(ctx: any, cm: boolean) {
+         if (cm) {
+           text(0, 'A');
+         }
+       }
+
+       function EmbeddedTemplateB(ctx: any, cm: boolean) {
+         if (cm) {
+           text(0, 'B');
+         }
+       }
+
+       /**
+        * before|
+        * <ng-template directive>A<ng-template>
+        * <ng-template directive>B<ng-template>
+        * |after
+        */
+       class TestComponent {
+         testDir: TestDirective;
+         static ngComponentDef = defineComponent({
+           type: TestComponent,
+           selectors: [['test-cmp']],
+           factory: () => new TestComponent(),
+           template: (cmp: TestComponent, cm: boolean) => {
+             if (cm) {
+               text(0, 'before|');
+               container(1, EmbeddedTemplateA, undefined, ['testdir', '']);
+               container(2, EmbeddedTemplateB, undefined, ['testdir', '']);
+               text(3, '|after');
+             }
+           },
+           directives: [TestDirective]
+         });
+       }
+
+       const fixture = new ComponentFixture(TestComponent);
+       expect(fixture.html).toEqual('before||after');
+
+       directiveInstances ![1].insertTpl({});
+       expect(fixture.html).toEqual('before|B|after');
+
+       directiveInstances ![0].insertTpl({});
+       expect(fixture.html).toEqual('before|AB|after');
+     });
+
+
+  it('should add embedded views at the right position in the DOM tree (ng-template next to a JS block)',
+     () => {
+       let directiveInstance: TestDirective;
+
+       class TestDirective {
+         static ngDirectiveDef = defineDirective({
+           type: TestDirective,
+           selectors: [['', 'testdir', '']],
+           factory: () => directiveInstance =
+                        new TestDirective(injectViewContainerRef(), injectTemplateRef())
+         });
+
+         constructor(private _vcRef: ViewContainerRef, private _tplRef: TemplateRef<{}>) {}
+
+         insertTpl(ctx: {}) { this._vcRef.createEmbeddedView(this._tplRef, ctx); }
+
+         remove(index?: number) { this._vcRef.remove(index); }
+       }
+
+       function EmbeddedTemplateA(ctx: any, cm: boolean) {
+         if (cm) {
+           text(0, 'A');
+         }
+       }
+
+       /**
+        * before|
+        * <ng-template directive>A<ng-template>
+        * % if (condition) {
+        *  B
+        * }
+        * |after
+        */
+       class TestComponent {
+         condition = false;
+         testDir: TestDirective;
+         static ngComponentDef = defineComponent({
+           type: TestComponent,
+           selectors: [['test-cmp']],
+           factory: () => new TestComponent(),
+           template: (cmp: TestComponent, cm: boolean) => {
+             if (cm) {
+               text(0, 'before|');
+               container(1, EmbeddedTemplateA, undefined, ['testdir', '']);
+               container(2);
+               text(3, '|after');
+             }
+             containerRefreshStart(2);
+             {
+               if (cmp.condition) {
+                 let cm1 = embeddedViewStart(0);
+                 {
+                   if (cm1) {
+                     text(0, 'B');
+                   }
+                 }
+                 embeddedViewEnd();
+               }
+             }
+             containerRefreshEnd();
+           },
+           directives: [TestDirective]
+         });
+       }
+
+       const fixture = new ComponentFixture(TestComponent);
+       expect(fixture.html).toEqual('before||after');
+
+       fixture.component.condition = true;
+       fixture.update();
+       expect(fixture.html).toEqual('before|B|after');
+
+       directiveInstance !.insertTpl({});
+       expect(fixture.html).toEqual('before|AB|after');
+
+       fixture.component.condition = false;
+       fixture.update();
+       expect(fixture.html).toEqual('before|A|after');
+
+       directiveInstance !.insertTpl({});
+       expect(fixture.html).toEqual('before|AA|after');
+
+       fixture.component.condition = true;
+       fixture.update();
+       expect(fixture.html).toEqual('before|AAB|after');
+     });
 });