New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(core): not inserting ViewContainerRef nodes when inside root of a component #39599
Conversation
fbffcd5
to
60273a3
Compare
const tNode = tData[parentTNode.index] as TNode; | ||
const encapsulation = (tData[tNode.directiveStart] as ComponentDef<any>).encapsulation; | ||
const encapsulation = | ||
(tView.data[parentTNode.directiveStart] as ComponentDef<unknown>).encapsulation; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes aren't required for the fix, I just noticed that the tNode
and parentTNode
are always the same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but please take care of @AndrewKushnir 's comments
… component 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 angular#39556.
…ot of a component
60273a3
to
88a9ca3
Compare
@AndrewKushnir the feedback has been addressed. |
@crisbeto FYI presubmit is successful for the changes in this PR. The missing piece is the "target" label, could you please set one when you get a chance? Thank you. |
… 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
… component (angular#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 angular#39556. PR Close angular#39599
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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 theappendChild
helper fromnode_manipulation.ts
in most cases.The problem with using
appendChild
here is that it has some extra logic which doesn't return a parentRNode
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 aViewContainerRef
for a non-component node at the root of another component like the following: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 theViewContainerRef
from being inserted into the DOM.These changes resolve the issue by not going through
appendChild
at all when creating a comment node forViewContainerRef
. This should work identically sinceappendChild
doesn't really do anything with the T structures anyway, it only uses them to reach the relevant DOM nodes.Fixes #39556.