-
Notifications
You must be signed in to change notification settings - Fork 26.9k
fix(ivy): ng-container with ViewContainerRef creates two comments #30611
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(ivy): ng-container with ViewContainerRef creates two comments #30611
Conversation
With Ivy, injecting a `ViewContainerRef` for a `<ng-container>` 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 `<ng-container>` 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 `<ng-container>`. For example `CdkTable` from `@angular/cdk` moves around the host element of a `<ng-container>` and also expects embedded views to be inserted next to the moved `<ng-container>` host element. See: https://github.com/angular/components/blob/f8be5329f8d94128ece5dfe3e8e998fe4077d44d/src/cdk/table/table.ts#L999-L1016 Resolves FW-1341
|
@devversion thnx, the change itself LGTM. I was just wondering if you could add an acceptance test for the exact use-case that was discovered in Material (moving around comment node). WDYT? |
|
@pkozlowski-opensource Thanks for reviewing. The acceptance test I added should already cover that scenario. It's not an exact test-case but covers the exact same use-case. can you please re-check and let me know if this is sufficient? Thanks! |
kara
left a comment
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, one optional nit
|
merge assistance: code fresh delayed |
|
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. |


With Ivy, injecting a
ViewContainerReffor a<ng-container>elementresults in two comments generated in the DOM. One comment as host
element for the
ElementContainerand one for the generatedLContainerwhich is needed for the created
ViewContainerRef.This is problematic as developers expect the same anchor element for
the
LContainerand theElementContainerin order to be able to movethe host element of
<ng-container>without leaving the actualLContaineranchor element at the original location.This worked differently in View Engine and various applications might
depend on the behavior where the
ViewContainerRefshares the anchorcomment node with the host comment node of the
<ng-container>. Forexample
CdkTablefrom@angular/cdkmoves around the host element ofa
<ng-container>and also expects embedded views to be inserted nextto the moved
<ng-container>host element.See: https://github.com/angular/components/blob/f8be5329f8d94128ece5dfe3e8e998fe4077d44d/src/cdk/table/table.ts#L999-L1016
Resolves FW-1341