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.

Sign up for GitHub

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

feat(texttracks): always use emulated text tracks #3798

Merged
merged 3 commits into from
Dec 2, 2016
Merged

feat(texttracks): always use emulated text tracks #3798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a531db
feat(texttracks): always use emulated text tracks
gkatsev Nov 21, 2016
312cdd4
fix tests for text track support
gkatsev Nov 22, 2016
a2ec559
don't negate a true statement
gkatsev Nov 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 1 addition & 19 deletions src/js/tech/html5.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -767,25 +767,7 @@ Html5.canControlPlaybackRate = function() {
* @return {Boolean}
*/
Html5.supportsNativeTextTracks = function() {
let supportsTextTracks;

// Figure out native text track support
// If mode is a number, we cannot change it because it'll disappear from view.
// Browsers with numeric modes include IE10 and older (<=2013) samsung android models.
// Firefox isn't playing nice either with modifying the mode
// TODO: Investigate firefox: https://github.com/videojs/video.js/issues/1862
supportsTextTracks = !!Html5.TEST_VID.textTracks;
if (supportsTextTracks && Html5.TEST_VID.textTracks.length > 0) {
supportsTextTracks = typeof Html5.TEST_VID.textTracks[0].mode !== 'number';
}
if (supportsTextTracks && browser.IS_FIREFOX) {
supportsTextTracks = false;
}
if (supportsTextTracks && !('onremovetrack' in Html5.TEST_VID.textTracks)) {
supportsTextTracks = false;
}

return supportsTextTracks;
return browser.IS_ANY_SAFARI;
Copy link
Contributor

Choose a reason for hiding this comment

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

wasn't the issue only with chrome? should we still support native on firefox?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't currently support native tracks on Firefox anyway. Firefox 50 may be better but older versions definitely had issues.
Also, I think it would be better to be consistent as much as we can.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok maybe we should just have an issue/TODO comment to revisit this at another time?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, now is the time. Either we should strive to always try and use native tracks or we should try and always use emulated tracks. Safari is a bit of a special case, particularly because of iOS and HLS that we allow native tracks for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am all for always using emulated tracks, because we can control all of the behavior

Copy link
Member

Choose a reason for hiding this comment

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

I agree with a move to doing emulated tracks wherever possible; I've been meaning to file an issue because, in order to support Descriptions tracks, you have to use emulated tracks even if the browser claims it supports text tracks, and having to switch to emulated tracks just because you want to add support for Descriptions tracks isn't going to be popular, if sites have become used to native track styles and controls. Emulated tracks allow the kind of cross-browser consistency, similar to control bar consistency, that is one of video.js' greatest strengths.

};

/**
Expand Down
3 changes: 3 additions & 0 deletions src/js/utils/browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,5 +66,8 @@ export const IE_VERSION = (function(result) {
return result && parseFloat(result[1]);
}((/MSIE\s(\d+)\.\d/).exec(USER_AGENT)));

export const IS_SAFARI = (/Safari/i).test(USER_AGENT) && !IS_CHROME && !IS_ANDROID && !IS_EDGE;
export const IS_ANY_SAFARI = IS_SAFARI || IS_IOS;

export const TOUCH_ENABLED = !!(('ontouchstart' in window) || window.DocumentTouch && document instanceof window.DocumentTouch);
export const BACKGROUND_SIZE_SUPPORTED = 'backgroundSize' in document.createElement('video').style;
29 changes: 8 additions & 21 deletions test/unit/tracks/text-tracks.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,37 +250,24 @@ QUnit.test('if native text tracks are not supported, create a texttrackdisplay',
player.dispose();
});

QUnit.test('html5 tech supports native text tracks if the video supports it, unless mode is a number', function(assert) {
QUnit.test('emulated tracks are always used, except in safari', function(assert) {
const oldTestVid = Html5.TEST_VID;
const oldIsAnySafari = browser.IS_ANY_SAFARI;

Html5.TEST_VID = {
textTracks: [{
mode: 0
}]
textTracks: []
};

assert.ok(!Html5.supportsNativeTextTracks(),
'native text tracks are not supported if mode is a number');

Html5.TEST_VID = oldTestVid;
});

QUnit.test('html5 tech supports native text tracks if the video supports it, unless it is firefox', function(assert) {
const oldTestVid = Html5.TEST_VID;
const oldIsFirefox = browser.IS_FIREFOX;
browser.IS_ANY_SAFARI = false;

Html5.TEST_VID = {
textTracks: []
};
assert.ok(!Html5.supportsNativeTextTracks(), 'Html5 does not support native text tracks, in non-safari');

browser.IS_FIREFOX = true;
browser.IS_ANY_SAFARI = true;

assert.ok(!Html5.supportsNativeTextTracks(),
'if textTracks are available on video element,' +
' native text tracks are supported');
assert.ok(Html5.supportsNativeTextTracks(), 'Html5 does support native text tracks in safari');

Html5.TEST_VID = oldTestVid;
browser.IS_FIREFOX = oldIsFirefox;
browser.IS_ANY_SAFARI = oldIsAnySafari;
});

QUnit.test('when switching techs, we should not get a new text track', function(assert) {
Expand Down