From c09bd67aee53b5c3156f953176ad586eb784b83c Mon Sep 17 00:00:00 2001 From: Pawel Kozlowski Date: Thu, 8 Mar 2018 12:10:20 +0100 Subject: [PATCH] fix(ivy): fix views manipulation logic (#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 #22656 --- packages/core/src/render3/di.ts | 9 +++ packages/core/src/render3/instructions.ts | 66 +++++++++++++------ .../core/src/render3/node_manipulation.ts | 9 +-- .../core/test/render3/control_flow_spec.ts | 57 ++++++++++++++++ 4 files changed, 113 insertions(+), 28 deletions(-) diff --git a/packages/core/src/render3/di.ts b/packages/core/src/render3/di.ts index a53438d1c7..c63ca32ad8 100644 --- 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)._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 index 36720f7cc8..1627ad66e5 100644 --- 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; + ngDevMode && assertNodeType(containerNode, LNodeFlags.Container); + const lContainer = containerNode.data; - if (viewIdChanged) { - insertView(container, viewNode, containerState.nextIndex - 1); + 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 index 41a12425ea..6a71ecaadf 100644 --- 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 index 4337df5d18..462722988f 100644 --- 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('
Hello
'); }); + 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]}; + /** + *
+ * % for (let i = 0; i < ctx.data1.length; i++) { + * {{data1[i]}} + * % } for (let j = 0; j < ctx.data2.length; j++) { + * {{data1[j]}} + * % } + *
+ */ function Template(ctx: any, cm: boolean) { if (cm) { elementStart(0, 'div');