Skip to content
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: Adding Keyboard Shorcuts to navigate the editor regions #3084

Merged
merged 8 commits into from
Oct 27, 2017

Conversation

youknowriad
Copy link
Contributor

closes #467

This PR adds keyboard shortcuts to navigate the aria regions ala slack.

For now, I came up with random shortcuts (ctrl+r and ctrl+shift+r), let me know which one you think I should use.

@youknowriad youknowriad added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Oct 20, 2017
@youknowriad youknowriad self-assigned this Oct 20, 2017
@jasmussen
Copy link
Contributor

jasmussen commented Oct 20, 2017

Totally love this! Very very nice work. Finding a good standardized shortcut for this is going to be important, especially given the backtick discussion in last weeks meeting.

I like the focus style for the region — is it kosher to do something like this? https://codepen.io/joen/pen/Nvjror — that is, a big flashing focus style that disappears?

I don't know if it's intentional — I assume it is, but the shortcut doesn't work if you have focus inside a text block.

Nice work!

@codecov
Copy link

codecov bot commented Oct 20, 2017

Codecov Report

Merging #3084 into master will decrease coverage by 0.67%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3084      +/-   ##
==========================================
- Coverage   31.54%   30.86%   -0.68%     
==========================================
  Files         221      222       +1     
  Lines        6311     6450     +139     
  Branches     1121     1158      +37     
==========================================
  Hits         1991     1991              
- Misses       3632     3734     +102     
- Partials      688      725      +37
Impacted Files Coverage Δ
editor/sidebar/index.js 0% <ø> (ø) ⬆️
editor/modes/text-editor/index.js 0% <ø> (ø) ⬆️
editor/modes/visual-editor/index.js 0% <ø> (ø) ⬆️
editor/header/index.js 0% <ø> (ø) ⬆️
components/higher-order/navigate-regions/index.js 0% <0%> (ø)
editor/layout/index.js 0% <0%> (ø) ⬆️
editor/modes/visual-editor/block.js 0% <0%> (ø) ⬆️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️

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 efa08a0...e893cb0. Read the comment docs.

@afercia
Copy link
Contributor

afercia commented Oct 20, 2017

Love to see this in action 😄 Thanks!
(and started considering to propose this for the WordPress admin too)

Shortcut: yee we should find a good one. I'd love to use G as Gutenberg ;) But I guess we should use a combination with a non a-z 0-9 key. @joedolson help please.

@jasmussen no objections to experiment the flashing focus limited to this case. I'd like to see this merged soon and ready for testing. We can always review later if necessary.

I don't know if it's intentional — I assume it is, but the shortcut doesn't work if you have focus inside a text block.

Noticed that and I understand that's maybe a bit tricky to implement. However, if the idea of the blocks "Navigation mode" and "Edit mode" discussed during last SLack chat will be implemented, then makes sense to me having these shortcuts working just in "Navigation mode".

Instead, when editing and focus is inside a block, the process would be:
press Escape > exit the edited block and goes back to "Navigation mode" > shortcuts work again
Thoughts?

Aside: we should document all this in a future Gutenberg help guide.

@mtias
Copy link
Member

mtias commented Oct 20, 2017

Aside: we should document all this in a future Gutenberg help guide.

Good call, we should start writing these sooner rather than later. They could even be extracted to be part of the "new user experience" cc @karmatosed

<div className="editor-layout__editor">
{ mode === 'text' && <TextEditor /> }
{ mode === 'visual' && <VisualEditor /> }
class Layout extends Component {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the previous simple component, wonder if we should convert the region-focus into a HoC that could be used in different editor layouts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think this should be a generic component in the components module? It looks like something that could be useful even outside an editor context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in daa9cf3

@jasmussen
Copy link
Contributor

Shortcut: yee we should find a good one. I'd love to use G as Gutenberg ;) But I guess we should use a combination with a non a-z 0-9 key.

Question: can we use both the standardized backtick shortcut for keyboards that support this, and a better one for us europeans? (perhaps Ctrl+$, as $ is right above tab)? The idea being that if backtick is a standard for this, good to embrace it for the majority of the world, with enhancements for the rest of us?

no objections to experiment the flashing focus limited to this case. I'd like to see this merged soon and ready for testing. We can always review later if necessary.

Cool, Riad can we try the animated focus style from that codepen I linked? Let me know if you'd like me to implement it. It uses $blue-medium-500.

Good call, we should start writing these sooner rather than later. They could even be extracted to be part of the "new user experience" cc @karmatosed

Would a good start be to implement the keyboard shortcuts cheatsheet (invoked by for example Shift + ?) as ticketed in #2980 be a good start?

I don't know how far we can go with the componentization thing, but it would be really nice if, when a shortcut is used, the tooltip and cheat sheet inclusion create themselves.

<Popover.Slot />
<KeyboardShortcuts shortcuts={ {
'ctrl+r': this.focusNextRegion,
'ctrl+shift+r': this.focusPreviousRegion,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Windows, Ctrl+r refreshes the page, see extended comment on the PR.

@afercia
Copy link
Contributor

afercia commented Oct 21, 2017

Testing on Windows, we've missed that Ctrl+r refreshes the page. The default action should be prevented but...

we should find a better shortcut anyways. Finding a good one that is safe to use across different browsers, platforms, and keyboard layouts is basically impossible.

perhaps Ctrl+$, as $ is right above tab

Not on my Italian keyboard, maybe that's the Danish Mac keyboard 😉. I guess $ is placed differently also in other keyboard layouts.

If we decide these shortcuts should be available only in "navigation mode" (i.e. not when editing a block or when inside any other form field) then I'd consider to use single key shortcuts.

To my understanding, modifiers like Control, Option, Shift, are necessary when inside a form field. Instead, outside of form fields, single keys should work because browsers will ignore single key presses. Thoughts?

Many big apps already use single keys shortcuts, for example Gmail uses j and k to navigate through previous/next messages, and those are pretty common shortcuts. Testing on Gmail:

  • open a message in your inbox
  • press j or k to navigate through messages
  • click the "Compose" button to open the new message modal
  • in the new message form fields, j or k do nothing of course
  • press Escape to close the modal
  • j and k work again

Maybe replicating this interaction would make sense also in Gutenberg? As in, when editing a block, press "Escape" exits edit mode and goes back to navigation mode, then the shortcuts work again. Note: it already works this way.

WordPress already uses single key shortcuts for the Comments screen: https://codex.wordpress.org/Keyboard_Shortcuts#Keyboard_Shortcuts_for_Comments

We don't have to worry about screen readers because they already have their mechanism to jump through landmark regions. Windows screen readers use "browse mode" and "forms mode". When in "browse mode" the Gutenberg shortcuts won't do anything because screen readers intercepts keypresses, so keypresses are not passed to the browser. This is not a problem because, for example, NVDA and JAWS already have their shortcuts:

NVDA list of single key shortcuts:
https://www.nvaccess.org/files/nvda/documentation/userGuide.html#toc41

d: landmark

JAWS list of single key shortcuts:
https://www.freedomscientific.com/Training/Surfs-Up/Quick_Keys.htm

Region R

I'd propose to try j as in "Jump" because it's easy to remember.

Note: the new meta boxes expanding panel is not included in an ARIA landmark region. Please consider, also for future developments, that all content in a web page should be inside a landmark region. Yes, it's inside the WordPress "main" landmark but the point is if this content section is important enough to be a navigable region. Should we make it a landmark adding role="region" aria-label="some label"? Should it be focusable and included in the shortcuts loop?
Want me to open a separate issue?

@jasmussen
Copy link
Contributor

jasmussen commented Oct 21, 2017

I love direct shortcuts like j/k, I'd wager they'd be really popular if we were able to add a bunch of those.

However I've seen the Ctrl+` shortcut used as a standard to navigate between views in an application in many places. MacOS has a ⌘+` shortcut for switching between windows in an application (i.e. if you have two Safari windows open, ⌘+` switches between those two, because for whatever dumb reason the Mac designers thought Alt + Tab should flip between only applications, not windows).

And so in my googling for european alternatives to the backtick, I realized that the backtick on US keyboards sits right above the tab button. That's where I have the $ key. And I figured ... hmmm... could they have? And sure enough, Ctrl+$ moves focus from region to region in Slack.

Andrea can you test whether Ctrl + the key right above your tab button, below Esc switches between regions in Slack? If yes, then that's how they did it — they chose the key by geography. And if it works for you — should we consider doing the same?

@mtias
Copy link
Member

mtias commented Oct 23, 2017

In my Spanish keyboard it works and is represented as:

image

@youknowriad
Copy link
Contributor Author

Wondering how do we match a keyboard position regardless of the layout. Is this the same keycode even if it's a different key? In my french keyboard it's @ and it works in slack.

If I try in this website http://keycode.info/ it gives me a keycode of 192. Are you seeing the same keycode?

@jasmussen
Copy link
Contributor

I would guess that we have to put together an array of keycodes for this character, based on whatever info we can glean.

@afercia
Copy link
Contributor

afercia commented Oct 23, 2017

Andrea can you test whether Ctrl + the key right above your tab button, below Esc switches between regions in Slack?

I'm afraid Slack implemented an alternate shortcuts only for the languages Slack is translated in. Spanish, French, and German. On an Italian keyboard layout, the key right above your tab button, below Esc is "Back slash" and does nothing. The Slack help keeps showing me Ctrl + backtick.

Switching to French does show me: Ctrl + @
Switching to German does show me: Ctrl + ^
Wasn't able to get the Spanish shortcut though.

I realized that the backtick on US keyboards sits right above the tab button

Not on all US keyboards, apparently. On my mac US keyboard is right above Ctrl:

screen shot 2017-10-23 at 16 47 47

Id recommend to keep it simple 😉

@mtias
Copy link
Member

mtias commented Oct 24, 2017

@youknowriad I do get 192 for º.

@youknowriad
Copy link
Contributor Author

Should we go with keycode 192 whatever that is on each keyboard layout :)?

I think we won't be able to use the KeyboardShortcuts component and mousetrap to match this but not a big issue.

@jasmussen
Copy link
Contributor

If we are able to go with multiple shortcuts for the same, it seems we might be able to make the most people happy by picking a shortcut that...

  1. sits at a convenient location on the keyboard
  2. is a standard

While 2 isn't universally true, until we find a single keyboard shortcut that's actually a standard, the closest is what we have here with ⌘+` that Slack uses. It seems worth trying this.

We could then supplement it, or replace it, with a simpler shortcut that doesn't change between keyboards, if we can find a good idea for what that should be.

@youknowriad
Copy link
Contributor Author

youknowriad commented Oct 26, 2017

Interestingly I've used ⌘+` as a shortcut using the mousetrap lib in it's working properly with ⌘+@ in my keyboard. Can y'all try now?

@youknowriad
Copy link
Contributor Author

And it's a flashing focus now :)

@jasmussen
Copy link
Contributor

Ctrl + $ (the key above my tab) isn't working for me, and Ctrl + @ doesn't work for me as @ requires an Alt for me.

@youknowriad
Copy link
Contributor Author

youknowriad commented Oct 26, 2017

🤔 Can you try this String.fromCharCode(192) on your console and tell me what it gives you?

@jasmussen
Copy link
Contributor

screen shot 2017-10-26 at 10 33 44

So that's À

@youknowriad
Copy link
Contributor Author

youknowriad commented Oct 26, 2017

@jasmussen Oh you're on a mac right? did you try ⌘ + $ instead?

@jasmussen
Copy link
Contributor

Oh interesting. I didn't try that, and I have that mapped to something else. But Macs also have Ctrl, and slack on Mac also uses Ctrl for this. I think CMD + backtick on a Mac is supposed to flip between windows of the same app

@youknowriad
Copy link
Contributor Author

Replaced command by ctrl, does it work now?

@youknowriad
Copy link
Contributor Author

@jasmussen I suspect the cropping to be fixed once we land this commit a0fdfc7 from #2998

@jasmussen
Copy link
Contributor

Then 👍 👍 for the feature from me. I'll let others chime in on the shortcuts and implementation, but I dig this implementation.



@mixin focus_flash {
animation: focus_flash .4s;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are spaces mixed in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 👍

@@ -60,4 +60,4 @@ export default connect(
notices: getNotices( state ),
} ),
{ removeNotice }
)( Layout );
)( navigateRegions( Layout ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much better, thanks.

@youknowriad
Copy link
Contributor Author

Rebased this, it should be ready to go

@mtias
Copy link
Member

mtias commented Oct 27, 2017

The cropping on the main area doesn't to be fixed after the rebase? Testing this, I feel that the fact it disappears so quickly doesn't communicate what this does very well—almost feels like a glitch.

@youknowriad
Copy link
Contributor Author

Ok per @mtias suggestion, I dropped the focus animation for now

@mtias
Copy link
Member

mtias commented Oct 27, 2017

Feels much better to me like this, though now it exposes the crop issue more clearly when you scroll:

image

@youknowriad
Copy link
Contributor Author

Good catch, it should be fixed now

@mtias
Copy link
Member

mtias commented Oct 27, 2017

Looks great to me.

@youknowriad youknowriad merged commit 5b05c5d into master Oct 27, 2017
@youknowriad youknowriad deleted the add/region-keyboard-shortcuts branch October 27, 2017 14:56
@afercia
Copy link
Contributor

afercia commented Oct 28, 2017

I've tested a bit on a Windows laptop with the Italian keyboard. There's no physical key for the backtick. On Italian keyboards with a numeric pad, the only way to produce a backtick is pressing Alt+96 on the numeric pad. Many laptops don't even have a numeric pad...

Pressing the key below Escape (in my case it's a back slash) does nothing.
Only when I switch the keyboard layout to English the key below Escape works as backtick and the regions navigation works.

Will open a new issue.

@afercia
Copy link
Contributor

afercia commented Mar 6, 2018

Just had confirmation from @rianrietveld the backtick doesn't work on the Dutch keyboard layout (and as said previously, probably on many other layouts). This makes #3218 really necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add keyboard shortcuts à la Slack to jump through the main UI sections
4 participants