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

the first default text tracks will be show by default in emulated tracks #3248

Closed
wants to merge 6 commits into from

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Apr 11, 2016

Description

As the title suggests, default emulated text tracks will now be shown by default. It will choose the first one from the list.

Specific Changes proposed

  • Native tracks will still be have the default attribute handled by the browser
  • TextTrack objects now have a default property that represents whether they were created with it originally
  • the first default emulated text track will be chosen to be shown by default
  • Fixes Subtitles default does not work #1914

Requirements Checklist

  • Feature implemented / Bug fixed
  • Reviewed by Two Core Contributors

@gkatsev gkatsev added this to the Text Tracks milestone Apr 11, 2016
@dmlap
Copy link
Member

dmlap commented Apr 11, 2016

This turns on captions/subtitles/descriptions if any of them are marked default? How does that line up with native browser behavior?

@gkatsev
Copy link
Member Author

gkatsev commented Apr 11, 2016

@dmlap that's what chrome does. Safari does it's own thing where it remembers your last position. Really hard to test this. Looks like firefox only does it for english (so, probably if the srclang matches your browser's locale).

@gkatsev
Copy link
Member Author

gkatsev commented Apr 11, 2016

Firefox also doesn't even have a captions button to toggle visibility of the current track on/off like chrome does.

@gkatsev
Copy link
Member Author

gkatsev commented Apr 11, 2016

Also, I think most users expect it to just show the first default text track, regardless of the locale.

@@ -58,6 +58,19 @@ class TextTrackDisplay extends Component {
let track = tracks[i];
this.player_.addRemoteTextTrack(track);
}

let modesToShow = {'captions': 1, 'subtitles': 1, 'descriptions': 1};
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you use an object instead of an array?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because in operator is O(1) vs indexOf is O(n).

@nickygerritsen
Copy link
Contributor

Ah oops @dmlap asked a similar question already, my bad

@gkatsev
Copy link
Member Author

gkatsev commented Apr 12, 2016

@nickygerritsen heh, it's ok. It made me realize that I need to default descriptions after defaulting captions/subtitles.

@gkatsev
Copy link
Member Author

gkatsev commented Apr 12, 2016

Here's an example: http://plnkr.co/edit/rncA4P?p=info

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

gkatsev commented Apr 12, 2016

I'll add a comment about defaulting to the first track unless there's both a caption/subtitles and a description, in which case captions/subtitles take priority.
I'll also see if I can move this block of code into the Tech constructor and add some tests.

@gkatsev
Copy link
Member Author

gkatsev commented Apr 12, 2016

Ok, updated with tests and also a comment about what's happening.

@nickygerritsen
Copy link
Contributor

Nice, LGTM

@gkatsev gkatsev added confirmed and removed needs: LGTM Needs one or more additional approvals labels Apr 13, 2016
@gkatsev gkatsev closed this in ff83aa6 Apr 19, 2016
@gkatsev gkatsev deleted the default-tracks branch April 19, 2016 21:21
jgubman added a commit to jgubman/video.js that referenced this pull request Apr 26, 2016
* upstream/stable: (77 commits)
  v5.9.2
  @gkatsev grouped text track errors in the console, if we can. closes videojs#3259
  v5.9.1
  @gkatsev fixed text track tests for older IEs. closes videojs#3269
  revert 75116d4 adding chrome to travis (videojs#3254)
  @forbesjo added back the background color to the poster. closes videojs#3267
  @gkatsev fixed removeRemoteTextTracks not working with return value from addRemoteTextTracks. closes videojs#3253
  @gkatsev made the first emulated text track enabled by default. closes videojs#3248
  @mister-ben blacklisted Chrome for Android for playback rate support. closes videojs#3246
  @benjipott updated IS_CHROME to not be true on MS Edge. closes videojs#3232
  v5.9.0
  @andyearnshaw updated document event handlers to use el.ownerDocument. closes videojs#3230
  @chrisauclair added ARIA region and label to player element. closes videojs#3227
  @MCGallaspy added vttjs to the self-hosting guide. closes videojs#3229
  @forbesjo added chrome for PR tests. closes videojs#3235
  @OwenEdwards improved handling of deprecated use of Button component. closes videojs#3236
  v5.8.8
  @seescode fixed dragging on mute toggle changing the volume. Fixes videojs#3036. Closes videojs#3228
  @seescode fixed css failing on IE8 due to incorrect ie8 hack. Fixes videojs#3140. Closes videojs#3226.
  @vtytar fixed auto-setup failing if taking too long to load. Fixes videojs#2386. Closes videojs#3233.
  ...
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.

Subtitles default does not work
3 participants