Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: fix tests on older IE #3800

Merged
merged 15 commits into from
Dec 2, 2016
8 changes: 7 additions & 1 deletion src/js/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -1443,7 +1443,13 @@ class Component {
if (name === 'Player' && Component.components_[name]) {
const Player = Component.components_[name];

if (Player.players && Object.keys(Player.players).length > 0) {
// If we have players that were disposed, then their name will still be
// in Players.players. So, we must loop through and verify that the value
// for each item is not null. This allows registration of the Player component
// after all players have been disposed or before any were created.
if (Player.players &&
Object.keys(Player.players).length > 0 &&
Object.keys(Player.players).map((playerName) => Player.players[playerName]).every(Boolean)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure whether we want a comment here but basically, if all the items in Player.players are null, you're allowed to register a custom player. This is partly so we can reset the Player component properly for the test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it deserves some kind of comment. Basically, what you just wrote! 😆

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

throw new Error('Can not register Player component after player has been created');
}
}
Expand Down
27 changes: 21 additions & 6 deletions test/unit/player.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,8 @@ QUnit.test('should get current source from source tag', function(assert) {

assert.ok(player.currentSource().src === 'http://google.com');
assert.ok(player.currentSource().type === 'video/mp4');

player.dispose();
});

QUnit.test('should get current sources from source tag', function(assert) {
Expand Down Expand Up @@ -172,6 +174,8 @@ QUnit.test('should get current sources from source tag', function(assert) {
assert.ok(player.currentSources()[0].src === 'http://google.com');
assert.ok(player.currentSources()[0].type === undefined);
assert.ok(player.currentSources()[1] === undefined);

player.dispose();
});

QUnit.test('should get current source from src set', function(assert) {
Expand Down Expand Up @@ -208,6 +212,7 @@ QUnit.test('should get current source from src set', function(assert) {

assert.ok(player.currentSource().src === 'http://google.com');
assert.ok(player.currentSource().type === 'video/mp4');
player.dispose();
});

QUnit.test('should get current sources from src set', function(assert) {
Expand Down Expand Up @@ -255,6 +260,8 @@ QUnit.test('should get current sources from src set', function(assert) {
assert.ok(player.currentSources()[0].src === 'http://hugo.com');
assert.ok(player.currentSources()[0].type === undefined);
assert.ok(player.currentSources()[1] === undefined);

player.dispose();
});

QUnit.test('should asynchronously fire error events during source selection', function(assert) {
Expand Down Expand Up @@ -334,6 +341,8 @@ QUnit.test('should default to 16:9 when fluid', function(assert) {

// IE8 rounds 0.5625 up to 0.563
assert.ok(((ratio >= 0.562) && (ratio <= 0.563)), 'fluid player without dimensions defaults to 16:9');

player.dispose();
});

QUnit.test('should set fluid to true if element has vjs-fluid class', function(assert) {
Expand All @@ -344,6 +353,8 @@ QUnit.test('should set fluid to true if element has vjs-fluid class', function(a
const player = TestHelpers.makePlayer({}, tag);

assert.ok(player.fluid(), 'fluid is true with vjs-fluid class');

player.dispose();
});

QUnit.test('should use an class name that begins with an alpha character', function(assert) {
Expand Down Expand Up @@ -1269,19 +1280,23 @@ QUnit.test('should allow to register custom player when any player has not been

assert.equal(player instanceof CustomPlayer, true, 'player is custom');
player.dispose();

// reset the Player to the original value;
videojs.registerComponent('Player', Player);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the actual issue with custom player on IE8.

});

QUnit.test('should not allow to register custom player when any player has been created', function(assert) {
const tag = TestHelpers.makeTag();
const player = videojs(tag);

class CustomPlayer extends Player {}
try {

assert.throws(function() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert.throws is better.

videojs.registerComponent('Player', CustomPlayer);
} catch (e) {
player.dispose();
return assert.equal(e.message, 'Can not register Player component after player has been created');
}
}, 'Can not register Player component after player has been created');

player.dispose();

assert.ok(false, 'It should throw Error when any player has been created');
// reset the Player to the original value;
videojs.registerComponent('Player', Player);
});
32 changes: 16 additions & 16 deletions test/unit/test-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,22 +139,22 @@ const TestHelpers = {
* @param {string} eventType
*/
triggerDomEvent(element, eventType) {
let event;
if (document.createEvent) {
event = document.createEvent('HTMLEvents');
event.initEvent(eventType, true, true);
} else {
event = document.createEventObject();
event.eventType = eventType;
}
event.eventName = eventType;
if (document.createEvent) {
element.dispatchEvent(event);
} else {
element.fireEvent('on' + event.eventType, event);
let event;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change is for removing windows line-endings.

Copy link
Contributor

@brandonocasey brandonocasey Nov 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch! (windows newline removals)


if (document.createEvent) {
event = document.createEvent('HTMLEvents');
event.initEvent(eventType, true, true);
} else {
event = document.createEventObject();
event.eventType = eventType;
}

event.eventName = eventType;

if (document.createEvent) {
element.dispatchEvent(event);
} else {
element.fireEvent('on' + event.eventType, event);
}
}
};
Expand Down
13 changes: 11 additions & 2 deletions test/unit/tracks/text-track-controls.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,8 @@ test('chapters menu should use track label as menu title', function() {
player.controlBar.chaptersButton.update();

const menu = player.controlBar.chaptersButton.menu;
const menuTitle = menu.contentEl().firstChild.textContent;
const titleEl = menu.contentEl().firstChild;
const menuTitle = titleEl.textContent || titleEl.innerText;

equal(menuTitle, 'Test Chapters', 'menu gets track label as title');

Expand All @@ -435,7 +436,15 @@ test('chapters should be displayed when remote track added and load event fired'

equal(chaptersEl.track.cues.length, 2);

TestHelpers.triggerDomEvent(chaptersEl, 'load');
// Anywhere where we support using native text tracks, we can trigger a custom DOM event.
// On IE8 and other places where we have emulated tracks, either we cannot trigger custom
// DOM events (like IE8 with the custom DOM element) or we aren't using a DOM element at all.
// In those cases just trigger `load` directly on the chaptersEl object.
if (player.tech_.featuresNativeTextTracks) {
TestHelpers.triggerDomEvent(chaptersEl, 'load');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why/how are these two statements different?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On IE8 we use a custom DOM element for text tracks, however, IE8 doesn't allow us to trigger custom events on a custom element.
So, because everywhere we support native text tracks, we can trigger a real DOM event, we use that and where we use emulated tracks, we just trigger load directly on the chapters element object.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add a comment in the code for this, if you think it's necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should this check for IE8 instead? or maybe just add a comment to specify that this is a protection for IE8?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided that a general solution would be better than specifically checking for IE8. However, I'll add a comment about this.

} else {
chaptersEl.trigger('load');
}

ok(!player.controlBar.chaptersButton.hasClass('vjs-hidden'), 'chapters menu is displayed');

Expand Down