Skip to content

fix: Dispatch keyboard events with the workspace they occurred on. #9137

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 2 commits into from
Jun 16, 2025
Merged

fix: Dispatch keyboard events with the workspace they occurred on. #9137

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0bd2834
fix: Dispatch keyboard events with the workspace they occurred on.
gonfunko Jun 11, 2025
7dd71a6
chore: Add comment warding off would-be refactorers.
gonfunko Jun 16, 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
17 changes: 12 additions & 5 deletions core/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,21 +320,28 @@ export function defineBlocks(blocks: {[key: string]: BlockDefinition}) {
* @param e Key down event.
*/
export function globalShortcutHandler(e: KeyboardEvent) {
const mainWorkspace = getMainWorkspace() as WorkspaceSvg;
if (!mainWorkspace) {
return;
// This would ideally just be a `focusedTree instanceof WorkspaceSvg`, but
// importing `WorkspaceSvg` (as opposed to just its type) causes cycles.
let workspace: WorkspaceSvg = getMainWorkspace() as WorkspaceSvg;
const focusedTree = getFocusManager().getFocusedTree();
for (const ws of getAllWorkspaces()) {
Copy link
Contributor

@maribethb maribethb Jun 13, 2025

Choose a reason for hiding this comment

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

This approach doesn't seem ideal. Do we need a function that gets the focused workspace? Seems like surely we'd need that elsewhere and not just in this shortcut handler.

Alternatively can you just check if the focused tree is a workspace svg, if so return that? Because the only things I think can be returned from getFocusedTree are workspaces and toolboxes, and if a toolbox is focused then this won't ever hit and we'll use the getMainWorkspace anyway. Flyouts are technically IFocusableTrees as well, but ultimately ben made them not actually in favor of just using the flyout's workspace directly.

Copy link
Contributor

@maribethb maribethb Jun 13, 2025

Choose a reason for hiding this comment

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

if it is a toolbox, we might want to get the workspace associated with that toolbox, which is why I think it might make sense to have a general purpose helper for getting the focused workspace. right now I think in core we don't have keyboard shortcuts that would work on a toolbox item but it's possible someone else could, especially now that keyboard shortcuts can take a general focused node as their scope.

Copy link
Contributor Author

@gonfunko gonfunko Jun 16, 2025

Choose a reason for hiding this comment

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

That seems sensible, but importing WorkspaceSvg (as opposed to just importing the type) causes a cycle or other trouble such that keyboard-experiment breaks, which is why I went with this tortured construct :/

Copy link
Contributor

@maribethb maribethb Jun 16, 2025

Choose a reason for hiding this comment

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

:/ Does putting a helper in a different file help at all?

Copy link
Contributor Author

@gonfunko gonfunko Jun 16, 2025

Choose a reason for hiding this comment

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

I don't think so since we'd need to import that file here which puts us right back in the situation we're in :/

Copy link
Contributor

@maribethb maribethb Jun 16, 2025

Choose a reason for hiding this comment

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

I think we should be able to move the entire globalShortcutHandler function to a different file, and then update the internal usages (it is @internal) to use the new location instead of common.js. There shouldn't be any circular dependencies within just that function.

I think that is a pretty straightforward fix and then we can avoid putting this for loop check in, and we can make the shortcuts work correctly for the toolbox case as well. common.js is already a mess with the opportunity to easily add circular dependencies, so I really don't want to create this very easy opportunity to add one in a future refactoring pass. If this approach doesn't work or isn't as straightforward as I think it is though, then we can live with what you have now and file an issue to clean it up. Does that work for you?

Copy link
Contributor Author

@gonfunko gonfunko Jun 16, 2025

Choose a reason for hiding this comment

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

Unfortunately that still causes a cycle (now with Field/DropDownDiv), and actually prevents Blockly from building as opposed to just surfacing once it's used in the keyboard-experiment. Sorry for all the back and forth on this, I'd really like to break it out as well but it seems like the workspace just has its tendrils in too many things. I'll file an issue to look into sorting this out though!

Copy link
Contributor

@maribethb maribethb Jun 16, 2025

Choose a reason for hiding this comment

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

:(

Thank you for trying!

Copy link
Contributor

@maribethb maribethb Jun 16, 2025

Choose a reason for hiding this comment

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

Possibly worth adding a comment to this code explaining you can't check the instanceof so that in the future we warn people off trying to do an "easy" refactor?

Copy link
Contributor Author

@gonfunko gonfunko Jun 16, 2025

Choose a reason for hiding this comment

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

Done!

if (focusedTree === (ws as WorkspaceSvg)) {
workspace = ws as WorkspaceSvg;
break;
}
}

if (
browserEvents.isTargetInput(e) ||
(mainWorkspace.rendered && !mainWorkspace.isVisible())
!workspace ||
(workspace.rendered && !workspace.isFlyout && !workspace.isVisible())
) {
// When focused on an HTML text input widget, don't trap any keys.
// Ignore keypresses on rendered workspaces that have been explicitly
// hidden.
return;
}
ShortcutRegistry.registry.onKeyDown(mainWorkspace, e);
ShortcutRegistry.registry.onKeyDown(workspace, e);
}

export const TEST_ONLY = {defineBlocksWithJsonArrayInternal};