Skip to content

fix: Make cut/copy/paste work consistently and as expected #9107

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
RoboErikG merged 5 commits into from
May 30, 2025
Merged

fix: Make cut/copy/paste work consistently and as expected #9107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
796e57a
Work on cut/copy/paste preconditions
RoboErikG May 28, 2025
a8c44c7
Cleanup and fixes to cut/copy/paste
RoboErikG May 28, 2025
c0ed717
Fix tests
RoboErikG May 28, 2025
857ecee
Remove editable check from isCopyable and isCuttable
RoboErikG May 30, 2025
8f7ca4f
Merge branch 'develop' into copy-cut-paste
RoboErikG May 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 75 additions & 23 deletions core/shortcut_items.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ let copyWorkspace: WorkspaceSvg | null = null;
let copyCoords: Coordinate | null = null;

/**
* Determine if a focusable node can be copied using cut or copy.
* Determine if a focusable node can be copied.
*
* Unfortunately the ICopyable interface doesn't include an isCopyable
* method, so we must use some other criteria to make the decision.
Expand All @@ -113,8 +113,8 @@ let copyCoords: Coordinate | null = null;
* - It must be an ICopyable.
* - So that a pasted copy can be manipluated and/or disposed of, it
* must be both an IDraggable and an IDeletable.
* - Additionally, both .isMovable() and .isDeletable() must return
* true (i.e., can currently be moved and deleted).
* - Additionally, both .isOwnMovable() and .isOwnDeletable() must return
* true (i.e., the copy could be moved and deleted).
*
* TODO(#9098): Revise these criteria. The latter criteria prevents
* shadow blocks from being copied; additionally, there are likely to
Expand All @@ -127,6 +127,40 @@ let copyCoords: Coordinate | null = null;
function isCopyable(
focused: IFocusableNode,
): focused is ICopyable<ICopyData> & IDeletable & IDraggable {
if (!(focused instanceof BlockSvg)) return false;
return (
isICopyable(focused) &&
isIDeletable(focused) &&
focused.isOwnDeletable() &&
isDraggable(focused) &&
focused.isOwnMovable()
);
Comment on lines +131 to +137
Copy link
Collaborator

@cpcallen cpcallen Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change makes things that we decided we did not want to make copyable copyable.

This includes shadow blocks, but also blocks that have custom isDeletable and isMovable methods.

While I was working on #9084 we decided we did not want to do that in v12.

I'm not necessarily averse to making some things copyable (which were not previously copyable), but It is in my opinion a mistake to have done so without introducing an isCopyable method on ICopyable so that block definition authors can have more fine-grained control.

}

/**
* Determine if a focusable node can be cut.
*
* Unfortunately the ICopyable interface doesn't include an isCuttable
* method, so we must use some other criteria to make the decision.
* Specifically,
*
* - It must be an ICopyable.
* - So that a pasted copy can be manipluated and/or disposed of, it
* must be both an IDraggable and an IDeletable.
* - Additionally, both .isMovable() and .isDeletable() must return
* true (i.e., can currently be moved and deleted). This is the main
* difference with isCopyable.
*
* TODO(#9098): Revise these criteria. The latter criteria prevents
* shadow blocks from being copied; additionally, there are likely to
* be other circumstances were it is desirable to allow movable /
* copyable copies of a currently-unmovable / -copyable block to be
* made.
*
* @param focused The focused object.
*/
function isCuttable(focused: IFocusableNode): boolean {
Copy link
Collaborator

@cpcallen cpcallen Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I created isCopyable, above, was to ensure that cut and copy used common criteria for deciding if something can be cut/copied, with cut being allowed for anything that can be copied and is additionally deletable—AFAICT there is no circumstance where it should be possible to cut something but not copy it.

So isCuttable should call isCopyable (not just isICopyable), and return false if it does. Or… we could just have the cut preconditionFn just check isCopyable(focused) && focused.isDeletable(), as it previously did.

if (!(focused instanceof BlockSvg)) return false;
return (
isICopyable(focused) &&
isIDeletable(focused) &&
Expand All @@ -151,29 +185,45 @@ export function registerCopy() {
name: names.COPY,
preconditionFn(workspace, scope) {
const focused = scope.focusedNode;
if (!(focused instanceof BlockSvg)) return false;
Copy link
Collaborator

@cpcallen cpcallen Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prevents anything except blocks—e.g., workspace comments—from being copied.


const targetWorkspace = workspace.isFlyout
? workspace.targetWorkspace
: workspace;
return (
!workspace.isReadOnly() &&
!Gesture.inProgress() &&
!!focused &&
isCopyable(focused) &&
!getFocusManager().ephemeralFocusTaken()
!!targetWorkspace &&
!targetWorkspace.isReadOnly() &&
Copy link
Collaborator

@cpcallen cpcallen Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me that the main workspace being readonly should prevent blocks from being copied from the flyout.

(It also isn't clear to me that it should prevent blocks from being copied from the main workspace either—but that was the previous behaviour so leaving it as-is for the time being is fine.)

!targetWorkspace.isDragging() &&
!getFocusManager().ephemeralFocusTaken() &&
isCopyable(focused)
);
},
callback(workspace, e, shortcut, scope) {
// Prevent the default copy behavior, which may beep or otherwise indicate
// an error due to the lack of a selection.
e.preventDefault();
workspace.hideChaff();

const focused = scope.focusedNode;
if (!focused || !isICopyable(focused)) return false;
copyData = focused.toCopyData();
copyWorkspace =
if (!focused || !isCopyable(focused)) return false;
let targetWorkspace: WorkspaceSvg | null =
focused.workspace instanceof WorkspaceSvg
? focused.workspace
: workspace;
copyCoords = isDraggable(focused)
? focused.getRelativeToSurfaceXY()
: null;
targetWorkspace = targetWorkspace.isFlyout
? targetWorkspace.targetWorkspace
: targetWorkspace;
if (!targetWorkspace) return false;

if (!focused.workspace.isFlyout) {
targetWorkspace.hideChaff();
}
copyData = focused.toCopyData();
copyWorkspace = targetWorkspace;
copyCoords =
isDraggable(focused) && focused.workspace == targetWorkspace
? focused.getRelativeToSurfaceXY()
: null;
return !!copyData;
},
keyCodes: [ctrlC, metaC],
Expand All @@ -197,14 +247,11 @@ export function registerCut() {
preconditionFn(workspace, scope) {
const focused = scope.focusedNode;
return (
!workspace.isReadOnly() &&
!Gesture.inProgress() &&
!!focused &&
isCopyable(focused) &&
// Extra criteria for cut (not just copy):
!focused.workspace.isFlyout &&
focused.isDeletable() &&
!getFocusManager().ephemeralFocusTaken()
!workspace.isReadOnly() &&
!workspace.isDragging() &&
!getFocusManager().ephemeralFocusTaken() &&
isCuttable(focused)
);
},
callback(workspace, e, shortcut, scope) {
Expand Down Expand Up @@ -251,9 +298,14 @@ export function registerPaste() {
const pasteShortcut: KeyboardShortcut = {
name: names.PASTE,
preconditionFn(workspace) {
const targetWorkspace = workspace.isFlyout
? workspace.targetWorkspace
: workspace;
return (
!workspace.isReadOnly() &&
!Gesture.inProgress() &&
!!copyData &&
!!targetWorkspace &&
!targetWorkspace.isReadOnly() &&
!targetWorkspace.isDragging() &&
!getFocusManager().ephemeralFocusTaken()
);
},
Expand Down
12 changes: 7 additions & 5 deletions tests/mocha/shortcut_items_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,13 +181,13 @@ suite('Keyboard Shortcut Items', function () {
runReadOnlyTest(keyEvent, testCaseName);
});
});
// Do not copy a block if a gesture is in progress.
suite('Gesture in progress', function () {
// Do not copy a block if a drag is in progress.
suite('Drag in progress', function () {
testCases.forEach(function (testCase) {
const testCaseName = testCase[0];
const keyEvent = testCase[1];
test(testCaseName, function () {
sinon.stub(Blockly.Gesture, 'inProgress').returns(true);
sinon.stub(this.workspace, 'isDragging').returns(true);
this.injectionDiv.dispatchEvent(keyEvent);
sinon.assert.notCalled(this.copySpy);
sinon.assert.notCalled(this.hideChaffSpy);
Expand All @@ -201,7 +201,7 @@ suite('Keyboard Shortcut Items', function () {
const keyEvent = testCase[1];
test(testCaseName, function () {
sinon
.stub(Blockly.common.getSelected(), 'isDeletable')
.stub(Blockly.common.getSelected(), 'isOwnDeletable')
.returns(false);
this.injectionDiv.dispatchEvent(keyEvent);
sinon.assert.notCalled(this.copySpy);
Expand All @@ -215,7 +215,9 @@ suite('Keyboard Shortcut Items', function () {
const testCaseName = testCase[0];
const keyEvent = testCase[1];
test(testCaseName, function () {
sinon.stub(Blockly.common.getSelected(), 'isMovable').returns(false);
sinon
.stub(Blockly.common.getSelected(), 'isOwnMovable')
.returns(false);
this.injectionDiv.dispatchEvent(keyEvent);
sinon.assert.notCalled(this.copySpy);
sinon.assert.notCalled(this.hideChaffSpy);
Expand Down