-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Changes from all commits
60b5db7
0e29845
668f6e8
41d9fc2
155c0a4
e828c61
e53c140
d257f42
2df1ee4
eb07f69
8ff5f28
fd56789
d6d95e1
75faaca
03c147d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
|
@@ -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) { | ||
|
@@ -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) { | ||
|
@@ -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) { | ||
|
@@ -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) { | ||
|
@@ -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) { | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this change is for removing windows line-endings. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
|
||
|
@@ -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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why/how are these two statements different? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'); | ||
|
||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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! 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done