[Multi-selection]: Copy/cut partial selected blocks #40098
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ntsekouras
added
[Type] Bug
ntsekouras
requested review from
mcsf,
Mamaduka,
youknowriad,
mtias,
oandregal,
gziolo,
jasmussen and
jorgefilipecosta
|
Size Change: +371 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
I think it's expected behavior for text editors to merge |
|
Yes, we'd definitely want to trigger a merge for cutting. |
ntsekouras
commented
Apr 7, 2022
| if ( isFullySelected && ! expandSelectionIsNeeded ) { | ||
| removeBlocks( selectedBlockClientIds ); | ||
| } else { | ||
| __unstableDeleteSelection(); |
There was a problem hiding this comment.
Do we need to handle how merging will be performed(forward or not)? This is defined in other cases explicitly by checking the pressed key(Escape or Delete). Should we decide based on where the selection starts and make this as a target to merge? --cc @mtias
There was a problem hiding this comment.
That's a good question to raise, but IMO cutting isn't a directional operation in the minds of users. I'd stick with a plain (isForward: false) deletion.
|
This is great, and it works exactly as I'd expect: Cutting only the text I've selected, and merging the remains together. Nice work. |
ntsekouras
commented
Apr 7, 2022
| */ | ||
| import { store as blockEditorStore } from '../../store'; | ||
|
|
||
| export default function CopyMenuItem( { blocks, onCopy } ) { |
There was a problem hiding this comment.
Does it make sense to partial copy from Copy button in Block Actions? Is this something mentally mapped to copying whole blocks? 🤔
There was a problem hiding this comment.
That's a good question. I'd love more thoughts on this, but my instinct would always be that ⌘X, ⌘C and ⌘V should match 1:1 what similar cut/copy/paste options are surfaced in the UI. I.e. be the same.
There was a problem hiding this comment.
Mmm, I think the opposite — the copy button should say "copy blocks" and work at the full block scope, like duplicate, group, movers, and transforms since it's meta to the whole block.
There was a problem hiding this comment.
Mmm, I think the opposite — the copy button should say "copy blocks" and work at the full block scope, like duplicate, group, movers, and transforms since it's meta to the whole block.
This is my instinct as well. If so, we should ensure that focusing the Copy action highlights the whole block, just as for other block-wise actions.
ntsekouras
force-pushed
the
try/partial-selection-copy-cut
branch
2 times, most recently
from
814b455
to
b2c8c40
Compare
mcsf
approved these changes
Apr 11, 2022
| if ( isFullySelected && ! expandSelectionIsNeeded ) { | ||
| removeBlocks( selectedBlockClientIds ); | ||
| } else { | ||
| __unstableDeleteSelection(); |
There was a problem hiding this comment.
That's a good question to raise, but IMO cutting isn't a directional operation in the minds of users. I'd stick with a plain (isForward: false) deletion.
This was referenced Apr 11, 2022
ntsekouras
force-pushed
the
try/partial-selection-copy-cut
branch
from
b2c8c40
to
e8b0901
Compare
ntsekouras
force-pushed
the
try/partial-selection-copy-cut
branch
from
78cb2af
to
0d20c29
Compare
ntsekouras
added
the
Backport to WP 6.7 Beta/RC
adamziel
pushed a commit
that referenced
this pull request
Apr 13, 2022
* [Multi-selection]: Copy/cut partial selected blocks * handle `copy` button in block actions * merge blocks on cut * partial copy from block actions only when is mergeable * update selector * revert partial selection copy in block actions menu item * add jsdoc for mapRichTextSettings util * update tests * add new e2e tests + pressKeyTimes playwright util
|
This was cherry-picked to the |
adamziel
removed
the
Backport to WP 6.7 Beta/RC
|
@ntsekouras: I don't know how I missed this in the review, but I'm experiencing a bug that I can trace back to this PR. In the screencast below, I am selecting a paragraph block, then pressing cmd+C to copy it entirely. Notice how the block flashes, but no copy occurs (as confirmed by the system clipboard). Only when I press cmd+C twice in a row do I get a successful whole-block copy. I haven't looked into it, but my immediate suspicion was that Writing Flow is incorrectly thinking that the block isn't fully selected and thus expands the selection on the first keyboard chord. copy-whole-block-expand.mov |
Confirmed by quickly patching diff --git a/packages/block-editor/src/store/selectors.js b/packages/block-editor/src/store/selectors.js
index df2c16aa91..ca1b60fb19 100644
--- a/packages/block-editor/src/store/selectors.js
+++ b/packages/block-editor/src/store/selectors.js
@@ -925,10 +925,15 @@ export function __unstableIsFullySelected( state ) {
const selectionAnchor = getSelectionStart( state );
const selectionFocus = getSelectionEnd( state );
return (
( ! selectionAnchor.attributeKey &&
! selectionFocus.attributeKey &&
typeof selectionAnchor.offset === 'undefined' &&
typeof selectionFocus.offset === 'undefined' )
+ // Or if selection is collapsed
+ || ( selectionAnchor &&
+ selectionAnchor.clientId === selectionFocus.clientId &&
+ selectionAnchor.attributeKey === selectionFocus.attributeKey &&
+ selectionAnchor.offset === selectionFocus.offset )
);
} |
This was referenced Apr 23, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes: #39455
This PR implements
copy/cutfunctionality for partial selections.Why?
Currently if we copy/cut when we have a partial selection, the whole blocks are affected(copied/cut). We need to only copy/cut the partial selection.
Testing Instructions
copybutton from block actions.Notes
I'll have to write e2e tests✅actions, etc...Screenshots or screencast
Screen.Recording.2022-04-06.at.4.51.19.PM.mov