fix(ivy): fix views manipulation logic (angular#22656)

This commit fixes a bug that would result in views insert / remove
even if a view needed only refresh operation.

The crux of the bug was that we were looking for a view to update
only in the LContainer.nextIndex position. This is incorrect as a
view with a given block id could be present later in the views
array (this happens if we about to remove a view in the middle of
the views array).

The code in this fix searches for a view to update in the views array and
can remove views in the middle of the views collection. Previously we
would remove views at the end of the collection only.

PR Close angular#22656

packages/core/src/render3/di.ts

@@ -582,6 +582,15 @@ class ViewContainerRef implements viewEngine_ViewContainerRef {
     const lView = (viewRef as EmbeddedViewRef<any>)._lViewNode;
     insertView(this._node, lView, index);
 
+    // TODO(pk): this is a temporary index adjustment so imperativelly inserted (through
+    // ViewContainerRef) views
+    // are not removed in the containerRefreshEnd instruction.
+    // The final fix will consist of creating a dedicated container node for views inserted through
+    // ViewContainerRef.
+    // Such container should not be trimmed as it is the case in the containerRefreshEnd
+    // instruction.
+    this._node.data.nextIndex = this._node.data.views.length;
+
     // If the view is dynamic (has a template), it needs to be counted both at the container
     // level and at the node above the container.
     if (lView.data.template !== null) {

packages/core/src/render3/instructions.ts

@@ -1204,8 +1204,9 @@ export function containerRefreshEnd(): void {
   container.native = undefined;
   ngDevMode && assertNodeType(container, LNodeFlags.Container);
   const nextIndex = container.data.nextIndex;
+
+  // remove extra views at the end of the container
   while (nextIndex < container.data.views.length) {
-    // remove extra view.
     removeView(container, nextIndex);
   }
 }
@@ -1222,6 +1223,35 @@ function refreshDynamicChildren() {
   }
 }
 
+/**
+ * Looks for a view with a given view block id inside a provided LContainer.
+ * Removes views that need to be deleted in the process.
+ *
+ * @param containerNode where to search for views
+ * @param startIdx starting index in the views array to search from
+ * @param viewBlockId exact view block id to look for
+ * @returns index of a found view or -1 if not found
+ */
+function scanForView(
+    containerNode: LContainerNode, startIdx: number, viewBlockId: number): LViewNode|null {
+  const views = containerNode.data.views;
+  for (let i = startIdx; i < views.length; i++) {
+    const viewAtPositionId = views[i].data.id;
+    if (viewAtPositionId === viewBlockId) {
+      return views[i];
+    } else if (viewAtPositionId < viewBlockId) {
+      // found a view that should not be at this position - remove
+      removeView(containerNode, i);
+    } else {
+      // found a view with id grater than the one we are searching for
+      // which means that required view doesn't exist and can't be found at
+      // later positions in the views array - stop the search here
+      break;
+    }
+  }
+  return null;
+}
+
 /**
  * Marks the start of an embedded view.
  *
@@ -1233,17 +1263,13 @@ export function embeddedViewStart(viewBlockId: number): boolean {
       (isParent ? previousOrParentNode : previousOrParentNode.parent !) as LContainerNode;
   ngDevMode && assertNodeType(container, LNodeFlags.Container);
   const lContainer = container.data;
-  const views = lContainer.views;
+  const existingViewNode = scanForView(container, lContainer.nextIndex, viewBlockId);
 
-  const existingView: LViewNode|false =
-      !creationMode && lContainer.nextIndex < views.length && views[lContainer.nextIndex];
-  let viewUpdateMode = existingView && viewBlockId === (existingView as LViewNode).data.id;
-
-  if (viewUpdateMode) {
-    previousOrParentNode = views[lContainer.nextIndex++];
+  if (existingViewNode) {
+    previousOrParentNode = existingViewNode;
     ngDevMode && assertNodeType(previousOrParentNode, LNodeFlags.View);
     isParent = true;
-    enterView((existingView as LViewNode).data, previousOrParentNode as LViewNode);
+    enterView((existingViewNode as LViewNode).data, existingViewNode as LViewNode);
   } else {
     // When we create a new LView, we always reset the state of the instructions.
     const newView = createLView(
@@ -1254,10 +1280,9 @@ export function embeddedViewStart(viewBlockId: number): boolean {
     }
 
     enterView(newView, createLNode(null, LNodeFlags.View, null, newView));
-    lContainer.nextIndex++;
   }
 
-  return !viewUpdateMode;
+  return !existingViewNode;
 }
 
 /**
@@ -1286,19 +1311,18 @@ export function embeddedViewEnd(): void {
   refreshChildComponents();
   isParent = false;
   const viewNode = previousOrParentNode = currentView.node as LViewNode;
-  const container = previousOrParentNode.parent as LContainerNode;
-  if (container) {
+  const containerNode = previousOrParentNode.parent as LContainerNode;
+  if (containerNode) {
     ngDevMode && assertNodeType(viewNode, LNodeFlags.View);
-    ngDevMode && assertNodeType(container, LNodeFlags.Container);
-    const containerState = container.data;
-    const previousView = containerState.nextIndex <= containerState.views.length ?
-        containerState.views[containerState.nextIndex - 1] as LViewNode :
-        null;
-    const viewIdChanged = previousView == null ? true : previousView.data.id !== viewNode.data.id;
-
-    if (viewIdChanged) {
-      insertView(container, viewNode, containerState.nextIndex - 1);
+    ngDevMode && assertNodeType(containerNode, LNodeFlags.Container);
+    const lContainer = containerNode.data;
+
+    if (creationMode) {
+      // it is a new view, insert it into collection of views for a given container
+      insertView(containerNode, viewNode, lContainer.nextIndex);
     }
+
+    lContainer.nextIndex++;
   }
   leaveView(currentView !.parent !);
   ngDevMode && assertEqual(isParent, false, 'isParent');

packages/core/src/render3/node_manipulation.ts

@@ -270,18 +270,13 @@ export function insertView(
     setViewNext(views[index - 1], newView);
   }
 
-  if (index < views.length && views[index].data.id !== newView.data.id) {
-    // View ID change replace the view.
+  if (index < views.length) {
     setViewNext(newView, views[index]);
     views.splice(index, 0, newView);
-  } else if (index >= views.length) {
+  } else {
     views.push(newView);
   }
 
-  if (state.nextIndex <= index) {
-    state.nextIndex++;
-  }
-
   // If the container's renderParent is null, we know that it is a root node of its own parent view
   // and we should wait until that parent processes its nodes (otherwise, we will insert this view's
   // nodes twice - once now and once when its parent inserts its views).

packages/core/test/render3/control_flow_spec.ts

@@ -125,6 +125,54 @@ describe('JS control flow', () => {
     expect(renderToHtml(Template, ctx)).toEqual('<div><span>Hello</span></div>');
   });
 
+  it('should work with adjacent if blocks managing views in the same container', () => {
+
+    const ctx = {condition1: true, condition2: true, condition3: true};
+
+    /**
+    *   % if(ctx.condition1) {
+    *     1
+    *   % }; if(ctx.condition2) {
+    *     2
+    *   % }; if(ctx.condition3) {
+    *     3
+    *   % }
+    */
+    function Template(ctx: any, cm: boolean) {
+      if (cm) {
+        container(0);
+      }
+      containerRefreshStart(0);
+      if (ctx.condition1) {
+        const cm1 = embeddedViewStart(1);
+        if (cm1) {
+          text(0, '1');
+        }
+        embeddedViewEnd();
+      }  // can't have ; here due linting rules
+      if (ctx.condition2) {
+        const cm2 = embeddedViewStart(2);
+        if (cm2) {
+          text(0, '2');
+        }
+        embeddedViewEnd();
+      }  // can't have ; here due linting rules
+      if (ctx.condition3) {
+        const cm3 = embeddedViewStart(3);
+        if (cm3) {
+          text(0, '3');
+        }
+        embeddedViewEnd();
+      }
+      containerRefreshEnd();
+    }
+
+    expect(renderToHtml(Template, ctx)).toEqual('123');
+
+    ctx.condition2 = false;
+    expect(renderToHtml(Template, ctx)).toEqual('13');
+  });
+
   it('should work with containers with views as parents', () => {
     function Template(ctx: any, cm: boolean) {
       if (cm) {
@@ -490,6 +538,15 @@ describe('JS for loop', () => {
     const ctx: {data1: string[] | null,
                 data2: number[] | null} = {data1: ['a', 'b', 'c'], data2: [1, 2]};
 
+    /**
+     * <div>
+     *    % for (let i = 0; i < ctx.data1.length; i++) {
+     *        {{data1[i]}}
+     *    % } for (let j = 0; j < ctx.data2.length; j++) {
+     *        {{data1[j]}}
+     *    % }
+     * </div>
+     */
     function Template(ctx: any, cm: boolean) {
       if (cm) {
         elementStart(0, 'div');