Combined tracks control #4028

mister-ben · 2017-02-02T21:12:33Z

Description

Puts captions and subtitles in a single control. Tidier UI and less confusing for those who don't generally distinguish between those types. Captions have '[CC]' appended - CSS currently, but this should be localisable.

For now, there's an example in /sandbox.

Specific Changes proposed

Added a 'TrackButton'
Modified the track controls to have a kinds array internally so they can iterate through more than one kind when toggling other tracks' state.
Doesn't break the exisiting single-kind controls (in fact fixes an issue that the subtitles menu doesn't update if a subtitle track was showing when captions are enabled)

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
- Example created (starter template on JSBin)
Reviewed by Two Core Contributors

OwenEdwards · 2017-02-06T16:19:38Z

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

+   */
+  constructor(player, options, ready) {
+    super(player, options, ready);
+    this.el_.setAttribute('aria-label', 'Captions Menu');


This aria-label assignment should be removed after #4034 lands.

That PR was landed. This PR probably needs to be rebased against master.

OwenEdwards · 2017-02-06T16:22:01Z

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

+ * @type {string}
+ * @private
+ */
+TracksButton.prototype.controlText_ = 'Captions';


This text will need to change depending on North America/Rest Of the World, as with the icon for the control.

And a little more complex to localise if the string needs to be localised as captions or subtitles here, but specifically as captions on the captions-only control, which might be translated as "subtitles for the hard of hearing".

This would be easier if we used something like CAPTIONS_OR_SUBTITLES as keys in the translations json and included en.json in the player.

OwenEdwards · 2017-02-06T16:23:18Z

src/js/control-bar/control-bar.js

@@ -67,6 +68,7 @@ ControlBar.prototype.options_ = {
    'descriptionsButton',
    'subtitlesButton',
    'captionsButton',
+    'tracksButton',


I assume that the default Control Bar won't have subtitlesButton, captionsButton and tracksButton - it'll be either/or?

Left in for testing to make sure they don't break each other, but I don't see a reason for them to be used like that for real.

What about removing from here and updating the combined-tracks test page to include all the separate caption buttons in the control bar as a default config?

OwenEdwards · 2017-02-06T16:24:58Z

To me, the name "tracksButton" is confusing because we have so many different kinds of tracks now, including (non-text) audio tracks. How about "subsCapsButton", something like that?

mister-ben · 2017-02-08T17:51:26Z

Agreed, tracksButton is inadequate. I haven't thought of anything better than subCapsButton that that's descriptive and accurate.

OwenEdwards · 2017-02-08T22:11:24Z

Looking at the way Safari handles this for videos with text tracks and an audio description track, it all gets rolled together with the Subtitles icon for the menu, then options for "Audio Track" and "Subtitles".
![2017-02-08_14-09-41]. Thoughts on that?(https://cloud.githubusercontent.com/assets/2245205/22759729/5f11e980-ee08-11e6-8401-c8f9a48c0a44.png)

gkatsev · 2017-02-13T23:40:31Z

src/js/control-bar/text-track-controls/off-text-track-menu-item.js

@@ -51,7 +58,7 @@ class OffTextTrackMenuItem extends TextTrackMenuItem {
    for (let i = 0, l = tracks.length; i < l; i++) {
      const track = tracks[i];

-      if (track.kind === this.track.kind && track.mode === 'showing') {
+      if ((this.track.kinds.indexOf(track.kind) > -1) && track.mode === 'showing') {


this line is erroring out for me. this.track.kindsdoesn't get set.

should it be this.options_.kinds?

gkatsev · 2017-02-13T23:46:12Z

src/css/components/_tracks-button.scss

+}
+
+// Todo
+.video-js .vjs-tracks-button .vjs-captions-menu-item::after {


we should continue using single colon for pseudo elements.

gkatsev · 2017-02-13T23:46:22Z

src/css/components/_tracks-button.scss

+  content: none;
+}
+
+.video-js:lang(en-US) .vjs-tracks-button .vjs-subtitles-menu-item::after,


I think this should be en?

gkatsev · 2017-02-13T23:47:36Z

src/css/components/_tracks-button.scss

+}
+
+// Todo
+.video-js .vjs-tracks-button .vjs-captions-menu-item::after {


so, captions are always indicated as [cc] and subs are only indicated as such in en and fr-CA locales?

gkatsev · 2017-02-13T23:48:17Z

src/js/control-bar/control-bar.js

@@ -67,6 +68,7 @@ ControlBar.prototype.options_ = {
    'descriptionsButton',
    'subtitlesButton',
    'captionsButton',
+    'tracksButton',


What about removing from here and updating the combined-tracks test page to include all the separate caption buttons in the control bar as a default config?

mister-ben · 2017-02-14T00:32:01Z

Updated and rebased.

I've left the old controls in for the moment as several tests need to be modified to take them out of the default ControlBar.

To distinguish captions in the menu, I've added the CC icon rather than "[cc]" text now, and for any locale - thoughts on whether that's readable enough? The material cc icon isn't great at small sizes.

gkatsev · 2017-02-14T16:23:56Z

src/js/control-bar/control-bar.js

@@ -68,6 +69,7 @@ ControlBar.prototype.options_ = {
    'descriptionsButton',
    'subtitlesButton',
    'captionsButton',
+    'subCapsButton',


this should be subsCapsButton took me forever to figure out why it wasn't working for me, haha

Urgh, sorry.

gkatsev · 2017-02-14T16:32:54Z

src/css/components/_subs-caps.scss

+}
+
+// Todo
+.video-js .vjs-subs-caps-button + .vjs-menu .vjs-captions-menu-item .vjs-menu-item-text:after {


this should be vertical-align: baseline so it's all on the same line.

vertical-align: bottom seems to work better.

mister-ben · 2017-02-16T21:59:02Z

I think this is in a viable state now, with updated tests

gkatsev

One thing I just realized is that after this, the Captions settings dialog would need to be updated. Currently, it returns focus directly to the captionsbutton, but would need to change slightly with this change, so, return to this menu button if it exists, otherwise, to the captions button, if that exists.

gkatsev · 2017-02-21T23:22:44Z

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

+     * @type {string}
+     * @private
+     */
+    SubsCapsButton.prototype.controlText_ = this.label_;


Should this just be subtitles as a default and then in here we just do this.controlText_ = this.label_?

The controlText wasn't getting set at all, that approach fixes it.

gkatsev · 2017-02-21T23:25:38Z

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

+
+    // Although North America uses "captions" in most cases for
+    // "captions and subtitles" other locales use "subtitles"
+    this.label_ = 'subtitles';


Does this need to be localized?

This gets concatenated into 'captions setttings' and 'captions off', which do get localised.

gkatsev

Looks like the cc icon in the menu is read out by the screen reader, we should wrap it in the icon-placeholder span like in other components.

gkatsev · 2017-02-22T22:06:53Z

src/css/components/_subs-caps.scss

+}
+
+.video-js .vjs-subs-caps-button + .vjs-menu .vjs-captions-menu-item .vjs-menu-item-text:after {
+  content: " \f10d";


this is showing up in the screen reader. We need to add an icon placeholder span.

Good catch @gkatsev !! I haven't had a chance to check this, but is there some controlText for the icon, so that something is read out by a screen reader instead of the icon? Otherwise, that's an #a11y violation too - information which is conveyed visually, but not to SR users.

OK, this can't be done with CSS alone in that case.

Yeah, I don't think this is possible to do in CSS. @mister-ben do you think you can get to it this week? Otherwise, I'm thinking we can merge it in as is and I or someone else can tackle it.

I probably won't have a chance to look at this now before the end of next week.

Cool, thanks @mister-ben this is pretty awesome as it is. We'll take care of this final piece!

OwenEdwards · 2017-02-23T17:57:50Z

My one slight concern about merging this PR is that, while the feature looks great, the dummy WebVTT tracks in the docs/examples/shared folder are just for testing, and aren't really what they say they are. Also, the test HTML file in the sandbox is the only one with a .html extension; we discussed why they have the .example extension a while ago, and I'd love to change it, but we ought to go one way or another with this, for consistency.

mister-ben · 2017-02-24T11:13:40Z

@OwenEdwards good point, I'll clean that up. The Oceans video doesn't really lend itself to a meaningful demo of captions and subtitles at all though.

gkatsev · 2017-02-24T15:54:04Z

Maybe we should switch the default video to something like elephants dream which has all the tracks? At least for the sandbox?

OwenEdwards · 2017-02-24T20:14:56Z

sandbox/combined-tracks.html.example

-    <track kind="subtitles" src="../docs/examples/shared/example-subtitles-fr.vtt" srclang="fr" label="French"></track><!-- Tracks need an ending tag thanks to IE9 -->
-    <track kind="subtitles" src="../docs/examples/shared/example-subtitles-de.vtt" srclang="de" label="German"></track><!-- Tracks need an ending tag thanks to IE9 -->
+  <video id="vid1" class="video-js vjs-default-skin" lang="en" controls preload="auto" width="640" height="264" poster="http://vjs.zencdn.net/v/oceans.png">
+    <source src="//d2zihajmogu5jn.cloudfront.net/elephantsdream/ed_hd.mp4" type="video/mp4">


Nice! I guess I ought to move the Descriptions example to use this Cloudfront URL!

This is the content used at videojs.com/advanced

OwenEdwards · 2017-02-24T22:25:16Z

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

@@ -0,0 +1,102 @@
+/**
+ * @file tracks-button.js


Just noticed - this file name needs updating. (<Captain Pedantic, at you're service ;-)>)

Ah yes. (Not rising to "you're"...)

OwenEdwards reviewed Feb 6, 2017

View reviewed changes

mister-ben mentioned this pull request Feb 9, 2017

Add lang attribute to player el #4046

Merged

5 tasks

gkatsev requested changes Feb 13, 2017

View reviewed changes

mister-ben force-pushed the combined-tracks branch from 6c3dd28 to 46a7035 Compare February 14, 2017 00:27

gkatsev reviewed Feb 14, 2017

View reviewed changes

mister-ben force-pushed the combined-tracks branch from 46a7035 to 5e5eae1 Compare February 16, 2017 19:10

mister-ben changed the title ~~WIP: Combined tracks control~~ Combined tracks control Feb 16, 2017

gkatsev mentioned this pull request Feb 21, 2017

fix(MenuButton) Implement unified method of showing/hiding MenuButton #3993

Merged

2 tasks

gkatsev approved these changes Feb 22, 2017

View reviewed changes

mister-ben added 7 commits February 22, 2017 23:50

Combined text track button

72b4c45

Correct typo

cf36fc4

Update test players to include captions button where needed

ce99947

Align and increase size of CC icon in menu

6cd1fff

use lowercase for captions/subtitles to match translations and tests

19d0091

update tests to use subsCapsButton

2a57cc9

Update menu button\'s control text

e876942

mister-ben force-pushed the combined-tracks branch from c701aad to e876942 Compare February 22, 2017 21:16

gkatsev requested changes Feb 22, 2017

View reviewed changes

Clean up example and use Elephants Dream

b77d2bc

OwenEdwards reviewed Feb 24, 2017

View reviewed changes

correct filename in jsdoc comment

b273528

gkatsev approved these changes Mar 2, 2017

View reviewed changes

gkatsev merged commit 74eb5d4 into videojs:master Mar 2, 2017

Combined tracks control #4028

Combined tracks control #4028

Conversation

mister-ben commented Feb 2, 2017

Description

Specific Changes proposed

Requirements Checklist

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

OwenEdwards commented Feb 6, 2017

mister-ben commented Feb 8, 2017

OwenEdwards commented Feb 8, 2017

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

mister-ben commented Feb 14, 2017 • edited Loading

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

mister-ben commented Feb 16, 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

gkatsev left a comment

Choose a reason for hiding this comment

gkatsev Feb 22, 2017 • edited Loading

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

OwenEdwards commented Feb 23, 2017 • edited Loading

mister-ben commented Feb 24, 2017

gkatsev commented Feb 24, 2017

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

mister-ben commented Feb 14, 2017 •

edited

Loading

gkatsev Feb 22, 2017 •

edited

Loading

OwenEdwards commented Feb 23, 2017 •

edited

Loading