From 1a759ecfd4025d8efc50a82be842631d8fa401c2 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Wed, 4 Sep 2019 20:44:38 +0200 Subject: [PATCH] fix(ivy): node placed in incorrect order inside ngFor with ng-container Fixes an issue where Ivy incorrectly inserts items in the beginning of an `ngFor`, if the `ngFor` is set on an `ng-container`. The issue comes from the fact that we choose the `ng-container` comment node as the anchor point before which to insert the content, however the node might be after any of the nodes inside the container. These changes switch to picking out the first node inside of the container instead. --- .../core/src/render3/node_manipulation.ts | 20 +- .../acceptance/view_container_ref_spec.ts | 172 ++++++++++++++++++ 2 files changed, 187 insertions(+), 5 deletions(-) diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index 0fa98dc9efd8b..9497e6b4e9df3 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -669,12 +669,22 @@ export function getBeforeNodeForView(viewIndexInContainer: number, lContainer: L if (nextViewIndex < lContainer.length) { const lView = lContainer[nextViewIndex] as LView; ngDevMode && assertDefined(lView[T_HOST], 'Missing Host TNode'); - const tViewNodeChild = (lView[T_HOST] as TViewNode).child; - return tViewNodeChild !== null ? getNativeByTNodeOrNull(tViewNodeChild, lView) : - lContainer[NATIVE]; - } else { - return lContainer[NATIVE]; + let tViewNodeChild = (lView[T_HOST] as TViewNode).child; + if (tViewNodeChild !== null) { + if (tViewNodeChild.type === TNodeType.ElementContainer || + tViewNodeChild.type === TNodeType.IcuContainer) { + let currentChild = tViewNodeChild.child; + while (currentChild && (currentChild.type === TNodeType.ElementContainer || + currentChild.type === TNodeType.IcuContainer)) { + currentChild = currentChild.child; + } + tViewNodeChild = currentChild || tViewNodeChild; + } + return getNativeByTNodeOrNull(tViewNodeChild, lView); + } } + + return lContainer[NATIVE]; } /** diff --git a/packages/core/test/acceptance/view_container_ref_spec.ts b/packages/core/test/acceptance/view_container_ref_spec.ts index a210dc32257e4..8fba78211182f 100644 --- a/packages/core/test/acceptance/view_container_ref_spec.ts +++ b/packages/core/test/acceptance/view_container_ref_spec.ts @@ -1276,6 +1276,178 @@ describe('ViewContainerRef', () => { ''); }); + it('should insert elements in the proper order when template root is an ng-container', () => { + @Component({ + template: ` + |{{ item }}| + ` + }) + class App { + items = ['one', 'two', 'three']; + } + + TestBed.configureTestingModule({imports: [CommonModule], declarations: [App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('|one||two||three|'); + + fixture.componentInstance.items.unshift('zero'); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('|zero||one||two||three|'); + + fixture.componentInstance.items.push('four'); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('|zero||one||two||three||four|'); + + fixture.componentInstance.items.splice(3, 0, 'two point five'); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent) + .toBe('|zero||one||two||two point five||three||four|'); + }); + + it('should insert elements in the proper order when template root is an ng-container and is wrapped by an ng-container', + () => { + @Component({ + template: ` + + |{{ item }}| + + ` + }) + class App { + items = ['one', 'two', 'three']; + } + + TestBed.configureTestingModule({imports: [CommonModule], declarations: [App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('|one||two||three|'); + + fixture.componentInstance.items.unshift('zero'); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('|zero||one||two||three|'); + + fixture.componentInstance.items.push('four'); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('|zero||one||two||three||four|'); + + fixture.componentInstance.items.splice(3, 0, 'two point five'); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent) + .toBe('|zero||one||two||two point five||three||four|'); + }); + + it('should insert elements in the proper order when template root is an ng-container and first node is a ng-container', + () => { + @Component({ + template: ` + |{{ item }}| + ` + }) + class App { + items = ['one', 'two', 'three']; + } + + TestBed.configureTestingModule({imports: [CommonModule], declarations: [App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('|one||two||three|'); + + fixture.componentInstance.items.unshift('zero'); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('|zero||one||two||three|'); + + fixture.componentInstance.items.push('four'); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('|zero||one||two||three||four|'); + + fixture.componentInstance.items.splice(3, 0, 'two point five'); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent) + .toBe('|zero||one||two||two point five||three||four|'); + }); + + it('should insert elements in the proper order when template root is an ng-container, wrapped in an ng-container with the root node as an ng-container', + () => { + @Component({ + template: ` + + |{{ item }}| + + ` + }) + class App { + items = ['one', 'two', 'three']; + } + + TestBed.configureTestingModule({imports: [CommonModule], declarations: [App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('|one||two||three|'); + + fixture.componentInstance.items.unshift('zero'); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('|zero||one||two||three|'); + + fixture.componentInstance.items.push('four'); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('|zero||one||two||three||four|'); + + fixture.componentInstance.items.splice(3, 0, 'two point five'); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent) + .toBe('|zero||one||two||two point five||three||four|'); + }); + + it('should insert elements in the proper order when the first child node is an ICU expression', + () => { + @Component({ + template: ` + {count, select, other {|{{ item }}|}} + ` + }) + class App { + items = ['one', 'two', 'three']; + } + + TestBed.configureTestingModule({imports: [CommonModule], declarations: [App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('|one||two||three|'); + + fixture.componentInstance.items.unshift('zero'); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('|zero||one||two||three|'); + + fixture.componentInstance.items.push('four'); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('|zero||one||two||three||four|'); + + fixture.componentInstance.items.splice(3, 0, 'two point five'); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent) + .toBe('|zero||one||two||two point five||three||four|'); + }); }); describe('lifecycle hooks', () => {