From 2431971defb1ff7621c46fa7aea0f1610713e89d Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 29 Apr 2025 23:37:37 +0000 Subject: [PATCH 1/8] feat: Use FocusManager as source of truth. This removes nearly all custom focus/blur management and instead leverages FocusManager as the source of truth and primary system for focusing/selecting elements. There are still a bunch of issues with this branch, so more commits will be necessary before this is ready to be merged. --- src/actions/arrow_navigation.ts | 49 ++++-- src/actions/clipboard.ts | 11 +- src/actions/enter.ts | 8 +- src/actions/exit.ts | 12 +- src/index.ts | 241 +-------------------------- src/navigation.ts | 284 ++++++-------------------------- src/navigation_controller.ts | 56 +------ src/passive_focus.ts | 183 -------------------- src/workspace_utilities.ts | 46 ------ test/index.html | 43 +++-- test/webdriverio/index.html | 16 -- 11 files changed, 140 insertions(+), 809 deletions(-) delete mode 100644 src/passive_focus.ts diff --git a/src/actions/arrow_navigation.ts b/src/actions/arrow_navigation.ts index 54b0227f..74e32d6c 100644 --- a/src/actions/arrow_navigation.ts +++ b/src/actions/arrow_navigation.ts @@ -8,6 +8,7 @@ import {ASTNode, ShortcutRegistry, utils as BlocklyUtils} from 'blockly/core'; import type {Field, Toolbox, WorkspaceSvg} from 'blockly/core'; +import * as Blockly from 'blockly/core'; import * as Constants from '../constants'; import type {Navigation} from '../navigation'; @@ -67,12 +68,11 @@ export class ArrowNavigation { } return isHandled; case Constants.STATE.TOOLBOX: - isHandled = - toolbox && typeof toolbox.onShortcut === 'function' - ? toolbox.onShortcut(shortcut) - : false; - if (!isHandled) { - this.navigation.focusFlyout(workspace); + // @ts-expect-error private method + isHandled = toolbox && toolbox.selectChild(); + const flyout = workspace.getFlyout(); + if (!isHandled && flyout) { + Blockly.getFocusManager().focusTree(flyout.getWorkspace()); } return true; default: @@ -100,12 +100,13 @@ export class ArrowNavigation { } return isHandled; case Constants.STATE.FLYOUT: - this.navigation.focusToolbox(workspace); + if (toolbox) { + Blockly.getFocusManager().focusTree(toolbox); + } return true; case Constants.STATE.TOOLBOX: - return toolbox && typeof toolbox.onShortcut === 'function' - ? toolbox.onShortcut(shortcut) - : false; + // @ts-expect-error private method + return toolbox && toolbox.selectParent(); default: return false; } @@ -173,9 +174,20 @@ export class ArrowNavigation { } return isHandled; case Constants.STATE.TOOLBOX: - return toolbox && typeof toolbox.onShortcut === 'function' - ? toolbox.onShortcut(shortcut) - : false; + if (!toolbox) return false; + if (!toolbox.getSelectedItem()) { + const firstItem = toolbox.getToolboxItems().find((item) => item.isSelectable()) ?? null; + toolbox.setSelectedItem(firstItem); + isHandled = true; + } else { + // @ts-expect-error private method + isHandled = toolbox.selectNext(); + } + const selectedItem = toolbox.getSelectedItem(); + if (selectedItem) { + Blockly.getFocusManager().focusNode(selectedItem); + } + return isHandled; default: return false; } @@ -221,9 +233,14 @@ export class ArrowNavigation { } return isHandled; case Constants.STATE.TOOLBOX: - return toolbox && typeof toolbox.onShortcut === 'function' - ? toolbox.onShortcut(shortcut) - : false; + if (!toolbox) return false; + // @ts-expect-error private method + isHandled = toolbox.selectPrevious(); + const selectedItem = toolbox.getSelectedItem(); + if (selectedItem) { + Blockly.getFocusManager().focusNode(selectedItem); + } + return isHandled; default: return false; } diff --git a/src/actions/clipboard.ts b/src/actions/clipboard.ts index d34991a7..e2eb2cf6 100644 --- a/src/actions/clipboard.ts +++ b/src/actions/clipboard.ts @@ -13,6 +13,7 @@ import { clipboard, ICopyData, LineCursor, + getFocusManager } from 'blockly'; import * as Constants from '../constants'; import type {BlockSvg, WorkspaceSvg} from 'blockly'; @@ -281,7 +282,7 @@ export class Clipboard { const copied = !!this.copyData; if (copied) { if (navigationState === Constants.STATE.FLYOUT) { - this.navigation.focusWorkspace(workspace); + getFocusManager().focusTree(workspace); } showCopiedHint(workspace); } @@ -375,10 +376,10 @@ export class Clipboard { ? workspace : this.copyWorkspace; - const targetNode = this.navigation.getStationaryNode(pasteWorkspace); - // If we're pasting in the flyout it still targets the workspace. Focus first - // so ensure correct selection handling. - this.navigation.focusWorkspace(workspace); + const targetNode = this.navigation.getFocusedASTNode(pasteWorkspace); + // If we're pasting in the flyout it still targets the workspace. Focus + // first as to ensure correct selection handling. + getFocusManager().focusTree(workspace); Events.setGroup(true); const block = clipboard.paste(this.copyData, pasteWorkspace) as BlockSvg; diff --git a/src/actions/enter.ts b/src/actions/enter.ts index d8016483..2d136659 100644 --- a/src/actions/enter.ts +++ b/src/actions/enter.ts @@ -9,14 +9,12 @@ import { Events, ShortcutRegistry, utils as BlocklyUtils, -} from 'blockly/core'; - -import type { Block, BlockSvg, Field, FlyoutButton, WorkspaceSvg, + getFocusManager } from 'blockly/core'; import * as Constants from '../constants'; @@ -134,7 +132,7 @@ export class EnterAction { Events.setGroup(true); } - const stationaryNode = this.navigation.getStationaryNode(workspace); + const stationaryNode = this.navigation.getFocusedASTNode(workspace); const newBlock = this.createNewBlock(workspace); if (!newBlock) return; const insertStartPoint = stationaryNode @@ -146,7 +144,7 @@ export class EnterAction { workspace.setResizesEnabled(true); - this.navigation.focusWorkspace(workspace); + getFocusManager().focusTree(workspace); // eslint-disable-next-line @typescript-eslint/no-non-null-assertion workspace.getCursor()?.setCurNode(ASTNode.createBlockNode(newBlock)!); this.mover.startMove(workspace, newBlock, insertStartPoint); diff --git a/src/actions/exit.ts b/src/actions/exit.ts index 3b9666be..8316c8ff 100644 --- a/src/actions/exit.ts +++ b/src/actions/exit.ts @@ -4,7 +4,12 @@ * SPDX-License-Identifier: Apache-2.0 */ -import {ShortcutRegistry, utils as BlocklyUtils} from 'blockly/core'; +import { + ShortcutRegistry, + utils as BlocklyUtils, + getFocusManager, + Gesture +} from 'blockly/core'; import * as Constants from '../constants'; import type {Navigation} from '../navigation'; @@ -29,7 +34,10 @@ export class ExitAction { switch (this.navigation.getState(workspace)) { case Constants.STATE.FLYOUT: case Constants.STATE.TOOLBOX: - this.navigation.focusWorkspace(workspace); + getFocusManager().focusTree(workspace); + if (!Gesture.inProgress()) { + workspace.hideChaff(); + } return true; default: return false; diff --git a/src/index.ts b/src/index.ts index 6acc6f38..77a03369 100644 --- a/src/index.ts +++ b/src/index.ts @@ -6,7 +6,6 @@ import * as Blockly from 'blockly/core'; import {NavigationController} from './navigation_controller'; -import {getFlyoutElement, getToolboxElement} from './workspace_utilities'; import {enableBlocksOnDrag} from './disabled_blocks'; /** Options object for KeyboardNavigation instances. */ @@ -24,27 +23,6 @@ export class KeyboardNavigation { /** The workspace. */ protected workspace: Blockly.WorkspaceSvg; - /** Event handler run when the workspace gains focus. */ - private focusListener: () => void; - - /** Event handler run when the workspace loses focus. */ - private blurListener: () => void; - - /** Event handler run when the widget or dropdown div loses focus. */ - private widgetDropDownDivFocusOutListener: (e: Event) => void; - - /** Event handler run when the toolbox gains focus. */ - private toolboxFocusInListener: (e: Event) => void; - - /** Event handler run when the toolbox loses focus. */ - private toolboxFocusOutListener: (e: Event) => void; - - /** Event handler run when the flyout gains focus. */ - private flyoutFocusListener: () => void; - - /** Event handler run when the flyout loses focus. */ - private flyoutBlurListener: (e: Event) => void; - /** Keyboard navigation controller instance for the workspace. */ private navigationController: NavigationController; @@ -55,19 +33,8 @@ export class KeyboardNavigation { * These fields are used to preserve the workspace's initial state to restore * it when/if keyboard navigation is disabled. */ - private injectionDivTabIndex: string | null; - private workspaceParentTabIndex: string | null; private originalTheme: Blockly.Theme; - /** - * Stored to enable us to undo workspace monkey patching. - */ - private oldMarkFocused: () => void; - /** - * Stored to enable us to undo flyout workspace monkey patching. - */ - private oldFlyoutMarkFocused: (() => void) | null = null; - /** * Constructs the keyboard navigation. * @@ -97,190 +64,26 @@ export class KeyboardNavigation { // Add the event listener to enable disabled blocks on drag. workspace.addChangeListener(enableBlocksOnDrag); - // Ensure that only the root SVG G (group) has a tab index. - this.injectionDivTabIndex = workspace - .getInjectionDiv() - .getAttribute('tabindex'); - workspace.getInjectionDiv().removeAttribute('tabindex'); - this.workspaceParentTabIndex = workspace - .getParentSvg() - .getAttribute('tabindex'); - // We add a focus listener below so use -1 so it doesn't become focusable. - workspace.getParentSvg().setAttribute('tabindex', '-1'); - // Move the flyout for logical tab order. - const flyoutElement = getFlyoutElement(workspace); - flyoutElement?.parentElement?.insertBefore( - flyoutElement, - workspace.getParentSvg(), - ); - - // Temporary workaround for #136. - this.oldMarkFocused = workspace.markFocused; - workspace.markFocused = () => { - // Starting a gesture unconditionally calls markFocused on the parent SVG - // but we really don't want to move to the workspace (and close the - // flyout) if all you did was click in a flyout, potentially on a - // button. See also `gesture_monkey_patch.js`. + const flyout = workspace.getFlyout(); + if (flyout != null && flyout instanceof Blockly.Flyout) { + // This relies on internals. // eslint-disable-next-line @typescript-eslint/no-explicit-any - const gestureInternals = this.workspace.currentGesture_ as any; - const gestureFlyout = gestureInternals?.flyout; - const gestureFlyoutAutoClose = gestureFlyout?.autoClose; - const gestureOnBlock = gestureInternals?.startBlock; - - if ( - // When clicking on flyout that cannot close. - (gestureFlyout && !gestureFlyoutAutoClose) || - // When clicking on a block in a flyout that can close. - (gestureFlyout && gestureFlyoutAutoClose && !gestureOnBlock) - ) { - this.navigationController.focusFlyout(workspace); - } else { - this.navigationController.focusWorkspace(workspace); - } - }; - const flyout = this.workspace.getFlyout(); - if (flyout) { - this.oldFlyoutMarkFocused = flyout.getWorkspace().markFocused; - flyout.getWorkspace().markFocused = () => { - // By default this would call markFocused on the main workspace. It is - // called when you click on the flyout scrollbars, which is a - // particularly unfortunate time for the flyout to close. - this.navigationController.focusFlyout(this.workspace); - }; - } - - this.focusListener = () => { - this.navigationController.handleFocusWorkspace(workspace); - }; - this.blurListener = () => { - this.navigationController.handleBlurWorkspace(workspace); - }; - - workspace.getSvgGroup().addEventListener('focus', this.focusListener); - workspace.getSvgGroup().addEventListener('blur', this.blurListener); - - this.widgetDropDownDivFocusOutListener = (e: Event) => { - this.navigationController.handleFocusOutWidgetDropdownDiv( - workspace, - (e as FocusEvent).relatedTarget, - ); - }; - - Blockly.WidgetDiv.getDiv()?.addEventListener( - 'focusout', - this.widgetDropDownDivFocusOutListener, - ); - Blockly.DropDownDiv.getContentDiv()?.addEventListener( - 'focusout', - this.widgetDropDownDivFocusOutListener, - ); - - const toolboxElement = getToolboxElement(workspace); - this.toolboxFocusInListener = (e: Event) => { - if ( - (e.currentTarget as Element).contains( - (e as FocusEvent).relatedTarget as Node, - ) - ) { - return; - } - - this.navigationController.handleFocusToolbox(workspace); - }; - this.toolboxFocusOutListener = (e: Event) => { - if ( - (e.currentTarget as Element).contains( - (e as FocusEvent).relatedTarget as Node, - ) - ) { - return; - } - - this.navigationController.handleBlurToolbox( - workspace, - this.shouldCloseFlyoutOnBlur( - (e as FocusEvent).relatedTarget, - flyoutElement, - ), - ); - }; - toolboxElement?.addEventListener('focusin', this.toolboxFocusInListener); - toolboxElement?.addEventListener('focusout', this.toolboxFocusOutListener); - - this.flyoutFocusListener = () => { - this.navigationController.handleFocusFlyout(workspace); - }; - this.flyoutBlurListener = (e: Event) => { - this.navigationController.handleBlurFlyout( - workspace, - this.shouldCloseFlyoutOnBlur( - (e as FocusEvent).relatedTarget, - toolboxElement, - ), + const flyoutElement = ((flyout as any).svgGroup_ as SVGElement) ?? null; + flyoutElement?.parentElement?.insertBefore( + flyoutElement, + workspace.getParentSvg(), ); - }; - flyoutElement?.addEventListener('focus', this.flyoutFocusListener); - flyoutElement?.addEventListener('blur', this.flyoutBlurListener); + } } /** * Disables keyboard navigation for this navigator's workspace. */ dispose() { - // Revert markFocused monkey patch. - this.workspace.markFocused = this.oldMarkFocused; - if (this.oldFlyoutMarkFocused) { - const flyout = this.workspace.getFlyout(); - if (flyout) { - flyout.getWorkspace().markFocused = this.oldFlyoutMarkFocused; - } - } - // Remove the event listener that enables blocks on drag this.workspace.removeChangeListener(enableBlocksOnDrag); - this.workspace.getSvgGroup().removeEventListener('blur', this.blurListener); - this.workspace - .getSvgGroup() - .removeEventListener('focus', this.focusListener); - - Blockly.WidgetDiv.getDiv()?.removeEventListener( - 'focusout', - this.widgetDropDownDivFocusOutListener, - ); - Blockly.DropDownDiv.getContentDiv()?.removeEventListener( - 'focusout', - this.widgetDropDownDivFocusOutListener, - ); - - const toolboxElement = getToolboxElement(this.workspace); - toolboxElement?.removeEventListener('focusin', this.toolboxFocusInListener); - toolboxElement?.removeEventListener( - 'focusout', - this.toolboxFocusOutListener, - ); - - const flyoutElement = getFlyoutElement(this.workspace); - flyoutElement?.removeEventListener('focus', this.flyoutFocusListener); - flyoutElement?.removeEventListener('blur', this.flyoutBlurListener); - - if (this.workspaceParentTabIndex) { - this.workspace - .getParentSvg() - .setAttribute('tabindex', this.workspaceParentTabIndex); - } else { - this.workspace.getParentSvg().removeAttribute('tabindex'); - } - - if (this.injectionDivTabIndex) { - this.workspace - .getInjectionDiv() - .setAttribute('tabindex', this.injectionDivTabIndex); - } else { - this.workspace.getInjectionDiv().removeAttribute('tabindex'); - } - this.workspace.setTheme(this.originalTheme); this.navigationController.dispose(); @@ -307,32 +110,4 @@ export class KeyboardNavigation { }); this.workspace.setTheme(newTheme); } - - /** - * Identify whether we should close the flyout when the toolbox or flyout - * blurs. If a gesture is in progerss or we're moving from one the other - * then we leave it open. - * - * @param relatedTarget The related target from the event on the flyout or toolbox. - * @param container The other element of flyout or toolbox (opposite to the event). - * @returns true if the flyout should be closed, false otherwise. - */ - private shouldCloseFlyoutOnBlur( - relatedTarget: EventTarget | null, - container: Element | null, - ) { - if (Blockly.Gesture.inProgress()) { - return false; - } - if (!relatedTarget) { - return false; - } - if ( - relatedTarget instanceof Node && - container?.contains(relatedTarget as Node) - ) { - return false; - } - return true; - } } diff --git a/src/navigation.ts b/src/navigation.ts index c6929e28..14c2c67e 100644 --- a/src/navigation.ts +++ b/src/navigation.ts @@ -17,12 +17,6 @@ import { registrationType as cursorRegistrationType, FlyoutCursor, } from './flyout_cursor'; -import { - getFlyoutElement, - getToolboxElement, - getWorkspaceElement, -} from './workspace_utilities'; -import {PassiveFocus} from './passive_focus'; /** * The default coordinate to use when focusing on the workspace and no @@ -44,12 +38,6 @@ const WS_COORDINATE_ON_DELETE: Blockly.utils.Coordinate = * Class that holds all methods necessary for keyboard navigation to work. */ export class Navigation { - /** - * Object holding the location of the cursor for each workspace. - * Possible locations of the cursor are: workspace, flyout or toolbox. - */ - workspaceStates: {[index: string]: Constants.STATE} = {}; - /** * Wrapper for method that deals with workspace changes. * Used for removing change listener. @@ -68,11 +56,6 @@ export class Navigation { */ protected workspaces: Blockly.WorkspaceSvg[] = []; - /** - * An object that renders a passive focus indicator at a specified location. - */ - private passiveFocusIndicator: PassiveFocus = new PassiveFocus(); - /** * Constructor for keyboard navigation. */ @@ -114,7 +97,6 @@ export class Navigation { if (workspaceIdx > -1) { this.workspaces.splice(workspaceIdx, 1); } - this.passiveFocusIndicator.dispose(); workspace.removeChangeListener(this.wsChangeWrapper); if (flyout) { @@ -122,36 +104,56 @@ export class Navigation { } } - /** - * Sets the state for the given workspace. - * - * @param workspace The workspace to set the state on. - * @param state The navigation state. - */ - setState(workspace: Blockly.WorkspaceSvg, state: Constants.STATE) { - this.workspaceStates[workspace.id] = state; - } - /** * Gets the navigation state of the current workspace. * + * 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 { - return this.workspaceStates[workspace.id]; + const focusedTree = Blockly.getFocusManager().getFocusedTree(); + if (focusedTree instanceof Blockly.WorkspaceSvg) { + if (focusedTree.isFlyout && workspace === focusedTree.targetWorkspace) { + return Constants.STATE.FLYOUT; + } else if (workspace === focusedTree) { + return Constants.STATE.WORKSPACE; + } + } else if (focusedTree instanceof Blockly.Toolbox) { + if (workspace === focusedTree.getWorkspace()) { + return Constants.STATE.TOOLBOX; + } + } else if (focusedTree instanceof Blockly.Flyout) { + if (workspace === focusedTree.targetWorkspace) { + return Constants.STATE.FLYOUT; + } + } + // Either a non-Blockly element currently has DOM focus, or a different + // workspace holds it. + return Constants.STATE.NOWHERE; } /** - * Gets the node to use as context for insert operations. + * Searches the specified workspace for an ASTNode representation of its + * current focused node (which may be active or passive). * - * @param workspace The main workspace. + * @param workspace The workspace being searched. + * @returns An ASTNode representation of the current focused node, or null if + * the specified workspace either doesn't have a focused node, or it + * cannot be represented as an ASTNode. */ - getStationaryNode(workspace: Blockly.WorkspaceSvg) { - return ( - this.passiveFocusIndicator.getCurNode() ?? - workspace.getCursor()?.getCurNode() - ); + getFocusedASTNode(workspace: Blockly.WorkspaceSvg): Blockly.ASTNode | null { + const passive = Blockly.FocusableTreeTraverser.findFocusedNode(workspace); + if (passive instanceof Blockly.BlockSvg) { + return Blockly.ASTNode.createBlockNode(passive); + } else if (passive instanceof Blockly.Field) { + return Blockly.ASTNode.createFieldNode(passive); + } else if (passive instanceof Blockly.Connection) { + return Blockly.ASTNode.createConnectionNode(passive); + } + return null; } /** @@ -213,19 +215,6 @@ export class Navigation { this.handleBlockMutation(workspace, e as Blockly.Events.BlockChange); } break; - case Blockly.Events.BLOCK_CREATE: - if (workspace.isDragging()) { - // Hide the passive focus indicator when dragging so as not to fight - // with the drop cues. Safe because of the gesture monkey patch. - this.passiveFocusIndicator.hide(); - } - break; - } - - // Hiding the cursor isn't permanent and can show again when we render. - // Rehide it: - if (this.passiveFocusIndicator.isVisible()) { - workspace.getCursor()?.hide(); } } @@ -372,191 +361,10 @@ export class Navigation { Blockly.ASTNode.createStackNode(curNodeBlock), ); } - this.focusFlyout(mainWorkspace); - } - - /** - * Sets browser focus to the workspace. - * - * @param workspace The workspace to focus. - */ - focusWorkspace(workspace: Blockly.WorkspaceSvg) { - getWorkspaceElement(workspace).focus(); - } - - /** - * Sets the navigation state to workspace and moves the cursor to either the - * top block on a workspace or to the workspace. Switches from passive focus - * indication to showing the cursor. - * - * @param workspace The workspace that has gained focus. - */ - handleFocusWorkspace(workspace: Blockly.WorkspaceSvg) { - this.setState(workspace, Constants.STATE.WORKSPACE); - if (!Blockly.Gesture.inProgress()) { - workspace.hideChaff(); - // This will make a selection which would interfere with any gesture. - this.defaultWorkspaceCursorPositionIfNeeded(workspace); - } - - const cursor = workspace.getCursor(); - if (cursor) { - const passiveFocusNode = this.passiveFocusIndicator.getCurNode(); - this.passiveFocusIndicator.hide(); - const disposed = passiveFocusNode?.getSourceBlock()?.disposed; - // If there's a gesture then it will either set the node if it has not - // been disposed (which can happen when blocks are reloaded) or be a click - // that should not set one. - if (!Blockly.Gesture.inProgress() && passiveFocusNode && !disposed) { - cursor.setCurNode(passiveFocusNode); - } - } - } - - /** - * Clears navigation state and switches to using the passive focus indicator - * if it is not the context menu / field input that is causing blur. - * - * @param workspace The workspace that has lost focus. - * @param ignorePopUpDivs Whether to skip the focus indicator change when - * the widget/dropdown divs are open. - */ - handleBlurWorkspace( - workspace: Blockly.WorkspaceSvg, - ignorePopUpDivs = false, - ) { - this.setState(workspace, Constants.STATE.NOWHERE); - const cursor = workspace.getCursor(); - const popUpDivsShowing = - Blockly.WidgetDiv.isVisible() || Blockly.DropDownDiv.isVisible(); - if (cursor && (ignorePopUpDivs || !popUpDivsShowing)) { - const curNode = cursor.getCurNode(); - if (curNode) { - this.passiveFocusIndicator.show(curNode); - } - // It's initially null so this is a valid state despite the types. - cursor.setCurNode(null); - } - } - - /** - * Handle the widget or dropdown div losing focus (via focusout). - * - * Because we skip the widget/dropdown div cases in `handleBlurWorkspace` we need - * to catch them here. - * - * @param workspace The workspace. - * @param relatedTarget The related target (newly focused element if any). - */ - handleFocusOutWidgetDropdownDiv( - workspace: Blockly.WorkspaceSvg, - relatedTarget: EventTarget | null, - ) { - if (relatedTarget === null) { - // Workaround: - // Skip document.body/null case until this blur bug is fixed to avoid - // flipping to passive focus as the user moves their mouse over the - // colour picker. - // https://github.com/google/blockly-samples/issues/2498 - return; - } - if (relatedTarget !== getWorkspaceElement(workspace)) { - this.handleBlurWorkspace(workspace, true); - } - } - - /** - * Sets browser focus to the toolbox (if any). - * - * @param workspace The workspace with the toolbox. - */ - focusToolbox(workspace: Blockly.WorkspaceSvg) { - getToolboxElement(workspace)?.focus(); - } - - /** - * Sets the navigation state to toolbox and selects the first category in the - * toolbox. No-op if a toolbox does not exist on the given workspace. - * - * @param workspace The workspace to get the toolbox on. - */ - handleFocusToolbox(workspace: Blockly.WorkspaceSvg) { - const toolbox = workspace.getToolbox(); - if (!toolbox) { - return; - } - this.setState(workspace, Constants.STATE.TOOLBOX); - - if (!toolbox.getSelectedItem() && toolbox instanceof Blockly.Toolbox) { - // Find the first item that is selectable. - const toolboxItems = toolbox.getToolboxItems(); - for (let i = 0, toolboxItem; (toolboxItem = toolboxItems[i]); i++) { - if (toolboxItem.isSelectable()) { - toolbox.selectItemByPosition(i); - break; - } - } - } - } - - /** - * Clears the navigation state and closes the flyout if `allowClose` is true - * and a gesture is not in progress. - * - * @param workspace The workspace the flyout is on. - * @param closeFlyout True to close the flyout, false otherwise. - */ - handleBlurToolbox(workspace: Blockly.WorkspaceSvg, closeFlyout: boolean) { - this.setState(workspace, Constants.STATE.NOWHERE); - if (closeFlyout) { - workspace.hideChaff(); - } - } - - /** - * Sets browser focus to the flyout (if any). - * - * @param workspace The workspace with the flyout. - */ - focusFlyout(workspace: Blockly.WorkspaceSvg) { - getFlyoutElement(workspace)?.focus(); - } - - /** - * Sets the navigation state to flyout and moves the cursor to the first - * block or button in the flyout. We disable tabbing to the toolbox while - * the flyout has focus as we use left/right for that. - * - * @param workspace The workspace the flyout is on. - */ - handleFocusFlyout(workspace: Blockly.WorkspaceSvg) { - // Note this can happen when the flyout was already focussed as regrettably - // a click on the flyout calls markFocused() on the workspace SVG and the - // focus is then redirected back to the flyout. - - this.setState(workspace, Constants.STATE.FLYOUT); - this.getFlyoutCursor(workspace)?.draw(); - - // This doesn't identify a click on the scrollbars which will unfortunately - // default the cursor if the flyout didn't already have focus. - if (!Blockly.Gesture.inProgress()) { - this.defaultFlyoutCursorIfNeeded(workspace); - } - } - - /** - * Clears the navigation state and closes the flyout if `allowClose` is true - * and a gesture is not in progress. - * - * @param workspace The workspace the flyout is on. - * @param closeFlyout True to close the flyout, false otherwise. - */ - handleBlurFlyout(workspace: Blockly.WorkspaceSvg, closeFlyout: boolean) { - this.setState(workspace, Constants.STATE.NOWHERE); - if (closeFlyout) { - workspace.hideChaff(); + const flyout = mainWorkspace.getFlyout(); + if (flyout) { + Blockly.getFocusManager().focusTree(flyout.getWorkspace()); } - this.getFlyoutCursor(workspace)?.hide(); } /** @@ -1121,10 +929,12 @@ export class Navigation { * @param workspace The active workspace. */ openToolboxOrFlyout(workspace: Blockly.WorkspaceSvg) { - if (workspace.getToolbox()) { - this.focusToolbox(workspace); - } else { - this.focusFlyout(workspace); + const toolbox = workspace.getToolbox(); + const flyout = workspace.getFlyout(); + if (toolbox) { + Blockly.getFocusManager().focusTree(toolbox); + } else if (flyout) { + Blockly.getFocusManager().focusTree(flyout.getWorkspace()); } } diff --git a/src/navigation_controller.ts b/src/navigation_controller.ts index 2afa4c89..72167ca8 100644 --- a/src/navigation_controller.ts +++ b/src/navigation_controller.ts @@ -171,53 +171,6 @@ export class NavigationController { this.navigation.removeWorkspace(workspace); } - focusWorkspace(workspace: WorkspaceSvg) { - this.navigation.focusWorkspace(workspace); - } - - handleFocusWorkspace(workspace: Blockly.WorkspaceSvg) { - this.navigation.handleFocusWorkspace(workspace); - } - - handleBlurWorkspace(workspace: Blockly.WorkspaceSvg) { - // Before we blur, end any ongoing move - if (this.mover.isMoving(workspace)) { - this.mover.finishMove(workspace); - } - this.navigation.handleBlurWorkspace(workspace); - } - - handleFocusOutWidgetDropdownDiv( - workspace: Blockly.WorkspaceSvg, - relatedTarget: EventTarget | null, - ) { - this.navigation.handleFocusOutWidgetDropdownDiv(workspace, relatedTarget); - } - - focusToolbox(workspace: Blockly.WorkspaceSvg) { - this.navigation.focusToolbox(workspace); - } - - handleFocusToolbox(workspace: Blockly.WorkspaceSvg) { - this.navigation.handleFocusToolbox(workspace); - } - - handleBlurToolbox(workspace: Blockly.WorkspaceSvg, closeFlyout: boolean) { - this.navigation.handleBlurToolbox(workspace, closeFlyout); - } - - focusFlyout(workspace: Blockly.WorkspaceSvg) { - this.navigation.focusFlyout(workspace); - } - - handleFocusFlyout(workspace: Blockly.WorkspaceSvg) { - this.navigation.handleFocusFlyout(workspace); - } - - handleBlurFlyout(workspace: Blockly.WorkspaceSvg, closeFlyout: boolean) { - this.navigation.handleBlurFlyout(workspace, closeFlyout); - } - /** * Turns on keyboard navigation. * @@ -252,11 +205,10 @@ export class NavigationController { callback: (workspace) => { switch (this.navigation.getState(workspace)) { case Constants.STATE.WORKSPACE: - if (!workspace.getToolbox()) { - this.navigation.focusFlyout(workspace); - } else { - this.navigation.focusToolbox(workspace); - } + Blockly.getFocusManager() + .focusTree( + workspace.getToolbox() ?? workspace.getFlyout() ?? workspace + ); return true; default: return false; diff --git a/src/passive_focus.ts b/src/passive_focus.ts deleted file mode 100644 index 08719211..00000000 --- a/src/passive_focus.ts +++ /dev/null @@ -1,183 +0,0 @@ -/** - * @license - * Copyright 2025 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -import { - ASTNode, - RenderedConnection, - BlockSvg, - ConnectionType, - utils, -} from 'blockly/core'; - -/** - * A renderer for passive focus on AST node locations on the main workspace. - * Responsible for showing and hiding in an ephemeral way. Not - * guaranteed to stay up to date if workspace contents change. - * In general, passive focus should be hidden when the main workspace - * has active focus. - */ -export class PassiveFocus { - // The node where the indicator is drawn, if any. - private curNode: ASTNode | null = null; - - // The line drawn to indicate passive focus on a next connection. - nextConnectionIndicator: SVGRectElement; - - constructor() { - this.nextConnectionIndicator = this.createNextIndicator(); - } - - /** - * Get the current passive focus node. - * - * @returns the node or null. - */ - getCurNode(): ASTNode | null { - return this.curNode; - } - - /** Dispose of this indicator. Do any necessary cleanup. */ - dispose() { - this.hide(); - if (this.nextConnectionIndicator) { - utils.dom.removeNode(this.nextConnectionIndicator); - } - } - - /** - * Hide the currently visible passive focus indicator. - * Implementation varies based on location type. - */ - hide() { - if (!this.curNode) return; - const type = this.curNode.getType(); - const location = this.curNode.getLocation(); - - // If old node was a block, unselect it or remove fake selection. - if (type === ASTNode.types.BLOCK) { - this.hideAtBlock(this.curNode); - } else if (this.curNode.isConnection()) { - const curNodeAsConnection = location as RenderedConnection; - const connectionType = curNodeAsConnection.type; - if (connectionType === ConnectionType.NEXT_STATEMENT) { - this.hideAtNext(this.curNode); - } - } else { - console.log('Could not hide passive focus indicator'); - } - this.curNode = null; - } - - /** - * Show the passive focus indicator at the specified location. - * Implementation varies based on location type. - * - * @param node The node to show passive focus for. - */ - show(node: ASTNode) { - // Hide last shown. - this.hide(); - this.curNode = node; - - const type = this.curNode.getType(); - const location = this.curNode.getLocation(); - if (type === ASTNode.types.BLOCK) { - this.showAtBlock(this.curNode); - return; - } else if (this.curNode.isConnection()) { - const curNodeAsConnection = location as RenderedConnection; - const connectionType = curNodeAsConnection.type; - if (connectionType === ConnectionType.NEXT_STATEMENT) { - this.showAtNext(this.curNode); - return; - } - } - console.log('Could not show passive focus indicator'); - } - - /** - * Show a passive focus indicator on a block. - * - * @param node The passively-focused block. - */ - showAtBlock(node: ASTNode) { - const block = node.getLocation() as BlockSvg; - utils.dom.addClass(block.pathObject.svgPath, 'passiveBlockFocus'); - } - - /** - * Hide a passive focus indicator on a block. - * - * @param node The passively-focused block. - */ - hideAtBlock(node: ASTNode) { - const block = node.getLocation() as BlockSvg; - // When a block is selected we can end up with a duplicate svgPath. - const svgPaths = block.getSvgRoot().querySelectorAll('.passiveBlockFocus'); - svgPaths.forEach((svgPath) => - utils.dom.removeClass(svgPath, 'passiveBlockFocus'), - ); - } - - /** - * Creates DOM elements for the next connection indicator. - * - * @returns The root element of the next indicator. - */ - createNextIndicator() { - // A horizontal line used to represent a next connection. - const indicator = utils.dom.createSvgElement(utils.Svg.RECT, { - 'width': 100, - 'height': 5, - 'class': 'passiveNextIndicator', - }); - return indicator; - } - - /** - * Show a passive focus indicator on a next connection. - * - * @param node The passively-focused connection. - */ - showAtNext(node: ASTNode) { - const connection = node.getLocation() as RenderedConnection; - const targetBlock = connection.getSourceBlock(); - - // Make the connection indicator a child of the block's SVG group. - const blockSvgRoot = targetBlock.getSvgRoot(); - blockSvgRoot.appendChild(this.nextConnectionIndicator); - - // Move the indicator relative to the origin of the block's SVG group. - let x = 0; - const y = connection.getOffsetInBlock().y; - const width = targetBlock.getHeightWidth().width; - if (targetBlock.workspace.RTL) { - x = -width; - } - - this.nextConnectionIndicator.setAttribute('x', `${x}`); - this.nextConnectionIndicator.setAttribute('y', `${y}`); - this.nextConnectionIndicator.setAttribute('width', `${width}`); - - this.nextConnectionIndicator.style.display = ''; - } - - /** - * Hide a passive focus indicator on a next connection. - * - * @param node The passively-focused connection. - */ - hideAtNext(node: ASTNode) { - this.nextConnectionIndicator.parentNode?.removeChild( - this.nextConnectionIndicator, - ); - this.nextConnectionIndicator.style.display = 'none'; - } - - isVisible(): boolean { - return !!this.curNode; - } -} diff --git a/src/workspace_utilities.ts b/src/workspace_utilities.ts index 86daf832..ca319967 100644 --- a/src/workspace_utilities.ts +++ b/src/workspace_utilities.ts @@ -71,49 +71,3 @@ export function scrollBoundsIntoView( deltaY *= scale; workspace.scroll(workspace.scrollX + deltaX, workspace.scrollY + deltaY); } - -/** - * Get the workspace SVG group which is the element that takes focus. - * - * @param workspace The workspace. - * @returns The element. - */ -export function getWorkspaceElement(workspace: Blockly.Workspace): SVGElement { - return (workspace as Blockly.WorkspaceSvg).getSvgGroup() as SVGElement; -} - -/** - * Get the toolbox element that takes the focus (if any). - * - * @param workspace The workspace. - * @returns The element or null if a toolbox is not in use. - */ -export function getToolboxElement( - workspace: Blockly.WorkspaceSvg, -): HTMLElement | null { - const toolbox = workspace.getToolbox(); - if (toolbox instanceof Blockly.Toolbox) { - return toolbox.HtmlDiv?.querySelector( - '.blocklyToolboxCategoryGroup', - ) as HTMLElement | null; - } - return null; -} - -/** - * Get the flyout element we focus. - * - * @param workspace The workspace. - * @returns The element, or null if there is no flyout. - */ -export function getFlyoutElement( - workspace: Blockly.WorkspaceSvg, -): SVGElement | null { - const flyout = workspace.getFlyout(); - if (flyout != null && flyout instanceof Blockly.Flyout) { - // This relies on internals. - // eslint-disable-next-line @typescript-eslint/no-explicit-any - return ((flyout as any).svgGroup_ as SVGElement) ?? null; - } - return null; -} diff --git a/test/index.html b/test/index.html index 19399fa5..73f7d5d9 100644 --- a/test/index.html +++ b/test/index.html @@ -34,11 +34,6 @@ --outline-width: 5px; } - .blocklyWorkspace:focus .blocklyMainBackground { - outline: Highlight solid var(--outline-width); - outline-offset: -5px; - } - .blocklyFlyout { top: var(--outline-width); left: var(--outline-width); @@ -102,20 +97,40 @@ font-weight: bold; } - .passiveBlockFocus.blocklyPath { - stroke-dasharray: 5 3; - stroke-width: 3; + .blocklyActiveFocus:is(.blocklyField,.blocklyPath,.blocklyHighlightedConnectionPath) { stroke: #ffa200; + stroke-width: 3px; + outline-width: 0px; } - - .passiveNextIndicator { + .blocklyActiveFocus > .blocklyFlyoutBackground, .blocklyActiveFocus > .blocklyMainBackground { stroke: #ffa200; - fill: #ffa200; + stroke-width: 3px; } - - .inputActiveFocus { - stroke-width: 3; + .blocklyActiveFocus:is(.blocklyFlyout,.blocklyWorkspace) { + outline-width: 0px; + } + .blocklyActiveFocus:is(.blocklyToolbox,.blocklyToolboxCategoryContainer) { + outline: 3px solid #ffa200; + } + .blocklyPassiveFocus:is(.blocklyField,.blocklyPath,.blocklyHighlightedConnectionPath) { stroke: #ffa200; + stroke-dasharray: 5px 3px; + stroke-width: 3px; + } + .blocklyPassiveFocus > .blocklyFlyoutBackground, .blocklyPassiveFocus > .blocklyMainBackground { + stroke: #ffa200; + stroke-dasharray: 5px 3px; + stroke-width: 3px; + } + .blocklyPassiveFocus:is(.blocklyToolbox,.blocklyToolboxCategoryContainer) { + border: 3px dashed #ffa200; + } + .blocklySelected:is(.blocklyPath) { + stroke: #ffa200; + stroke-width: 5; + } + .blocklySelected>.blocklyPathLight { + display: none; } diff --git a/test/webdriverio/index.html b/test/webdriverio/index.html index 17451527..432dd1a0 100644 --- a/test/webdriverio/index.html +++ b/test/webdriverio/index.html @@ -57,22 +57,6 @@ thead { font-weight: bold; } - - .passiveBlockFocus.blocklyPath { - stroke-dasharray: 5 3; - stroke-width: 3; - stroke: #ffa200; - } - - .passiveNextIndicator { - stroke: #ffa200; - fill: #ffa200; - } - - .inputActiveFocus { - stroke-width: 3; - stroke: #ffa200; - } From eb7467812f84a6aabc83010092d77e12087957f9 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 30 Apr 2025 18:00:06 +0000 Subject: [PATCH 2/8] chore: lint fixes --- src/actions/arrow_navigation.ts | 43 ++++++++++++++++++--------------- src/actions/clipboard.ts | 2 +- src/actions/enter.ts | 2 +- src/actions/exit.ts | 2 +- src/navigation_controller.ts | 7 +++--- test/index.html | 32 ++++++++++++++++++------ 6 files changed, 54 insertions(+), 34 deletions(-) diff --git a/src/actions/arrow_navigation.ts b/src/actions/arrow_navigation.ts index 74e32d6c..b45ec61f 100644 --- a/src/actions/arrow_navigation.ts +++ b/src/actions/arrow_navigation.ts @@ -54,6 +54,7 @@ export class ArrowNavigation { shortcut: ShortcutRegistry.KeyboardShortcut, ): boolean => { const toolbox = workspace.getToolbox() as Toolbox; + const flyout = workspace.getFlyout(); let isHandled = false; switch (this.navigation.getState(workspace)) { case Constants.STATE.WORKSPACE: @@ -70,7 +71,6 @@ export class ArrowNavigation { case Constants.STATE.TOOLBOX: // @ts-expect-error private method isHandled = toolbox && toolbox.selectChild(); - const flyout = workspace.getFlyout(); if (!isHandled && flyout) { Blockly.getFocusManager().focusTree(flyout.getWorkspace()); } @@ -174,18 +174,22 @@ export class ArrowNavigation { } return isHandled; case Constants.STATE.TOOLBOX: - if (!toolbox) return false; - if (!toolbox.getSelectedItem()) { - const firstItem = toolbox.getToolboxItems().find((item) => item.isSelectable()) ?? null; - toolbox.setSelectedItem(firstItem); - isHandled = true; - } else { - // @ts-expect-error private method - isHandled = toolbox.selectNext(); - } - const selectedItem = toolbox.getSelectedItem(); - if (selectedItem) { - Blockly.getFocusManager().focusNode(selectedItem); + if (toolbox) { + if (!toolbox.getSelectedItem()) { + const firstItem = + toolbox + .getToolboxItems() + .find((item) => item.isSelectable()) ?? null; + toolbox.setSelectedItem(firstItem); + isHandled = true; + } else { + // @ts-expect-error private method + isHandled = toolbox.selectNext(); + } + const selectedItem = toolbox.getSelectedItem(); + if (selectedItem) { + Blockly.getFocusManager().focusNode(selectedItem); + } } return isHandled; default: @@ -233,12 +237,13 @@ export class ArrowNavigation { } return isHandled; case Constants.STATE.TOOLBOX: - if (!toolbox) return false; - // @ts-expect-error private method - isHandled = toolbox.selectPrevious(); - const selectedItem = toolbox.getSelectedItem(); - if (selectedItem) { - Blockly.getFocusManager().focusNode(selectedItem); + if (toolbox) { + // @ts-expect-error private method + isHandled = toolbox.selectPrevious(); + const selectedItem = toolbox.getSelectedItem(); + if (selectedItem) { + Blockly.getFocusManager().focusNode(selectedItem); + } } return isHandled; default: diff --git a/src/actions/clipboard.ts b/src/actions/clipboard.ts index e2eb2cf6..677bdf7f 100644 --- a/src/actions/clipboard.ts +++ b/src/actions/clipboard.ts @@ -13,7 +13,7 @@ import { clipboard, ICopyData, LineCursor, - getFocusManager + getFocusManager, } from 'blockly'; import * as Constants from '../constants'; import type {BlockSvg, WorkspaceSvg} from 'blockly'; diff --git a/src/actions/enter.ts b/src/actions/enter.ts index 2d136659..4251de4a 100644 --- a/src/actions/enter.ts +++ b/src/actions/enter.ts @@ -14,7 +14,7 @@ import { Field, FlyoutButton, WorkspaceSvg, - getFocusManager + getFocusManager, } from 'blockly/core'; import * as Constants from '../constants'; diff --git a/src/actions/exit.ts b/src/actions/exit.ts index 8316c8ff..e1afb99d 100644 --- a/src/actions/exit.ts +++ b/src/actions/exit.ts @@ -8,7 +8,7 @@ import { ShortcutRegistry, utils as BlocklyUtils, getFocusManager, - Gesture + Gesture, } from 'blockly/core'; import * as Constants from '../constants'; diff --git a/src/navigation_controller.ts b/src/navigation_controller.ts index 72167ca8..2d762269 100644 --- a/src/navigation_controller.ts +++ b/src/navigation_controller.ts @@ -205,10 +205,9 @@ export class NavigationController { callback: (workspace) => { switch (this.navigation.getState(workspace)) { case Constants.STATE.WORKSPACE: - Blockly.getFocusManager() - .focusTree( - workspace.getToolbox() ?? workspace.getFlyout() ?? workspace - ); + Blockly.getFocusManager().focusTree( + workspace.getToolbox() ?? workspace.getFlyout() ?? workspace, + ); return true; default: return false; diff --git a/test/index.html b/test/index.html index 73f7d5d9..a1694dcb 100644 --- a/test/index.html +++ b/test/index.html @@ -97,39 +97,55 @@ font-weight: bold; } - .blocklyActiveFocus:is(.blocklyField,.blocklyPath,.blocklyHighlightedConnectionPath) { + .blocklyActiveFocus:is( + .blocklyField, + .blocklyPath, + .blocklyHighlightedConnectionPath + ) { stroke: #ffa200; stroke-width: 3px; outline-width: 0px; } - .blocklyActiveFocus > .blocklyFlyoutBackground, .blocklyActiveFocus > .blocklyMainBackground { + .blocklyActiveFocus > .blocklyFlyoutBackground, + .blocklyActiveFocus > .blocklyMainBackground { stroke: #ffa200; stroke-width: 3px; } - .blocklyActiveFocus:is(.blocklyFlyout,.blocklyWorkspace) { + .blocklyActiveFocus:is(.blocklyFlyout, .blocklyWorkspace) { outline-width: 0px; } - .blocklyActiveFocus:is(.blocklyToolbox,.blocklyToolboxCategoryContainer) { + .blocklyActiveFocus:is( + .blocklyToolbox, + .blocklyToolboxCategoryContainer + ) { outline: 3px solid #ffa200; } - .blocklyPassiveFocus:is(.blocklyField,.blocklyPath,.blocklyHighlightedConnectionPath) { + .blocklyPassiveFocus:is( + .blocklyField, + .blocklyPath, + .blocklyHighlightedConnectionPath + ) { stroke: #ffa200; stroke-dasharray: 5px 3px; stroke-width: 3px; } - .blocklyPassiveFocus > .blocklyFlyoutBackground, .blocklyPassiveFocus > .blocklyMainBackground { + .blocklyPassiveFocus > .blocklyFlyoutBackground, + .blocklyPassiveFocus > .blocklyMainBackground { stroke: #ffa200; stroke-dasharray: 5px 3px; stroke-width: 3px; } - .blocklyPassiveFocus:is(.blocklyToolbox,.blocklyToolboxCategoryContainer) { + .blocklyPassiveFocus:is( + .blocklyToolbox, + .blocklyToolboxCategoryContainer + ) { border: 3px dashed #ffa200; } .blocklySelected:is(.blocklyPath) { stroke: #ffa200; stroke-width: 5; } - .blocklySelected>.blocklyPathLight { + .blocklySelected > .blocklyPathLight { display: none; } From 9d1738034f14efb16468be56fef2e96be2dc8354 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 1 May 2025 04:43:49 +0000 Subject: [PATCH 3/8] fix: Block movement & init flyout. This ensures block movement works again, and that the initial item selected when opening the flyout is correctly focused. --- src/actions/arrow_navigation.ts | 1 + src/actions/mover.ts | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/src/actions/arrow_navigation.ts b/src/actions/arrow_navigation.ts index b45ec61f..7945e72d 100644 --- a/src/actions/arrow_navigation.ts +++ b/src/actions/arrow_navigation.ts @@ -73,6 +73,7 @@ export class ArrowNavigation { isHandled = toolbox && toolbox.selectChild(); if (!isHandled && flyout) { Blockly.getFocusManager().focusTree(flyout.getWorkspace()); + this.navigation.defaultFlyoutCursorIfNeeded(workspace); } return true; default: diff --git a/src/actions/mover.ts b/src/actions/mover.ts index 9093e065..56134f6b 100644 --- a/src/actions/mover.ts +++ b/src/actions/mover.ts @@ -15,6 +15,7 @@ import { ASTNode, Connection, dragging, + getFocusManager, registry, utils, WorkspaceSvg, @@ -132,6 +133,9 @@ export class Mover { // Begin drag. dragger.onDragStart(info.fakePointerEvent('pointerdown')); info.updateTotalDelta(); + // In case the block is detached, ensure that it still retains focus + // (otherwise dragging will break). + getFocusManager().focusNode(block); return true; } @@ -159,6 +163,8 @@ export class Mover { this.moves.delete(workspace); // Delay scroll until after block has finished moving. setTimeout(() => this.scrollCurrentBlockIntoView(workspace), 0); + // If the block gets reattached, ensure it retains focus. + getFocusManager().focusNode(info.block); return true; } @@ -205,6 +211,8 @@ export class Mover { this.moves.delete(workspace); // Delay scroll until after block has finished moving. setTimeout(() => this.scrollCurrentBlockIntoView(workspace), 0); + // If the block gets reattached, ensure it retains focus. + getFocusManager().focusNode(info.block); return true; } From e8a1072793ebc4385847eda47680e0b3a8638c3a Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 1 May 2025 06:05:25 +0000 Subject: [PATCH 4/8] chore: Remove gesture monkey patch. It no longer seems required. --- src/gesture_monkey_patch.js | 32 -------------------------------- src/navigation_controller.ts | 1 - 2 files changed, 33 deletions(-) delete mode 100644 src/gesture_monkey_patch.js diff --git a/src/gesture_monkey_patch.js b/src/gesture_monkey_patch.js deleted file mode 100644 index 3e5a1bff..00000000 --- a/src/gesture_monkey_patch.js +++ /dev/null @@ -1,32 +0,0 @@ -/** - * @license - * Copyright 2021 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -/** - * @fileoverview Overrides methods on Blockly.Gesture to integrate focus mangagement - * with the gesture handling. - * @author aschmiedt@google.com (Abby Schmiedt) - */ - -import * as Blockly from 'blockly/core'; - -const oldDispose = Blockly.Gesture.prototype.dispose; - -/** - * Intercept the end of a gesture and ensure the workspace is focused. - * See also the listener is index.ts that subverts the markFocused call - * in `doStart`. - * @this {Blockly.Gesture} - * @override - */ -Blockly.Gesture.prototype.dispose = function () { - // This is a bit of a cludge and focus management needs to be better - // integrated with Gesture. The intent is to move focus at the end of - // a drag from a non-auto-closing flyout. - if (this.isDragging()) { - this.creatorWorkspace?.getSvgGroup().focus(); - } - oldDispose.call(this); -}; diff --git a/src/navigation_controller.ts b/src/navigation_controller.ts index 2d762269..c4301f43 100644 --- a/src/navigation_controller.ts +++ b/src/navigation_controller.ts @@ -10,7 +10,6 @@ * @author aschmiedt@google.com (Abby Schmiedt) */ -import './gesture_monkey_patch'; import './toolbox_monkey_patch'; import * as Blockly from 'blockly/core'; From e99d31525998e8cb908391f28d336c501b354d69 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 1 May 2025 06:27:17 +0000 Subject: [PATCH 5/8] chore: Empty commit to re-trigger CI. From 911d8241e452101b0e28a4f2510f06845f0ad491 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Fri, 2 May 2025 01:18:14 +0000 Subject: [PATCH 6/8] chore: Remove outline removal CSS. This is now in Core. --- test/index.html | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/index.html b/test/index.html index a1694dcb..a528362f 100644 --- a/test/index.html +++ b/test/index.html @@ -104,16 +104,12 @@ ) { stroke: #ffa200; stroke-width: 3px; - outline-width: 0px; } .blocklyActiveFocus > .blocklyFlyoutBackground, .blocklyActiveFocus > .blocklyMainBackground { stroke: #ffa200; stroke-width: 3px; } - .blocklyActiveFocus:is(.blocklyFlyout, .blocklyWorkspace) { - outline-width: 0px; - } .blocklyActiveFocus:is( .blocklyToolbox, .blocklyToolboxCategoryContainer From 893f6b17c2ae4690843f4e9cf35348ac96d032e1 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Fri, 2 May 2025 05:21:11 +0000 Subject: [PATCH 7/8] fix: webdriverio tests. Use tab navigation instead of clicking for selection. --- test/webdriverio/test/basic_test.ts | 9 +++----- test/webdriverio/test/clipboard_test.ts | 27 +++++++++++++----------- test/webdriverio/test/test_setup.ts | 28 +++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 18 deletions(-) diff --git a/test/webdriverio/test/basic_test.ts b/test/webdriverio/test/basic_test.ts index e57dc4bd..6b142b0b 100644 --- a/test/webdriverio/test/basic_test.ts +++ b/test/webdriverio/test/basic_test.ts @@ -15,10 +15,9 @@ import { testSetup, testFileLocations, PAUSE_TIME, - getBlockElementById, - clickBlock, + tabNavigateToWorkspace, } from './test_setup.js'; -import {Key, ClickOptions} from 'webdriverio'; +import {Key} from 'webdriverio'; suite('Keyboard navigation', function () { // Setting timeout to unlimited as these tests take a longer time to run than most mocha test @@ -38,9 +37,7 @@ suite('Keyboard navigation', function () { }); test('Selected block', async function () { - const block = await getBlockElementById(this.browser, 'p5_setup_1'); - await clickBlock(this.browser, block, {button: 0} as ClickOptions); - await this.browser.pause(PAUSE_TIME); + await tabNavigateToWorkspace(this.browser); for (let i = 0; i < 8; i++) { await this.browser.keys(Key.ArrowDown); diff --git a/test/webdriverio/test/clipboard_test.ts b/test/webdriverio/test/clipboard_test.ts index f83c8368..6797c615 100644 --- a/test/webdriverio/test/clipboard_test.ts +++ b/test/webdriverio/test/clipboard_test.ts @@ -12,16 +12,10 @@ import { PAUSE_TIME, getBlockElementById, getSelectedBlockId, - clickBlock, ElementWithId, + tabNavigateToWorkspace, } from './test_setup.js'; -import { - ClickOptions, - Key, - KeyAction, - PointerAction, - WheelAction, -} from 'webdriverio'; +import {Key, KeyAction, PointerAction, WheelAction} from 'webdriverio'; suite('Clipboard test', function () { // Setting timeout to unlimited as these tests take longer time to run @@ -34,17 +28,21 @@ suite('Clipboard test', function () { }); test('Copy and paste while block selected', async function () { - const block = await getBlockElementById(this.browser, 'draw_circle_1'); - await clickBlock(this.browser, block, {button: 0} as ClickOptions); + // Navigate to draw_circle_1. + await tabNavigateToWorkspace(this.browser); + for (let i = 0; i < 8; i++) { + await this.browser.keys(Key.ArrowDown); + await this.browser.pause(PAUSE_TIME); + } // Copy and paste await this.browser.keys([Key.Ctrl, 'c']); await this.browser.keys([Key.Ctrl, 'v']); await this.browser.pause(PAUSE_TIME); + const block = await getBlockElementById(this.browser, 'draw_circle_1'); const blocks = await getSameBlocks(this.browser, block); const selectedId = await getSelectedBlockId(this.browser); - chai.assert.equal(await blocks.length, 2); chai.assert.equal( selectedId, @@ -54,8 +52,13 @@ suite('Clipboard test', function () { }); test('Cut and paste while block selected', async function () { + // Navigate to draw_circle_1. + await tabNavigateToWorkspace(this.browser); + for (let i = 0; i < 8; i++) { + await this.browser.keys(Key.ArrowDown); + await this.browser.pause(PAUSE_TIME); + } const block = await getBlockElementById(this.browser, 'draw_circle_1'); - await clickBlock(this.browser, block, {button: 0} as ClickOptions); // Cut and paste await this.browser.keys([Key.Ctrl, 'x']); diff --git a/test/webdriverio/test/test_setup.ts b/test/webdriverio/test/test_setup.ts index 2869a799..7e108295 100644 --- a/test/webdriverio/test/test_setup.ts +++ b/test/webdriverio/test/test_setup.ts @@ -260,6 +260,34 @@ export async function getBlockElementById( return elem; } +/** + * Uses tabs to navigate to the workspace on the test page (i.e. by going + * through top-level tab stops). + * + * @param browser The active WebdriverIO Browser object. + * @param hasToolbox Whether a toolbox is configured on the test page. + * @param hasFlyout Whether a flyout is configured on the test page. + */ +export async function tabNavigateToWorkspace( + browser: WebdriverIO.Browser, + hasToolbox = true, + hasFlyout = true, +) { + if (hasToolbox) tabNavigateForward(browser); + if (hasFlyout) tabNavigateForward(browser); + tabNavigateForward(browser); // Tab to the workspace itself. +} + +/** + * Navigates forward to the test page's next tab stop. + * + * @param browser The active WebdriverIO Browser object. + */ +export async function tabNavigateForward(browser: WebdriverIO.Browser) { + await browser.keys(webdriverio.Key.Tab); + await browser.pause(PAUSE_TIME); +} + /** * Copied from blockly browser test_setup.mjs and amended for typescript * From d0fcd5970bca43354e88ec70b07d1c0adada68c7 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Fri, 2 May 2025 18:59:07 +0000 Subject: [PATCH 8/8] chore: restore type imports. Accidentally moved to full imports in an earlier commit. This addresses a review comment. --- src/actions/enter.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/actions/enter.ts b/src/actions/enter.ts index 4251de4a..161199f6 100644 --- a/src/actions/enter.ts +++ b/src/actions/enter.ts @@ -9,12 +9,15 @@ import { Events, ShortcutRegistry, utils as BlocklyUtils, + getFocusManager, +} from 'blockly/core'; + +import type { Block, BlockSvg, Field, FlyoutButton, WorkspaceSvg, - getFocusManager, } from 'blockly/core'; import * as Constants from '../constants';