diff --git a/package-lock.json b/package-lock.json index 90baf06d..4a68f389 100644 --- a/package-lock.json +++ b/package-lock.json @@ -26,6 +26,7 @@ "eslint-plugin-jsdoc": "^46.8.2", "globals": "^15.4.0", "html-webpack-plugin": "^5.6.0", + "jsdom-global": "^3.0.2", "mocha": "^11.1.0", "p5": "^1.10.0", "prettier": "^3.3.1", @@ -6693,6 +6694,15 @@ } } }, + "node_modules/jsdom-global": { + "version": "3.0.2", + "resolved": "https://registry.npmjs.org/jsdom-global/-/jsdom-global-3.0.2.tgz", + "integrity": "sha512-t1KMcBkz/pT5JrvcJbpUR2u/w1kO9jXctaaGJ0vZDzwFnIvGWw9IDSRciT83kIs8Bnw4qpOl8bQK08V01YgMPg==", + "dev": true, + "peerDependencies": { + "jsdom": ">=10.0.0" + } + }, "node_modules/jsesc": { "version": "2.5.2", "resolved": "https://registry.npmjs.org/jsesc/-/jsesc-2.5.2.tgz", diff --git a/package.json b/package.json index 48810466..26ba4d39 100644 --- a/package.json +++ b/package.json @@ -14,7 +14,9 @@ "predeploy": "blockly-scripts predeploy", "prepublishOnly": "npm login --registry https://wombat-dressing-room.appspot.com", "start": "blockly-scripts start", - "test": "npm run wdio:clean && npm run wdio:build && npm run wdio:run", + "test": "npm run test:mocha && npm run test:wdio", + "test:mocha": "blockly-scripts test", + "test:wdio": "npm run wdio:clean && npm run wdio:run", "wdio:build": "npm run wdio:build:app && npm run wdio:build:tests", "wdio:build:app": "cd test/webdriverio && webpack", "wdio:build:tests": "tsc -p ./test/webdriverio/test/tsconfig.json", @@ -65,6 +67,7 @@ "eslint-plugin-jsdoc": "^46.8.2", "globals": "^15.4.0", "html-webpack-plugin": "^5.6.0", + "jsdom-global": "^3.0.2", "mocha": "^11.1.0", "p5": "^1.10.0", "prettier": "^3.3.1", diff --git a/src/actions/clipboard.ts b/src/actions/clipboard.ts index ad67833b..2f2eb097 100644 --- a/src/actions/clipboard.ts +++ b/src/actions/clipboard.ts @@ -60,9 +60,7 @@ export class Clipboard { /** * Uninstall this action as both a keyboard shortcut and a context menu item. - * N. B. This does *not* currently reinstall the original keyboard shortcuts. - * You should manually reinstall the previously registered shortcuts (either - * from core or from another plugin you may be using). + * Reinstall the original cut/copy/paste shortcuts. */ uninstall() { ContextMenuRegistry.registry.unregister('blockCutFromContextMenu'); @@ -72,6 +70,18 @@ export class Clipboard { ShortcutRegistry.registry.unregister(Constants.SHORTCUT_NAMES.CUT); ShortcutRegistry.registry.unregister(Constants.SHORTCUT_NAMES.COPY); ShortcutRegistry.registry.unregister(Constants.SHORTCUT_NAMES.PASTE); + + if (this.oldCutShortcut) { + ShortcutRegistry.registry.register(this.oldCutShortcut); + } + + if (this.oldCopyShortcut) { + ShortcutRegistry.registry.register(this.oldCopyShortcut); + } + + if (this.oldPasteShortcut) { + ShortcutRegistry.registry.register(this.oldPasteShortcut); + } } /** diff --git a/src/actions/edit.ts b/src/actions/edit.ts index 2458a642..6387e128 100644 --- a/src/actions/edit.ts +++ b/src/actions/edit.ts @@ -5,7 +5,7 @@ */ import {ContextMenuRegistry, Msg, keyboardNavigationController} from 'blockly'; -import {Navigation} from 'src/navigation'; +import {Navigation} from '../navigation'; import {getMenuItem} from '../shortcut_formatting'; import * as Constants from '../constants'; diff --git a/src/actions/enter.ts b/src/actions/enter.ts index 1603f15f..374059d0 100644 --- a/src/actions/enter.ts +++ b/src/actions/enter.ts @@ -105,6 +105,7 @@ export class EnterAction { } }, keyCodes: [KeyCodes.ENTER, KeyCodes.SPACE], + allowCollision: true, }); } @@ -116,6 +117,7 @@ export class EnterAction { */ private shouldHandleEnterForWS(workspace: WorkspaceSvg): boolean { if (!this.navigation.canCurrentlyNavigate(workspace)) return false; + if (workspace.isDragging()) return false; const curNode = workspace.getCursor().getCurNode(); if (!curNode) return false; diff --git a/src/actions/move.ts b/src/actions/move.ts index b1146b49..e1ab4499 100644 --- a/src/actions/move.ts +++ b/src/actions/move.ts @@ -33,193 +33,185 @@ const createSerializedKey = ShortcutRegistry.registry.createSerializedKey.bind( export class MoveActions { constructor(private mover: Mover) {} - private shortcutNames: string[] = []; - private menuItemNames: string[] = []; - - private registerShortcuts() { - const shortcuts: ShortcutRegistry.KeyboardShortcut[] = [ - // Begin and end move. - { - name: 'start_move', - preconditionFn: (workspace) => { - const startDraggable = this.getCurrentDraggable(workspace); - return ( - !!startDraggable && this.mover.canMove(workspace, startDraggable) - ); - }, - callback: (workspace) => { - keyboardNavigationController.setIsActive(true); - const startDraggable = this.getCurrentDraggable(workspace); - // Focus the root draggable in case one of its children - // was focused when the move was triggered. - if (startDraggable) { - getFocusManager().focusNode(startDraggable); - } - return ( - !!startDraggable && - this.mover.startMove(workspace, startDraggable, MoveType.Move, null) - ); - }, - keyCodes: [KeyCodes.M], - }, - { - name: 'finish_move', - preconditionFn: (workspace) => this.mover.isMoving(workspace), - callback: (workspace) => this.mover.finishMove(workspace), - keyCodes: [KeyCodes.ENTER, KeyCodes.SPACE], - allowCollision: true, + private shortcuts: ShortcutRegistry.KeyboardShortcut[] = [ + // Begin and end move. + { + name: 'start_move', + preconditionFn: (workspace) => { + const startDraggable = this.getCurrentDraggable(workspace); + return ( + !!startDraggable && this.mover.canMove(workspace, startDraggable) + ); }, - { - name: 'abort_move', - preconditionFn: (workspace) => this.mover.isMoving(workspace), - callback: (workspace) => this.mover.abortMove(workspace), - keyCodes: [KeyCodes.ESC], - allowCollision: true, + callback: (workspace) => { + keyboardNavigationController.setIsActive(true); + const startDraggable = this.getCurrentDraggable(workspace); + // Focus the root draggable in case one of its children + // was focused when the move was triggered. + if (startDraggable) { + getFocusManager().focusNode(startDraggable); + } + return ( + !!startDraggable && + this.mover.startMove(workspace, startDraggable, MoveType.Move, null) + ); }, + keyCodes: [KeyCodes.M], + }, + { + name: 'finish_move', + preconditionFn: (workspace) => this.mover.isMoving(workspace), + callback: (workspace) => this.mover.finishMove(workspace), + keyCodes: [KeyCodes.ENTER, KeyCodes.SPACE], + allowCollision: true, + }, + { + name: 'abort_move', + preconditionFn: (workspace) => this.mover.isMoving(workspace), + callback: (workspace) => this.mover.abortMove(workspace), + keyCodes: [KeyCodes.ESC], + allowCollision: true, + }, - // Constrained moves. - { - name: 'move_left_constrained', - preconditionFn: (workspace) => this.mover.isMoving(workspace), - callback: (workspace) => - this.mover.moveConstrained(workspace, Direction.Left), - keyCodes: [KeyCodes.LEFT], - allowCollision: true, - }, - { - name: 'move_right_constrained', - preconditionFn: (workspace) => this.mover.isMoving(workspace), - callback: (workspace) => - this.mover.moveConstrained(workspace, Direction.Right), - keyCodes: [KeyCodes.RIGHT], - allowCollision: true, - }, - { - name: 'move_up_constrained', - preconditionFn: (workspace) => this.mover.isMoving(workspace), - callback: (workspace) => - this.mover.moveConstrained(workspace, Direction.Up), - keyCodes: [KeyCodes.UP], - allowCollision: true, - }, - { - name: 'move_down_constrained', - preconditionFn: (workspace) => this.mover.isMoving(workspace), - callback: (workspace) => - this.mover.moveConstrained(workspace, Direction.Down), - keyCodes: [KeyCodes.DOWN], - allowCollision: true, - }, + // Constrained moves. + { + name: 'move_left_constrained', + preconditionFn: (workspace) => this.mover.isMoving(workspace), + callback: (workspace) => + this.mover.moveConstrained(workspace, Direction.Left), + keyCodes: [KeyCodes.LEFT], + allowCollision: true, + }, + { + name: 'move_right_constrained', + preconditionFn: (workspace) => this.mover.isMoving(workspace), + callback: (workspace) => + this.mover.moveConstrained(workspace, Direction.Right), + keyCodes: [KeyCodes.RIGHT], + allowCollision: true, + }, + { + name: 'move_up_constrained', + preconditionFn: (workspace) => this.mover.isMoving(workspace), + callback: (workspace) => + this.mover.moveConstrained(workspace, Direction.Up), + keyCodes: [KeyCodes.UP], + allowCollision: true, + }, + { + name: 'move_down_constrained', + preconditionFn: (workspace) => this.mover.isMoving(workspace), + callback: (workspace) => + this.mover.moveConstrained(workspace, Direction.Down), + keyCodes: [KeyCodes.DOWN], + allowCollision: true, + }, + + // Unconstrained moves. + { + name: 'move_left_unconstrained', + preconditionFn: (workspace) => this.mover.isMoving(workspace), + callback: (workspace) => + this.mover.moveUnconstrained(workspace, Direction.Left), + keyCodes: [ + createSerializedKey(KeyCodes.LEFT, [KeyCodes.ALT]), + createSerializedKey(KeyCodes.LEFT, [KeyCodes.CTRL]), + ], + }, + { + name: 'move_right_unconstrained', + preconditionFn: (workspace) => this.mover.isMoving(workspace), + callback: (workspace) => + this.mover.moveUnconstrained(workspace, Direction.Right), + keyCodes: [ + createSerializedKey(KeyCodes.RIGHT, [KeyCodes.ALT]), + createSerializedKey(KeyCodes.RIGHT, [KeyCodes.CTRL]), + ], + }, + { + name: 'move_up_unconstrained', + preconditionFn: (workspace) => this.mover.isMoving(workspace), + callback: (workspace) => + this.mover.moveUnconstrained(workspace, Direction.Up), + keyCodes: [ + createSerializedKey(KeyCodes.UP, [KeyCodes.ALT]), + createSerializedKey(KeyCodes.UP, [KeyCodes.CTRL]), + ], + }, + { + name: 'move_down_unconstrained', + preconditionFn: (workspace) => this.mover.isMoving(workspace), + callback: (workspace) => + this.mover.moveUnconstrained(workspace, Direction.Down), + keyCodes: [ + createSerializedKey(KeyCodes.DOWN, [KeyCodes.ALT]), + createSerializedKey(KeyCodes.DOWN, [KeyCodes.CTRL]), + ], + }, + ]; - // Unconstrained moves. - { - name: 'move_left_unconstrained', - preconditionFn: (workspace) => this.mover.isMoving(workspace), - callback: (workspace) => - this.mover.moveUnconstrained(workspace, Direction.Left), - keyCodes: [ - createSerializedKey(KeyCodes.LEFT, [KeyCodes.ALT]), - createSerializedKey(KeyCodes.LEFT, [KeyCodes.CTRL]), - ], + private menuItems: ContextMenuRegistry.RegistryItem[] = [ + { + displayText: () => getMenuItem(Msg['MOVE_BLOCK'], 'start_move'), + preconditionFn: (scope, menuOpenEvent) => { + const workspace = scope.block?.workspace as WorkspaceSvg | null; + if (!workspace || menuOpenEvent instanceof PointerEvent) + return 'hidden'; + + const startDraggable = this.getCurrentDraggable(workspace); + return !!startDraggable && this.mover.canMove(workspace, startDraggable) + ? 'enabled' + : 'disabled'; }, - { - name: 'move_right_unconstrained', - preconditionFn: (workspace) => this.mover.isMoving(workspace), - callback: (workspace) => - this.mover.moveUnconstrained(workspace, Direction.Right), - keyCodes: [ - createSerializedKey(KeyCodes.RIGHT, [KeyCodes.ALT]), - createSerializedKey(KeyCodes.RIGHT, [KeyCodes.CTRL]), - ], + callback: (scope) => { + const workspace = scope.block?.workspace as WorkspaceSvg | null; + if (!workspace) return false; + const startDraggable = this.getCurrentDraggable(workspace); + // Focus the start block in case one of its fields or a shadow block + // was focused when the move was triggered. + if (startDraggable) { + getFocusManager().focusNode(startDraggable); + } + return ( + !!startDraggable && + this.mover.startMove(workspace, startDraggable, MoveType.Move, null) + ); }, - { - name: 'move_up_unconstrained', - preconditionFn: (workspace) => this.mover.isMoving(workspace), - callback: (workspace) => - this.mover.moveUnconstrained(workspace, Direction.Up), - keyCodes: [ - createSerializedKey(KeyCodes.UP, [KeyCodes.ALT]), - createSerializedKey(KeyCodes.UP, [KeyCodes.CTRL]), - ], + scopeType: ContextMenuRegistry.ScopeType.BLOCK, + id: 'move', + weight: 8.5, + }, + { + displayText: () => + getMenuItem(Msg['MOVE_COMMENT'] ?? 'Move Comment', 'start_move'), + preconditionFn: (scope, menuOpenEvent) => { + const comment = scope.comment; + if (!comment) return 'hidden'; + + return this.mover.canMove(comment.workspace, comment) + ? 'enabled' + : 'disabled'; }, - { - name: 'move_down_unconstrained', - preconditionFn: (workspace) => this.mover.isMoving(workspace), - callback: (workspace) => - this.mover.moveUnconstrained(workspace, Direction.Down), - keyCodes: [ - createSerializedKey(KeyCodes.DOWN, [KeyCodes.ALT]), - createSerializedKey(KeyCodes.DOWN, [KeyCodes.CTRL]), - ], + callback: (scope) => { + const comment = scope.comment; + if (!comment) return false; + this.mover.startMove(comment.workspace, comment, MoveType.Move, null); }, - ]; + scopeType: ContextMenuRegistry.ScopeType.COMMENT, + id: 'move_comment', + weight: 8.5, + }, + ]; - for (const shortcut of shortcuts) { + private registerShortcuts() { + for (const shortcut of this.shortcuts) { ShortcutRegistry.registry.register(shortcut); - this.shortcutNames.push(shortcut.name); } } private registerMenuItems() { - const menuItems: ContextMenuRegistry.RegistryItem[] = [ - { - displayText: getMenuItem(Msg['MOVE_BLOCK'], 'start_move'), - preconditionFn: (scope, menuOpenEvent) => { - const workspace = scope.block?.workspace as WorkspaceSvg | null; - if (!workspace || menuOpenEvent instanceof PointerEvent) - return 'hidden'; - - const startDraggable = this.getCurrentDraggable(workspace); - return !!startDraggable && - this.mover.canMove(workspace, startDraggable) - ? 'enabled' - : 'disabled'; - }, - callback: (scope) => { - const workspace = scope.block?.workspace as WorkspaceSvg | null; - if (!workspace) return false; - const startDraggable = this.getCurrentDraggable(workspace); - // Focus the start block in case one of its fields or a shadow block - // was focused when the move was triggered. - if (startDraggable) { - getFocusManager().focusNode(startDraggable); - } - return ( - !!startDraggable && - this.mover.startMove(workspace, startDraggable, MoveType.Move, null) - ); - }, - scopeType: ContextMenuRegistry.ScopeType.BLOCK, - id: 'move', - weight: 8.5, - }, - { - displayText: getMenuItem( - Msg['MOVE_COMMENT'] ?? 'Move Comment', - 'start_move', - ), - preconditionFn: (scope, menuOpenEvent) => { - const comment = scope.comment; - if (!comment) return 'hidden'; - - return this.mover.canMove(comment.workspace, comment) - ? 'enabled' - : 'disabled'; - }, - callback: (scope) => { - const comment = scope.comment; - if (!comment) return false; - this.mover.startMove(comment.workspace, comment, MoveType.Move, null); - }, - scopeType: ContextMenuRegistry.ScopeType.COMMENT, - id: 'move_comment', - weight: 8.5, - }, - ]; - - for (const menuItem of menuItems) { + for (const menuItem of this.menuItems) { ContextMenuRegistry.registry.register(menuItem); - this.menuItemNames.push(menuItem.id); } } @@ -236,11 +228,11 @@ export class MoveActions { * Uninstall these actions. */ uninstall() { - for (const shortcut of this.shortcutNames) { - ShortcutRegistry.registry.unregister(shortcut); + for (const shortcut of this.shortcuts) { + ShortcutRegistry.registry.unregister(shortcut.name); } - for (const menuItem of this.menuItemNames) { - ContextMenuRegistry.registry.unregister(menuItem); + for (const menuItem of this.menuItems) { + ContextMenuRegistry.registry.unregister(menuItem.id); } } diff --git a/src/actions/mover.ts b/src/actions/mover.ts index 689bdba2..ebc9b1e4 100644 --- a/src/actions/mover.ts +++ b/src/actions/mover.ts @@ -45,7 +45,7 @@ const CONSTRAINED_ADDITIONAL_PADDING = 70; /** * Identifier for a keyboard shortcut that commits the in-progress move. */ -const COMMIT_MOVE_SHORTCUT = 'commitMove'; +export const COMMIT_MOVE_SHORTCUT = 'commitMove'; /** * Whether this is an insert or a move. diff --git a/src/actions/ws_movement.ts b/src/actions/ws_movement.ts index 73d42f4f..b286f068 100644 --- a/src/actions/ws_movement.ts +++ b/src/actions/ws_movement.ts @@ -11,7 +11,7 @@ import { } from 'blockly'; import * as Constants from '../constants'; import type {WorkspaceSvg} from 'blockly'; -import {Navigation} from 'src/navigation'; +import {Navigation} from '../navigation'; const KeyCodes = BlocklyUtils.KeyCodes; const createSerializedKey = ShortcutRegistry.registry.createSerializedKey.bind( diff --git a/src/navigation_controller.ts b/src/navigation_controller.ts index de53ade4..2e4be2d5 100644 --- a/src/navigation_controller.ts +++ b/src/navigation_controller.ts @@ -32,7 +32,7 @@ import {EnterAction} from './actions/enter'; import {DisconnectAction} from './actions/disconnect'; import {ActionMenu} from './actions/action_menu'; import {MoveActions} from './actions/move'; -import {Mover} from './actions/mover'; +import {COMMIT_MOVE_SHORTCUT, Mover} from './actions/mover'; import {DuplicateAction} from './actions/duplicate'; import {StackNavigationAction} from './actions/stack_navigation'; @@ -285,6 +285,12 @@ export class NavigationController { this.shortcutDialog.uninstall(); this.stackNavigationAction.uninstall(); + // This should get unregistered when a move finishes, + // but it's possible the controller is disposed mid-move. + if (ShortcutRegistry.registry.getRegistry()[COMMIT_MOVE_SHORTCUT]) { + ShortcutRegistry.registry.unregister(COMMIT_MOVE_SHORTCUT); + } + for (const shortcut of Object.values(this.shortcuts)) { ShortcutRegistry.registry.unregister(shortcut.name); } diff --git a/test/controller_test.mocha.js b/test/controller_test.mocha.js new file mode 100644 index 00000000..118636f9 --- /dev/null +++ b/test/controller_test.mocha.js @@ -0,0 +1,68 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import 'jsdom-global/register'; +import * as Blockly from 'blockly'; +import {KeyboardNavigation} from '../src/index'; +import {registerFlyoutCursor} from '../src/flyout_cursor'; +import {registerNavigationDeferringToolbox} from '../src/navigation_deferring_toolbox'; + +suite('NavigationController', function () { + setup(function () { + this.jsdomCleanup = require('jsdom-global')( + '
', + {pretendToBeVisual: true}, + ); + + // Reset registries to their initial state. + Blockly.ShortcutRegistry.registry.reset(); + Blockly.ShortcutItems.registerDefaultShortcuts(); + Blockly.ContextMenuRegistry.registry.reset(); + Blockly.ContextMenuItems.registerDefaultOptions(); + + // Create the workspace for the test. + registerFlyoutCursor(); + registerNavigationDeferringToolbox(); + this.workspace = Blockly.inject('blocklyDiv'); + }); + + teardown(function () { + this.jsdomCleanup(); + }); + + test('Dispose and reinitialize the navigation controller without crashing', async function () { + const kb = new KeyboardNavigation(this.workspace); + kb.navigationController.dispose(); + kb.navigationController.init(); + kb.navigationController.dispose(); + }); + + test('Dispose plugin and reinitialize the navigation controller without crashing', async function () { + const kb = new KeyboardNavigation(this.workspace); + kb.dispose(); + kb.navigationController.init(); + kb.navigationController.dispose(); + }); + + test('Dispose controller and create a new KeyboardNavigation instance without crashing', async function () { + const kb = new KeyboardNavigation(this.workspace); + kb.navigationController.dispose(); + new KeyboardNavigation(this.workspace); + }); + + test('Dispose and create a new KeyboardNavigation instance without crashing', async function () { + const kb = new KeyboardNavigation(this.workspace); + kb.dispose(); + new KeyboardNavigation(this.workspace); + }); + + test('Add a workspace to existing instance without crashing', async function () { + const kb = new KeyboardNavigation(this.workspace); + const workspace2 = Blockly.inject('blocklyDiv'); + kb.navigationController.addWorkspace(workspace2); + kb.dispose(); + }); +});