fix: Dispatch keyboard events with the workspace they occurred on. #9137
Conversation
| return; | ||
| let workspace: WorkspaceSvg = getMainWorkspace() as WorkspaceSvg; | ||
| const focusedTree = getFocusManager().getFocusedTree(); | ||
| for (const ws of getAllWorkspaces()) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :/
There was a problem hiding this comment.
:/ Does putting a helper in a different file help at all?
There was a problem hiding this comment.
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 :/
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
:(
Thank you for trying!
There was a problem hiding this comment.
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?
|
Just an FYI, I suspect this will break some of the kb-nav actions that assume the passed in workspace is the main workspace. RaspberryPiFoundation/blockly-keyboard-experimentation#597 should fix most of those cases, but I may have missed some and I'm still working on tests for it. |
Yeah I already have fixes for keyboard-experiment that I'm waiting to land this to send out. |
maribethb
left a comment
maribethb
left a comment
There was a problem hiding this comment.
whoops, meant to approve on the previous pass. sorry & thanks for all the back and forth!
4 participants
The basics
The details
This PR dispatches keyboard shortcut events with a reference to the workspace they actually occurred on. Previously, the main workspace was always used. This unblocks functionality in the keyboard navigation experiment, and is broadly more correct. I verified that the built-in shortcuts (cut/copy/paste, delete, escape, undo, redo) work in the main workspace, mutators and flyouts with this change.