-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Basic descriptions track support #3098
Basic descriptions track support #3098
Conversation
@@ -124,6 +124,10 @@ body.vjs-full-window { | |||
// Hide disabled or unsupported controls. | |||
.vjs-hidden { display: none !important; } | |||
|
|||
.vjs-disabled { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can also change the cursor
not-allowed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering whether cursor: not-allowed
is overkill, since a disabled button <button type="button" disabled>Click Me!</button>
only shows cursor: default
instead of cursor: pointer
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cursor: pointer
seems fine as well.
Thanks @OwenEdwards, looks great. |
@gkatsev would love some help in building out the unit tests, as I mention. Otherwise, I think this functionality is ready to go with these latest CSS updates. |
Sorry it's been a while. I should be able to help out with tests for this PR this week. |
Also, looks like this needs to be rebase or updated from master. |
I'll take a look. If I do a rebase, then it'll have to become a new PR, right? Not a big deal, just wanted to know if I'm missing something. |
You can force push if you want. Or you could merge. |
Descriptions tracks are enabled by their own Control Bar menu button, and shown like other text tracks. Display of a text track is marked as 'aria-live', so it will be read by a screen reader when it changes. A descriptions track cannot be displayed at the same time as a Subtitles or Captions track; if either of those tracks is enabled, then the Descriptions track is disabled, and the menu button is also disabled.
…k in the sandbox as descriptions.html.example. Track examples are in docs/examples/elephantsdream. Descriptions text track from @silviapfeiffer, Captions from https://github.com/walsh9/videojs-transcript/tree/master/captions (converted from SRT to WebVTT) Chapters text track created by @OwenEdwards.
2683ba3
to
d5c79f3
Compare
Okay, this is rebased, simplified (fewer separate commits) and fixed to work with my other PR. Just needs some work on the tests, then it's ready to go. |
+1 |
*/ | ||
enable() { | ||
this.removeClass('vjs-disabled'); | ||
this.el_.setAttribute('aria-disabled', 'false'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not necessary for this, but we're doing a lot of attribute setting, we probably should make a helper for components so you could just do this.setAttribute('aria-disabled', 'false');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 That would be a good place to check that we're consistently passing 'false' or 'true' (strings) to ARIA attributes, rather than booleans, which I think causes problems.
Looks like the main missing unit test from this PR is one that checks that the descriptions button is disabled when caption or subtitle is enabled. |
…on being disabled when captions are showing.
@gkatsev I think this covers the tests we talked about. Can you take a look and let me know? |
expect(6); | ||
|
||
var player, menuButton, el; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be 3 let
s.
We really need to update videojs to use vjsstandard
already so I won't have to keep pointing these out myself :)
test('enabling a captions track should disable the descriptions menu button', function() { | ||
expect(14); | ||
|
||
var player = TestHelpers.makePlayer({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let
@OwenEdwards thanks! Looks good. Just some stylistic issues left :) |
@gkatsev think that does it! |
Awesome. LGTM! |
@gkatsev realized that I needed to add some strings into en.json. Other than that, this is ready :-) |
Cool! |
👍 |
Kind of an aside, but does description track support work with |
@chemoish this change requires a @gkatsev, I guess we ought to document this somewhere? |
Yes, we need a text tracks guide doc. Also, maybe we should just always subsume descriptions into emulated mode until browsers that implementing them? We can always do that as a follow-up PR. |
Adds support for descriptions text tracks, with a separate menu button on the control bar, and display using the same mechanism as captions and subtitles. To handle the situation where a descriptions and a captions or subtitles track could be enabled at the same time, the TextTrackDisplay prefers non-descriptions tracks over descriptions tracks, and the descriptions menu button disables itself if another track is enabled.
This PR could probably use some more Unit tests to check the priority model is TextTrackDisplay, and the self-disabling in descriptions-button. Also, the CSS styling applied to the button when it is disabled (just making it 0.5 opacity) may need more work.
I'd appreciate any feedback, suggestions, etc.