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

Persist caption/description choice over source changes #4295

Merged
merged 10 commits into from
May 25, 2017
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ class TextTrackMenuItem extends MenuItem {

// Modify options for parent MenuItem class's init.
options.label = track.label || track.language || 'Unknown';
options.selected = track.default || track.mode === 'showing';
options.selected = (track.language === player.cache_.selectedLanguage) ||
track.default || track.mode === 'showing';

super(player, options);

Expand Down Expand Up @@ -102,6 +103,7 @@ class TextTrackMenuItem extends MenuItem {

if (track === this.track && (kinds.indexOf(track.kind) > -1)) {
track.mode = 'showing';
this.player_.cache_.selectedLanguage = track.language;
} else {
track.mode = 'disabled';
}
Expand Down
28 changes: 23 additions & 5 deletions src/js/tracks/text-track-display.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,24 @@ class TextTrackDisplay extends Component {
const trackList = this.player_.textTracks();
let firstDesc;
let firstCaptions;
let preferredDesc;
let preferredCaptions;

for (let i = 0; i < trackList.length; i++) {
const track = trackList[i];

// If we find a track matching the preferred language
// there is no need to find the first default track
if (player.cache_.selectedLanguage &&
player.cache_.selectedLanguage === track.language) {
if (track.kind === 'descriptions' && !preferredDesc) {
preferredDesc = track;
} else if (track.kind in modes && !preferredCaptions) {
preferredCaptions = track;
}
break;
}

if (track.default) {
Copy link
Member

Choose a reason for hiding this comment

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

should this be an else if for the above if?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will update

if (track.kind === 'descriptions' && !firstDesc) {
firstDesc = track;
Expand All @@ -128,11 +142,15 @@ class TextTrackDisplay extends Component {
}
}

// We want to show the first default track but captions and subtitles
// take precedence over descriptions.
// So, display the first default captions or subtitles track
// and otherwise the first default descriptions track.
if (firstCaptions) {
// The preferred tracks take precedence over the first default track,
// captions and subtitles take precedence over descriptions.
// So, display the preferred track before the first default track
// and the subtitles or captions track before the descriptions track
if (preferredCaptions) {
preferredCaptions.mode = 'showing';
} else if (preferredDesc) {
preferredDesc.mode = 'showing';
} else if (firstCaptions) {
firstCaptions.mode = 'showing';
} else if (firstDesc) {
firstDesc.mode = 'showing';
Expand Down
279 changes: 279 additions & 0 deletions test/unit/tracks/text-track-display.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,279 @@
/* eslint-env qunit */
import Html5 from '../../../src/js/tech/html5.js';
import Component from '../../../src/js/component.js';

import * as browser from '../../../src/js/utils/browser.js';
import TestHelpers from '../test-helpers.js';
import document from 'global/document';
import sinon from 'sinon';

QUnit.module('Text Track Display', {
beforeEach(assert) {
this.clock = sinon.useFakeTimers();
},
afterEach(assert) {
this.clock.restore();
}
});

const getTrackByLanguage = function(player, language) {
const tracks = player.tech_.remoteTextTracks();

for (let i = 0; i < tracks.length; i++) {
const track = tracks[i];

if (track.language === language) {
return track;
}
}
};

const getMenuItemByLanguage = function(items, language) {
for (let i = items.length - 1; i > 0; i--) {
const captionMenuItem = items[i];
const trackLanguage = captionMenuItem.track.language;

if (trackLanguage && trackLanguage === language) {
return captionMenuItem;
}
}
};

QUnit.test('if native text tracks are not supported, create a texttrackdisplay', function(assert) {
Copy link
Member

Choose a reason for hiding this comment

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

for reference, this test was moved from text-tracks.test.js

const oldTestVid = Html5.TEST_VID;
const oldIsFirefox = browser.IS_FIREFOX;
const oldTextTrackDisplay = Component.getComponent('TextTrackDisplay');
const tag = document.createElement('video');
const track1 = document.createElement('track');
const track2 = document.createElement('track');

track1.kind = 'captions';
track1.label = 'en';
track1.language = 'English';
track1.src = 'en.vtt';
tag.appendChild(track1);

track2.kind = 'captions';
track2.label = 'es';
track2.language = 'Spanish';
track2.src = 'es.vtt';
tag.appendChild(track2);

Html5.TEST_VID = {
textTracks: []
};

browser.IS_FIREFOX = true;

const fakeTTDSpy = sinon.spy();

class FakeTTD extends Component {
constructor() {
super();
fakeTTDSpy();
}
}

Component.registerComponent('TextTrackDisplay', FakeTTD);

const player = TestHelpers.makePlayer({}, tag);

assert.strictEqual(fakeTTDSpy.callCount, 1, 'text track display was created');

Html5.TEST_VID = oldTestVid;
browser.IS_FIREFOX = oldIsFirefox;
Component.registerComponent('TextTrackDisplay', oldTextTrackDisplay);

player.dispose();
});

QUnit.test('no captions tracks, no captions are displayed', function(assert) {
Copy link
Member

Choose a reason for hiding this comment

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

looks like this is a SubsCapsButton test rather than a TextTrackDisplay test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right! Turns out there's a test like this in text-track-controls.test.js already which I will expand to check if remoteTextTracks is empty as well.

Copy link
Member

Choose a reason for hiding this comment

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

remoteTextTracks should never have stuff in it if textTracks is already empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh nice, will just remove this one then.

// The video has no captions
const player = TestHelpers.makePlayer();
const tracks = player.tech_.remoteTextTracks();
const captionsButton = player.controlBar.getChild('SubsCapsButton');

player.src({type: 'video/mp4', src: 'http://google.com'});
this.clock.tick(1);

// the captions track and button are not shown
assert.ok(tracks.length === 0, 'No captions tracks');
assert.ok(captionsButton.hasClass('vjs-hidden'), 'The captions button should not be shown');
player.dispose();
});

QUnit.test('shows the default caption track first', function(assert) {
const player = TestHelpers.makePlayer();
const track1 = {
kind: 'captions',
label: 'English',
language: 'en',
src: 'en.vtt',
default: true
};
const track2 = {
kind: 'captions',
label: 'Spanish',
language: 'es',
src: 'es.vtt'
};

// Add the text tracks
player.addRemoteTextTrack(track1);
player.addRemoteTextTrack(track2);

// Make sure the ready handler runs
this.clock.tick(1);

const englishTrack = getTrackByLanguage(player, 'en');
const spanishTrack = getTrackByLanguage(player, 'es');

assert.ok(englishTrack.mode === 'showing', 'English track should be showing');
assert.ok(spanishTrack.mode === 'disabled', 'Spanish track should not be showing');
player.dispose();
});

if (!Html5.supportsNativeTextTracks()) {
QUnit.test('if user-selected language is unavailable, don\'t pick a track to show', function(assert) {
Copy link
Member

Choose a reason for hiding this comment

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

I believe our eslint rules allow us to use double-quotes in this situation to avoid escaping the quote.

// The video has no default language but has ‘English’ captions only
const oldIsFirefox = browser.IS_FIREFOX;
const player = TestHelpers.makePlayer();
const track1 = {
kind: 'captions',
label: 'English',
language: 'en',
src: 'en.vtt'
};
const captionsButton = player.controlBar.getChild('SubsCapsButton');

browser.IS_FIREFOX = true;
player.src({type: 'video/mp4', src: 'http://google.com'});
// manualCleanUp = true by default
player.addRemoteTextTrack(track1);
// Force 'es' as user-selected track
player.cache_.selectedLanguage = 'es';

this.clock.tick(1);
player.play();

const englishTrack = getTrackByLanguage(player, 'en');

assert.ok(!captionsButton.hasClass('vjs-hidden'), 'The captions button is shown');
assert.ok(englishTrack.mode === 'disabled', 'English track should be disabled');

browser.IS_FIREFOX = oldIsFirefox;
player.dispose();
});

QUnit.test('the user-selected language takes priority over default language', function(assert) {
// The video has ‘English’ captions as default, but has ‘Spanish’ captions also
const oldIsFirefox = browser.IS_FIREFOX;
const player = TestHelpers.makePlayer({techOrder: ['html5']});
const track1 = {
kind: 'captions',
label: 'English',
language: 'en',
src: 'en.vtt',
default: true
};
const track2 = {
kind: 'captions',
label: 'Spanish',
language: 'es',
src: 'es.vtt'
};

browser.IS_FIREFOX = true;
Copy link
Member

Choose a reason for hiding this comment

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

why force firefox?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I don't actually need this if I check supportsNativeTextTracks

player.src({type: 'video/mp4', src: 'http://google.com'});
// manualCleanUp = true by default
player.addRemoteTextTrack(track1);
player.addRemoteTextTrack(track2);
// Force 'es' as user-selected track
player.cache_.selectedLanguage = 'es';
this.clock.tick(1);

// the spanish captions track should be shown
const englishTrack = getTrackByLanguage(player, 'en');
Copy link
Member

Choose a reason for hiding this comment

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

player.addRemoteTextTrack returns an HTMLTrackElement (or a shim) that has the track in a .track property. That might be another way of getting it rather than getTrackByLanguage, if that's the only places that getTrackByLanguage is used.

const spanishTrack = getTrackByLanguage(player, 'es');

assert.ok(spanishTrack.mode === 'showing', 'Spanish captions should be shown');
assert.ok(englishTrack.mode === 'disabled', 'English captions should be hidden');

browser.IS_FIREFOX = oldIsFirefox;
player.dispose();
});

QUnit.test('the user-selected language is used for subsequent source changes', function(assert) {
// Start with two captions tracks: English and Spanish
const oldIsFirefox = browser.IS_FIREFOX;
const player = TestHelpers.makePlayer();
const track1 = {
kind: 'captions',
label: 'English',
language: 'en',
src: 'en.vtt'
};
const track2 = {
kind: 'captions',
label: 'Spanish',
language: 'es',
src: 'es.vtt'
};
const tracks = player.tech_.remoteTextTracks();
const captionsButton = player.controlBar.getChild('SubsCapsButton');
let esCaptionMenuItem;
let enCaptionMenuItem;

browser.IS_FIREFOX = true;
player.src({type: 'video/mp4', src: 'http://google.com'});
// manualCleanUp = true by default
player.addRemoteTextTrack(track1);
player.addRemoteTextTrack(track2);

// Keep track of menu items
esCaptionMenuItem = getMenuItemByLanguage(captionsButton.items, 'es');
enCaptionMenuItem = getMenuItemByLanguage(captionsButton.items, 'en');

// The user chooses Spanish
player.play();
captionsButton.pressButton();
Copy link
Member

Choose a reason for hiding this comment

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

probably no need to press the captions button.

esCaptionMenuItem.trigger('click');

// Track mode changes on user-selection
assert.ok(esCaptionMenuItem.track.mode === 'showing',
'Spanish should be showing after selection');
assert.ok(enCaptionMenuItem.track.mode === 'disabled',
'English should be disabled after selecting Spanish');

// Switch source and remove old tracks
player.tech_.src({type: 'video/mp4', src: 'http://example.com'});
while (tracks.length > 0) {
player.removeRemoteTextTrack(tracks[0]);
}
// Add tracks for the new source
player.addRemoteTextTrack(track1);
player.addRemoteTextTrack(track2);

// Make sure player ready handler runs
this.clock.tick(1);

// Keep track of menu items
esCaptionMenuItem = getMenuItemByLanguage(captionsButton.items, 'es');
enCaptionMenuItem = getMenuItemByLanguage(captionsButton.items, 'en');

// The user-selection should have persisted
assert.ok(esCaptionMenuItem.track.mode === 'showing',
'Spanish should remain showing');
assert.ok(enCaptionMenuItem.track.mode === 'disabled',
'English should remain disabled');

const englishTrack = getTrackByLanguage(player, 'en');
const spanishTrack = getTrackByLanguage(player, 'es');

assert.ok(spanishTrack.mode === 'showing', 'Spanish track remains showing');
assert.ok(englishTrack.mode === 'disabled', 'English track remains disabled');

browser.IS_FIREFOX = oldIsFirefox;
player.dispose();
});
}