Persist caption/description choice over source changes #4295
Conversation
| player.dispose(); | ||
| }); | ||
|
|
||
| QUnit.test('no captions tracks, no captions are displayed', function(assert) { |
There was a problem hiding this comment.
looks like this is a SubsCapsButton test rather than a TextTrackDisplay test.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
remoteTextTracks should never have stuff in it if textTracks is already empty.
There was a problem hiding this comment.
oh nice, will just remove this one then.
| }); | ||
|
|
||
| if (!Html5.supportsNativeTextTracks()) { | ||
| QUnit.test('if user-selected language is unavailable, don\'t pick a track to show', function(assert) { |
There was a problem hiding this comment.
I believe our eslint rules allow us to use double-quotes in this situation to avoid escaping the quote.
| } | ||
| }; | ||
|
|
||
| QUnit.test('if native text tracks are not supported, create a texttrackdisplay', function(assert) { |
There was a problem hiding this comment.
for reference, this test was moved from text-tracks.test.js
| src: 'es.vtt' | ||
| }; | ||
|
|
||
| browser.IS_FIREFOX = true; |
There was a problem hiding this comment.
You're right, I don't actually need this if I check supportsNativeTextTracks
| this.clock.tick(1); | ||
|
|
||
| // the spanish captions track should be shown | ||
| const englishTrack = getTrackByLanguage(player, 'en'); |
There was a problem hiding this comment.
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.
src/js/tracks/text-track-display.js
Outdated
| } | ||
| break; | ||
| } | ||
|
|
||
| if (track.default) { |
There was a problem hiding this comment.
should this be an else if for the above if?
There was a problem hiding this comment.
Yes, will update
|
|
||
| // The user chooses Spanish | ||
| player.play(); | ||
| captionsButton.pressButton(); |
There was a problem hiding this comment.
probably no need to press the captions button.
|
Also, it's great that the non-code changes are pretty small and fairly localized! |
|
Updated code and tests according to the feedback above. We decided that the reset method should continue to behave as it does currently and selectedLanguage will remain set if reset is called. |
| item.addClass(`vjs-${track.kind}-menu-item`); | ||
| items.push(item); | ||
| } | ||
| } | ||
|
|
||
| if (preferredItem) { |
There was a problem hiding this comment.
couldn't this have the side-effect of switching the selected track when a new track is added? Also, this seems like it'll try and enable a track per text-track-button.
There was a problem hiding this comment.
Yes and yes. Back to the drawing board :)
|
Updated once again, this time with more encapsulation |
| @@ -92,6 +92,7 @@ class TextTrackDisplay extends Component { | |||
|
|
|||
| player.on('loadstart', Fn.bind(this, this.toggleDisplay)); | |||
| player.on('texttrackchange', Fn.bind(this, this.updateDisplay)); | |||
| player.on('loadstart', Fn.bind(this, this.preselectTrack)); | |||
There was a problem hiding this comment.
did we decide on whether we wanted this to happen on addtrack as well?
There was a problem hiding this comment.
I believe that we discussed it and decided that it would be disorienting to preselect a new track if a track had already been selected by the user.
|
|
||
| if (nonLanguageTextTrackKind.indexOf(track.kind) === -1) { | ||
| track.addEventListener('modechange', Fn.bind(this, function() { | ||
| this.trigger('selectedlanguagechange'); |
There was a problem hiding this comment.
just realized it's possible that the language doesn't change, so, the naming is a big weird, but maybe still ok.
For example, if you go from english captions to english subtitles, the language is the same but only the kind changed.
There was a problem hiding this comment.
maybe displayedtexttrackchange would be a better name?
|
QA LGTM |
Description
This should allow a user's choice of subtitle or description to be respected across an entire session(if available).
Specific Changes proposed
Requirements Checklist