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

pkozlowski-opensource · kara · commit c09bd67aee53 · 2018-03-14T12:07:15.000-07:00
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
diff --git a/packages/core/src/render3/di.ts b/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) {
diff --git a/packages/core/src/render3/instructions.ts b/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');
diff --git a/packages/core/src/render3/node_manipulation.ts b/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).
diff --git a/packages/core/test/render3/control_flow_spec.ts b/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');
