From 1b5d4ddd9a64faf3965807dc8f5d6c7b683b0d6b Mon Sep 17 00:00:00 2001 From: Erik Pasternak Date: Fri, 23 May 2025 14:32:36 -0700 Subject: [PATCH 1/7] Work on fixing menu shortcuts --- src/actions/action_menu.ts | 15 +++++++++++++-- src/navigation.ts | 8 +++----- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/actions/action_menu.ts b/src/actions/action_menu.ts index ed456fbc..dea986d2 100644 --- a/src/actions/action_menu.ts +++ b/src/actions/action_menu.ts @@ -45,12 +45,23 @@ export class ActionMenu { private registerShortcut() { const menuShortcut: ShortcutRegistry.KeyboardShortcut = { name: Constants.SHORTCUT_NAMES.MENU, - preconditionFn: (workspace) => - this.navigation.canCurrentlyNavigate(workspace), + preconditionFn: (workspace) => { + return ( + this.navigation.canCurrentlyNavigate(workspace) && + !workspace.isDragging() + ); + }, callback: (workspace) => { switch (this.navigation.getState(workspace)) { case Constants.STATE.WORKSPACE: return this.openActionMenu(workspace); + case Constants.STATE.FLYOUT: { + const flyoutWorkspace = workspace.getFlyout()?.getWorkspace(); + if (flyoutWorkspace) { + return this.openActionMenu(flyoutWorkspace); + } + return false; + } default: return false; } diff --git a/src/navigation.ts b/src/navigation.ts index d1ec3342..e0a45152 100644 --- a/src/navigation.ts +++ b/src/navigation.ts @@ -99,9 +99,9 @@ export class Navigation { getState(workspace: Blockly.WorkspaceSvg): Constants.STATE { const focusedTree = Blockly.getFocusManager().getFocusedTree(); if (focusedTree instanceof Blockly.WorkspaceSvg) { - if (focusedTree.isFlyout && workspace === focusedTree.targetWorkspace) { + if (focusedTree.isFlyout) { return Constants.STATE.FLYOUT; - } else if (workspace === focusedTree) { + } else { return Constants.STATE.WORKSPACE; } } else if (focusedTree instanceof Blockly.Toolbox) { @@ -109,9 +109,7 @@ export class Navigation { return Constants.STATE.TOOLBOX; } } else if (focusedTree instanceof Blockly.Flyout) { - if (workspace === focusedTree.targetWorkspace) { - return Constants.STATE.FLYOUT; - } + return Constants.STATE.FLYOUT; } // Either a non-Blockly element currently has DOM focus, or a different // workspace holds it. From 8bf9929884ea313568192f71e08fdc11de6b99f2 Mon Sep 17 00:00:00 2001 From: Erik Pasternak Date: Tue, 27 May 2025 17:12:34 -0700 Subject: [PATCH 2/7] Add tests and fix case where ws is flyout --- src/navigation.ts | 5 +++- test/webdriverio/test/test_setup.ts | 37 +++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/navigation.ts b/src/navigation.ts index e0a45152..6cbf0866 100644 --- a/src/navigation.ts +++ b/src/navigation.ts @@ -820,8 +820,11 @@ export class Navigation { * @returns whether keyboard navigation is currently allowed. */ canCurrentlyNavigate(workspace: Blockly.WorkspaceSvg) { + const accessibilityMode = workspace.isFlyout + ? workspace.targetWorkspace?.keyboardAccessibilityMode + : workspace.keyboardAccessibilityMode; return ( - workspace.keyboardAccessibilityMode && + !!accessibilityMode && this.getState(workspace) !== Constants.STATE.NOWHERE ); } diff --git a/test/webdriverio/test/test_setup.ts b/test/webdriverio/test/test_setup.ts index 8c9fd94a..cdfbe302 100644 --- a/test/webdriverio/test/test_setup.ts +++ b/test/webdriverio/test/test_setup.ts @@ -431,3 +431,40 @@ export async function isDragging( return workspaceSvg.isDragging(); }); } + +/** + * Returns the result of the specificied action precondition. + * + * @param browser The active WebdriverIO Browser object. + * @param action The action to check the precondition for. + */ +export async function checkActionPrecondition( + browser: WebdriverIO.Browser, + action: string, +): Promise { + return await browser.execute((action) => { + const node = Blockly.getFocusManager().getFocusedNode(); + let workspace; + if (node instanceof Blockly.BlockSvg) { + workspace = node.workspace as Blockly.WorkspaceSvg; + } else if (node instanceof Blockly.Workspace) { + workspace = node as Blockly.WorkspaceSvg; + } else if (node instanceof Blockly.Field) { + workspace = node.getSourceBlock()?.workspace as Blockly.WorkspaceSvg; + } + + if (!workspace) { + throw new Error('Unable to derive workspace from focused node'); + } + const actionItem = Blockly.ShortcutRegistry.registry.getRegistry()[action]; + if (!actionItem || !actionItem.preconditionFn) { + throw new Error( + `No registered action or missing precondition: ${action}`, + ); + } + return actionItem.preconditionFn(workspace, { + focusedNode: + Blockly.FocusManager.getFocusManager().getFocusedNode() ?? undefined, + }); + }, action); +} From 21895c70f04ab3d4b82cf19156e54bcc331e40c4 Mon Sep 17 00:00:00 2001 From: Erik Pasternak Date: Tue, 27 May 2025 17:13:13 -0700 Subject: [PATCH 3/7] Add tests for real --- test/webdriverio/test/actions_test.ts | 66 +++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 test/webdriverio/test/actions_test.ts diff --git a/test/webdriverio/test/actions_test.ts b/test/webdriverio/test/actions_test.ts new file mode 100644 index 00000000..79371053 --- /dev/null +++ b/test/webdriverio/test/actions_test.ts @@ -0,0 +1,66 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import * as chai from 'chai'; +import {Key} from 'webdriverio'; +import { + checkActionPrecondition, + getFocusedBlockType, + moveToToolboxCategory, + PAUSE_TIME, + setCurrentCursorNodeById, + tabNavigateToWorkspace, + testFileLocations, + testSetup, +} from './test_setup.js'; + +suite('Menus test', function () { + // Setting timeout to unlimited as these tests take longer time to run + this.timeout(0); + + // Clear the workspace and load start blocks + setup(async function () { + this.browser = await testSetup(testFileLocations.BASE); + await this.browser.pause(PAUSE_TIME); + }); + + test('Menu action returns true normally', async function () { + // Navigate to draw_circle_1. + await tabNavigateToWorkspace(this.browser); + await setCurrentCursorNodeById(this.browser, 'draw_circle_1'); + chai.assert.isTrue( + await checkActionPrecondition(this.browser, 'menu'), + 'The menu should be openable on a block', + ); + }); + + test('Menu action returns true in the toolbox', async function () { + // Navigate to draw_circle_1. + await tabNavigateToWorkspace(this.browser); + await setCurrentCursorNodeById(this.browser, 'draw_circle_1'); + // Navigate to a toolbox category + await moveToToolboxCategory(this.browser, 'Functions'); + // Move to flyout. + await this.browser.keys(Key.ArrowRight); + + chai.assert.isTrue( + await checkActionPrecondition(this.browser, 'menu'), + 'The menu should be openable on a block in the toolbox', + ); + }); + + test('Menu action returns false during drag', async function () { + // Navigate to draw_circle_1. + await tabNavigateToWorkspace(this.browser); + await setCurrentCursorNodeById(this.browser, 'draw_circle_1'); + // Start moving the block + await this.browser.keys('m'); + chai.assert.isFalse( + await checkActionPrecondition(this.browser, 'menu'), + 'The menu should not be openable during a move', + ); + }); +}); From b5aea222ae515229f52408b41732cef719a5cf08 Mon Sep 17 00:00:00 2001 From: Erik Pasternak Date: Tue, 27 May 2025 17:14:50 -0700 Subject: [PATCH 4/7] Remove unused import --- test/webdriverio/test/actions_test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/webdriverio/test/actions_test.ts b/test/webdriverio/test/actions_test.ts index 79371053..94123c9d 100644 --- a/test/webdriverio/test/actions_test.ts +++ b/test/webdriverio/test/actions_test.ts @@ -8,7 +8,6 @@ import * as chai from 'chai'; import {Key} from 'webdriverio'; import { checkActionPrecondition, - getFocusedBlockType, moveToToolboxCategory, PAUSE_TIME, setCurrentCursorNodeById, From fb817c7d27718dc6b8effd08c41f2a802bb3a633 Mon Sep 17 00:00:00 2001 From: Erik Pasternak Date: Tue, 27 May 2025 20:47:04 -0700 Subject: [PATCH 5/7] Update tests to check for menu existence --- test/webdriverio/test/actions_test.ts | 19 +++++++++++++------ test/webdriverio/test/test_setup.ts | 16 ++++++++++++++++ 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/test/webdriverio/test/actions_test.ts b/test/webdriverio/test/actions_test.ts index 94123c9d..239ed7ed 100644 --- a/test/webdriverio/test/actions_test.ts +++ b/test/webdriverio/test/actions_test.ts @@ -7,7 +7,7 @@ import * as chai from 'chai'; import {Key} from 'webdriverio'; import { - checkActionPrecondition, + contextMenuExists, moveToToolboxCategory, PAUSE_TIME, setCurrentCursorNodeById, @@ -26,12 +26,15 @@ suite('Menus test', function () { await this.browser.pause(PAUSE_TIME); }); - test('Menu action returns true normally', async function () { + test('Menu action opens menu', async function () { // Navigate to draw_circle_1. await tabNavigateToWorkspace(this.browser); await setCurrentCursorNodeById(this.browser, 'draw_circle_1'); + await this.browser.pause(PAUSE_TIME); + await this.browser.keys([Key.Ctrl, Key.Return]); + await this.browser.pause(PAUSE_TIME); chai.assert.isTrue( - await checkActionPrecondition(this.browser, 'menu'), + await contextMenuExists(this.browser, 'Duplicate'), 'The menu should be openable on a block', ); }); @@ -44,9 +47,11 @@ suite('Menus test', function () { await moveToToolboxCategory(this.browser, 'Functions'); // Move to flyout. await this.browser.keys(Key.ArrowRight); + await this.browser.keys([Key.Ctrl, Key.Return]); + await this.browser.pause(PAUSE_TIME); chai.assert.isTrue( - await checkActionPrecondition(this.browser, 'menu'), + await contextMenuExists(this.browser, 'Help'), 'The menu should be openable on a block in the toolbox', ); }); @@ -57,8 +62,10 @@ suite('Menus test', function () { await setCurrentCursorNodeById(this.browser, 'draw_circle_1'); // Start moving the block await this.browser.keys('m'); - chai.assert.isFalse( - await checkActionPrecondition(this.browser, 'menu'), + await this.browser.keys([Key.Ctrl, Key.Return]); + await this.browser.pause(PAUSE_TIME); + chai.assert.isTrue( + await contextMenuExists(this.browser, 'Duplicate', true), 'The menu should not be openable during a move', ); }); diff --git a/test/webdriverio/test/test_setup.ts b/test/webdriverio/test/test_setup.ts index cdfbe302..2580c147 100644 --- a/test/webdriverio/test/test_setup.ts +++ b/test/webdriverio/test/test_setup.ts @@ -468,3 +468,19 @@ export async function checkActionPrecondition( }); }, action); } + +/** + * Wait for the specified context menu item to exist. + * + * @param browser The active WebdriverIO Browser object. + * @param itemText The display text of the context menu item to click. + * @return A Promise that resolves when the actions are completed. + */ +export async function contextMenuExists( + browser: WebdriverIO.Browser, + itemText: string, + reverse = false, +): Promise { + const item = await browser.$(`div=${itemText}`); + return await item.waitForExist({timeout: 200, reverse: reverse}); +} From 21b5181a2d628db45c3b10728f938a4cfad2b77b Mon Sep 17 00:00:00 2001 From: Erik Pasternak Date: Tue, 27 May 2025 20:51:08 -0700 Subject: [PATCH 6/7] Add param description --- test/webdriverio/test/test_setup.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/test/webdriverio/test/test_setup.ts b/test/webdriverio/test/test_setup.ts index 2580c147..0ffc1aae 100644 --- a/test/webdriverio/test/test_setup.ts +++ b/test/webdriverio/test/test_setup.ts @@ -474,6 +474,7 @@ export async function checkActionPrecondition( * * @param browser The active WebdriverIO Browser object. * @param itemText The display text of the context menu item to click. + * @param reverse Whether to check for non-existence instead. * @return A Promise that resolves when the actions are completed. */ export async function contextMenuExists( From 4c2f429815a220f4eec2afda198c9b3bf4e22fc1 Mon Sep 17 00:00:00 2001 From: Erik Pasternak Date: Wed, 28 May 2025 08:16:16 -0700 Subject: [PATCH 7/7] reuse node --- test/webdriverio/test/test_setup.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/webdriverio/test/test_setup.ts b/test/webdriverio/test/test_setup.ts index 0ffc1aae..85f0bf0f 100644 --- a/test/webdriverio/test/test_setup.ts +++ b/test/webdriverio/test/test_setup.ts @@ -463,8 +463,7 @@ export async function checkActionPrecondition( ); } return actionItem.preconditionFn(workspace, { - focusedNode: - Blockly.FocusManager.getFocusManager().getFocusedNode() ?? undefined, + focusedNode: node ?? undefined, }); }, action); }