Persist caption/description choice over source changes #4295

ldayananda · 2017-04-19T19:25:11Z

Description

This should allow a user's choice of subtitle or description to be respected across an entire session(if available).

Specific Changes proposed

Remember the user selection
User selected choice is preferred over the first default track

Requirements Checklist

Feature implemented / Bug fixed
If necessary, more likely in a feature request than a bug fix
- Change has been verified in an actual browser (Chome, Firefox, IE)
- Unit Tests updated or fixed
- Docs/guides updated (n/a)
Reviewed by Two Core Contributors

gkatsev

Love the tests.
We have method called reset that tries to bring the player to as close to an un-initialized state as possible, should it also unset the selectedLanguage?

gkatsev · 2017-04-19T20:55:43Z

test/unit/tracks/text-track-display.test.js

+  player.dispose();
+});
+
+QUnit.test('no captions tracks, no captions are displayed', function(assert) {


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

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.

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

oh nice, will just remove this one then.

gkatsev · 2017-04-19T20:56:40Z

test/unit/tracks/text-track-display.test.js

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


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

gkatsev · 2017-04-19T20:57:35Z

test/unit/tracks/text-track-display.test.js

+  }
+};
+
+QUnit.test('if native text tracks are not supported, create a texttrackdisplay', function(assert) {


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

gkatsev · 2017-04-19T20:59:37Z

test/unit/tracks/text-track-display.test.js

+      src: 'es.vtt'
+    };
+
+    browser.IS_FIREFOX = true;


why force firefox?

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

gkatsev · 2017-04-19T21:01:45Z

test/unit/tracks/text-track-display.test.js

+    this.clock.tick(1);
+
+    // the spanish captions track should be shown
+    const englishTrack = getTrackByLanguage(player, 'en');


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.

gkatsev · 2017-04-19T21:05:47Z

src/js/tracks/text-track-display.js

+          }
+          break;
+        }
+
        if (track.default) {


should this be an else if for the above if?

Yes, will update

gkatsev · 2017-04-19T21:10:11Z

test/unit/tracks/text-track-display.test.js

+
+    // The user chooses Spanish
+    player.play();
+    captionsButton.pressButton();


probably no need to press the captions button.

gkatsev · 2017-04-19T21:14:34Z

Also, it's great that the non-code changes are pretty small and fairly localized!

ldayananda · 2017-04-20T18:03:38Z

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.

…selected

gkatsev · 2017-04-28T15:02:57Z

src/js/control-bar/text-track-controls/text-track-button.js

        item.addClass(`vjs-${track.kind}-menu-item`);
        items.push(item);
      }
    }

+    if (preferredItem) {


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.

Yes and yes. Back to the drawing board :)

ldayananda · 2017-04-28T22:37:23Z

Updated once again, this time with more encapsulation

gkatsev

This looks good and I think it's good enough to go to QA.

Though, I'd like to take play around with it a bit myself as well.

gkatsev · 2017-05-08T22:18:17Z

src/js/tracks/text-track-display.js

@@ -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));


did we decide on whether we wanted this to happen on addtrack as well?

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.

gkatsev · 2017-05-08T22:20:43Z

src/js/tracks/text-track-list.js

+
+    if (nonLanguageTextTrackKind.indexOf(track.kind) === -1) {
+      track.addEventListener('modechange', Fn.bind(this, function() {
+        this.trigger('selectedlanguagechange');


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.

maybe displayedtexttrackchange would be a better name?

apadhye · 2017-05-18T15:48:14Z

QA LGTM

ldayananda added 2 commits April 19, 2017 11:59

Initial unit tests and work to persist a user choice

35e4958

Handling Safari in the tests

03a3b70

gkatsev reviewed Apr 19, 2017

View reviewed changes

Updates from code review comments

ecf6b6f

ldayananda added 4 commits April 27, 2017 12:00

Changes to make sure track mode is changed when preselecting a language

1001162

Make sure to keep the originally chosen track's kind on mode changes

54506ea

Updating tests and making sure only the most preferred text track is …

27acd7e

…selected

selectedlanguagechange on text track list rather than tech

692783e

misteroneill approved these changes Apr 28, 2017

View reviewed changes

gkatsev reviewed Apr 28, 2017

View reviewed changes

ldayananda added 2 commits April 28, 2017 18:27

Only preselect if there's a loadstart event in the text track display

485fe93

Updated tests

50f346e

gkatsev added this to Ready to Review in video.js May 8, 2017

gkatsev reviewed May 8, 2017

View reviewed changes

gkatsev moved this from Ready to Review to Ready for testing in video.js May 8, 2017

Always clear preselection if user turns off captions

2eedf77

gkatsev approved these changes May 25, 2017

View reviewed changes

gkatsev merged commit 188ead1 into videojs:master May 25, 2017

ldayananda deleted the persist-caption branch August 23, 2017 02:28

gkatsev moved this from Ready for testing to Done in video.js Jun 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persist caption/description choice over source changes #4295

Persist caption/description choice over source changes #4295

ldayananda commented Apr 19, 2017 •

edited

gkatsev left a comment

gkatsev Apr 19, 2017

ldayananda Apr 20, 2017

gkatsev Apr 20, 2017

ldayananda Apr 20, 2017

gkatsev Apr 19, 2017

gkatsev Apr 19, 2017

gkatsev Apr 19, 2017

ldayananda Apr 20, 2017

gkatsev Apr 19, 2017

gkatsev Apr 19, 2017

ldayananda Apr 20, 2017

gkatsev Apr 19, 2017

gkatsev commented Apr 19, 2017

ldayananda commented Apr 20, 2017

gkatsev Apr 28, 2017

ldayananda Apr 28, 2017

ldayananda commented Apr 28, 2017

gkatsev left a comment

gkatsev May 8, 2017

ldayananda May 9, 2017

gkatsev May 8, 2017

ldayananda May 9, 2017

apadhye commented May 18, 2017

Persist caption/description choice over source changes #4295

Persist caption/description choice over source changes #4295

Conversation

ldayananda commented Apr 19, 2017 • edited

Description

Specific Changes proposed

Requirements Checklist

gkatsev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented Apr 19, 2017

ldayananda commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldayananda commented Apr 28, 2017

gkatsev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apadhye commented May 18, 2017

ldayananda commented Apr 19, 2017 •

edited