diff --git a/core/block.ts b/core/block.ts index 43bc6bbc5..9f7c11d4f 100644 --- a/core/block.ts +++ b/core/block.ts @@ -791,6 +791,7 @@ export class Block { isDeletable(): boolean { return ( this.deletable && + !this.isInFlyout && !this.shadow && !this.isDeadOrDying() && !this.workspace.isReadOnly() @@ -824,6 +825,7 @@ export class Block { isMovable(): boolean { return ( this.movable && + !this.isInFlyout && !this.shadow && !this.isDeadOrDying() && !this.workspace.isReadOnly() diff --git a/core/block_svg.ts b/core/block_svg.ts index 8ea26e354..a30cc34ed 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -1721,6 +1721,11 @@ export class BlockSvg this.dragStrategy = dragStrategy; } + /** Returns whether this block is copyable or not. */ + isCopyable(): boolean { + return this.isOwnDeletable() && this.isOwnMovable(); + } + /** Returns whether this block is movable or not. */ override isMovable(): boolean { return this.dragStrategy.isMovable(); diff --git a/core/comments/rendered_workspace_comment.ts b/core/comments/rendered_workspace_comment.ts index 3a3d57a44..42fb1fda4 100644 --- a/core/comments/rendered_workspace_comment.ts +++ b/core/comments/rendered_workspace_comment.ts @@ -244,6 +244,11 @@ export class RenderedWorkspaceComment } } + /** Returns whether this comment is copyable or not */ + isCopyable(): boolean { + return this.isOwnMovable() && this.isOwnDeletable(); + } + /** Returns whether this comment is movable or not. */ isMovable(): boolean { return this.dragStrategy.isMovable(); diff --git a/core/comments/workspace_comment.ts b/core/comments/workspace_comment.ts index 190efd64d..b5dc3023c 100644 --- a/core/comments/workspace_comment.ts +++ b/core/comments/workspace_comment.ts @@ -165,7 +165,11 @@ export class WorkspaceComment { * workspace is read-only. */ isMovable() { - return this.isOwnMovable() && !this.workspace.isReadOnly(); + return ( + this.isOwnMovable() && + !this.workspace.isReadOnly() && + !this.workspace.isFlyout + ); } /** @@ -189,7 +193,8 @@ export class WorkspaceComment { return ( this.isOwnDeletable() && !this.isDeadOrDying() && - !this.workspace.isReadOnly() + !this.workspace.isReadOnly() && + !this.workspace.isFlyout ); } diff --git a/core/interfaces/i_copyable.ts b/core/interfaces/i_copyable.ts index b653bd20a..6c354926a 100644 --- a/core/interfaces/i_copyable.ts +++ b/core/interfaces/i_copyable.ts @@ -15,6 +15,14 @@ export interface ICopyable extends ISelectable { * @returns Copy metadata. */ toCopyData(): T | null; + + /** + * Whether this instance is currently copyable. The standard implementation + * is to return true if isOwnDeletable and isOwnMovable return true. + * + * @returns True if it can currently be copied. + */ + isCopyable?(): boolean; } export namespace ICopyable { @@ -25,7 +33,7 @@ export namespace ICopyable { export type ICopyData = ICopyable.ICopyData; -/** @returns true if the given object is copyable. */ +/** @returns true if the given object is an ICopyable. */ export function isCopyable(obj: any): obj is ICopyable { return obj.toCopyData !== undefined; } diff --git a/core/shortcut_items.ts b/core/shortcut_items.ts index 161d5fceb..25295e417 100644 --- a/core/shortcut_items.ts +++ b/core/shortcut_items.ts @@ -8,19 +8,13 @@ import {BlockSvg} from './block_svg.js'; import * as clipboard from './clipboard.js'; +import {RenderedWorkspaceComment} from './comments.js'; import * as eventUtils from './events/utils.js'; import {getFocusManager} from './focus_manager.js'; import {Gesture} from './gesture.js'; -import { - ICopyable, - ICopyData, - isCopyable as isICopyable, -} from './interfaces/i_copyable.js'; -import { - IDeletable, - isDeletable as isIDeletable, -} from './interfaces/i_deletable.js'; -import {IDraggable, isDraggable} from './interfaces/i_draggable.js'; +import {ICopyData, isCopyable as isICopyable} from './interfaces/i_copyable.js'; +import {isDeletable as isIDeletable} from './interfaces/i_deletable.js'; +import {isDraggable} from './interfaces/i_draggable.js'; import {IFocusableNode} from './interfaces/i_focusable_node.js'; import {KeyboardShortcut, ShortcutRegistry} from './shortcut_registry.js'; import {Coordinate} from './utils/coordinate.js'; @@ -100,74 +94,43 @@ export function registerDelete() { } let copyData: ICopyData | null = null; -let copyWorkspace: WorkspaceSvg | null = null; let copyCoords: Coordinate | null = null; /** * 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. - * 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 .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 - * be other circumstances were it is desirable to allow movable / - * copyable copies of a currently-unmovable / -copyable block to be - * made. + * This will use the isCopyable method if the node implements it, otherwise + * it will fall back to checking if the node is deletable and draggable not + * considering the workspace's edit state. * * @param focused The focused object. */ -function isCopyable( - focused: IFocusableNode, -): focused is ICopyable & IDeletable & IDraggable { - if (!(focused instanceof BlockSvg)) return false; - return ( - isICopyable(focused) && - isIDeletable(focused) && - focused.isOwnDeletable() && - isDraggable(focused) && - focused.isOwnMovable() - ); +function isCopyable(focused: IFocusableNode): boolean { + if (!isICopyable(focused) || !isIDeletable(focused) || !isDraggable(focused)) + return false; + if (focused.isCopyable) { + return focused.isCopyable(); + } else if ( + focused instanceof BlockSvg || + focused instanceof RenderedWorkspaceComment + ) { + return focused.isOwnDeletable() && focused.isOwnMovable(); + } + // This isn't a class Blockly knows about, so fall back to the stricter + // checks for deletable and movable. + return focused.isDeletable() && focused.isMovable(); } /** * 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. + * This will check if the node can be both copied and deleted in its current + * workspace. * * @param focused The focused object. */ function isCuttable(focused: IFocusableNode): boolean { - if (!(focused instanceof BlockSvg)) return false; - return ( - isICopyable(focused) && - isIDeletable(focused) && - focused.isDeletable() && - isDraggable(focused) && - focused.isMovable() - ); + return isCopyable(focused) && isIDeletable(focused) && focused.isDeletable(); } /** @@ -185,7 +148,6 @@ export function registerCopy() { name: names.COPY, preconditionFn(workspace, scope) { const focused = scope.focusedNode; - if (!(focused instanceof BlockSvg)) return false; const targetWorkspace = workspace.isFlyout ? workspace.targetWorkspace @@ -193,7 +155,6 @@ export function registerCopy() { return ( !!focused && !!targetWorkspace && - !targetWorkspace.isReadOnly() && !targetWorkspace.isDragging() && !getFocusManager().ephemeralFocusTaken() && isCopyable(focused) @@ -205,21 +166,17 @@ export function registerCopy() { e.preventDefault(); const focused = scope.focusedNode; - if (!focused || !isCopyable(focused)) return false; - let targetWorkspace: WorkspaceSvg | null = - focused.workspace instanceof WorkspaceSvg - ? focused.workspace - : workspace; - targetWorkspace = targetWorkspace.isFlyout - ? targetWorkspace.targetWorkspace - : targetWorkspace; + if (!focused || !isICopyable(focused) || !isCopyable(focused)) + return false; + const targetWorkspace = workspace.isFlyout + ? workspace.targetWorkspace + : workspace; if (!targetWorkspace) return false; if (!focused.workspace.isFlyout) { targetWorkspace.hideChaff(); } copyData = focused.toCopyData(); - copyWorkspace = targetWorkspace; copyCoords = isDraggable(focused) && focused.workspace == targetWorkspace ? focused.getRelativeToSurfaceXY() @@ -256,27 +213,20 @@ export function registerCut() { }, callback(workspace, e, shortcut, scope) { const focused = scope.focusedNode; + if (!focused || !isCuttable(focused) || !isICopyable(focused)) { + return false; + } + copyData = focused.toCopyData(); + copyCoords = isDraggable(focused) + ? focused.getRelativeToSurfaceXY() + : null; if (focused instanceof BlockSvg) { - copyData = focused.toCopyData(); - copyWorkspace = workspace; - copyCoords = focused.getRelativeToSurfaceXY(); focused.checkAndDelete(); - return true; - } else if ( - isIDeletable(focused) && - focused.isDeletable() && - isICopyable(focused) - ) { - copyData = focused.toCopyData(); - copyWorkspace = workspace; - copyCoords = isDraggable(focused) - ? focused.getRelativeToSurfaceXY() - : null; + } else if (isIDeletable(focused)) { focused.dispose(); - return true; } - return false; + return !!copyData; }, keyCodes: [ctrlX, metaX], }; @@ -310,7 +260,11 @@ export function registerPaste() { ); }, callback(workspace: WorkspaceSvg, e: Event) { - if (!copyData || !copyWorkspace) return false; + if (!copyData) return false; + const targetWorkspace = workspace.isFlyout + ? workspace.targetWorkspace + : workspace; + if (!targetWorkspace || targetWorkspace.isReadOnly()) return false; if (e instanceof PointerEvent) { // The event that triggers a shortcut would conventionally be a KeyboardEvent. @@ -319,19 +273,19 @@ export function registerPaste() { // at the mouse coordinates where the menu was opened, and this PointerEvent // is where the menu was opened. const mouseCoords = svgMath.screenToWsCoordinates( - copyWorkspace, + targetWorkspace, new Coordinate(e.clientX, e.clientY), ); - return !!clipboard.paste(copyData, copyWorkspace, mouseCoords); + return !!clipboard.paste(copyData, targetWorkspace, mouseCoords); } if (!copyCoords) { // If we don't have location data about the original copyable, let the // paster determine position. - return !!clipboard.paste(copyData, copyWorkspace); + return !!clipboard.paste(copyData, targetWorkspace); } - const {left, top, width, height} = copyWorkspace + const {left, top, width, height} = targetWorkspace .getMetricsManager() .getViewMetrics(true); const viewportRect = new Rect(top, top + height, left, left + width); @@ -339,12 +293,12 @@ export function registerPaste() { if (viewportRect.contains(copyCoords.x, copyCoords.y)) { // If the original copyable is inside the viewport, let the paster // determine position. - return !!clipboard.paste(copyData, copyWorkspace); + return !!clipboard.paste(copyData, targetWorkspace); } // Otherwise, paste in the middle of the viewport. const centerCoords = new Coordinate(left + width / 2, top + height / 2); - return !!clipboard.paste(copyData, copyWorkspace, centerCoords); + return !!clipboard.paste(copyData, targetWorkspace, centerCoords); }, keyCodes: [ctrlV, metaV], }; diff --git a/tests/mocha/shortcut_items_test.js b/tests/mocha/shortcut_items_test.js index 622df9efc..4ab83d8e1 100644 --- a/tests/mocha/shortcut_items_test.js +++ b/tests/mocha/shortcut_items_test.js @@ -47,6 +47,16 @@ suite('Keyboard Shortcut Items', function () { .returns(block.nextConnection); } + /** + * Creates a workspace comment and set it as the focused node. + * @param {Blockly.Workspace} workspace The workspace to create a new comment on. + */ + function setSelectedComment(workspace) { + const comment = workspace.newComment(); + sinon.stub(Blockly.getFocusManager(), 'getFocusedNode').returns(comment); + return comment; + } + /** * Creates a test for not running keyDown events when the workspace is in read only mode. * @param {Object} keyEvent Mocked key down event. Use createKeyDownEvent. @@ -173,12 +183,17 @@ suite('Keyboard Shortcut Items', function () { }); }); }); - // Do not copy a block if a workspace is in readonly mode. - suite('Not called when readOnly is true', function () { + // Allow copying a block if a workspace is in readonly mode. + suite('Called when readOnly is true', function () { testCases.forEach(function (testCase) { const testCaseName = testCase[0]; const keyEvent = testCase[1]; - runReadOnlyTest(keyEvent, testCaseName); + test(testCaseName, function () { + this.workspace.setIsReadOnly(true); + this.injectionDiv.dispatchEvent(keyEvent); + sinon.assert.calledOnce(this.copySpy); + sinon.assert.calledOnce(this.hideChaffSpy); + }); }); }); // Do not copy a block if a drag is in progress. @@ -236,6 +251,152 @@ suite('Keyboard Shortcut Items', function () { sinon.assert.notCalled(this.copySpy); sinon.assert.notCalled(this.hideChaffSpy); }); + // Copy a comment. + test('Workspace comment', function () { + testCases.forEach(function (testCase) { + const testCaseName = testCase[0]; + const keyEvent = testCase[1]; + test(testCaseName, function () { + Blockly.getFocusManager().getFocusedNode.restore(); + this.comment = setSelectedComment(this.workspace); + this.copySpy = sinon.spy(this.comment, 'toCopyData'); + + this.injectionDiv.dispatchEvent(keyEvent); + sinon.assert.calledOnce(this.copySpy); + sinon.assert.calledOnce(this.hideChaffSpy); + }); + }); + }); + }); + + suite('Cut', function () { + setup(function () { + this.block = setSelectedBlock(this.workspace); + this.copySpy = sinon.spy(this.block, 'toCopyData'); + this.disposeSpy = sinon.spy(this.block, 'dispose'); + this.hideChaffSpy = sinon.spy( + Blockly.WorkspaceSvg.prototype, + 'hideChaff', + ); + }); + const testCases = [ + [ + 'Control X', + createKeyDownEvent(Blockly.utils.KeyCodes.X, [ + Blockly.utils.KeyCodes.CTRL, + ]), + ], + [ + 'Meta X', + createKeyDownEvent(Blockly.utils.KeyCodes.X, [ + Blockly.utils.KeyCodes.META, + ]), + ], + ]; + // Cut a block. + suite('Simple', function () { + testCases.forEach(function (testCase) { + const testCaseName = testCase[0]; + const keyEvent = testCase[1]; + test(testCaseName, function () { + this.injectionDiv.dispatchEvent(keyEvent); + sinon.assert.calledOnce(this.copySpy); + sinon.assert.calledOnce(this.disposeSpy); + sinon.assert.calledOnce(this.hideChaffSpy); + }); + }); + }); + // Do not cut a block if a workspace is in readonly mode. + suite('Not called when readOnly is true', function () { + testCases.forEach(function (testCase) { + const testCaseName = testCase[0]; + const keyEvent = testCase[1]; + test(testCaseName, function () { + this.workspace.setIsReadOnly(true); + this.injectionDiv.dispatchEvent(keyEvent); + sinon.assert.notCalled(this.copySpy); + sinon.assert.notCalled(this.disposeSpy); + sinon.assert.notCalled(this.hideChaffSpy); + }); + }); + }); + // Do not cut 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(this.workspace, 'isDragging').returns(true); + this.injectionDiv.dispatchEvent(keyEvent); + sinon.assert.notCalled(this.copySpy); + sinon.assert.notCalled(this.disposeSpy); + sinon.assert.notCalled(this.hideChaffSpy); + }); + }); + }); + // Do not cut a block if is is not deletable. + suite('Block is not deletable', function () { + testCases.forEach(function (testCase) { + const testCaseName = testCase[0]; + const keyEvent = testCase[1]; + test(testCaseName, function () { + sinon + .stub(Blockly.common.getSelected(), 'isOwnDeletable') + .returns(false); + this.injectionDiv.dispatchEvent(keyEvent); + sinon.assert.notCalled(this.copySpy); + sinon.assert.notCalled(this.disposeSpy); + sinon.assert.notCalled(this.hideChaffSpy); + }); + }); + }); + // Do not cut a block if it is not movable. + suite('Block is not movable', function () { + testCases.forEach(function (testCase) { + const testCaseName = testCase[0]; + const keyEvent = testCase[1]; + test(testCaseName, function () { + sinon + .stub(Blockly.common.getSelected(), 'isOwnMovable') + .returns(false); + this.injectionDiv.dispatchEvent(keyEvent); + sinon.assert.notCalled(this.copySpy); + sinon.assert.notCalled(this.disposeSpy); + sinon.assert.notCalled(this.hideChaffSpy); + }); + }); + }); + test('Not called when connection is focused', function () { + // Restore the stub behavior called during setup + Blockly.getFocusManager().getFocusedNode.restore(); + + setSelectedConnection(this.workspace); + const event = createKeyDownEvent(Blockly.utils.KeyCodes.C, [ + Blockly.utils.KeyCodes.CTRL, + ]); + this.injectionDiv.dispatchEvent(event); + sinon.assert.notCalled(this.copySpy); + sinon.assert.notCalled(this.disposeSpy); + sinon.assert.notCalled(this.hideChaffSpy); + }); + + // Cut a comment. + suite('Workspace comment', function () { + testCases.forEach(function (testCase) { + const testCaseName = testCase[0]; + const keyEvent = testCase[1]; + test(testCaseName, function () { + Blockly.getFocusManager().getFocusedNode.restore(); + this.comment = setSelectedComment(this.workspace); + this.copySpy = sinon.spy(this.comment, 'toCopyData'); + this.disposeSpy = sinon.spy(this.comment, 'dispose'); + + this.injectionDiv.dispatchEvent(keyEvent); + sinon.assert.calledOnce(this.copySpy); + sinon.assert.calledOnce(this.disposeSpy); + }); + }); + }); }); suite('Undo', function () {