Multi-selection: allow partial block selection #38892
Conversation
ellatrix
added
[Type] Enhancement
|
Size Change: +1.5 kB (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
|
🎉 Let us know when you'd like some testing and reviewing done. For now this was really cool to try out, but I can also consistently crash the editor if I delete a span of text across two blocks (using Enter) and then use the editor's undo action. |
ellatrix
force-pushed
the
try/partial-multi-selection
branch
from
d44bd99 to
c245115
Compare
That's because |
|
Almost all e2e tests should be passing now. What remains to be done is polishing up the code and reusing more of the existing merge function (merge the As part of that, or after that, we can look into adding
|
| event.preventDefault(); | ||
| removeBlocks( clientIds ); | ||
|
|
||
| if ( clientIds.length < 2 ) { |
There was a problem hiding this comment.
Some of these cases could use a comment to describe the situation we are addressing
|
Excellent to see this! I'm glad it's able to build incrementally on the foundation we have. Ensuring Outside of the mechanics, I think we need to revisit the multiselect outline to only show when you are performing a block level operation (moving multiple blocks, transforming, etc) similar to how we highlight the outline on hover / focus of the blocktype area on single blocks. It might need some design tweaks as well. One last comment: I think it might be worth (as a follow up) to consider whether it makes sense to allow pressing shift while selecting across blocks to force full block selection instead of partial. Curious what you think about that or if it'd even be necessary given block level tools would still apply block level even with partial selections (inspector controls, etc). |
Yes I'm definitely going to work on enabling this for keyboard as well. So far focus has been on getting tests to pass :D |
Added selection by keyboard. To do: add |
| const selectionAnchor = select.getSelectionStart(); | ||
| const selectionFocus = select.getSelectionEnd(); | ||
|
|
||
| if ( selectionAnchor.clientId === selectionFocus.clientId ) return; |
There was a problem hiding this comment.
should we not wrap these in {}s to avoid making it easy to recreate some infamous bugs due to the missing brackets?
// oops!
if ( … )
someNewThing();
return;There was a problem hiding this comment.
Prettier will decrease indentation, so imho it's clear when you add lines. If we don't like this style, we should probably add the relevant rule to the linter.
There was a problem hiding this comment.
just raising it because I thought the practice was long abandoned because of its risk. I hadn't seen it in Gutenberg before and it seems riskier still to rely on the auto-formatter to avoid code patterns that invite bugs. carry on 🙇♂️
(by the way, it would seem fine to add this to the linter if other people are also leery of single-line if blocks)
There was a problem hiding this comment.
Right, I'm fine with avoiding it. Maybe we should add it to the linter though, so it's "official".
| * @param {string} attributeKey The selected block attribute key. | ||
| * @param {number} startOffset The start offset. | ||
| * @param {number} endOffset The end offset. | ||
| * @param {string|Object} clientId The selected block client ID. |
There was a problem hiding this comment.
should we update the description here, or better, use a different attribute name for clientId if we aren't actually passing a clientId? I'm guessing we're looking at passing an actual block instead of a clientId, but I would be surprised if I had to do this: block = action.clientId;
There was a problem hiding this comment.
Right, I just haven't stared fixing all the doc blocks yet.
There was a problem hiding this comment.
➕ to update the docs and possibly the param name here. Also can we rename the passed object's property from start to selectionStart to map the name in state?
There was a problem hiding this comment.
This is a great start, people will love to have this. I didn't dive into the code but I wanted to try it a bit.
Things I noticed so far:
- It seems the selection works well for paragraph blocks only
- When I tried, list, quote blocks, selecting across two blocks does some weird things: The selection changes when I release the mouse.
- After using the feature (Enter after cross paragraph selection), I think the second paragraph block end up in a weird state. I'm not able to use "Enter" on that paragraph anymore, sometimes cross-selection doesn't work anymore.
- In Firefox, multi-selection doesn't seem to work.
- I have some blocks that can fail (JS errors) when I try merge or undo after a cross-selection removal or "Enter" (I'm not sure yet about the exact step but here's what I see on the console.
|
@ellatrix sorry :) to clarify my comment regarding shift, I meant combining shift with pointer based selection (so you drag between blocks while holding shift) to force pure multi-selection. Though I don't think it'd be necessary if we always have block selection happening anyways. |
If works for me. Did you pull the most recent changes? Both list and quote blocks have merging functions and I checked that this works correctly. For some blocks that don't have merging functions we will select the entire blocks at mouse up, which is what we do for all blocks currently. That is to communicate to the user that the blocks are selected as a whole (not partially).
I'm still able to press Enter in both the newly created empty paragraph (sitting in the middle) and the third paragraph. Not sure if we're at the same commit 😅
Confirmed. I will investigate this.
This also indicates to me that you're maybe at an earlier state of this PR? I used to get error messages, but I no longer get them. |
|
@ellatrix I think the last Firefox fix broke the behavior in Safari and Chrome. Also, in Firefox, I noticed that I'm not able to start a selection from the currently selected block. |
ellatrix
force-pushed
the
try/partial-multi-selection
branch
3 times, most recently
from
a790836 to
f875847
Compare
|
In case anyone's interested, I had a hard time assessing the clipboard contents (as usual when dealing with the clipboard) while testing with this. I made a simple clipboard viewer to reveal all the MIME types and previews of the items in the paste events. |
ellatrix
force-pushed
the
try/partial-multi-selection
branch
from
f875847 to
90240a5
Compare
ellatrix
requested review from
cameronvoell,
ntwb,
nerrad,
ajitbohra and
mkaz
as code owners
|
This is still working good but I'm noticing a couple more issues:
|
|
@youknowriad I tested both of your points in different browsers and it works fine. Can select text in Safari, Firefox and Chrome. Shift+Arrow seems to work well in Safari.
|
|
I just tested again and I also can't reproduce. I don't know if I was on the wrong commit or if it something that starts happening after some interactions. Anyway, found another smaller thing where sometimes I have to hit "backspace" twice to delete a simple selection (inside a quote). Screen.Recording.2022-03-28.at.9.22.52.AM.mov |
Yes, seeing that also. Can't delete selection in list as well. So many behaviours we don't have tests for 🙈 |
I fixed it, just waiting for the tests to run on the last commit to push. |
|
Outstanding work here, thanks for persevering @ellatrix :) |
Description
Fixes #27481.
See also #3629.
These changes allow the user to select multiple blocks without selecting the blocks as a whole. This is done by allowing native selection across block boundaries, detecting the selection start and end in the appropriate rich text instances, and handling actions for this selection.
This PR implements
Enter,Backspace,Delete(forward merge) and any input (typing) over the selection.You can select partially by mouse (drag), shift + click, and also the keyboard (shift + arrow).
It works in all browsers including Firefox.
We use the
mergefunction on the block type to handle merges and detect if two blocks are mergeable. If two blocks are not mergeable, we fall back to the current behaviour, which is to select the blocks entirely.Testing Instructions
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).