From 3ccfba9c4b7d3953caec14a3cfc347c0205c9c06 Mon Sep 17 00:00:00 2001 From: Maribeth Moffatt Date: Fri, 30 May 2025 09:42:23 -0700 Subject: [PATCH 1/4] feat: ephemeral focus public getter, use in shortcut precondition (#9110) --- core/focus_manager.ts | 7 +++++++ core/shortcut_items.ts | 29 +++++++++++++++++++++++------ 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/core/focus_manager.ts b/core/focus_manager.ts index 01be4813f..3d0a9347f 100644 --- a/core/focus_manager.ts +++ b/core/focus_manager.ts @@ -458,6 +458,13 @@ export class FocusManager { }; } + /** + * @returns whether something is currently holding ephemeral focus + */ + ephemeralFocusTaken(): boolean { + return this.currentlyHoldsEphemeralFocus; + } + /** * Ensures that the manager is currently allowing operations that change its * internal focus state (such as via focusNode()). diff --git a/core/shortcut_items.ts b/core/shortcut_items.ts index c175637c6..fb1ca0fce 100644 --- a/core/shortcut_items.ts +++ b/core/shortcut_items.ts @@ -9,6 +9,7 @@ import {BlockSvg} from './block_svg.js'; import * as clipboard from './clipboard.js'; import * as eventUtils from './events/utils.js'; +import {getFocusManager} from './focus_manager.js'; import {Gesture} from './gesture.js'; import { ICopyable, @@ -72,7 +73,9 @@ export function registerDelete() { focused != null && isIDeletable(focused) && focused.isDeletable() && - !Gesture.inProgress() + !Gesture.inProgress() && + // Don't delete the block if a field editor is open + !getFocusManager().ephemeralFocusTaken() ); }, callback(workspace, e, shortcut, scope) { @@ -152,7 +155,8 @@ export function registerCopy() { !workspace.isReadOnly() && !Gesture.inProgress() && !!focused && - isCopyable(focused) + isCopyable(focused) && + !getFocusManager().ephemeralFocusTaken() ); }, callback(workspace, e, shortcut, scope) { @@ -199,7 +203,8 @@ export function registerCut() { isCopyable(focused) && // Extra criteria for cut (not just copy): !focused.workspace.isFlyout && - focused.isDeletable() + focused.isDeletable() && + !getFocusManager().ephemeralFocusTaken() ); }, callback(workspace, e, shortcut, scope) { @@ -246,7 +251,11 @@ export function registerPaste() { const pasteShortcut: KeyboardShortcut = { name: names.PASTE, preconditionFn(workspace) { - return !workspace.isReadOnly() && !Gesture.inProgress(); + return ( + !workspace.isReadOnly() && + !Gesture.inProgress() && + !getFocusManager().ephemeralFocusTaken() + ); }, callback(workspace: WorkspaceSvg, e: Event) { if (!copyData || !copyWorkspace) return false; @@ -305,7 +314,11 @@ export function registerUndo() { const undoShortcut: KeyboardShortcut = { name: names.UNDO, preconditionFn(workspace) { - return !workspace.isReadOnly() && !Gesture.inProgress(); + return ( + !workspace.isReadOnly() && + !Gesture.inProgress() && + !getFocusManager().ephemeralFocusTaken() + ); }, callback(workspace, e) { // 'z' for undo 'Z' is for redo. @@ -340,7 +353,11 @@ export function registerRedo() { const redoShortcut: KeyboardShortcut = { name: names.REDO, preconditionFn(workspace) { - return !Gesture.inProgress() && !workspace.isReadOnly(); + return ( + !Gesture.inProgress() && + !workspace.isReadOnly() && + !getFocusManager().ephemeralFocusTaken() + ); }, callback(workspace, e) { // 'z' for undo 'Z' is for redo. From fdffd6558b7a2618c5aae48070be987cb921478a Mon Sep 17 00:00:00 2001 From: RoboErikG Date: Fri, 30 May 2025 10:49:30 -0700 Subject: [PATCH 2/4] 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 --- core/shortcut_items.ts | 98 +++++++++++++++++++++++------- tests/mocha/shortcut_items_test.js | 12 ++-- 2 files changed, 82 insertions(+), 28 deletions(-) diff --git a/core/shortcut_items.ts b/core/shortcut_items.ts index fb1ca0fce..161d5fceb 100644 --- a/core/shortcut_items.ts +++ b/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 & 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() ); }, diff --git a/tests/mocha/shortcut_items_test.js b/tests/mocha/shortcut_items_test.js index 29487ec58..622df9efc 100644 --- a/tests/mocha/shortcut_items_test.js +++ b/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); From d1b17d1f900b89ce7fee8d9a631f79dde7bd35ac Mon Sep 17 00:00:00 2001 From: Maribeth Moffatt Date: Fri, 30 May 2025 11:00:56 -0700 Subject: [PATCH 3/4] fix: context menus on flyout (#9116) --- core/workspace_svg.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index 5d5a40ccc..9eb5ea545 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -2806,10 +2806,11 @@ export class WorkspaceSvg /** See IFocusableTree.onTreeBlur. */ onTreeBlur(nextTree: IFocusableTree | null): void { // If the flyout loses focus, make sure to close it unless focus is being - // lost to the toolbox. + // lost to the toolbox or ephemeral focus. if (this.isFlyout && this.targetWorkspace) { // Only hide the flyout if the flyout's workspace is losing focus and that - // focus isn't returning to the flyout itself or the toolbox. + // focus isn't returning to the flyout itself, the toolbox, or ephemeral. + if (getFocusManager().ephemeralFocusTaken()) return; const flyout = this.targetWorkspace.getFlyout(); const toolbox = this.targetWorkspace.getToolbox(); if (toolbox && nextTree === toolbox) return; From cb0824736b9a95a073c92f7b8d72b03ed9fe44eb Mon Sep 17 00:00:00 2001 From: Aaron Dodson Date: Fri, 30 May 2025 13:09:52 -0700 Subject: [PATCH 4/4] fix: Fix bug that caused the focus manager to attempt to focus unfocusable elements. (#9117) --- core/layer_manager.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/core/layer_manager.ts b/core/layer_manager.ts index 1d5afdd74..fd7d8fe23 100644 --- a/core/layer_manager.ts +++ b/core/layer_manager.ts @@ -104,9 +104,11 @@ export class LayerManager { moveToDragLayer(elem: IRenderedElement & IFocusableNode) { this.dragLayer?.appendChild(elem.getSvgRoot()); - // Since moving the element to the drag layer will cause it to lose focus, - // ensure it regains focus (to ensure proper highlights & sent events). - getFocusManager().focusNode(elem); + if (elem.canBeFocused()) { + // Since moving the element to the drag layer will cause it to lose focus, + // ensure it regains focus (to ensure proper highlights & sent events). + getFocusManager().focusNode(elem); + } } /** @@ -117,9 +119,11 @@ export class LayerManager { moveOffDragLayer(elem: IRenderedElement & IFocusableNode, layerNum: number) { this.append(elem, layerNum); - // Since moving the element off the drag layer will cause it to lose focus, - // ensure it regains focus (to ensure proper highlights & sent events). - getFocusManager().focusNode(elem); + if (elem.canBeFocused()) { + // Since moving the element off the drag layer will cause it to lose focus, + // ensure it regains focus (to ensure proper highlights & sent events). + getFocusManager().focusNode(elem); + } } /**