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

Fix missing native HTML5 tracks #3212

Closed

Conversation

mister-ben
Copy link
Contributor

Description

Fixes missing text tracks when using native tracks in Chrome etc, fixes #2689
Tracks are currently added when the native text tracks addtrack event is triggered. Depending on how the player is initialised, this does not occur or fires before the event listener has been added.

Specific Changes proposed

  • Adds tracks when the HTML5 tech is initialised.
  • Modifies addTrack() to not add a track that already exists. This is to prevent duplication where addtrack is triggered.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

type: 'addtrack'
});
// Do not add duplicate tracks
if (!this.tracks_.includes(track)) {
Copy link
Member

Choose a reason for hiding this comment

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

Array#includes is ES6 only, we can't use it.

@gkatsev gkatsev added patch This PR can be added to a patch release. needs: LGTM Needs one or more additional approvals labels Mar 28, 2016
@gkatsev
Copy link
Member

gkatsev commented Mar 28, 2016

Also, could you rebase against stable? We could get this out as a patch update.

@mister-ben
Copy link
Contributor Author

I've removed includes(), but rebasing against stable will add other commits as before. Should I do that?

@gkatsev
Copy link
Member

gkatsev commented Mar 29, 2016

Don't worry about it then, when merging I'll see if I can get it merged correctly into stable, otherwise, we'll just merge it into master and have it be part of the next minor release.

@gkatsev gkatsev added confirmed and removed needs: LGTM Needs one or more additional approvals labels Mar 29, 2016
@gkatsev
Copy link
Member

gkatsev commented Mar 29, 2016

Ah, figured out how to do it locally. You'd have wanted to do an interactive rebase and then delete each of the commits that aren't part of this PR

$ git rebase -i stable

and only leave the last two commits (fe91dc8, and 5fb69e3). But I'm getting this merged locally along with a small tweak to make sure that forloop doesn't run if tt is null.

@gkatsev
Copy link
Member

gkatsev commented Mar 29, 2016

This is how it looks like (#3224), I created the PR so that I can run browserstack on it before merging.

@gkatsev gkatsev closed this in a22c7f2 Mar 29, 2016
@mister-ben mister-ben deleted the hotfix/html5tracks-chrome branch August 10, 2016 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chrome and Opera fail to load subtitles and captions
2 participants