fix: remove child from old parent when moving to new parent via addChild

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;
 

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`');
+});

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');
+});