From fa6cbb3ffe2e8de635901421a0411cbefd826c99 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Wed, 22 May 2019 13:21:46 +0200 Subject: [PATCH] fix(ivy): ng-container with ViewContainerRef creates two comments (#30611) With Ivy, injecting a `ViewContainerRef` for a `` element results in two comments generated in the DOM. One comment as host element for the `ElementContainer` and one for the generated `LContainer` which is needed for the created `ViewContainerRef`. This is problematic as developers expect the same anchor element for the `LContainer` and the `ElementContainer` in order to be able to move the host element of `` without leaving the actual `LContainer` anchor element at the original location. This worked differently in View Engine and various applications might depend on the behavior where the `ViewContainerRef` shares the anchor comment node with the host comment node of the ``. For example `CdkTable` from `@angular/cdk` moves around the host element of a `` and also expects embedded views to be inserted next to the moved `` host element. See: https://github.com/angular/components/blob/f8be5329f8d94128ece5dfe3e8e998fe4077d44d/src/cdk/table/table.ts#L999-L1016 Resolves FW-1341 PR Close #30611 --- .../src/render3/view_engine_compatibility.ts | 15 +++-- packages/core/test/acceptance/i18n_spec.ts | 4 +- .../acceptance/view_container_ref_spec.ts | 57 ++++++++++++++++++- 3 files changed, 69 insertions(+), 7 deletions(-) diff --git a/packages/core/src/render3/view_engine_compatibility.ts b/packages/core/src/render3/view_engine_compatibility.ts index cacee5f0cc..d6fde0e553 100644 --- a/packages/core/src/render3/view_engine_compatibility.ts +++ b/packages/core/src/render3/view_engine_compatibility.ts @@ -19,7 +19,7 @@ import {assertDefined, assertGreaterThan, assertLessThan} from '../util/assert'; import {NodeInjector, getParentInjectorLocation} from './di'; import {addToViewTree, createEmbeddedViewAndNode, createLContainer, renderEmbeddedTemplate} from './instructions/shared'; -import {ACTIVE_INDEX, CONTAINER_HEADER_OFFSET, LContainer, NATIVE} from './interfaces/container'; +import {ACTIVE_INDEX, CONTAINER_HEADER_OFFSET, LContainer} from './interfaces/container'; import {TContainerNode, TElementContainerNode, TElementNode, TNode, TNodeType, TViewNode} from './interfaces/node'; import {RComment, RElement, isProceduralRenderer} from './interfaces/renderer'; import {CONTEXT, LView, QUERIES, RENDERER, TView, T_HOST} from './interfaces/view'; @@ -29,7 +29,7 @@ import {getParentInjectorTNode} from './node_util'; import {getLView, getPreviousOrParentTNode} from './state'; import {getParentInjectorView, hasParentInjector} from './util/injector_utils'; import {findComponentView} from './util/view_traversal_utils'; -import {getComponentViewByIndex, getNativeByTNode, isComponent, isLContainer, isRootView, viewAttachedToContainer} from './util/view_utils'; +import {getComponentViewByIndex, getNativeByTNode, isComponent, isLContainer, isRootView, unwrapRNode, viewAttachedToContainer} from './util/view_utils'; import {ViewRef} from './view_ref'; @@ -313,8 +313,15 @@ export function createContainerRef( lContainer = slotValue; lContainer[ACTIVE_INDEX] = -1; } else { - const commentNode = hostView[RENDERER].createComment(ngDevMode ? 'container' : ''); - ngDevMode && ngDevMode.rendererCreateComment++; + let commentNode: RComment; + // If the host is an element container, the native host element is guaranteed to be a + // comment and we can reuse that comment as anchor element for the new LContainer. + if (hostTNode.type === TNodeType.ElementContainer) { + commentNode = unwrapRNode(slotValue) as RComment; + } else { + ngDevMode && ngDevMode.rendererCreateComment++; + commentNode = hostView[RENDERER].createComment(ngDevMode ? 'container' : ''); + } // A container can be created on the root (topmost / bootstrapped) component and in this case we // can't use LTree to insert container's marker node (both parent of a comment node and the diff --git a/packages/core/test/acceptance/i18n_spec.ts b/packages/core/test/acceptance/i18n_spec.ts index 647d86c5ec..9679058eb2 100644 --- a/packages/core/test/acceptance/i18n_spec.ts +++ b/packages/core/test/acceptance/i18n_spec.ts @@ -937,14 +937,14 @@ onlyInIvy('Ivy i18n logic').describe('runtime i18n', () => { fixture.detectChanges(); expect(q.query.length).toEqual(1); expect(toHtml(fixture.nativeElement)) - .toEqual(`Contenu`); + .toEqual(`Contenu`); // Disable ng-if fixture.componentInstance.visible = false; fixture.detectChanges(); expect(q.query.length).toEqual(0); expect(toHtml(fixture.nativeElement)) - .toEqual(`Contenu`); + .toEqual(`Contenu`); }); }); }); diff --git a/packages/core/test/acceptance/view_container_ref_spec.ts b/packages/core/test/acceptance/view_container_ref_spec.ts index 98048b7546..ca428508b4 100644 --- a/packages/core/test/acceptance/view_container_ref_spec.ts +++ b/packages/core/test/acceptance/view_container_ref_spec.ts @@ -61,6 +61,59 @@ describe('ViewContainerRef', () => { expect(fixture.componentInstance.foo).toBeAnInstanceOf(TemplateRef); }); + it('should use comment node of host ng-container as insertion marker', () => { + @Component({template: 'hello'}) + class HelloComp { + } + + @NgModule({entryComponents: [HelloComp], declarations: [HelloComp]}) + class HelloCompModule { + } + + @Component({ + template: ` + + ` + }) + class TestComp { + @ViewChild(VCRefDirective, {static: true}) vcRefDir !: VCRefDirective; + } + + TestBed.configureTestingModule( + {declarations: [TestComp, VCRefDirective], imports: [HelloCompModule]}); + const fixture = TestBed.createComponent(TestComp); + const {vcref, cfr, elementRef} = fixture.componentInstance.vcRefDir; + fixture.detectChanges(); + + expect(fixture.nativeElement.innerHTML) + .toMatch(//, 'Expected only one comment node to be generated.'); + + const testParent = document.createElement('div'); + testParent.appendChild(elementRef.nativeElement); + + expect(testParent.textContent).toBe(''); + expect(testParent.childNodes.length).toBe(1); + expect(testParent.childNodes[0].nodeType).toBe(Node.COMMENT_NODE); + + // Add a test component to the view container ref to ensure that + // the "ng-container" comment was used as marker for the insertion. + vcref.createComponent(cfr.resolveComponentFactory(HelloComp)); + fixture.detectChanges(); + + expect(testParent.textContent).toBe('hello'); + expect(testParent.childNodes.length).toBe(2); + + // With Ivy, views are inserted before the container comment marker. + if (ivyEnabled) { + expect(testParent.childNodes[0].nodeType).toBe(Node.ELEMENT_NODE); + expect(testParent.childNodes[0].textContent).toBe('hello'); + expect(testParent.childNodes[1].nodeType).toBe(Node.COMMENT_NODE); + } else { + expect(testParent.childNodes[0].nodeType).toBe(Node.COMMENT_NODE); + expect(testParent.childNodes[1].nodeType).toBe(Node.ELEMENT_NODE); + expect(testParent.childNodes[1].textContent).toBe('hello'); + } + }); }); describe('insert', () => { @@ -1668,7 +1721,9 @@ class VCRefDirective { // Injecting the ViewContainerRef to create a dynamic container in which // embedded views will be created - constructor(public vcref: ViewContainerRef, public cfr: ComponentFactoryResolver) {} + constructor( + public vcref: ViewContainerRef, public cfr: ComponentFactoryResolver, + public elementRef: ElementRef) {} createView(s: string, index?: number): EmbeddedViewRef { if (!this.tplRef) {