Accessibility: Keyboard shortcuts to navigate to/from/in the block's toolbar #2960

youknowriad · 2017-10-10T15:34:08Z

closes #552

This PR adds keyboard shortcuts to navigate

from a block to its toolbar (using alt key)
in the toolbar using arrows
back to the block (using esc key)

I've used alt because it's common to navigate to menus using alt in desktop applications. Also, avoid the hurdle of dealing with specific keys by OS (ctrl and command).

Caveats

The block's selector is hard coded in the component :(
We need to stop propagation in the onKeyDown event to avoid messing up with the WritingFlow

youknowriad · 2017-10-10T15:40:26Z

editor/block-toolbar/index.js

+
+	onKeyUp( event ) {
+		// Is there a better way to focus the selected block
+		const selectedBlock = document.querySelector( '.editor-visual-editor__block.is-selected' );


Tried avoiding the selector by relying on state, but we do not have a reliable way to focus a block with the state only. The focus block action doesn't refocus if it's already selected.

Hm, but this also fires on every keyup?

codecov · 2017-10-10T15:43:01Z

Codecov Report

Merging #2960 into master will decrease coverage by 0.37%.
The diff coverage is 2.94%.

@@            Coverage Diff             @@
##           master    #2960      +/-   ##
==========================================
- Coverage   33.52%   33.15%   -0.38%     
==========================================
  Files         197      197              
  Lines        5754     5849      +95     
  Branches     1017     1041      +24     
==========================================
+ Hits         1929     1939      +10     
- Misses       3230     3293      +63     
- Partials      595      617      +22

Impacted Files	Coverage Δ
editor/utils/dom.js	`39.53% <0%> (-0.95%)`	⬇️
components/navigable-menu/index.js	`28% <0%> (ø)`	⬆️
editor/block-toolbar/index.js	`0% <0%> (ø)`	⬆️
utils/keycodes.js	`100% <100%> (ø)`	⬆️
blocks/library/gallery/block.js	`9.75% <0%> (-1.36%)`	⬇️
blocks/library/table/table-block.js	`7.4% <0%> (-0.6%)`	⬇️
blocks/library/image/block.js	`0% <0%> (ø)`	⬆️
editor/block-settings-menu/content.js	`0% <0%> (ø)`	⬆️
editor/block-settings-menu/index.js	`0% <0%> (ø)`	⬆️
blocks/library/audio/index.js	`14.63% <0%> (+2.51%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d36dc1b...3522271. Read the comment docs.

ellatrix · 2017-10-10T18:28:00Z

Hm, I'm not sure if I like using alt as much as ctrl/cmd. The former means the alternative input of a key, similar to shift. The latter is for commands, so it might make more sense to use that, and it's a key you'd need for shortcutting the commands in the toolbar, e.g. cmd+B.

ellatrix · 2017-10-10T18:29:58Z

(Note: alt can also be used for an alternate command, but still in combination with the command key.)

ellatrix · 2017-10-10T18:31:21Z

A more convincing reason to not do this is that you'd move the focus away while someone is typing a character.

ephox-mogran · 2017-10-11T06:04:10Z

Typically, the keyboard shortcut to use for focusing the toolbar is F10 + some meta keys. Tinymce uses Alt + F10 (https://www.tinymce.com/docs/advanced/keyboard-shortcuts/) and CKEditor uses Alt + F10 (https://docs.ckeditor.com/ckeditor4/docs/#!/guide/dev_shortcuts). I can't find any ARIA recommendations ( @afercia any ideas?) that say this is required / expected, but I can find the one that says that context menu is generally Shift + F10 (Section 2.1 of https://www.w3.org/TR/wai-aria-practices-1.1/)

ellatrix · 2017-10-11T06:59:33Z

Yes, we should still add Alt + F10 , but this is quite hard to remember (and a bit strange Imo). I thought a single cmd/ctrl press would be ideal, because it's not used for anything, and you're literally accessing controls/commands. :) In addition, you'd need the key anyway to shortcut them. This could also go nicely together with displaying the letter than goes with the combination when it is pressed: http://littlebigdetails.com/post/114672131085/mailbox-holding-the-cmd-key-on-mac-displays-each. Maybe we can do something similar spoken when requested.

jasmussen · 2017-10-11T07:16:00Z

Completely love this, thanks for working on it. It seems to work exactly as you describe.

Two things:

We should make sure the keyboard shortcuts at least match what works in WordPress today (sounds like Alt + F10), but I agree that it would be nice with a much simpler and predictable way to do this. Ella opened Pressing cmd/ctrl as a way to navigate into block toolbars #552 a while back, which seems to have some concencus. Perhaps we can address that ticket here, and have both the old and new keyboard shortcuts apply?
I noticed this previously, and it's almost certainly unrelated to this PR, but it seems to me like navigating between buttons in the toolbar (whether through tabs or arrowkeys) requires TWO stops. Any idea what this might be?

gziolo · 2017-10-11T07:18:52Z

Point (2) from Joen said above. esc key kind of works, but I couldn't continue typing in the block.

afercia · 2017-10-11T07:57:47Z

@ephox-mogran and all, see #552 (comment) in the ARIA Authoring Practices it's in the Menu/Menu bar keyboard interaction "note" (the big green box).

However, while Alt-F10 is recommended, I'm not sure I've ever got why it is recommended :) Will self-quote my comment from #552

However, I've never got the reasoning behind that. Alt + F10 doesn't seem the most discoverable and easy to remember shortcut. I'd vote for keeping Alt + F10 for users that are used to that and introduce cmd/ctrl as a new, easier one.

ellatrix · 2017-10-11T08:10:04Z

@youknowriad I had more something like this in mind: https://github.com/WordPress/gutenberg/compare/try/toolbar-arrow-navigation-alt
Where there's a wrapper around all block controls, which are navigated separately. I think this is what @afercia recommended?

ellatrix · 2017-10-11T08:11:01Z

(That's a WIP, focus should then be limited to that wrapper, until you ESC or click outside of it.)

youknowriad · 2017-10-11T09:32:00Z

@iseulde I'm not sure I understand everything here but these controls has been to a separate BlockToolbar component which a wrapper around them.

So what are the actionable items here?

Use meta key instead of alt
Also support alt + f10

esc key kind of works, but I couldn't continue typing in the block.

This is somehow unrelated, we need to rework how we apply the focus prop to the block container and the block itself. (And ensure it's separated from the isSelected flag). It's a difficult task to make this work seemlesly between the different blocks. I think this should be addressed in its own PR.

it seems to me like navigating between buttons in the toolbar (whether through tabs or arrowkeys) requires TWO stops

I don't know yet, same we need to investigate separately but it behaves differently in Firefox, where you tab only once. Seems related to the diffeerence in focusable elements between the browsers.

jasmussen · 2017-10-11T09:36:45Z

Use meta key instead of alt

If by "meta key" you're referring to Cmd or Control as recommended in #552, then 👍 👍

youknowriad · 2017-10-11T10:02:36Z

Updated with the last proposals.

One glitch to note, it's not easy to know if the user only clicked on a "meta" key without simultaneously clicking on another key. For example, clicking command + g (or any random key) may trigger the behavior to move to the toolbar.

youknowriad · 2017-10-11T10:22:12Z

Fixed the glitch by using @iseulde 's technique, it should be ok now.

afercia · 2017-10-11T11:04:24Z

it seems to me like navigating between buttons in the toolbar (whether through tabs or arrowkeys) requires TWO stops

I can confirm this happening in Chrome and Safari 11 for all the buttons also the ones in the top toolbar. Seems unrelated to the SVG icons because it happens also when I remove the icon and put just some text in the buttons:

youknowriad · 2017-10-11T11:10:37Z

@afercia I tracked this a bit and it's being caused by the different tabIndex Value between the different browsers (this line https://github.com/WordPress/gutenberg/blob/master/utils/focus/tabbable.js#L13) I don't have a fix yet, but this should be fixed separately.

ellatrix · 2017-10-11T11:11:20Z

I'm not sure I understand everything here but these controls has been to a separate BlockToolbar component which a wrapper around them.

Not sure I understand this completely, but if you're asking why to put a wrapper around all controls: the idea is that none of the controls around the blocks (block movers, toolbars, right side menu) should be tabbable as part of the main flow, but to let them all be accessed by a special key (in this case ctrl/cmd or alt+f10). When in that flow, you can use the tab or arrow keys to navigate all the block's surrounding controls and buttons, in a circle. To exit that flow, escape should put the focus back where it was earlier (and of course you can also use the mouse to exit). Does that sound correct, @afercia?

youknowriad · 2017-10-11T11:25:26Z

Anything else I should address here? I feel this already works as suggested

jasmussen · 2017-10-11T11:32:15Z

This is very possibly not related to this PR, so probably fine to fix separately, but in case it's related, I'm seeing some issues around using the spacebar to activate buttons on the toolbar. Specifically, it both adds a space, and activates the button, when it should only do the latter. Do space to toggle enough times and the toolbar disappears, and will no longer reappear with the ⌘ shortcut:

youknowriad · 2017-10-13T06:56:33Z

@ephox-mogran Thanks for the great feedback. I share most of these concerns.

listeners being added directly to the document

As you notes, this is needed for handling the "Enter the toolbar" shortcut. About the other shortcuts, I'm hoping to land #2989 which takes the approach you suggest and reuse this component here.

onKeyDown
no global event listeners
looping

c) extra complicated logic to support the "meta" key shortcut.

I see no other option if we want to keep the "meta" key.

--

I think we are on the same page since #2989 would correspond to your second option, I don't think we should do the third option right now, handling focus in state has proven to be very hard to do properly, We need to rethink this entirely at some poing (focus management in state)

afercia · 2017-10-13T07:13:23Z

@ephox-mogran thanks for your impressive analysis 🙂

I share some of your general concerns. Keyboard events on components are tricky because theoretically components can be reused everywhere and events bubble. There have been already cases where events fired on a component were triggering also events on other elements up in the DOM tree. This basically requires to always stop propagation and should be clearly documented as a requirement for devs willing to extend Gutenberg. As I see it, all the events handling is a bit fragile. There's always the risk to break something. Regardless of the option that will be chosen, there's probably the need to establish a strict convention and clearly document it.

e) the arrows don't cycle from the end of the toolbar back to the start of the toolbar (and vice versa). I expected they would, but that might be an incorrect assumption. (@afercia?)

While it's not a requirement (the ARIA Authoring Practices say that's "optional"), personally I'd prefer having arrow navigation loop through the items. It already works this way in the drop down menus and for consistency it should work the same way in all the widgets with similar interaction.

For menus, toolbars, etc. I'd prefer keydown because the ability to hold down the arrow key and quickly navigate is very important, as you pointed out.

I guess another question to do with this is when/if the toolbar becomes docked at the top, can you focus it from any element? Would you be able to focus it from the inserter menu for example, or the move block icon? Is that important?

For screen reader users, there are now ARIA landmarks that help jumping to the main UI sections, see #2380. For keyboard users, the idea discussed in #467 (comment) is to implement something like what Slack does: keyboard shortcuts to move focus to the main UI sections, ideally the same sections marked as landmarks.

…toolbar

youknowriad · 2017-10-13T08:09:08Z

PR updated with looping, using onKeyDown and avoid global event listeners when possible. (Reused the NavigableMenu component)

ephox-mogran · 2017-10-13T09:56:59Z

Looks good :)

gziolo · 2017-10-13T09:55:55Z

editor/block-toolbar/index.js

 function FirstChild( { children } ) {
 	const childrenArray = Children.toArray( children );
 	return childrenArray[ 0 ] || null;
 }

+function isMac() {


It looks like it could be moved to the file with browser's utils or should we wait until it's used somewhere else?

gziolo · 2017-10-13T09:59:37Z

utils/keycodes.js

@@ -8,3 +8,5 @@ export const UP = 38;
 export const RIGHT = 39;
 export const DOWN = 40;
 export const DELETE = 46;
+
+export const F10 = 121;


Nice, we should have all keycodes here to make code easier to read 👍

gziolo

I did some testing with the latest versions of FF, Chrome and Safari. Everything works as advertised. My only concern is that when using esc key inside the paragraph the cursor goes to the beginning of the block. See:

In overall, this is a really nice improvement. So I would say let's merge it and open a follow up for esc key to make it even better.

youknowriad · 2017-10-13T10:46:05Z

Acknowledged the esc is not working perfectly on text nodes, this is not easy to fix without some global refactoring of the focus management. Let's merge and improve later.

afercia · 2017-10-13T11:11:00Z

Acknowledged the esc is not working perfectly on text nodes, this is not easy to fix without some global refactoring of the focus management. Let's merge and improve later.

See #3003

gziolo · 2017-10-13T11:18:27Z

editor/utils/dom.js

@@ -86,3 +86,12 @@ export function placeCaretAtEdge( container, start = false ) {
 	sel.addRange( range );
 	container.focus();
 }
+
+/**


Thanks 🙇

ellatrix · 2017-10-13T12:16:47Z

@youknowriad I'm getting the following warning now: Warning: Received truefor non-boolean attributedeep. If this is expected, cast the value to a string.

youknowriad · 2017-10-13T12:47:18Z

@iseulde Good catch, forgot to exclude this prop 👍

aduth · 2017-10-16T19:10:49Z

editor/block-toolbar/index.js


 function FirstChild( { children } ) {
 	const childrenArray = Children.toArray( children );
 	return childrenArray[ 0 ] || null;
 }

+function metaKeyPressed( event ) {


Would it have been possible to use the KeyboardShortcuts component to manage and normalize these keyboard combinations?

https://github.com/WordPress/gutenberg/tree/master/components/keyboard-shortcuts

I tried but it failed to trigger on "command" key only, not sure why

I tried but it failed to trigger on "command" key only, not sure why

Might have been running into caveat of Mousetrap not firing callbacks while input is focused. See #3031 for workaround.

aduth · 2017-10-16T19:11:15Z

editor/block-toolbar/index.js

@@ -19,19 +20,51 @@ import './style.scss';
 import BlockSwitcher from '../block-switcher';
 import BlockMover from '../block-mover';
 import BlockRightMenu from '../block-settings-menu';
+import { isMac } from '../utils/dom';


While it uses DOM properties to determine, isMac doesn't seem like a DOM-specific utility function.

aduth · 2017-10-31T20:13:51Z

Noting that the choice of using Cmd had me in a bit of a state of confusion earlier today. I think I caught myself in a flow of...

Write some text
Select some text with intention to bold it (Cmd+B)
Hold Cmd key and...
...Decide against bolding ("Changed my mind", or "Actually, let me revise the selection...") and release the Cmd key
Suddenly focus has left the input, and I'm left bewildered about how to return to editing text except by clicking again in the input

youknowriad · 2017-10-31T20:38:36Z

Yes, I'm starting to think wee should drop the "command" shortcut and just stick with "alt+f10". Technically it's not easy to get right and has several side effects.

@afercia @iseulde @jasmussen

ephox-mogran · 2017-10-31T20:42:47Z

Agree 100%. If you absolutely have to have it, then maybe you need to only do it if the timestamp difference of keydown and keyup is very small.

aduth · 2017-10-31T20:55:43Z

We could revisit #3031 if we opt to drop the modifier shortcut.

youknowriad added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Oct 10, 2017

youknowriad self-assigned this Oct 10, 2017

youknowriad requested review from afercia, ellatrix and jasmussen October 10, 2017 15:34

youknowriad commented Oct 10, 2017

View reviewed changes

aduth mentioned this pull request Oct 10, 2017

Try/2031 manual focus #2392

Closed

jasmussen mentioned this pull request Oct 11, 2017

Show keyboard shortcuts in menus and tooltips #2980

Closed

youknowriad force-pushed the try/toolbar-arrow-navigation branch from 3a573dc to 25817c9 Compare October 11, 2017 10:21

jasmussen mentioned this pull request Oct 11, 2017

Try to reduce the visual weight of the block #2983

Closed

youknowriad added 5 commits October 13, 2017 08:36

Accessibility: Keyboard shortcuts to navigate to/from/in the block's …

5fc8f97

…toolbar

Block Toolbar: Replace alt with command and support alt + f10

8c64118

Block Toolbar: Avoid running selectors on eack key

ac7936d

Block Toolbar: Fix typo componentWillUnmount

2a407a0

Block Toolbar: Reuse Navigable Menu instead of custom keyUp handlers

6f12750

youknowriad force-pushed the try/toolbar-arrow-navigation branch from 5b8aec3 to 6f12750 Compare October 13, 2017 08:05

Drop unused keycodes

59259b7

gziolo reviewed Oct 13, 2017

View reviewed changes

gziolo approved these changes Oct 13, 2017

View reviewed changes

Utils: Move the isMac util to the DOM utils

3522271

youknowriad merged commit d8b04b3 into master Oct 13, 2017

youknowriad deleted the try/toolbar-arrow-navigation branch October 13, 2017 10:49

gziolo reviewed Oct 13, 2017

View reviewed changes

aduth reviewed Oct 16, 2017

View reviewed changes

aduth mentioned this pull request Oct 16, 2017

Block Toolbar: Refactor using KeyboardShorcuts component #3031

Closed

This was referenced Oct 17, 2017

Unable to use keyboard shortcut if toolbar is hidden #3035

Closed

Show toolbar on meta key press #873

Closed

youknowriad mentioned this pull request Nov 1, 2017

Accessibility: Remove the "command" shortcut #3285

Merged

Accessibility: Keyboard shortcuts to navigate to/from/in the block's toolbar #2960

Accessibility: Keyboard shortcuts to navigate to/from/in the block's toolbar #2960

Conversation

youknowriad commented Oct 10, 2017

Caveats

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 10, 2017 • edited Loading

Codecov Report

ellatrix commented Oct 10, 2017 • edited Loading

ellatrix commented Oct 10, 2017

ellatrix commented Oct 10, 2017

ephox-mogran commented Oct 11, 2017

ellatrix commented Oct 11, 2017

jasmussen commented Oct 11, 2017

gziolo commented Oct 11, 2017 • edited Loading

afercia commented Oct 11, 2017 • edited Loading

ellatrix commented Oct 11, 2017

ellatrix commented Oct 11, 2017

youknowriad commented Oct 11, 2017

jasmussen commented Oct 11, 2017

youknowriad commented Oct 11, 2017

youknowriad commented Oct 11, 2017

afercia commented Oct 11, 2017

youknowriad commented Oct 11, 2017

ellatrix commented Oct 11, 2017 • edited Loading

youknowriad commented Oct 11, 2017

jasmussen commented Oct 11, 2017

youknowriad commented Oct 13, 2017

afercia commented Oct 13, 2017

youknowriad commented Oct 13, 2017

ephox-mogran commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

youknowriad commented Oct 13, 2017

afercia commented Oct 13, 2017

Choose a reason for hiding this comment

ellatrix commented Oct 13, 2017

youknowriad commented Oct 13, 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

aduth commented Oct 31, 2017

youknowriad commented Oct 31, 2017 • edited Loading

ephox-mogran commented Oct 31, 2017

aduth commented Oct 31, 2017

codecov bot commented Oct 10, 2017 •

edited

Loading

ellatrix commented Oct 10, 2017 •

edited

Loading

gziolo commented Oct 11, 2017 •

edited

Loading

afercia commented Oct 11, 2017 •

edited

Loading

ellatrix commented Oct 11, 2017 •

edited

Loading

youknowriad commented Oct 31, 2017 •

edited

Loading