fix: Allow some things to handle the enter key on read only workspaces #597

RoboErikG · 2025-06-16T19:51:11Z

Fixes #585

This updates the enter handler to do more selective checks when deciding whether or not to handle an enter key. Also simplifies getState() slightly to remove an unnecessary parameter.

In general, this is aiming to reach parity between mouse and keyboard behavior when on a read-only workspace. Specific changes to behavior:

Enables creating a workspace cursor ('w' key) while in read-only mode.
Enables enter key to "click" on icons while in read-only mode.
Enabled navigating to the toolbox while in read-only mode.
Factors out most of the checks for whether or not an enter will be handled to the precondition.
Returns false from the precondition and callback more often when the shortcut isn't actually handled.

TODO for RaspberryPiFoundation/blockly#8915
Add additional tests for enter and click with a read-only workspace and ensure both modalities are consistent.

maribethb · 2025-06-17T20:08:31Z

src/actions/clipboard.ts

      this.oldCutShortcut.callback(workspace, e, shortcut, scope);
    if (didCut) {
-      this.copyWorkspace = workspace;
+      this.copyWorkspace = workspace.isFlyout


just checking, is this change supposed to be here? it doesn't seem related to the PR title

Whoops, not sure how this slipped in.

maribethb · 2025-06-17T20:31:28Z

src/actions/enter.ts

+          case Constants.STATE.WORKSPACE:
+            // The main workspace may or may not handle it depending on what's
+            // selected, so always pass it through to the callback.
+            return true;


The precondition should really try to give an accurate answer. The keyboard shortcut help display could be depending on the precondition being accurate. Rachel envisions a contextual help where only keyboard shortcuts that would currently do anything are shown.

For some cases there's no API to ask if it would be allowed and some of them we don't even know if it did anything. For example, the Icon.onClick() method doesn't have a return and there's no isClickable() method, but it only opens the mutator if the workspace is editable.

Because of that, it didn't seem like it was worth making the precondition overly complex if we're going to have to handle it anyway, but I'll factor out the logic to check all the known cases.

src/actions/enter.ts

maribethb · 2025-06-17T20:52:26Z

src/actions/enter.ts

-            flyoutCursor = this.navigation.getFlyoutCursor(
-              workspace.targetWorkspace,
-            );
+            if (targetWorkspace.isReadOnly()) return false;


Do you need to check this again since you're already checking it in the precondition?

Removed. Thanks for catching!

maribethb · 2025-06-17T20:53:07Z

src/actions/enter.ts

    } else if (curNode instanceof icons.Icon) {
      curNode.onClick();
    }
+    return true;


Despite the comment in the precondition, it seems like now we're saying the keyboard shortcut for enter will always be handled regardless of whether the workspace is in read-only mode or not. This doesn't seem correct to me, e.g. the field editors shouldn't open if the workspace is in read-only mode.

Based on #585 and playing with the current mouse behavior for read-only mode, I think only icons should be special cased, and the rest of these should continue not doing anything.

It would be helpful if you can add in the PR descriptions which cases you intended to make interactable in read-only, as then I know if we're on the same page about the intentions which may be different than being on the same page about the actual result of the code (which I may be misreading, for example).

The problem is we don't know if it was handled or not for all cases, specifically icons which have an on click() method that doesn't return anything. I've added additional return false statements where we do know.

OK, that's fine for icons.

For connections and workspace, I don't think the enter key should do anything in read-only mode, but I think this does actually cause them to do something. I can't tell if you intended them to change behavior and we should discuss that intention, or if that is unintentional, or if I'm wrong and the behavior hasn't changed.

Ah, I'd misunderstood that path. Added a check to shouldHandle for this case.

maribethb · 2025-06-17T20:53:35Z

src/actions/enter.ts

      for (const input of block.inputList) {
        for (const field of input.fieldRow) {
-          if (field.isClickable() && field.isFullBlockField()) {
+          if (field.isFullBlockField()) {


why remove this check? if the field isn't clickable, it shouldn't be "enter"able either, I think.

I'd removed it because field.showEditor() calls this.isClickable() before showing the editor so it had seemed redundant. That said, I'm adding it back so that this method returns whether or not it handled the return correctly.

…turn

maribethb · 2025-06-17T23:03:44Z

src/actions/enter.ts

+    if (!curNode) return false;
+    if (curNode instanceof Field && !curNode.isClickable()) return false;
+    // Returning true is sometimes incorrect for icons, but there's no API to check.
+    return true;


This is still going to be a behavior change for pressing enter on other cases, if I'm reading this correctly. If the workspace is read-only then pressing enter shouldn't open the toolbox. It doesn't before this change but I think this change will cause it to open because of this precondition + you're not checking it in the callback.

Yes, that was intentional to make it consistent with mouse which still opens the toolbox on click.

err, ignore my previous comment. It's intentional that pressing 't' opens the toolbox on a read only workspace. I've updated it so pressing enter on the ws or a connection won't since that's an insert action.

maribethb · 2025-06-17T23:07:35Z

src/actions/enter.ts

    } else if (curNode instanceof icons.Icon) {
      curNode.onClick();
    }
+    return true;


OK, that's fine for icons.

For connections and workspace, I don't think the enter key should do anything in read-only mode, but I think this does actually cause them to do something. I can't tell if you intended them to change behavior and we should discuss that intention, or if that is unintentional, or if I'm wrong and the behavior hasn't changed.

maribethb · 2025-06-17T23:07:58Z

src/actions/enter.ts

    } else if (curNode instanceof icons.Icon) {
      curNode.onClick();
    }
+    return true;


Also, you shouldn't return true as the default, because if the current node isn't an instance of any of these things (e.g. custom focusable object, or workspace comment), you shouldn't say we've handled the shortcut because we haven't. Those custom objects could register their own shortcut handler for enter and if you return true that could cause theirs to not be executed.

I realize this is the old behavior, but I think that's a bug.

Updated to return false by default and true for each case we handle.

maribethb

lgtm, but it would be helpful if you can make the pr description more informative for future reference when reading the changelog, e.g.:

Enables creating a workspace cursor ('w' key) while in readonly mode
Enables enter key to "click" on icons while in readonly mode
Returns false from the keyboard shortcut callback more often when the shortcut isn't actually handled

RoboErikG mentioned this pull request Jun 16, 2025

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

Merged

1 task

RoboErikG added 3 commits June 17, 2025 10:32

Allow some things to handle return on read only workspaces

6c865bc

Allow navigating to the toolbox on readonly workspaces

a6f526d

Allow navigating to the main workspace from readonly workspaces

bd45dd7

RoboErikG force-pushed the enter-on-icons branch from ae074db to bd45dd7 Compare June 17, 2025 17:32

Make tests with mouse more robust

0695cfc

RoboErikG force-pushed the enter-on-icons branch from 390afaa to 0695cfc Compare June 17, 2025 19:30

RoboErikG marked this pull request as ready for review June 17, 2025 19:42

RoboErikG requested a review from a team as a code owner June 17, 2025 19:56

RoboErikG requested review from maribethb and removed request for a team June 17, 2025 19:56

RoboErikG assigned maribethb Jun 17, 2025

maribethb reviewed Jun 17, 2025

View reviewed changes

Minor fixes and refactor out a method to check if ws should handle re…

ffc9554

…turn

maribethb reviewed Jun 17, 2025

View reviewed changes

RoboErikG added 2 commits June 18, 2025 10:11

Respond to comments

2f33035

Change default for handling enter to false.

bcaf238

maribethb approved these changes Jun 18, 2025

View reviewed changes

RoboErikG merged commit fd1e4fb into RaspberryPiFoundation:main Jun 18, 2025
8 checks passed

RoboErikG mentioned this pull request Jun 18, 2025

fix: Allow enter to be handled on blocks #608

Merged

microbit-matt-hillsdon mentioned this pull request Jun 24, 2025

fix: account for ephemeral focus in enter precondition #619

Merged

fix: Allow some things to handle the enter key on read only workspaces #597

fix: Allow some things to handle the enter key on read only workspaces #597

Uh oh!

Conversation

RoboErikG commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maribethb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

RoboErikG commented Jun 16, 2025 •

edited

Loading