Skip to content
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): ComponentFactory.create should clear host element content #33487

Conversation

@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Oct 30, 2019

Prior to this change, ComponentFactory.create function invocation in Ivy retained the content of the host element (in case host element reference or CSS selector is provided as an argument). This behavior is different in View Engine, where the content of the host element was cleared, except for the case when ShadowDom encapsulation is used (to make sure native slot projection works). This commit aligns Ivy and View Engine and makes sure the host element is cleared before component content insertion.

This PR resolves FW-1649.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No
@ngbot ngbot bot modified the milestone: needsTriage Oct 30, 2019
@googlebot googlebot added the cla: yes label Oct 30, 2019
@AndrewKushnir AndrewKushnir force-pushed the AndrewKushnir:FW-1649_componentfactory_create branch 2 times, most recently from 072bc85 to 74d63d8 Nov 1, 2019
@AndrewKushnir AndrewKushnir marked this pull request as ready for review Nov 2, 2019
@AndrewKushnir AndrewKushnir requested review from angular/fw-compiler as code owners Nov 2, 2019
@AndrewKushnir AndrewKushnir force-pushed the AndrewKushnir:FW-1649_componentfactory_create branch from 74d63d8 to 0aaf5c1 Nov 9, 2019
@AndrewKushnir AndrewKushnir requested a review from atscott Nov 9, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor Author

AndrewKushnir commented Nov 9, 2019

Copy link
Contributor

atscott left a comment

LGTM. As discussed, please add a test case covering host elements with more than just a text node.

@AndrewKushnir AndrewKushnir force-pushed the AndrewKushnir:FW-1649_componentfactory_create branch from 6b92714 to 942d964 Nov 11, 2019
@pkozlowski-opensource

This comment has been minimized.

Copy link
Member

pkozlowski-opensource commented Nov 12, 2019

Initially I thought that this change limits our ability to fix #4946 but after digging into it more, I don't think it blocks anything. WE can go ahead with this PR from the existing GH issues point of view.

@kara
kara approved these changes Nov 12, 2019
Copy link
Contributor

kara left a comment

LGTM, once typo is fixed

packages/core/src/render3/instructions/shared.ts Outdated Show resolved Hide resolved
@kara kara removed their assignment Nov 12, 2019
Prior to this change, ComponentFactory.create function invocation in Ivy retained the content of the host element (in case host element reference or CSS seelctor is provided as an argument). This behavior is different in View Engine, where the content of the host element was cleared, except for the case when ShadowDom encapsulation is used (to make sure native slot projection works). This commit aligns Ivy and View Engine and makes sure the host element is cleared before component content insertion.
@AndrewKushnir AndrewKushnir force-pushed the AndrewKushnir:FW-1649_componentfactory_create branch from 3ce43bb to af46b66 Nov 12, 2019
kara added a commit that referenced this pull request Nov 13, 2019
…33487)

Prior to this change, ComponentFactory.create function invocation in Ivy retained the content of the host element (in case host element reference or CSS seelctor is provided as an argument). This behavior is different in View Engine, where the content of the host element was cleared, except for the case when ShadowDom encapsulation is used (to make sure native slot projection works). This commit aligns Ivy and View Engine and makes sure the host element is cleared before component content insertion.

PR Close #33487
@kara kara closed this in cf10b33 Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.