diff --git a/src/actions/action_menu.ts b/src/actions/action_menu.ts index 4a2d1396..b92ffdf0 100644 --- a/src/actions/action_menu.ts +++ b/src/actions/action_menu.ts @@ -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); diff --git a/src/actions/arrow_navigation.ts b/src/actions/arrow_navigation.ts index d0d6de0e..a543ab63 100644 --- a/src/actions/arrow_navigation.ts +++ b/src/actions/arrow_navigation.ts @@ -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) { @@ -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) { @@ -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) { @@ -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) { diff --git a/src/actions/clipboard.ts b/src/actions/clipboard.ts index 5ae05c79..7ef4d2fa 100644 --- a/src/actions/clipboard.ts +++ b/src/actions/clipboard.ts @@ -285,7 +285,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; diff --git a/src/actions/disconnect.ts b/src/actions/disconnect.ts index 3604dec2..7995e6fb 100644 --- a/src/actions/disconnect.ts +++ b/src/actions/disconnect.ts @@ -58,7 +58,7 @@ export class DisconnectAction { 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; diff --git a/src/actions/enter.ts b/src/actions/enter.ts index f721b9a0..27af2675 100644 --- a/src/actions/enter.ts +++ b/src/actions/enter.ts @@ -6,7 +6,6 @@ import { Events, - Msg, ShortcutRegistry, utils as BlocklyUtils, getFocusManager, @@ -54,33 +53,48 @@ 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: + return this.shouldHandleEnterForWS(workspace); + 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, - ); + 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; @@ -90,30 +104,57 @@ export class EnterAction { }); } + /** + * Checks if the enter key should do anything for this ws. + * + * @param workspace The workspace to check. + * @returns True if the enter action should be handled. + */ + private shouldHandleEnterForWS(workspace: WorkspaceSvg): boolean { + const cursor = workspace.getCursor(); + const curNode = cursor?.getCurNode(); + if (!curNode) return false; + if (curNode instanceof Field) return curNode.isClickable(); + if ( + curNode instanceof RenderedConnection || + curNode instanceof WorkspaceSvg + ) { + return !workspace.isReadOnly(); + } + // Returning true is sometimes incorrect for icons, but there's no API to check. + if (curNode instanceof icons.Icon) return true; + return false; + } + /** * 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; - const curNode = cursor.getCurNode(); - if (!curNode) return; + const curNode = cursor?.getCurNode(); + if (!curNode) return false; if (curNode instanceof Field) { curNode.showEditor(); + return true; } else if (curNode instanceof BlockSvg) { if (!this.tryShowFullBlockFieldEditor(curNode)) { showHelpHint(workspace); } + return true; } else if ( curNode instanceof RenderedConnection || curNode instanceof WorkspaceSvg ) { this.navigation.openToolboxOrFlyout(workspace); + return true; } else if (curNode instanceof icons.Icon) { curNode.onClick(); + return true; } + return false; } /** diff --git a/src/actions/exit.ts b/src/actions/exit.ts index dcf1bbbb..f3e7c69f 100644 --- a/src/actions/exit.ts +++ b/src/actions/exit.ts @@ -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); diff --git a/src/actions/mover.ts b/src/actions/mover.ts index 979664f6..cb810955 100644 --- a/src/actions/mover.ts +++ b/src/actions/mover.ts @@ -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() diff --git a/src/actions/ws_movement.ts b/src/actions/ws_movement.ts index 3d4dfe50..cc8630fc 100644 --- a/src/actions/ws_movement.ts +++ b/src/actions/ws_movement.ts @@ -68,11 +68,16 @@ export class WorkspaceMovement { /** 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], }, diff --git a/src/navigation.ts b/src/navigation.ts index e8e9b430..97f3c506 100644 --- a/src/navigation.ts +++ b/src/navigation.ts @@ -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) { @@ -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; } @@ -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. @@ -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() ); } diff --git a/src/navigation_controller.ts b/src/navigation_controller.ts index fc40a049..76d9e4cf 100644 --- a/src/navigation_controller.ts +++ b/src/navigation_controller.ts @@ -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() ?? diff --git a/test/webdriverio/test/keyboard_mode_test.ts b/test/webdriverio/test/keyboard_mode_test.ts index 96f04cb7..692f0d9f 100644 --- a/test/webdriverio/test/keyboard_mode_test.ts +++ b/test/webdriverio/test/keyboard_mode_test.ts @@ -13,6 +13,7 @@ import { PAUSE_TIME, getBlockElementById, tabNavigateToWorkspace, + clickBlock, } from './test_setup.js'; import {Key} from 'webdriverio'; @@ -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)); @@ -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); diff --git a/test/webdriverio/test/test_setup.ts b/test/webdriverio/test/test_setup.ts index 154e4593..897756ee 100644 --- a/test/webdriverio/test/test_setup.ts +++ b/test/webdriverio/test/test_setup.ts @@ -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 | 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); +}