Skip to content

Commit 20db90a

Browse files
crisbetoatscott
crisbeto
authored and
atscott
committedNov 12, 2020
fix(core): not inserting ViewContainerRef nodes when inside root of a component (#39599)
When a `ViewContainerRef` is injected, we dynamically create a comment node next to the host so that it can be used as an anchor point for inserting views. The comment node is inserted through the `appendChild` helper from `node_manipulation.ts` in most cases. The problem with using `appendChild` here is that it has some extra logic which doesn't return a parent `RNode` if an element is at the root of a component. I __think__ that this is a performance optimization which is used to avoid inserting an element in one place in the DOM and then moving it a bit later when it is projected. This can break down in some cases when creating a `ViewContainerRef` for a non-component node at the root of another component like the following: ``` <root> <div #viewContainerRef></div> </root> ``` In this case the `#viewContainerRef` node is at the root of a component so we intentionally don't insert it, but since its anchor element was created manually, it'll never be projected. This will prevent any views added through the `ViewContainerRef` from being inserted into the DOM. These changes resolve the issue by not going through `appendChild` at all when creating a comment node for `ViewContainerRef`. This should work identically since `appendChild` doesn't really do anything with the T structures anyway, it only uses them to reach the relevant DOM nodes. Fixes #39556. PR Close #39599
·
11.0.911.0.1
1 parent 050cea9 commit 20db90a

File tree

6 files changed

+60
-45
lines changed

6 files changed

+60
-45
lines changed
 

‎packages/core/src/render3/node_manipulation.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -557,9 +557,8 @@ export function getClosestRElement(tView: TView, tNode: TNode|null, lView: LView
557557
ngDevMode && assertTNodeType(parentTNode, TNodeType.AnyRNode | TNodeType.Container);
558558
if (parentTNode.flags & TNodeFlags.isComponentHost) {
559559
ngDevMode && assertTNodeForLView(parentTNode, lView);
560-
const tData = tView.data;
561-
const tNode = tData[parentTNode.index] as TNode;
562-
const encapsulation = (tData[tNode.directiveStart] as ComponentDef<any>).encapsulation;
560+
const encapsulation =
561+
(tView.data[parentTNode.directiveStart] as ComponentDef<unknown>).encapsulation;
563562
// We've got a parent which is an element in the current view. We just need to verify if the
564563
// parent element is not a component. Component's content nodes are not inserted immediately
565564
// because they will be projected, and so doing insert at this point would be wasteful.

‎packages/core/src/render3/view_engine_compatibility.ts

Lines changed: 14 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -20,15 +20,15 @@ import {assertDefined, assertEqual, assertGreaterThan, assertLessThan} from '../
2020

2121
import {assertLContainer, assertNodeInjector} from './assert';
2222
import {getParentInjectorLocation, NodeInjector} from './di';
23-
import {addToViewTree, createLContainer, createLView, createTNode, renderView} from './instructions/shared';
23+
import {addToViewTree, createLContainer, createLView, renderView} from './instructions/shared';
2424
import {CONTAINER_HEADER_OFFSET, LContainer, NATIVE, VIEW_REFS} from './interfaces/container';
2525
import {NodeInjectorOffset} from './interfaces/injector';
2626
import {TContainerNode, TDirectiveHostNode, TElementContainerNode, TElementNode, TNode, TNodeType} from './interfaces/node';
2727
import {isProceduralRenderer, RComment, RElement} from './interfaces/renderer';
28-
import {isComponentHost, isLContainer, isLView, isRootView} from './interfaces/type_checks';
28+
import {isComponentHost, isLContainer, isLView} from './interfaces/type_checks';
2929
import {DECLARATION_COMPONENT_VIEW, DECLARATION_LCONTAINER, LView, LViewFlags, PARENT, QUERIES, RENDERER, T_HOST, TVIEW, TView} from './interfaces/view';
3030
import {assertTNodeType} from './node_assert';
31-
import {addViewToContainer, appendChild, destroyLView, detachView, getBeforeNodeForView, insertView, nativeInsertBefore, nativeNextSibling, nativeParentNode} from './node_manipulation';
31+
import {addViewToContainer, destroyLView, detachView, getBeforeNodeForView, insertView, nativeInsertBefore, nativeNextSibling, nativeParentNode} from './node_manipulation';
3232
import {getCurrentTNode, getLView} from './state';
3333
import {getParentInjectorIndex, getParentInjectorView, hasParentInjector} from './util/injector_utils';
3434
import {getComponentLViewByIndex, getNativeByTNode, unwrapRNode, viewAttachedToContainer} from './util/view_utils';
@@ -373,28 +373,18 @@ export function createContainerRef(
373373
if (hostTNode.type & TNodeType.ElementContainer) {
374374
commentNode = unwrapRNode(slotValue) as RComment;
375375
} else {
376+
// If the host is a regular element, we have to insert a comment node manually which will
377+
// be used as an anchor when inserting elements. In this specific case we use low-level DOM
378+
// manipulation to insert it.
379+
const renderer = hostView[RENDERER];
376380
ngDevMode && ngDevMode.rendererCreateComment++;
377-
commentNode = hostView[RENDERER].createComment(ngDevMode ? 'container' : '');
378-
379-
// A `ViewContainerRef` can be injected by the root (topmost / bootstrapped) component. In
380-
// this case we can't use TView / TNode data structures to insert container's marker node
381-
// (both a parent of a comment node and the comment node itself are not part of any view). In
382-
// this specific case we use low-level DOM manipulation to insert container's marker (comment)
383-
// node.
384-
if (isRootView(hostView)) {
385-
const renderer = hostView[RENDERER];
386-
const hostNative = getNativeByTNode(hostTNode, hostView)!;
387-
const parentOfHostNative = nativeParentNode(renderer, hostNative);
388-
nativeInsertBefore(
389-
renderer, parentOfHostNative!, commentNode, nativeNextSibling(renderer, hostNative),
390-
false);
391-
} else {
392-
// The TNode created here is bogus, in that it is not added to the TView. It is only created
393-
// to allow us to create a dynamic Comment node.
394-
const commentTNode =
395-
createTNode(hostView[TVIEW], hostTNode.parent, TNodeType.Container, 0, null, null);
396-
appendChild(hostView[TVIEW], hostView, commentNode, commentTNode);
397-
}
381+
commentNode = renderer.createComment(ngDevMode ? 'container' : '');
382+
383+
const hostNative = getNativeByTNode(hostTNode, hostView)!;
384+
const parentOfHostNative = nativeParentNode(renderer, hostNative);
385+
nativeInsertBefore(
386+
renderer, parentOfHostNative!, commentNode, nativeNextSibling(renderer, hostNative),
387+
false);
398388
}
399389

400390
hostView[hostTNode.index] = lContainer =

‎packages/core/test/acceptance/component_spec.ts

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,50 @@ describe('component', () => {
9696
expect(fixture.nativeElement).toHaveText('foo|bar');
9797
});
9898

99+
it('should be able to dynamically insert a component into a view container at the root of a component',
100+
() => {
101+
@Component({template: 'hello'})
102+
class HelloComponent {
103+
}
104+
105+
// TODO: This module is only used to declare the `entryComponets` since
106+
// `configureTestingModule` doesn't support it. The module can be removed
107+
// once ViewEngine is removed.
108+
@NgModule({
109+
declarations: [HelloComponent],
110+
exports: [HelloComponent],
111+
entryComponents: [HelloComponent]
112+
})
113+
class HelloModule {
114+
}
115+
116+
@Component({selector: 'wrapper', template: '<ng-content></ng-content>'})
117+
class Wrapper {
118+
}
119+
120+
@Component({
121+
template: `
122+
<wrapper>
123+
<div #insertionPoint></div>
124+
</wrapper>
125+
`
126+
})
127+
class App {
128+
@ViewChild('insertionPoint', {read: ViewContainerRef}) viewContainerRef!: ViewContainerRef;
129+
constructor(public componentFactoryResolver: ComponentFactoryResolver) {}
130+
}
131+
132+
TestBed.configureTestingModule({declarations: [App, Wrapper], imports: [HelloModule]});
133+
const fixture = TestBed.createComponent(App);
134+
fixture.detectChanges();
135+
136+
const instance = fixture.componentInstance;
137+
const factory = instance.componentFactoryResolver.resolveComponentFactory(HelloComponent);
138+
instance.viewContainerRef.createComponent(factory);
139+
140+
expect(fixture.nativeElement.textContent.trim()).toBe('hello');
141+
});
142+
99143
// TODO: add tests with Native once tests run in real browser (domino doesn't support shadow root)
100144
describe('encapsulation', () => {
101145
@Component({

‎packages/core/test/bundling/forms/bundle.golden_symbols.json

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -824,9 +824,6 @@
824824
{
825825
"name": "createPlatformFactory"
826826
},
827-
{
828-
"name": "createTNode"
829-
},
830827
{
831828
"name": "createTView"
832829
},
@@ -1244,9 +1241,6 @@
12441241
{
12451242
"name": "isPropertyUpdated"
12461243
},
1247-
{
1248-
"name": "isRootView"
1249-
},
12501244
{
12511245
"name": "isScheduler"
12521246
},

‎packages/core/test/bundling/router/bundle.golden_symbols.json

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1070,9 +1070,6 @@
10701070
{
10711071
"name": "createRouterScroller"
10721072
},
1073-
{
1074-
"name": "createTNode"
1075-
},
10761073
{
10771074
"name": "createTView"
10781075
},
@@ -1568,9 +1565,6 @@
15681565
{
15691566
"name": "isPromise"
15701567
},
1571-
{
1572-
"name": "isRootView"
1573-
},
15741568
{
15751569
"name": "isScheduler"
15761570
},

‎packages/core/test/bundling/todo/bundle.golden_symbols.json

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -260,9 +260,6 @@
260260
{
261261
"name": "createLView"
262262
},
263-
{
264-
"name": "createTNode"
265-
},
266263
{
267264
"name": "createTView"
268265
},
@@ -518,9 +515,6 @@
518515
{
519516
"name": "isProceduralRenderer"
520517
},
521-
{
522-
"name": "isRootView"
523-
},
524518
{
525519
"name": "isStylingMatch"
526520
},

0 commit comments

Comments
 (0)
Please sign in to comment.