Add a new keyboard shortcut package #19100

youknowriad · 2019-12-12T12:23:56Z

There's a few ideas in this PR:

Create a new package that handles the keyboard shortcuts in a data store:
- Ultimately this allows us to implement a UI to change the keyboard combination per user.
- Potentially, this also allows third-party shortcuts and shortcut categories.
Create a useShortcut hook as a replacement to the KeyboardShorcuts package but there's no need to pass the actual combination since it's retrieved from the store.
Refactors the BlockEditorKeyboardShortcuts component to use this new package. (Ultimately, all shortcuts should rely on this package).
While refactoring BlockEditorKeyboardShortcuts, I moved some logic we had in components into data actions (DuplicateBlocks, InsertBefore, InsertAfter actions) and also handle the checks in the action itself. Removing selectors calls from components into one of actions can have potential performance boost and also allow easy logic sharing. Related here, I think the BlockActions component should be deprecated and removed in favor of selectors and actions.

Once all the keyboard shortcuts refactored to use this package, the whole modal could be generated without any hard-coded code.

gziolo · 2019-12-12T13:55:04Z

It's a huge PR and I won't have time to review but I applaud the efforts. Great idea 💯

packages/block-editor/src/components/keyboard-shortcuts/index.js

MarcoZehe

I found some misspellings of the word "shortcut" throughout the files and suggested corrections so they don't look weird.

packages/block-editor/src/components/keyboard-shortcuts/index.js

packages/keyboard-shortcuts/package.json

packages/keyboard-shortcuts/src/hooks/use-shortcut.js

packages/keyboard-shortcuts/src/store/selectors.js

youknowriad · 2019-12-17T16:36:23Z

I updated the Modal accordingly.

packages/keyboard-shortcuts/src/store/selectors.js

packages/keyboard-shortcuts/src/store/actions.js

packages/edit-post/src/components/keyboard-shortcut-help-modal/shortcut.js

packages/keyboard-shortcuts/src/hooks/use-shortcut.js

aduth · 2019-12-17T18:28:16Z

packages/keyboard-shortcuts/src/hooks/use-shortcut.js

+
+ shortcutKeys.forEach( ( shortcut ) => {
+ const keys = shortcut.split( '+' );
+ const modifiers = new Set( keys.filter( ( value ) => value.length > 1 ) );


What's the single-character modifier we're filtering here? Comment or externalized/named function could help clarify.

I can't tell. This is ported from KeyboardShortcuts.

It looks like it determines whether a key is a modifier by the length of the string. E.g. if I add a pass a shortcut Shift+Cmd+M, it'll determine that the modifiers are Shift and Cmd because they're not a single character.

That does seem a little bit unusual. Might be safer to check against an array of known modifiers.

It looks like it determines whether a key is a modifier by the length of the string. E.g. if I add a pass a shortcut Shift+Cmd+M, it'll determine that the modifiers are Shift and Cmd because they're not a single character.

That sounds about right.

Might be safer to check against an array of known modifiers.

I'd have been content if the original implementation had simply added an inline comment, to save ourselves the struggle in trying to decipher its meaning 😅

packages/block-editor/src/components/keyboard-shortcuts/index.js

talldan

I really like the direction this is taking. 👍

I spotted a couple of bugs to be fixed - shortcuts not working and the unregister action has the wrong type.

packages/block-editor/src/components/keyboard-shortcuts/index.js

packages/block-editor/src/store/actions.js

packages/keyboard-shortcuts/src/store/actions.js

talldan · 2019-12-18T03:54:44Z

packages/block-editor/src/components/keyboard-shortcuts/index.js

+ // Registering the shortcuts
+ const { registerShortcut } = useDispatch( 'core/keyboard-shortcuts' );
+ useEffect( () => {
+ registerShortcut(


In terms of displaying a list of shortcuts in the help modal, it seems great that this can be done dynamically using the values from the store, and it might be good to move towards this more.

The main issue I see with shortcuts being registered in this way is that it could result in a situation where a component with shortcuts hasn't yet been rendered, meaning the shortcut isn't registered and so the description won't appear in the help modal.

I wondered if an api more like registerBlock in the blocks api might be preferable to get around that issue and then have these registered outside of the component scope.

I think registration belongs to the initialization of a package. In this case, the block-editor package does both at the same time but I agree that we should be careful about that.

I do think it should always be tied to a component though and registerBlock is the wrong approach here (can't change now) to avoid globals. See #8981 for the problems of the globals approach.

I might be wrong, but I think a similar issue exists today with the KeyboardShortcuts component anyway. If I remember correctly that's why there was a separate BlockActions component in the first place—the shortcuts have to be rendered in a different part of the render tree. So it might be a moot point, if the component isn't rendered the shortcuts won't work.

There's one case where this is still a little problematic, which is if you load the editor in Code Editor mode and then open the help modal, Code Editor elminates a huge part of the render tree, including the shortcut registration. Most of the Block shortcuts are then not visible. 😄

There's one case where this is still a little problematic, which is if you load the editor in Code Editor mode and then open the help modal, Code Editor elminates a huge part of the render tree, including the shortcut registration. Most of the Block shortcuts are then not visible.

That's a good point, I'll fix.

I split the registration and the handling into two components.

packages/block-editor/src/components/keyboard-shortcuts/index.js

talldan

@youknowriad I took if for a one last spin and it's all working well. Code looks good to me too. 🎉

Only thing that might be nice is tests for those new selectors and reducers, but the bulk of the code looks great and I'm about to go away until 2020. 😄

Co-Authored-By: Daniel Richards <daniel.richards@automattic.com>

aduth · 2019-12-20T13:24:20Z

packages/edit-post/src/components/keyboard-shortcut-help-modal/index.js

+ keyCombination: '/',
+ description: __( 'Change the block type after adding a new paragraph.' ),
+ /* translators: The forward-slash character. e.g. '/'. */
+ ariaLabel: __( 'Forward-slash' ),


Should this be an option in the shortcut registration? I see there's a few more usages of this in globalShortcuts as well.

Potentially yes, we'll probably find a few other options once we refactor all existing shortcuts.

packages/keyboard-shortcuts/src/store/actions.js

packages/keyboard-shortcuts/src/store/selectors.js

aduth · 2019-12-20T13:35:42Z

packages/keyboard-shortcuts/src/hooks/use-shortcut.js

+
+ shortcutKeys.forEach( ( shortcut ) => {
+ const keys = shortcut.split( '+' );
+ const modifiers = new Set( keys.filter( ( value ) => value.length > 1 ) );


It looks like it determines whether a key is a modifier by the length of the string. E.g. if I add a pass a shortcut Shift+Cmd+M, it'll determine that the modifiers are Shift and Cmd because they're not a single character.

That sounds about right.

Might be safer to check against an array of known modifiers.

I'd have been content if the original implementation had simply added an inline comment, to save ourselves the struggle in trying to decipher its meaning 😅

Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>

afercia · 2020-02-19T13:49:47Z

Thanks for this change. Read also the dev-note post and looking forward to the next steps to offer "a centralized UI to modify the keyboard shortcuts per user" so that #3218 gets addressed 🎉

A couple quick questions /Cc @youknowriad

1
What happens when two or more plugins register the same shortcut? I do realize they're registered under different "namespaces" but in the browser they will conflict. E.g. a potential keyboard shortcut Cmd + Alt + F + O + O added by plugin X will conflict with Cmd + Alt + F + O + O added by plugin Y. Not sure what would actually happen in the browser. One of the twos might not work or both might be triggered simultaneously, which doesn't seem ideal. Does the package handle potential conflicts?

2
Regarding:

Registered shortcuts can also be unregistered (and potentially replaced) by third-party plugins

Does this mean plugin X can unregister a keyboard shortcut added by plugin Y? If so, that doesn't sound ideal as it actually allows plugins to disrupt other plugins functionalities.

youknowriad · 2020-02-19T14:13:38Z

1-

I believe right now, both callbacks are triggered but we can enhance the package to do some conflicts detection.

2-

Yes, that's what it means but that's the case for blocks, for style variations, for any filter. Basically any WordPress API. So I'm not sure that's really a concern.

aduth · 2020-03-18T14:46:25Z

packages/keyboard-shortcuts/README.md

@@ -0,0 +1,32 @@
+# Keycodes


I assume this was meant to be "Keyboard Shortcuts"? 😃 (Guessing it may have been missed for revision in a copy paste)

Related: https://github.com/WordPress/gutenberg/blob/master/packages/keycodes/README.md

Copy/paste? me? never

Fixed in 8c6916c

The commit doesn't seem right though :) copy/paste issue?

The commit doesn't seem right though :) copy/paste issue?

🤦‍♂ ac818ae

youknowriad added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [a11y] Keyboard & Focus labels Dec 12, 2019

youknowriad requested review from ellatrix and talldan as code owners December 12, 2019 12:23

youknowriad self-assigned this Dec 12, 2019

youknowriad requested review from aduth, afercia and a team December 12, 2019 12:24

talldan reviewed Dec 12, 2019

View reviewed changes

packages/block-editor/src/components/keyboard-shortcuts/index.js Outdated Show resolved Hide resolved

MarcoZehe reviewed Dec 12, 2019

View reviewed changes

youknowriad force-pushed the add/keyboard-shortcuts-package branch 2 times, most recently from d810e97 to 6708224 Compare December 17, 2019 16:35

youknowriad requested review from ajitbohra, chrisvanpatten, mkaz, nerrad, oandregal and ntwb as code owners December 17, 2019 16:35

aduth reviewed Dec 17, 2019

View reviewed changes

talldan reviewed Dec 18, 2019

View reviewed changes

packages/block-editor/src/components/keyboard-shortcuts/index.js Outdated Show resolved Hide resolved

talldan requested changes Dec 18, 2019

View reviewed changes

youknowriad requested review from talldan and aduth December 18, 2019 08:30

talldan reviewed Dec 19, 2019

View reviewed changes

packages/block-editor/src/components/keyboard-shortcuts/index.js Show resolved Hide resolved

dpkm95 mentioned this pull request Dec 20, 2019

Keyboard shortcut to toggle block level HTML editing #9723 #19152

Closed

6 tasks

talldan approved these changes Dec 20, 2019

View reviewed changes

youknowriad and others added 2 commits December 20, 2019 14:08

Add a new keyboard shortcut package

a73c754

Fix typos

a1681f6

Co-Authored-By: Daniel Richards <daniel.richards@automattic.com>

youknowriad added 12 commits December 20, 2019 14:10

Fix Help Modal classnames

10822cb

Typing objects

1d77672

multiple tweaks per reviews

eb6cc9d

Use kebab-case consistently

c57d234

Remove unnecessary indirection

90d9e07

Fix documentation

fd1b491

Add data documentation

ab028f6

Fix shortcuts

3c020a4

Fix alias format

525d442

Split Keyboard Shortcuts registration and handling

f97b49f

Use lodash compact

6ca8806

Fix backspace

6aa4d62

youknowriad force-pushed the add/keyboard-shortcuts-package branch from c450eb9 to 6aa4d62 Compare December 20, 2019 13:12

aduth reviewed Dec 20, 2019

View reviewed changes

youknowriad and others added 6 commits December 20, 2019 16:55

fix optional parameter

87b374d

Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>

Fix aliases type

8dabd09

Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>

Fix aliases type

20b8f0e

Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>

fix docs

c7e0bc4

add comment

8ae05da

Fix multiselection removal shortcut

37d5f5b

youknowriad merged commit fba1069 into master Dec 23, 2019

youknowriad deleted the add/keyboard-shortcuts-package branch December 23, 2019 08:26

youknowriad added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Dec 23, 2019

aduth mentioned this pull request Jan 2, 2020

Undo / redo (keyboard shortcuts) affecting the whole document rather than the focussed TextControl #18755

Open

youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020

jorgefilipecosta mentioned this pull request Feb 12, 2020

Block Editor Dev Notes 5.4 #20185

Closed

23 tasks

aduth reviewed Mar 18, 2020

View reviewed changes

youknowriad removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new keyboard shortcut package #19100

Add a new keyboard shortcut package #19100

youknowriad commented Dec 12, 2019 •

edited

gziolo commented Dec 12, 2019

MarcoZehe left a comment

youknowriad commented Dec 17, 2019

aduth Dec 17, 2019

youknowriad Dec 18, 2019

talldan Dec 20, 2019 •

edited

aduth Dec 20, 2019

talldan left a comment

talldan Dec 18, 2019

youknowriad Dec 18, 2019

talldan Dec 19, 2019 •

edited

youknowriad Dec 19, 2019

youknowriad Dec 19, 2019

talldan left a comment

aduth Dec 20, 2019

youknowriad Dec 20, 2019

aduth Dec 20, 2019

afercia commented Feb 19, 2020 •

edited

youknowriad commented Feb 19, 2020

aduth Mar 18, 2020 •

edited

youknowriad Mar 18, 2020

aduth Apr 3, 2020

youknowriad Apr 3, 2020

youknowriad Apr 3, 2020

aduth Apr 6, 2020

Add a new keyboard shortcut package #19100

Add a new keyboard shortcut package #19100

Conversation

youknowriad commented Dec 12, 2019 • edited

gziolo commented Dec 12, 2019

MarcoZehe left a comment

Choose a reason for hiding this comment

youknowriad commented Dec 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talldan Dec 20, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talldan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talldan Dec 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talldan left a comment

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

afercia commented Feb 19, 2020 • edited

youknowriad commented Feb 19, 2020

aduth Mar 18, 2020 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Dec 12, 2019 •

edited

talldan Dec 20, 2019 •

edited

talldan Dec 19, 2019 •

edited

afercia commented Feb 19, 2020 •

edited

aduth Mar 18, 2020 •

edited