Skip to content

feat: ephemeral focus public getter, use in shortcut precondition #9110

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
maribethb merged 1 commit into from
May 30, 2025
Merged

feat: ephemeral focus public getter, use in shortcut precondition #9110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
View file
Open in desktop
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
29 changes: 23 additions & 6 deletions core/shortcut_items.ts
View file
Open in desktop
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 @@ -152,7 +155,8 @@ export function registerCopy() {
!workspace.isReadOnly() &&
!Gesture.inProgress() &&
!!focused &&
isCopyable(focused)
isCopyable(focused) &&
!getFocusManager().ephemeralFocusTaken()
);
},
callback(workspace, e, shortcut, scope) {
Expand Down Expand Up @@ -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()
Copy link
Collaborator

@BenHenning BenHenning 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.

Do we ever want shortcuts to work when ephemeral focus is taken? I'm wondering if it might make sense to filter this out at a higher level than this since we presumably will otherwise need to always check this for any shortcut precondition.

Understandable if we want to expose this as a customization for applications, though.

Edit: Ah, just noticed the fact that escape is already excluded from this. Perhaps we could make it an explicit configuration (that defaults to 'on'), but that's probably too heavy-handed.

Copy link
Contributor Author

@maribethb maribethb May 30, 2025

Choose a reason for hiding this comment

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

I think it might make sense to filter it out at a higher level, but it would be a breaking change. This is especially evident by the fact that escape should be excluded, so it's plausible that someone has other keyboard shortcuts they would want to take effect. I gave an example of one with the pitch field editor in the PR description (though I probably wouldn't implement that feature with keyboard shortcuts if it were me, it's plausible someone is)

);
},
callback(workspace, e, shortcut, scope) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down