fix: remove child from old parent when moving to new parent via addChild (videojs#5702)

liuruenshen · gkatsev · commit dd63cf94d510 · 2019-01-08T14:14:46.000-05:00
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.
diff --git 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
@@ -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
@@ -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');
+});
