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: Remove the "command" shortcut #3285

Merged
merged 4 commits into from
Nov 2, 2017

Conversation

youknowriad
Copy link
Contributor

The "command" shortcut to navigate to the editor's toolbar has proven to be technically difficult to implement properly without edge-cases and also have side effects. This PR removes it in favor of the alt+f10 alternative. See #2960 (comment) and #3183

If you can think of another simple shortcut, I'd be happy to add.
This also refactors the navigation to use the KeyboardShortcuts component.

Testing instructions

  • Check that alt+f10 moves the focus to the header's toolbar

closes #3183

@youknowriad youknowriad added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended labels Nov 1, 2017
@youknowriad youknowriad self-assigned this Nov 1, 2017
@codecov
Copy link

codecov bot commented Nov 1, 2017

Codecov Report

Merging #3285 into master will increase coverage by 0.15%.
The diff coverage is 37.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3285      +/-   ##
==========================================
+ Coverage   31.02%   31.18%   +0.15%     
==========================================
  Files         232      232              
  Lines        6518     6504      -14     
  Branches     1163     1159       -4     
==========================================
+ Hits         2022     2028       +6     
+ Misses       3770     3755      -15     
+ Partials      726      721       -5
Impacted Files Coverage Δ
editor/utils/dom.js 15.2% <ø> (+0.12%) ⬆️
editor/header/header-toolbar/index.js 0% <0%> (ø) ⬆️
components/keyboard-shortcuts/index.js 85.71% <100%> (+85.71%) ⬆️

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 ce7870c...13a54ed. Read the comment docs.

@jasmussen
Copy link
Contributor

I can confirm that this works. In my case, I have the Mac keyboard set to use function keys instead of F keys, so I had to press Alt + Fn + F10, which is a super awkward key combination. But it works, which also means it works when I set MacOS to use the F keys instead of the function keys. It's also a standard, so this is a good baseline for the feature.

I understand the removal of the ⌘ key because of the technical issues it introduces. I respect that. Is there a way we can have a different command key we can use for this, in addition to Alt F10?

To be clear, if yes, we can have multiple keyboard shortcuts, totally fine to explore that in a separate PR.

@youknowriad
Copy link
Contributor Author

yes, we can have multiple keyboard shortcuts, totally fine to explore that in a separate PR.

Yes and I'd be happy to add this to the current PR, I just don't know which shortcut to use.

@jasmussen
Copy link
Contributor

Brainstorm:

  • ⌘ + Shift — would this be less conflicting than just alone?
  • Alt + ⌘ — similar to above
  • ⌘ ⌘ — that is, ⌘ followed by ⌘ shortly after

@aduth
Copy link
Member

aduth commented Nov 1, 2017

the removal of the ⌘ key because of the technical issues it introduces

To clarify, the issues I experienced in #2960 (comment) were not technical in nature.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Code-wise, 👍

Alt+F10 can be awkward on MacBook Pro with Touch Display since... there are no function keys 😄

@youknowriad youknowriad merged commit 7c1637b into master Nov 2, 2017
@youknowriad
Copy link
Contributor Author

Let's add another shortcut once we settle on a new one.

@youknowriad youknowriad deleted the update/toolbar-keyboard-shortcut branch November 2, 2017 10:46
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). [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lost focus on navigating across words with CTRL and arrow keys
4 participants