Fix accessibility for the Keyboard Shortcuts help dialog

[azaozz]

TinyMCE adds role=application to the contained div of all dialogs. This works when they contain forms however this dialog contains only text.

[azaozz]

This is hard-coded in TinyMCE. Maybe can be fixed by changing role="application" to role="document" from JS on opening the dialog, or we can set role="document" on the inner wrapper.

See ​https://wordpress.slack.com/archives/core-editor/p1437135778000702.

[afercia]

Tested the patch with Firefox+NVDA and it works nicely. Now when using a screen reader, it is possible to use arrows or other screen readers keystrokes to read the modal dialog content.

There are some more issues though, most notably the initial focus given to the Close button at the bottom of the modal dialog. Since it's the only element TinyMCE gives focus to, users can't use Tab to move focus to other elements. Also, users won't have a clue that the actual content is "before" the initial focused element so it would be very difficult for them to understand they actually have to navigate backwards to read the content.

To my understanding this would be a bit tricky to solve if we still want to use native TinyMCE modal dialogs. Wondering if there's an easy way to set initial focus before the content.

Other improvements would be:

• the "titles" before each table should be h2 this would allow users to "jump" through content blocks
• for the same reason, consider to add a "title" (h2) to the editor patterns shortcuts too ("Magic patterns"?)
• wondering if we should really use tables, they're a bit confusing when read out by screen readers, especially the rows with 4 columns because in a single row there are 2 row headers th. Anyways, they need a scope="row" attribute, otherwise the default is scope="col"
• Chrome needs a tabindex="0" on the div with overflow: auto otherwise the dive can't receive focus and users won't be able to scroll the content

[azaozz]

All of the above should be fixes in 33031.1.patch.

Replying to afercia:

The shortcuts data is (kind of) tabular, so using tables makes sense. The only other element that makes sense is ​definition/description list.

We don't need to keep the th, especially if they make it more confusing for screen readers. They were used there before so I kept them. Looking at these tables, should probably use th for the Letter | Action | Letter | Action row.

[afercia]

Tested a bit the patch and yes it improves things a bit. There's still a big issue though. Had a look at what 'tinymce.ui.KeyboardNavigation' does for TinyMCE "containers" and it tries to implement some sort of proprietary focus management. So what happens that impact accessibility? As soon as you press Tab or the Down Arrow key, as screen reader users normally do to read content, focus is moved to the Close "button" and can't be moved further. You're basically "trapped" on the Close button.

'tinymce.ui.KeyboardNavigation' works (more or less) for TinyMCE dialogs that contain TinyMCE controls or what TinyMCE considers focusable elements which are input, textarea and elements with an ARIA role of button|menuitem|checkbox|tab|menuitemcheckbox|option|gridcell.
It's something tied to what TinyMCE needs, but I fear it's not what we need here.

To my understanding, if we still want to use a TinyMCE modal dialog, we should basically remove the TinyMCE focus management and implement our own. This would mean we should rebuild many things inside this modal. Does it worth it? Wouldn't be easier to just don't use a TinyMCE modal dialog in the first place?

For an example of what should be removed and rebuilt in order to have a decent level of accessibility, see the attached patch. That's not to say we should do this :) It's basically hacking TinyMCE, with all the implied consequences, and maintenance issues.

[afercia]

Hacking TinyMCE

[jorbin]

Is this something that should go into 4.3? Is it a regression or should we put this in future release?

[azaozz]

Not sure if this is a regression, but some things have changed. Need to get it working as well as possible in 4.3.

Wouldn't be easier to just don't use a TinyMCE modal dialog in the first place?

Then we will have to build all from scratch without using external JS libraries (jQuery) so it stays fully compatible. This can be done but... not sure it's worth it.

As soon as you press Tab or the Down Arrow key, as screen reader users normally do to read content, focus is moved to the Close "button" and can't be moved further. You're basically "trapped" on the Close button.

We can try to disable capturing of arrow keys, Home, Page Up, Page Down, etc. from JS and let them work "normally".

Tab key (usually) only moves the focus between "focusable" elements. The only focusable element in that modal is the Close button. The X at the top/right is not focusable in any MCE dialog (this may be fixed/changed upstream). Even if it was focusable, the tab key will still be very limited. Don't think this is a defect, there just aren't any other controls there.

[azaozz]

In 33031.4.patch: same as 33031.2.patch plus attempt to prevent capturing of the arrow keys and "scrolling" keys.

@afercia can you see if that works better? Planning to commit it tomorrow (Sunday) or Monday.

[afercia]

Tested a bit the patch and yes it's an improvement. I'm all for having this in 4.3, there are some outstanding issues though. To recap:

• using a screen reader (and the keyboard of course): users can now use the up arrow key to move back to the scrollable div and all the other keystrokes for reading content (h2, tables, arrows) seem to work correctly

Outstanding issues:

• using a keyboard only: when focus is on the Close button users can't move focus back and scroll the content, basically users can't read the content, the only chance is to close the modal and open it again
• we should consider to add some descriptive screen reader text for the editor Text Patterns characters: symbols like * - > # get announced in a weird way or they don't get announced at all. Also 1. 1) gets read out as "one one" most of the times, this may vary depending on screen readers punctuation and verbosity settings
• the last <h2> should be just a <p> since headings are useful to mark the beginning of a content section

The only focusable element in that modal is the Close button.

Well, we have just made the scrollable div focusable so there are two focusable elements. It's TinyMCE that has its own opinions about what is focusable and what is not :) Maybe, TinyMCE should include in its focusable elements index also all the natively focusable elements that may be used in a TinyMCE "container".

Anyway, I wouldn't say it's a regression. What tinymce.ui.KeyboardNavigation does is strictly tied to what TinyMCE needs and assumes the presence of TinyMCE "controls". It just doesn't fit the case when the modal dialog content is just... content. For reference, this worked slightly better in WordPress 3.8 because the modal used a tabbed interface and "tabs" are TinyMCE focusable elements. Also, the Close button was inside the scrollable div so it was always possible to scroll the content:

[afercia]

Let the last paragraph be a paragraph

[azaozz]

...when focus is on the Close button users can't move focus back and scroll the content

Adding role="tab" to the scrollable wrapper seems to fix this. Not sure if that breaks something else for screen readers.

symbols like * - > # get announced in a weird way or they don't get announced at all

Hmm, thought screen readers would "understand" the <kbd> tag, seems they don't... Would it help if we wrap these in a <code> (although semantically <kbd> is the proper tag there). Is there anything other we can add to make screen readers "read" these? Perhaps some aria attr?

We can also add spans with screen-reader-text, not sure if that won't make it messier.

Going to commit it with the above changes so it can be tested easier.

[azaozz]

In 33429:

TinyMCE: fix accessibility for the keyboard shortcuts help dialog.
Props afercia, azaozz. Fixes #33031.

[afercia]

Replying to azaozz:

Adding role="tab" to the scrollable wrapper seems to fix this. Not sure if that breaks something else for screen readers.

Yup, adding ​any of the roles listed here would make an element be included in the focusable elements index built by TinyMCE. By the way "role=tab" also adds a semantic meaning and screen readers would announce a tabbed interface, expecting more than one tab. NVDA announces "tab selected one of one".
Also, noticed NVDA goes in "forms mode" when it finds a tabbed interface and this is potentially confusing for users, I'd recommend to remove the tab role.

About the editor patterns characters:

Is there anything other we can add to make screen readers "read" these? Perhaps some aria attr?

I'm afraid screen readers are not so sophisticated :) Some characters are interpreted as punctuation and depending on the screen reader used and verbosity settings they may be ignored.
Even when reading content "by character", for example using left and right arrows in NVDA they get read out in a way that's a bit difficult to understand:

star space dash
one dot space one right parens
greater
number number
number number number
number number number number
number number number number number
number number number number number number

[azaozz]

In 33430:

TinyMCE: remove role=tab from the keyboard shortcuts help dialog.
See #33031.