Skip to content

feat: Commit moves before running keyboard shortcuts. #564

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
gonfunko merged 3 commits into from
May 29, 2025
Merged

feat: Commit moves before running keyboard shortcuts. #564

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a0bc47
feat: Commit moves before running keyboard shortcuts.
gonfunko May 29, 2025
b502968
fix: Handle aborting moves.
gonfunko May 29, 2025
d151145
Merge branch 'main' into move-shortcuts
gonfunko May 29, 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
49 changes: 49 additions & 0 deletions src/actions/mover.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
registry,
utils,
WorkspaceSvg,
ShortcutRegistry,
} from 'blockly';
import * as Constants from '../constants';
import {Direction, getXYFromDirection} from '../drag_direction';
Expand All @@ -36,6 +37,11 @@ const UNCONSTRAINED_MOVE_DISTANCE = 20;
*/
const CONSTRAINED_ADDITIONAL_PADDING = 70;

/**
* Identifier for a keyboard shortcut that commits the in-progress move.
*/
const COMMIT_MOVE_SHORTCUT = 'commitMove';

/**
* Low-level code for moving blocks with keyboard shortcuts.
*/
Expand Down Expand Up @@ -140,6 +146,48 @@ export class Mover {
// (otherwise dragging will break).
getFocusManager().focusNode(block);
block.getFocusableElement().addEventListener('blur', blurListener);

// Register a keyboard shortcut under the key combos of all existing
// keyboard shortcuts that commits the move before allowing the real
// shortcut to proceed. This avoids all kinds of fun brokenness when
// deleting/copying/otherwise acting on a block in move mode.
const shortcutKeys = Object.values(ShortcutRegistry.registry.getRegistry())
.flatMap((shortcut) => shortcut.keyCodes)
.filter((keyCode) => {
return (
keyCode &&
![
utils.KeyCodes.RIGHT,
utils.KeyCodes.LEFT,
utils.KeyCodes.UP,
utils.KeyCodes.DOWN,
utils.KeyCodes.ENTER,
Copy link
Contributor

@microbit-matt-hillsdon microbit-matt-hillsdon May 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Also needs KeyCodes.SPACE from the finish move action alongside ENTER.

Potentially these could be pulled from a list of the move actions from move.ts? Would be helpful in future as we hope to add UI with the ability to rebind shortcuts (though these ones are perhaps less likely to change than some).

Copy link
Collaborator

@rachel-fenichel rachel-fenichel May 29, 2025

Choose a reason for hiding this comment

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

And we do anticipate that when all of the keyboard nav code makes it into core, the shortcut actions will directly cancel moves. This setup is necessary because some shortcuts are in core and some are in the plugin. We will keep UI rebinding in mind for design work on that.

Copy link
Contributor Author

@gonfunko gonfunko May 29, 2025

Choose a reason for hiding this comment

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

We don't actually need space, because this shortcut finishes the move itself; Enter is there to support command-enter to bring up the contextual menu, not to commit the move. But we do need Escape to abort the move, so I added that in (and cleaned up this shortcut in the new common cleanup method introduced in #561).

utils.KeyCodes.ESC,
].includes(
typeof keyCode === 'number'
? keyCode
: parseInt(`${keyCode.split('+').pop()}`),
)
);
})
// Convince TS there aren't undefined values.
.filter((keyCode): keyCode is string | number => !!keyCode);

const commitMoveShortcut = {
name: COMMIT_MOVE_SHORTCUT,
preconditionFn: (workspace: WorkspaceSvg) => {
return !!this.moves.get(workspace);
},
callback: (workspace: WorkspaceSvg) => {
this.finishMove(workspace);
return false;
},
keyCodes: shortcutKeys,
allowCollision: true,
};

ShortcutRegistry.registry.register(commitMoveShortcut);

return true;
}

Expand Down Expand Up @@ -208,6 +256,7 @@ export class Mover {
* @returns The info for the block.
*/
private preDragEndCleanup(workspace: WorkspaceSvg) {
ShortcutRegistry.registry.unregister(COMMIT_MOVE_SHORTCUT);
clearMoveHints(workspace);

const info = this.moves.get(workspace);
Expand Down
Loading