From 41c8215465a538edd59ffefae067c920c1ce147d Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Mon, 21 Apr 2025 23:26:46 +0000 Subject: [PATCH 01/43] feat: Make WorkspaceSvg focusable. This adds basic support for WorkspaceSvg being focusable via its blocks, though fields and connections are not yet supported. --- core/block_svg.ts | 30 +++++++++++- core/inject.ts | 5 -- core/workspace_svg.ts | 77 ++++++++++++++++++++++++++++++- tests/mocha/workspace_svg_test.js | 2 +- 4 files changed, 105 insertions(+), 9 deletions(-) diff --git a/core/block_svg.ts b/core/block_svg.ts index b8712b01914..7b11e142148 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -44,6 +44,8 @@ import {IContextMenu} from './interfaces/i_contextmenu.js'; import type {ICopyable} from './interfaces/i_copyable.js'; import {IDeletable} from './interfaces/i_deletable.js'; import type {IDragStrategy, IDraggable} from './interfaces/i_draggable.js'; +import type {IFocusableNode} from './interfaces/i_focusable_node.js'; +import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import {IIcon} from './interfaces/i_icon.js'; import * as internalConstants from './internal_constants.js'; import {MarkerManager} from './marker_manager.js'; @@ -76,7 +78,8 @@ export class BlockSvg IContextMenu, ICopyable, IDraggable, - IDeletable + IDeletable, + IFocusableNode { /** * Constant for identifying rows that are to be rendered inline. @@ -210,6 +213,7 @@ export class BlockSvg // Expose this block's ID on its top-level SVG group. this.svgGroup.setAttribute('data-id', this.id); + svgPath.id = this.id; this.doInit_(); } @@ -1819,4 +1823,28 @@ export class BlockSvg ); } } + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + return this.pathObject.svgPath; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this.workspace; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void { + if (!this.isShadow()) { + common.setSelected(this); + } + } + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void { + if (common.getSelected() === this) { + common.setSelected(null); + } + } } diff --git a/core/inject.ts b/core/inject.ts index de78fbfae75..34d9c1795f8 100644 --- a/core/inject.ts +++ b/core/inject.ts @@ -13,13 +13,11 @@ import * as common from './common.js'; import * as Css from './css.js'; import * as dropDownDiv from './dropdowndiv.js'; import {Grid} from './grid.js'; -import {Msg} from './msg.js'; import {Options} from './options.js'; import {ScrollbarPair} from './scrollbar_pair.js'; import {ShortcutRegistry} from './shortcut_registry.js'; import * as Tooltip from './tooltip.js'; import * as Touch from './touch.js'; -import * as aria from './utils/aria.js'; import * as dom from './utils/dom.js'; import {Svg} from './utils/svg.js'; import * as WidgetDiv from './widgetdiv.js'; @@ -56,8 +54,6 @@ export function inject( if (opt_options?.rtl) { dom.addClass(subContainer, 'blocklyRTL'); } - subContainer.tabIndex = 0; - aria.setState(subContainer, aria.State.LABEL, Msg['WORKSPACE_ARIA_LABEL']); containerElement!.appendChild(subContainer); const svg = createDom(subContainer, options); @@ -126,7 +122,6 @@ function createDom(container: HTMLElement, options: Options): SVGElement { 'xmlns:xlink': dom.XLINK_NS, 'version': '1.1', 'class': 'blocklySvg', - 'tabindex': '0', }, container, ); diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index 91668b744d4..6d6505d0545 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -37,6 +37,7 @@ import {EventType} from './events/type.js'; import * as eventUtils from './events/utils.js'; import {Flyout} from './flyout_base.js'; import type {FlyoutButton} from './flyout_button.js'; +import {getFocusManager} from './focus_manager.js'; import {Gesture} from './gesture.js'; import {Grid} from './grid.js'; import type {IASTNodeLocationSvg} from './interfaces/i_ast_node_location_svg.js'; @@ -44,6 +45,8 @@ import type {IBoundedElement} from './interfaces/i_bounded_element.js'; import {IContextMenu} from './interfaces/i_contextmenu.js'; import type {IDragTarget} from './interfaces/i_drag_target.js'; import type {IFlyout} from './interfaces/i_flyout.js'; +import type {IFocusableNode} from './interfaces/i_focusable_node.js'; +import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import type {IMetricsManager} from './interfaces/i_metrics_manager.js'; import type {IToolbox} from './interfaces/i_toolbox.js'; import type { @@ -54,6 +57,7 @@ import type {LineCursor} from './keyboard_nav/line_cursor.js'; import type {Marker} from './keyboard_nav/marker.js'; import {LayerManager} from './layer_manager.js'; import {MarkerManager} from './marker_manager.js'; +import {Msg} from './msg.js'; import {Options} from './options.js'; import * as Procedures from './procedures.js'; import * as registry from './registry.js'; @@ -66,6 +70,7 @@ import {Classic} from './theme/classic.js'; import {ThemeManager} from './theme_manager.js'; import * as Tooltip from './tooltip.js'; import type {Trashcan} from './trashcan.js'; +import * as aria from './utils/aria.js'; import * as arrayUtils from './utils/array.js'; import {Coordinate} from './utils/coordinate.js'; import * as dom from './utils/dom.js'; @@ -93,7 +98,7 @@ const ZOOM_TO_FIT_MARGIN = 20; */ export class WorkspaceSvg extends Workspace - implements IASTNodeLocationSvg, IContextMenu + implements IASTNodeLocationSvg, IContextMenu, IFocusableNode, IFocusableTree { /** * A wrapper function called when a resize event occurs. @@ -764,7 +769,19 @@ export class WorkspaceSvg * * */ - this.svgGroup_ = dom.createSvgElement(Svg.G, {'class': 'blocklyWorkspace'}); + this.svgGroup_ = dom.createSvgElement(Svg.G, { + 'class': 'blocklyWorkspace', + // Only the main workspace should be tabbable. + 'tabindex': injectionDiv ? '0' : '-1', + 'id': this.id, + }); + if (injectionDiv) { + aria.setState( + this.svgGroup_, + aria.State.LABEL, + Msg['WORKSPACE_ARIA_LABEL'], + ); + } // Note that a alone does not receive mouse events--it must have a // valid target inside it. If no background class is specified, as in the @@ -840,6 +857,9 @@ export class WorkspaceSvg this.getTheme(), isParentWorkspace ? this.getInjectionDiv() : undefined, ); + + getFocusManager().registerTree(this); + return this.svgGroup_; } @@ -924,6 +944,10 @@ export class WorkspaceSvg document.body.removeEventListener('wheel', this.dummyWheelListener); this.dummyWheelListener = null; } + + if (getFocusManager().isRegistered(this)) { + getFocusManager().unregisterTree(this); + } } /** @@ -2618,6 +2642,55 @@ export class WorkspaceSvg deltaY *= scale; this.scroll(this.scrollX + deltaX, this.scrollY + deltaY); } + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + return this.svgGroup_; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void {} + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void {} + + /** See IFocusableTree.getRootFocusableNode. */ + getRootFocusableNode(): IFocusableNode { + return this; + } + + /** See IFocusableTree.getRestoredFocusableNode. */ + getRestoredFocusableNode( + previousNode: IFocusableNode | null, + ): IFocusableNode | null { + if (!previousNode) { + return this.getTopBlocks(true)[0] ?? null; + } else return null; + } + + /** See IFocusableTree.getNestedTrees. */ + getNestedTrees(): Array { + return []; + } + + /** See IFocusableTree.lookUpFocusableNode. */ + lookUpFocusableNode(id: string): IFocusableNode | null { + return this.getBlockById(id) as IFocusableNode; + } + + /** See IFocusableTree.onTreeFocus. */ + onTreeFocus( + _node: IFocusableNode, + _previousTree: IFocusableTree | null, + ): void {} + + /** See IFocusableTree.onTreeBlur. */ + onTreeBlur(_nextTree: IFocusableTree | null): void {} } /** diff --git a/tests/mocha/workspace_svg_test.js b/tests/mocha/workspace_svg_test.js index 207cad45dc7..f13ba27b16b 100644 --- a/tests/mocha/workspace_svg_test.js +++ b/tests/mocha/workspace_svg_test.js @@ -18,7 +18,7 @@ import { } from './test_helpers/setup_teardown.js'; import {testAWorkspace} from './test_helpers/workspace.js'; -suite('WorkspaceSvg', function () { +suite.only('WorkspaceSvg', function () { setup(function () { this.clock = sharedTestSetup.call(this, {fireEventsNow: false}).clock; const toolbox = document.getElementById('toolbox-categories'); From 14b486e6cc25402d5cefb4852fce35f0018c55f0 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Mon, 21 Apr 2025 23:50:40 +0000 Subject: [PATCH 02/43] chore: remove accidental 'test.only'. --- tests/mocha/workspace_svg_test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/mocha/workspace_svg_test.js b/tests/mocha/workspace_svg_test.js index f13ba27b16b..207cad45dc7 100644 --- a/tests/mocha/workspace_svg_test.js +++ b/tests/mocha/workspace_svg_test.js @@ -18,7 +18,7 @@ import { } from './test_helpers/setup_teardown.js'; import {testAWorkspace} from './test_helpers/workspace.js'; -suite.only('WorkspaceSvg', function () { +suite('WorkspaceSvg', function () { setup(function () { this.clock = sharedTestSetup.call(this, {fireEventsNow: false}).clock; const toolbox = document.getElementById('toolbox-categories'); From 26cf8dbf8a2c0a59f6694b0fce2ee4d8107d8b51 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 22 Apr 2025 00:17:50 +0000 Subject: [PATCH 03/43] feat: Make Toolbox & Flyout focusable. This introduces the fundamental support needed to ensure that both toolboxes and flyouts are focusable using FocusManager. --- core/flyout_base.ts | 69 +++++++++++++++++++++++++++- core/interfaces/i_flyout.ts | 3 +- core/interfaces/i_toolbox.ts | 3 +- core/interfaces/i_toolbox_item.ts | 4 +- core/toolbox/category.ts | 2 + core/toolbox/separator.ts | 2 + core/toolbox/toolbox.ts | 74 ++++++++++++++++++++++++++++++- core/toolbox/toolbox_item.ts | 24 ++++++++++ core/workspace_svg.ts | 14 +++++- tests/mocha/toolbox_test.js | 3 ++ 10 files changed, 191 insertions(+), 7 deletions(-) diff --git a/core/flyout_base.ts b/core/flyout_base.ts index e738470a606..d24ea2758a0 100644 --- a/core/flyout_base.ts +++ b/core/flyout_base.ts @@ -21,9 +21,12 @@ import * as eventUtils from './events/utils.js'; import {FlyoutItem} from './flyout_item.js'; import {FlyoutMetricsManager} from './flyout_metrics_manager.js'; import {FlyoutSeparator, SeparatorAxis} from './flyout_separator.js'; +import {getFocusManager} from './focus_manager.js'; import {IAutoHideable} from './interfaces/i_autohideable.js'; import type {IFlyout} from './interfaces/i_flyout.js'; import type {IFlyoutInflater} from './interfaces/i_flyout_inflater.js'; +import {IFocusableNode} from './interfaces/i_focusable_node.js'; +import {IFocusableTree} from './interfaces/i_focusable_tree.js'; import type {Options} from './options.js'; import * as registry from './registry.js'; import * as renderManagement from './render_management.js'; @@ -43,7 +46,7 @@ import {WorkspaceSvg} from './workspace_svg.js'; */ export abstract class Flyout extends DeleteArea - implements IAutoHideable, IFlyout + implements IAutoHideable, IFlyout, IFocusableNode { /** * Position the flyout. @@ -303,6 +306,7 @@ export abstract class Flyout // hide/show code will set up proper visibility and size later. this.svgGroup_ = dom.createSvgElement(tagName, { 'class': 'blocklyFlyout', + 'tabindex': '0', }); this.svgGroup_.style.display = 'none'; this.svgBackground_ = dom.createSvgElement( @@ -317,6 +321,9 @@ export abstract class Flyout this.workspace_ .getThemeManager() .subscribe(this.svgBackground_, 'flyoutOpacity', 'fill-opacity'); + + getFocusManager().registerTree(this); + return this.svgGroup_; } @@ -398,6 +405,7 @@ export abstract class Flyout if (this.svgGroup_) { dom.removeNode(this.svgGroup_); } + getFocusManager().unregisterTree(this); } /** @@ -961,4 +969,63 @@ export abstract class Flyout return null; } + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + if (!this.svgGroup_) throw new Error('Flyout DOM is not yet created.'); + return this.svgGroup_; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void {} + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void {} + + /** See IFocusableTree.getRootFocusableNode. */ + getRootFocusableNode(): IFocusableNode { + return this; + } + + /** See IFocusableTree.getRestoredFocusableNode. */ + getRestoredFocusableNode( + _previousNode: IFocusableNode | null, + ): IFocusableNode | null { + return null; + } + + /** See IFocusableTree.getNestedTrees. */ + getNestedTrees(): Array { + return [this.workspace_]; + } + + /** See IFocusableTree.lookUpFocusableNode. */ + lookUpFocusableNode(_id: string): IFocusableNode | null { + // No focusable node needs to be returned since the flyout's subtree is a + // workspace that will manage its own focusable state. + return null; + } + + /** See IFocusableTree.onTreeFocus. */ + onTreeFocus( + _node: IFocusableNode, + _previousTree: IFocusableTree | null, + ): void {} + + /** See IFocusableTree.onTreeBlur. */ + onTreeBlur(nextTree: IFocusableTree | null): void { + const toolbox = this.targetWorkspace.getToolbox(); + // If focus is moving to either the toolbox or the flyout's workspace, do + // not close the flyout. For anything else, do close it since the flyout is + // no longer focused. + if (toolbox && nextTree === toolbox) return; + if (nextTree == this.workspace_) return; + if (toolbox) toolbox.clearSelection(); + this.autoHide(false); + } } diff --git a/core/interfaces/i_flyout.ts b/core/interfaces/i_flyout.ts index 42204775ece..067cd5ef20d 100644 --- a/core/interfaces/i_flyout.ts +++ b/core/interfaces/i_flyout.ts @@ -12,12 +12,13 @@ import type {Coordinate} from '../utils/coordinate.js'; import type {Svg} from '../utils/svg.js'; import type {FlyoutDefinition} from '../utils/toolbox.js'; import type {WorkspaceSvg} from '../workspace_svg.js'; +import {IFocusableTree} from './i_focusable_tree.js'; import type {IRegistrable} from './i_registrable.js'; /** * Interface for a flyout. */ -export interface IFlyout extends IRegistrable { +export interface IFlyout extends IRegistrable, IFocusableTree { /** Whether the flyout is laid out horizontally or not. */ horizontalLayout: boolean; diff --git a/core/interfaces/i_toolbox.ts b/core/interfaces/i_toolbox.ts index 1780af94d8a..f5d9c9fd7c6 100644 --- a/core/interfaces/i_toolbox.ts +++ b/core/interfaces/i_toolbox.ts @@ -9,13 +9,14 @@ import type {ToolboxInfo} from '../utils/toolbox.js'; import type {WorkspaceSvg} from '../workspace_svg.js'; import type {IFlyout} from './i_flyout.js'; +import type {IFocusableTree} from './i_focusable_tree.js'; import type {IRegistrable} from './i_registrable.js'; import type {IToolboxItem} from './i_toolbox_item.js'; /** * Interface for a toolbox. */ -export interface IToolbox extends IRegistrable { +export interface IToolbox extends IRegistrable, IFocusableTree { /** Initializes the toolbox. */ init(): void; diff --git a/core/interfaces/i_toolbox_item.ts b/core/interfaces/i_toolbox_item.ts index e3c9864f0c0..661624fd7e8 100644 --- a/core/interfaces/i_toolbox_item.ts +++ b/core/interfaces/i_toolbox_item.ts @@ -6,10 +6,12 @@ // Former goog.module ID: Blockly.IToolboxItem +import type {IFocusableNode} from './i_focusable_node.js'; + /** * Interface for an item in the toolbox. */ -export interface IToolboxItem { +export interface IToolboxItem extends IFocusableNode { /** * Initializes the toolbox item. * This includes creating the DOM and updating the state of any items based diff --git a/core/toolbox/category.ts b/core/toolbox/category.ts index d8ee8736ea6..fc7d1aa03cf 100644 --- a/core/toolbox/category.ts +++ b/core/toolbox/category.ts @@ -225,6 +225,8 @@ export class ToolboxCategory */ protected createContainer_(): HTMLDivElement { const container = document.createElement('div'); + container.tabIndex = -1; + container.id = this.getId(); const className = this.cssConfig_['container']; if (className) { dom.addClass(container, className); diff --git a/core/toolbox/separator.ts b/core/toolbox/separator.ts index 31ccb7e42f3..44ae358cf53 100644 --- a/core/toolbox/separator.ts +++ b/core/toolbox/separator.ts @@ -54,6 +54,8 @@ export class ToolboxSeparator extends ToolboxItem { */ protected createDom_(): HTMLDivElement { const container = document.createElement('div'); + container.tabIndex = -1; + container.id = this.getId(); const className = this.cssConfig_['container']; if (className) { dom.addClass(container, className); diff --git a/core/toolbox/toolbox.ts b/core/toolbox/toolbox.ts index b0fd82e97f2..f44ebac5348 100644 --- a/core/toolbox/toolbox.ts +++ b/core/toolbox/toolbox.ts @@ -22,11 +22,14 @@ import {DeleteArea} from '../delete_area.js'; import '../events/events_toolbox_item_select.js'; import {EventType} from '../events/type.js'; import * as eventUtils from '../events/utils.js'; +import {getFocusManager} from '../focus_manager.js'; import type {IAutoHideable} from '../interfaces/i_autohideable.js'; import type {ICollapsibleToolboxItem} from '../interfaces/i_collapsible_toolbox_item.js'; import {isDeletable} from '../interfaces/i_deletable.js'; import type {IDraggable} from '../interfaces/i_draggable.js'; import type {IFlyout} from '../interfaces/i_flyout.js'; +import type {IFocusableNode} from '../interfaces/i_focusable_node.js'; +import type {IFocusableTree} from '../interfaces/i_focusable_tree.js'; import type {IKeyboardAccessible} from '../interfaces/i_keyboard_accessible.js'; import type {ISelectableToolboxItem} from '../interfaces/i_selectable_toolbox_item.js'; import {isSelectableToolboxItem} from '../interfaces/i_selectable_toolbox_item.js'; @@ -51,7 +54,12 @@ import {CollapsibleToolboxCategory} from './collapsible_category.js'; */ export class Toolbox extends DeleteArea - implements IAutoHideable, IKeyboardAccessible, IStyleable, IToolbox + implements + IAutoHideable, + IKeyboardAccessible, + IStyleable, + IToolbox, + IFocusableNode { /** * The unique ID for this component that is used to register with the @@ -163,6 +171,7 @@ export class Toolbox ComponentManager.Capability.DRAG_TARGET, ], }); + getFocusManager().registerTree(this); } /** @@ -177,7 +186,6 @@ export class Toolbox const container = this.createContainer_(); this.contentsDiv_ = this.createContentsContainer_(); - this.contentsDiv_.tabIndex = 0; aria.setRole(this.contentsDiv_, aria.Role.TREE); container.appendChild(this.contentsDiv_); @@ -194,6 +202,7 @@ export class Toolbox */ protected createContainer_(): HTMLDivElement { const toolboxContainer = document.createElement('div'); + toolboxContainer.tabIndex = 0; toolboxContainer.setAttribute('layout', this.isHorizontal() ? 'h' : 'v'); dom.addClass(toolboxContainer, 'blocklyToolbox'); toolboxContainer.setAttribute('dir', this.RTL ? 'RTL' : 'LTR'); @@ -1077,7 +1086,68 @@ export class Toolbox this.workspace_.getThemeManager().unsubscribe(this.HtmlDiv); dom.removeNode(this.HtmlDiv); } + + getFocusManager().unregisterTree(this); + } + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + if (!this.HtmlDiv) throw Error('Toolbox DOM has not yet been created.'); + return this.HtmlDiv; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this; } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void {} + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void {} + + /** See IFocusableTree.getRootFocusableNode. */ + getRootFocusableNode(): IFocusableNode { + return this; + } + + /** See IFocusableTree.getRestoredFocusableNode. */ + getRestoredFocusableNode( + previousNode: IFocusableNode | null, + ): IFocusableNode | null { + // Always try to select the first selectable toolbox item rather than the + // root of the toolbox. + if (!previousNode || previousNode === this) { + return this.getToolboxItems().find((item) => item.isSelectable()) ?? null; + } else return null; + } + + /** See IFocusableTree.getNestedTrees. */ + getNestedTrees(): Array { + return []; + } + + /** See IFocusableTree.lookUpFocusableNode. */ + lookUpFocusableNode(id: string): IFocusableNode | null { + return this.getToolboxItemById(id) as IFocusableNode; + } + + /** See IFocusableTree.onTreeFocus. */ + onTreeFocus( + node: IFocusableNode, + _previousTree: IFocusableTree | null, + ): void { + if (node !== this) { + // Only select the item if it isn't already selected so as to not toggle. + if (this.getSelectedItem() !== node) { + this.setSelectedItem(node as IToolboxItem); + } + } else this.clearSelection(); + } + + /** See IFocusableTree.onTreeBlur. */ + onTreeBlur(_nextTree: IFocusableTree | null): void {} } /** CSS for Toolbox. See css.js for use. */ diff --git a/core/toolbox/toolbox_item.ts b/core/toolbox/toolbox_item.ts index ef9d979ab43..0d46a5eadfd 100644 --- a/core/toolbox/toolbox_item.ts +++ b/core/toolbox/toolbox_item.ts @@ -12,6 +12,7 @@ // Former goog.module ID: Blockly.ToolboxItem import type {ICollapsibleToolboxItem} from '../interfaces/i_collapsible_toolbox_item.js'; +import type {IFocusableTree} from '../interfaces/i_focusable_tree.js'; import type {IToolbox} from '../interfaces/i_toolbox.js'; import type {IToolboxItem} from '../interfaces/i_toolbox_item.js'; import * as idGenerator from '../utils/idgenerator.js'; @@ -148,5 +149,28 @@ export class ToolboxItem implements IToolboxItem { * @param _isVisible True if category should be visible. */ setVisible_(_isVisible: boolean) {} + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + const div = this.getDiv(); + if (!div) { + throw Error('Trying to access toolbox item before DOM is initialized.'); + } + if (!(div instanceof HTMLElement)) { + throw Error('Toolbox item div is unexpectedly not an HTML element.'); + } + return div as HTMLElement; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this.parentToolbox_; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void {} + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void {} } // nop by default diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index 6d6505d0545..66b21e9a731 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -2690,7 +2690,19 @@ export class WorkspaceSvg ): void {} /** See IFocusableTree.onTreeBlur. */ - onTreeBlur(_nextTree: IFocusableTree | null): void {} + onTreeBlur(nextTree: IFocusableTree | null): void { + // If the flyout loses focus, make sure to close it. + if (this.isFlyout && this.targetWorkspace) { + // Only hide the flyout if the flyout's workspace is losing focus and that + // focus isn't returning to the flyout itself or the toolbox. + const flyout = this.targetWorkspace.getFlyout(); + const toolbox = this.targetWorkspace.getToolbox(); + if (flyout && nextTree === flyout) return; + if (toolbox && nextTree === toolbox) return; + if (toolbox) toolbox.clearSelection(); + if (flyout && flyout instanceof Flyout) flyout.autoHide(false); + } + } } /** diff --git a/tests/mocha/toolbox_test.js b/tests/mocha/toolbox_test.js index 10bfd335223..f32319c6779 100644 --- a/tests/mocha/toolbox_test.js +++ b/tests/mocha/toolbox_test.js @@ -54,6 +54,7 @@ suite('Toolbox', function () { const themeManagerSpy = sinon.spy(themeManager, 'subscribe'); const componentManager = this.toolbox.workspace_.getComponentManager(); sinon.stub(componentManager, 'addComponent'); + this.toolbox.dispose(); // Dispose of the old toolbox so that it can be reinited. this.toolbox.init(); sinon.assert.calledWith( themeManagerSpy, @@ -72,12 +73,14 @@ suite('Toolbox', function () { const renderSpy = sinon.spy(this.toolbox, 'render'); const componentManager = this.toolbox.workspace_.getComponentManager(); sinon.stub(componentManager, 'addComponent'); + this.toolbox.dispose(); // Dispose of the old toolbox so that it can be reinited. this.toolbox.init(); sinon.assert.calledOnce(renderSpy); }); test('Init called -> Flyout is initialized', function () { const componentManager = this.toolbox.workspace_.getComponentManager(); sinon.stub(componentManager, 'addComponent'); + this.toolbox.dispose(); // Dispose of the old toolbox so that it can be reinited. this.toolbox.init(); assert.isDefined(this.toolbox.getFlyout()); }); From d3acbff46f7826c78fcaaff2d8d28639aaf26a3f Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 22 Apr 2025 21:15:41 +0000 Subject: [PATCH 04/43] feat!: Force lifecycle methods for fields. This ensures that fields with editors properly signal when the editor is shown/hidden, and introduces show/hide callbacks that can be overridden to properly trigger ephemeral focus. --- blocks/procedures.ts | 6 ++-- core/field.ts | 23 +++++++++++--- core/field_checkbox.ts | 10 +++++- core/field_dropdown.ts | 18 ++++++++--- core/field_image.ts | 10 +++++- core/field_input.ts | 31 +++++++++++++++---- core/field_label.ts | 11 +++++++ tests/mocha/field_textinput_test.js | 4 +-- .../src/field/different_user_input.ts | 11 +++++++ 9 files changed, 104 insertions(+), 20 deletions(-) diff --git a/blocks/procedures.ts b/blocks/procedures.ts index 7284973d9cc..317eb82190b 100644 --- a/blocks/procedures.ts +++ b/blocks/procedures.ts @@ -648,8 +648,10 @@ class ProcedureArgumentField extends FieldTextInput { * * @param e The event that triggered display of the field editor. */ - protected override showEditor_(e?: Event) { - super.showEditor_(e); + protected override showEditor_( + onEditorShown: () => void, onEditorHidden: () => void, e?: Event + ) { + super.showEditor_(onEditorShown, onEditorHidden, e); this.editingInteractively = true; this.editingVariable = undefined; } diff --git a/core/field.ts b/core/field.ts index 725a2867d9e..cecb90ff385 100644 --- a/core/field.ts +++ b/core/field.ts @@ -770,7 +770,7 @@ export abstract class Field */ showEditor(e?: Event) { if (this.isClickable()) { - this.showEditor_(e); + this.showEditor_(() => this.onShowEditor(), () => this.onHideEditor(), e); } } @@ -778,11 +778,26 @@ export abstract class Field * A developer hook to create an editor for the field. This is no-op by * default, and must be overriden to create an editor. * - * @param _e Optional mouse event that triggered the field to open, or + * @param onEditorShown Callback that must be called when the editor is shown. + * @param onEditorHidden Callback that must be called when the editor hides. + * @param e Optional mouse event that triggered the field to open, or * undefined if triggered programmatically. */ - protected showEditor_(_e?: Event): void {} - // NOP + protected abstract showEditor_( + onEditorShown: () => void, onEditorHidden: () => void, e?: Event + ): void; + + /** + * Called when an editor is shown. This is expected to be used for ensuring + * that the editor has proper focus. + */ + protected abstract onShowEditor(): void; + + /** + * Called when an editor is hidden. This is expected to be used for ensuring + * that the editor no longer proper focus. + */ + protected abstract onHideEditor(): void; /** * A developer hook to reposition the WidgetDiv during a window resize. You diff --git a/core/field_checkbox.ts b/core/field_checkbox.ts index 55ed42cbf4b..3d32585ad98 100644 --- a/core/field_checkbox.ts +++ b/core/field_checkbox.ts @@ -136,10 +136,18 @@ export class FieldCheckbox extends Field { } /** Toggle the state of the checkbox on click. */ - protected override showEditor_() { + protected override showEditor_( + onEditorShown: () => void, onEditorHidden: () => void + ) { + onEditorShown(); this.setValue(!this.value_); + onEditorHidden(); } + protected override onShowEditor(): void {} + + protected override onHideEditor(): void {} + /** * Ensure that the input value is valid ('TRUE' or 'FALSE'). * diff --git a/core/field_dropdown.ts b/core/field_dropdown.ts index 81279e2a1f5..4afa2dffca7 100644 --- a/core/field_dropdown.ts +++ b/core/field_dropdown.ts @@ -269,7 +269,9 @@ export class FieldDropdown extends Field { * @param e Optional mouse event that triggered the field to open, or * undefined if triggered programmatically. */ - protected override showEditor_(e?: MouseEvent) { + protected override showEditor_( + onEditorShown: () => void, onEditorHidden: () => void, e?: MouseEvent + ) { const block = this.getSourceBlock(); if (!block) { throw new UnattachedFieldError(); @@ -277,6 +279,7 @@ export class FieldDropdown extends Field { this.dropdownCreate(); if (!this.menu_) return; + onEditorShown(); if (e && typeof e.clientX === 'number') { this.menu_.openingCoords = new Coordinate(e.clientX, e.clientY); } else { @@ -295,7 +298,10 @@ export class FieldDropdown extends Field { dropDownDiv.setColour(primaryColour, borderColour); } - dropDownDiv.showPositionedByField(this, this.dropdownDispose_.bind(this)); + dropDownDiv.showPositionedByField(this, () => { + this.dropdownDispose_.bind(this); + onEditorHidden(); + }); dropDownDiv.getContentDiv().style.height = `${this.menu_.getSize().height}px`; @@ -311,6 +317,10 @@ export class FieldDropdown extends Field { this.applyColour(); } + protected override onShowEditor(): void {} + + protected override onHideEditor(): void {} + /** Create the dropdown editor. */ private dropdownCreate() { const block = this.getSourceBlock(); @@ -769,7 +779,7 @@ export class FieldDropdown extends Field { } else if (typeof option[1] !== 'string') { foundError = true; console.error( - `Invalid option[${i}]: Each FieldDropdown option id must be a string. + `Invalid option[${i}]: Each FieldDropdown option id must be a string. Found ${option[1]} in: ${option}`, ); } else if ( @@ -780,7 +790,7 @@ export class FieldDropdown extends Field { ) { foundError = true; console.error( - `Invalid option[${i}]: Each FieldDropdown option must have a string + `Invalid option[${i}]: Each FieldDropdown option must have a string label, image description, or HTML element. Found ${option[0]} in: ${option}`, ); } diff --git a/core/field_image.ts b/core/field_image.ts index 650575f59a3..9b809e4058b 100644 --- a/core/field_image.ts +++ b/core/field_image.ts @@ -217,12 +217,20 @@ export class FieldImage extends Field { * If field click is called, and click handler defined, * call the handler. */ - protected override showEditor_() { + protected override showEditor_( + _onEditorShown: () => void, _onEditorHidden: () => void + ) { + // Note that an editor shouldn't be shown for this field, so the callbacks + // are ignored. if (this.clickHandler) { this.clickHandler(this); } } + protected override onShowEditor(): void {} + + protected override onHideEditor(): void {} + /** * Set the function that is called when this image is clicked. * diff --git a/core/field_input.ts b/core/field_input.ts index 2cdd8056553..615521c41cc 100644 --- a/core/field_input.ts +++ b/core/field_input.ts @@ -327,30 +327,42 @@ export abstract class FieldInput extends Field< * Shows a prompt editor for mobile browsers if the modalInputs option is * enabled. * + * @param onEditorShown Callback that must be called when the editor is shown. + * @param onEditorHidden Callback that must be called when the editor hides. * @param _e Optional mouse event that triggered the field to open, or * undefined if triggered programmatically. * @param quietInput True if editor should be created without focus. * Defaults to false. */ - protected override showEditor_(_e?: Event, quietInput = false) { + protected override showEditor_( + onEditorShown: () => void, onEditorHidden: () => void, _e?: Event, + quietInput: boolean = false + ) { this.workspace_ = (this.sourceBlock_ as BlockSvg).workspace; if ( !quietInput && this.workspace_.options.modalInputs && (userAgent.MOBILE || userAgent.ANDROID || userAgent.IPAD) ) { - this.showPromptEditor(); + this.showPromptEditor(onEditorShown, onEditorHidden); } else { - this.showInlineEditor(quietInput); + this.showInlineEditor(onEditorShown, onEditorHidden, quietInput); } } + protected override onShowEditor(): void {} + + protected override onHideEditor(): void {} + /** * Create and show a text input editor that is a prompt (usually a popup). * Mobile browsers may have issues with in-line textareas (focus and * keyboards). */ - private showPromptEditor() { + private showPromptEditor( + onEditorShown: () => void, onEditorHidden: () => void + ) { + onEditorShown(); dialog.prompt( Msg['CHANGE_VALUE_TITLE'], this.getText(), @@ -360,6 +372,7 @@ export abstract class FieldInput extends Field< this.setValue(this.getValueFromEditorText_(text)); } this.onFinishEditing_(this.value_); + onEditorHidden(); }, ); } @@ -369,7 +382,9 @@ export abstract class FieldInput extends Field< * * @param quietInput True if editor should be created without focus. */ - private showInlineEditor(quietInput: boolean) { + private showInlineEditor( + onEditorShown: () => void, onEditorHidden: () => void, quietInput: boolean + ) { const block = this.getSourceBlock(); if (!block) { throw new UnattachedFieldError(); @@ -377,9 +392,13 @@ export abstract class FieldInput extends Field< WidgetDiv.show( this, block.RTL, - this.widgetDispose_.bind(this), + () => { + this.widgetDispose_(); + onEditorHidden(); + }, this.workspace_, ); + onEditorShown(); this.htmlInput_ = this.widgetCreate_() as HTMLInputElement; this.isBeingEdited_ = true; this.valueWhenEditorWasOpened_ = this.value_; diff --git a/core/field_label.ts b/core/field_label.ts index 236154cc7b1..d108ac499fe 100644 --- a/core/field_label.ts +++ b/core/field_label.ts @@ -94,6 +94,17 @@ export class FieldLabel extends Field { return `${newValue}`; } + protected override showEditor_( + _onEditorShown: () => void, _onEditorHidden: () => void + ) { + // Note that an editor shouldn't be shown for this field, so the callbacks + // are ignored. + } + + protected override onShowEditor(): void {} + + protected override onHideEditor(): void {} + /** * Set the CSS class applied to the field's textElement_. * diff --git a/tests/mocha/field_textinput_test.js b/tests/mocha/field_textinput_test.js index 82c1a645e6d..4ab40d4f506 100644 --- a/tests/mocha/field_textinput_test.js +++ b/tests/mocha/field_textinput_test.js @@ -228,7 +228,7 @@ suite('Text Input Fields', function () { this.assertSpellcheck = function (field, value) { this.prepField(field); - field.showEditor_(); + field.showEditor_(() => {}, () => {}); assert.equal( field.htmlInput_.getAttribute('spellcheck'), value.toString(), @@ -265,7 +265,7 @@ suite('Text Input Fields', function () { test('setSpellcheck Editor Shown', function () { const field = new Blockly.FieldTextInput('test'); this.prepField(field); - field.showEditor_(); + field.showEditor_(() => {}, () => {}); field.setSpellcheck(false); assert.equal(field.htmlInput_.getAttribute('spellcheck'), 'false'); }); diff --git a/tests/typescript/src/field/different_user_input.ts b/tests/typescript/src/field/different_user_input.ts index f91f25bbc86..afc150da48d 100644 --- a/tests/typescript/src/field/different_user_input.ts +++ b/tests/typescript/src/field/different_user_input.ts @@ -61,6 +61,17 @@ class FieldMitosis extends Field { }); this.value_ = {cells}; } + + protected override showEditor_( + _onEditorShown: () => void, _onEditorHidden: () => void + ) { + // Note that an editor shouldn't be shown for this field, so the callbacks + // are ignored. + } + + protected override onShowEditor(): void {} + + protected override onHideEditor(): void {} } fieldRegistry.register('field_mitosis', FieldMitosis); From ed0f14070d80611da409c9b11701cd87d06b1db5 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 22 Apr 2025 21:29:54 +0000 Subject: [PATCH 05/43] feat: Make fields ephemerally focusable. --- core/field.ts | 43 +++++++++++++++++++++++++++++++++++++++--- core/field_checkbox.ts | 6 +++--- core/field_dropdown.ts | 15 +++++++++++++-- core/field_input.ts | 12 ++++++++++-- 4 files changed, 66 insertions(+), 10 deletions(-) diff --git a/core/field.ts b/core/field.ts index cecb90ff385..bc091955f76 100644 --- a/core/field.ts +++ b/core/field.ts @@ -25,6 +25,8 @@ import * as eventUtils from './events/utils.js'; import type {Input} from './inputs/input.js'; import type {IASTNodeLocationSvg} from './interfaces/i_ast_node_location_svg.js'; import type {IASTNodeLocationWithBlock} from './interfaces/i_ast_node_location_with_block.js'; +import type {IFocusableNode} from './interfaces/i_focusable_node.js'; +import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import type {IKeyboardAccessible} from './interfaces/i_keyboard_accessible.js'; import type {IRegistrable} from './interfaces/i_registrable.js'; import {ISerializable} from './interfaces/i_serializable.js'; @@ -42,7 +44,8 @@ import {Svg} from './utils/svg.js'; import * as userAgent from './utils/useragent.js'; import * as utilsXml from './utils/xml.js'; import * as WidgetDiv from './widgetdiv.js'; -import type {WorkspaceSvg} from './workspace_svg.js'; +import {WorkspaceSvg} from './workspace_svg.js'; +import * as idGenerator from './utils/idgenerator.js'; /** * A function that is called to validate changes to the field's value before @@ -72,7 +75,8 @@ export abstract class Field IASTNodeLocationWithBlock, IKeyboardAccessible, IRegistrable, - ISerializable + ISerializable, + IFocusableNode { /** * To overwrite the default value which is set in **Field**, directly update @@ -191,6 +195,8 @@ export abstract class Field */ SERIALIZABLE = false; + private id_: string | null = null; + /** * @param value The initial value of the field. * Also accepts Field.SKIP_SETUP if you wish to skip setup (only used by @@ -255,6 +261,7 @@ export abstract class Field throw Error('Field already bound to a block'); } this.sourceBlock_ = block; + this.id_ = `${block.id}_field_${idGenerator.getNextUniqueId()}`; } /** @@ -298,7 +305,13 @@ export abstract class Field // Field has already been initialized once. return; } - this.fieldGroup_ = dom.createSvgElement(Svg.G, {}); + const id = this.id_; + if (!id) throw new Error('Expected ID to be defined prior to init.'); + this.fieldGroup_ = dom.createSvgElement(Svg.G, { + 'tabindex': '-1', + 'id': id, + 'aria-label': 'Field ' + this.name, + }); if (!this.isVisible()) { this.fieldGroup_.style.display = 'none'; } @@ -1416,6 +1429,30 @@ export abstract class Field } } + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + if (!this.fieldGroup_) { + throw Error("This field currently has no representative DOM element."); + } + return this.fieldGroup_; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + const block = this.getSourceBlock(); + if (!block) { + throw new UnattachedFieldError(); + } + // TODO: Remove the latter two casts once WorkspaceSvg is a focusable tree. + return block.workspace as WorkspaceSvg as unknown as IFocusableTree; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void {} + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void {} + /** * Subclasses should reimplement this method to construct their Field * subclass from a JSON arg object. diff --git a/core/field_checkbox.ts b/core/field_checkbox.ts index 3d32585ad98..ac4cda28df0 100644 --- a/core/field_checkbox.ts +++ b/core/field_checkbox.ts @@ -137,11 +137,11 @@ export class FieldCheckbox extends Field { /** Toggle the state of the checkbox on click. */ protected override showEditor_( - onEditorShown: () => void, onEditorHidden: () => void + _onEditorShown: () => void, _onEditorHidden: () => void ) { - onEditorShown(); + // There's no explicit editor for checkboxes, so let DOM focus be handled + // normally. this.setValue(!this.value_); - onEditorHidden(); } protected override onShowEditor(): void {} diff --git a/core/field_dropdown.ts b/core/field_dropdown.ts index 4afa2dffca7..ca5694fcf2f 100644 --- a/core/field_dropdown.ts +++ b/core/field_dropdown.ts @@ -22,6 +22,7 @@ import { UnattachedFieldError, } from './field.js'; import * as fieldRegistry from './field_registry.js'; +import { getFocusManager } from './focus_manager.js'; import {Menu} from './menu.js'; import {MenuSeparator} from './menu_separator.js'; import {MenuItem} from './menuitem.js'; @@ -88,6 +89,8 @@ export class FieldDropdown extends Field { private selectedOption!: MenuOption; override clickTarget_: SVGElement | null = null; + private returnFocusCallback: (() => void) | null = null; + /** * The y offset from the top of the field to the top of the image, if an image * is selected. @@ -317,9 +320,17 @@ export class FieldDropdown extends Field { this.applyColour(); } - protected override onShowEditor(): void {} + protected override onShowEditor(): void { + const menuElement = this.menu_?.getElement(); + if (menuElement) { + this.returnFocusCallback = + getFocusManager().takeEphemeralFocus(menuElement); + } + } - protected override onHideEditor(): void {} + protected override onHideEditor(): void { + if (this.returnFocusCallback) this.returnFocusCallback(); + } /** Create the dropdown editor. */ private dropdownCreate() { diff --git a/core/field_input.ts b/core/field_input.ts index 615521c41cc..d9851d3fb17 100644 --- a/core/field_input.ts +++ b/core/field_input.ts @@ -35,6 +35,7 @@ import {Size} from './utils/size.js'; import * as userAgent from './utils/useragent.js'; import * as WidgetDiv from './widgetdiv.js'; import type {WorkspaceSvg} from './workspace_svg.js'; +import { getFocusManager } from './focus_manager.js'; /** * Supported types for FieldInput subclasses. @@ -100,6 +101,8 @@ export abstract class FieldInput extends Field< */ override SERIALIZABLE = true; + private returnFocusCallback: (() => void) | null = null; + /** * @param value The initial value of the field. Should cast to a string. * Defaults to an empty string if null or undefined. Also accepts @@ -350,9 +353,14 @@ export abstract class FieldInput extends Field< } } - protected override onShowEditor(): void {} + protected override onShowEditor(): void { + this.returnFocusCallback = + getFocusManager().takeEphemeralFocus(document.body); + } - protected override onHideEditor(): void {} + protected override onHideEditor(): void { + if (this.returnFocusCallback) this.returnFocusCallback(); + } /** * Create and show a text input editor that is a prompt (usually a popup). From 94672d98460f49994e02580bace4233a805cd53c Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 22 Apr 2025 21:32:50 +0000 Subject: [PATCH 06/43] chore: Lint fixes. --- blocks/procedures.ts | 4 +++- core/field.ts | 14 +++++++++---- core/field_checkbox.ts | 3 ++- core/field_dropdown.ts | 6 ++++-- core/field_image.ts | 3 ++- core/field_input.ts | 20 ++++++++++++------- core/field_label.ts | 3 ++- tests/mocha/field_textinput_test.js | 10 ++++++++-- .../src/field/different_user_input.ts | 3 ++- 9 files changed, 46 insertions(+), 20 deletions(-) diff --git a/blocks/procedures.ts b/blocks/procedures.ts index 317eb82190b..656794002b9 100644 --- a/blocks/procedures.ts +++ b/blocks/procedures.ts @@ -649,7 +649,9 @@ class ProcedureArgumentField extends FieldTextInput { * @param e The event that triggered display of the field editor. */ protected override showEditor_( - onEditorShown: () => void, onEditorHidden: () => void, e?: Event + onEditorShown: () => void, + onEditorHidden: () => void, + e?: Event, ) { super.showEditor_(onEditorShown, onEditorHidden, e); this.editingInteractively = true; diff --git a/core/field.ts b/core/field.ts index bc091955f76..8b912d237bb 100644 --- a/core/field.ts +++ b/core/field.ts @@ -36,6 +36,7 @@ import type {KeyboardShortcut} from './shortcut_registry.js'; import * as Tooltip from './tooltip.js'; import type {Coordinate} from './utils/coordinate.js'; import * as dom from './utils/dom.js'; +import * as idGenerator from './utils/idgenerator.js'; import * as parsing from './utils/parsing.js'; import {Rect} from './utils/rect.js'; import {Size} from './utils/size.js'; @@ -45,7 +46,6 @@ import * as userAgent from './utils/useragent.js'; import * as utilsXml from './utils/xml.js'; import * as WidgetDiv from './widgetdiv.js'; import {WorkspaceSvg} from './workspace_svg.js'; -import * as idGenerator from './utils/idgenerator.js'; /** * A function that is called to validate changes to the field's value before @@ -783,7 +783,11 @@ export abstract class Field */ showEditor(e?: Event) { if (this.isClickable()) { - this.showEditor_(() => this.onShowEditor(), () => this.onHideEditor(), e); + this.showEditor_( + () => this.onShowEditor(), + () => this.onHideEditor(), + e, + ); } } @@ -797,7 +801,9 @@ export abstract class Field * undefined if triggered programmatically. */ protected abstract showEditor_( - onEditorShown: () => void, onEditorHidden: () => void, e?: Event + onEditorShown: () => void, + onEditorHidden: () => void, + e?: Event, ): void; /** @@ -1432,7 +1438,7 @@ export abstract class Field /** See IFocusableNode.getFocusableElement. */ getFocusableElement(): HTMLElement | SVGElement { if (!this.fieldGroup_) { - throw Error("This field currently has no representative DOM element."); + throw Error('This field currently has no representative DOM element.'); } return this.fieldGroup_; } diff --git a/core/field_checkbox.ts b/core/field_checkbox.ts index ac4cda28df0..1feb65a85c3 100644 --- a/core/field_checkbox.ts +++ b/core/field_checkbox.ts @@ -137,7 +137,8 @@ export class FieldCheckbox extends Field { /** Toggle the state of the checkbox on click. */ protected override showEditor_( - _onEditorShown: () => void, _onEditorHidden: () => void + _onEditorShown: () => void, + _onEditorHidden: () => void, ) { // There's no explicit editor for checkboxes, so let DOM focus be handled // normally. diff --git a/core/field_dropdown.ts b/core/field_dropdown.ts index ca5694fcf2f..616c00ce689 100644 --- a/core/field_dropdown.ts +++ b/core/field_dropdown.ts @@ -22,7 +22,7 @@ import { UnattachedFieldError, } from './field.js'; import * as fieldRegistry from './field_registry.js'; -import { getFocusManager } from './focus_manager.js'; +import {getFocusManager} from './focus_manager.js'; import {Menu} from './menu.js'; import {MenuSeparator} from './menu_separator.js'; import {MenuItem} from './menuitem.js'; @@ -273,7 +273,9 @@ export class FieldDropdown extends Field { * undefined if triggered programmatically. */ protected override showEditor_( - onEditorShown: () => void, onEditorHidden: () => void, e?: MouseEvent + onEditorShown: () => void, + onEditorHidden: () => void, + e?: MouseEvent, ) { const block = this.getSourceBlock(); if (!block) { diff --git a/core/field_image.ts b/core/field_image.ts index 9b809e4058b..f83fdeb5b27 100644 --- a/core/field_image.ts +++ b/core/field_image.ts @@ -218,7 +218,8 @@ export class FieldImage extends Field { * call the handler. */ protected override showEditor_( - _onEditorShown: () => void, _onEditorHidden: () => void + _onEditorShown: () => void, + _onEditorHidden: () => void, ) { // Note that an editor shouldn't be shown for this field, so the callbacks // are ignored. diff --git a/core/field_input.ts b/core/field_input.ts index d9851d3fb17..2d8be8c089a 100644 --- a/core/field_input.ts +++ b/core/field_input.ts @@ -27,6 +27,7 @@ import { FieldValidator, UnattachedFieldError, } from './field.js'; +import {getFocusManager} from './focus_manager.js'; import {Msg} from './msg.js'; import * as renderManagement from './render_management.js'; import * as aria from './utils/aria.js'; @@ -35,7 +36,6 @@ import {Size} from './utils/size.js'; import * as userAgent from './utils/useragent.js'; import * as WidgetDiv from './widgetdiv.js'; import type {WorkspaceSvg} from './workspace_svg.js'; -import { getFocusManager } from './focus_manager.js'; /** * Supported types for FieldInput subclasses. @@ -338,8 +338,10 @@ export abstract class FieldInput extends Field< * Defaults to false. */ protected override showEditor_( - onEditorShown: () => void, onEditorHidden: () => void, _e?: Event, - quietInput: boolean = false + onEditorShown: () => void, + onEditorHidden: () => void, + _e?: Event, + quietInput: boolean = false, ) { this.workspace_ = (this.sourceBlock_ as BlockSvg).workspace; if ( @@ -354,8 +356,9 @@ export abstract class FieldInput extends Field< } protected override onShowEditor(): void { - this.returnFocusCallback = - getFocusManager().takeEphemeralFocus(document.body); + this.returnFocusCallback = getFocusManager().takeEphemeralFocus( + document.body, + ); } protected override onHideEditor(): void { @@ -368,7 +371,8 @@ export abstract class FieldInput extends Field< * keyboards). */ private showPromptEditor( - onEditorShown: () => void, onEditorHidden: () => void + onEditorShown: () => void, + onEditorHidden: () => void, ) { onEditorShown(); dialog.prompt( @@ -391,7 +395,9 @@ export abstract class FieldInput extends Field< * @param quietInput True if editor should be created without focus. */ private showInlineEditor( - onEditorShown: () => void, onEditorHidden: () => void, quietInput: boolean + onEditorShown: () => void, + onEditorHidden: () => void, + quietInput: boolean, ) { const block = this.getSourceBlock(); if (!block) { diff --git a/core/field_label.ts b/core/field_label.ts index d108ac499fe..b0dd6af54d2 100644 --- a/core/field_label.ts +++ b/core/field_label.ts @@ -95,7 +95,8 @@ export class FieldLabel extends Field { } protected override showEditor_( - _onEditorShown: () => void, _onEditorHidden: () => void + _onEditorShown: () => void, + _onEditorHidden: () => void, ) { // Note that an editor shouldn't be shown for this field, so the callbacks // are ignored. diff --git a/tests/mocha/field_textinput_test.js b/tests/mocha/field_textinput_test.js index 4ab40d4f506..20f05432d82 100644 --- a/tests/mocha/field_textinput_test.js +++ b/tests/mocha/field_textinput_test.js @@ -228,7 +228,10 @@ suite('Text Input Fields', function () { this.assertSpellcheck = function (field, value) { this.prepField(field); - field.showEditor_(() => {}, () => {}); + field.showEditor_( + () => {}, + () => {}, + ); assert.equal( field.htmlInput_.getAttribute('spellcheck'), value.toString(), @@ -265,7 +268,10 @@ suite('Text Input Fields', function () { test('setSpellcheck Editor Shown', function () { const field = new Blockly.FieldTextInput('test'); this.prepField(field); - field.showEditor_(() => {}, () => {}); + field.showEditor_( + () => {}, + () => {}, + ); field.setSpellcheck(false); assert.equal(field.htmlInput_.getAttribute('spellcheck'), 'false'); }); diff --git a/tests/typescript/src/field/different_user_input.ts b/tests/typescript/src/field/different_user_input.ts index afc150da48d..60cb8e00aa6 100644 --- a/tests/typescript/src/field/different_user_input.ts +++ b/tests/typescript/src/field/different_user_input.ts @@ -63,7 +63,8 @@ class FieldMitosis extends Field { } protected override showEditor_( - _onEditorShown: () => void, _onEditorHidden: () => void + _onEditorShown: () => void, + _onEditorHidden: () => void, ) { // Note that an editor shouldn't be shown for this field, so the callbacks // are ignored. From 243064676c05d38a2adc5e4da401079ff22d0676 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 22 Apr 2025 21:36:05 +0000 Subject: [PATCH 07/43] chore: Remove incorrect aria-label. --- core/field.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/core/field.ts b/core/field.ts index 8b912d237bb..7d3a5e7bf68 100644 --- a/core/field.ts +++ b/core/field.ts @@ -310,7 +310,6 @@ export abstract class Field this.fieldGroup_ = dom.createSvgElement(Svg.G, { 'tabindex': '-1', 'id': id, - 'aria-label': 'Field ' + this.name, }); if (!this.isVisible()) { this.fieldGroup_.style.display = 'none'; From 41bc01a14e59a8c00ff1e2e627c79272fb395346 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 23 Apr 2025 21:14:06 +0000 Subject: [PATCH 08/43] feat: Make RenderedConnection focusable. Note that this doesn't yet contain all of the changes needed in order to ensure that this works correctly. --- core/connection.ts | 4 ++ core/rendered_connection.ts | 94 +++++++++++++++++++++++++++++++++++-- 2 files changed, 95 insertions(+), 3 deletions(-) diff --git a/core/connection.ts b/core/connection.ts index 039d8822c01..d28099d1d69 100644 --- a/core/connection.ts +++ b/core/connection.ts @@ -20,6 +20,7 @@ import type {Input} from './inputs/input.js'; import type {IASTNodeLocationWithBlock} from './interfaces/i_ast_node_location_with_block.js'; import type {IConnectionChecker} from './interfaces/i_connection_checker.js'; import * as blocks from './serialization/blocks.js'; +import { idGenerator } from './utils.js'; import * as Xml from './xml.js'; /** @@ -55,6 +56,8 @@ export class Connection implements IASTNodeLocationWithBlock { /** DOM representation of a shadow block, or null if none. */ private shadowDom: Element | null = null; + id: string; + /** * Horizontal location of this connection. * @@ -80,6 +83,7 @@ export class Connection implements IASTNodeLocationWithBlock { public type: number, ) { this.sourceBlock_ = source; + this.id = `${source.id}_connection_${idGenerator.getNextUniqueId()}`; } /** diff --git a/core/rendered_connection.ts b/core/rendered_connection.ts index 168e59744d2..de6b6df2bad 100644 --- a/core/rendered_connection.ts +++ b/core/rendered_connection.ts @@ -17,6 +17,8 @@ import * as common from './common.js'; import {config} from './config.js'; import {Connection} from './connection.js'; import type {ConnectionDB} from './connection_db.js'; +import type {IFocusableNode} from './interfaces/i_focusable_node.js'; +import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import {ConnectionType} from './connection_type.js'; import * as ContextMenu from './contextmenu.js'; import {ContextMenuRegistry} from './contextmenu_registry.js'; @@ -25,6 +27,11 @@ import {IContextMenu} from './interfaces/i_contextmenu.js'; import {hasBubble} from './interfaces/i_has_bubble.js'; import * as internalConstants from './internal_constants.js'; import {Coordinate} from './utils/coordinate.js'; +import {WorkspaceSvg} from './workspace_svg.js'; +import * as dom from './utils/dom.js'; +import {Svg} from './utils/svg.js'; +import * as svgPaths from './utils/svg_paths.js'; +import type {ConstantProvider, PuzzleTab} from './renderers/common/constants.js'; import * as svgMath from './utils/svg_math.js'; /** Maximum randomness in workspace units for bumping a block. */ @@ -33,7 +40,9 @@ const BUMP_RANDOMNESS = 10; /** * Class for a connection between blocks that may be rendered on screen. */ -export class RenderedConnection extends Connection implements IContextMenu { +export class RenderedConnection + extends Connection + implements IContextMenu, IFocusableNode { // TODO(b/109816955): remove '!', see go/strict-prop-init-fix. sourceBlock_!: BlockSvg; private readonly db: ConnectionDB; @@ -41,6 +50,9 @@ export class RenderedConnection extends Connection implements IContextMenu { private readonly offsetInBlock: Coordinate; private trackedState: TrackedState; private highlighted: boolean = false; + private constants: ConstantProvider; + private svgGroup: SVGElement; + private svgPath: SVGElement; /** Connection this connection connects to. Null if not connected. */ override targetConnection: RenderedConnection | null = null; @@ -70,6 +82,43 @@ export class RenderedConnection extends Connection implements IContextMenu { /** Describes the state of this connection's tracked-ness. */ this.trackedState = RenderedConnection.TrackedState.WILL_TRACK; + + this.constants = (source.workspace as WorkspaceSvg).getRenderer().getConstants(); + + this.svgGroup = dom.createSvgElement( + Svg.G, + { + 'class': 'blocklyCursor', + 'width': this.constants.CURSOR_WS_WIDTH, + 'height': this.constants.WS_CURSOR_HEIGHT, + } + ); + + this.svgPath = dom.createSvgElement( + Svg.PATH, + {'transform': ''}, + this.svgGroup, + ); + + // TODO: Ensure this auto-moves with the block. + const x = this.getOffsetInBlock().x; + const y = this.getOffsetInBlock().y; + + const path = + svgPaths.moveTo(0, 0) + + 'c 0,10 -8,-8 -8,7.5 s 8,-2.5 8,7.5'; + // TODO: It seems that constants isn't yet initialized at this point. + // (this.constants.shapeFor(this) as PuzzleTab).pathDown; + this.svgPath.setAttribute('d', path); + this.svgPath.setAttribute( + 'transform', + 'translate(' + + x + + ',' + + y + + ')' + + (this.sourceBlock_.workspace.RTL ? ' scale(-1 1)' : ''), + ); } /** @@ -320,13 +369,21 @@ export class RenderedConnection extends Connection implements IContextMenu { /** Add highlighting around this connection. */ highlight() { this.highlighted = true; - this.getSourceBlock().queueRender(); + const highlightSvg = this.findHighlightSvg(); + // if (highlightSvg) { + // highlightSvg.style.display = ''; + // } + // this.getSourceBlock().queueRender(); } /** Remove the highlighting around this connection. */ unhighlight() { this.highlighted = false; - this.getSourceBlock().queueRender(); + const highlightSvg = this.findHighlightSvg(); + // if (highlightSvg) { + // highlightSvg.style.display = 'none'; + // } + // this.getSourceBlock().queueRender(); } /** Returns true if this connection is highlighted, false otherwise. */ @@ -593,6 +650,12 @@ export class RenderedConnection extends Connection implements IContextMenu { return this; } + private findHighlightSvg(): SVGElement | null { + // This cast is valid as TypeScript's definition is wrong. See: + // https://github.com/microsoft/TypeScript/issues/60996. + return document.getElementById(this.id) as unknown | null as SVGElement | null; + } + /** * Handles showing the context menu when it is opened on a connection. * Note that typically the context menu can't be opened with the mouse @@ -626,6 +689,31 @@ export class RenderedConnection extends Connection implements IContextMenu { ContextMenu.show(e, menuOptions, block.RTL, workspace, location); } + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + const highlightSvg = this.findHighlightSvg(); + if (highlightSvg) { + highlightSvg.setAttribute('aria-label', 'Connection'); + return highlightSvg; + } + throw new Error('No highlight SVG found corresponding to this connection.'); + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this.getSourceBlock().workspace as WorkspaceSvg; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void { + this.highlight(); + } + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void { + this.unhighlight(); + } } export namespace RenderedConnection { From 263773610804e3212a922d7d2a515a104d6fc7d6 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 23 Apr 2025 21:43:32 +0000 Subject: [PATCH 09/43] fix: Ensure Block paths are focusable. --- core/renderers/common/path_object.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/renderers/common/path_object.ts b/core/renderers/common/path_object.ts index 077f80bb741..72cf2a594ce 100644 --- a/core/renderers/common/path_object.ts +++ b/core/renderers/common/path_object.ts @@ -62,7 +62,7 @@ export class PathObject implements IPathObject { /** The primary path of the block. */ this.svgPath = dom.createSvgElement( Svg.PATH, - {'class': 'blocklyPath'}, + {'class': 'blocklyPath', 'tabindex': '-1'}, this.svgRoot, ); From 49192ba4823c8ac466a136e6319a8b4364dc26cc Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 23 Apr 2025 22:25:53 +0000 Subject: [PATCH 10/43] chore: Fix line comment. Addresses review comment. --- core/workspace_svg.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index 6d6505d0545..b1c96373771 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -771,7 +771,7 @@ export class WorkspaceSvg */ this.svgGroup_ = dom.createSvgElement(Svg.G, { 'class': 'blocklyWorkspace', - // Only the main workspace should be tabbable. + // Only the top-level workspace should be tabbable. 'tabindex': injectionDiv ? '0' : '-1', 'id': this.id, }); From d276dbcc2edfc39ea1fbc03f700fc96a2583bc2d Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 24 Apr 2025 19:08:22 +0000 Subject: [PATCH 11/43] chore: reduce branching. Addresses reviewer comment. --- core/toolbox/toolbox.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/toolbox/toolbox.ts b/core/toolbox/toolbox.ts index f44ebac5348..03442e13795 100644 --- a/core/toolbox/toolbox.ts +++ b/core/toolbox/toolbox.ts @@ -1120,7 +1120,8 @@ export class Toolbox // root of the toolbox. if (!previousNode || previousNode === this) { return this.getToolboxItems().find((item) => item.isSelectable()) ?? null; - } else return null; + } + return null; } /** See IFocusableTree.getNestedTrees. */ From 90fdde21fdd51e88a90b5ed83fd40ad2bcc0f0fd Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 24 Apr 2025 19:16:23 +0000 Subject: [PATCH 12/43] feat: make drop down & widget divs focusable. This is a more general purpose alternative to making fields explicitly focusable (at least making them hook up properly to ephemeral focus). --- core/dropdowndiv.ts | 14 ++++++++++++++ core/widgetdiv.ts | 15 +++++++++++++-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/core/dropdowndiv.ts b/core/dropdowndiv.ts index 0d259bc53d7..6d61354adf7 100644 --- a/core/dropdowndiv.ts +++ b/core/dropdowndiv.ts @@ -15,6 +15,7 @@ import type {BlockSvg} from './block_svg.js'; import * as common from './common.js'; import type {Field} from './field.js'; +import { ReturnEphemeralFocus, getFocusManager } from './focus_manager.js'; import * as dom from './utils/dom.js'; import * as math from './utils/math.js'; import {Rect} from './utils/rect.js'; @@ -82,6 +83,9 @@ let owner: Field | null = null; /** Whether the dropdown was positioned to a field or the source block. */ let positionToField: boolean | null = null; +/** Callback to FocusManager to return ephemeral focus when the div closes. */ +let returnEphemeralFocus: ReturnEphemeralFocus | null = null; + /** * Dropdown bounds info object used to encapsulate sizing information about a * bounding element (bounding box and width/height). @@ -338,6 +342,8 @@ export function show( dom.addClass(div, renderedClassName); dom.addClass(div, themeClassName); + returnEphemeralFocus = getFocusManager().takeEphemeralFocus(div); + // When we change `translate` multiple times in close succession, // Chrome may choose to wait and apply them all at once. // Since we want the translation to initial X, Y to be immediate, @@ -623,6 +629,10 @@ export function hide() { animateOutTimer = setTimeout(function () { hideWithoutAnimation(); }, ANIMATION_TIME * 1000); + if (returnEphemeralFocus) { + returnEphemeralFocus(); + returnEphemeralFocus = null; + } if (onHide) { onHide(); onHide = null; @@ -638,6 +648,10 @@ export function hideWithoutAnimation() { clearTimeout(animateOutTimer); } + if (returnEphemeralFocus) { + returnEphemeralFocus(); + returnEphemeralFocus = null; + } if (onHide) { onHide(); onHide = null; diff --git a/core/widgetdiv.ts b/core/widgetdiv.ts index f167b6cf04d..3d2e2e9d9a7 100644 --- a/core/widgetdiv.ts +++ b/core/widgetdiv.ts @@ -8,6 +8,7 @@ import * as common from './common.js'; import {Field} from './field.js'; +import { ReturnEphemeralFocus, getFocusManager } from './focus_manager.js'; import * as dom from './utils/dom.js'; import type {Rect} from './utils/rect.js'; import type {Size} from './utils/size.js'; @@ -34,6 +35,9 @@ let themeClassName = ''; /** The HTML container for popup overlays (e.g. editor widgets). */ let containerDiv: HTMLDivElement | null; +/** Callback to FocusManager to return ephemeral focus when the div closes. */ +let returnEphemeralFocus: ReturnEphemeralFocus | null = null; + /** * Returns the HTML container for editor widgets. * @@ -110,6 +114,7 @@ export function show( if (themeClassName) { dom.addClass(div, themeClassName); } + returnEphemeralFocus = getFocusManager().takeEphemeralFocus(div); } /** @@ -126,8 +131,14 @@ export function hide() { div.style.display = 'none'; div.style.left = ''; div.style.top = ''; - if (dispose) dispose(); - dispose = null; + if (returnEphemeralFocus) { + returnEphemeralFocus(); + returnEphemeralFocus = null; + } + if (dispose) { + dispose(); + dispose = null; + } div.textContent = ''; if (rendererClassName) { From 7c2f705be956d4c7fc34206e4fa2e5021de5c671 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 24 Apr 2025 19:31:13 +0000 Subject: [PATCH 13/43] chore: undo breaking field changes. With widget and drop down divs focusable directly, all field editors should automatically inherit this benefit. --- blocks/procedures.ts | 7 +-- core/dialog.ts | 7 +++ core/field.ts | 29 ++---------- core/field_checkbox.ts | 9 +--- core/field_dropdown.ts | 26 +---------- core/field_image.ts | 11 +---- core/field_input.ts | 44 +++---------------- core/field_label.ts | 12 ----- tests/mocha/field_textinput_test.js | 10 +---- .../src/field/different_user_input.ts | 12 ----- 10 files changed, 24 insertions(+), 143 deletions(-) diff --git a/blocks/procedures.ts b/blocks/procedures.ts index 656794002b9..7a0899eacb0 100644 --- a/blocks/procedures.ts +++ b/blocks/procedures.ts @@ -648,12 +648,7 @@ class ProcedureArgumentField extends FieldTextInput { * * @param e The event that triggered display of the field editor. */ - protected override showEditor_( - onEditorShown: () => void, - onEditorHidden: () => void, - e?: Event, - ) { - super.showEditor_(onEditorShown, onEditorHidden, e); + protected override showEditor_(e?: Event) { this.editingInteractively = true; this.editingVariable = undefined; } diff --git a/core/dialog.ts b/core/dialog.ts index 374961323da..96631e9cbc7 100644 --- a/core/dialog.ts +++ b/core/dialog.ts @@ -33,6 +33,8 @@ const defaultPrompt = function ( defaultValue: string, callback: (result: string | null) => void, ) { + // NOTE TO DEVELOPER: Ephemeral focus doesn't need to be taken for the native + // window prompt since it prevents focus from changing while open. callback(window.prompt(message, defaultValue)); }; @@ -116,6 +118,11 @@ export function prompt( /** * Sets the function to be run when Blockly.dialog.prompt() is called. * + * **Important**: When overridding this, be aware that non-native prompt + * experiences may require managing ephemeral focus in FocusManager. This isn't + * needed for the native window prompt because it prevents focus from being + * changed while open. + * * @param promptFunction The function to be run, or undefined to restore the * default implementation. * @see Blockly.dialog.prompt diff --git a/core/field.ts b/core/field.ts index 7d3a5e7bf68..df166fbe778 100644 --- a/core/field.ts +++ b/core/field.ts @@ -782,11 +782,7 @@ export abstract class Field */ showEditor(e?: Event) { if (this.isClickable()) { - this.showEditor_( - () => this.onShowEditor(), - () => this.onHideEditor(), - e, - ); + this.showEditor_(e); } } @@ -794,28 +790,11 @@ export abstract class Field * A developer hook to create an editor for the field. This is no-op by * default, and must be overriden to create an editor. * - * @param onEditorShown Callback that must be called when the editor is shown. - * @param onEditorHidden Callback that must be called when the editor hides. - * @param e Optional mouse event that triggered the field to open, or + * @param _e Optional mouse event that triggered the field to open, or * undefined if triggered programmatically. */ - protected abstract showEditor_( - onEditorShown: () => void, - onEditorHidden: () => void, - e?: Event, - ): void; - - /** - * Called when an editor is shown. This is expected to be used for ensuring - * that the editor has proper focus. - */ - protected abstract onShowEditor(): void; - - /** - * Called when an editor is hidden. This is expected to be used for ensuring - * that the editor no longer proper focus. - */ - protected abstract onHideEditor(): void; + protected showEditor_(_e?: Event): void {} + // NOP /** * A developer hook to reposition the WidgetDiv during a window resize. You diff --git a/core/field_checkbox.ts b/core/field_checkbox.ts index 1feb65a85c3..c8307d1328a 100644 --- a/core/field_checkbox.ts +++ b/core/field_checkbox.ts @@ -136,19 +136,12 @@ export class FieldCheckbox extends Field { } /** Toggle the state of the checkbox on click. */ - protected override showEditor_( - _onEditorShown: () => void, - _onEditorHidden: () => void, - ) { + protected override showEditor_() { // There's no explicit editor for checkboxes, so let DOM focus be handled // normally. this.setValue(!this.value_); } - protected override onShowEditor(): void {} - - protected override onHideEditor(): void {} - /** * Ensure that the input value is valid ('TRUE' or 'FALSE'). * diff --git a/core/field_dropdown.ts b/core/field_dropdown.ts index 616c00ce689..ab9e4e060d6 100644 --- a/core/field_dropdown.ts +++ b/core/field_dropdown.ts @@ -89,8 +89,6 @@ export class FieldDropdown extends Field { private selectedOption!: MenuOption; override clickTarget_: SVGElement | null = null; - private returnFocusCallback: (() => void) | null = null; - /** * The y offset from the top of the field to the top of the image, if an image * is selected. @@ -272,11 +270,7 @@ export class FieldDropdown extends Field { * @param e Optional mouse event that triggered the field to open, or * undefined if triggered programmatically. */ - protected override showEditor_( - onEditorShown: () => void, - onEditorHidden: () => void, - e?: MouseEvent, - ) { + protected override showEditor_(e?: MouseEvent) { const block = this.getSourceBlock(); if (!block) { throw new UnattachedFieldError(); @@ -284,7 +278,6 @@ export class FieldDropdown extends Field { this.dropdownCreate(); if (!this.menu_) return; - onEditorShown(); if (e && typeof e.clientX === 'number') { this.menu_.openingCoords = new Coordinate(e.clientX, e.clientY); } else { @@ -303,10 +296,7 @@ export class FieldDropdown extends Field { dropDownDiv.setColour(primaryColour, borderColour); } - dropDownDiv.showPositionedByField(this, () => { - this.dropdownDispose_.bind(this); - onEditorHidden(); - }); + dropDownDiv.showPositionedByField(this, this.dropdownDispose_.bind(this)); dropDownDiv.getContentDiv().style.height = `${this.menu_.getSize().height}px`; @@ -322,18 +312,6 @@ export class FieldDropdown extends Field { this.applyColour(); } - protected override onShowEditor(): void { - const menuElement = this.menu_?.getElement(); - if (menuElement) { - this.returnFocusCallback = - getFocusManager().takeEphemeralFocus(menuElement); - } - } - - protected override onHideEditor(): void { - if (this.returnFocusCallback) this.returnFocusCallback(); - } - /** Create the dropdown editor. */ private dropdownCreate() { const block = this.getSourceBlock(); diff --git a/core/field_image.ts b/core/field_image.ts index f83fdeb5b27..650575f59a3 100644 --- a/core/field_image.ts +++ b/core/field_image.ts @@ -217,21 +217,12 @@ export class FieldImage extends Field { * If field click is called, and click handler defined, * call the handler. */ - protected override showEditor_( - _onEditorShown: () => void, - _onEditorHidden: () => void, - ) { - // Note that an editor shouldn't be shown for this field, so the callbacks - // are ignored. + protected override showEditor_() { if (this.clickHandler) { this.clickHandler(this); } } - protected override onShowEditor(): void {} - - protected override onHideEditor(): void {} - /** * Set the function that is called when this image is clicked. * diff --git a/core/field_input.ts b/core/field_input.ts index 2d8be8c089a..7bf19e217ce 100644 --- a/core/field_input.ts +++ b/core/field_input.ts @@ -101,8 +101,6 @@ export abstract class FieldInput extends Field< */ override SERIALIZABLE = true; - private returnFocusCallback: (() => void) | null = null; - /** * @param value The initial value of the field. Should cast to a string. * Defaults to an empty string if null or undefined. Also accepts @@ -330,51 +328,30 @@ export abstract class FieldInput extends Field< * Shows a prompt editor for mobile browsers if the modalInputs option is * enabled. * - * @param onEditorShown Callback that must be called when the editor is shown. - * @param onEditorHidden Callback that must be called when the editor hides. * @param _e Optional mouse event that triggered the field to open, or * undefined if triggered programmatically. * @param quietInput True if editor should be created without focus. * Defaults to false. */ - protected override showEditor_( - onEditorShown: () => void, - onEditorHidden: () => void, - _e?: Event, - quietInput: boolean = false, - ) { + protected override showEditor_(_e?: Event, quietInput: boolean = false) { this.workspace_ = (this.sourceBlock_ as BlockSvg).workspace; if ( !quietInput && this.workspace_.options.modalInputs && (userAgent.MOBILE || userAgent.ANDROID || userAgent.IPAD) ) { - this.showPromptEditor(onEditorShown, onEditorHidden); + this.showPromptEditor(); } else { - this.showInlineEditor(onEditorShown, onEditorHidden, quietInput); + this.showInlineEditor(quietInput); } } - protected override onShowEditor(): void { - this.returnFocusCallback = getFocusManager().takeEphemeralFocus( - document.body, - ); - } - - protected override onHideEditor(): void { - if (this.returnFocusCallback) this.returnFocusCallback(); - } - /** * Create and show a text input editor that is a prompt (usually a popup). * Mobile browsers may have issues with in-line textareas (focus and * keyboards). */ - private showPromptEditor( - onEditorShown: () => void, - onEditorHidden: () => void, - ) { - onEditorShown(); + private showPromptEditor() { dialog.prompt( Msg['CHANGE_VALUE_TITLE'], this.getText(), @@ -384,7 +361,6 @@ export abstract class FieldInput extends Field< this.setValue(this.getValueFromEditorText_(text)); } this.onFinishEditing_(this.value_); - onEditorHidden(); }, ); } @@ -394,11 +370,7 @@ export abstract class FieldInput extends Field< * * @param quietInput True if editor should be created without focus. */ - private showInlineEditor( - onEditorShown: () => void, - onEditorHidden: () => void, - quietInput: boolean, - ) { + private showInlineEditor(quietInput: boolean) { const block = this.getSourceBlock(); if (!block) { throw new UnattachedFieldError(); @@ -406,13 +378,9 @@ export abstract class FieldInput extends Field< WidgetDiv.show( this, block.RTL, - () => { - this.widgetDispose_(); - onEditorHidden(); - }, + this.widgetDispose_.bind(this), this.workspace_, ); - onEditorShown(); this.htmlInput_ = this.widgetCreate_() as HTMLInputElement; this.isBeingEdited_ = true; this.valueWhenEditorWasOpened_ = this.value_; diff --git a/core/field_label.ts b/core/field_label.ts index b0dd6af54d2..236154cc7b1 100644 --- a/core/field_label.ts +++ b/core/field_label.ts @@ -94,18 +94,6 @@ export class FieldLabel extends Field { return `${newValue}`; } - protected override showEditor_( - _onEditorShown: () => void, - _onEditorHidden: () => void, - ) { - // Note that an editor shouldn't be shown for this field, so the callbacks - // are ignored. - } - - protected override onShowEditor(): void {} - - protected override onHideEditor(): void {} - /** * Set the CSS class applied to the field's textElement_. * diff --git a/tests/mocha/field_textinput_test.js b/tests/mocha/field_textinput_test.js index 20f05432d82..93b8df89fd0 100644 --- a/tests/mocha/field_textinput_test.js +++ b/tests/mocha/field_textinput_test.js @@ -228,10 +228,7 @@ suite('Text Input Fields', function () { this.assertSpellcheck = function (field, value) { this.prepField(field); - field.showEditor_( - () => {}, - () => {}, - ); + field.showEditor_(); assert.equal( field.htmlInput_.getAttribute('spellcheck'), value.toString(), @@ -268,10 +265,7 @@ suite('Text Input Fields', function () { test('setSpellcheck Editor Shown', function () { const field = new Blockly.FieldTextInput('test'); this.prepField(field); - field.showEditor_( - () => {}, - () => {}, - ); + field.showEditor_(; field.setSpellcheck(false); assert.equal(field.htmlInput_.getAttribute('spellcheck'), 'false'); }); diff --git a/tests/typescript/src/field/different_user_input.ts b/tests/typescript/src/field/different_user_input.ts index 60cb8e00aa6..f91f25bbc86 100644 --- a/tests/typescript/src/field/different_user_input.ts +++ b/tests/typescript/src/field/different_user_input.ts @@ -61,18 +61,6 @@ class FieldMitosis extends Field { }); this.value_ = {cells}; } - - protected override showEditor_( - _onEditorShown: () => void, - _onEditorHidden: () => void, - ) { - // Note that an editor shouldn't be shown for this field, so the callbacks - // are ignored. - } - - protected override onShowEditor(): void {} - - protected override onHideEditor(): void {} } fieldRegistry.register('field_mitosis', FieldMitosis); From 97263894d988d645204b5f470123292f1156d6c0 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 24 Apr 2025 19:36:22 +0000 Subject: [PATCH 14/43] chore: some more clean-ups after removals. --- blocks/procedures.ts | 1 + core/field_checkbox.ts | 2 -- core/field_dropdown.ts | 5 ++--- core/field_input.ts | 3 +-- tests/mocha/field_textinput_test.js | 2 +- 5 files changed, 5 insertions(+), 8 deletions(-) diff --git a/blocks/procedures.ts b/blocks/procedures.ts index 7a0899eacb0..7284973d9cc 100644 --- a/blocks/procedures.ts +++ b/blocks/procedures.ts @@ -649,6 +649,7 @@ class ProcedureArgumentField extends FieldTextInput { * @param e The event that triggered display of the field editor. */ protected override showEditor_(e?: Event) { + super.showEditor_(e); this.editingInteractively = true; this.editingVariable = undefined; } diff --git a/core/field_checkbox.ts b/core/field_checkbox.ts index c8307d1328a..55ed42cbf4b 100644 --- a/core/field_checkbox.ts +++ b/core/field_checkbox.ts @@ -137,8 +137,6 @@ export class FieldCheckbox extends Field { /** Toggle the state of the checkbox on click. */ protected override showEditor_() { - // There's no explicit editor for checkboxes, so let DOM focus be handled - // normally. this.setValue(!this.value_); } diff --git a/core/field_dropdown.ts b/core/field_dropdown.ts index ab9e4e060d6..81279e2a1f5 100644 --- a/core/field_dropdown.ts +++ b/core/field_dropdown.ts @@ -22,7 +22,6 @@ import { UnattachedFieldError, } from './field.js'; import * as fieldRegistry from './field_registry.js'; -import {getFocusManager} from './focus_manager.js'; import {Menu} from './menu.js'; import {MenuSeparator} from './menu_separator.js'; import {MenuItem} from './menuitem.js'; @@ -770,7 +769,7 @@ export class FieldDropdown extends Field { } else if (typeof option[1] !== 'string') { foundError = true; console.error( - `Invalid option[${i}]: Each FieldDropdown option id must be a string. + `Invalid option[${i}]: Each FieldDropdown option id must be a string. Found ${option[1]} in: ${option}`, ); } else if ( @@ -781,7 +780,7 @@ export class FieldDropdown extends Field { ) { foundError = true; console.error( - `Invalid option[${i}]: Each FieldDropdown option must have a string + `Invalid option[${i}]: Each FieldDropdown option must have a string label, image description, or HTML element. Found ${option[0]} in: ${option}`, ); } diff --git a/core/field_input.ts b/core/field_input.ts index 7bf19e217ce..2cdd8056553 100644 --- a/core/field_input.ts +++ b/core/field_input.ts @@ -27,7 +27,6 @@ import { FieldValidator, UnattachedFieldError, } from './field.js'; -import {getFocusManager} from './focus_manager.js'; import {Msg} from './msg.js'; import * as renderManagement from './render_management.js'; import * as aria from './utils/aria.js'; @@ -333,7 +332,7 @@ export abstract class FieldInput extends Field< * @param quietInput True if editor should be created without focus. * Defaults to false. */ - protected override showEditor_(_e?: Event, quietInput: boolean = false) { + protected override showEditor_(_e?: Event, quietInput = false) { this.workspace_ = (this.sourceBlock_ as BlockSvg).workspace; if ( !quietInput && diff --git a/tests/mocha/field_textinput_test.js b/tests/mocha/field_textinput_test.js index 93b8df89fd0..82c1a645e6d 100644 --- a/tests/mocha/field_textinput_test.js +++ b/tests/mocha/field_textinput_test.js @@ -265,7 +265,7 @@ suite('Text Input Fields', function () { test('setSpellcheck Editor Shown', function () { const field = new Blockly.FieldTextInput('test'); this.prepField(field); - field.showEditor_(; + field.showEditor_(); field.setSpellcheck(false); assert.equal(field.htmlInput_.getAttribute('spellcheck'), 'false'); }); From 1094787a802b7d69cb8965121a875b1cbeedd804 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 24 Apr 2025 20:27:29 +0000 Subject: [PATCH 15/43] feat: fix field node retrieval. Also, remove unnecessary casting. --- core/field.ts | 3 +-- core/workspace_svg.ts | 11 +++++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/core/field.ts b/core/field.ts index df166fbe778..1da60f18b81 100644 --- a/core/field.ts +++ b/core/field.ts @@ -1427,8 +1427,7 @@ export abstract class Field if (!block) { throw new UnattachedFieldError(); } - // TODO: Remove the latter two casts once WorkspaceSvg is a focusable tree. - return block.workspace as WorkspaceSvg as unknown as IFocusableTree; + return block.workspace as WorkspaceSvg; } /** See IFocusableNode.onNodeFocus. */ diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index 250d6cf43e2..b5a5b245a0a 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -2680,6 +2680,17 @@ export class WorkspaceSvg /** See IFocusableTree.lookUpFocusableNode. */ lookUpFocusableNode(id: string): IFocusableNode | null { + const fieldIndicatorIndex = id.indexOf('_field_'); + if (fieldIndicatorIndex !== -1) { + const blockId = id.substring(0, fieldIndicatorIndex); + const block = this.getBlockById(blockId); + if (block) { + for (const field of block.getFields()) { + if (field.getFocusableElement().id === id) return field; + } + } + return null; + } return this.getBlockById(id) as IFocusableNode; } From 082a6efd4b0a02cce8972fea99cfcc0045a34628 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 24 Apr 2025 20:29:57 +0000 Subject: [PATCH 16/43] chore: lint fixes. --- core/dropdowndiv.ts | 2 +- core/widgetdiv.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/core/dropdowndiv.ts b/core/dropdowndiv.ts index 6d61354adf7..dcf8fa24ef7 100644 --- a/core/dropdowndiv.ts +++ b/core/dropdowndiv.ts @@ -15,7 +15,7 @@ import type {BlockSvg} from './block_svg.js'; import * as common from './common.js'; import type {Field} from './field.js'; -import { ReturnEphemeralFocus, getFocusManager } from './focus_manager.js'; +import {ReturnEphemeralFocus, getFocusManager} from './focus_manager.js'; import * as dom from './utils/dom.js'; import * as math from './utils/math.js'; import {Rect} from './utils/rect.js'; diff --git a/core/widgetdiv.ts b/core/widgetdiv.ts index 3d2e2e9d9a7..cb006160455 100644 --- a/core/widgetdiv.ts +++ b/core/widgetdiv.ts @@ -8,7 +8,7 @@ import * as common from './common.js'; import {Field} from './field.js'; -import { ReturnEphemeralFocus, getFocusManager } from './focus_manager.js'; +import {ReturnEphemeralFocus, getFocusManager} from './focus_manager.js'; import * as dom from './utils/dom.js'; import type {Rect} from './utils/rect.js'; import type {Size} from './utils/size.js'; From 7f14372f67336568e89ec147d05c5f36c1f7964d Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 24 Apr 2025 20:40:38 +0000 Subject: [PATCH 17/43] feat: add remaining connection support There's a lot of clean-up and simplification work needed yet. --- core/css.ts | 8 +++---- core/renderers/common/constants.ts | 6 +++--- core/renderers/common/drawer.ts | 20 ++++++++++------- core/renderers/common/i_path_object.ts | 2 +- core/renderers/common/path_object.ts | 30 ++++++++++++++++++-------- core/renderers/zelos/constants.ts | 6 +++--- core/renderers/zelos/drawer.ts | 7 +++--- core/renderers/zelos/path_object.ts | 30 +++++++++++++------------- core/workspace_svg.ts | 10 +++++++++ 9 files changed, 72 insertions(+), 47 deletions(-) diff --git a/core/css.ts b/core/css.ts index 6ca262f3b25..ebdc613c050 100644 --- a/core/css.ts +++ b/core/css.ts @@ -147,8 +147,8 @@ let content = ` .blocklyHighlightedConnectionPath { fill: none; - stroke: #fc3; - stroke-width: 4px; + // stroke: #fc3; + // stroke-width: 4px; } .blocklyPathLight { @@ -463,8 +463,8 @@ input[type=number] { } .blocklyMenuSeparator { - background-color: #ccc; - height: 1px; + background-color: #ccc; + height: 1px; border: 0; margin-left: 4px; margin-right: 4px; diff --git a/core/renderers/common/constants.ts b/core/renderers/common/constants.ts index c5a7a759c5c..c8f770cc407 100644 --- a/core/renderers/common/constants.ts +++ b/core/renderers/common/constants.ts @@ -1203,9 +1203,9 @@ export class ConstantProvider { `}`, // Connection highlight. - `${selector} .blocklyHighlightedConnectionPath {`, - `stroke: #fc3;`, - `}`, + // `${selector} .blocklyHighlightedConnectionPath {`, + // `stroke: #fc3;`, + // `}`, // Replaceable highlight. `${selector} .blocklyReplaceable .blocklyPath {`, diff --git a/core/renderers/common/drawer.ts b/core/renderers/common/drawer.ts index 09320710c51..f805451a725 100644 --- a/core/renderers/common/drawer.ts +++ b/core/renderers/common/drawer.ts @@ -435,19 +435,23 @@ export class Drawer { for (const elem of row.elements) { if (!(elem instanceof Connection)) continue; - if (elem.highlighted) { - this.drawConnectionHighlightPath(elem); - } else { - this.block_.pathObject.removeConnectionHighlight?.( - elem.connectionModel, - ); + const highlightSvg = this.drawConnectionHighlightPath(elem); + if (highlightSvg) { + // highlightSvg.style.display = elem.highlighted ? '' : 'none'; } + // if (elem.highlighted) { + // this.drawConnectionHighlightPath(elem); + // } else { + // this.block_.pathObject.removeConnectionHighlight?.( + // elem.connectionModel, + // ); + // } } } } /** Returns a path to highlight the given connection. */ - drawConnectionHighlightPath(measurable: Connection) { + drawConnectionHighlightPath(measurable: Connection): SVGElement | undefined { const conn = measurable.connectionModel; let path = ''; if ( @@ -459,7 +463,7 @@ export class Drawer { path = this.getStatementConnectionHighlightPath(measurable); } const block = conn.getSourceBlock(); - block.pathObject.addConnectionHighlight?.( + return block.pathObject.addConnectionHighlight?.( conn, path, conn.getOffsetInBlock(), diff --git a/core/renderers/common/i_path_object.ts b/core/renderers/common/i_path_object.ts index 699f1d92edb..776ba0067ea 100644 --- a/core/renderers/common/i_path_object.ts +++ b/core/renderers/common/i_path_object.ts @@ -113,7 +113,7 @@ export interface IPathObject { connectionPath: string, offset: Coordinate, rtl: boolean, - ): void; + ): SVGElement; /** * Apply the stored colours to the block's path, taking into account whether diff --git a/core/renderers/common/path_object.ts b/core/renderers/common/path_object.ts index 72cf2a594ce..d216db1a221 100644 --- a/core/renderers/common/path_object.ts +++ b/core/renderers/common/path_object.ts @@ -153,13 +153,17 @@ export class PathObject implements IPathObject { * removed. */ protected setClass_(className: string, add: boolean) { + this.setClassOnElem_(this.svgRoot, className, add); + } + + private setClassOnElem_(root: SVGElement, className: string, add: boolean) { if (!className) { return; } if (add) { - dom.addClass(this.svgRoot, className); + dom.addClass(root, className); } else { - dom.removeClass(this.svgRoot, className); + dom.removeClass(root, className); } } @@ -209,7 +213,7 @@ export class PathObject implements IPathObject { * @param enable True if selection is enabled, false otherwise. */ updateSelected(enable: boolean) { - this.setClass_('blocklySelected', enable); + this.setClassOnElem_(this.svgPath, 'blocklySelected', enable); } /** @@ -268,25 +272,33 @@ export class PathObject implements IPathObject { connectionPath: string, offset: Coordinate, rtl: boolean, - ) { - if (this.connectionHighlights.has(connection)) { - if (this.currentHighlightMatchesNew(connection, connectionPath, offset)) { - return; - } - this.removeConnectionHighlight(connection); + ): SVGElement { + const previousHighlight = this.connectionHighlights.get(connection); + if (previousHighlight) { + // TODO: Fix the highlight seemingly being recreated every time it's focused. + // if (this.currentHighlightMatchesNew(connection, connectionPath, offset)) { + return previousHighlight; + // } + // this.removeConnectionHighlight(connection); } const highlight = dom.createSvgElement( Svg.PATH, { + 'id': connection.id, 'class': 'blocklyHighlightedConnectionPath', + // 'style': 'display: none;', + 'tabindex': '-1', 'd': connectionPath, 'transform': `translate(${offset.x}, ${offset.y})` + (rtl ? ' scale(-1 1)' : ''), }, this.svgRoot, ); + // TODO: Do this in a cleaner way. One possibility: create the path without 'd' or 'transform' in RenderedConnection, then just update it here (and keep registrations). + (highlight as any).renderedConnection = connection; this.connectionHighlights.set(connection, highlight); + return highlight; } private currentHighlightMatchesNew( diff --git a/core/renderers/zelos/constants.ts b/core/renderers/zelos/constants.ts index 8cd36e02589..d7be09b93de 100644 --- a/core/renderers/zelos/constants.ts +++ b/core/renderers/zelos/constants.ts @@ -885,9 +885,9 @@ export class ConstantProvider extends BaseConstantProvider { `}`, // Connection highlight. - `${selector} .blocklyHighlightedConnectionPath {`, - `stroke: ${this.SELECTED_GLOW_COLOUR};`, - `}`, + // `${selector} .blocklyHighlightedConnectionPath {`, + // `stroke: ${this.SELECTED_GLOW_COLOUR};`, + // `}`, // Disabled outline paths. `${selector} .blocklyDisabledPattern > .blocklyOutlinePath {`, diff --git a/core/renderers/zelos/drawer.ts b/core/renderers/zelos/drawer.ts index 5cc52c0cbb2..9e08fd5d193 100644 --- a/core/renderers/zelos/drawer.ts +++ b/core/renderers/zelos/drawer.ts @@ -234,15 +234,14 @@ export class Drawer extends BaseDrawer { } /** Returns a path to highlight the given connection. */ - drawConnectionHighlightPath(measurable: Connection) { + override drawConnectionHighlightPath(measurable: Connection): SVGElement | undefined { const conn = measurable.connectionModel; if ( conn.type === ConnectionType.NEXT_STATEMENT || conn.type === ConnectionType.PREVIOUS_STATEMENT || (conn.type === ConnectionType.OUTPUT_VALUE && !measurable.isDynamicShape) ) { - super.drawConnectionHighlightPath(measurable); - return; + return super.drawConnectionHighlightPath(measurable); } let path = ''; @@ -261,7 +260,7 @@ export class Drawer extends BaseDrawer { (output.shape as DynamicShape).pathDown(output.height); } const block = conn.getSourceBlock(); - block.pathObject.addConnectionHighlight?.( + return block.pathObject.addConnectionHighlight?.( conn, path, conn.getOffsetInBlock(), diff --git a/core/renderers/zelos/path_object.ts b/core/renderers/zelos/path_object.ts index f40426483a7..091c8719f71 100644 --- a/core/renderers/zelos/path_object.ts +++ b/core/renderers/zelos/path_object.ts @@ -85,21 +85,21 @@ export class PathObject extends BasePathObject { } } - override updateSelected(enable: boolean) { - this.setClass_('blocklySelected', enable); - if (enable) { - if (!this.svgPathSelected) { - this.svgPathSelected = this.svgPath.cloneNode(true) as SVGElement; - this.svgPathSelected.classList.add('blocklyPathSelected'); - this.svgRoot.appendChild(this.svgPathSelected); - } - } else { - if (this.svgPathSelected) { - this.svgRoot.removeChild(this.svgPathSelected); - this.svgPathSelected = null; - } - } - } + // override updateSelected(enable: boolean) { + // this.setClass_('blocklySelected', enable); + // if (enable) { + // if (!this.svgPathSelected) { + // this.svgPathSelected = this.svgPath.cloneNode(true) as SVGElement; + // this.svgPathSelected.classList.add('blocklyPathSelected'); + // this.svgRoot.appendChild(this.svgPathSelected); + // } + // } else { + // if (this.svgPathSelected) { + // this.svgRoot.removeChild(this.svgPathSelected); + // this.svgPathSelected = null; + // } + // } + // } override updateReplacementFade(enable: boolean) { this.setClass_('blocklyReplaceable', enable); diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index b5a5b245a0a..983f62b8597 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -2681,6 +2681,7 @@ export class WorkspaceSvg /** See IFocusableTree.lookUpFocusableNode. */ lookUpFocusableNode(id: string): IFocusableNode | null { const fieldIndicatorIndex = id.indexOf('_field_'); + const connectionIndicatorIndex = id.indexOf('_connection_'); if (fieldIndicatorIndex !== -1) { const blockId = id.substring(0, fieldIndicatorIndex); const block = this.getBlockById(blockId); @@ -2690,6 +2691,15 @@ export class WorkspaceSvg } } return null; + } else if (connectionIndicatorIndex !== -1) { + const blockId = id.substring(0, connectionIndicatorIndex); + const block = this.getBlockById(blockId); + if (block) { + for (const connection of block.getConnections_(true)) { + if (connection.id === id) return connection; + } + } + return null; } return this.getBlockById(id) as IFocusableNode; } From 59198db06a4520afd8fdaece1d23924a6d51b8d9 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 24 Apr 2025 20:47:04 +0000 Subject: [PATCH 18/43] chore: lint fixes. --- core/connection.ts | 2 +- core/rendered_connection.ts | 47 ++++++++++++++-------------- core/renderers/common/path_object.ts | 2 +- core/renderers/zelos/drawer.ts | 4 ++- 4 files changed, 29 insertions(+), 26 deletions(-) diff --git a/core/connection.ts b/core/connection.ts index d28099d1d69..5730fff996d 100644 --- a/core/connection.ts +++ b/core/connection.ts @@ -20,7 +20,7 @@ import type {Input} from './inputs/input.js'; import type {IASTNodeLocationWithBlock} from './interfaces/i_ast_node_location_with_block.js'; import type {IConnectionChecker} from './interfaces/i_connection_checker.js'; import * as blocks from './serialization/blocks.js'; -import { idGenerator } from './utils.js'; +import {idGenerator} from './utils.js'; import * as Xml from './xml.js'; /** diff --git a/core/rendered_connection.ts b/core/rendered_connection.ts index de6b6df2bad..5355033ea12 100644 --- a/core/rendered_connection.ts +++ b/core/rendered_connection.ts @@ -17,22 +17,22 @@ import * as common from './common.js'; import {config} from './config.js'; import {Connection} from './connection.js'; import type {ConnectionDB} from './connection_db.js'; -import type {IFocusableNode} from './interfaces/i_focusable_node.js'; -import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import {ConnectionType} from './connection_type.js'; import * as ContextMenu from './contextmenu.js'; import {ContextMenuRegistry} from './contextmenu_registry.js'; import * as eventUtils from './events/utils.js'; import {IContextMenu} from './interfaces/i_contextmenu.js'; +import type {IFocusableNode} from './interfaces/i_focusable_node.js'; +import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import {hasBubble} from './interfaces/i_has_bubble.js'; import * as internalConstants from './internal_constants.js'; +import type {ConstantProvider} from './renderers/common/constants.js'; import {Coordinate} from './utils/coordinate.js'; -import {WorkspaceSvg} from './workspace_svg.js'; import * as dom from './utils/dom.js'; import {Svg} from './utils/svg.js'; -import * as svgPaths from './utils/svg_paths.js'; -import type {ConstantProvider, PuzzleTab} from './renderers/common/constants.js'; import * as svgMath from './utils/svg_math.js'; +import * as svgPaths from './utils/svg_paths.js'; +import {WorkspaceSvg} from './workspace_svg.js'; /** Maximum randomness in workspace units for bumping a block. */ const BUMP_RANDOMNESS = 10; @@ -42,7 +42,8 @@ const BUMP_RANDOMNESS = 10; */ export class RenderedConnection extends Connection - implements IContextMenu, IFocusableNode { + implements IContextMenu, IFocusableNode +{ // TODO(b/109816955): remove '!', see go/strict-prop-init-fix. sourceBlock_!: BlockSvg; private readonly db: ConnectionDB; @@ -83,16 +84,15 @@ export class RenderedConnection /** Describes the state of this connection's tracked-ness. */ this.trackedState = RenderedConnection.TrackedState.WILL_TRACK; - this.constants = (source.workspace as WorkspaceSvg).getRenderer().getConstants(); + this.constants = (source.workspace as WorkspaceSvg) + .getRenderer() + .getConstants(); - this.svgGroup = dom.createSvgElement( - Svg.G, - { - 'class': 'blocklyCursor', - 'width': this.constants.CURSOR_WS_WIDTH, - 'height': this.constants.WS_CURSOR_HEIGHT, - } - ); + this.svgGroup = dom.createSvgElement(Svg.G, { + 'class': 'blocklyCursor', + 'width': this.constants.CURSOR_WS_WIDTH, + 'height': this.constants.WS_CURSOR_HEIGHT, + }); this.svgPath = dom.createSvgElement( Svg.PATH, @@ -105,10 +105,9 @@ export class RenderedConnection const y = this.getOffsetInBlock().y; const path = - svgPaths.moveTo(0, 0) + - 'c 0,10 -8,-8 -8,7.5 s 8,-2.5 8,7.5'; - // TODO: It seems that constants isn't yet initialized at this point. - // (this.constants.shapeFor(this) as PuzzleTab).pathDown; + svgPaths.moveTo(0, 0) + 'c 0,10 -8,-8 -8,7.5 s 8,-2.5 8,7.5'; + // TODO: It seems that constants isn't yet initialized at this point. + // (this.constants.shapeFor(this) as PuzzleTab).pathDown; this.svgPath.setAttribute('d', path); this.svgPath.setAttribute( 'transform', @@ -369,7 +368,7 @@ export class RenderedConnection /** Add highlighting around this connection. */ highlight() { this.highlighted = true; - const highlightSvg = this.findHighlightSvg(); + const _highlightSvg = this.findHighlightSvg(); // if (highlightSvg) { // highlightSvg.style.display = ''; // } @@ -379,9 +378,9 @@ export class RenderedConnection /** Remove the highlighting around this connection. */ unhighlight() { this.highlighted = false; - const highlightSvg = this.findHighlightSvg(); + const _highlightSvg = this.findHighlightSvg(); // if (highlightSvg) { - // highlightSvg.style.display = 'none'; + // highlightSvg.style.display = 'none'; // } // this.getSourceBlock().queueRender(); } @@ -653,7 +652,9 @@ export class RenderedConnection private findHighlightSvg(): SVGElement | null { // This cast is valid as TypeScript's definition is wrong. See: // https://github.com/microsoft/TypeScript/issues/60996. - return document.getElementById(this.id) as unknown | null as SVGElement | null; + return document.getElementById(this.id) as + | unknown + | null as SVGElement | null; } /** diff --git a/core/renderers/common/path_object.ts b/core/renderers/common/path_object.ts index d216db1a221..f722170b0d1 100644 --- a/core/renderers/common/path_object.ts +++ b/core/renderers/common/path_object.ts @@ -277,7 +277,7 @@ export class PathObject implements IPathObject { if (previousHighlight) { // TODO: Fix the highlight seemingly being recreated every time it's focused. // if (this.currentHighlightMatchesNew(connection, connectionPath, offset)) { - return previousHighlight; + return previousHighlight; // } // this.removeConnectionHighlight(connection); } diff --git a/core/renderers/zelos/drawer.ts b/core/renderers/zelos/drawer.ts index 9e08fd5d193..b38711eb6c3 100644 --- a/core/renderers/zelos/drawer.ts +++ b/core/renderers/zelos/drawer.ts @@ -234,7 +234,9 @@ export class Drawer extends BaseDrawer { } /** Returns a path to highlight the given connection. */ - override drawConnectionHighlightPath(measurable: Connection): SVGElement | undefined { + override drawConnectionHighlightPath( + measurable: Connection, + ): SVGElement | undefined { const conn = measurable.connectionModel; if ( conn.type === ConnectionType.NEXT_STATEMENT || From a346a920a900abd35da979391b88d868916e12f6 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 24 Apr 2025 21:25:16 +0000 Subject: [PATCH 19/43] fix: remove unnecessary shadow check. --- core/block_svg.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/core/block_svg.ts b/core/block_svg.ts index 7b11e142148..8bc0b7af3f6 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -1836,9 +1836,7 @@ export class BlockSvg /** See IFocusableNode.onNodeFocus. */ onNodeFocus(): void { - if (!this.isShadow()) { - common.setSelected(this); - } + common.setSelected(this); } /** See IFocusableNode.onNodeBlur. */ From 80c8859de8a6af30566132f8e9189eacbfd38c12 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 24 Apr 2025 21:33:56 +0000 Subject: [PATCH 20/43] chore: add braces. Addresses a reviewer comment. --- core/toolbox/toolbox.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/toolbox/toolbox.ts b/core/toolbox/toolbox.ts index 03442e13795..ceb756afbd6 100644 --- a/core/toolbox/toolbox.ts +++ b/core/toolbox/toolbox.ts @@ -1144,7 +1144,9 @@ export class Toolbox if (this.getSelectedItem() !== node) { this.setSelectedItem(node as IToolboxItem); } - } else this.clearSelection(); + } else { + this.clearSelection(); + } } /** See IFocusableTree.onTreeBlur. */ From c2384c61ced7b3b770630f869f9453d78f847965 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 24 Apr 2025 21:51:26 +0000 Subject: [PATCH 21/43] chore: empty commit to make CI pass. Not really necessary, but I like seeing green CI before merging. From c75aea7a4c6cddb4bfdd7258b6cd5fb56a7eb408 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 24 Apr 2025 14:48:16 -0700 Subject: [PATCH 22/43] feat: Make WorkspaceSvg and BlockSvg focusable (#8916) ## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes #8913 Fixes #8914 Fixes part of #8771 ### Proposed Changes This updates `WorkspaceSvg` and `BlockSvg` to be focusable, that is, it makes the workspace a `IFocusableTree` and blocks `IFocusableNode`s. Some important details: - While this introduces focusable tree support for `Workspace` it doesn't include two other components that are obviously needed by the keyboard navigation plugin's playground: fields and connections. These will be introduced in subsequent PRs. - Blocks are set up to automatically synchronize their selection state with their focus state. This will eventually help to replace `LineCursor`'s responsibility for managing selection state itself. - The tabindex property for the workspace and its ARIA label have been moved down to the `.blocklyWorkspace` element itself rather than its wrapper. This helps address some tab stop issues that are already addressed in the plugin (via monkey patches), but also to ensure that the workspace's main SVG group interacts correctly with `FocusManager`. - `WorkspaceSvg` is being initially set up to default to its first top block when being focused for the first time. This is to match parity with the keyboard navigation plugin, however the latter also has functionality for defaulting to a position when no blocks are present. It's not clear how to actually support this under the new focus-based system (without adding an ephemeral element on which to focus), or if it's even necessary (since the workspace root can hold focus). ### Reason for Changes This is part of an ongoing effort to ensure key components of Blockly are focusable so that they can be keyboard-navigable (with other needed changes yet both in Core Blockly and the keyboard navigation plugin). ### Test Coverage No new tests have been added. It's certainly possible to add unit tests for the focusable configurations being introduced in this PR, but it may not be highly beneficial. It's largely assumed that the individual implementations should work due to a highly tested FocusManager, and it may be the case that the interactions of the components working together is far more important to verify (that is, the end user flows). The latter is planned to be tackled as part of #8915. ### Documentation No documentation changes should be needed here. ### Additional Information This includes changes that have been pulled from #8875. --- core/block_svg.ts | 28 +++++++++- core/inject.ts | 5 -- core/renderers/common/path_object.ts | 2 +- core/workspace_svg.ts | 77 +++++++++++++++++++++++++++- 4 files changed, 103 insertions(+), 9 deletions(-) diff --git a/core/block_svg.ts b/core/block_svg.ts index 4cef79df8e6..04b7d88d1f2 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -44,6 +44,8 @@ import {IContextMenu} from './interfaces/i_contextmenu.js'; import type {ICopyable} from './interfaces/i_copyable.js'; import {IDeletable} from './interfaces/i_deletable.js'; import type {IDragStrategy, IDraggable} from './interfaces/i_draggable.js'; +import type {IFocusableNode} from './interfaces/i_focusable_node.js'; +import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import {IIcon} from './interfaces/i_icon.js'; import * as internalConstants from './internal_constants.js'; import {MarkerManager} from './marker_manager.js'; @@ -76,7 +78,8 @@ export class BlockSvg IContextMenu, ICopyable, IDraggable, - IDeletable + IDeletable, + IFocusableNode { /** * Constant for identifying rows that are to be rendered inline. @@ -210,6 +213,7 @@ export class BlockSvg // Expose this block's ID on its top-level SVG group. this.svgGroup.setAttribute('data-id', this.id); + svgPath.id = this.id; this.doInit_(); } @@ -1827,4 +1831,26 @@ export class BlockSvg ); } } + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + return this.pathObject.svgPath; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this.workspace; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void { + common.setSelected(this); + } + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void { + if (common.getSelected() === this) { + common.setSelected(null); + } + } } diff --git a/core/inject.ts b/core/inject.ts index de78fbfae75..34d9c1795f8 100644 --- a/core/inject.ts +++ b/core/inject.ts @@ -13,13 +13,11 @@ import * as common from './common.js'; import * as Css from './css.js'; import * as dropDownDiv from './dropdowndiv.js'; import {Grid} from './grid.js'; -import {Msg} from './msg.js'; import {Options} from './options.js'; import {ScrollbarPair} from './scrollbar_pair.js'; import {ShortcutRegistry} from './shortcut_registry.js'; import * as Tooltip from './tooltip.js'; import * as Touch from './touch.js'; -import * as aria from './utils/aria.js'; import * as dom from './utils/dom.js'; import {Svg} from './utils/svg.js'; import * as WidgetDiv from './widgetdiv.js'; @@ -56,8 +54,6 @@ export function inject( if (opt_options?.rtl) { dom.addClass(subContainer, 'blocklyRTL'); } - subContainer.tabIndex = 0; - aria.setState(subContainer, aria.State.LABEL, Msg['WORKSPACE_ARIA_LABEL']); containerElement!.appendChild(subContainer); const svg = createDom(subContainer, options); @@ -126,7 +122,6 @@ function createDom(container: HTMLElement, options: Options): SVGElement { 'xmlns:xlink': dom.XLINK_NS, 'version': '1.1', 'class': 'blocklySvg', - 'tabindex': '0', }, container, ); diff --git a/core/renderers/common/path_object.ts b/core/renderers/common/path_object.ts index 077f80bb741..72cf2a594ce 100644 --- a/core/renderers/common/path_object.ts +++ b/core/renderers/common/path_object.ts @@ -62,7 +62,7 @@ export class PathObject implements IPathObject { /** The primary path of the block. */ this.svgPath = dom.createSvgElement( Svg.PATH, - {'class': 'blocklyPath'}, + {'class': 'blocklyPath', 'tabindex': '-1'}, this.svgRoot, ); diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index e8d411651c5..2648005b257 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -37,6 +37,7 @@ import {EventType} from './events/type.js'; import * as eventUtils from './events/utils.js'; import {Flyout} from './flyout_base.js'; import type {FlyoutButton} from './flyout_button.js'; +import {getFocusManager} from './focus_manager.js'; import {Gesture} from './gesture.js'; import {Grid} from './grid.js'; import type {IASTNodeLocationSvg} from './interfaces/i_ast_node_location_svg.js'; @@ -44,6 +45,8 @@ import type {IBoundedElement} from './interfaces/i_bounded_element.js'; import {IContextMenu} from './interfaces/i_contextmenu.js'; import type {IDragTarget} from './interfaces/i_drag_target.js'; import type {IFlyout} from './interfaces/i_flyout.js'; +import type {IFocusableNode} from './interfaces/i_focusable_node.js'; +import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import type {IMetricsManager} from './interfaces/i_metrics_manager.js'; import type {IToolbox} from './interfaces/i_toolbox.js'; import type { @@ -54,6 +57,7 @@ import type {LineCursor} from './keyboard_nav/line_cursor.js'; import type {Marker} from './keyboard_nav/marker.js'; import {LayerManager} from './layer_manager.js'; import {MarkerManager} from './marker_manager.js'; +import {Msg} from './msg.js'; import {Options} from './options.js'; import * as Procedures from './procedures.js'; import * as registry from './registry.js'; @@ -66,6 +70,7 @@ import {Classic} from './theme/classic.js'; import {ThemeManager} from './theme_manager.js'; import * as Tooltip from './tooltip.js'; import type {Trashcan} from './trashcan.js'; +import * as aria from './utils/aria.js'; import * as arrayUtils from './utils/array.js'; import {Coordinate} from './utils/coordinate.js'; import * as dom from './utils/dom.js'; @@ -93,7 +98,7 @@ const ZOOM_TO_FIT_MARGIN = 20; */ export class WorkspaceSvg extends Workspace - implements IASTNodeLocationSvg, IContextMenu + implements IASTNodeLocationSvg, IContextMenu, IFocusableNode, IFocusableTree { /** * A wrapper function called when a resize event occurs. @@ -764,7 +769,19 @@ export class WorkspaceSvg * * */ - this.svgGroup_ = dom.createSvgElement(Svg.G, {'class': 'blocklyWorkspace'}); + this.svgGroup_ = dom.createSvgElement(Svg.G, { + 'class': 'blocklyWorkspace', + // Only the top-level workspace should be tabbable. + 'tabindex': injectionDiv ? '0' : '-1', + 'id': this.id, + }); + if (injectionDiv) { + aria.setState( + this.svgGroup_, + aria.State.LABEL, + Msg['WORKSPACE_ARIA_LABEL'], + ); + } // Note that a alone does not receive mouse events--it must have a // valid target inside it. If no background class is specified, as in the @@ -840,6 +857,9 @@ export class WorkspaceSvg this.getTheme(), isParentWorkspace ? this.getInjectionDiv() : undefined, ); + + getFocusManager().registerTree(this); + return this.svgGroup_; } @@ -924,6 +944,10 @@ export class WorkspaceSvg document.body.removeEventListener('wheel', this.dummyWheelListener); this.dummyWheelListener = null; } + + if (getFocusManager().isRegistered(this)) { + getFocusManager().unregisterTree(this); + } } /** @@ -2631,6 +2655,55 @@ export class WorkspaceSvg deltaY *= scale; this.scroll(this.scrollX + deltaX, this.scrollY + deltaY); } + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + return this.svgGroup_; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void {} + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void {} + + /** See IFocusableTree.getRootFocusableNode. */ + getRootFocusableNode(): IFocusableNode { + return this; + } + + /** See IFocusableTree.getRestoredFocusableNode. */ + getRestoredFocusableNode( + previousNode: IFocusableNode | null, + ): IFocusableNode | null { + if (!previousNode) { + return this.getTopBlocks(true)[0] ?? null; + } else return null; + } + + /** See IFocusableTree.getNestedTrees. */ + getNestedTrees(): Array { + return []; + } + + /** See IFocusableTree.lookUpFocusableNode. */ + lookUpFocusableNode(id: string): IFocusableNode | null { + return this.getBlockById(id) as IFocusableNode; + } + + /** See IFocusableTree.onTreeFocus. */ + onTreeFocus( + _node: IFocusableNode, + _previousTree: IFocusableTree | null, + ): void {} + + /** See IFocusableTree.onTreeBlur. */ + onTreeBlur(_nextTree: IFocusableTree | null): void {} } /** From d4883f57d18ac92da2752942b15ae90cffb7cee5 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 24 Apr 2025 15:08:18 -0700 Subject: [PATCH 23/43] feat: Make toolbox and flyout focusable (#8920) ## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes #8918 Fixes #8919 Fixes part of #8771 ### Proposed Changes This updates several classes in order to make toolboxes and flyouts focusable: - `IFlyout` is now an `IFocusableTree` with corresponding implementations in `FlyoutBase`. - `IToolbox` is now an `IFocusableTree` with corresponding implementations in `Toolbox`. - `IToolboxItem` is now an `IFocusableNode` with corresponding implementations in `ToolboxItem`. - As the primary toolbox items, `ToolboxCategory` and `ToolboxSeparator` were updated to have -1 tab indexes and defined IDs to help `ToolboxItem` fulfill its contracted for `IFocusableNode.getFocusableElement`. Each of these two new focusable trees have specific noteworthy behaviors behaviors: - `Toolbox` will automatically indicate that its first item should be focused (if one is present), even overriding the ability to focus the toolbox's root (however there are some cases where that can still happen). - `Toolbox` will automatically synchronize its selection state with its item nodes being focused. - `FlyoutBase`, now being a focusable tree, has had a tab index of 0 added. Normally a tab index of -1 is all that's needed, but the keyboard navigation plugin specifically uses 0 for flyout so that the flyout is tabbable. This is a **new** tab stop being introduced. - `FlyoutBase` holds a workspace (for rendering blocks) and, since `WorkspaceSvg` is already set up to be a focusable tree, it's represented as a subtree to `FlyoutBase`. This does introduce some wonky behaviors: the flyout's root will have passive focus while its contents have active focus. This could be manually disabled with some CSS if it ends up being a confusing user experience. - Both `FlyoutBase` and `WorkspaceSvg` have built-in behaviors for detecting when a user tries navigating away from an open flyout to ensure that the flyout is closed when it's supposed to be. That is, the flyout is auto-hideable and a non-flyout, non-toolbox node has then been focused. This matches parity with the `T`/`Esc` flows supported in the keyboard navigation plugin playground. One other thing to note: `Toolbox` had a few tests to update that were trying to reinit a toolbox without first disposing of it (which was caught by one of `FocusManager`'s state guardrails). ### Reason for Changes This is part of an ongoing effort to ensure key components of Blockly are focusable so that they can be keyboard-navigable (with other needed changes yet both in Core Blockly and the keyboard navigation plugin). ### Test Coverage No new tests have been added. It's certainly possible to add unit tests for the focusable configurations being introduced in this PR, but it may not be highly beneficial. It's largely assumed that the individual implementations should work due to a highly tested FocusManager, and it may be the case that the interactions of the components working together is far more important to verify (that is, the end user flows). The latter is planned to be tackled as part of #8915. ### Documentation No documentation changes should be needed here. ### Additional Information This includes changes that have been pulled from #8875. --- core/flyout_base.ts | 69 ++++++++++++++++++++++++++- core/interfaces/i_flyout.ts | 3 +- core/interfaces/i_toolbox.ts | 3 +- core/interfaces/i_toolbox_item.ts | 4 +- core/toolbox/category.ts | 2 + core/toolbox/separator.ts | 2 + core/toolbox/toolbox.ts | 77 ++++++++++++++++++++++++++++++- core/toolbox/toolbox_item.ts | 24 ++++++++++ core/workspace_svg.ts | 14 +++++- tests/mocha/toolbox_test.js | 3 ++ 10 files changed, 194 insertions(+), 7 deletions(-) diff --git a/core/flyout_base.ts b/core/flyout_base.ts index e738470a606..d24ea2758a0 100644 --- a/core/flyout_base.ts +++ b/core/flyout_base.ts @@ -21,9 +21,12 @@ import * as eventUtils from './events/utils.js'; import {FlyoutItem} from './flyout_item.js'; import {FlyoutMetricsManager} from './flyout_metrics_manager.js'; import {FlyoutSeparator, SeparatorAxis} from './flyout_separator.js'; +import {getFocusManager} from './focus_manager.js'; import {IAutoHideable} from './interfaces/i_autohideable.js'; import type {IFlyout} from './interfaces/i_flyout.js'; import type {IFlyoutInflater} from './interfaces/i_flyout_inflater.js'; +import {IFocusableNode} from './interfaces/i_focusable_node.js'; +import {IFocusableTree} from './interfaces/i_focusable_tree.js'; import type {Options} from './options.js'; import * as registry from './registry.js'; import * as renderManagement from './render_management.js'; @@ -43,7 +46,7 @@ import {WorkspaceSvg} from './workspace_svg.js'; */ export abstract class Flyout extends DeleteArea - implements IAutoHideable, IFlyout + implements IAutoHideable, IFlyout, IFocusableNode { /** * Position the flyout. @@ -303,6 +306,7 @@ export abstract class Flyout // hide/show code will set up proper visibility and size later. this.svgGroup_ = dom.createSvgElement(tagName, { 'class': 'blocklyFlyout', + 'tabindex': '0', }); this.svgGroup_.style.display = 'none'; this.svgBackground_ = dom.createSvgElement( @@ -317,6 +321,9 @@ export abstract class Flyout this.workspace_ .getThemeManager() .subscribe(this.svgBackground_, 'flyoutOpacity', 'fill-opacity'); + + getFocusManager().registerTree(this); + return this.svgGroup_; } @@ -398,6 +405,7 @@ export abstract class Flyout if (this.svgGroup_) { dom.removeNode(this.svgGroup_); } + getFocusManager().unregisterTree(this); } /** @@ -961,4 +969,63 @@ export abstract class Flyout return null; } + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + if (!this.svgGroup_) throw new Error('Flyout DOM is not yet created.'); + return this.svgGroup_; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void {} + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void {} + + /** See IFocusableTree.getRootFocusableNode. */ + getRootFocusableNode(): IFocusableNode { + return this; + } + + /** See IFocusableTree.getRestoredFocusableNode. */ + getRestoredFocusableNode( + _previousNode: IFocusableNode | null, + ): IFocusableNode | null { + return null; + } + + /** See IFocusableTree.getNestedTrees. */ + getNestedTrees(): Array { + return [this.workspace_]; + } + + /** See IFocusableTree.lookUpFocusableNode. */ + lookUpFocusableNode(_id: string): IFocusableNode | null { + // No focusable node needs to be returned since the flyout's subtree is a + // workspace that will manage its own focusable state. + return null; + } + + /** See IFocusableTree.onTreeFocus. */ + onTreeFocus( + _node: IFocusableNode, + _previousTree: IFocusableTree | null, + ): void {} + + /** See IFocusableTree.onTreeBlur. */ + onTreeBlur(nextTree: IFocusableTree | null): void { + const toolbox = this.targetWorkspace.getToolbox(); + // If focus is moving to either the toolbox or the flyout's workspace, do + // not close the flyout. For anything else, do close it since the flyout is + // no longer focused. + if (toolbox && nextTree === toolbox) return; + if (nextTree == this.workspace_) return; + if (toolbox) toolbox.clearSelection(); + this.autoHide(false); + } } diff --git a/core/interfaces/i_flyout.ts b/core/interfaces/i_flyout.ts index 42204775ece..067cd5ef20d 100644 --- a/core/interfaces/i_flyout.ts +++ b/core/interfaces/i_flyout.ts @@ -12,12 +12,13 @@ import type {Coordinate} from '../utils/coordinate.js'; import type {Svg} from '../utils/svg.js'; import type {FlyoutDefinition} from '../utils/toolbox.js'; import type {WorkspaceSvg} from '../workspace_svg.js'; +import {IFocusableTree} from './i_focusable_tree.js'; import type {IRegistrable} from './i_registrable.js'; /** * Interface for a flyout. */ -export interface IFlyout extends IRegistrable { +export interface IFlyout extends IRegistrable, IFocusableTree { /** Whether the flyout is laid out horizontally or not. */ horizontalLayout: boolean; diff --git a/core/interfaces/i_toolbox.ts b/core/interfaces/i_toolbox.ts index 1780af94d8a..f5d9c9fd7c6 100644 --- a/core/interfaces/i_toolbox.ts +++ b/core/interfaces/i_toolbox.ts @@ -9,13 +9,14 @@ import type {ToolboxInfo} from '../utils/toolbox.js'; import type {WorkspaceSvg} from '../workspace_svg.js'; import type {IFlyout} from './i_flyout.js'; +import type {IFocusableTree} from './i_focusable_tree.js'; import type {IRegistrable} from './i_registrable.js'; import type {IToolboxItem} from './i_toolbox_item.js'; /** * Interface for a toolbox. */ -export interface IToolbox extends IRegistrable { +export interface IToolbox extends IRegistrable, IFocusableTree { /** Initializes the toolbox. */ init(): void; diff --git a/core/interfaces/i_toolbox_item.ts b/core/interfaces/i_toolbox_item.ts index e3c9864f0c0..661624fd7e8 100644 --- a/core/interfaces/i_toolbox_item.ts +++ b/core/interfaces/i_toolbox_item.ts @@ -6,10 +6,12 @@ // Former goog.module ID: Blockly.IToolboxItem +import type {IFocusableNode} from './i_focusable_node.js'; + /** * Interface for an item in the toolbox. */ -export interface IToolboxItem { +export interface IToolboxItem extends IFocusableNode { /** * Initializes the toolbox item. * This includes creating the DOM and updating the state of any items based diff --git a/core/toolbox/category.ts b/core/toolbox/category.ts index d8ee8736ea6..fc7d1aa03cf 100644 --- a/core/toolbox/category.ts +++ b/core/toolbox/category.ts @@ -225,6 +225,8 @@ export class ToolboxCategory */ protected createContainer_(): HTMLDivElement { const container = document.createElement('div'); + container.tabIndex = -1; + container.id = this.getId(); const className = this.cssConfig_['container']; if (className) { dom.addClass(container, className); diff --git a/core/toolbox/separator.ts b/core/toolbox/separator.ts index 31ccb7e42f3..44ae358cf53 100644 --- a/core/toolbox/separator.ts +++ b/core/toolbox/separator.ts @@ -54,6 +54,8 @@ export class ToolboxSeparator extends ToolboxItem { */ protected createDom_(): HTMLDivElement { const container = document.createElement('div'); + container.tabIndex = -1; + container.id = this.getId(); const className = this.cssConfig_['container']; if (className) { dom.addClass(container, className); diff --git a/core/toolbox/toolbox.ts b/core/toolbox/toolbox.ts index b0fd82e97f2..ceb756afbd6 100644 --- a/core/toolbox/toolbox.ts +++ b/core/toolbox/toolbox.ts @@ -22,11 +22,14 @@ import {DeleteArea} from '../delete_area.js'; import '../events/events_toolbox_item_select.js'; import {EventType} from '../events/type.js'; import * as eventUtils from '../events/utils.js'; +import {getFocusManager} from '../focus_manager.js'; import type {IAutoHideable} from '../interfaces/i_autohideable.js'; import type {ICollapsibleToolboxItem} from '../interfaces/i_collapsible_toolbox_item.js'; import {isDeletable} from '../interfaces/i_deletable.js'; import type {IDraggable} from '../interfaces/i_draggable.js'; import type {IFlyout} from '../interfaces/i_flyout.js'; +import type {IFocusableNode} from '../interfaces/i_focusable_node.js'; +import type {IFocusableTree} from '../interfaces/i_focusable_tree.js'; import type {IKeyboardAccessible} from '../interfaces/i_keyboard_accessible.js'; import type {ISelectableToolboxItem} from '../interfaces/i_selectable_toolbox_item.js'; import {isSelectableToolboxItem} from '../interfaces/i_selectable_toolbox_item.js'; @@ -51,7 +54,12 @@ import {CollapsibleToolboxCategory} from './collapsible_category.js'; */ export class Toolbox extends DeleteArea - implements IAutoHideable, IKeyboardAccessible, IStyleable, IToolbox + implements + IAutoHideable, + IKeyboardAccessible, + IStyleable, + IToolbox, + IFocusableNode { /** * The unique ID for this component that is used to register with the @@ -163,6 +171,7 @@ export class Toolbox ComponentManager.Capability.DRAG_TARGET, ], }); + getFocusManager().registerTree(this); } /** @@ -177,7 +186,6 @@ export class Toolbox const container = this.createContainer_(); this.contentsDiv_ = this.createContentsContainer_(); - this.contentsDiv_.tabIndex = 0; aria.setRole(this.contentsDiv_, aria.Role.TREE); container.appendChild(this.contentsDiv_); @@ -194,6 +202,7 @@ export class Toolbox */ protected createContainer_(): HTMLDivElement { const toolboxContainer = document.createElement('div'); + toolboxContainer.tabIndex = 0; toolboxContainer.setAttribute('layout', this.isHorizontal() ? 'h' : 'v'); dom.addClass(toolboxContainer, 'blocklyToolbox'); toolboxContainer.setAttribute('dir', this.RTL ? 'RTL' : 'LTR'); @@ -1077,7 +1086,71 @@ export class Toolbox this.workspace_.getThemeManager().unsubscribe(this.HtmlDiv); dom.removeNode(this.HtmlDiv); } + + getFocusManager().unregisterTree(this); + } + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + if (!this.HtmlDiv) throw Error('Toolbox DOM has not yet been created.'); + return this.HtmlDiv; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void {} + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void {} + + /** See IFocusableTree.getRootFocusableNode. */ + getRootFocusableNode(): IFocusableNode { + return this; + } + + /** See IFocusableTree.getRestoredFocusableNode. */ + getRestoredFocusableNode( + previousNode: IFocusableNode | null, + ): IFocusableNode | null { + // Always try to select the first selectable toolbox item rather than the + // root of the toolbox. + if (!previousNode || previousNode === this) { + return this.getToolboxItems().find((item) => item.isSelectable()) ?? null; + } + return null; } + + /** See IFocusableTree.getNestedTrees. */ + getNestedTrees(): Array { + return []; + } + + /** See IFocusableTree.lookUpFocusableNode. */ + lookUpFocusableNode(id: string): IFocusableNode | null { + return this.getToolboxItemById(id) as IFocusableNode; + } + + /** See IFocusableTree.onTreeFocus. */ + onTreeFocus( + node: IFocusableNode, + _previousTree: IFocusableTree | null, + ): void { + if (node !== this) { + // Only select the item if it isn't already selected so as to not toggle. + if (this.getSelectedItem() !== node) { + this.setSelectedItem(node as IToolboxItem); + } + } else { + this.clearSelection(); + } + } + + /** See IFocusableTree.onTreeBlur. */ + onTreeBlur(_nextTree: IFocusableTree | null): void {} } /** CSS for Toolbox. See css.js for use. */ diff --git a/core/toolbox/toolbox_item.ts b/core/toolbox/toolbox_item.ts index ef9d979ab43..0d46a5eadfd 100644 --- a/core/toolbox/toolbox_item.ts +++ b/core/toolbox/toolbox_item.ts @@ -12,6 +12,7 @@ // Former goog.module ID: Blockly.ToolboxItem import type {ICollapsibleToolboxItem} from '../interfaces/i_collapsible_toolbox_item.js'; +import type {IFocusableTree} from '../interfaces/i_focusable_tree.js'; import type {IToolbox} from '../interfaces/i_toolbox.js'; import type {IToolboxItem} from '../interfaces/i_toolbox_item.js'; import * as idGenerator from '../utils/idgenerator.js'; @@ -148,5 +149,28 @@ export class ToolboxItem implements IToolboxItem { * @param _isVisible True if category should be visible. */ setVisible_(_isVisible: boolean) {} + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + const div = this.getDiv(); + if (!div) { + throw Error('Trying to access toolbox item before DOM is initialized.'); + } + if (!(div instanceof HTMLElement)) { + throw Error('Toolbox item div is unexpectedly not an HTML element.'); + } + return div as HTMLElement; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this.parentToolbox_; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void {} + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void {} } // nop by default diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index 2648005b257..5f4c57a5774 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -2703,7 +2703,19 @@ export class WorkspaceSvg ): void {} /** See IFocusableTree.onTreeBlur. */ - onTreeBlur(_nextTree: IFocusableTree | null): void {} + onTreeBlur(nextTree: IFocusableTree | null): void { + // If the flyout loses focus, make sure to close it. + if (this.isFlyout && this.targetWorkspace) { + // Only hide the flyout if the flyout's workspace is losing focus and that + // focus isn't returning to the flyout itself or the toolbox. + const flyout = this.targetWorkspace.getFlyout(); + const toolbox = this.targetWorkspace.getToolbox(); + if (flyout && nextTree === flyout) return; + if (toolbox && nextTree === toolbox) return; + if (toolbox) toolbox.clearSelection(); + if (flyout && flyout instanceof Flyout) flyout.autoHide(false); + } + } } /** diff --git a/tests/mocha/toolbox_test.js b/tests/mocha/toolbox_test.js index 10bfd335223..f32319c6779 100644 --- a/tests/mocha/toolbox_test.js +++ b/tests/mocha/toolbox_test.js @@ -54,6 +54,7 @@ suite('Toolbox', function () { const themeManagerSpy = sinon.spy(themeManager, 'subscribe'); const componentManager = this.toolbox.workspace_.getComponentManager(); sinon.stub(componentManager, 'addComponent'); + this.toolbox.dispose(); // Dispose of the old toolbox so that it can be reinited. this.toolbox.init(); sinon.assert.calledWith( themeManagerSpy, @@ -72,12 +73,14 @@ suite('Toolbox', function () { const renderSpy = sinon.spy(this.toolbox, 'render'); const componentManager = this.toolbox.workspace_.getComponentManager(); sinon.stub(componentManager, 'addComponent'); + this.toolbox.dispose(); // Dispose of the old toolbox so that it can be reinited. this.toolbox.init(); sinon.assert.calledOnce(renderSpy); }); test('Init called -> Flyout is initialized', function () { const componentManager = this.toolbox.workspace_.getComponentManager(); sinon.stub(componentManager, 'addComponent'); + this.toolbox.dispose(); // Dispose of the old toolbox so that it can be reinited. this.toolbox.init(); assert.isDefined(this.toolbox.getFlyout()); }); From 8fc5c05ff549e24e81dfb97e5edb093ebc76cda6 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 29 Apr 2025 20:06:25 +0000 Subject: [PATCH 24/43] chore: remove connections rendering changes. These will be done in a later change, instead, to expedite merging the actual focus integration bits. --- core/css.ts | 8 +-- core/rendered_connection.ts | 75 +++----------------------- core/renderers/common/constants.ts | 6 +-- core/renderers/common/drawer.ts | 20 +++---- core/renderers/common/i_path_object.ts | 2 +- core/renderers/common/path_object.ts | 30 ++++------- core/renderers/zelos/constants.ts | 6 +-- core/renderers/zelos/drawer.ts | 9 ++-- core/renderers/zelos/path_object.ts | 30 +++++------ 9 files changed, 56 insertions(+), 130 deletions(-) diff --git a/core/css.ts b/core/css.ts index ebdc613c050..6ca262f3b25 100644 --- a/core/css.ts +++ b/core/css.ts @@ -147,8 +147,8 @@ let content = ` .blocklyHighlightedConnectionPath { fill: none; - // stroke: #fc3; - // stroke-width: 4px; + stroke: #fc3; + stroke-width: 4px; } .blocklyPathLight { @@ -463,8 +463,8 @@ input[type=number] { } .blocklyMenuSeparator { - background-color: #ccc; - height: 1px; + background-color: #ccc; + height: 1px; border: 0; margin-left: 4px; margin-right: 4px; diff --git a/core/rendered_connection.ts b/core/rendered_connection.ts index 5355033ea12..406b539b4e9 100644 --- a/core/rendered_connection.ts +++ b/core/rendered_connection.ts @@ -26,12 +26,8 @@ import type {IFocusableNode} from './interfaces/i_focusable_node.js'; import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import {hasBubble} from './interfaces/i_has_bubble.js'; import * as internalConstants from './internal_constants.js'; -import type {ConstantProvider} from './renderers/common/constants.js'; import {Coordinate} from './utils/coordinate.js'; -import * as dom from './utils/dom.js'; -import {Svg} from './utils/svg.js'; import * as svgMath from './utils/svg_math.js'; -import * as svgPaths from './utils/svg_paths.js'; import {WorkspaceSvg} from './workspace_svg.js'; /** Maximum randomness in workspace units for bumping a block. */ @@ -51,9 +47,6 @@ export class RenderedConnection private readonly offsetInBlock: Coordinate; private trackedState: TrackedState; private highlighted: boolean = false; - private constants: ConstantProvider; - private svgGroup: SVGElement; - private svgPath: SVGElement; /** Connection this connection connects to. Null if not connected. */ override targetConnection: RenderedConnection | null = null; @@ -83,41 +76,6 @@ export class RenderedConnection /** Describes the state of this connection's tracked-ness. */ this.trackedState = RenderedConnection.TrackedState.WILL_TRACK; - - this.constants = (source.workspace as WorkspaceSvg) - .getRenderer() - .getConstants(); - - this.svgGroup = dom.createSvgElement(Svg.G, { - 'class': 'blocklyCursor', - 'width': this.constants.CURSOR_WS_WIDTH, - 'height': this.constants.WS_CURSOR_HEIGHT, - }); - - this.svgPath = dom.createSvgElement( - Svg.PATH, - {'transform': ''}, - this.svgGroup, - ); - - // TODO: Ensure this auto-moves with the block. - const x = this.getOffsetInBlock().x; - const y = this.getOffsetInBlock().y; - - const path = - svgPaths.moveTo(0, 0) + 'c 0,10 -8,-8 -8,7.5 s 8,-2.5 8,7.5'; - // TODO: It seems that constants isn't yet initialized at this point. - // (this.constants.shapeFor(this) as PuzzleTab).pathDown; - this.svgPath.setAttribute('d', path); - this.svgPath.setAttribute( - 'transform', - 'translate(' + - x + - ',' + - y + - ')' + - (this.sourceBlock_.workspace.RTL ? ' scale(-1 1)' : ''), - ); } /** @@ -368,21 +326,13 @@ export class RenderedConnection /** Add highlighting around this connection. */ highlight() { this.highlighted = true; - const _highlightSvg = this.findHighlightSvg(); - // if (highlightSvg) { - // highlightSvg.style.display = ''; - // } - // this.getSourceBlock().queueRender(); + this.getSourceBlock().queueRender(); } /** Remove the highlighting around this connection. */ unhighlight() { this.highlighted = false; - const _highlightSvg = this.findHighlightSvg(); - // if (highlightSvg) { - // highlightSvg.style.display = 'none'; - // } - // this.getSourceBlock().queueRender(); + this.getSourceBlock().queueRender(); } /** Returns true if this connection is highlighted, false otherwise. */ @@ -649,14 +599,6 @@ export class RenderedConnection return this; } - private findHighlightSvg(): SVGElement | null { - // This cast is valid as TypeScript's definition is wrong. See: - // https://github.com/microsoft/TypeScript/issues/60996. - return document.getElementById(this.id) as - | unknown - | null as SVGElement | null; - } - /** * Handles showing the context menu when it is opened on a connection. * Note that typically the context menu can't be opened with the mouse @@ -693,11 +635,12 @@ export class RenderedConnection /** See IFocusableNode.getFocusableElement. */ getFocusableElement(): HTMLElement | SVGElement { - const highlightSvg = this.findHighlightSvg(); - if (highlightSvg) { - highlightSvg.setAttribute('aria-label', 'Connection'); - return highlightSvg; - } + // This cast is valid as TypeScript's type definition is wrong. See: + // https://github.com/microsoft/TypeScript/issues/60996. + const highlightSvg = document.getElementById(this.id) as + | unknown + | null as SVGElement | null; + if (highlightSvg) return highlightSvg; throw new Error('No highlight SVG found corresponding to this connection.'); } @@ -708,12 +651,10 @@ export class RenderedConnection /** See IFocusableNode.onNodeFocus. */ onNodeFocus(): void { - this.highlight(); } /** See IFocusableNode.onNodeBlur. */ onNodeBlur(): void { - this.unhighlight(); } } diff --git a/core/renderers/common/constants.ts b/core/renderers/common/constants.ts index c8f770cc407..c5a7a759c5c 100644 --- a/core/renderers/common/constants.ts +++ b/core/renderers/common/constants.ts @@ -1203,9 +1203,9 @@ export class ConstantProvider { `}`, // Connection highlight. - // `${selector} .blocklyHighlightedConnectionPath {`, - // `stroke: #fc3;`, - // `}`, + `${selector} .blocklyHighlightedConnectionPath {`, + `stroke: #fc3;`, + `}`, // Replaceable highlight. `${selector} .blocklyReplaceable .blocklyPath {`, diff --git a/core/renderers/common/drawer.ts b/core/renderers/common/drawer.ts index f805451a725..09320710c51 100644 --- a/core/renderers/common/drawer.ts +++ b/core/renderers/common/drawer.ts @@ -435,23 +435,19 @@ export class Drawer { for (const elem of row.elements) { if (!(elem instanceof Connection)) continue; - const highlightSvg = this.drawConnectionHighlightPath(elem); - if (highlightSvg) { - // highlightSvg.style.display = elem.highlighted ? '' : 'none'; + if (elem.highlighted) { + this.drawConnectionHighlightPath(elem); + } else { + this.block_.pathObject.removeConnectionHighlight?.( + elem.connectionModel, + ); } - // if (elem.highlighted) { - // this.drawConnectionHighlightPath(elem); - // } else { - // this.block_.pathObject.removeConnectionHighlight?.( - // elem.connectionModel, - // ); - // } } } } /** Returns a path to highlight the given connection. */ - drawConnectionHighlightPath(measurable: Connection): SVGElement | undefined { + drawConnectionHighlightPath(measurable: Connection) { const conn = measurable.connectionModel; let path = ''; if ( @@ -463,7 +459,7 @@ export class Drawer { path = this.getStatementConnectionHighlightPath(measurable); } const block = conn.getSourceBlock(); - return block.pathObject.addConnectionHighlight?.( + block.pathObject.addConnectionHighlight?.( conn, path, conn.getOffsetInBlock(), diff --git a/core/renderers/common/i_path_object.ts b/core/renderers/common/i_path_object.ts index 776ba0067ea..699f1d92edb 100644 --- a/core/renderers/common/i_path_object.ts +++ b/core/renderers/common/i_path_object.ts @@ -113,7 +113,7 @@ export interface IPathObject { connectionPath: string, offset: Coordinate, rtl: boolean, - ): SVGElement; + ): void; /** * Apply the stored colours to the block's path, taking into account whether diff --git a/core/renderers/common/path_object.ts b/core/renderers/common/path_object.ts index f722170b0d1..5bd234b0d4d 100644 --- a/core/renderers/common/path_object.ts +++ b/core/renderers/common/path_object.ts @@ -62,7 +62,7 @@ export class PathObject implements IPathObject { /** The primary path of the block. */ this.svgPath = dom.createSvgElement( Svg.PATH, - {'class': 'blocklyPath', 'tabindex': '-1'}, + {'class': 'blocklyPath'}, this.svgRoot, ); @@ -153,17 +153,13 @@ export class PathObject implements IPathObject { * removed. */ protected setClass_(className: string, add: boolean) { - this.setClassOnElem_(this.svgRoot, className, add); - } - - private setClassOnElem_(root: SVGElement, className: string, add: boolean) { if (!className) { return; } if (add) { - dom.addClass(root, className); + dom.addClass(this.svgRoot, className); } else { - dom.removeClass(root, className); + dom.removeClass(this.svgRoot, className); } } @@ -213,7 +209,7 @@ export class PathObject implements IPathObject { * @param enable True if selection is enabled, false otherwise. */ updateSelected(enable: boolean) { - this.setClassOnElem_(this.svgPath, 'blocklySelected', enable); + this.setClass_('blocklySelected', enable); } /** @@ -272,14 +268,12 @@ export class PathObject implements IPathObject { connectionPath: string, offset: Coordinate, rtl: boolean, - ): SVGElement { - const previousHighlight = this.connectionHighlights.get(connection); - if (previousHighlight) { - // TODO: Fix the highlight seemingly being recreated every time it's focused. - // if (this.currentHighlightMatchesNew(connection, connectionPath, offset)) { - return previousHighlight; - // } - // this.removeConnectionHighlight(connection); + ) { + if (this.connectionHighlights.has(connection)) { + if (this.currentHighlightMatchesNew(connection, connectionPath, offset)) { + return; + } + this.removeConnectionHighlight(connection); } const highlight = dom.createSvgElement( @@ -287,7 +281,6 @@ export class PathObject implements IPathObject { { 'id': connection.id, 'class': 'blocklyHighlightedConnectionPath', - // 'style': 'display: none;', 'tabindex': '-1', 'd': connectionPath, 'transform': @@ -295,10 +288,7 @@ export class PathObject implements IPathObject { }, this.svgRoot, ); - // TODO: Do this in a cleaner way. One possibility: create the path without 'd' or 'transform' in RenderedConnection, then just update it here (and keep registrations). - (highlight as any).renderedConnection = connection; this.connectionHighlights.set(connection, highlight); - return highlight; } private currentHighlightMatchesNew( diff --git a/core/renderers/zelos/constants.ts b/core/renderers/zelos/constants.ts index d7be09b93de..8cd36e02589 100644 --- a/core/renderers/zelos/constants.ts +++ b/core/renderers/zelos/constants.ts @@ -885,9 +885,9 @@ export class ConstantProvider extends BaseConstantProvider { `}`, // Connection highlight. - // `${selector} .blocklyHighlightedConnectionPath {`, - // `stroke: ${this.SELECTED_GLOW_COLOUR};`, - // `}`, + `${selector} .blocklyHighlightedConnectionPath {`, + `stroke: ${this.SELECTED_GLOW_COLOUR};`, + `}`, // Disabled outline paths. `${selector} .blocklyDisabledPattern > .blocklyOutlinePath {`, diff --git a/core/renderers/zelos/drawer.ts b/core/renderers/zelos/drawer.ts index b38711eb6c3..5cc52c0cbb2 100644 --- a/core/renderers/zelos/drawer.ts +++ b/core/renderers/zelos/drawer.ts @@ -234,16 +234,15 @@ export class Drawer extends BaseDrawer { } /** Returns a path to highlight the given connection. */ - override drawConnectionHighlightPath( - measurable: Connection, - ): SVGElement | undefined { + drawConnectionHighlightPath(measurable: Connection) { const conn = measurable.connectionModel; if ( conn.type === ConnectionType.NEXT_STATEMENT || conn.type === ConnectionType.PREVIOUS_STATEMENT || (conn.type === ConnectionType.OUTPUT_VALUE && !measurable.isDynamicShape) ) { - return super.drawConnectionHighlightPath(measurable); + super.drawConnectionHighlightPath(measurable); + return; } let path = ''; @@ -262,7 +261,7 @@ export class Drawer extends BaseDrawer { (output.shape as DynamicShape).pathDown(output.height); } const block = conn.getSourceBlock(); - return block.pathObject.addConnectionHighlight?.( + block.pathObject.addConnectionHighlight?.( conn, path, conn.getOffsetInBlock(), diff --git a/core/renderers/zelos/path_object.ts b/core/renderers/zelos/path_object.ts index 091c8719f71..f40426483a7 100644 --- a/core/renderers/zelos/path_object.ts +++ b/core/renderers/zelos/path_object.ts @@ -85,21 +85,21 @@ export class PathObject extends BasePathObject { } } - // override updateSelected(enable: boolean) { - // this.setClass_('blocklySelected', enable); - // if (enable) { - // if (!this.svgPathSelected) { - // this.svgPathSelected = this.svgPath.cloneNode(true) as SVGElement; - // this.svgPathSelected.classList.add('blocklyPathSelected'); - // this.svgRoot.appendChild(this.svgPathSelected); - // } - // } else { - // if (this.svgPathSelected) { - // this.svgRoot.removeChild(this.svgPathSelected); - // this.svgPathSelected = null; - // } - // } - // } + override updateSelected(enable: boolean) { + this.setClass_('blocklySelected', enable); + if (enable) { + if (!this.svgPathSelected) { + this.svgPathSelected = this.svgPath.cloneNode(true) as SVGElement; + this.svgPathSelected.classList.add('blocklyPathSelected'); + this.svgRoot.appendChild(this.svgPathSelected); + } + } else { + if (this.svgPathSelected) { + this.svgRoot.removeChild(this.svgPathSelected); + this.svgPathSelected = null; + } + } + } override updateReplacementFade(enable: boolean) { this.setClass_('blocklyReplaceable', enable); From cb2148b5b5ca24d46fe8f292354bfed48bdb711a Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 29 Apr 2025 21:12:07 +0000 Subject: [PATCH 25/43] feat: Update LineCursor to use FocusManager. This updates LineCursor to use IFocusableNodes by default, only falling back to selection when necessary. It also heavily relies on BlockSvg's automatic selection synchronizing to avoid needing to explicitly update selection. Eventually, the selection logic will be able to be removed entirely once comprehensive focus support is verified. Separately, the marker drawing (and marker itself) will be able to be removed once selection, connection, and active/passive rendering is finished. --- core/keyboard_nav/line_cursor.ts | 114 +++++++++++-------------------- 1 file changed, 39 insertions(+), 75 deletions(-) diff --git a/core/keyboard_nav/line_cursor.ts b/core/keyboard_nav/line_cursor.ts index 2025da7bf06..325cce17315 100644 --- a/core/keyboard_nav/line_cursor.ts +++ b/core/keyboard_nav/line_cursor.ts @@ -18,11 +18,9 @@ import {BlockSvg} from '../block_svg.js'; import * as common from '../common.js'; import type {Connection} from '../connection.js'; import {ConnectionType} from '../connection_type.js'; -import type {Abstract} from '../events/events_abstract.js'; -import {Click, ClickTarget} from '../events/events_click.js'; -import {EventType} from '../events/type.js'; -import * as eventUtils from '../events/utils.js'; import type {Field} from '../field.js'; +import {getFocusManager} from '../focus_manager.js'; +import {isFocusableNode} from '../interfaces/i_focusable_node.js'; import * as registry from '../registry.js'; import type {MarkerSvg} from '../renderers/common/marker_svg.js'; import type {PathObject} from '../renderers/zelos/path_object.js'; @@ -70,23 +68,12 @@ export class LineCursor extends Marker { options?: Partial, ) { super(); - // Bind changeListener to facilitate future disposal. - this.changeListener = this.changeListener.bind(this); - this.workspace.addChangeListener(this.changeListener); // Regularise options and apply defaults. this.options = {...defaultOptions, ...options}; this.isZelos = workspace.getRenderer() instanceof Renderer; } - /** - * Clean up this cursor. - */ - dispose() { - this.workspace.removeChangeListener(this.changeListener); - super.dispose(); - } - /** * Moves the cursor to the next previous connection, next connection or block * in the pre order traversal. Finds the next node in the pre order traversal. @@ -331,8 +318,8 @@ export class LineCursor extends Marker { * @param node The current position in the AST. * @param isValid A function true/false depending on whether the given node * should be traversed. - * @param loop Whether to loop around to the beginning of the workspace if - * novalid node was found. + * @param loop Whether to loop around to the beginning of the workspace if no + * valid node was found. * @returns The next node in the traversal. */ getNextNode( @@ -385,8 +372,8 @@ export class LineCursor extends Marker { * @param node The current position in the AST. * @param isValid A function true/false depending on whether the given node * should be traversed. - * @param loop Whether to loop around to the end of the workspace if no - * valid node was found. + * @param loop Whether to loop around to the end of the workspace if no valid + * node was found. * @returns The previous node in the traversal or null if no previous node * exists. */ @@ -527,7 +514,12 @@ export class LineCursor extends Marker { * @returns The current field, connection, or block the cursor is on. */ override getCurNode(): ASTNode | null { - this.updateCurNodeFromSelection(); + if (!this.updateCurNodeFromFocus()) { + // Fall back to selection if focus fails to sync. This can happen for + // non-focusable nodes or for cases when focus may not properly propagate + // (such as for mouse clicks). + this.updateCurNodeFromSelection(); + } return super.getCurNode(); } @@ -572,16 +564,15 @@ export class LineCursor extends Marker { * this.drawMarker() instead of this.drawer.draw() directly. * * @param newNode The new location of the cursor. - * @param updateSelection If true (the default) we'll update the selection - * too. */ - override setCurNode(newNode: ASTNode | null, updateSelection = true) { - if (updateSelection) { - this.updateSelectionFromNode(newNode); - } - + override setCurNode(newNode: ASTNode | null) { super.setCurNode(newNode); + const newNodeLocation = newNode?.getLocation(); + if (isFocusableNode(newNodeLocation)) { + getFocusManager().focusNode(newNodeLocation); + } + // Try to scroll cursor into view. if (newNode?.getType() === ASTNode.types.BLOCK) { const block = newNode.getLocation() as BlockSvg; @@ -709,32 +700,6 @@ export class LineCursor extends Marker { } } - /** - * Event listener that syncs the cursor location to the selected block on - * SELECTED events. - * - * This does not run early enough in all cases so `getCurNode()` also updates - * the node from the selection. - * - * @param event The `Selected` event. - */ - private changeListener(event: Abstract) { - switch (event.type) { - case EventType.SELECTED: - this.updateCurNodeFromSelection(); - break; - case EventType.CLICK: { - const click = event as Click; - if ( - click.workspaceId === this.workspace.id && - click.targetType === ClickTarget.WORKSPACE - ) { - this.setCurNode(null); - } - } - } - } - /** * Updates the current node to match the selection. * @@ -749,7 +714,7 @@ export class LineCursor extends Marker { const selected = common.getSelected(); if (selected === null && curNode?.getType() === ASTNode.types.BLOCK) { - this.setCurNode(null, false); + this.setCurNode(null); return; } if (selected?.workspace !== this.workspace) { @@ -770,36 +735,35 @@ export class LineCursor extends Marker { block = block.getParent(); } if (block) { - this.setCurNode(ASTNode.createBlockNode(block), false); + this.setCurNode(ASTNode.createBlockNode(block)); } } } /** - * Updates the selection from the node. + * Updates the current node to match what's currently focused. * - * Clears the selection for non-block nodes. - * Clears the selection for shadow blocks as the selection is drawn on - * the parent but the cursor will be drawn on the shadow block itself. - * We need to take care not to later clear the current node due to that null - * selection, so we track the latest selection we're in sync with. - * - * @param newNode The new node. + * @returns Whether the current node has been set successfully from the + * current focused node. */ - private updateSelectionFromNode(newNode: ASTNode | null) { - if (newNode?.getType() === ASTNode.types.BLOCK) { - if (common.getSelected() !== newNode.getLocation()) { - eventUtils.disable(); - common.setSelected(newNode.getLocation() as BlockSvg); - eventUtils.enable(); - } - } else { - if (common.getSelected()) { - eventUtils.disable(); - common.setSelected(null); - eventUtils.enable(); + private updateCurNodeFromFocus(): boolean { + const focused = getFocusManager().getFocusedNode(); + + if (focused instanceof BlockSvg) { + let block: BlockSvg | null = focused; + if (block && block.workspace === this.workspace) { + if (block.getRootBlock() === block && this.workspace.isFlyout) { + // This block actually represents a stack. Note that this is needed + // because ASTNode special cases stack for cross-block navigation. + this.setCurNode(ASTNode.createStackNode(block)); + } else { + this.setCurNode(ASTNode.createBlockNode(block)); + } + return true; } } + + return false; } /** From 2cae22e424c9a1451bc0e32fdcf176e0ef61df36 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 30 Apr 2025 00:17:10 +0000 Subject: [PATCH 26/43] fix: A few things for the plugin. This exposes FocusableTreeTraverser for the plugin's use. It also fixes a few callback issues with FocusManager that were missed in #8909. --- core/blockly.ts | 4 ++++ core/focus_manager.ts | 14 ++++++++++---- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/core/blockly.ts b/core/blockly.ts index c38a1d48e4b..94c4ca04132 100644 --- a/core/blockly.ts +++ b/core/blockly.ts @@ -111,6 +111,9 @@ import { ReturnEphemeralFocus, getFocusManager, } from './focus_manager.js'; +import { + FocusableTreeTraverser, +} from './utils/focusable_tree_traverser.js'; import {CodeGenerator} from './generator.js'; import {Gesture} from './gesture.js'; import {Grid} from './grid.js'; @@ -527,6 +530,7 @@ export { FlyoutMetricsManager, FlyoutSeparator, FocusManager, + FocusableTreeTraverser, CodeGenerator as Generator, Gesture, Grid, diff --git a/core/focus_manager.ts b/core/focus_manager.ts index 88eef46b530..a5104c7d96a 100644 --- a/core/focus_manager.ts +++ b/core/focus_manager.ts @@ -247,7 +247,7 @@ export class FocusManager { const prevNode = this.focusedNode; const prevTree = prevNode?.getFocusableTree(); - if (prevNode && prevTree !== nextTree) { + if (prevNode) { this.passivelyFocusNode(prevNode, nextTree); } @@ -374,7 +374,9 @@ export class FocusManager { // node's focusable element (which *is* allowed to be invisible until the // node needs to be focused). this.lockFocusStateChanges = true; - node.getFocusableTree().onTreeFocus(node, prevTree); + if (node.getFocusableTree() !== prevTree) { + node.getFocusableTree().onTreeFocus(node, prevTree); + } node.onNodeFocus(); this.lockFocusStateChanges = false; @@ -399,11 +401,15 @@ export class FocusManager { nextTree: IFocusableTree | null, ): void { this.lockFocusStateChanges = true; - node.getFocusableTree().onTreeBlur(nextTree); + if (node.getFocusableTree() !== nextTree) { + node.getFocusableTree().onTreeBlur(nextTree); + } node.onNodeBlur(); this.lockFocusStateChanges = false; - this.setNodeToVisualPassiveFocus(node); + if (node.getFocusableTree() !== nextTree) { + this.setNodeToVisualPassiveFocus(node); + } } /** From 8f65a3b55d2d5bf30e9ac4deda8e18c681078bd5 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 30 Apr 2025 00:18:56 +0000 Subject: [PATCH 27/43] fix: Undo break for Block focusability. --- core/renderers/common/path_object.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/renderers/common/path_object.ts b/core/renderers/common/path_object.ts index 5bd234b0d4d..fe7908b383f 100644 --- a/core/renderers/common/path_object.ts +++ b/core/renderers/common/path_object.ts @@ -62,7 +62,7 @@ export class PathObject implements IPathObject { /** The primary path of the block. */ this.svgPath = dom.createSvgElement( Svg.PATH, - {'class': 'blocklyPath'}, + {'class': 'blocklyPath', 'tabindex': '-1'}, this.svgRoot, ); From e5abf720c887c05ac93c6855199b1a85e547bcff Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 30 Apr 2025 00:21:52 +0000 Subject: [PATCH 28/43] fix: Remove CSS for active/passive focus. The CSS work here is far more extensive than should go in this or any of the structural branches. Instead, proper rendering will be done via the plugin and eventually merged into Core once finalized. --- core/css.ts | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/core/css.ts b/core/css.ts index 6ca262f3b25..ed47e8037ec 100644 --- a/core/css.ts +++ b/core/css.ts @@ -463,8 +463,8 @@ input[type=number] { } .blocklyMenuSeparator { - background-color: #ccc; - height: 1px; + background-color: #ccc; + height: 1px; border: 0; margin-left: 4px; margin-right: 4px; @@ -494,12 +494,4 @@ input[type=number] { cursor: grabbing; } -.blocklyActiveFocus { - outline-color: #2ae; - outline-width: 2px; -} -.blocklyPassiveFocus { - outline-color: #3fdfff; - outline-width: 1.5px; -} `; From 585c950f38428612b551761f92ca2acfb661da53 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 30 Apr 2025 00:41:26 +0000 Subject: [PATCH 29/43] feat: Make FlyoutButton focusable. --- core/flyout_button.ts | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/core/flyout_button.ts b/core/flyout_button.ts index 3b9b2fe0735..f24a3d3bc5f 100644 --- a/core/flyout_button.ts +++ b/core/flyout_button.ts @@ -15,6 +15,8 @@ import type {IASTNodeLocationSvg} from './blockly.js'; import * as browserEvents from './browser_events.js'; import * as Css from './css.js'; import type {IBoundedElement} from './interfaces/i_bounded_element.js'; +import type {IFocusableNode} from './interfaces/i_focusable_node.js'; +import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import type {IRenderedElement} from './interfaces/i_rendered_element.js'; import {Coordinate} from './utils/coordinate.js'; import * as dom from './utils/dom.js'; @@ -29,7 +31,11 @@ import type {WorkspaceSvg} from './workspace_svg.js'; * Class for a button or label in the flyout. */ export class FlyoutButton - implements IASTNodeLocationSvg, IBoundedElement, IRenderedElement + implements + IASTNodeLocationSvg, + IBoundedElement, + IRenderedElement, + IFocusableNode { /** The horizontal margin around the text in the button. */ static TEXT_MARGIN_X = 5; @@ -107,7 +113,7 @@ export class FlyoutButton this.svgGroup = dom.createSvgElement( Svg.G, - {'class': cssClass}, + {'class': cssClass, 'tabindex': '-1'}, this.workspace.getCanvas(), ); @@ -389,6 +395,22 @@ export class FlyoutButton getSvgRoot() { return this.svgGroup; } + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + return this.svgGroup; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this.workspace; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void {} + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void {} } /** CSS for buttons and labels. See css.js for use. */ From 151d8f433c6bfaf3fa06b418dbb4af596545feeb Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 30 Apr 2025 01:23:04 +0000 Subject: [PATCH 30/43] fix: Make RenderedConnection display correctly. This essentially makes the underlying highlight path permanent so that only its transform/d attributes need to be updated, plus its display property. --- core/rendered_connection.ts | 35 ++++++++++++++++++++------ core/renderers/common/drawer.ts | 13 ++++------ core/renderers/common/i_path_object.ts | 2 +- core/renderers/common/path_object.ts | 34 +++++++++++-------------- core/renderers/zelos/drawer.ts | 9 ++++--- 5 files changed, 53 insertions(+), 40 deletions(-) diff --git a/core/rendered_connection.ts b/core/rendered_connection.ts index 406b539b4e9..7ada1b9f6b7 100644 --- a/core/rendered_connection.ts +++ b/core/rendered_connection.ts @@ -326,13 +326,28 @@ export class RenderedConnection /** Add highlighting around this connection. */ highlight() { this.highlighted = true; - this.getSourceBlock().queueRender(); + + // Note that this needs to be done synchronously (vs. queuing a render pass) + // since only a displayed element can be focused, and this focusable node is + // implemented to make itself visible immediately prior to receiving DOM + // focus. It's expected that the connection's position should already be + // correct by this point (otherwise it will be corrected in a subsequent + // draw pass). + const highlightSvg = this.findHighlightSvg(); + if (highlightSvg) { + highlightSvg.style.display = ''; + } } /** Remove the highlighting around this connection. */ unhighlight() { this.highlighted = false; - this.getSourceBlock().queueRender(); + + // Note that this is done synchronously for parity with highlight(). + const highlightSvg = this.findHighlightSvg(); + if (highlightSvg) { + highlightSvg.style.display = 'none'; + } } /** Returns true if this connection is highlighted, false otherwise. */ @@ -635,11 +650,7 @@ export class RenderedConnection /** See IFocusableNode.getFocusableElement. */ getFocusableElement(): HTMLElement | SVGElement { - // This cast is valid as TypeScript's type definition is wrong. See: - // https://github.com/microsoft/TypeScript/issues/60996. - const highlightSvg = document.getElementById(this.id) as - | unknown - | null as SVGElement | null; + const highlightSvg = this.findHighlightSvg(); if (highlightSvg) return highlightSvg; throw new Error('No highlight SVG found corresponding to this connection.'); } @@ -651,10 +662,20 @@ export class RenderedConnection /** See IFocusableNode.onNodeFocus. */ onNodeFocus(): void { + this.highlight(); } /** See IFocusableNode.onNodeBlur. */ onNodeBlur(): void { + this.unhighlight(); + } + + private findHighlightSvg(): SVGElement | null { + // This cast is valid as TypeScript's definition is wrong. See: + // https://github.com/microsoft/TypeScript/issues/60996. + return document.getElementById(this.id) as + | unknown + | null as SVGElement | null; } } diff --git a/core/renderers/common/drawer.ts b/core/renderers/common/drawer.ts index 09320710c51..7046406adc7 100644 --- a/core/renderers/common/drawer.ts +++ b/core/renderers/common/drawer.ts @@ -435,19 +435,16 @@ export class Drawer { for (const elem of row.elements) { if (!(elem instanceof Connection)) continue; - if (elem.highlighted) { - this.drawConnectionHighlightPath(elem); - } else { - this.block_.pathObject.removeConnectionHighlight?.( - elem.connectionModel, - ); + const highlightSvg = this.drawConnectionHighlightPath(elem); + if (highlightSvg) { + highlightSvg.style.display = elem.highlighted ? '' : 'none'; } } } } /** Returns a path to highlight the given connection. */ - drawConnectionHighlightPath(measurable: Connection) { + drawConnectionHighlightPath(measurable: Connection): SVGElement | undefined { const conn = measurable.connectionModel; let path = ''; if ( @@ -459,7 +456,7 @@ export class Drawer { path = this.getStatementConnectionHighlightPath(measurable); } const block = conn.getSourceBlock(); - block.pathObject.addConnectionHighlight?.( + return block.pathObject.addConnectionHighlight?.( conn, path, conn.getOffsetInBlock(), diff --git a/core/renderers/common/i_path_object.ts b/core/renderers/common/i_path_object.ts index 699f1d92edb..776ba0067ea 100644 --- a/core/renderers/common/i_path_object.ts +++ b/core/renderers/common/i_path_object.ts @@ -113,7 +113,7 @@ export interface IPathObject { connectionPath: string, offset: Coordinate, rtl: boolean, - ): void; + ): SVGElement; /** * Apply the stored colours to the block's path, taking into account whether diff --git a/core/renderers/common/path_object.ts b/core/renderers/common/path_object.ts index fe7908b383f..ed2bb7dda75 100644 --- a/core/renderers/common/path_object.ts +++ b/core/renderers/common/path_object.ts @@ -268,12 +268,17 @@ export class PathObject implements IPathObject { connectionPath: string, offset: Coordinate, rtl: boolean, - ) { - if (this.connectionHighlights.has(connection)) { - if (this.currentHighlightMatchesNew(connection, connectionPath, offset)) { - return; - } - this.removeConnectionHighlight(connection); + ): SVGElement { + const transformation = + `translate(${offset.x}, ${offset.y})` + (rtl ? ' scale(-1 1)' : ''); + + const previousHighlight = this.connectionHighlights.get(connection); + if (previousHighlight) { + // Since a connection already exists, make sure that its path and + // transform are correct. + previousHighlight.setAttribute('d', connectionPath); + previousHighlight.setAttribute('transform', transformation); + return previousHighlight; } const highlight = dom.createSvgElement( @@ -281,26 +286,15 @@ export class PathObject implements IPathObject { { 'id': connection.id, 'class': 'blocklyHighlightedConnectionPath', + 'style': 'display: none;', 'tabindex': '-1', 'd': connectionPath, - 'transform': - `translate(${offset.x}, ${offset.y})` + (rtl ? ' scale(-1 1)' : ''), + 'transform': transformation, }, this.svgRoot, ); this.connectionHighlights.set(connection, highlight); - } - - private currentHighlightMatchesNew( - connection: RenderedConnection, - newPath: string, - newOffset: Coordinate, - ): boolean { - const currPath = this.connectionHighlights - .get(connection) - ?.getAttribute('d'); - const currOffset = this.highlightOffsets.get(connection); - return currPath === newPath && Coordinate.equals(currOffset, newOffset); + return highlight; } /** diff --git a/core/renderers/zelos/drawer.ts b/core/renderers/zelos/drawer.ts index 5cc52c0cbb2..b38711eb6c3 100644 --- a/core/renderers/zelos/drawer.ts +++ b/core/renderers/zelos/drawer.ts @@ -234,15 +234,16 @@ export class Drawer extends BaseDrawer { } /** Returns a path to highlight the given connection. */ - drawConnectionHighlightPath(measurable: Connection) { + override drawConnectionHighlightPath( + measurable: Connection, + ): SVGElement | undefined { const conn = measurable.connectionModel; if ( conn.type === ConnectionType.NEXT_STATEMENT || conn.type === ConnectionType.PREVIOUS_STATEMENT || (conn.type === ConnectionType.OUTPUT_VALUE && !measurable.isDynamicShape) ) { - super.drawConnectionHighlightPath(measurable); - return; + return super.drawConnectionHighlightPath(measurable); } let path = ''; @@ -261,7 +262,7 @@ export class Drawer extends BaseDrawer { (output.shape as DynamicShape).pathDown(output.height); } const block = conn.getSourceBlock(); - block.pathObject.addConnectionHighlight?.( + return block.pathObject.addConnectionHighlight?.( conn, path, conn.getOffsetInBlock(), From d6dcc4b42da30de1c03ef7fc1954587811b63b05 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 30 Apr 2025 01:24:32 +0000 Subject: [PATCH 31/43] fix: Actually make FlyoutButton focusable. It needed a unique ID and to be properly hooked up to node retrieval. --- core/flyout_button.ts | 7 ++++++- core/workspace_svg.ts | 15 ++++++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/core/flyout_button.ts b/core/flyout_button.ts index f24a3d3bc5f..21aa408e543 100644 --- a/core/flyout_button.ts +++ b/core/flyout_button.ts @@ -26,6 +26,7 @@ import * as style from './utils/style.js'; import {Svg} from './utils/svg.js'; import type * as toolbox from './utils/toolbox.js'; import type {WorkspaceSvg} from './workspace_svg.js'; +import {idGenerator} from './utils.js'; /** * Class for a button or label in the flyout. @@ -74,6 +75,9 @@ export class FlyoutButton */ cursorSvg: SVGElement | null = null; + /** The unique ID for this FlyoutButton. */ + private id: string; + /** * @param workspace The workspace in which to place this button. * @param targetWorkspace The flyout's target workspace. @@ -111,9 +115,10 @@ export class FlyoutButton cssClass += ' ' + this.cssClass; } + this.id = idGenerator.getNextUniqueId(); this.svgGroup = dom.createSvgElement( Svg.G, - {'class': cssClass, 'tabindex': '-1'}, + {'id': this.id, 'class': cssClass, 'tabindex': '-1'}, this.workspace.getCanvas(), ); diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index 5f4c57a5774..b99679db3fd 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -45,7 +45,7 @@ import type {IBoundedElement} from './interfaces/i_bounded_element.js'; import {IContextMenu} from './interfaces/i_contextmenu.js'; import type {IDragTarget} from './interfaces/i_drag_target.js'; import type {IFlyout} from './interfaces/i_flyout.js'; -import type {IFocusableNode} from './interfaces/i_focusable_node.js'; +import {isFocusableNode, type IFocusableNode} from './interfaces/i_focusable_node.js'; import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import type {IMetricsManager} from './interfaces/i_metrics_manager.js'; import type {IToolbox} from './interfaces/i_toolbox.js'; @@ -2693,6 +2693,19 @@ export class WorkspaceSvg /** See IFocusableTree.lookUpFocusableNode. */ lookUpFocusableNode(id: string): IFocusableNode | null { + // Check against flyout items if this workspace is part of a flyout. Note + // that blocks may match against this pass before reaching getBlockById() + // below (but only for a flyout workspace). + const flyout = this.targetWorkspace?.getFlyout(); + if (this.isFlyout && flyout) { + for (const flyoutItem of flyout.getContents()) { + const elem = flyoutItem.getElement(); + if (isFocusableNode(elem) && elem.getFocusableElement().id === id) { + return elem; + } + } + } + return this.getBlockById(id) as IFocusableNode; } From a9cf3d73990bd4e29ab24fc920057472d58f2f5c Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 30 Apr 2025 01:27:25 +0000 Subject: [PATCH 32/43] chore: lint fixes. --- core/flyout_button.ts | 2 +- core/workspace_svg.ts | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/core/flyout_button.ts b/core/flyout_button.ts index 21aa408e543..666a2e081bf 100644 --- a/core/flyout_button.ts +++ b/core/flyout_button.ts @@ -18,6 +18,7 @@ import type {IBoundedElement} from './interfaces/i_bounded_element.js'; import type {IFocusableNode} from './interfaces/i_focusable_node.js'; import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import type {IRenderedElement} from './interfaces/i_rendered_element.js'; +import {idGenerator} from './utils.js'; import {Coordinate} from './utils/coordinate.js'; import * as dom from './utils/dom.js'; import * as parsing from './utils/parsing.js'; @@ -26,7 +27,6 @@ import * as style from './utils/style.js'; import {Svg} from './utils/svg.js'; import type * as toolbox from './utils/toolbox.js'; import type {WorkspaceSvg} from './workspace_svg.js'; -import {idGenerator} from './utils.js'; /** * Class for a button or label in the flyout. diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index b99679db3fd..921fb72ecc7 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -45,7 +45,10 @@ import type {IBoundedElement} from './interfaces/i_bounded_element.js'; import {IContextMenu} from './interfaces/i_contextmenu.js'; import type {IDragTarget} from './interfaces/i_drag_target.js'; import type {IFlyout} from './interfaces/i_flyout.js'; -import {isFocusableNode, type IFocusableNode} from './interfaces/i_focusable_node.js'; +import { + isFocusableNode, + type IFocusableNode, +} from './interfaces/i_focusable_node.js'; import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import type {IMetricsManager} from './interfaces/i_metrics_manager.js'; import type {IToolbox} from './interfaces/i_toolbox.js'; From f18670afd714850c5b437c477bc3a0a0c0e1fe8c Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 30 Apr 2025 01:33:07 +0000 Subject: [PATCH 33/43] chore: Use strict equals. Addresses self-review comment. --- core/flyout_base.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/flyout_base.ts b/core/flyout_base.ts index d24ea2758a0..a0ba75eb9fe 100644 --- a/core/flyout_base.ts +++ b/core/flyout_base.ts @@ -1024,7 +1024,7 @@ export abstract class Flyout // not close the flyout. For anything else, do close it since the flyout is // no longer focused. if (toolbox && nextTree === toolbox) return; - if (nextTree == this.workspace_) return; + if (nextTree === this.workspace_) return; if (toolbox) toolbox.clearSelection(); this.autoHide(false); } From 34970cc5b7fdd222e2b3b75076d88bd5555a6874 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 30 Apr 2025 01:40:25 +0000 Subject: [PATCH 34/43] chore: Empty commit to re-trigger CI. From 3cf8fb8eed1295068708d4143878cf7f1229df70 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 30 Apr 2025 01:56:22 +0000 Subject: [PATCH 35/43] chore: Add field doc. Addresses self-review comment. --- core/field.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/core/field.ts b/core/field.ts index 1da60f18b81..a5e43a27665 100644 --- a/core/field.ts +++ b/core/field.ts @@ -195,6 +195,7 @@ export abstract class Field */ SERIALIZABLE = false; + /** The unique ID of this field. */ private id_: string | null = null; /** From d146f61141b9b637cd0ee22e4ddfe4b123bad2b6 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 30 Apr 2025 02:14:23 +0000 Subject: [PATCH 36/43] chore: Add field docs. This addresses a self-review comment. --- core/connection.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/core/connection.ts b/core/connection.ts index 5730fff996d..aed90e7c78c 100644 --- a/core/connection.ts +++ b/core/connection.ts @@ -56,6 +56,7 @@ export class Connection implements IASTNodeLocationWithBlock { /** DOM representation of a shadow block, or null if none. */ private shadowDom: Element | null = null; + /** The unique ID of this connection. */ id: string; /** From acf4b9e2a1c4b04f063ecd891721ab0a1d328ef5 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 30 Apr 2025 17:35:05 +0000 Subject: [PATCH 37/43] chore: lint fixes. --- core/blockly.ts | 4 +--- core/keyboard_nav/line_cursor.ts | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/core/blockly.ts b/core/blockly.ts index 94c4ca04132..069e21c81f6 100644 --- a/core/blockly.ts +++ b/core/blockly.ts @@ -111,9 +111,6 @@ import { ReturnEphemeralFocus, getFocusManager, } from './focus_manager.js'; -import { - FocusableTreeTraverser, -} from './utils/focusable_tree_traverser.js'; import {CodeGenerator} from './generator.js'; import {Gesture} from './gesture.js'; import {Grid} from './grid.js'; @@ -123,6 +120,7 @@ import * as inputs from './inputs.js'; import {IFlyoutInflater} from './interfaces/i_flyout_inflater.js'; import {LabelFlyoutInflater} from './label_flyout_inflater.js'; import {SeparatorFlyoutInflater} from './separator_flyout_inflater.js'; +import {FocusableTreeTraverser} from './utils/focusable_tree_traverser.js'; import {Input} from './inputs/input.js'; import {InsertionMarkerPreviewer} from './insertion_marker_previewer.js'; diff --git a/core/keyboard_nav/line_cursor.ts b/core/keyboard_nav/line_cursor.ts index 325cce17315..196a0854763 100644 --- a/core/keyboard_nav/line_cursor.ts +++ b/core/keyboard_nav/line_cursor.ts @@ -750,7 +750,7 @@ export class LineCursor extends Marker { const focused = getFocusManager().getFocusedNode(); if (focused instanceof BlockSvg) { - let block: BlockSvg | null = focused; + const block: BlockSvg | null = focused; if (block && block.workspace === this.workspace) { if (block.getRootBlock() === block && this.workspace.isFlyout) { // This block actually represents a stack. Note that this is needed From ae62af30f66f5b29bc32b34d6d83051164b440b7 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 1 May 2025 04:47:44 +0000 Subject: [PATCH 38/43] fix: FocusManager tracking issue + flyout refocus. This fixes an issue with FocusManager incorrectly missing that complete focus hsa been lost. It also attempts to fix an issue with the flyout automatically closing after creating a variable, though more work is still needed there. --- core/focus_manager.ts | 45 +++++++++++-------- core/variables.ts | 12 ++++- tests/mocha/focus_manager_test.js | 75 ++++++++++++++++++++++++------- 3 files changed, 98 insertions(+), 34 deletions(-) diff --git a/core/focus_manager.ts b/core/focus_manager.ts index a5104c7d96a..cb6b390957f 100644 --- a/core/focus_manager.ts +++ b/core/focus_manager.ts @@ -65,26 +65,15 @@ export class FocusManager { constructor( addGlobalEventListener: (type: string, listener: EventListener) => void, ) { - // Register root document focus listeners for tracking when focus leaves all - // tracked focusable trees. - addGlobalEventListener('focusin', (event) => { - if (!(event instanceof FocusEvent)) return; - - // The target that now has focus. - const activeElement = document.activeElement; + // Note that 'element' here is the element *gaining* focus. + const maybeFocus = (element: Element | EventTarget | null) => { let newNode: IFocusableNode | null | undefined = null; - if ( - activeElement instanceof HTMLElement || - activeElement instanceof SVGElement - ) { - // If the target losing focus maps to any tree, then it should be - // updated. Per the contract of findFocusableNodeFor only one tree - // should claim the element. + if (element instanceof HTMLElement || element instanceof SVGElement) { + // If the target losing or gaining focus maps to any tree, then it + // should be updated. Per the contract of findFocusableNodeFor only one + // tree should claim the element, so the search can be exited early. for (const tree of this.registeredTrees) { - newNode = FocusableTreeTraverser.findFocusableNodeFor( - activeElement, - tree, - ); + newNode = FocusableTreeTraverser.findFocusableNodeFor(element, tree); if (newNode) break; } } @@ -103,6 +92,26 @@ export class FocusManager { } else { this.defocusCurrentFocusedNode(); } + }; + + // Register root document focus listeners for tracking when focus leaves all + // tracked focusable trees. Note that focusin and focusout can be somewhat + // overlapping in the information that they provide. This is fine because + // they both aim to check for focus changes on the element gaining or having + // received focus, and maybeFocus should behave relatively deterministic. + addGlobalEventListener('focusin', (event) => { + if (!(event instanceof FocusEvent)) return; + + // When something receives focus, always use the current active element as + // the source of truth. + maybeFocus(document.activeElement); + }); + addGlobalEventListener('focusout', (event) => { + if (!(event instanceof FocusEvent)) return; + + // When something loses focus, it seems that document.activeElement may + // not necessarily be correct. Instead, use relatedTarget. + maybeFocus(event.relatedTarget); }); } diff --git a/core/variables.ts b/core/variables.ts index c896efd0f1a..4f0e1d62358 100644 --- a/core/variables.ts +++ b/core/variables.ts @@ -9,6 +9,8 @@ import type {Block} from './block.js'; import {Blocks} from './blocks.js'; import * as dialog from './dialog.js'; +import {Flyout} from './flyout_base.js'; +import {getFocusManager} from './focus_manager.js'; import {isLegacyProcedureDefBlock} from './interfaces/i_legacy_procedure_blocks.js'; import {isVariableBackedParameterModel} from './interfaces/i_variable_backed_parameter_model.js'; import {IVariableModel, IVariableState} from './interfaces/i_variable_model.js'; @@ -17,7 +19,7 @@ import * as deprecation from './utils/deprecation.js'; import type {BlockInfo, FlyoutItemInfo} from './utils/toolbox.js'; import * as utilsXml from './utils/xml.js'; import type {Workspace} from './workspace.js'; -import type {WorkspaceSvg} from './workspace_svg.js'; +import {WorkspaceSvg} from './workspace_svg.js'; /** * String for use in the "custom" attribute of a category in toolbox XML. @@ -417,6 +419,14 @@ export function createVariableButtonHandler( // No conflict workspace.createVariable(text, type); if (opt_callback) opt_callback(text); + // Make sure to restore focus to the Flyout since creating a variable + // recreates elements of the Flyout. + if (workspace instanceof WorkspaceSvg) { + const flyout = workspace.getFlyout(); + if (flyout && flyout instanceof Flyout) { + getFocusManager().focusTree(flyout.getWorkspace()); + } + } return; } diff --git a/tests/mocha/focus_manager_test.js b/tests/mocha/focus_manager_test.js index 69ecfe722a5..acb8e290caa 100644 --- a/tests/mocha/focus_manager_test.js +++ b/tests/mocha/focus_manager_test.js @@ -71,27 +71,27 @@ suite('FocusManager', function () { const ACTIVE_FOCUS_NODE_CSS_SELECTOR = `.${FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME}`; const PASSIVE_FOCUS_NODE_CSS_SELECTOR = `.${FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME}`; + const createFocusableTree = function (rootElementId, nestedTrees) { + return new FocusableTreeImpl( + document.getElementById(rootElementId), + nestedTrees || [], + ); + }; + const createFocusableNode = function (tree, elementId) { + return tree.addNode(document.getElementById(elementId)); + }; + setup(function () { sharedTestSetup.call(this); const testState = this; + testState.globalDocumentEventListeners = []; const addDocumentEventListener = function (type, listener) { - testState.globalDocumentEventListenerType = type; - testState.globalDocumentEventListener = listener; + testState.globalDocumentEventListeners.push({type, listener}); document.addEventListener(type, listener); }; this.focusManager = new FocusManager(addDocumentEventListener); - const createFocusableTree = function (rootElementId, nestedTrees) { - return new FocusableTreeImpl( - document.getElementById(rootElementId), - nestedTrees || [], - ); - }; - const createFocusableNode = function (tree, elementId) { - return tree.addNode(document.getElementById(elementId)); - }; - this.testFocusableTree1 = createFocusableTree('testFocusableTree1'); this.testFocusableTree1Node1 = createFocusableNode( this.testFocusableTree1, @@ -162,9 +162,12 @@ suite('FocusManager', function () { // Remove the globally registered listener from FocusManager to avoid state being shared across // test boundaries. - const eventType = this.globalDocumentEventListenerType; - const eventListener = this.globalDocumentEventListener; - document.removeEventListener(eventType, eventListener); + for (const registeredListener of this.globalDocumentEventListeners) { + const eventType = registeredListener.type; + const eventListener = registeredListener.listener; + document.removeEventListener(eventType, eventListener); + } + this.globalDocumentEventListeners = []; // Ensure all node CSS styles are reset so that state isn't leaked between tests. const activeElems = document.querySelectorAll( @@ -832,6 +835,27 @@ suite('FocusManager', function () { this.testFocusableNestedTree4Node1, ); }); + + test('deletion after focusNode() returns null', function () { + const rootElem = document.createElement('div'); + const nodeElem = document.createElement('div'); + rootElem.setAttribute('id', 'focusRoot'); + rootElem.setAttribute('tabindex', '-1'); + nodeElem.setAttribute('id', 'focusNode'); + nodeElem.setAttribute('tabindex', '-1'); + nodeElem.textContent = 'Focusable node'; + rootElem.appendChild(nodeElem); + document.body.appendChild(rootElem); + const root = createFocusableTree('focusRoot'); + const node = createFocusableNode(root, 'focusNode'); + this.focusManager.registerTree(root); + this.focusManager.focusNode(node); + + node.getFocusableElement().remove(); + + assert.notStrictEqual(this.focusManager.getFocusedNode(), node); + rootElem.remove(); // Cleanup. + }); }); suite('CSS classes', function () { test('registered tree focusTree()ed no prev focus root elem has active property', function () { @@ -1724,6 +1748,27 @@ suite('FocusManager', function () { this.testFocusableNestedTree4Node1, ); }); + + test('deletion after focus() returns null', function () { + const rootElem = document.createElement('div'); + const nodeElem = document.createElement('div'); + rootElem.setAttribute('id', 'focusRoot'); + rootElem.setAttribute('tabindex', '-1'); + nodeElem.setAttribute('id', 'focusNode'); + nodeElem.setAttribute('tabindex', '-1'); + nodeElem.textContent = 'Focusable node'; + rootElem.appendChild(nodeElem); + document.body.appendChild(rootElem); + const root = createFocusableTree('focusRoot'); + const node = createFocusableNode(root, 'focusNode'); + this.focusManager.registerTree(root); + document.getElementById('focusNode').focus(); + + node.getFocusableElement().remove(); + + assert.notStrictEqual(this.focusManager.getFocusedNode(), node); + rootElem.remove(); // Cleanup. + }); }); suite('CSS classes', function () { test('registered root focus()ed no prev focus returns root elem has active property', function () { From 766dd23fdd861a25d28529e8e36e56f39efd30e9 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 1 May 2025 18:02:27 +0000 Subject: [PATCH 39/43] fix: disable outlines for most things. These will be replaced with better styling in the keyboard navigation plugin. --- core/css.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/core/css.ts b/core/css.ts index ed47e8037ec..eeba6dbf14a 100644 --- a/core/css.ts +++ b/core/css.ts @@ -494,4 +494,14 @@ input[type=number] { cursor: grabbing; } +.blocklyActiveFocus:is( + .blocklyFlyout, + .blocklyWorkspace, + .blocklyField, + .blocklyPath, + .blocklyHighlightedConnectionPath +) { + outline-width: 0px; +} + `; From c78fc683e3d29fafd660c673cda28b869557fab3 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 1 May 2025 21:54:38 +0000 Subject: [PATCH 40/43] fix: Fix broken Mocha tests. This fixes rendering for connection tests, switches out a now-contrived output connection test, and revises FocusManager state such that a unique FocusManager is created for every individual test to avoid cross-test state bleeding. Focus state can still bleed, unfortunately, but FocusManager generally guarantees eventual consistency. --- core/focus_manager.ts | 29 +++++++++++++--------- tests/mocha/cursor_test.js | 19 +++++++------- tests/mocha/focus_manager_test.js | 17 +------------ tests/mocha/test_helpers/setup_teardown.js | 23 +++++++++++++++++ 4 files changed, 51 insertions(+), 37 deletions(-) diff --git a/core/focus_manager.ts b/core/focus_manager.ts index cb6b390957f..7091c4efb08 100644 --- a/core/focus_manager.ts +++ b/core/focus_manager.ts @@ -456,19 +456,24 @@ export class FocusManager { dom.removeClass(element, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME); dom.removeClass(element, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME); } -} -let focusManager: FocusManager | null = null; + private static focusManager: FocusManager | null = null; -/** - * Returns the page-global FocusManager. - * - * The returned instance is guaranteed to not change across function calls, but - * may change across page loads. - */ -export function getFocusManager(): FocusManager { - if (!focusManager) { - focusManager = new FocusManager(document.addEventListener); + /** + * Returns the page-global FocusManager. + * + * The returned instance is guaranteed to not change across function calls, but + * may change across page loads. + */ + static getFocusManager(): FocusManager { + if (!FocusManager.focusManager) { + FocusManager.focusManager = new FocusManager(document.addEventListener); + } + return FocusManager.focusManager; } - return focusManager; +} + +/** Convenience function for FocusManager.getFocusManager. */ +export function getFocusManager(): FocusManager { + return FocusManager.getFocusManager(); } diff --git a/tests/mocha/cursor_test.js b/tests/mocha/cursor_test.js index 53f0714da11..55595df7b05 100644 --- a/tests/mocha/cursor_test.js +++ b/tests/mocha/cursor_test.js @@ -6,6 +6,7 @@ import {ASTNode} from '../../build/src/core/keyboard_nav/ast_node.js'; import {assert} from '../../node_modules/chai/chai.js'; +import {createRenderedBlock} from './test_helpers/block_definitions.js'; import { sharedTestSetup, sharedTestTeardown, @@ -63,11 +64,11 @@ suite('Cursor', function () { ]); this.workspace = Blockly.inject('blocklyDiv', {}); this.cursor = this.workspace.getCursor(); - const blockA = this.workspace.newBlock('input_statement'); - const blockB = this.workspace.newBlock('input_statement'); - const blockC = this.workspace.newBlock('input_statement'); - const blockD = this.workspace.newBlock('input_statement'); - const blockE = this.workspace.newBlock('field_input'); + const blockA = createRenderedBlock(this.workspace, 'input_statement'); + const blockB = createRenderedBlock(this.workspace, 'input_statement'); + const blockC = createRenderedBlock(this.workspace, 'input_statement'); + const blockD = createRenderedBlock(this.workspace, 'input_statement'); + const blockE = createRenderedBlock(this.workspace, 'field_input'); blockA.nextConnection.connect(blockB.previousConnection); blockA.inputList[0].connection.connect(blockE.outputConnection); @@ -105,12 +106,12 @@ suite('Cursor', function () { ); }); - test('In - From output connection', function () { + test('In - From attached input connection', function () { const fieldBlock = this.blocks.E; - const outputNode = ASTNode.createConnectionNode( - fieldBlock.outputConnection, + const inputConnectionNode = ASTNode.createConnectionNode( + this.blocks.A.inputList[0].connection, ); - this.cursor.setCurNode(outputNode); + this.cursor.setCurNode(inputConnectionNode); this.cursor.in(); const curNode = this.cursor.getCurNode(); assert.equal(curNode.getLocation(), fieldBlock); diff --git a/tests/mocha/focus_manager_test.js b/tests/mocha/focus_manager_test.js index acb8e290caa..bcd74c1a03c 100644 --- a/tests/mocha/focus_manager_test.js +++ b/tests/mocha/focus_manager_test.js @@ -84,13 +84,7 @@ suite('FocusManager', function () { setup(function () { sharedTestSetup.call(this); - const testState = this; - testState.globalDocumentEventListeners = []; - const addDocumentEventListener = function (type, listener) { - testState.globalDocumentEventListeners.push({type, listener}); - document.addEventListener(type, listener); - }; - this.focusManager = new FocusManager(addDocumentEventListener); + this.focusManager = getFocusManager(); this.testFocusableTree1 = createFocusableTree('testFocusableTree1'); this.testFocusableTree1Node1 = createFocusableNode( @@ -160,15 +154,6 @@ suite('FocusManager', function () { teardown(function () { sharedTestTeardown.call(this); - // Remove the globally registered listener from FocusManager to avoid state being shared across - // test boundaries. - for (const registeredListener of this.globalDocumentEventListeners) { - const eventType = registeredListener.type; - const eventListener = registeredListener.listener; - document.removeEventListener(eventType, eventListener); - } - this.globalDocumentEventListeners = []; - // Ensure all node CSS styles are reset so that state isn't leaked between tests. const activeElems = document.querySelectorAll( ACTIVE_FOCUS_NODE_CSS_SELECTOR, diff --git a/tests/mocha/test_helpers/setup_teardown.js b/tests/mocha/test_helpers/setup_teardown.js index 2fc08cb6943..4f4134e052c 100644 --- a/tests/mocha/test_helpers/setup_teardown.js +++ b/tests/mocha/test_helpers/setup_teardown.js @@ -5,6 +5,7 @@ */ import * as eventUtils from '../../../build/src/core/events/utils.js'; +import {FocusManager} from '../../../build/src/core/focus_manager.js'; /** * Safely disposes of Blockly workspace, logging any errors. @@ -124,6 +125,18 @@ export function sharedTestSetup(options = {}) { }; this.blockTypesCleanup_ = this.sharedCleanup.blockTypesCleanup_; this.messagesCleanup_ = this.sharedCleanup.messagesCleanup_; + + // Set up FocusManager to run in isolation for this test. + this.globalDocumentEventListeners = []; + const testState = this; + const addDocumentEventListener = function (type, listener) { + testState.globalDocumentEventListeners.push({type, listener}); + document.addEventListener(type, listener); + }; + const specificFocusManager = new FocusManager(addDocumentEventListener); + this.oldGetFocusManager = FocusManager.getFocusManager; + FocusManager.getFocusManager = () => specificFocusManager; + wrapDefineBlocksWithJsonArrayWithCleanup_(this.sharedCleanup); return { clock: this.clock, @@ -142,6 +155,16 @@ export function sharedTestTeardown() { } try { + // Remove the globally registered listener from FocusManager to avoid state + // being shared across test boundaries. + for (const registeredListener of this.globalDocumentEventListeners) { + const eventType = registeredListener.type; + const eventListener = registeredListener.listener; + document.removeEventListener(eventType, eventListener); + } + this.globalDocumentEventListeners = []; + FocusManager.getFocusManager = this.oldGetFocusManager; + if (this.workspace) { workspaceTeardown.call(this, this.workspace); this.workspace = null; From 1f3b90394e9ac662c326be42c8b5540452d57b84 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 1 May 2025 22:26:51 +0000 Subject: [PATCH 41/43] fix: Fix deiniting order for tests. Workspace clean-up must happen before resetting FocusManager otherwise it can't be properly unregistered. --- tests/mocha/test_helpers/setup_teardown.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/mocha/test_helpers/setup_teardown.js b/tests/mocha/test_helpers/setup_teardown.js index 4f4134e052c..b0d7c83c697 100644 --- a/tests/mocha/test_helpers/setup_teardown.js +++ b/tests/mocha/test_helpers/setup_teardown.js @@ -155,16 +155,6 @@ export function sharedTestTeardown() { } try { - // Remove the globally registered listener from FocusManager to avoid state - // being shared across test boundaries. - for (const registeredListener of this.globalDocumentEventListeners) { - const eventType = registeredListener.type; - const eventListener = registeredListener.listener; - document.removeEventListener(eventType, eventListener); - } - this.globalDocumentEventListeners = []; - FocusManager.getFocusManager = this.oldGetFocusManager; - if (this.workspace) { workspaceTeardown.call(this, this.workspace); this.workspace = null; @@ -207,6 +197,16 @@ export function sharedTestTeardown() { } Blockly.WidgetDiv.testOnly_setDiv(null); + + // Remove the globally registered listener from FocusManager to avoid state + // being shared across test boundaries. + for (const registeredListener of this.globalDocumentEventListeners) { + const eventType = registeredListener.type; + const eventListener = registeredListener.listener; + document.removeEventListener(eventType, eventListener); + } + this.globalDocumentEventListeners = []; + FocusManager.getFocusManager = this.oldGetFocusManager; } } From cc2a879ab8584f0e5ac90587ddd769930b15d6e7 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Fri, 2 May 2025 00:13:04 +0000 Subject: [PATCH 42/43] fix: remove variables flyout focusing. Besides the fact that this wasn't a complete solution, switching the workspace import to be non-type caused a circular dependency which caused the advanced compilation tests to fail. --- core/variables.ts | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/core/variables.ts b/core/variables.ts index 4f0e1d62358..c896efd0f1a 100644 --- a/core/variables.ts +++ b/core/variables.ts @@ -9,8 +9,6 @@ import type {Block} from './block.js'; import {Blocks} from './blocks.js'; import * as dialog from './dialog.js'; -import {Flyout} from './flyout_base.js'; -import {getFocusManager} from './focus_manager.js'; import {isLegacyProcedureDefBlock} from './interfaces/i_legacy_procedure_blocks.js'; import {isVariableBackedParameterModel} from './interfaces/i_variable_backed_parameter_model.js'; import {IVariableModel, IVariableState} from './interfaces/i_variable_model.js'; @@ -19,7 +17,7 @@ import * as deprecation from './utils/deprecation.js'; import type {BlockInfo, FlyoutItemInfo} from './utils/toolbox.js'; import * as utilsXml from './utils/xml.js'; import type {Workspace} from './workspace.js'; -import {WorkspaceSvg} from './workspace_svg.js'; +import type {WorkspaceSvg} from './workspace_svg.js'; /** * String for use in the "custom" attribute of a category in toolbox XML. @@ -419,14 +417,6 @@ export function createVariableButtonHandler( // No conflict workspace.createVariable(text, type); if (opt_callback) opt_callback(text); - // Make sure to restore focus to the Flyout since creating a variable - // recreates elements of the Flyout. - if (workspace instanceof WorkspaceSvg) { - const flyout = workspace.getFlyout(); - if (flyout && flyout instanceof Flyout) { - getFocusManager().focusTree(flyout.getWorkspace()); - } - } return; } From d1ffabe8b66b787820fc8e3f2c880add8404bf12 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Fri, 2 May 2025 00:17:31 +0000 Subject: [PATCH 43/43] chore: empty commit to re-trigger CI