Block editor: add a keyboard shortcut to create group from the selected blocks #46972
Conversation
t-hamano
added
[Type] Enhancement
|
Size Change: +214 B (+0.01%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
t-hamano
requested review from
ntwb,
nerrad,
ajitbohra,
ellatrix,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
t-hamano
requested review from
talldan
and removed request for
juanmaguitar,
ntwb,
ajitbohra,
nerrad,
fabiankaegy and
ryanwelcher
|
Flaky tests detected in 4b69935. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8938077208
|
t-hamano
force-pushed
the
enhancement/shorcut-selected-blocks-to-group
branch
from
af7a71d
to
f329fa4
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
@t-hamano any reason this never got added? 🤔 |
|
I think this PR makes sense, but maybe it got buried because there are so many open PRs out there 😅 There is a conflict going on and I would like to resolve this and |
|
I changed the key combination as follows.
This should match the command for grouping in Figma on both Windows and Mac, is this correct? Please let me know if this change makes sense since I can't test it on a Mac 😅 However, this key combination should override the browser's search shortcut on Windows and Mac. So I think this shortcut should only be allowed in the context of multiple blocks being selected.
Yes, this could be considered in follow-up. Or maybe we can include it in this PR. If we set the shortcut for grouping as described here, the shortcut for ungrouping would be:
|
I made the shortcut key appear only when multiple blocks are selected. Because this current PR does not allow grouping when a single block is selected. A single block is selected:
Multiple blocks are selected:
|
There was a problem hiding this comment.
Behavior wise this is solid. There's a shortcut indication in the menu:
⌘G works great for grouping when you have two blocks selected. When you do not have blocks selected, ⌘G invokes the browser shortcut for find next. This works great.
There's one followup to do, I noticed that when you have a group or other container block selected, the shortcut for the Group does not show up:
But this is an excellent quality of life feature, very nice work. I can't wait for a followup that adds ⌘⇧G for ungrouping 👌
| const clientIds = getSelectedBlockClientIds(); | ||
| if ( clientIds.length > 1 && isGroupable( clientIds ) ) { | ||
| event.preventDefault(); | ||
| const blocks = getBlocksByClientId( clientIds ); | ||
| const groupingBlockName = getGroupingBlockName(); | ||
| const newBlocks = switchToBlockType( | ||
| blocks, | ||
| groupingBlockName | ||
| ); | ||
| replaceBlocks( clientIds, newBlocks ); | ||
| } |
There was a problem hiding this comment.
A spoken message when grouping is successful might be a good affordance. I tested with voiceover and there's not any indication that anything happens.
There was a problem hiding this comment.
Updated by 8cf7f34
When you run the shortcut, you should hear "Selected blocks are grouped.".
Video tested with NVDA:
7274217df9cd891d1cc3d9bcedd95b7d.mp4
There was a problem hiding this comment.
By the way, we may need to add a spoken message to other shortcuts as well. See #61168.
Let's get feedback before merging this PR. @afercia, @alexstine, if you have any feedback regarding this PR, please let us know. Summary
|
|
A few notes from my testing (MacOS)
I would also second the idea that if we're adding a shortcut for Group it makes sense to do Ungroup as well (with the But overall great work and this is going to be a very popular enhancement. |
To be honest I don't feel this guardrail is necessary. In most design software, repetition of the shortcut creates additional groups. In not allowing this, I worry it might be frustrating in situations you want nested groups, rare as it may be. Ultimately it feels like something we could add later based on feedback. |
Nice catch! Added shortcut to work in the list view as well.
If we remove this guardrail, in most cases you will override the browser's search shortcut (Find Next / Find Previous). If a user wants to use the browser's search shortcut, they are forced to move focus away from the editor canvas or list view. This is my only concern, but I'd love to hear what the accessibility team has to say. |
|
It could be reasonable to start with the current PR behavior, and then remove the guardrail as a separate discussion at a later time, if the grouping behavior feels unintuitive. |
|
@t-hamano Thanks for waiting on feedback, much appreciated. Hoping I can start reviewing faster soon. Do not have time to do a code review at this point but the accessibility is good. The announcement is working as expected. Let's not remove the guardrail in this PR because we need to figure out how to communicate to non-sighted users that multiple nested groups could be created depending on how many times they use the shortcut. Honestly, I would have a hard time understanding why someone would want a group inside a group. The only issue I could find is when grouping blocks from the list view, the editor canvas steals focus. I think this has come up in other PRs and if you don't have an easy way to fix it, let's figure out how to solve the actual problem in a follow-up. Thanks. |
| @@ -324,6 +332,19 @@ function ListViewBlock( { | |||
| collapseAll(); | |||
| // Expand all parents of the current block. | |||
| expand( blockParents ); | |||
| } else if ( isMatch( 'core/block-editor/group', event ) ) { | |||
| const clientIds = getSelectedBlockClientIds(); | |||
There was a problem hiding this comment.
In the List View, rather than using the selected block client ids, I think it's generally better to use const { blocksToUpdate } = getBlocksToUpdate(); as used in the insert before and after keyboard shortcuts above. This will ensure that if the user is focused on a block that isn't part of the selection, then the action is performed on the currently focused block. If the focused block is part of the selection, then the whole selection is used.
There was a problem hiding this comment.
I just realised this might not be particularly clear as to why it matters. Here's a quick example video of the behaviour as it is currently, where if I move up the list view to outside the selection, the keyboard shortcut acts on the selected blocks instead of the focused one:
2024-05-08.16.12.33.mp4
| blocks, | ||
| groupingBlockName | ||
| ); | ||
| replaceBlocks( clientIds, newBlocks ); |
There was a problem hiding this comment.
To fix the issue of the canvas stealing focus that Alex mentioned, we might be able to use a similar approach as in the insert before and after keyboard shortcuts above, where we grab the selected blocks after calling replaceBlocks and then call the following to shift focus back to the list view again:
const newlySelectedBlocks = getSelectedBlockClientIds();
// Focus the first block of the newly inserted blocks, to keep focus within the list view.
updateFocusAndSelection( newlySelectedBlocks[ 0 ], false );
|
@alexstine @andrewserong Thanks for the review!
I see, let's keep the current specs.
I have also addressed this approach. This part of the E2E test should ensure that the focus remains on the list view when you run the shortcut in the list view. |
Could a Snackbar work, similar to the 'Copy all blocks' action? |
| @@ -324,6 +332,23 @@ function ListViewBlock( { | |||
| collapseAll(); | |||
| // Expand all parents of the current block. | |||
| expand( blockParents ); | |||
| } else if ( isMatch( 'core/block-editor/group', event ) ) { | |||
| const { blocksToUpdate } = getBlocksToUpdate(); | |||
| if ( blocksToUpdate.length > 1 && isGroupable( blocksToUpdate ) ) { | |||
There was a problem hiding this comment.
Why is the check that the length is greater than one? If a single block is selected or focused, should we still be able to group it? In the editor canvas, I think it's good to only allow it in a multi-selection, but it could still be useful to allow it in the list view, otherwise this is what happens when using the shortcut over a focused item that isn't part of the selection:
2024-05-09.11.10.36.mp4
The above video demoes grouping two blocks, then moving up to a block outside the selection and pressing the keyboard shortcut.
There was a problem hiding this comment.
The reason for checking that the length is greater than 1 is to be consistent with the editor canvas. When I run the shortcut on a single item that has focus and is not in a selection, the browser's search window opens. I think this is the expected behavior for now, but is the same behavior happening in your environment?
There was a problem hiding this comment.
Yes, that's what's happening for me, too. All good to leave it as-is for now, and we can always revisit in follow-ups if folks run into any issues. The main value is to group multiple items together so I doubt that in practice anyone will really struggle with the current behaviour.
|
Thank you everyone for your reviews! I think everything is ready, so let's merge this PR. If we have any accessibility issues or spec concerns, I will be happy to follow up and address them. |
Closes #42227
What?
This PR adds a keyboard shortcut to create a group block from the selected blocks.
Why?
Many design tools provide shortcuts for grouping elements. It would make sense to add a similar shortcut, especially since the grouping of blocks should be done frequently in the site editing.
How?
To explore which key combinations were available, I tested all variations that included the letter
G. The list can be found in this comment.The results show that the following two modifiers are available:
primaryAlt:⌘ command+⌥ option+Gon Mac /ctrl+alt+Gon Windowsecondary:⌘ command+shift+⌥ option+Gon Mac /ctrl+shift+alt+GI choseprimaryAltbecause thesecondarymodifier could be used for ungrouping (unwrapping) in the future.However, as noted in this comment, it appears that Ctrl + Alt is usually used for Windows OS-level functions. I would appreciate your advice on whether it makes sense to add this shortcut and whether it will cause any confusion to the user.Update: After some discussion, we adopted the simpler
⌘ command + G(ctrl + G) as a shortcut.Also, this shortcut will not allow to create a group block if a single block is selected. This is consistent with the behavior of group block transformation.
Testing Instructions
ctrl+alt+G. On Mac, press⌘ command+⌥ option+G.Screenshots or screencast
83a8606c38f7f66351eb8b7854058146.mp4