feat: fix accessibility of the captions setting dialog #4050

gkatsev · 2017-02-09T22:39:37Z

This fixes a lot of the issues from #2746 by making the dialog inherit from our actual ModalDialog which now has tab focus trapping.
Todo copied from #2746 to track what has been fixed. #2746 should be used to track what has been verified as fixed.

The Captions Settings dialog has some accessibility issues:

gkatsev · 2017-02-09T22:41:34Z

@OwenEdwards I'm not sure how the "extent" is supposed to be specified.

OwenEdwards · 2017-02-09T22:57:02Z

Here's the idea of the "extent" of the dialog:

The top and bottom of the dialog area are clearly marked for sighted users. Focus is constrained inside the dialog, but a screen reader user can use the up and down arrow keys to move the "virtual cursor" and read content which isn't focusable/actionable. If they move out of the top or bottom of the dialog and into other content in the DOM, they may not realize that this has happened, and may become confused/lost.

The preferred solution is to use aria-hidden="true" on all of the rest of the page except the dialog - this is tricky because aria-hidden doesn't nest, so you can't have aria-hidden="false" on the dialog element when it is a child of the main page which has aria-hidden="true". So you need to mark everything else as aria-hidden, or have a structure like this:

<body>
    <div class="main" aria-hidden="true> ... </div>
    <div role="dialog" ... > .... </div>
</body>

Alternatively, you can use hidden (screen reader only) text to say "Start of modal dialog", "End of modal dialog" at exactly the place where the extent of the dialog is visibly marked in the DOM.

There has been some talk that marking an ARIA region, or even the role="dialog", ought to be enough in itself, but screen readers don't always announce movement of the virtual cursor into/out of an ARIA region.

gkatsev · 2017-02-09T23:16:17Z

It sounds like have a "start" and "end" elements that are only available to screen readers is what we want.

OwenEdwards · 2017-02-09T23:22:55Z

Right, and I forgot that we already have Beginning of dialog window...., so we only need End of dialog window.

gkatsev · 2017-02-09T23:34:34Z

@OwenEdwards this is ready for testing. Only issue is #4049.

gkatsev · 2017-02-10T22:36:13Z

@OwenEdwards I'm not really sure how to improve the highlighting of the defaults and done buttons. Both of those are using the default outlines when just using the keyboard or using voiceover.

The others can definitely be fixed and I've already fixed the last one.

gkatsev · 2017-02-10T23:02:30Z

I decided to update it so that it re-focuses on the captions button when it is closed.

OwenEdwards

Tested with IE11 & JAWS - a couple of changes needed. Also, it's failing one of the tests: ModalDialog should create the expected description element.

OwenEdwards · 2017-02-11T21:46:41Z

src/js/tracks/text-track-settings.js

      }),
-      createEl('select', {id}, undefined, config.options.map(o => {
+      createEl('select', {}, {
+        'aria-labeledby': `${legendId} ${id}`


TYPO? aria-labelledby isn't spelled the way you'd expect!

OwenEdwards · 2017-02-11T21:47:33Z

src/js/tracks/text-track-settings.js

          textContent: this.localize(o[1]),
          value: o[0]
+        }, {
+          'aria-labeledby': `${legendId} ${id} ${optionId}`


You don't need to aria-label each <option>, only the <select>.

I was testing with voiceover on mac. Seems like Safari reads it out like White, Text Color, menu item but on chrome it reads it out only as white unless I add this labelledby.

misteroneill

It looks like several methods of TextTrackSettings are missing JSDoc comments.

gkatsev · 2017-02-14T22:03:10Z

@misteroneill all the missing jsdocs are fixed by adding @extends ModalDialog.

gkatsev · 2017-02-15T17:11:31Z

src/js/tracks/text-track-settings.js

          textContent: this.localize(o[1]),
          value: o[0]
+        }, {
+          'aria-labelledby': `${legendId} ${id} ${optionId}`


@OwenEdwards did we have an answer whether we should keep this because of VO and Chrome or remove it?

I'm a little concerned about how well this will work - there's a circular reference where the label for this element includes the id of 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:

if computing a name, and the current node has a non-empty aria-labelledby attribute, and the current node is not already part of an aria-labelledby traversal, process its IDREFs in the order they occur:

but it may not work correctly in all browsers. I guess we'll just have to fix it if that happens?

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.

OwenEdwards · 2017-02-15T23:32:59Z

src/js/tracks/text-track-settings.js

          textContent: this.localize(o[1]),
          value: o[0]
+        }, {
+          'aria-labelledby': `${legendId} ${id} ${optionId}`


I'm a little concerned about how well this will work - there's a circular reference where the label for this element includes the id of 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:

if computing a name, and the current node has a non-empty aria-labelledby attribute, and the current node is not already part of an aria-labelledby traversal, process its IDREFs in the order they occur:

but it may not work correctly in all browsers. I guess we'll just have to fix it if that happens?

* update caption settings button styles * minor button style updates

gkatsev · 2017-02-16T19:14:55Z

src/js/tracks/text-track-settings.js

    const defaultsButton = createEl('button', {
      className: 'vjs-default-button',
-      textContent: this.localize('Defaults')
+      title: defaultsDescription,
+      innerHTML: `${this.localize('Defaults')}<span class='vjs-control-text'> - ${defaultsDescription}</span>`


@OwenEdwards @mmcc brought up that maybe 'Reset' would be a better name for this button, thoughts?

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.

Reset seems like a better name to me

brandonocasey · 2017-02-16T19:58:38Z

src/js/tracks/text-track-settings.js

@@ -457,6 +485,10 @@ class TextTrackSettings extends Component {
   *         The element that was created.
   */
  createEl() {
+    return super.createEl();


I don't know if we need this here, won't this be the default behavior?

Um... yeah, probably not needed.

brandonocasey · 2017-02-16T20:00:09Z

src/js/tracks/text-track-settings.js

+    return super.createEl();
+  }
+
+  content() {


This function and a few of the funcitons below this need jsdoc comments

JSDoc gets them from the parent.

gkatsev added the a11y This item might affect the accessibility of the player label Feb 10, 2017

OwenEdwards requested changes Feb 11, 2017

View reviewed changes

misteroneill reviewed Feb 13, 2017

View reviewed changes

gkatsev commented Feb 15, 2017

View reviewed changes

OwenEdwards approved these changes Feb 15, 2017

View reviewed changes

gkatsev added 13 commits February 16, 2017 14:02

make the text track settings inherit from the ModalDialog

917883e

describe defaults button to screen readers

58d5bef

better select and option element labeling

d02ce22

add a 'End of dialog window' element

7f6e543

use vjs-control-text rather than vjs-offscreen for SR hiding

5068e0f

re-focus ccBtn on close. Close modal when 'done' is clicked

f35a474

fix test that checks for offscreen, now control-text

c5a6811

redo how we describe defaults button

6ae7828

fix aria-labelledby typo

33e6f21

update TTS jsdoc

7c65c3e

update buttons (#4096)

690ab9b

* update caption settings button styles * minor button style updates

switch btn back to 'Defaults'

707768f

update translations-needed

1946cbe

gkatsev force-pushed the caption-settings-modal branch from e30adff to 1946cbe Compare February 16, 2017 19:14

gkatsev commented Feb 16, 2017

View reviewed changes

brandonocasey requested changes Feb 16, 2017

View reviewed changes

misteroneill approved these changes Feb 16, 2017

View reviewed changes

gkatsev added 3 commits February 16, 2017 15:29

switch defaults button to reset button

6bc04c9

remove Defaults from the other lang files

45afbad

update translations-needed

9ca5fa3

brandonocasey approved these changes Feb 16, 2017

View reviewed changes

brandonocasey added the confirmed label Feb 21, 2017

gkatsev merged commit 0d0dea4 into master Feb 21, 2017

gkatsev deleted the caption-settings-modal branch February 21, 2017 20:58

josephshanak mentioned this pull request Jan 12, 2021

Reset button text repeated by screenreader / assistive technology #7036

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: fix accessibility of the captions setting dialog #4050

feat: fix accessibility of the captions setting dialog #4050

gkatsev commented Feb 9, 2017 •

edited

Loading

gkatsev commented Feb 9, 2017

OwenEdwards commented Feb 9, 2017

gkatsev commented Feb 9, 2017

OwenEdwards commented Feb 9, 2017

gkatsev commented Feb 9, 2017

gkatsev commented Feb 10, 2017

gkatsev commented Feb 10, 2017

OwenEdwards left a comment •

edited

Loading

OwenEdwards Feb 11, 2017

gkatsev Feb 13, 2017

OwenEdwards Feb 11, 2017

gkatsev Feb 13, 2017

misteroneill left a comment

gkatsev commented Feb 14, 2017

gkatsev Feb 15, 2017

OwenEdwards Feb 15, 2017

gkatsev Feb 16, 2017

OwenEdwards Feb 15, 2017

gkatsev Feb 16, 2017

OwenEdwards Feb 16, 2017

brandonocasey Feb 16, 2017

misteroneill Feb 16, 2017

gkatsev Feb 16, 2017

brandonocasey Feb 16, 2017

gkatsev Feb 16, 2017

brandonocasey Feb 16, 2017

gkatsev Feb 16, 2017

feat: fix accessibility of the captions setting dialog #4050

feat: fix accessibility of the captions setting dialog #4050

Conversation

gkatsev commented Feb 9, 2017 • edited Loading

gkatsev commented Feb 9, 2017

OwenEdwards commented Feb 9, 2017

gkatsev commented Feb 9, 2017

OwenEdwards commented Feb 9, 2017

gkatsev commented Feb 9, 2017

gkatsev commented Feb 10, 2017

gkatsev commented Feb 10, 2017

OwenEdwards left a comment • 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

misteroneill left a comment

Choose a reason for hiding this comment

gkatsev commented Feb 14, 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

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

gkatsev commented Feb 9, 2017 •

edited

Loading

OwenEdwards left a comment •

edited

Loading