Permalink
Browse files

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

…ild (#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.
  • Loading branch information...
dustin71728 authored and gkatsev committed Jan 8, 2019
1 parent 7f507df commit 8a3e2a72a9b2098547651afc64cc0a7825210b20
Showing with 64 additions and 0 deletions.
  1. +11 −0 src/js/component.js
  2. +22 −0 test/unit/component.test.js
  3. +31 −0 test/unit/menu.test.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;

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

0 comments on commit 8a3e2a7

Please sign in to comment.