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 Captions Settings Dialog labeling for accessibility #3281

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
2 participants
@OwenEdwards
Member

OwenEdwards commented Apr 26, 2016

Description

Based on #3054, this PR fixes screen reader labeling of the Captions Settings Dialog. This fixes point 1 of #2746.

Specific Changes proposed

Add fieldsets & legends, and off-screen text for labels. Also, remove the '---' values.

Requirements Checklist

  • Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Unit Tests updated or fixed
  • Reviewed by Two Core Contributors
Show outdated Hide outdated src/js/tracks/text-track-settings.js
let template = `
<div role="document">
<h1 id="${dialogLabelId}" class="vjs-control-text">Captions Settings Dialog</h1>

This comment has been minimized.

@gkatsev

gkatsev Apr 29, 2016

Member

We probably should use a div here instead of h1, I had to change the elements used in videojs-dock because websites often style h1 elements and it broke the title and descriptions of the plugin

@gkatsev

gkatsev Apr 29, 2016

Member

We probably should use a div here instead of h1, I had to change the elements used in videojs-dock because websites often style h1 elements and it broke the title and descriptions of the plugin

This comment has been minimized.

@OwenEdwards

OwenEdwards Apr 29, 2016

Member

Even though class="vjs-control-text" means this one is visually hidden, and just there for Screen Readers?

@OwenEdwards

OwenEdwards Apr 29, 2016

Member

Even though class="vjs-control-text" means this one is visually hidden, and just there for Screen Readers?

This comment has been minimized.

@gkatsev

gkatsev Apr 29, 2016

Member

Ah, right, I guess that's fine then.

@gkatsev

gkatsev Apr 29, 2016

Member

Ah, right, I guess that's fine then.

Show outdated Hide outdated src/js/tracks/text-track-settings.js
<div role="document">
<h1 id="${dialogLabelId}" class="vjs-control-text">Captions Settings Dialog</h1>
<div id="${dialogDescriptionId}" class="vjs-control-text">Beginning of dialog window. Escape will cancel and close the window.</div>
<div class=""></div>

This comment has been minimized.

@gkatsev

gkatsev Apr 29, 2016

Member

Looks superfluous.

@gkatsev

gkatsev Apr 29, 2016

Member

Looks superfluous.

@@ -9,6 +9,16 @@ const tracks = [{
label: 'test'
}];
const defaultSettings = {

This comment has been minimized.

@gkatsev

gkatsev Apr 29, 2016

Member

I'm not a fan of having these default values, I would prefer if vttjs just did whatever it wanted by default unless a user selects a value.
Maybe the --- values should just change to be called default? or something?

@gkatsev

gkatsev Apr 29, 2016

Member

I'm not a fan of having these default values, I would prefer if vttjs just did whatever it wanted by default unless a user selects a value.
Maybe the --- values should just change to be called default? or something?

This comment has been minimized.

@OwenEdwards

OwenEdwards Apr 29, 2016

Member

I think you've got the logic of this backwards - basic Text Tracks do not specify a foreground or background color, and nor does vtt.js when it parses them, so there needs to be a "I want text displayed this way" setting, so "default" doesn't make sense.

Some cues in some tracks do specify a color or other styling, in which case the setting option ought to be "leave it as specified in the track" for those cues which have a specification in the track, or "force it to the styling which I've specified, overriding what's in the cue/track".

So "---" or "default" doesn't make sense, although there ought to be an extra checkbox (or slide switch) for each setting saying "override track's own setting".

@OwenEdwards

OwenEdwards Apr 29, 2016

Member

I think you've got the logic of this backwards - basic Text Tracks do not specify a foreground or background color, and nor does vtt.js when it parses them, so there needs to be a "I want text displayed this way" setting, so "default" doesn't make sense.

Some cues in some tracks do specify a color or other styling, in which case the setting option ought to be "leave it as specified in the track" for those cues which have a specification in the track, or "force it to the styling which I've specified, overriding what's in the cue/track".

So "---" or "default" doesn't make sense, although there ought to be an extra checkbox (or slide switch) for each setting saying "override track's own setting".

This comment has been minimized.

@gkatsev

gkatsev Apr 29, 2016

Member

Yeah, I meant with how the workflow from this dialog is. The selection boxes turn into an object which we then use down the line to see if we need to apply extra styles to the elements that vttjs produced.
By default, no such extra processing should be done; also, if the persistence option is set, we shouldn't record these default values.
Checkboxes that toggle each override could be ok, since that's sort of what the --- option is supposed to represent.

@gkatsev

gkatsev Apr 29, 2016

Member

Yeah, I meant with how the workflow from this dialog is. The selection boxes turn into an object which we then use down the line to see if we need to apply extra styles to the elements that vttjs produced.
By default, no such extra processing should be done; also, if the persistence option is set, we shouldn't record these default values.
Checkboxes that toggle each override could be ok, since that's sort of what the --- option is supposed to represent.

This comment has been minimized.

@OwenEdwards

OwenEdwards Jun 23, 2016

Member

@gkatsev I needed to do some more research on this; I was wrong about "... do not specify a foreground or background color, and nor does vtt.js when it parses them".

The idea of --- or default in the Captions Settings Dialog is bogus, since if a Text Track or cue doesn't specify its own styling, then the default is buried in vtt.js, and is entirely arbitrary; neither the WebVTT standard nor anything from the FCC specifies a "default" setting for caption display if none is specified by the captions track. In my mind, the vtt.js default is equivalent to the Need Text in video.js' components - it's there in case nothing else is specified, but is no kind of "default".

Which takes me back to my point that there shouldn't be a default in drop downs the video.js Captions Settings Dialog; there should be a default set of settings for color, opacity/transparency and font/outline/etc. (which can match the existing behavior of vtt.js for consistency), and a new setting which should be an option of whether this overrides any color specified in the Text Track or not.

Whether the settings are saved by video.js and then restored (the "persistence option") is a separate feature from all of this.

@OwenEdwards

OwenEdwards Jun 23, 2016

Member

@gkatsev I needed to do some more research on this; I was wrong about "... do not specify a foreground or background color, and nor does vtt.js when it parses them".

The idea of --- or default in the Captions Settings Dialog is bogus, since if a Text Track or cue doesn't specify its own styling, then the default is buried in vtt.js, and is entirely arbitrary; neither the WebVTT standard nor anything from the FCC specifies a "default" setting for caption display if none is specified by the captions track. In my mind, the vtt.js default is equivalent to the Need Text in video.js' components - it's there in case nothing else is specified, but is no kind of "default".

Which takes me back to my point that there shouldn't be a default in drop downs the video.js Captions Settings Dialog; there should be a default set of settings for color, opacity/transparency and font/outline/etc. (which can match the existing behavior of vtt.js for consistency), and a new setting which should be an option of whether this overrides any color specified in the Text Track or not.

Whether the settings are saved by video.js and then restored (the "persistence option") is a separate feature from all of this.

This comment has been minimized.

@gkatsev

gkatsev Jun 24, 2016

Member

I think we are in agreement.
The dropdowns shouldn't have any defaults or "empty values" and each setting should have an option of whether it will be used.

@gkatsev

gkatsev Jun 24, 2016

Member

I think we are in agreement.
The dropdowns shouldn't have any defaults or "empty values" and each setting should have an option of whether it will be used.

This comment has been minimized.

@OwenEdwards

OwenEdwards Jun 24, 2016

Member

We're almost in agreement ;-)

[x] The dropdowns shouldn't have any defaults or "empty values"
[ ] See YouTube and iOS - the "checkbox" (whether for each value or for the settings as a whole) defines whether styling in the WebVTT overrides the values set by the user. That's not quite the same as whether or not to change what vtt.js does - the vtt.js defaults would disappear, and be replaced by the settings in video.js's Settings Dialog, although those setting values may or may not be changed by settings in the WebVTT file.

I can see the value in being able to completely turn off the Captions Settings functionality and not changing what vtt.js does (as was discussed elsewhere), and that could be implemented as a separate PR with a video.js parameter to control it.

Implementing this behavior of Captions Settings control won't be easy with the existing vtt.js though, and it's interface to Services and WebVTTSet object. We would need to fork vtt.js, and hope we can push a better user control API upstream to them.

@OwenEdwards

OwenEdwards Jun 24, 2016

Member

We're almost in agreement ;-)

[x] The dropdowns shouldn't have any defaults or "empty values"
[ ] See YouTube and iOS - the "checkbox" (whether for each value or for the settings as a whole) defines whether styling in the WebVTT overrides the values set by the user. That's not quite the same as whether or not to change what vtt.js does - the vtt.js defaults would disappear, and be replaced by the settings in video.js's Settings Dialog, although those setting values may or may not be changed by settings in the WebVTT file.

I can see the value in being able to completely turn off the Captions Settings functionality and not changing what vtt.js does (as was discussed elsewhere), and that could be implemented as a separate PR with a video.js parameter to control it.

Implementing this behavior of Captions Settings control won't be easy with the existing vtt.js though, and it's interface to Services and WebVTTSet object. We would need to fork vtt.js, and hope we can push a better user control API upstream to them.

This comment has been minimized.

@gkatsev

gkatsev Jun 24, 2016

Member

We don't actually need to worry about the Services and WebVTTSet in vttjs because that's only used in native firefox. However, I wish that it was possible to get a build of vttjs without those. Also, we're still using a fork to enable our usage of vttjs the way we want.
I think I need to load up this PR and think about it some more.

@gkatsev

gkatsev Jun 24, 2016

Member

We don't actually need to worry about the Services and WebVTTSet in vttjs because that's only used in native firefox. However, I wish that it was possible to get a build of vttjs without those. Also, we're still using a fork to enable our usage of vttjs the way we want.
I think I need to load up this PR and think about it some more.

This comment has been minimized.

@OwenEdwards

OwenEdwards Jun 24, 2016

Member

We could suggest that vtt.js provides an API which allows user settings, including whether to override styling in the WebVTT or not, and that Services and WebVTTSet are one thing which drive that flexible API, but if Services does not exist, it's still possible to drive the user settings API. That's what I was thinking we should create in our fork, and then submit a PR to the Mozilla repo.

Because inside vtt.js is by far the easiest place to detect whether the WebVTT has styling or not ;-)

@OwenEdwards

OwenEdwards Jun 24, 2016

Member

We could suggest that vtt.js provides an API which allows user settings, including whether to override styling in the WebVTT or not, and that Services and WebVTTSet are one thing which drive that flexible API, but if Services does not exist, it's still possible to drive the user settings API. That's what I was thinking we should create in our fork, and then submit a PR to the Mozilla repo.

Because inside vtt.js is by far the easiest place to detect whether the WebVTT has styling or not ;-)

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jun 20, 2016

Member

Hey, @OwenEdwards any updates on this one? Thoughts on my latest comment?

Member

gkatsev commented Jun 20, 2016

Hey, @OwenEdwards any updates on this one? Thoughts on my latest comment?

@gkatsev gkatsev closed this in 238c752 Jul 19, 2016

@OwenEdwards OwenEdwards deleted the OwenEdwards:fix/captions-settings-labeling-for-a11y branch Jul 22, 2016

@OwenEdwards OwenEdwards restored the OwenEdwards:fix/captions-settings-labeling-for-a11y branch Jul 22, 2016

@OwenEdwards OwenEdwards deleted the OwenEdwards:fix/captions-settings-labeling-for-a11y branch Jul 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment