-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Accessibility: Keyboard shortcuts to navigate to/from/in the block's toolbar #2960
Conversation
editor/block-toolbar/index.js
Outdated
|
||
onKeyUp( event ) { | ||
// Is there a better way to focus the selected block | ||
const selectedBlock = document.querySelector( '.editor-visual-editor__block.is-selected' ); |
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.
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.
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.
Hm, but this also fires on every keyup?
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Hm, I'm not sure if I like using |
(Note: |
A more convincing reason to not do this is that you'd move the focus away while someone is typing a character. |
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/) |
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. |
Completely love this, thanks for working on it. It seems to work exactly as you describe. Two things:
|
@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
|
@youknowriad I had more something like this in mind: https://github.com/WordPress/gutenberg/compare/try/toolbar-arrow-navigation-alt |
(That's a WIP, focus should then be limited to that wrapper, until you ESC or click outside of it.) |
@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?
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.
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. |
If by "meta key" you're referring to Cmd or Control as recommended in #552, then 👍 👍 |
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. |
3a573dc
to
25817c9
Compare
Fixed the glitch by using @iseulde 's technique, it should be ok now. |
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: |
@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. |
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? |
Anything else I should address here? I feel this already works as suggested |
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: |
@ephox-mogran Thanks for the great feedback. I share most of these concerns.
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.
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) |
@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.
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.
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. |
5b8aec3
to
6f12750
Compare
PR updated with looping, using onKeyDown and avoid global event listeners when possible. (Reused the NavigableMenu component) |
Looks good :) |
editor/block-toolbar/index.js
Outdated
function FirstChild( { children } ) { | ||
const childrenArray = Children.toArray( children ); | ||
return childrenArray[ 0 ] || null; | ||
} | ||
|
||
function isMac() { |
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.
It looks like it could be moved to the file with browser's utils or should we wait until it's used somewhere else?
@@ -8,3 +8,5 @@ export const UP = 38; | |||
export const RIGHT = 39; | |||
export const DOWN = 40; | |||
export const DELETE = 46; | |||
|
|||
export const F10 = 121; |
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.
Nice, we should have all keycodes here to make code easier to read 👍
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 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.
Acknowledged the |
See #3003 |
@@ -86,3 +86,12 @@ export function placeCaretAtEdge( container, start = false ) { | |||
sel.addRange( range ); | |||
container.focus(); | |||
} | |||
|
|||
/** |
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.
Thanks 🙇
@youknowriad I'm getting the following warning now: |
@iseulde Good catch, forgot to exclude this prop 👍 |
|
||
function FirstChild( { children } ) { | ||
const childrenArray = Children.toArray( children ); | ||
return childrenArray[ 0 ] || null; | ||
} | ||
|
||
function metaKeyPressed( event ) { |
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.
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
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 tried but it failed to trigger on "command" key only, not sure why
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 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.
@@ -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'; |
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.
While it uses DOM properties to determine, isMac
doesn't seem like a DOM-specific utility function.
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...
|
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. |
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. |
We could revisit #3031 if we opt to drop the modifier shortcut. |
closes #552
This PR adds keyboard shortcuts to navigate
alt
key)esc
key)I've used
alt
because it's common to navigate to menus usingalt
in desktop applications. Also, avoid the hurdle of dealing with specific keys by OS (ctrl and command).Caveats