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

Check for text track changes that occurred before tech was listening #2835

Closed
wants to merge 1 commit into from

Conversation

gesinger
Copy link
Contributor

If any code sets a track mode during player setup (e.g., track.mode = 'showing') for non native text tracks, there is a race case where the player may not be listening for changes on the track. For instance:

var player = videojs('my-video', {
    html5: {
        nativeTextTracks: false
    }
});

var track = player.addTextTrack('captions');
track.mode = 'showing';

The caption settings will show the captions as on for the track, but the captions will not show up on the video unless a user switches to fullscreen or changes the caption settings (both will update display appropriately).

This change ensures that before videojs starts listening to change events on tracks, it gets any changes that may have happened between then and the start of setup.

@pam
Copy link

pam commented Nov 21, 2015

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 8a3a97ecc936e34ca052570e4efb14ea1d8e1e45
Build details: https://travis-ci.org/pam/video.js/builds/92385457

(Please note that this is a fully automated comment.)

'we didn\'t have an extra text tracks update for non native text tracks');

Player.prototype.textTracks = origTextTracks;
Tech.prototype['featuresNativeTextTracks'] = false;
Copy link
Member

Choose a reason for hiding this comment

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

What if the original value was true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Will store the original value and reset it here.

@pam
Copy link

pam commented Nov 21, 2015

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: dc174d890bef253d042e086aa8add0986afac9d9
Build details: https://travis-ci.org/pam/video.js/builds/92431612

(Please note that this is a fully automated comment.)

@gkatsev
Copy link
Member

gkatsev commented Nov 21, 2015

LGTM

@gkatsev gkatsev added patch This PR can be added to a patch release. needs: LGTM Needs one or more additional approvals labels Nov 21, 2015
@@ -322,6 +322,7 @@ class Tech extends Component {
}

let textTracksChanges = Fn.bind(this, function() {
let tracks = this.textTracks();
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? Seems like this function could not run unless tracks was already defined at the assignment in emulateTextTracks above.

Copy link
Member

Choose a reason for hiding this comment

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

It isn't strictly necessary but it makes testing this significantly easier.

@pam
Copy link

pam commented Nov 23, 2015

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: b02fdef
Build details: https://travis-ci.org/pam/video.js/builds/92834268

(Please note that this is a fully automated comment.)

@gkatsev
Copy link
Member

gkatsev commented Nov 24, 2015

Sweet, that test looks a lot nicer now too.

@gkatsev
Copy link
Member

gkatsev commented Nov 25, 2015

LGTM

@mmcc
Copy link
Member

mmcc commented Nov 25, 2015

Lgtm!

@gkatsev gkatsev closed this in 7171ea8 Nov 25, 2015
@gkatsev gkatsev mentioned this pull request Nov 25, 2015
@gkatsev gkatsev removed the needs: LGTM Needs one or more additional approvals label Dec 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants