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

* Work on cut/copy/paste preconditions

* Cleanup and fixes to cut/copy/paste

* Fix tests

* Remove editable check from isCopyable and isCuttable

Author: RoboErikG

core/shortcut_items.ts

@@ -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.
@@ -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
@@ -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()
+  );
+}
+
+/**
+ * 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 {
+  if (!(focused instanceof BlockSvg)) return false;
   return (
     isICopyable(focused) &&
     isIDeletable(focused) &&
@@ -151,29 +185,45 @@ 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
+        : workspace;
       return (
-        !workspace.isReadOnly() &&
-        !Gesture.inProgress() &&
         !!focused &&
-        isCopyable(focused) &&
-        !getFocusManager().ephemeralFocusTaken()
+        !!targetWorkspace &&
+        !targetWorkspace.isReadOnly() &&
+        !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],
@@ -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) {
@@ -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()
       );
     },

tests/mocha/shortcut_items_test.js

@@ -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);
@@ -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);
@@ -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);