Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions core/focus_manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()).
Expand Down
16 changes: 10 additions & 6 deletions core/layer_manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

/**
Expand All @@ -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);
}
}

/**
Expand Down
113 changes: 91 additions & 22 deletions core/shortcut_items.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -101,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.
Expand All @@ -110,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
Expand All @@ -124,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) &&
Expand All @@ -148,28 +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 &&
!!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],
Expand All @@ -193,13 +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()
!workspace.isReadOnly() &&
!workspace.isDragging() &&
!getFocusManager().ephemeralFocusTaken() &&
isCuttable(focused)
);
},
callback(workspace, e, shortcut, scope) {
Expand Down Expand Up @@ -246,7 +298,16 @@ export function registerPaste() {
const pasteShortcut: KeyboardShortcut = {
name: names.PASTE,
preconditionFn(workspace) {
return !workspace.isReadOnly() && !Gesture.inProgress();
const targetWorkspace = workspace.isFlyout
? workspace.targetWorkspace
: workspace;
return (
!!copyData &&
!!targetWorkspace &&
!targetWorkspace.isReadOnly() &&
!targetWorkspace.isDragging() &&
!getFocusManager().ephemeralFocusTaken()
);
},
callback(workspace: WorkspaceSvg, e: Event) {
if (!copyData || !copyWorkspace) return false;
Expand Down Expand Up @@ -305,7 +366,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.
Expand Down Expand Up @@ -340,7 +405,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.
Expand Down
5 changes: 3 additions & 2 deletions core/workspace_svg.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
12 changes: 7 additions & 5 deletions tests/mocha/shortcut_items_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down