List View: Add multi-select behaviour for blocks when shift key is selected #38314
Conversation
andrewserong
added
[Type] Enhancement
|
Size Change: +253 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
| } = select( blockEditorStore ); | ||
| return { | ||
| rootClientId: getBlockRootClientId( clientId ) || '', |
There was a problem hiding this comment.
As of #33320 the rootClientId is no longer used in this function, so I think it can safely be removed here.
andrewserong
force-pushed
the
add/multi-selection-to-list-view
branch
from
e0937d1 to
356673e
Compare
andrewserong
requested review from
adamziel,
ajitbohra,
draganescu,
ellatrix,
kevin940726,
nerrad,
noisysocks,
ntwb,
talldan and
tellthemachines
as code owners
|
Hi folks! (Looks like this PR touches files that ping quite a number of folks!) 👋 I don't have a lot of experience making changes to the |
|
Wow this works insanely well! Nice job. I tested: selecting multiple blocks and using keyboard shortcuts (e.g. Del, Cmd+c), selecting multiple blocks and dragging them within the same parent, selecting multiple blocks and dragging them into a different parent, selecting multiple blocks that have children, and doing this across each of the four editors. (Thank you for remembering that we have multiple editors!) I set Firefox to emulate an iPad in landscape mode and noticed that I could not select multiple blocks. (iPads support bluetooth keyboards.) But you can't do this using the block editor either, so definitely not a blocker for this PR. My only thought is that maybe we should hide the ⋮ buttons when multiple blocks are selected? They're a bit confusing in this context. It's not clear if clicking on one and selecting an action will perform that action on one or multiple blocks.
|
| @@ -89,11 +98,64 @@ function ListView( | |||
| [ draggedClientIds ] | |||
| ); | |||
| const selectEditorBlock = useCallback( | |||
There was a problem hiding this comment.
This callback does a lot more than selecting an editor block now so maybe its name should change to updateBlockSelection or something like that.
There was a problem hiding this comment.
Oh, nice idea. Happy to update it 👍
| multiSelect( start, end ); | ||
| } | ||
| } | ||
| } | ||
| onSelect( clientId ); |
There was a problem hiding this comment.
Should we pass multiple IDs to this prop?
Or, since it looks like we're no longer using it, remove the prop altogether? Maybe third parties would like to have it though.
ListView is currently experimental so we can safely make changes to its API.
There was a problem hiding this comment.
I actually really like the idea of removing it altogether if we can. Happy for any feedback folks might have!
|
Thanks for reviewing and taking this for a spin @noisysocks! 🙇
That's a good question — currently the ⋮ button adjusts settings for the individual block. Personally, I'd lean toward making changes to the UI in subsequent PRs if we can. Another nice thing to look into would be how we visualise dragging multiple blocks so that it's clear it isn't the one block that's being dragged. But my thinking was, if we can land the multi-select first, then it'll hopefully be easier to look at some of these more granular UI things in isolation. Happy to do it here, though, if folks think it's a blocker! |
|
Wow, this is a pretty cool feature! Appreciate all the hard work.
I don't have any other suggestions, so this seems good. Might be nice to wrap it up into a hook or something. It'd be good to make sure this gets some accessibility testing. @alexstine has been working on List View a lot recently so can possibly assist. One thing that jumps out is that some spoken messages might be required to announce the selection range. Though potentially that could already be implemented in the multiselect store actions. Worth double-checking that and whether any existing messages are still up to the task. (edit: I notice now this is only usable via shift+click, it should probably work via arrow keys too. It could be done in stages via a separate PR, but without that it's not really an accessible feature) |
|
Here's what I'm seeing
I tested with 👍 👍 👍
👍 This is a wonderful UX improvement, thank you! And for the e2e tests.
+1 |
🤔 I don't remember offhand, but I'm still catching up on things after my 3 month sabbatical too. 😆 I'll try and 🔍 this one today. This is a bit of a tangent, but one other thought that comes to mind is that I'd really like to optionally run performance tests with list view open when we make changes in this component. I'll maybe smoke test in a large post for now, and think through some options in another PR. |
|
Reserving this comment for manual testing: TIL that shift + mousewheel = scroll horizontally ✅ In a large post (where we end up using windowing), the selection behaves as expected example.of.selection.in.large.post.mp4❌ This might be arguable/better left for a follow up PR. A selection starting from nested child blocks, ends up selecting the parent. If you compare behavior in a normal filesystem, this isn't the case. unexpected.selection.mp4🤔 Looking at some older videos, we didn't use to select blocks in the modal view (behavior is now in trunk too). Was this an intentional change? ❌ Open List View, also open the navigation modal. Nav menu is removed and inserted to bottom of post: list.view.open.drag.items.in.menu.bug.mp4 |
|
@andrewserong Accessibility with the keyboard is getting much better, great work. 👍 My only comment is about the unselect behavior. If I select a Paragraph Block and a Table Block, then unselect the Table Block, it announces "1 Block selected, Paragraph Block selected". Is there anyway to change this so it says which item was "unselected"? E.g. "1 Blocks selected, Table Block unselected". It may even be a good idea to change it around so name of Block comes first followed by how count. Thoughts? Thanks. |
andrewserong
force-pushed
the
add/multi-selection-to-list-view
branch
from
4c7e257 to
db31ffd
Compare
|
@talldan I've updated this PR to pass in the @javierarce great catch about the expand / collapse button. I've added in an |
|
@alexstine, thanks for giving this another test! 🙇 That's a great idea about announcing the blocks that are deselected. I've made an attempt at adding in an announcement, but I'm not sure if it's quite right. It now announces the name of the block that has been deselected if there is only one. If there is greater than one deselected block, then it announces the number of blocks deselected. Because the code that announces the selection of blocks is spread across different areas, I'm not sure how feasible it is in this PR to move the logic all to the one place, which means that we might be limited in terms of the order in which things are announced, unless there's another way to manipulate it. It now reads out both the block that is selected and the one that is deselected, so I could imagine that being confusing. I think the block that is selected is announced via the component we are currently focused on, rather than a call to Please let me know if you have further feedback on how it should work, and I'm happy to keep iterating! |
| ###### onChangeRow( event: Event, startRow: HTMLElement, destinationRow: HTMLElement ) | ||
|
|
||
| Callback that fires when focus is shifted from one row to another via the UP and DOWN keys. | ||
| The callback is passed the event, the start row element that the focus was on originally, and | ||
| the destination row element after the focus has moved. | ||
|
|
||
| - Type: `Function` | ||
| - Required: No | ||
|
|
||
| ###### onCollapseRow( row: HTMLElement ) | ||
|
|
||
| A callback that passes in the row element to be collapsed. | ||
|
|
||
| - Type: `Function` | ||
| - Required: No | ||
|
|
||
| ###### onExpandRow( row: HTMLElement ) | ||
|
|
||
| A callback that passes in the row element to be expanded. | ||
|
|
||
| - Type: `Function` | ||
| - Required: No | ||
|
|
There was a problem hiding this comment.
Thanks for documenting the expand/collapse props too!
| } | ||
|
|
||
| if ( label ) { | ||
| speak( label ); |
There was a problem hiding this comment.
This message works pretty well in testing.
In terms of code, it could be an option to try moving this announcement to the multiSelect action which already has a call to speak.
It should hopefully be possible to combine it with the existing message. Something like '2 blocks selected (Paragraph deselected)'. I'll defer to Alex's call on what the best text would be.
I think it'd be ok to try this out in a separate PR.
There was a problem hiding this comment.
Thanks for confirming, Dan! I agree, this PR has already gotten fairly big, so it'd probably be a good thing to explore separately as a follow-up.
|
Thanks for reviewing again @talldan! I'll leave this PR overnight just in case anyone else has feedback they'd like to see incorporated before merging. If there aren't any objections, I'll then merge it in tomorrow. Thanks again for all the reviews, everyone 🙇 |
There was a problem hiding this comment.
This looks good for accessibility. Can improve in separate PR where diffs will be smaller and easier to test.
Thanks for your hard work on this @andrewserong . Certainly looks like a ton more code involved for the keyboard part of this.
There was a problem hiding this comment.
I'll leave this PR overnight just in case anyone else has feedback they'd like to see incorporated before merging. If there aren't any objections, I'll then merge it in tomorrow. Thanks again for all the reviews, everyone 🙇
Sorry for my late review, @andrewserong ! Unfortunately I'm lagging a bit behind with PR reviews these days 😅 I only left a couple of tiny inline comments, nothing that should really delay this PR further.
Thank you!
|
Alrighty, tests are passing, so I'll merge this in now. Thanks so much again @noisysocks, @gwwar, @talldan, @alexstine, @ciampo, and @javierarce for all the reviews, testing, and feedback — it's been immeasurably helpful in developing this feature (and I've learnt a lot in the process, too). Kudos! 🙇 |
|
🔥 |
|
I've just opened a PR in #38942 as a follow-up to add in some unit tests for the |
| export const multiSelect = ( | ||
| start, | ||
| end, | ||
| __experimentalInitialPosition = 0 |
There was a problem hiding this comment.
@andrewserong Why did we add this parameter? How does initialPosition make any sense for a multi block selection? initialPosition implies collapsed selection, while multi selection implies an uncollapsed selection.
There was a problem hiding this comment.
@ellatrix the existing initialPosition state is used in the writing flow to control whether or not to shift focus to the editor (for single block selection), so I thought it might make sense to re-use that same logic for multiple block selection so that the logic is as consistent as we can make it. In this case, we're using it effectively like a boolean, so setting to null says "there is no initial position" (and therefore don't focus in the editor canvas), and with the default 0 value, the position is "within the editor canvas".
For accessibility it was important that we don't shift focus to the editor canvas when multi-selecting from the list view. If there's a more semantic way to do this than using initialPosition we can definitely refactor it (one of the reasons I think @talldan suggested we use the __experimental prefix for the moment).
Do you have any guidance on how best to preserve the behaviour of skipping focusing on the editor canvas, if we were to refactor this?
There was a problem hiding this comment.
Yes, I understood the intent after digging a bit further. :) The naming is a bit confusing and I'm not sure if reusing the 0 value makes sense to mean selecting all content. Another thing to keep in mind is that focus and selection are two separate things. Even if focus has to stay in the List View, we still need to select the block contents. Anyway, it's not important right now :)
There was a problem hiding this comment.
Okay, great. Thanks for confirming @ellatrix, please do ping me if it does become important / needs updating, though, and I can follow up!
|
I've opened up another small follow-up PR in #39272 to look at adding support for shift-selecting via the Home and End keys. |
Description
Fixes: #37514
This PR borrows logic from the
useMultiSelectionhook to implement multiple block selection from the list view within the post, site, and widget editors, and the list view in the Navigation block. The goal is that wherever theListViewcomponent is used, it comes with multi-block selection via the SHIFT key by default.With the SHIFT key held down, you should be able to select multiple blocks from the same nesting level.
Testing Instructions
Test shift+click behaviour
Test shift+up/down arrow behaviour
There are an additional two e2e tests added in this PR to cover mouse and keyboard behaviour. You can run them locally via:
While running locally, I noticed that some of the other tests failed intermittently. If you wish to run this locally, too, to make it easier for local testing, you can update the
it(lines in themulti-block-selection.test.jsfile toit.only(to only test the added tests.There is also an added unit test which can be run via:
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
To-do
edit-postandedit-sitebehaviours — can we move the select logic to a hook to remove the duplication? Update: yes! I've moved the logic to the component since we'd like this logic in place in each of the use cases for the list view.Checklist:
*.native.jsfiles for terms that need renaming or removal).