Skip to content

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

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
RoboErikG merged 7 commits into from
Jun 18, 2025
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6c865bc
Allow some things to handle return on read only workspaces
RoboErikG Jun 16, 2025
a6f526d
Allow navigating to the toolbox on readonly workspaces
RoboErikG Jun 16, 2025
bd45dd7
Allow navigating to the main workspace from readonly workspaces
RoboErikG Jun 16, 2025
0695cfc
Make tests with mouse more robust
RoboErikG Jun 17, 2025
ffc9554
Minor fixes and refactor out a method to check if ws should handle re…
RoboErikG Jun 17, 2025
2f33035
Respond to comments
RoboErikG Jun 18, 2025
bcaf238
Change default for handling enter to false.
RoboErikG Jun 18, 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
2 changes: 1 addition & 1 deletion src/actions/action_menu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export class ActionMenu {
);
},
callback: (workspace) => {
switch (this.navigation.getState(workspace)) {
switch (this.navigation.getState()) {
case Constants.STATE.WORKSPACE:
case Constants.STATE.FLYOUT:
return this.openActionMenu(workspace);
Expand Down
8 changes: 4 additions & 4 deletions src/actions/arrow_navigation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ export class ArrowNavigation {
? workspace.targetWorkspace?.getFlyout()
: workspace.getFlyout();
let isHandled = false;
switch (this.navigation.getState(workspace)) {
switch (this.navigation.getState()) {
case Constants.STATE.WORKSPACE:
isHandled = this.fieldShortcutHandler(workspace, shortcut);
if (!isHandled && workspace) {
Expand Down Expand Up @@ -97,7 +97,7 @@ export class ArrowNavigation {
? workspace.targetWorkspace?.getToolbox()
: workspace.getToolbox();
let isHandled = false;
switch (this.navigation.getState(workspace)) {
switch (this.navigation.getState()) {
case Constants.STATE.WORKSPACE:
isHandled = this.fieldShortcutHandler(workspace, shortcut);
if (!isHandled && workspace) {
Expand Down Expand Up @@ -161,7 +161,7 @@ export class ArrowNavigation {
callback: (workspace, e, shortcut) => {
keyboardNavigationController.setIsActive(true);
let isHandled = false;
switch (this.navigation.getState(workspace)) {
switch (this.navigation.getState()) {
case Constants.STATE.WORKSPACE:
isHandled = this.fieldShortcutHandler(workspace, shortcut);
if (!isHandled && workspace) {
Expand Down Expand Up @@ -223,7 +223,7 @@ export class ArrowNavigation {
callback: (workspace, e, shortcut) => {
keyboardNavigationController.setIsActive(true);
let isHandled = false;
switch (this.navigation.getState(workspace)) {
switch (this.navigation.getState()) {
case Constants.STATE.WORKSPACE:
isHandled = this.fieldShortcutHandler(workspace, shortcut);
if (!isHandled) {
Expand Down
8 changes: 6 additions & 2 deletions src/actions/clipboard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,9 @@ export class Clipboard {
!!this.oldCutShortcut?.callback &&
this.oldCutShortcut.callback(workspace, e, shortcut, scope);
if (didCut) {
this.copyWorkspace = workspace;
this.copyWorkspace = workspace.isFlyout
Copy link
Collaborator

@maribethb maribethb Jun 17, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@RoboErikG RoboErikG Jun 17, 2025

Choose a reason for hiding this comment

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

Whoops, not sure how this slipped in.

? workspace.targetWorkspace
: workspace;
showCutHint(workspace);
}
return didCut;
Expand Down Expand Up @@ -285,7 +287,9 @@ export class Clipboard {
!!this.oldCopyShortcut?.callback &&
this.oldCopyShortcut.callback(workspace, e, shortcut, scope);
if (didCopy) {
this.copyWorkspace = workspace;
this.copyWorkspace = workspace.isFlyout
? workspace.targetWorkspace
: workspace;
showCopiedHint(workspace);
}
return didCopy;
Expand Down
2 changes: 1 addition & 1 deletion src/actions/disconnect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
Events,
ShortcutRegistry,
utils as BlocklyUtils,
Connection,

Check warning on line 12 in src/actions/disconnect.ts

View workflow job for this annotation

GitHub Actions / Eslint check 

'Connection' is defined but never used
ConnectionType,

Check warning on line 13 in src/actions/disconnect.ts

View workflow job for this annotation

GitHub Actions / Eslint check 

'ConnectionType' is defined but never used
keyboardNavigationController,
} from 'blockly';
import * as Constants from '../constants';
Expand Down Expand Up @@ -58,7 +58,7 @@
this.navigation.canCurrentlyEdit(workspace),
callback: (workspace) => {
keyboardNavigationController.setIsActive(true);
switch (this.navigation.getState(workspace)) {
switch (this.navigation.getState()) {
case Constants.STATE.WORKSPACE:
this.disconnectBlocks(workspace);
return true;
Expand Down
53 changes: 36 additions & 17 deletions src/actions/enter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

import {
Events,
Msg,
ShortcutRegistry,
utils as BlocklyUtils,
getFocusManager,
Expand Down Expand Up @@ -54,33 +53,51 @@ export class EnterAction {
*/
ShortcutRegistry.registry.register({
name: Constants.SHORTCUT_NAMES.EDIT_OR_CONFIRM,
preconditionFn: (workspace) =>
this.navigation.canCurrentlyEdit(workspace),
callback: (workspace, event) => {
preconditionFn: (workspace): boolean => {
switch (this.navigation.getState()) {
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;
Copy link
Collaborator

@maribethb maribethb Jun 17, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@RoboErikG RoboErikG Jun 17, 2025

Choose a reason for hiding this comment

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

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.

case Constants.STATE.FLYOUT: {
// If we're in the flyout the only supported actions are inserting
// blocks or clicking buttons, so don't handle this if the
// main workspace is read only.
const targetWorkspace = workspace.isFlyout
? workspace.targetWorkspace
: workspace;
return !!targetWorkspace && !targetWorkspace.isReadOnly();
}
default:
return false;
}
},
callback: (workspace, event): boolean => {
event.preventDefault();

const targetWorkspace = workspace.isFlyout
? workspace.targetWorkspace
: workspace;
if (!targetWorkspace) return false;

let flyoutCursor;
let curNode;

switch (this.navigation.getState(workspace)) {
switch (this.navigation.getState()) {
case Constants.STATE.WORKSPACE:
this.handleEnterForWS(workspace);
return true;
return this.handleEnterForWS(workspace);
case Constants.STATE.FLYOUT:
if (!workspace.targetWorkspace) return false;
flyoutCursor = this.navigation.getFlyoutCursor(
workspace.targetWorkspace,
);
if (targetWorkspace.isReadOnly()) return false;
Copy link
Collaborator

@maribethb maribethb Jun 17, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@RoboErikG RoboErikG Jun 17, 2025

Choose a reason for hiding this comment

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

Removed. Thanks for catching!

flyoutCursor = this.navigation.getFlyoutCursor(targetWorkspace);
if (!flyoutCursor) {
return false;
}
curNode = flyoutCursor.getCurNode();
if (curNode instanceof BlockSvg) {
this.insertFromFlyout(workspace.targetWorkspace);
this.insertFromFlyout(targetWorkspace);
} else if (curNode instanceof FlyoutButton) {
this.triggerButtonCallback(workspace);
}

return true;
default:
return false;
Expand All @@ -94,12 +111,13 @@ export class EnterAction {
* Handles hitting the enter key on the workspace.
*
* @param workspace The workspace.
* @returns True if the enter was handled, false otherwise.
*/
private handleEnterForWS(workspace: WorkspaceSvg) {
private handleEnterForWS(workspace: WorkspaceSvg): boolean {
const cursor = workspace.getCursor();
if (!cursor) return;
if (!cursor) return false;
const curNode = cursor.getCurNode();
if (!curNode) return;
if (!curNode) return false;
if (curNode instanceof Field) {
curNode.showEditor();
} else if (curNode instanceof BlockSvg) {
Expand All @@ -114,6 +132,7 @@ export class EnterAction {
} else if (curNode instanceof icons.Icon) {
curNode.onClick();
}
return true;
Copy link
Collaborator

@maribethb maribethb Jun 17, 2025

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@RoboErikG RoboErikG Jun 17, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@maribethb maribethb Jun 17, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@RoboErikG RoboErikG Jun 18, 2025

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@maribethb maribethb Jun 17, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@RoboErikG RoboErikG Jun 18, 2025

Choose a reason for hiding this comment

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

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

}

/**
Expand Down Expand Up @@ -278,7 +297,7 @@ export class EnterAction {
if (block.isSimpleReporter()) {
for (const input of block.inputList) {
for (const field of input.fieldRow) {
if (field.isClickable() && field.isFullBlockField()) {
if (field.isFullBlockField()) {
Copy link
Collaborator

@maribethb maribethb Jun 17, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@RoboErikG RoboErikG Jun 17, 2025

Choose a reason for hiding this comment

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

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.

field.showEditor();
return true;
}
Expand Down
2 changes: 1 addition & 1 deletion src/actions/exit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export class ExitAction {
preconditionFn: (workspace) =>
this.navigation.canCurrentlyNavigate(workspace),
callback: (workspace) => {
switch (this.navigation.getState(workspace)) {
switch (this.navigation.getState()) {
case Constants.STATE.FLYOUT:
case Constants.STATE.TOOLBOX:
getFocusManager().focusTree(workspace.targetWorkspace ?? workspace);
Expand Down
2 changes: 1 addition & 1 deletion src/actions/mover.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ export class Mover {
*/
canMove(workspace: WorkspaceSvg, block: BlockSvg) {
return !!(
this.navigation.getState(workspace) === Constants.STATE.WORKSPACE &&
this.navigation.getState() === Constants.STATE.WORKSPACE &&
this.navigation.canCurrentlyEdit(workspace) &&
!this.moves.has(workspace) && // No move in progress.
block?.isMovable()
Expand Down
11 changes: 8 additions & 3 deletions src/actions/ws_movement.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
/**
* The distance to move the cursor when the cursor is on the workspace.
*/
const WS_MOVE_DISTANCE = 40;

Check warning on line 24 in src/actions/ws_movement.ts

View workflow job for this annotation

GitHub Actions / Eslint check 

'WS_MOVE_DISTANCE' is assigned a value but never used

/**
* Logic for free movement of the cursor on the workspace with keyboard
Expand Down Expand Up @@ -68,11 +68,16 @@
/** Move the cursor to the workspace. */
{
name: Constants.SHORTCUT_NAMES.CREATE_WS_CURSOR,
preconditionFn: (workspace) =>
this.navigation.canCurrentlyEdit(workspace),
preconditionFn: (workspace) => {
return true;
},
callback: (workspace) => {
const targetWorkspace = workspace.isFlyout
? workspace.targetWorkspace
: workspace;
if (!targetWorkspace) return false;
keyboardNavigationController.setIsActive(true);
return this.createWSCursor(workspace);
return this.createWSCursor(targetWorkspace);
},
keyCodes: [KeyCodes.W],
},
Expand Down
11 changes: 4 additions & 7 deletions src/navigation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,9 @@ export class Navigation {
* Note that this assumes a workspace with passive focus (including for its
* toolbox or flyout) has a state of NOWHERE.
*
* @param workspace The workspace to get the state of.
* @returns The state of the given workspace.
*/
getState(workspace: Blockly.WorkspaceSvg): Constants.STATE {
getState(): Constants.STATE {
const focusedTree = Blockly.getFocusManager().getFocusedTree();
if (focusedTree instanceof Blockly.WorkspaceSvg) {
if (focusedTree.isFlyout) {
Expand All @@ -105,9 +104,7 @@ export class Navigation {
return Constants.STATE.WORKSPACE;
}
} else if (focusedTree instanceof Blockly.Toolbox) {
if (workspace === focusedTree.getWorkspace()) {
return Constants.STATE.TOOLBOX;
}
return Constants.STATE.TOOLBOX;
} else if (focusedTree instanceof Blockly.Flyout) {
return Constants.STATE.FLYOUT;
}
Expand Down Expand Up @@ -222,7 +219,7 @@ export class Navigation {
}
} else if (
e.type === Blockly.Events.BLOCK_CREATE &&
this.getState(mainWorkspace) === Constants.STATE.FLYOUT
this.getState() === Constants.STATE.FLYOUT
) {
// When variables are created, that recreates the flyout contents, leaving the
// cursor in an invalid state.
Expand Down Expand Up @@ -825,7 +822,7 @@ export class Navigation {
: workspace.keyboardAccessibilityMode;
return (
!!accessibilityMode &&
this.getState(workspace) !== Constants.STATE.NOWHERE &&
this.getState() !== Constants.STATE.NOWHERE &&
!Blockly.getFocusManager().ephemeralFocusTaken()
);
}
Expand Down
5 changes: 2 additions & 3 deletions src/navigation_controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,11 +196,10 @@ export class NavigationController {
/** Move focus to or from the toolbox. */
focusToolbox: {
name: Constants.SHORTCUT_NAMES.TOOLBOX,
preconditionFn: (workspace) =>
!workspace.isDragging() && this.navigation.canCurrentlyEdit(workspace),
preconditionFn: (workspace) => !workspace.isDragging(),
callback: (workspace) => {
keyboardNavigationController.setIsActive(true);
switch (this.navigation.getState(workspace)) {
switch (this.navigation.getState()) {
case Constants.STATE.WORKSPACE:
Blockly.getFocusManager().focusTree(
workspace.getToolbox() ??
Expand Down
13 changes: 11 additions & 2 deletions test/webdriverio/test/keyboard_mode_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
PAUSE_TIME,
getBlockElementById,
tabNavigateToWorkspace,
clickBlock,
} from './test_setup.js';
import {Key} from 'webdriverio';

Expand Down Expand Up @@ -125,8 +126,7 @@ suite(

await this.browser.pause(PAUSE_TIME);
// Right click a block
const element = await getBlockElementById(this.browser, 'controls_if_1');
await element.click({button: 'right'});
clickBlock(this.browser, 'controls_if_1', {button: 'right'});
await this.browser.pause(PAUSE_TIME);

chai.assert.isFalse(await isKeyboardNavigating(this.browser));
Expand All @@ -140,6 +140,15 @@ suite(
await this.browser.pause(PAUSE_TIME);
// Drag a block
const element = await getBlockElementById(this.browser, 'controls_if_1');

await this.browser.execute(() => {
const ws = Blockly.getMainWorkspace() as Blockly.WorkspaceSvg;
const block = ws.getBlockById('controls_if_1') as Blockly.BlockSvg;
ws.scrollBoundsIntoView(
block.getBoundingRectangleWithoutChildren(),
10,
);
});
await element.dragAndDrop({x: 10, y: 10});
await this.browser.pause(PAUSE_TIME);

Expand Down
56 changes: 56 additions & 0 deletions test/webdriverio/test/test_setup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,3 +555,59 @@ export async function contextMenuExists(
const item = await browser.$(`div=${itemText}`);
return await item.waitForExist({timeout: 200, reverse: reverse});
}

/**
* Find a clickable element on the block and click it.
* We can't always use the block's SVG root because clicking will always happen
* in the middle of the block's bounds (including children) by default, which
* causes problems if it has holes (e.g. statement inputs). Instead, this tries
* to get the first text field on the block. It falls back on the block's SVG root.
*
* @param browser The active WebdriverIO Browser object.
* @param blockId The id of the block to click, as an interactable element.
* @param clickOptions The options to pass to webdriverio's element.click function.
* @return A Promise that resolves when the actions are completed.
*/
export async function clickBlock(
browser: WebdriverIO.Browser,
blockId: string,
clickOptions?: Partial<webdriverio.ClickOptions> | undefined,
) {
const findableId = 'clickTargetElement';
// In the browser context, find the element that we want and give it a findable ID.
await browser.execute(
(blockId, newElemId) => {
const ws = Blockly.getMainWorkspace() as Blockly.WorkspaceSvg;
const block = ws.getBlockById(blockId) as Blockly.BlockSvg;
// Ensure the block we want to click is within the viewport.
ws.scrollBoundsIntoView(block.getBoundingRectangleWithoutChildren(), 10);
if (!block.isCollapsed()) {
for (const input of block.inputList) {
for (const field of input.fieldRow) {
if (field instanceof Blockly.FieldLabel) {
const svgRoot = field.getSvgRoot();
if (svgRoot) {
svgRoot.id = newElemId;
return;
}
}
}
}
}
// No label field found. Fall back to the block's SVG root.
block.getSvgRoot().id = newElemId;
},
blockId,
findableId,
);

// In the test context, get the Webdriverio Element that we've identified.
const elem = await browser.$(`#${findableId}`);

await elem.click(clickOptions);

// In the browser context, remove the ID.
await browser.execute((elemId) => {
document.getElementById(elemId)?.removeAttribute('id');
}, findableId);
}
Loading