From dd63cf94d51037bab641062a341b1cb69352d3b6 Mon Sep 17 00:00:00 2001
From: dustin71728 <kiwi71728@icloud.com>
Date: Wed, 9 Jan 2019 03:14:46 +0800
Subject: [PATCH] fix: remove child from old parent when moving to new parent
 via addChild (#5702)

A child component may have been assigned to another
parent before assigning that child component to the
new parent via "addChild" method. In this case, the
original parent should remove the child then it can
be safely added back to the new parent. This commit
will keep the parent's "children_" and its DOM
element's child nodes in the consistent state.
---
 src/js/component.js         | 11 +++++++++++
 test/unit/component.test.js | 22 ++++++++++++++++++++++
 test/unit/menu.test.js      | 31 +++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+)

diff --git a/src/js/component.js b/src/js/component.js
index ee7adffb1f..bbeaf04b53 100644
--- a/src/js/component.js
+++ b/src/js/component.js
@@ -58,6 +58,9 @@ class Component {
       this.player_ = player;
     }
 
+    // Hold the reference to the parent component via `addChild` method
+    this.parentComponent_ = null;
+
     // Make a copy of prototype.options_ to protect against overriding defaults
     this.options_ = mergeOptions({}, this.options_);
 
@@ -142,6 +145,8 @@ class Component {
     this.childIndex_ = null;
     this.childNameIndex_ = null;
 
+    this.parentComponent_ = null;
+
     if (this.el_) {
       // Remove element from DOM
       if (this.el_.parentNode) {
@@ -416,7 +421,11 @@ class Component {
       component = child;
     }
 
+    if (component.parentComponent_) {
+      component.parentComponent_.removeChild(component);
+    }
     this.children_.splice(index, 0, component);
+    component.parentComponent_ = this;
 
     if (typeof component.id === 'function') {
       this.childIndex_[component.id()] = component;
@@ -473,6 +482,8 @@ class Component {
       return;
     }
 
+    component.parentComponent_ = null;
+
     this.childIndex_[component.id()] = null;
     this.childNameIndex_[component.name()] = null;
 
diff --git a/test/unit/component.test.js b/test/unit/component.test.js
index f2549fa4a2..642d3dd407 100644
--- a/test/unit/component.test.js
+++ b/test/unit/component.test.js
@@ -1038,3 +1038,25 @@ QUnit.test('should use the stateful mixin', function(assert) {
 
   comp.dispose();
 });
+
+QUnit.test('should remove child when the child moves to the other parent', function(assert) {
+  const parentComponent1 = new Component(getFakePlayer(), {});
+  const parentComponent2 = new Component(getFakePlayer(), {});
+  const childComponent = new Component(getFakePlayer(), {});
+
+  parentComponent1.addChild(childComponent);
+
+  assert.strictEqual(parentComponent1.children().length, 1, 'the children number of `parentComponent1` is 1');
+  assert.strictEqual(parentComponent1.children()[0], childComponent, 'the first child of `parentComponent1` is `childComponent`');
+  assert.strictEqual(parentComponent1.el().childNodes[0], childComponent.el(), '`parentComponent1` contains the DOM element of `childComponent`');
+
+  parentComponent2.addChild(childComponent);
+
+  assert.strictEqual(parentComponent1.children().length, 0, 'the children number of `parentComponent1` is 0');
+  assert.strictEqual(parentComponent1.el().childNodes.length, 0, 'the length of `childNodes` of `parentComponent1` is 0');
+
+  assert.strictEqual(parentComponent2.children().length, 1, 'the children number of `parentComponent2` is 1');
+  assert.strictEqual(parentComponent2.children()[0], childComponent, 'the first child of `parentComponent2` is `childComponent`');
+  assert.strictEqual(parentComponent2.el().childNodes.length, 1, 'the length of `childNodes` of `parentComponent2` is 1');
+  assert.strictEqual(parentComponent2.el().childNodes[0], childComponent.el(), '`parentComponent2` contains the DOM element of `childComponent`');
+});
diff --git a/test/unit/menu.test.js b/test/unit/menu.test.js
index 611b5b368d..bc72f613d2 100644
--- a/test/unit/menu.test.js
+++ b/test/unit/menu.test.js
@@ -1,5 +1,6 @@
 /* eslint-env qunit */
 import MenuButton from '../../src/js/menu/menu-button.js';
+import MenuItem from '../../src/js/menu/menu-item.js';
 import TestHelpers from './test-helpers.js';
 import * as Events from '../../src/js/utils/events.js';
 
@@ -75,3 +76,33 @@ QUnit.test('clicking should display the menu', function(assert) {
   menuButton.dispose();
   player.dispose();
 });
+
+QUnit.test('should keep all the added menu items', function(assert) {
+  const player = TestHelpers.makePlayer();
+
+  const menuItems = [];
+  const menuItem1 = new MenuItem(player, { label: 'menu-item1' });
+  const menuItem2 = new MenuItem(player, { label: 'menu-item2' });
+
+  MenuButton.prototype.createItems = function() {
+    return menuItems;
+  };
+
+  const menuButton = new MenuButton(player, {});
+
+  menuItems.push(menuItem1);
+  menuButton.update();
+
+  assert.strictEqual(menuButton.children()[1].children().length, 1, 'the children number of the menu is 1 ');
+  assert.strictEqual(menuButton.children()[1].children()[0], menuItem1, 'the first child of the menu is `menuItem1`');
+  assert.ok(menuButton.el().contains(menuItem1.el()), 'the menu button contains the DOM element of `menuItem1`');
+
+  menuItems.push(menuItem2);
+  menuButton.update();
+
+  assert.strictEqual(menuButton.children()[1].children().length, 2, 'the children number of the menu is 2 after second update');
+  assert.strictEqual(menuButton.children()[1].children()[0], menuItem1, 'the first child of the menu is `menuItem1` after second update');
+  assert.strictEqual(menuButton.children()[1].children()[1], menuItem2, 'the second child of the menu is `menuItem2` after second update');
+  assert.ok(menuButton.el().contains(menuItem1.el()), 'the menu button contains the DOM element of `menuItem1` after second update');
+  assert.ok(menuButton.el().contains(menuItem2.el()), 'the menu button contains the DOM element of `menuItem2` after second update');
+});