fix(ivy): remove DOM nodes from their real parent vs saved parent #28455

jelbourn · 2019-01-30T18:26:50Z

Currently, DOM node removal called removeChild on the saved parent
node when destroying a component. However, this will fail if the
component has been manually moved in the DOM. This change makes the
removal always use the node's real parentNode and ignore the provided
parent.

Reference: FW-977

packages/core/src/render3/node_manipulation.ts

packages/core/test/render3/view_container_ref_spec.ts

pkozlowski-opensource · 2019-01-31T15:23:03Z

packages/core/src/render3/node_manipulation.ts

@@ -598,8 +598,16 @@ function nativeAppendOrInsertBefore(
 */
 export function nativeRemoveChild(
    renderer: Renderer3, parent: RElement, child: RNode, isHostElement?: boolean): void {
-  isProceduralRenderer(renderer) ? renderer.removeChild(parent as RElement, child, isHostElement) :
-                                   parent.removeChild(child);
+  if (isProceduralRenderer(renderer)) {


If we do this logic here in nativeRemoveChild it will mean that we still execute all the logic to find a logical render parent and this is useless. I would rather modify the removeChild function and there lookup a parent from the DOM, instead of executing const parentNative = getRenderParent(childTNode, currentView);

Then the null check be in there as well and you wouldn't need to modify renderer

This is what I originally had and @kara asked to change it. @kara what are your thoughts here?

@jelbourn I believe @pkozlowski-opensource is actually talking about Ivy's version of removeChild here (not the renderer):
https://github.com/angular/angular/blob/master/packages/core/src/render3/node_manipulation.ts#L696

I'd agree with him that we should update that function to remove the logic that searches the tree for the render parent manually.

I totally misread Pawel's original comment; I hadn't seen that getRenderParent function earlier.

In making the change to avoid calling that, I didn't want to move the "get parent node" bit into removeChild because then nativeRemoveChild would have the assumption that it's the right parent node baked into it. So, I instead changed removeChild to removeNode and made it just a pass-through to nativeRemoveChild, which also now does not have parent in its arguments.

Let me know what you think.

pkozlowski-opensource

I think that we should rather:

modify the removeChild function (see my in-line comments);
have a TestBed test on top of / instead of renderer3 runtime test

Happy to help if needed but I would like to very much avoid the situation where we do both logical render parent lookup and lookup in the DOM.

jelbourn

Comments addressed, let me know what you think

jelbourn · 2019-02-01T23:55:41Z

packages/core/src/render3/node_manipulation.ts

@@ -598,8 +598,16 @@ function nativeAppendOrInsertBefore(
 */
 export function nativeRemoveChild(
    renderer: Renderer3, parent: RElement, child: RNode, isHostElement?: boolean): void {
-  isProceduralRenderer(renderer) ? renderer.removeChild(parent as RElement, child, isHostElement) :
-                                   parent.removeChild(child);
+  if (isProceduralRenderer(renderer)) {


I totally misread Pawel's original comment; I hadn't seen that getRenderParent function earlier.

In making the change to avoid calling that, I didn't want to move the "get parent node" bit into removeChild because then nativeRemoveChild would have the assumption that it's the right parent node baked into it. So, I instead changed removeChild to removeNode and made it just a pass-through to nativeRemoveChild, which also now does not have parent in its arguments.

Let me know what you think.

alfaproject · 2019-02-02T08:27:12Z

packages/core/src/render3/node_manipulation.ts

+      renderer.removeChild(renderParent, child, isHostElement);
+    }
+  } else {
+    // We intentionally don't use the given parent node since it may no longer


There is no given parent node anymore, right?

So the idea behind the native[...]() functions in this file that those are here to basically abstract a renderer type being used. So far those functions didn't have any logic and I would love to keep it this way.

I would rather prefer that we keep nativeRemoveChild "dumb" and move all the "logic" into the removeChild function. I find this approach easier to follow and IMO the code is shorter / simpler (less checks / branching logic).

I've pushed a fixup commit I've pushed a fixup commit a529638 to your branch with the changes I'm describing above, please review. to your branch with the changes I'm describing above, please review.

pkozlowski-opensource · 2019-02-04T09:28:50Z

packages/core/src/render3/node_manipulation.ts

-  if (parentNative) {
-    nativeRemoveChild(currentView[RENDERER], parentNative, childEl);
-  }
+export function removeNode(childTNode: TNode, childEl: RNode, currentView: LView): void {


childTNode argument is not used here any more, remove

Actually, I've pushed a fixup commit a529638 to your branch with the changes I'm describing above, please review.

googlebot · 2019-02-04T09:51:35Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

pkozlowski-opensource

LGTM with the fixup commit but we can iterate more if @jelbourn has objections

pkozlowski-opensource · 2019-02-04T15:28:31Z

@jelbourn I've pushed a fixup commit and approved the PR with it. Feel free to apply the merge label if this looks good to you or ask / push changes if you want us to iterate more.

googlebot · 2019-02-04T15:29:14Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

googlebot · 2019-02-05T00:00:22Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2019-02-05T00:03:45Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

jelbourn · 2019-02-05T00:04:15Z

I'm good with the proposed changes

Currently, DOM node removal called `removeChild` on the saved parent node when destroying a component. However, this will fail if the component has been manually moved in the DOM. This change makes the removal always use the node's real `parentNode` and ignore the provided `parent`.

googlebot · 2019-02-05T19:53:49Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2019-02-05T19:54:38Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

matsko · 2019-02-05T19:58:26Z

http://test/OCL:232530052:BASE:232530123:1549396551445:4021aaca

PR Close #28455

angular-automatic-lock-bot · 2019-09-14T15:53:23Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

jelbourn added hotlist: components team Related to Angular CDK or Angular Material area: core Issues related to the framework runtime target: major This PR is targeted for the next major release comp: ivy labels Jan 30, 2019

jelbourn requested a review from a team as a code owner January 30, 2019 18:26

ngbot bot added this to the needsTriage milestone Jan 30, 2019

googlebot added the cla: yes label Jan 30, 2019

jelbourn added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jan 30, 2019

kara requested changes Jan 30, 2019

View reviewed changes

packages/core/src/render3/node_manipulation.ts Outdated Show resolved Hide resolved

packages/core/test/render3/view_container_ref_spec.ts Outdated Show resolved Hide resolved

jelbourn force-pushed the dom-moved-removal branch from 46153c9 to a810dde Compare January 31, 2019 03:20

pkozlowski-opensource reviewed Jan 31, 2019

View reviewed changes

packages/core/test/render3/view_container_ref_spec.ts Outdated Show resolved Hide resolved

pkozlowski-opensource reviewed Jan 31, 2019

View reviewed changes

pkozlowski-opensource requested changes Jan 31, 2019

View reviewed changes

mhevery self-assigned this Jan 31, 2019

kara unassigned mhevery Feb 1, 2019

kara added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 1, 2019

kara assigned mhevery and jelbourn Feb 1, 2019

jelbourn force-pushed the dom-moved-removal branch from a810dde to 3adf1aa Compare February 1, 2019 23:39

jelbourn requested a review from a team as a code owner February 1, 2019 23:39

jelbourn force-pushed the dom-moved-removal branch from 3adf1aa to e8dffca Compare February 1, 2019 23:51

jelbourn commented Feb 1, 2019

View reviewed changes

jelbourn force-pushed the dom-moved-removal branch 3 times, most recently from 02eba52 to b57c765 Compare February 2, 2019 00:25

alfaproject reviewed Feb 2, 2019

View reviewed changes

pkozlowski-opensource reviewed Feb 4, 2019

View reviewed changes

googlebot removed the cla: yes label Feb 4, 2019

pkozlowski-opensource approved these changes Feb 4, 2019

View reviewed changes

kara added cla: yes and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews cla: no labels Feb 4, 2019

jelbourn force-pushed the dom-moved-removal branch from 52df2cd to 71b72cd Compare February 5, 2019 00:00

googlebot added cla: no and removed cla: yes labels Feb 5, 2019

jelbourn added cla: yes and removed cla: no labels Feb 5, 2019

jelbourn added the action: merge The PR is ready for merge by the caretaker label Feb 5, 2019

AndrewKushnir mentioned this pull request Feb 5, 2019

fix(ivy): extended lookup of the next pointer while traversing tNode tree #28533

Closed

3 tasks

jelbourn and others added 2 commits February 5, 2019 11:53

refactor(ivy): move around logic of getting render parent

cf528fa

jelbourn force-pushed the dom-moved-removal branch from 71b72cd to cf528fa Compare February 5, 2019 19:53

googlebot added cla: no and removed cla: yes labels Feb 5, 2019

jelbourn added cla: yes and removed cla: no labels Feb 5, 2019

matsko closed this in 89eac70 Feb 6, 2019

matsko pushed a commit that referenced this pull request Feb 6, 2019

refactor(ivy): move around logic of getting render parent (#28455)

7c5c1fa

PR Close #28455

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ivy): remove DOM nodes from their real parent vs saved parent #28455

fix(ivy): remove DOM nodes from their real parent vs saved parent #28455

jelbourn commented Jan 30, 2019

pkozlowski-opensource Jan 31, 2019

jelbourn Jan 31, 2019

kara Feb 1, 2019 •

edited

jelbourn Feb 1, 2019

pkozlowski-opensource left a comment

jelbourn left a comment

jelbourn Feb 1, 2019

alfaproject Feb 2, 2019

pkozlowski-opensource Feb 4, 2019 •

edited

pkozlowski-opensource Feb 4, 2019

pkozlowski-opensource Feb 4, 2019 •

edited

googlebot commented Feb 4, 2019

pkozlowski-opensource left a comment

pkozlowski-opensource commented Feb 4, 2019

googlebot commented Feb 4, 2019

googlebot commented Feb 5, 2019

googlebot commented Feb 5, 2019

jelbourn commented Feb 5, 2019

googlebot commented Feb 5, 2019

googlebot commented Feb 5, 2019

matsko commented Feb 5, 2019

angular-automatic-lock-bot bot commented Sep 14, 2019

fix(ivy): remove DOM nodes from their real parent vs saved parent #28455

fix(ivy): remove DOM nodes from their real parent vs saved parent #28455

Conversation

jelbourn commented Jan 30, 2019

pkozlowski-opensource Jan 31, 2019

Choose a reason for hiding this comment

jelbourn Jan 31, 2019

Choose a reason for hiding this comment

kara Feb 1, 2019 • edited

Choose a reason for hiding this comment

jelbourn Feb 1, 2019

Choose a reason for hiding this comment

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

jelbourn Feb 1, 2019

Choose a reason for hiding this comment

alfaproject Feb 2, 2019

Choose a reason for hiding this comment

pkozlowski-opensource Feb 4, 2019 • edited

Choose a reason for hiding this comment

pkozlowski-opensource Feb 4, 2019

Choose a reason for hiding this comment

pkozlowski-opensource Feb 4, 2019 • edited

Choose a reason for hiding this comment

googlebot commented Feb 4, 2019

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

pkozlowski-opensource commented Feb 4, 2019

googlebot commented Feb 4, 2019

googlebot commented Feb 5, 2019

googlebot commented Feb 5, 2019

jelbourn commented Feb 5, 2019

googlebot commented Feb 5, 2019

googlebot commented Feb 5, 2019

matsko commented Feb 5, 2019

angular-automatic-lock-bot bot commented Sep 14, 2019

kara Feb 1, 2019 •

edited

pkozlowski-opensource Feb 4, 2019 •

edited

pkozlowski-opensource Feb 4, 2019 •

edited