Block Toolbar: Refactor using KeyboardShorcuts component #3031
Conversation
To capture events within inputs
aduth
added
the
[Focus] Accessibility (a11y)
Codecov Report
@@ Coverage Diff @@
## master #3031 +/- ##
==========================================
+ Coverage 32.76% 32.92% +0.16%
==========================================
Files 203 203
Lines 5951 5940 -11
Branches 1052 1049 -3
==========================================
+ Hits 1950 1956 +6
+ Misses 3375 3362 -13
+ Partials 626 622 -4
Continue to review full report at Codecov.
|
| this.mousetrap.bindGlobal( key, callback ); | ||
| } else { | ||
| this.mousetrap.bind( key, callback ); | ||
| } | ||
| } ); |
There was a problem hiding this comment.
Do you think the shortcuts prop could change over time? Should we implement componentWillReceivePorps or not?
There was a problem hiding this comment.
Do you think the shortcuts prop could change over time? Should we implement
componentWillReceivePorpsor not?
Potentially, though the effect could be imitated by rendering separate KeyboardShortcuts components (or changing the key of an existing one). Doesn't seem like a need we currently have, but then again, could be a potential source of confusion for future usage.
There was a problem hiding this comment.
I started looking at this, and realized that there may be some non-trivial checks to consider whether keyboard bindings need to be updated.
https://gist.github.com/aduth/52f5c4ff561723c2d71b7e9d7fc142e7
Instead, and since there's currently no need, I improved documentation to note that shortcuts changes won't take effect and a suggested workaround (2601680).
editor/block-toolbar/index.js
Outdated
| <KeyboardShortcuts shortcuts={ { | ||
| mod: [ this.focusToolbar, true ], | ||
| 'alt+f10': [ this.focusToolbar, true ], | ||
| } } /> |
There was a problem hiding this comment.
Can we handle the "escape" key as well onToolbarKeyDown?
There was a problem hiding this comment.
Can we handle the "escape" key as well
onToolbarKeyDown?
We can, but to recreate the handling of these keys only taking effect while within the toolbar, we'd want to introduce child scoping for KeyboardShortcuts. I mentioned this in #1944, and should probably be done as a separate refactoring step.
There was a problem hiding this comment.
There are two differences with the previous behavior:
- This fires on key down
- This also fires when we try a combination (command + c) since it's on key down. I think we need to protect against this, we do not want to lose the focus on an input if we copy/paste...
Mousetrap does appear to support specifying the event to bind as a third argument: Will take a look to see how best we can expose this, especially considering we now expose a global option, and an array sequence is probably not very obvious / scalable. |
|
In latest commits (which are delayed to show by GitHub service interruption) I've refactored global binding and specifying an event name to separate props of |
|
Mmm, it looks like it's catching "command+*" key presses even on key up. This is a blocker IMO. Unless we change the "command" key (use "alt" or something else), I don't know how we can fix this and still use the |
|
@youknowriad Yeah, I've dug around a bit and there doesn't appear to be an obvious way to bind to just the modifier alone: keydown and keypress are too early since it could be the start of a combination, and keyup also fires for combinations. Related: ccampbell/mousetrap#216 |
|
Going to close this as not solvable with Mousetrap currently, though it would be a nice refactor. Changes here around adding a new |
Related: #2960 (specifically #2960 (comment))
Cherry-picks 334351c from #2863
This pull request seeks to refactor keyboard event handling introduced in #2960 to use the KeyboardShortcuts component, which abstracts many of the complexities of keyboard combinations and OS-varying modifiers.
The main challenge with this implementation is that Mousetrap (the underlying library behind KeyboardShortcuts) does not trigger callbacks if the keyboard combination is fired while within an input, except via its global binding extension. Since this was already implemented for #2863 (work-in-progress), it was merely cherry-picked for this pull request.
This pull request does not seek to implement any of the follow-up tasks from #2960. It is purely a refactor of existing behaviors.
Testing instructions:
Repeat testing instructions from #2960, verifying no regressions. From what I can tell from the discussion of #2960, this means: Pressing Ctrl (Windows) or Cmd (Mac) or a combination of Alt+F10 while a block is focused should move focus to the toolbar.