feat: fix accessibility of the captions setting dialog #4050
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,9 @@ | |
| * @file text-track-settings.js | ||
| */ | ||
| import window from 'global/window'; | ||
| import document from 'global/document'; | ||
| import Component from '../component'; | ||
| import ModalDialog from '../modal-dialog'; | ||
| import {createEl} from '../utils/dom'; | ||
| import * as Fn from '../utils/fn'; | ||
| import * as Obj from '../utils/obj'; | ||
|
|
@@ -236,9 +238,9 @@ function setSelectedOption(el, value, parser) { | |
| /** | ||
| * Manipulate Text Tracks settings. | ||
| * | ||
| * @extends ModalDialog | ||
| */ | ||
| class TextTrackSettings extends ModalDialog { | ||
|
|
||
| /** | ||
| * Creates an instance of this class. | ||
|
|
@@ -250,20 +252,34 @@ class TextTrackSettings extends Component { | |
| * The key/value store of player options. | ||
| */ | ||
| constructor(player, options) { | ||
| options.temporary = false; | ||
|
|
||
| super(player, options); | ||
|
|
||
| this.contentEl().className += ' vjs-caption-settings'; | ||
|
|
||
| this.updateDisplay = Fn.bind(this, this.updateDisplay); | ||
|
|
||
| // fill the modal and pretend we have opened it | ||
| this.fill(); | ||
| this.hasBeenOpened_ = this.hasBeenFilled_ = true; | ||
|
|
||
| this.endDialog = createEl('p', { | ||
| className: 'vjs-control-text', | ||
| textContent: this.localize('End of dialog window.') | ||
| }); | ||
| this.el().appendChild(this.endDialog); | ||
|
|
||
| this.setDefaults(); | ||
|
|
||
| // Grab `persistTextTrackSettings` from the player options if not passed in child options | ||
| if (options.persistTextTrackSettings === undefined) { | ||
| this.options_.persistTextTrackSettings = this.options_.playerOptions.persistTextTrackSettings; | ||
| } | ||
|
|
||
| this.on(this.$('.vjs-done-button'), 'click', () => { | ||
| this.saveSettings(); | ||
| this.close(); | ||
| }); | ||
|
|
||
| this.on(this.$('.vjs-default-button'), 'click', () => { | ||
|
|
@@ -290,21 +306,28 @@ class TextTrackSettings extends Component { | |
| * The DOM element that gets created. | ||
| * @private | ||
| */ | ||
| createElSelect_(key, legendId = '') { | ||
| const config = selectConfigs[key]; | ||
| const id = config.id.replace('%s', this.id_); | ||
|
|
||
| return [ | ||
| createEl('label', { | ||
| id, | ||
| className: 'vjs-label', | ||
| textContent: this.localize(config.label) | ||
| }, { | ||
| }), | ||
| createEl('select', {}, { | ||
| 'aria-labelledby': `${legendId} ${id}` | ||
| }, config.options.map(o => { | ||
| const optionId = id + '-' + o[1]; | ||
|
|
||
| return createEl('option', { | ||
| id: optionId, | ||
| textContent: this.localize(o[1]), | ||
| value: o[0] | ||
| }, { | ||
| 'aria-labelledby': `${legendId} ${id} ${optionId}` | ||
| }); | ||
| })) | ||
| ]; | ||
|
|
@@ -320,14 +343,15 @@ class TextTrackSettings extends Component { | |
| */ | ||
| createElFgColor_() { | ||
| const legend = createEl('legend', { | ||
| id: `captions-text-legend-${this.id_}`, | ||
| textContent: this.localize('Text') | ||
| }); | ||
|
|
||
| const select = this.createElSelect_('color', legend.id); | ||
|
|
||
| const opacity = createEl('span', { | ||
| className: 'vjs-text-opacity vjs-opacity' | ||
| }, undefined, this.createElSelect_('textOpacity', legend.id)); | ||
|
|
||
| return createEl('fieldset', { | ||
| className: 'vjs-fg-color vjs-tracksetting' | ||
|
|
@@ -344,14 +368,15 @@ class TextTrackSettings extends Component { | |
| */ | ||
| createElBgColor_() { | ||
| const legend = createEl('legend', { | ||
| id: `captions-background-${this.id_}`, | ||
| textContent: this.localize('Background') | ||
| }); | ||
|
|
||
| const select = this.createElSelect_('backgroundColor', legend.id); | ||
|
|
||
| const opacity = createEl('span', { | ||
| className: 'vjs-bg-opacity vjs-opacity' | ||
| }, undefined, this.createElSelect_('backgroundOpacity', legend.id)); | ||
|
|
||
| return createEl('fieldset', { | ||
| className: 'vjs-bg-color vjs-tracksetting' | ||
|
|
@@ -368,14 +393,15 @@ class TextTrackSettings extends Component { | |
| */ | ||
| createElWinColor_() { | ||
| const legend = createEl('legend', { | ||
| id: `captions-window-${this.id_}`, | ||
| textContent: this.localize('Window') | ||
| }); | ||
|
|
||
| const select = this.createElSelect_('windowColor', legend.id); | ||
|
|
||
| const opacity = createEl('span', { | ||
| className: 'vjs-window-opacity vjs-opacity' | ||
| }, undefined, this.createElSelect_('windowOpacity', legend.id)); | ||
|
|
||
| return createEl('fieldset', { | ||
| className: 'vjs-window-color vjs-tracksetting' | ||
|
|
@@ -435,9 +461,11 @@ class TextTrackSettings extends Component { | |
| * @private | ||
| */ | ||
| createElControls_() { | ||
| const defaultsDescription = this.localize('restore all settings to the default values'); | ||
| const defaultsButton = createEl('button', { | ||
| className: 'vjs-default-button', | ||
| title: defaultsDescription, | ||
| innerHTML: `${this.localize('Defaults')}<span class='vjs-control-text'> - ${defaultsDescription}</span>` | ||
|
There was a problem hiding this comment. @OwenEdwards @mmcc brought up that maybe 'Reset' would be a better name for this button, thoughts? There was a problem hiding this comment. I think that would work fine - if the button is marked 'Reset', then the hidden text can be ' all settings to the default values', and you don't need the ' - ' in the hidden text. There was a problem hiding this comment. Reset seems like a better name to me |
||
| }); | ||
|
|
||
| const doneButton = createEl('button', { | ||
|
|
@@ -457,6 +485,10 @@ class TextTrackSettings extends Component { | |
| * The element that was created. | ||
| */ | ||
| createEl() { | ||
| return super.createEl(); | ||
|
There was a problem hiding this comment. I don't know if we need this here, won't this be the default behavior? There was a problem hiding this comment. Um... yeah, probably not needed. |
||
| } | ||
|
|
||
| content() { | ||
|
There was a problem hiding this comment. This function and a few of the funcitons below this need jsdoc comments There was a problem hiding this comment. JSDoc gets them from the parent. |
||
| const settings = createEl('div', { | ||
| className: 'vjs-tracksettings' | ||
| }, undefined, [ | ||
|
|
@@ -465,33 +497,19 @@ class TextTrackSettings extends Component { | |
| this.createElControls_() | ||
| ]); | ||
|
|
||
| return settings; | ||
| } | ||
|
|
||
| label() { | ||
| return this.localize('Caption Settings Dialog'); | ||
| } | ||
|
|
||
| description() { | ||
| return this.localize('Beginning of dialog window. Escape will cancel and close the window.'); | ||
| } | ||
|
|
||
| buildCSSClass() { | ||
| return super.buildCSSClass() + ' vjs-text-track-settings'; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -525,7 +543,7 @@ class TextTrackSettings extends Component { | |
| } | ||
|
|
||
| /** | ||
| * Sets all `<select>` elements to their default values. | ||
| */ | ||
| setDefaults() { | ||
| Obj.each(selectConfigs, (config) => { | ||
|
|
@@ -584,6 +602,23 @@ class TextTrackSettings extends Component { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * conditionally blur the element and refocus the captions button | ||
| * | ||
| * @private | ||
| */ | ||
| conditionalBlur_() { | ||
| this.previouslyActiveEl_ = null; | ||
| this.off(document, 'keydown', this.handleKeyDown); | ||
|
|
||
| const cb = this.player_.controlBar; | ||
| const ccBtn = cb && cb.captionsButton; | ||
|
|
||
| if (ccBtn) { | ||
| ccBtn.focus(); | ||
| } | ||
| } | ||
|
|
||
| } | ||
|
|
||
| Component.registerComponent('TextTrackSettings', TextTrackSettings); | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@OwenEdwards did we have an answer whether we should keep this because of VO and Chrome or remove it?
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.
I'm a little concerned about how well this will work - there's a circular reference where the label for this element includes the
idof two other elements, and then the id of this element. The behavior in this situation is mentioned in the W3C Accessible Name Computation - Text Alternative Computation:but it may not work correctly in all browsers. I guess we'll just have to fix it if that happens?
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.
from the little I understand about this, I think it should be fine and it falls within what the spec allows.
I'm 👌 with leaving this and then fixing in the future if we find an issue.