From 0ed9db2b1a87ace13135c5377c84a886917bbf1c Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 9 Apr 2025 23:03:36 +0000 Subject: [PATCH 01/33] feat!: initial integration of FocusableTree/Node. Much more work is needed, and this is just a staged example of where things are moving. The actual introduction of FocusableTree and FocusableNode implementations will happen in isolation, but this is exploring other changes that may be needed in order to bring FocusManager to the keyboard nav plugin with current functional parity. --- core/block_svg.ts | 15 +++- core/blockly.ts | 6 +- core/css.ts | 10 +-- core/field.ts | 33 ++++++++- core/flyout_base.ts | 46 +++++++++++- core/focus_manager.ts | 7 +- core/interfaces/i_flyout.ts | 3 +- core/interfaces/i_focusable_node.ts | 4 ++ core/interfaces/i_focusable_tree.ts | 4 ++ core/interfaces/i_toolbox.ts | 3 +- core/interfaces/i_toolbox_item.ts | 4 +- core/keyboard_nav/line_cursor.ts | 106 ++++++++++++++++++---------- core/rendered_connection.ts | 60 +++++++++++++++- core/renderers/common/marker_svg.ts | 17 ++--- core/toolbox/category.ts | 2 + core/toolbox/separator.ts | 2 + core/toolbox/toolbox.ts | 53 ++++++++++++-- core/toolbox/toolbox_item.ts | 16 +++++ core/workspace_svg.ts | 63 ++++++++++++++++- 19 files changed, 384 insertions(+), 70 deletions(-) diff --git a/core/block_svg.ts b/core/block_svg.ts index 1b76ed3f1c4..83b39f66457 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -43,6 +43,8 @@ import type {IASTNodeLocationSvg} from './interfaces/i_ast_node_location_svg.js' import type {IBoundedElement} from './interfaces/i_bounded_element.js'; import type {ICopyable} from './interfaces/i_copyable.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'; @@ -74,7 +76,8 @@ export class BlockSvg IBoundedElement, ICopyable, IDraggable, - IDeletable + IDeletable, + IFocusableNode { /** * Constant for identifying rows that are to be rendered inline. @@ -189,7 +192,7 @@ export class BlockSvg throw TypeError('Cannot create a rendered block in a headless workspace'); } this.workspace = workspace; - this.svgGroup = dom.createSvgElement(Svg.G, {}); + this.svgGroup = dom.createSvgElement(Svg.G, {'tabindex': '-1', 'id': this.id}); if (prototypeName) { dom.addClass(this.svgGroup, prototypeName); @@ -1767,4 +1770,12 @@ export class BlockSvg ); } } + + getFocusableElement(): HTMLElement | SVGElement { + return this.svgGroup; + } + + getFocusableTree(): IFocusableTree { + return this.workspace; + } } diff --git a/core/blockly.ts b/core/blockly.ts index 46ea1fcaf43..588f299dc35 100644 --- a/core/blockly.ts +++ b/core/blockly.ts @@ -145,8 +145,8 @@ import { } from './interfaces/i_draggable.js'; import {IDragger} from './interfaces/i_dragger.js'; import {IFlyout} from './interfaces/i_flyout.js'; -import {IFocusableNode} from './interfaces/i_focusable_node.js'; -import {IFocusableTree} from './interfaces/i_focusable_tree.js'; +import {IFocusableNode, isFocusableNode} from './interfaces/i_focusable_node.js'; +import {IFocusableTree, isFocusableTree} from './interfaces/i_focusable_tree.js'; import {IHasBubble, hasBubble} from './interfaces/i_has_bubble.js'; import {IIcon, isIcon} from './interfaces/i_icon.js'; import {IKeyboardAccessible} from './interfaces/i_keyboard_accessible.js'; @@ -619,6 +619,8 @@ export { isCopyable, isDeletable, isDraggable, + isFocusableNode, + isFocusableTree, isIcon, isObservable, isPaster, diff --git a/core/css.ts b/core/css.ts index 0502edbf13a..edd31fbda68 100644 --- a/core/css.ts +++ b/core/css.ts @@ -496,11 +496,13 @@ input[type=number] { } .blocklyActiveFocus { - outline-color: #2ae; - outline-width: 2px; + stroke: #2ae; + stroke-width: 2px; + fill: #0f0; } .blocklyPassiveFocus { - outline-color: #3fdfff; - outline-width: 1.5px; + stroke: #3fdfff; + stroke-width: 1.5px; + fill: #00f; } `; diff --git a/core/field.ts b/core/field.ts index 725a2867d9e..89004f9dc0d 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,12 @@ 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 + }); if (!this.isVisible()) { this.fieldGroup_.style.display = 'none'; } @@ -1401,6 +1413,21 @@ export abstract class Field } } + getFocusableElement(): HTMLElement | SVGElement { + if (!this.fieldGroup_) { + throw Error("This field currently has no representative DOM element."); + } + return this.fieldGroup_; + } + + getFocusableTree(): IFocusableTree { + const block = this.getSourceBlock(); + if (!block) { + throw new UnattachedFieldError(); + } + return block.workspace as WorkspaceSvg; + } + /** * Subclasses should reimplement this method to construct their Field * subclass from a JSON arg object. diff --git a/core/flyout_base.ts b/core/flyout_base.ts index e738470a606..1a1da7ffec3 100644 --- a/core/flyout_base.ts +++ b/core/flyout_base.ts @@ -12,6 +12,8 @@ // Former goog.module ID: Blockly.Flyout import {BlockSvg} from './block_svg.js'; +import { IFocusableNode, isFocusableNode } from './interfaces/i_focusable_node.js'; +import { IFocusableTree } from './interfaces/i_focusable_tree.js'; import * as browserEvents from './browser_events.js'; import {ComponentManager} from './component_manager.js'; import {DeleteArea} from './delete_area.js'; @@ -32,18 +34,20 @@ import {SEPARATOR_TYPE} from './separator_flyout_inflater.js'; import * as blocks from './serialization/blocks.js'; import {Coordinate} from './utils/coordinate.js'; import * as dom from './utils/dom.js'; +import { FocusableTreeTraverser } from './utils/focusable_tree_traverser.js'; import * as idGenerator from './utils/idgenerator.js'; import {Svg} from './utils/svg.js'; import * as toolbox from './utils/toolbox.js'; import * as Variables from './variables.js'; import {WorkspaceSvg} from './workspace_svg.js'; +import { getFocusManager } from './focus_manager.js'; /** * Class for a flyout. */ export abstract class Flyout extends DeleteArea - implements IAutoHideable, IFlyout + implements IAutoHideable, IFlyout, IFocusableNode { /** * Position the flyout. @@ -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,40 @@ export abstract class Flyout return null; } + + getFocusableElement(): HTMLElement | SVGElement { + if (!this.svgGroup_) { + throw new Error("Flyout has no DOM created."); + } + return this.svgGroup_; + } + + getFocusableTree(): IFocusableTree { + return this; + } + + getFocusedNode(): IFocusableNode | null { + return FocusableTreeTraverser.findFocusedNode(this); + } + + getRootFocusableNode(): IFocusableNode { + return this; + } + + getNestedTrees(): Array { + return []; + } + + lookUpFocusableNode(id: string): IFocusableNode | null { + return this.getContents() + .filter((item) => item instanceof isFocusableNode) + .map((item) => item as unknown as IFocusableNode) + .find((node) => node.getFocusableElement().id == id) ?? null; + } + + findFocusableNodeFor( + element: HTMLElement | SVGElement, + ): IFocusableNode | null { + return FocusableTreeTraverser.findFocusableNodeFor(element, this); + } } diff --git a/core/focus_manager.ts b/core/focus_manager.ts index c1fc295b991..a8a1a03db17 100644 --- a/core/focus_manager.ts +++ b/core/focus_manager.ts @@ -60,6 +60,8 @@ export class FocusManager { registeredTrees: Array = []; private currentlyHoldsEphemeralFocus: boolean = false; + // TODO: Figure out why this is needed, and how to test it. + private ignoreFocusChanges: boolean = false; constructor( addGlobalEventListener: (type: string, listener: EventListener) => void, @@ -68,6 +70,7 @@ export class FocusManager { // tracked focusable trees. addGlobalEventListener('focusin', (event) => { if (!(event instanceof FocusEvent)) return; + if (this.ignoreFocusChanges) return; // The target that now has focus. const activeElement = document.activeElement; @@ -205,8 +208,7 @@ export class FocusManager { * Any previously focused node will be updated to be passively highlighted (if * it's in a different focusable tree) or blurred (if it's in the same one). * - * @param focusableNode The node that should receive active - * focus. + * @param focusableNode The node that should receive active focus. */ focusNode(focusableNode: IFocusableNode): void { const nextTree = focusableNode.getFocusableTree(); @@ -302,6 +304,7 @@ export class FocusManager { dom.addClass(element, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME); dom.removeClass(element, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME); element.focus(); + // this.ignoreFocusChanges = false; } private setNodeToPassive(node: IFocusableNode): void { diff --git a/core/interfaces/i_flyout.ts b/core/interfaces/i_flyout.ts index 42204775ece..0e3e8e33938 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_focusable_node.ts b/core/interfaces/i_focusable_node.ts index 14100d44c7f..d238dc80cd5 100644 --- a/core/interfaces/i_focusable_node.ts +++ b/core/interfaces/i_focusable_node.ts @@ -37,3 +37,7 @@ export interface IFocusableNode { */ getFocusableTree(): IFocusableTree; } + +export function isFocusableNode(object: any | null): object is IFocusableNode { + return object && 'getFocusableElement' in object && 'getFocusableTree' in object; +} diff --git a/core/interfaces/i_focusable_tree.ts b/core/interfaces/i_focusable_tree.ts index bc0c38849c8..0aa55c07053 100644 --- a/core/interfaces/i_focusable_tree.ts +++ b/core/interfaces/i_focusable_tree.ts @@ -59,3 +59,7 @@ export interface IFocusableTree { */ lookUpFocusableNode(id: string): IFocusableNode | null; } + +export function isFocusableTree(object: any | null): object is IFocusableTree { + return object && 'getFocusedNode' in object && 'getRootFocusableNode' in object && 'getNestedTrees' in object && 'lookUpFocusableNode' in object && 'findFocusableNodeFor' in object; +} 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/keyboard_nav/line_cursor.ts b/core/keyboard_nav/line_cursor.ts index cf5317f0c83..610ee5698c8 100644 --- a/core/keyboard_nav/line_cursor.ts +++ b/core/keyboard_nav/line_cursor.ts @@ -31,6 +31,8 @@ import * as dom from '../utils/dom.js'; import type {WorkspaceSvg} from '../workspace_svg.js'; import {ASTNode} from './ast_node.js'; import {Marker} from './marker.js'; +import {isFocusableNode} from '../interfaces/i_focusable_node.js'; +import {getFocusManager} from '../focus_manager.js'; /** Options object for LineCursor instances. */ export interface CursorOptions { @@ -530,13 +532,39 @@ export class LineCursor extends Marker { * @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, selectionUpToDate = false) { + // if (!selectionUpToDate) { + // this.updateSelectionFromNode(newNode); + // } super.setCurNode(newNode); + // If old node was a block, unselect it or remove fake selection. + // if (oldNode?.getType() === ASTNode.types.BLOCK) { + // const block = oldNode.getLocation() as Blockly.BlockSvg; + // if (!block.isShadow()) { + // Blockly.common.setSelected(null); + // } else { + // block.removeSelect(); + // } + // } + + // this.drawMarker(oldNode, newNode); + + const newNodeLocation = newNode?.getLocation(); + console.trace('@@@@@@', 'setCurNode -> focus node:', newNodeLocation); + if (isFocusableNode(newNodeLocation)) { + getFocusManager().focusNode(newNodeLocation); + } else console.log('Error: node is not focusable:', newNodeLocation); + // if (newNode?.getType() === ASTNode.types.BLOCK) { + // const block = newNode.getLocation() as Blockly.BlockSvg; + // if (!block.isShadow()) { + // Blockly.common.setSelected(block); + // } else { + // block.addSelect(); + // } + // } + // Try to scroll cursor into view. if (newNode?.getType() === ASTNode.types.BLOCK) { const block = newNode.getLocation() as BlockSvg; @@ -577,47 +605,49 @@ export class LineCursor extends Marker { realDrawer: MarkerSvg, ) { // If old node was a block, unselect it or remove fake selection. - if (oldNode?.getType() === ASTNode.types.BLOCK) { - const block = oldNode.getLocation() as BlockSvg; - if (!block.isShadow()) { - // Selection should already be in sync. - } else { - block.removeSelect(); - } - } - - if (this.isZelos && oldNode && this.isValueInputConnection(oldNode)) { - this.hideAtInput(oldNode); - } + // if (oldNode?.getType() === ASTNode.types.BLOCK) { + // const block = oldNode.getLocation() as BlockSvg; + // if (!block.isShadow()) { + // // Selection should already be in sync. + // } else { + // block.removeSelect(); + // } + // } + + // if (this.isZelos && oldNode && this.isValueInputConnection(oldNode)) { + // this.hideAtInput(oldNode); + // } const curNodeType = curNode?.getType(); const isZelosInputConnection = this.isZelos && curNode && this.isValueInputConnection(curNode); + realDrawer.draw(oldNode, curNode); + // If drawing can't be handled locally, just use the drawer. if (curNodeType !== ASTNode.types.BLOCK && !isZelosInputConnection) { - realDrawer.draw(oldNode, curNode); + // realDrawer.draw(oldNode, curNode); return; } // Hide any visible marker SVG and instead do some manual rendering. realDrawer.hide(); - if (isZelosInputConnection) { - this.showAtInput(curNode); - } else if (curNode && curNodeType === ASTNode.types.BLOCK) { - const block = curNode.getLocation() as BlockSvg; - if (!block.isShadow()) { - // Selection should already be in sync. - } else { - block.addSelect(); - block.getParent()?.removeSelect(); - } - } + // if (isZelosInputConnection) { + // this.showAtInput(curNode); + // } else if (curNode && curNodeType === ASTNode.types.BLOCK) { + // const block = curNode.getLocation() as BlockSvg; + // if (!block.isShadow()) { + // // Selection should already be in sync. + // } else { + // block.addSelect(); + // block.getParent()?.removeSelect(); + // } + // } // Call MarkerSvg.prototype.fireMarkerEvent like // MarkerSvg.prototype.draw would (even though it's private). - (realDrawer as any)?.fireMarkerEvent?.(oldNode, curNode); + // (realDrawer as any)?.fireMarkerEvent?.(oldNode, curNode); } /** @@ -701,18 +731,20 @@ export class LineCursor extends Marker { */ private updateCurNodeFromSelection() { const curNode = super.getCurNode(); - const selected = common.getSelected(); + const focused = getFocusManager().getFocusedNode(); + // const selected = common.getSelected(); - if (selected === null && curNode?.getType() === ASTNode.types.BLOCK) { + if (focused === null && curNode?.getType() === ASTNode.types.BLOCK) { this.setCurNode(null, false); return; } - if (selected?.workspace !== this.workspace) { - return; - } - if (selected instanceof BlockSvg) { - let block: BlockSvg | null = selected; - if (selected.isShadow()) { + if (focused instanceof BlockSvg) { + let block: BlockSvg | null = focused; + + if (block?.workspace !== this.workspace) { + return; + } + if (block.isShadow()) { // OK if the current node is on the parent OR the shadow block. // The former happens for clicks, the latter for keyboard nav. if ( diff --git a/core/rendered_connection.ts b/core/rendered_connection.ts index c1d97dcddee..1bcd95c0245 100644 --- a/core/rendered_connection.ts +++ b/core/rendered_connection.ts @@ -17,11 +17,18 @@ 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 eventUtils from './events/utils.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'; /** Maximum randomness in workspace units for bumping a block. */ const BUMP_RANDOMNESS = 10; @@ -29,7 +36,7 @@ const BUMP_RANDOMNESS = 10; /** * Class for a connection between blocks that may be rendered on screen. */ -export class RenderedConnection extends Connection { +export class RenderedConnection extends Connection implements IFocusableNode { // TODO(b/109816955): remove '!', see go/strict-prop-init-fix. sourceBlock_!: BlockSvg; private readonly db: ConnectionDB; @@ -37,6 +44,9 @@ export class RenderedConnection extends Connection { private readonly offsetInBlock: Coordinate; private trackedState: TrackedState; private highlighted: boolean = false; + private constants: ConstantProvider; + private svgGroup: SVGElement | null = null; + private svgPath: SVGElement | null = null; /** Connection this connection connects to. Null if not connected. */ override targetConnection: RenderedConnection | null = null; @@ -66,6 +76,8 @@ export class RenderedConnection extends Connection { /** Describes the state of this connection's tracked-ness. */ this.trackedState = RenderedConnection.TrackedState.WILL_TRACK; + + this.constants = (source.workspace as WorkspaceSvg).getRenderer().getConstants(); } /** @@ -554,6 +566,41 @@ export class RenderedConnection extends Connection { const visible = parentInput.isVisible(); childBlock.getSvgRoot().style.display = visible ? 'block' : 'none'; } + + 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)' : ''), + ); } /** @@ -588,6 +635,17 @@ export class RenderedConnection extends Connection { this.sourceBlock_.queueRender(); return this; } + + getFocusableElement(): HTMLElement | SVGElement { + if (!this.svgGroup) { + throw Error("This connection hasn't been connected."); + } + return this.svgGroup; + } + + getFocusableTree(): IFocusableTree { + return this.getSourceBlock().workspace as WorkspaceSvg; + } } export namespace RenderedConnection { diff --git a/core/renderers/common/marker_svg.ts b/core/renderers/common/marker_svg.ts index 4805e70400a..5bd703ee31d 100644 --- a/core/renderers/common/marker_svg.ts +++ b/core/renderers/common/marker_svg.ts @@ -172,16 +172,16 @@ export class MarkerSvg { this.showAtLocation_(curNode); - this.fireMarkerEvent(oldNode, curNode); + // this.fireMarkerEvent(oldNode, curNode); // Ensures the marker will be visible immediately after the move. - const animate = this.currentMarkerSvg!.childNodes[0]; - if ( - animate !== undefined && - (animate as SVGAnimationElement).beginElement - ) { - (animate as SVGAnimationElement).beginElement(); - } + // const animate = this.currentMarkerSvg!.childNodes[0]; + // if ( + // animate !== undefined && + // (animate as SVGAnimationElement).beginElement + // ) { + // (animate as SVGAnimationElement).beginElement(); + // } } /** @@ -192,6 +192,7 @@ export class MarkerSvg { protected showAtLocation_(curNode: ASTNode) { const curNodeAsConnection = curNode.getLocation() as Connection; const connectionType = curNodeAsConnection.type; + console.log('@@@@@@ show marker at location:', curNode.getType()); if (curNode.getType() === ASTNode.types.BLOCK) { this.showWithBlock_(curNode); } else if (curNode.getType() === ASTNode.types.OUTPUT) { 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..b83129aace5 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'; @@ -39,6 +42,7 @@ import type {KeyboardShortcut} from '../shortcut_registry.js'; import * as Touch from '../touch.js'; import * as aria from '../utils/aria.js'; import * as dom from '../utils/dom.js'; +import {FocusableTreeTraverser} from '../utils/focusable_tree_traverser.js'; import {Rect} from '../utils/rect.js'; import * as toolbox from '../utils/toolbox.js'; import type {WorkspaceSvg} from '../workspace_svg.js'; @@ -51,7 +55,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 +172,7 @@ export class Toolbox ComponentManager.Capability.DRAG_TARGET, ], }); + getFocusManager().registerTree(this); } /** @@ -969,7 +979,7 @@ export class Toolbox * * @returns True if a parent category was selected, false otherwise. */ - private selectParent(): boolean { + selectParent(): boolean { if (!this.selectedItem_) { return false; } @@ -997,7 +1007,7 @@ export class Toolbox * * @returns True if a child category was selected, false otherwise. */ - private selectChild(): boolean { + selectChild(): boolean { if (!this.selectedItem_ || !this.selectedItem_.isCollapsible()) { return false; } @@ -1016,7 +1026,7 @@ export class Toolbox * * @returns True if a next category was selected, false otherwise. */ - private selectNext(): boolean { + selectNext(): boolean { if (!this.selectedItem_) { return false; } @@ -1041,7 +1051,7 @@ export class Toolbox * * @returns True if a previous category was selected, false otherwise. */ - private selectPrevious(): boolean { + selectPrevious(): boolean { if (!this.selectedItem_) { return false; } @@ -1077,6 +1087,39 @@ export class Toolbox this.workspace_.getThemeManager().unsubscribe(this.HtmlDiv); dom.removeNode(this.HtmlDiv); } + + getFocusManager().unregisterTree(this); + } + + getFocusableElement(): HTMLElement | SVGElement { + if (!this.HtmlDiv) throw Error('Toolbox DOM has not yet been created.'); + return this.HtmlDiv; + } + + getFocusableTree(): IFocusableTree { + return this; + } + + getFocusedNode(): IFocusableNode | null { + return FocusableTreeTraverser.findFocusedNode(this); + } + + getRootFocusableNode(): IFocusableNode { + return this; + } + + getNestedTrees(): Array { + if (this.flyout) return [this.flyout]; else return []; + } + + lookUpFocusableNode(id: string): IFocusableNode | null { + return this.getToolboxItemById(id) as IFocusableNode; + } + + findFocusableNodeFor( + element: HTMLElement | SVGElement, + ): IFocusableNode | null { + return FocusableTreeTraverser.findFocusableNodeFor(element, this); } } diff --git a/core/toolbox/toolbox_item.ts b/core/toolbox/toolbox_item.ts index ef9d979ab43..2e538f31478 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,20 @@ export class ToolboxItem implements IToolboxItem { * @param _isVisible True if category should be visible. */ setVisible_(_isVisible: boolean) {} + + 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; + } + + getFocusableTree(): IFocusableTree { + return this.parentToolbox_; + } } // nop by default diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index 68a1bd939fd..569a0cc120e 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -37,12 +37,15 @@ 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'; import type {IBoundedElement} from './interfaces/i_bounded_element.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 { @@ -69,6 +72,7 @@ import * as arrayUtils from './utils/array.js'; import {Coordinate} from './utils/coordinate.js'; import * as dom from './utils/dom.js'; import * as drag from './utils/drag.js'; +import {FocusableTreeTraverser} from './utils/focusable_tree_traverser.js'; import type {Metrics} from './utils/metrics.js'; import {Rect} from './utils/rect.js'; import {Size} from './utils/size.js'; @@ -82,6 +86,7 @@ import * as WidgetDiv from './widgetdiv.js'; import {Workspace} from './workspace.js'; import {WorkspaceAudio} from './workspace_audio.js'; import {ZoomControls} from './zoom_controls.js'; +import type {Field} from './field.js'; /** Margin around the top/bottom/left/right after a zoomToFit call. */ const ZOOM_TO_FIT_MARGIN = 20; @@ -90,7 +95,10 @@ const ZOOM_TO_FIT_MARGIN = 20; * Class for a workspace. This is an onscreen area with optional trashcan, * scrollbars, bubbles, and dragging. */ -export class WorkspaceSvg extends Workspace implements IASTNodeLocationSvg { +export class WorkspaceSvg + extends Workspace + implements IASTNodeLocationSvg, IFocusableNode, IFocusableTree +{ /** * A wrapper function called when a resize event occurs. * You can pass the result to `eventHandling.unbind`. @@ -760,7 +768,11 @@ export class WorkspaceSvg extends Workspace implements IASTNodeLocationSvg { * * */ - this.svgGroup_ = dom.createSvgElement(Svg.G, {'class': 'blocklyWorkspace'}); + this.svgGroup_ = dom.createSvgElement(Svg.G, { + 'class': 'blocklyWorkspace', + 'tabindex': '-1', + 'id': this.id, + }); // 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 @@ -836,6 +848,9 @@ export class WorkspaceSvg extends Workspace implements IASTNodeLocationSvg { this.getTheme(), isParentWorkspace ? this.getInjectionDiv() : undefined, ); + + getFocusManager().registerTree(this); + return this.svgGroup_; } @@ -920,6 +935,8 @@ export class WorkspaceSvg extends Workspace implements IASTNodeLocationSvg { document.body.removeEventListener('wheel', this.dummyWheelListener); this.dummyWheelListener = null; } + + getFocusManager().unregisterTree(this); } /** @@ -2606,6 +2623,48 @@ export class WorkspaceSvg extends Workspace implements IASTNodeLocationSvg { deltaY *= scale; this.scroll(this.scrollX + deltaX, this.scrollY + deltaY); } + + getFocusableElement(): HTMLElement | SVGElement { + return this.svgGroup_; + } + + getFocusableTree(): IFocusableTree { + return this; + } + + getFocusedNode(): IFocusableNode | null { + return FocusableTreeTraverser.findFocusedNode(this); + } + + getRootFocusableNode(): IFocusableNode { + return this; + } + + getNestedTrees(): Array { + return []; + } + + lookUpFocusableNode(id: string): IFocusableNode | null { + // TODO: This isn't a complete solution since non-blocks can have focus. + const fieldIndicator = '_field_'; + const fieldIndicatorIndex = id.indexOf(fieldIndicator); + if (fieldIndicatorIndex !== -1) { + const blockId = id.substring(0, fieldIndicatorIndex); + const block = this.getBlockById(blockId); + if (block != null) { + for (const field of block.getFields()) { + if (field.getFocusableElement().id === id) return field; + } + } + return null; + } else return this.getBlockById(id) as IFocusableNode; + } + + findFocusableNodeFor( + element: HTMLElement | SVGElement, + ): IFocusableNode | null { + return FocusableTreeTraverser.findFocusableNodeFor(element, this); + } } /** From 45b1b50e02d6ff22ed88ae51187668945f85deee Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Fri, 11 Apr 2025 01:44:42 +0000 Subject: [PATCH 02/33] feat: bunch more focus fixes. This change largely fixes tab and focus-based navigation for the toolbox, workspace, and flyout. The plugin has near feature parity with the existing flows, though highlighting still has a lot of work to do and there are some edge cases that are still broken here. --- core/common.ts | 1 + core/flyout_base.ts | 35 +++++-------- core/focus_manager.ts | 85 +++++++++++++++++++++++++++----- core/inject.ts | 5 -- core/keyboard_nav/line_cursor.ts | 8 ++- core/rendered_connection.ts | 77 ++++++++++++++--------------- core/toolbox/toolbox.ts | 31 +++++++----- core/workspace_svg.ts | 38 +++++++++----- 8 files changed, 174 insertions(+), 106 deletions(-) diff --git a/core/common.ts b/core/common.ts index bc31bf17eea..80c5ecdc345 100644 --- a/core/common.ts +++ b/core/common.ts @@ -82,6 +82,7 @@ export function getMainWorkspace(): Workspace { * @param workspace The most recently used top level workspace. */ export function setMainWorkspace(workspace: Workspace) { + console.log('@@@@@@ set main workspace:', workspace.id); mainWorkspace = workspace; } diff --git a/core/flyout_base.ts b/core/flyout_base.ts index 1a1da7ffec3..8407510f926 100644 --- a/core/flyout_base.ts +++ b/core/flyout_base.ts @@ -322,7 +322,7 @@ export abstract class Flyout .getThemeManager() .subscribe(this.svgBackground_, 'flyoutOpacity', 'fill-opacity'); - getFocusManager().registerTree(this); + // getFocusManager().registerTree(this); return this.svgGroup_; } @@ -405,7 +405,7 @@ export abstract class Flyout if (this.svgGroup_) { dom.removeNode(this.svgGroup_); } - getFocusManager().unregisterTree(this); + // getFocusManager().unregisterTree(this); } /** @@ -971,38 +971,27 @@ export abstract class Flyout } getFocusableElement(): HTMLElement | SVGElement { - if (!this.svgGroup_) { - throw new Error("Flyout has no DOM created."); - } - return this.svgGroup_; + return this.workspace_.getFocusableElement(); } getFocusableTree(): IFocusableTree { - return this; - } - - getFocusedNode(): IFocusableNode | null { - return FocusableTreeTraverser.findFocusedNode(this); + return this.workspace_.getFocusableTree(); } getRootFocusableNode(): IFocusableNode { - return this; + return this.workspace_.getRootFocusableNode(); } getNestedTrees(): Array { - return []; + return this.workspace_.getNestedTrees(); } lookUpFocusableNode(id: string): IFocusableNode | null { - return this.getContents() - .filter((item) => item instanceof isFocusableNode) - .map((item) => item as unknown as IFocusableNode) - .find((node) => node.getFocusableElement().id == id) ?? null; - } - - findFocusableNodeFor( - element: HTMLElement | SVGElement, - ): IFocusableNode | null { - return FocusableTreeTraverser.findFocusableNodeFor(element, this); + return this.workspace_.lookUpFocusableNode(id); + // TODO: This may violate the cross-subtree boundary (since flyout contains a workspace that is itself a tree). + // return this.getContents() + // .filter((item) => isFocusableNode(item)) + // .map((item) => item as unknown as IFocusableNode) + // .find((node) => node.getFocusableElement().id == id) ?? null; } } diff --git a/core/focus_manager.ts b/core/focus_manager.ts index a8a1a03db17..ae4ede14245 100644 --- a/core/focus_manager.ts +++ b/core/focus_manager.ts @@ -17,6 +17,28 @@ import {FocusableTreeTraverser} from './utils/focusable_tree_traverser.js'; */ export type ReturnEphemeralFocus = () => void; +// TODO: Initialize can also happen if a focused node is outright removed. May need additional mutable state in the registration class or something to track. +export type InitializeFirstFocus = () => IFocusableNode | null; +// TODO: Sync can be called multiple times due to focus looping. Maybe we need to debounce a bit? TreeRegistration can also easily keep track of the focused node which might simplify a lot...though the node could also vanish. +export type SyncFocusState = (node: IFocusableNode) => void; +export type BlurFocus = () => void; + +export interface TreeCustomizationCallbacks { + Initialize: InitializeFirstFocus | null; + Synchronize: SyncFocusState | null; + BlurFocus: BlurFocus | null; +} + +class TreeRegistration { + readonly tree: IFocusableTree; + readonly callbacks: TreeCustomizationCallbacks; + + constructor(tree: IFocusableTree, callbacks: TreeCustomizationCallbacks) { + this.tree = tree; + this.callbacks = callbacks; + } +} + /** * A per-page singleton that manages Blockly focus across one or more * IFocusableTrees, and bidirectionally synchronizes this focus with the DOM. @@ -57,7 +79,7 @@ export class FocusManager { static readonly PASSIVE_FOCUS_NODE_CSS_CLASS_NAME = 'blocklyPassiveFocus'; focusedNode: IFocusableNode | null = null; - registeredTrees: Array = []; + registeredTrees: Array = []; private currentlyHoldsEphemeralFocus: boolean = false; // TODO: Figure out why this is needed, and how to test it. @@ -82,17 +104,34 @@ export class FocusManager { // 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. - for (const tree of this.registeredTrees) { + for (const registration of this.registeredTrees) { newNode = FocusableTreeTraverser.findFocusableNodeFor( activeElement, - tree, + registration.tree, ); if (newNode) break; } } if (newNode) { - this.focusNode(newNode); + const newTree = newNode.getFocusableTree(); + const oldTree = this.focusedNode?.getFocusableTree(); + const prevFocusedNode = FocusableTreeTraverser.findFocusedNode(newTree); + // TODO: Discuss the commented out conditional with the team. This means that tabbing to the + // root of a tree will always behave the same as focusTree(), that is, it will restore focus + // active focus back to the previous node if there was any. This introduces consistency with + // explicit focusTree() calls, but may not always be expected by the user as it will shift + // focus to something new. Note that the extra tree check is to ensure users can tab out of + // a tree (in which case they WILL go to the root and set that as the last known focus state + // of the tree which should generally stabilize the user experience since they explicitly + // navigated away). + if (newNode === newTree.getRootFocusableNode() && newTree !== oldTree/* && !prevFocusedNode*/) { + // If the root of the tree is the one taking focus, try to focus the + // whole tree explicitly since it hasn't yet received focus. + this.focusTree(newTree); + } else { + this.focusNode(newNode); + } } else { this.defocusCurrentFocusedNode(); } @@ -110,11 +149,12 @@ export class FocusManager { * in this manager. Use isRegistered to check in cases when it can't be * certain whether the tree has been registered. */ - registerTree(tree: IFocusableTree): void { + registerTree(tree: IFocusableTree, callbacks: TreeCustomizationCallbacks = {Initialize: null, Synchronize: null, BlurFocus: null}): void { if (this.isRegistered(tree)) { throw Error(`Attempted to re-register already registered tree: ${tree}.`); } - this.registeredTrees.push(tree); + this.registeredTrees.push( + new TreeRegistration(tree, callbacks)); } /** @@ -123,7 +163,11 @@ export class FocusManager { * unregisterTree. */ isRegistered(tree: IFocusableTree): boolean { - return this.registeredTrees.findIndex((reg) => reg === tree) !== -1; + return !!this.lookUpRegistration(tree); + } + + private lookUpRegistration(tree: IFocusableTree): TreeRegistration | null { + return this.registeredTrees.find((reg) => reg.tree === tree) ?? null; } /** @@ -195,11 +239,20 @@ export class FocusManager { * focus. */ focusTree(focusableTree: IFocusableTree): void { - if (!this.isRegistered(focusableTree)) { + const registration = this.lookUpRegistration(focusableTree); + if (!registration) { throw Error(`Attempted to focus unregistered tree: ${focusableTree}.`); } const currNode = FocusableTreeTraverser.findFocusedNode(focusableTree); - this.focusNode(currNode ?? focusableTree.getRootFocusableNode()); + const initialize = registration.callbacks.Initialize; + let nextNodeToFocus: IFocusableNode | null = currNode; + if (!nextNodeToFocus && initialize) { + // The tree hasn't yet received focus, so initialize it. + nextNodeToFocus = initialize() + } + // Either the previous node has been restored, a new node has been + // initialized, or the root should be used (due to no or failed initing). + this.focusNode(nextNodeToFocus ?? focusableTree.getRootFocusableNode()); } /** @@ -212,11 +265,13 @@ export class FocusManager { */ focusNode(focusableNode: IFocusableNode): void { const nextTree = focusableNode.getFocusableTree(); - if (!this.isRegistered(nextTree)) { + const registration = this.lookUpRegistration(nextTree); + if (!registration) { throw Error(`Attempted to focus unregistered node: ${focusableNode}.`); } const prevNode = this.focusedNode; - if (prevNode && prevNode.getFocusableTree() !== nextTree) { + const prevTree = prevNode?.getFocusableTree(); + if (prevNode && prevTree !== nextTree) { this.setNodeToPassive(prevNode); } // If there's a focused node in the new node's tree, ensure it's reset. @@ -236,6 +291,14 @@ export class FocusManager { this.setNodeToActive(focusableNode); } this.focusedNode = focusableNode; + if (registration.callbacks.Synchronize) { + // Provide the tree with an opportunity to synchronize to focus state. + registration.callbacks.Synchronize(focusableNode); + } + if (prevTree && prevTree !== nextTree) { + const blurFocus = this.lookUpRegistration(prevTree)?.callbacks?.BlurFocus; + if (blurFocus) blurFocus(); + } } /** 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/keyboard_nav/line_cursor.ts b/core/keyboard_nav/line_cursor.ts index 610ee5698c8..0a356ec53d1 100644 --- a/core/keyboard_nav/line_cursor.ts +++ b/core/keyboard_nav/line_cursor.ts @@ -757,7 +757,13 @@ export class LineCursor extends Marker { block = block.getParent(); } if (block) { - this.setCurNode(ASTNode.createBlockNode(block), false); + 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), false); + } else { + this.setCurNode(ASTNode.createBlockNode(block), false); + } } } } diff --git a/core/rendered_connection.ts b/core/rendered_connection.ts index 1bcd95c0245..afcd844c032 100644 --- a/core/rendered_connection.ts +++ b/core/rendered_connection.ts @@ -45,8 +45,8 @@ export class RenderedConnection extends Connection implements IFocusableNode { private trackedState: TrackedState; private highlighted: boolean = false; private constants: ConstantProvider; - private svgGroup: SVGElement | null = null; - private svgPath: SVGElement | null = null; + private svgGroup: SVGElement; + private svgPath: SVGElement; /** Connection this connection connects to. Null if not connected. */ override targetConnection: RenderedConnection | null = null; @@ -78,6 +78,41 @@ export class RenderedConnection extends Connection implements IFocusableNode { 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)' : ''), + ); } /** @@ -566,41 +601,6 @@ export class RenderedConnection extends Connection implements IFocusableNode { const visible = parentInput.isVisible(); childBlock.getSvgRoot().style.display = visible ? 'block' : 'none'; } - - 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)' : ''), - ); } /** @@ -637,9 +637,6 @@ export class RenderedConnection extends Connection implements IFocusableNode { } getFocusableElement(): HTMLElement | SVGElement { - if (!this.svgGroup) { - throw Error("This connection hasn't been connected."); - } return this.svgGroup; } diff --git a/core/toolbox/toolbox.ts b/core/toolbox/toolbox.ts index b83129aace5..cadf73dbe2b 100644 --- a/core/toolbox/toolbox.ts +++ b/core/toolbox/toolbox.ts @@ -22,7 +22,7 @@ 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 {getFocusManager, TreeCustomizationCallbacks} 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'; @@ -172,7 +172,22 @@ export class Toolbox ComponentManager.Capability.DRAG_TARGET, ], }); - getFocusManager().registerTree(this); + const customizationOptions: TreeCustomizationCallbacks = { + Initialize: () => { + return this.getToolboxItems().find((item) => + item.isSelectable()) ?? null; + }, + Synchronize: (node: IFocusableNode) => { + if (node !== this) { + // Only select the item if it isn't already selected as to not toggle. + if (this.getSelectedItem() !== node) { + this.setSelectedItem(node as IToolboxItem); + } + } else this.clearSelection(); + }, + BlurFocus: null + }; + getFocusManager().registerTree(this, customizationOptions); } /** @@ -187,7 +202,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_); @@ -204,6 +218,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'); @@ -1100,10 +1115,6 @@ export class Toolbox return this; } - getFocusedNode(): IFocusableNode | null { - return FocusableTreeTraverser.findFocusedNode(this); - } - getRootFocusableNode(): IFocusableNode { return this; } @@ -1115,12 +1126,6 @@ export class Toolbox lookUpFocusableNode(id: string): IFocusableNode | null { return this.getToolboxItemById(id) as IFocusableNode; } - - findFocusableNodeFor( - element: HTMLElement | SVGElement, - ): IFocusableNode | null { - return FocusableTreeTraverser.findFocusableNodeFor(element, this); - } } /** CSS for Toolbox. See css.js for use. */ diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index 569a0cc120e..92768ef4fbc 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -37,7 +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 {getFocusManager, TreeCustomizationCallbacks} 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'; @@ -87,6 +87,8 @@ import {Workspace} from './workspace.js'; import {WorkspaceAudio} from './workspace_audio.js'; import {ZoomControls} from './zoom_controls.js'; import type {Field} from './field.js'; +import * as aria from './utils/aria.js'; +import {Msg} from './msg.js'; /** Margin around the top/bottom/left/right after a zoomToFit call. */ const ZOOM_TO_FIT_MARGIN = 20; @@ -770,9 +772,13 @@ export class WorkspaceSvg */ this.svgGroup_ = dom.createSvgElement(Svg.G, { 'class': 'blocklyWorkspace', - 'tabindex': '-1', + // TODO: Verify whether using this or opt_backgroundClass is a reasonable proxy for the main workspace, or if something else should be used. + '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 @@ -849,7 +855,23 @@ export class WorkspaceSvg isParentWorkspace ? this.getInjectionDiv() : undefined, ); - getFocusManager().registerTree(this); + const customizationOptions: TreeCustomizationCallbacks = { + Initialize: () => { + // TODO: This doesn't handle the idea of starting in a preset location like the plugin does. + return this.getTopBlocks(true)[0] ?? null; + }, + // TODO: Perhaps synchronize here could select the block? Would drastically simplify selection management in cursor (I think). + Synchronize: null, + BlurFocus: () => { + // TODO: make sure this works correctly for a permanent flyout. + // If the flyout loses focus, make sure to close it. + if (this.isFlyout) { + // TODO: fix this as it doesn't seem to work (it gets triggered correctly, hide() just doesn't seem to do anything). + this.getFlyout()?.hide(); + } + } + }; + getFocusManager().registerTree(this, customizationOptions); return this.svgGroup_; } @@ -2632,10 +2654,6 @@ export class WorkspaceSvg return this; } - getFocusedNode(): IFocusableNode | null { - return FocusableTreeTraverser.findFocusedNode(this); - } - getRootFocusableNode(): IFocusableNode { return this; } @@ -2659,12 +2677,6 @@ export class WorkspaceSvg return null; } else return this.getBlockById(id) as IFocusableNode; } - - findFocusableNodeFor( - element: HTMLElement | SVGElement, - ): IFocusableNode | null { - return FocusableTreeTraverser.findFocusableNodeFor(element, this); - } } /** From 5f9de824a7b837cc503a47f5e8c2a7c57a3374e7 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Fri, 18 Apr 2025 00:32:57 +0000 Subject: [PATCH 03/33] feat: LOTS of functionality changes. Essentially, this: - Adds rendered connection support (though a lot of the path management still needs to be cleaned up). - Adds ephemeral focus support for field input editing. - Fixes a lot of tabbing issues between the toolbox, flyout, and main workspace. - Makes selection CSS-based. - Adds a lot of new lifecycle inspection methods for both focusable trees and nodes. - Adds default overriding support for trees so that tree-level focusing (both explicitly and via the DOM) can have domain state changes. - Automatically manages toolbox item selection based on focus. - Automatically manages block selection based on focus. There are still some fundamental issues with CSS styling inheritance that haven't been figured out yet. --- core/block_svg.ts | 17 ++++- core/common.ts | 1 - core/connection.ts | 4 ++ core/css.ts | 26 +++++--- core/field.ts | 4 ++ core/field_input.ts | 9 ++- core/flyout_base.ts | 48 ++++++++++++-- core/focus_manager.ts | 92 ++++++++++++++++---------- core/interfaces/i_focusable_node.ts | 4 ++ core/interfaces/i_focusable_tree.ts | 6 ++ core/keyboard_nav/line_cursor.ts | 4 +- core/rendered_connection.ts | 30 ++++++++- 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 | 32 ++++++--- core/renderers/zelos/constants.ts | 6 +- core/renderers/zelos/drawer.ts | 7 +- core/renderers/zelos/path_object.ts | 30 ++++----- core/toolbox/toolbox.ts | 68 ++++++++++++++----- core/toolbox/toolbox_item.ts | 4 ++ core/workspace_svg.ts | 83 +++++++++++++++++------ 22 files changed, 362 insertions(+), 141 deletions(-) diff --git a/core/block_svg.ts b/core/block_svg.ts index 83b39f66457..51252f8fe96 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -192,7 +192,7 @@ export class BlockSvg throw TypeError('Cannot create a rendered block in a headless workspace'); } this.workspace = workspace; - this.svgGroup = dom.createSvgElement(Svg.G, {'tabindex': '-1', 'id': this.id}); + this.svgGroup = dom.createSvgElement(Svg.G, {}); if (prototypeName) { dom.addClass(this.svgGroup, prototypeName); @@ -211,6 +211,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_(); } @@ -1772,10 +1773,22 @@ export class BlockSvg } getFocusableElement(): HTMLElement | SVGElement { - return this.svgGroup; + return this.pathObject.svgPath; } getFocusableTree(): IFocusableTree { return this.workspace; } + + onNodeFocus(): void { + if (!this.isShadow()) { + common.setSelected(this); + } + } + + onNodeBlur(): void { + if (common.getSelected() === this) { + common.setSelected(null); + } + } } diff --git a/core/common.ts b/core/common.ts index 80c5ecdc345..bc31bf17eea 100644 --- a/core/common.ts +++ b/core/common.ts @@ -82,7 +82,6 @@ export function getMainWorkspace(): Workspace { * @param workspace The most recently used top level workspace. */ export function setMainWorkspace(workspace: Workspace) { - console.log('@@@@@@ set main workspace:', workspace.id); mainWorkspace = workspace; } 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/css.ts b/core/css.ts index edd31fbda68..ee3f56b5965 100644 --- a/core/css.ts +++ b/core/css.ts @@ -150,8 +150,8 @@ let content = ` .blocklyHighlightedConnectionPath { fill: none; - stroke: #fc3; - stroke-width: 4px; + // stroke: #fc3; + // stroke-width: 4px; } .blocklyPathLight { @@ -160,6 +160,11 @@ let content = ` stroke-width: 1; } +.blocklySelected { + stroke: #ffa200; + stroke-width: 5; +} + .blocklySelected>.blocklyPathLight { display: none; } @@ -464,8 +469,8 @@ input[type=number] { } .blocklyMenuSeparator { - background-color: #ccc; - height: 1px; + background-color: #ccc; + height: 1px; border: 0; margin-left: 4px; margin-right: 4px; @@ -496,13 +501,14 @@ input[type=number] { } .blocklyActiveFocus { - stroke: #2ae; - stroke-width: 2px; - fill: #0f0; + stroke: #ffa200; + stroke-width: 3; + outline-color: #ffa200; } .blocklyPassiveFocus { - stroke: #3fdfff; - stroke-width: 1.5px; - fill: #00f; + stroke: #ffa200; + stroke-dasharray: 5 3; + stroke-width: 3; + outline-color: #ffa200; } `; diff --git a/core/field.ts b/core/field.ts index 89004f9dc0d..e4fe86095c7 100644 --- a/core/field.ts +++ b/core/field.ts @@ -1428,6 +1428,10 @@ export abstract class Field return block.workspace as WorkspaceSvg; } + onNodeFocus(): void {} + + onNodeBlur(): void {} + /** * Subclasses should reimplement this method to construct their Field * subclass from a JSON arg object. diff --git a/core/field_input.ts b/core/field_input.ts index 2cdd8056553..d13b58214cc 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. @@ -351,6 +352,7 @@ export abstract class FieldInput extends Field< * keyboards). */ private showPromptEditor() { + const returnFocusCallback = getFocusManager().takeEphemeralFocus(document.body); dialog.prompt( Msg['CHANGE_VALUE_TITLE'], this.getText(), @@ -360,6 +362,7 @@ export abstract class FieldInput extends Field< this.setValue(this.getValueFromEditorText_(text)); } this.onFinishEditing_(this.value_); + returnFocusCallback(); }, ); } @@ -374,10 +377,14 @@ export abstract class FieldInput extends Field< if (!block) { throw new UnattachedFieldError(); } + const returnFocusCallback = getFocusManager().takeEphemeralFocus(document.body); WidgetDiv.show( this, block.RTL, - this.widgetDispose_.bind(this), + () => { + this.widgetDispose_(); + returnFocusCallback(); + }, this.workspace_, ); this.htmlInput_ = this.widgetCreate_() as HTMLInputElement; diff --git a/core/flyout_base.ts b/core/flyout_base.ts index 8407510f926..0feb245a664 100644 --- a/core/flyout_base.ts +++ b/core/flyout_base.ts @@ -307,6 +307,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( @@ -322,7 +323,7 @@ export abstract class Flyout .getThemeManager() .subscribe(this.svgBackground_, 'flyoutOpacity', 'fill-opacity'); - // getFocusManager().registerTree(this); + getFocusManager().registerTree(this); return this.svgGroup_; } @@ -405,7 +406,7 @@ export abstract class Flyout if (this.svgGroup_) { dom.removeNode(this.svgGroup_); } - // getFocusManager().unregisterTree(this); + getFocusManager().unregisterTree(this); } /** @@ -971,23 +972,56 @@ export abstract class Flyout } getFocusableElement(): HTMLElement | SVGElement { - return this.workspace_.getFocusableElement(); + if (!this.svgGroup_) throw new Error('Flyout DOM is not yet created.'); + return this.svgGroup_; + // return this.workspace_.getFocusableElement(); } getFocusableTree(): IFocusableTree { - return this.workspace_.getFocusableTree(); + return this; + // return this.workspace_.getFocusableTree(); + } + + onNodeFocus(): void { + // this.workspace_.onNodeFocus(); + } + + onNodeBlur(): void { + // this.workspace_.onNodeBlur(); } getRootFocusableNode(): IFocusableNode { - return this.workspace_.getRootFocusableNode(); + return this; + // return this.workspace_.getRootFocusableNode(); + } + + getRestoredFocusableNode(previousNode: IFocusableNode | null): IFocusableNode | null { + return null; } getNestedTrees(): Array { - return this.workspace_.getNestedTrees(); + return [this.workspace_]; + // return this.workspace_.getNestedTrees(); + } + + onTreeFocus(node: IFocusableNode, previousTree: IFocusableTree | null): void { + // this.workspace_.onTreeFocus(node); + } + + 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. + if (toolbox && nextTree === toolbox) return; + if (nextTree == this.workspace_) return; + if (toolbox) toolbox.clearSelection(); + this.autoHide(false); + // this.workspace_.onTreeBlur(); } lookUpFocusableNode(id: string): IFocusableNode | null { - return this.workspace_.lookUpFocusableNode(id); + return null; + // return this.workspace_.lookUpFocusableNode(id); // TODO: This may violate the cross-subtree boundary (since flyout contains a workspace that is itself a tree). // return this.getContents() // .filter((item) => isFocusableNode(item)) diff --git a/core/focus_manager.ts b/core/focus_manager.ts index ae4ede14245..429280d351e 100644 --- a/core/focus_manager.ts +++ b/core/focus_manager.ts @@ -79,7 +79,7 @@ export class FocusManager { static readonly PASSIVE_FOCUS_NODE_CSS_CLASS_NAME = 'blocklyPassiveFocus'; focusedNode: IFocusableNode | null = null; - registeredTrees: Array = []; + registeredTrees: Array = []; private currentlyHoldsEphemeralFocus: boolean = false; // TODO: Figure out why this is needed, and how to test it. @@ -104,10 +104,10 @@ export class FocusManager { // 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. - for (const registration of this.registeredTrees) { + for (const tree of this.registeredTrees) { newNode = FocusableTreeTraverser.findFocusableNodeFor( activeElement, - registration.tree, + tree, ); if (newNode) break; } @@ -125,6 +125,7 @@ export class FocusManager { // a tree (in which case they WILL go to the root and set that as the last known focus state // of the tree which should generally stabilize the user experience since they explicitly // navigated away). + console.log('@@@@@@ focusing a new node, is tree root:', newNode === newTree.getRootFocusableNode(), 'tree is different:',newTree !== oldTree); if (newNode === newTree.getRootFocusableNode() && newTree !== oldTree/* && !prevFocusedNode*/) { // If the root of the tree is the one taking focus, try to focus the // whole tree explicitly since it hasn't yet received focus. @@ -149,12 +150,11 @@ export class FocusManager { * in this manager. Use isRegistered to check in cases when it can't be * certain whether the tree has been registered. */ - registerTree(tree: IFocusableTree, callbacks: TreeCustomizationCallbacks = {Initialize: null, Synchronize: null, BlurFocus: null}): void { + registerTree(tree: IFocusableTree): void {//, callbacks: TreeCustomizationCallbacks = {Initialize: null, Synchronize: null, BlurFocus: null}): void { if (this.isRegistered(tree)) { - throw Error(`Attempted to re-register already registered tree: ${tree}.`); + throw new Error(`Attempted to re-register already registered tree: ${tree}.`); } - this.registeredTrees.push( - new TreeRegistration(tree, callbacks)); + this.registeredTrees.push(tree); } /** @@ -163,11 +163,7 @@ export class FocusManager { * unregisterTree. */ isRegistered(tree: IFocusableTree): boolean { - return !!this.lookUpRegistration(tree); - } - - private lookUpRegistration(tree: IFocusableTree): TreeRegistration | null { - return this.registeredTrees.find((reg) => reg.tree === tree) ?? null; + return this.registeredTrees.findIndex((reg) => reg === tree) !== -1; } /** @@ -181,7 +177,7 @@ export class FocusManager { */ unregisterTree(tree: IFocusableTree): void { if (!this.isRegistered(tree)) { - throw Error(`Attempted to unregister not registered tree: ${tree}.`); + throw new Error(`Attempted to unregister not registered tree: ${tree}.`); } const treeIndex = this.registeredTrees.findIndex((tree) => tree === tree); this.registeredTrees.splice(treeIndex, 1); @@ -239,20 +235,24 @@ export class FocusManager { * focus. */ focusTree(focusableTree: IFocusableTree): void { - const registration = this.lookUpRegistration(focusableTree); - if (!registration) { - throw Error(`Attempted to focus unregistered tree: ${focusableTree}.`); + if (!this.isRegistered(focusableTree)) { + throw new Error(`Attempted to focus unregistered tree: ${focusableTree}.`); } const currNode = FocusableTreeTraverser.findFocusedNode(focusableTree); - const initialize = registration.callbacks.Initialize; - let nextNodeToFocus: IFocusableNode | null = currNode; - if (!nextNodeToFocus && initialize) { + const rootFallback = focusableTree.getRootFocusableNode(); + const nodeToRestore = focusableTree.getRestoredFocusableNode(currNode); + // let nextNodeToFocus: IFocusableNode | null = currNode; + // // const initialize = registration.callbacks.Initialize; + // if (!nextNodeToFocus) { + // nextNodeToFocus = focusableTree.getDefaultFocusableNode(); + // } + // if (!nextNodeToFocus && initialize) { // The tree hasn't yet received focus, so initialize it. - nextNodeToFocus = initialize() - } + // nextNodeToFocus = initialize() + // } // Either the previous node has been restored, a new node has been // initialized, or the root should be used (due to no or failed initing). - this.focusNode(nextNodeToFocus ?? focusableTree.getRootFocusableNode()); + this.focusNode(nodeToRestore ?? currNode ?? rootFallback); } /** @@ -264,11 +264,24 @@ export class FocusManager { * @param focusableNode The node that should receive active focus. */ focusNode(focusableNode: IFocusableNode): void { + if (this.focusedNode == focusableNode) return; // State is unchanged. + const nextTree = focusableNode.getFocusableTree(); - const registration = this.lookUpRegistration(nextTree); - if (!registration) { - throw Error(`Attempted to focus unregistered node: ${focusableNode}.`); + if (!this.isRegistered(nextTree)) { + throw new Error(`Attempted to focus unregistered node: ${focusableNode}.`); + } + + // Safety check for ensuring focusNode() doesn't get called for a node that + // isn't actually hooked up to its parent tree correctly (since this can + // cause weird inconsistencies). + const matchedNode = FocusableTreeTraverser.findFocusableNodeFor( + focusableNode.getFocusableElement(), + nextTree, + ); + if (matchedNode !== focusableNode) { + throw new Error(`Attempting to focus node which isn't recognized by its parent tree: ${focusableNode}.`); } + const prevNode = this.focusedNode; const prevTree = prevNode?.getFocusableTree(); if (prevNode && prevTree !== nextTree) { @@ -286,19 +299,28 @@ export class FocusManager { if (nextTreeRoot !== focusableNode) { this.removeHighlight(nextTreeRoot); } + + // TODO: Add guardrails to prevent focus methods from being invoked in callbacks. + // TODO: Make this work correctly with ephemeral focus. + // TODO: Figure out correct ordering here, but if we allow onNodeFocus to change internal display visibility then the focus() call *must* happen after it. + if (prevTree && prevTree !== nextTree) prevTree.onTreeBlur(nextTree); + nextTree.onTreeFocus(focusableNode, prevTree ?? null); + if (prevNode) prevNode.onNodeBlur(); + focusableNode.onNodeFocus(); + if (!this.currentlyHoldsEphemeralFocus) { // Only change the actively focused node if ephemeral state isn't held. this.setNodeToActive(focusableNode); } this.focusedNode = focusableNode; - if (registration.callbacks.Synchronize) { + // if (registration.callbacks.Synchronize) { // Provide the tree with an opportunity to synchronize to focus state. - registration.callbacks.Synchronize(focusableNode); - } - if (prevTree && prevTree !== nextTree) { - const blurFocus = this.lookUpRegistration(prevTree)?.callbacks?.BlurFocus; - if (blurFocus) blurFocus(); - } + // registration.callbacks.Synchronize(focusableNode); + // } + // if (prevTree && prevTree !== nextTree) { + // const blurFocus = this.lookUpRegistration(prevTree)?.callbacks?.BlurFocus; + // if (blurFocus) blurFocus(); + // } } /** @@ -323,7 +345,7 @@ export class FocusManager { focusableElement: HTMLElement | SVGElement, ): ReturnEphemeralFocus { if (this.currentlyHoldsEphemeralFocus) { - throw Error( + throw new Error( `Attempted to take ephemeral focus when it's already held, ` + `with new element: ${focusableElement}.`, ); @@ -338,7 +360,7 @@ export class FocusManager { let hasFinishedEphemeralFocus = false; return () => { if (hasFinishedEphemeralFocus) { - throw Error( + throw new Error( `Attempted to finish ephemeral focus twice for element: ` + `${focusableElement}.`, ); @@ -358,6 +380,8 @@ export class FocusManager { // restored upon exiting ephemeral focus mode. if (this.focusedNode && !this.currentlyHoldsEphemeralFocus) { this.setNodeToPassive(this.focusedNode); + this.focusedNode.getFocusableTree().onTreeBlur(null); + this.focusedNode.onNodeBlur(); this.focusedNode = null; } } diff --git a/core/interfaces/i_focusable_node.ts b/core/interfaces/i_focusable_node.ts index d238dc80cd5..bc1c985e69f 100644 --- a/core/interfaces/i_focusable_node.ts +++ b/core/interfaces/i_focusable_node.ts @@ -36,6 +36,10 @@ export interface IFocusableNode { * belongs. */ getFocusableTree(): IFocusableTree; + + onNodeFocus(): void; + + onNodeBlur(): void; } export function isFocusableNode(object: any | null): object is IFocusableNode { diff --git a/core/interfaces/i_focusable_tree.ts b/core/interfaces/i_focusable_tree.ts index 0aa55c07053..c2cbd016a88 100644 --- a/core/interfaces/i_focusable_tree.ts +++ b/core/interfaces/i_focusable_tree.ts @@ -37,6 +37,8 @@ export interface IFocusableTree { */ getRootFocusableNode(): IFocusableNode; + getRestoredFocusableNode(previousNode: IFocusableNode | null): IFocusableNode | null; + /** * Returns all directly nested trees under this tree. * @@ -58,6 +60,10 @@ export interface IFocusableTree { * @param id The ID of the node's focusable HTMLElement or SVGElement. */ lookUpFocusableNode(id: string): IFocusableNode | null; + + onTreeFocus(node: IFocusableNode, previousTree: IFocusableTree | null): void; + + onTreeBlur(nextTree: IFocusableTree | null): void; } export function isFocusableTree(object: any | null): object is IFocusableTree { diff --git a/core/keyboard_nav/line_cursor.ts b/core/keyboard_nav/line_cursor.ts index 0a356ec53d1..c606b3d2c77 100644 --- a/core/keyboard_nav/line_cursor.ts +++ b/core/keyboard_nav/line_cursor.ts @@ -74,7 +74,7 @@ export class LineCursor extends Marker { super(); // Bind changeListener to facilitate future disposal. this.changeListener = this.changeListener.bind(this); - this.workspace.addChangeListener(this.changeListener); + // this.workspace.addChangeListener(this.changeListener); // Regularise options and apply defaults. this.options = {...defaultOptions, ...options}; @@ -622,7 +622,7 @@ export class LineCursor extends Marker { const isZelosInputConnection = this.isZelos && curNode && this.isValueInputConnection(curNode); - realDrawer.draw(oldNode, curNode); + // realDrawer.draw(oldNode, curNode); // If drawing can't be handled locally, just use the drawer. if (curNodeType !== ASTNode.types.BLOCK && !isZelosInputConnection) { diff --git a/core/rendered_connection.ts b/core/rendered_connection.ts index afcd844c032..92a13ca26ed 100644 --- a/core/rendered_connection.ts +++ b/core/rendered_connection.ts @@ -363,13 +363,21 @@ export class RenderedConnection extends Connection implements IFocusableNode { /** 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. */ @@ -637,12 +645,28 @@ export class RenderedConnection extends Connection implements IFocusableNode { } getFocusableElement(): HTMLElement | SVGElement { - return this.svgGroup; + const highlightSvg = this.findHighlightSvg(); + if (highlightSvg) return highlightSvg; + throw new Error('No highlight SVG found corresponding to this connection.'); } getFocusableTree(): IFocusableTree { return this.getSourceBlock().workspace as WorkspaceSvg; } + + onNodeFocus(): void { + this.highlight(); + } + + 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; + } } export namespace RenderedConnection { 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 077f80bb741..d216db1a221 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, ); @@ -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..02260f308c4 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/toolbox/toolbox.ts b/core/toolbox/toolbox.ts index cadf73dbe2b..b6d174ac9bb 100644 --- a/core/toolbox/toolbox.ts +++ b/core/toolbox/toolbox.ts @@ -172,22 +172,23 @@ export class Toolbox ComponentManager.Capability.DRAG_TARGET, ], }); - const customizationOptions: TreeCustomizationCallbacks = { - Initialize: () => { - return this.getToolboxItems().find((item) => - item.isSelectable()) ?? null; - }, - Synchronize: (node: IFocusableNode) => { - if (node !== this) { - // Only select the item if it isn't already selected as to not toggle. - if (this.getSelectedItem() !== node) { - this.setSelectedItem(node as IToolboxItem); - } - } else this.clearSelection(); - }, - BlurFocus: null - }; - getFocusManager().registerTree(this, customizationOptions); + // const customizationOptions: TreeCustomizationCallbacks = { + // Initialize: () => { + // return this.getToolboxItems().find((item) => + // item.isSelectable()) ?? null; + // }, + // Synchronize: (node: IFocusableNode) => { + // if (node !== this) { + // // Only select the item if it isn't already selected as to not toggle. + // if (this.getSelectedItem() !== node) { + // this.setSelectedItem(node as IToolboxItem); + // } + // } else this.clearSelection(); + // }, + // BlurFocus: null + // }; + // getFocusManager().registerTree(this, customizationOptions); + getFocusManager().registerTree(this); } /** @@ -1119,13 +1120,46 @@ export class Toolbox return this; } + onNodeFocus(): void {} + + onNodeBlur(): void {} + + getRestoredFocusableNode(previousNode: IFocusableNode | null): IFocusableNode | null { + if (!previousNode || previousNode === this) { + return this.getToolboxItems().find((item) => item.isSelectable()) ?? null; + } else return null; + } + getNestedTrees(): Array { - if (this.flyout) return [this.flyout]; else return []; + return []; + // if (this.flyout) return [this.flyout.getWorkspace()]; else return []; } lookUpFocusableNode(id: string): IFocusableNode | null { return this.getToolboxItemById(id) as IFocusableNode; } + + onTreeFocus(node: IFocusableNode, previousTree: IFocusableTree | null): void { + console.log('@@@@@ toolbox.onTreeFocus', node, 'is root:', node===this); + if (node !== this) { + // Only select the item if it isn't already selected so as to not toggle. + if (this.getSelectedItem() !== node) { + console.log('@@@@@ select item', node as IToolboxItem); + this.setSelectedItem(node as IToolboxItem); + } + } else this.clearSelection(); + /*if (node !== this && this.getSelectedItem() !== node) { + console.log('@@@@@ select item', node as IToolboxItem); + this.setSelectedItem(node as IToolboxItem); + } else { + // There should always be a toolbox item selected, if possible. + const first = this.getToolboxItems().find((item) => item.isSelectable()); + this.setSelectedItem(first ?? null); + }*/ + } + + 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 2e538f31478..223328e3dc0 100644 --- a/core/toolbox/toolbox_item.ts +++ b/core/toolbox/toolbox_item.ts @@ -164,5 +164,9 @@ export class ToolboxItem implements IToolboxItem { getFocusableTree(): IFocusableTree { return this.parentToolbox_; } + + onNodeFocus(): void {} + + onNodeBlur(): void {} } // nop by default diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index 92768ef4fbc..b948582d2d5 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -89,6 +89,7 @@ import {ZoomControls} from './zoom_controls.js'; import type {Field} from './field.js'; import * as aria from './utils/aria.js'; import {Msg} from './msg.js'; +import { IAutoHideable } from './blockly.js'; /** Margin around the top/bottom/left/right after a zoomToFit call. */ const ZOOM_TO_FIT_MARGIN = 20; @@ -855,23 +856,24 @@ export class WorkspaceSvg isParentWorkspace ? this.getInjectionDiv() : undefined, ); - const customizationOptions: TreeCustomizationCallbacks = { - Initialize: () => { - // TODO: This doesn't handle the idea of starting in a preset location like the plugin does. - return this.getTopBlocks(true)[0] ?? null; - }, - // TODO: Perhaps synchronize here could select the block? Would drastically simplify selection management in cursor (I think). - Synchronize: null, - BlurFocus: () => { - // TODO: make sure this works correctly for a permanent flyout. - // If the flyout loses focus, make sure to close it. - if (this.isFlyout) { - // TODO: fix this as it doesn't seem to work (it gets triggered correctly, hide() just doesn't seem to do anything). - this.getFlyout()?.hide(); - } - } - }; - getFocusManager().registerTree(this, customizationOptions); + // const customizationOptions: TreeCustomizationCallbacks = { + // Initialize: () => { + // // TODO: This doesn't handle the idea of starting in a preset location like the plugin does. + // return this.getTopBlocks(true)[0] ?? null; + // }, + // // TODO: Perhaps synchronize here could select the block? Would drastically simplify selection management in cursor (I think). + // Synchronize: null, + // BlurFocus: () => { + // // TODO: make sure this works correctly for a permanent flyout. + // // If the flyout loses focus, make sure to close it. + // if (this.isFlyout) { + // // TODO: fix this as it doesn't seem to work (it gets triggered correctly, hide() just doesn't seem to do anything). + // this.getFlyout()?.hide(); + // } + // } + // }; + // getFocusManager().registerTree(this, customizationOptions); + getFocusManager().registerTree(this); return this.svgGroup_; } @@ -2654,29 +2656,70 @@ export class WorkspaceSvg return this; } + onNodeFocus(): void {} + + onNodeBlur(): void {} + getRootFocusableNode(): IFocusableNode { return this; } + getRestoredFocusableNode(previousNode: IFocusableNode | null): IFocusableNode | null { + if (!previousNode) { + // TODO: This doesn't handle the idea of starting in a preset location like the plugin does. + return this.getTopBlocks(true)[0] ?? null; + } else return null; + } + getNestedTrees(): Array { return []; } lookUpFocusableNode(id: string): IFocusableNode | null { // TODO: This isn't a complete solution since non-blocks can have focus. - const fieldIndicator = '_field_'; - const fieldIndicatorIndex = id.indexOf(fieldIndicator); + const fieldIndicatorIndex = id.indexOf('_field_'); + const connectionIndicatorIndex = id.indexOf('_connection_'); if (fieldIndicatorIndex !== -1) { const blockId = id.substring(0, fieldIndicatorIndex); const block = this.getBlockById(blockId); - if (block != null) { + if (block) { for (const field of block.getFields()) { if (field.getFocusableElement().id === id) return field; } } 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; } else return this.getBlockById(id) as IFocusableNode; } + + onTreeFocus(node: IFocusableNode, previousTree: IFocusableTree | null): void { + console.log('@@@@@@ onTreeFocus, isFlyout:', this.isFlyout); + // TODO: Perhaps synchronize here could select the block? Would drastically simplify selection management in cursor (I think). + } + + onTreeBlur(nextTree: IFocusableTree | null): void { + // TODO: make sure this works correctly for a permanent flyout. + // If the flyout loses focus, make sure to close it. + console.log('@@@@@@ onTreeBlur, isFlyout:', this.isFlyout); + 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); + } + } } /** From 9acf14494b8f685cc9b9896e1e64216906bab408 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Mon, 21 Apr 2025 19:40:40 +0000 Subject: [PATCH 04/33] chore: finalize focus infra changes pt1. This cleans up some obsolete code paths, fixes tests, and adds missing documentation in an effort to bring focus manager infrastructural changes into a state ready for review. There's still a bit more to resolve here before a new PR can be created. --- core/focus_manager.ts | 80 ++++---------------- core/interfaces/i_focusable_node.ts | 25 +++++- core/interfaces/i_focusable_tree.ts | 62 ++++++++++++++- core/toolbox/toolbox.ts | 18 +---- core/workspace_svg.ts | 19 +---- tests/mocha/focus_manager_test.js | 44 +++++++---- tests/mocha/focusable_tree_traverser_test.js | 12 ++- 7 files changed, 138 insertions(+), 122 deletions(-) diff --git a/core/focus_manager.ts b/core/focus_manager.ts index 429280d351e..fffcb527ee8 100644 --- a/core/focus_manager.ts +++ b/core/focus_manager.ts @@ -17,28 +17,6 @@ import {FocusableTreeTraverser} from './utils/focusable_tree_traverser.js'; */ export type ReturnEphemeralFocus = () => void; -// TODO: Initialize can also happen if a focused node is outright removed. May need additional mutable state in the registration class or something to track. -export type InitializeFirstFocus = () => IFocusableNode | null; -// TODO: Sync can be called multiple times due to focus looping. Maybe we need to debounce a bit? TreeRegistration can also easily keep track of the focused node which might simplify a lot...though the node could also vanish. -export type SyncFocusState = (node: IFocusableNode) => void; -export type BlurFocus = () => void; - -export interface TreeCustomizationCallbacks { - Initialize: InitializeFirstFocus | null; - Synchronize: SyncFocusState | null; - BlurFocus: BlurFocus | null; -} - -class TreeRegistration { - readonly tree: IFocusableTree; - readonly callbacks: TreeCustomizationCallbacks; - - constructor(tree: IFocusableTree, callbacks: TreeCustomizationCallbacks) { - this.tree = tree; - this.callbacks = callbacks; - } -} - /** * A per-page singleton that manages Blockly focus across one or more * IFocusableTrees, and bidirectionally synchronizes this focus with the DOM. @@ -82,8 +60,6 @@ export class FocusManager { registeredTrees: Array = []; private currentlyHoldsEphemeralFocus: boolean = false; - // TODO: Figure out why this is needed, and how to test it. - private ignoreFocusChanges: boolean = false; constructor( addGlobalEventListener: (type: string, listener: EventListener) => void, @@ -92,7 +68,6 @@ export class FocusManager { // tracked focusable trees. addGlobalEventListener('focusin', (event) => { if (!(event instanceof FocusEvent)) return; - if (this.ignoreFocusChanges) return; // The target that now has focus. const activeElement = document.activeElement; @@ -116,19 +91,10 @@ export class FocusManager { if (newNode) { const newTree = newNode.getFocusableTree(); const oldTree = this.focusedNode?.getFocusableTree(); - const prevFocusedNode = FocusableTreeTraverser.findFocusedNode(newTree); - // TODO: Discuss the commented out conditional with the team. This means that tabbing to the - // root of a tree will always behave the same as focusTree(), that is, it will restore focus - // active focus back to the previous node if there was any. This introduces consistency with - // explicit focusTree() calls, but may not always be expected by the user as it will shift - // focus to something new. Note that the extra tree check is to ensure users can tab out of - // a tree (in which case they WILL go to the root and set that as the last known focus state - // of the tree which should generally stabilize the user experience since they explicitly - // navigated away). - console.log('@@@@@@ focusing a new node, is tree root:', newNode === newTree.getRootFocusableNode(), 'tree is different:',newTree !== oldTree); - if (newNode === newTree.getRootFocusableNode() && newTree !== oldTree/* && !prevFocusedNode*/) { - // If the root of the tree is the one taking focus, try to focus the - // whole tree explicitly since it hasn't yet received focus. + if (newNode === newTree.getRootFocusableNode() && newTree !== oldTree) { + // If the root of the tree is the one taking focus (such as due to + // being tabbed), try to focus the whole tree explicitly to ensure the + // correct node re-receives focus. this.focusTree(newTree); } else { this.focusNode(newNode); @@ -150,9 +116,9 @@ export class FocusManager { * in this manager. Use isRegistered to check in cases when it can't be * certain whether the tree has been registered. */ - registerTree(tree: IFocusableTree): void {//, callbacks: TreeCustomizationCallbacks = {Initialize: null, Synchronize: null, BlurFocus: null}): void { + registerTree(tree: IFocusableTree): void { if (this.isRegistered(tree)) { - throw new Error(`Attempted to re-register already registered tree: ${tree}.`); + throw Error(`Attempted to re-register already registered tree: ${tree}.`); } this.registeredTrees.push(tree); } @@ -177,7 +143,7 @@ export class FocusManager { */ unregisterTree(tree: IFocusableTree): void { if (!this.isRegistered(tree)) { - throw new Error(`Attempted to unregister not registered tree: ${tree}.`); + throw Error(`Attempted to unregister not registered tree: ${tree}.`); } const treeIndex = this.registeredTrees.findIndex((tree) => tree === tree); this.registeredTrees.splice(treeIndex, 1); @@ -236,22 +202,11 @@ export class FocusManager { */ focusTree(focusableTree: IFocusableTree): void { if (!this.isRegistered(focusableTree)) { - throw new Error(`Attempted to focus unregistered tree: ${focusableTree}.`); + throw Error(`Attempted to focus unregistered tree: ${focusableTree}.`); } const currNode = FocusableTreeTraverser.findFocusedNode(focusableTree); - const rootFallback = focusableTree.getRootFocusableNode(); const nodeToRestore = focusableTree.getRestoredFocusableNode(currNode); - // let nextNodeToFocus: IFocusableNode | null = currNode; - // // const initialize = registration.callbacks.Initialize; - // if (!nextNodeToFocus) { - // nextNodeToFocus = focusableTree.getDefaultFocusableNode(); - // } - // if (!nextNodeToFocus && initialize) { - // The tree hasn't yet received focus, so initialize it. - // nextNodeToFocus = initialize() - // } - // Either the previous node has been restored, a new node has been - // initialized, or the root should be used (due to no or failed initing). + const rootFallback = focusableTree.getRootFocusableNode(); this.focusNode(nodeToRestore ?? currNode ?? rootFallback); } @@ -268,7 +223,7 @@ export class FocusManager { const nextTree = focusableNode.getFocusableTree(); if (!this.isRegistered(nextTree)) { - throw new Error(`Attempted to focus unregistered node: ${focusableNode}.`); + throw Error(`Attempted to focus unregistered node: ${focusableNode}.`); } // Safety check for ensuring focusNode() doesn't get called for a node that @@ -279,7 +234,7 @@ export class FocusManager { nextTree, ); if (matchedNode !== focusableNode) { - throw new Error(`Attempting to focus node which isn't recognized by its parent tree: ${focusableNode}.`); + throw Error(`Attempting to focus node which isn't recognized by its parent tree: ${focusableNode}.`); } const prevNode = this.focusedNode; @@ -313,14 +268,6 @@ export class FocusManager { this.setNodeToActive(focusableNode); } this.focusedNode = focusableNode; - // if (registration.callbacks.Synchronize) { - // Provide the tree with an opportunity to synchronize to focus state. - // registration.callbacks.Synchronize(focusableNode); - // } - // if (prevTree && prevTree !== nextTree) { - // const blurFocus = this.lookUpRegistration(prevTree)?.callbacks?.BlurFocus; - // if (blurFocus) blurFocus(); - // } } /** @@ -345,7 +292,7 @@ export class FocusManager { focusableElement: HTMLElement | SVGElement, ): ReturnEphemeralFocus { if (this.currentlyHoldsEphemeralFocus) { - throw new Error( + throw Error( `Attempted to take ephemeral focus when it's already held, ` + `with new element: ${focusableElement}.`, ); @@ -360,7 +307,7 @@ export class FocusManager { let hasFinishedEphemeralFocus = false; return () => { if (hasFinishedEphemeralFocus) { - throw new Error( + throw Error( `Attempted to finish ephemeral focus twice for element: ` + `${focusableElement}.`, ); @@ -391,7 +338,6 @@ export class FocusManager { dom.addClass(element, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME); dom.removeClass(element, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME); element.focus(); - // this.ignoreFocusChanges = false; } private setNodeToPassive(node: IFocusableNode): void { diff --git a/core/interfaces/i_focusable_node.ts b/core/interfaces/i_focusable_node.ts index bc1c985e69f..ed0dad2e436 100644 --- a/core/interfaces/i_focusable_node.ts +++ b/core/interfaces/i_focusable_node.ts @@ -37,11 +37,34 @@ export interface IFocusableNode { */ getFocusableTree(): IFocusableTree; + /** + * Called when this node receives active focus. + * + * Note that it's fine for implementations to change visibility modifiers, but + * they should avoid the following: + * - Creating or removing DOM elements (including via the renderer or drawer). + * - Affecting focus via DOM focus() calls or the FocusManager. + */ onNodeFocus(): void; + /** + * Called when this node loses active focus. It may still have passive focus. + * + * This has the same implementation restrictions as onNodeFocus(). + */ onNodeBlur(): void; } +/** + * Determines whether the provided object fulfills the contract of + * IFocusableNode. + * + * @param object The object to test. + * @returns Whether the provided object can be used as an IFocusableNode. + */ export function isFocusableNode(object: any | null): object is IFocusableNode { - return object && 'getFocusableElement' in object && 'getFocusableTree' in object; + return object && 'getFocusableElement' in object + && 'getFocusableTree' in object + && 'onNodeFocus' in object + && 'onNodeBlur' in object; } diff --git a/core/interfaces/i_focusable_tree.ts b/core/interfaces/i_focusable_tree.ts index c2cbd016a88..96c971924ae 100644 --- a/core/interfaces/i_focusable_tree.ts +++ b/core/interfaces/i_focusable_tree.ts @@ -37,7 +37,33 @@ export interface IFocusableTree { */ getRootFocusableNode(): IFocusableNode; - getRestoredFocusableNode(previousNode: IFocusableNode | null): IFocusableNode | null; + /** + * Returns the IFocusableNode of this tree that should receive active focus + * when the tree itself has focused returned to it. + * + * There are some very important notes to consider about a tree's focus + * lifecycle when implementing a version of this method that doesn't return + * null: + * 1. A null previousNode does not guarantee first-time focus state as nodes + * can be deleted. + * 2. This method is only used when the tree itself is focused, either through + * tab navigation or via FocusManager.focusTree(). In many cases, the + * previously focused node will be directly focused instead which will + * bypass this method. + * 3. The default behavior (i.e. returning null here) involves either + * restoring the previous node (previousNode) or focusing the tree's root. + * + * This method is largely intended to provide tree implementations with the + * means of specifying a better default node than their root. + * + * @param previousNode The node that previously held passive focus for this + * tree, or null if the tree hasn't yet been focused. + * @returns The IFocusableNode that should now receive focus, or null if + * default behavior should be used, instead. + */ + getRestoredFocusableNode( + previousNode: IFocusableNode | null + ): IFocusableNode | null; /** * Returns all directly nested trees under this tree. @@ -61,11 +87,43 @@ export interface IFocusableTree { */ lookUpFocusableNode(id: string): IFocusableNode | null; + /** + * Called when a node of this tree has received active focus. + * + * See IFocusableNode.onNodeFocus() as implementations have the same + * restrictions as with that method. + * + * @param node The node receiving active focus. + * @param previousTree The previous tree that held active focus, or null if + * none. + */ onTreeFocus(node: IFocusableNode, previousTree: IFocusableTree | null): void; + /** + * Called when the previously actively focused node of this tree is now + * passively focused and there is no other active node of this tree taking its + * place. + * + * This has the same implementation restrictions as onTreeFocus(). + * + * @param nextTree The next tree receiving active focus, or null if none (such + * as in the case that Blockly is entirely losing DOM focus). + */ onTreeBlur(nextTree: IFocusableTree | null): void; } +/** + * Determines whether the provided object fulfills the contract of + * IFocusableTree. + * + * @param object The object to test. + * @returns Whether the provided object can be used as an IFocusableTree. + */ export function isFocusableTree(object: any | null): object is IFocusableTree { - return object && 'getFocusedNode' in object && 'getRootFocusableNode' in object && 'getNestedTrees' in object && 'lookUpFocusableNode' in object && 'findFocusableNodeFor' in object; + return object && 'getRootFocusableNode' in object + && 'getRestoredFocusableNode' in object + && 'getNestedTrees' in object + && 'lookUpFocusableNode' in object + && 'onTreeFocus' in object + && 'onTreeBlur' in object; } diff --git a/core/toolbox/toolbox.ts b/core/toolbox/toolbox.ts index b6d174ac9bb..a5b5b795345 100644 --- a/core/toolbox/toolbox.ts +++ b/core/toolbox/toolbox.ts @@ -22,7 +22,7 @@ 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, TreeCustomizationCallbacks} from '../focus_manager.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'; @@ -172,22 +172,6 @@ export class Toolbox ComponentManager.Capability.DRAG_TARGET, ], }); - // const customizationOptions: TreeCustomizationCallbacks = { - // Initialize: () => { - // return this.getToolboxItems().find((item) => - // item.isSelectable()) ?? null; - // }, - // Synchronize: (node: IFocusableNode) => { - // if (node !== this) { - // // Only select the item if it isn't already selected as to not toggle. - // if (this.getSelectedItem() !== node) { - // this.setSelectedItem(node as IToolboxItem); - // } - // } else this.clearSelection(); - // }, - // BlurFocus: null - // }; - // getFocusManager().registerTree(this, customizationOptions); getFocusManager().registerTree(this); } diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index b948582d2d5..0e2161d6250 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -37,7 +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, TreeCustomizationCallbacks} from './focus_manager.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'; @@ -856,23 +856,6 @@ export class WorkspaceSvg isParentWorkspace ? this.getInjectionDiv() : undefined, ); - // const customizationOptions: TreeCustomizationCallbacks = { - // Initialize: () => { - // // TODO: This doesn't handle the idea of starting in a preset location like the plugin does. - // return this.getTopBlocks(true)[0] ?? null; - // }, - // // TODO: Perhaps synchronize here could select the block? Would drastically simplify selection management in cursor (I think). - // Synchronize: null, - // BlurFocus: () => { - // // TODO: make sure this works correctly for a permanent flyout. - // // If the flyout loses focus, make sure to close it. - // if (this.isFlyout) { - // // TODO: fix this as it doesn't seem to work (it gets triggered correctly, hide() just doesn't seem to do anything). - // this.getFlyout()?.hide(); - // } - // } - // }; - // getFocusManager().registerTree(this, customizationOptions); getFocusManager().registerTree(this); return this.svgGroup_; diff --git a/tests/mocha/focus_manager_test.js b/tests/mocha/focus_manager_test.js index 4a3f6b3ad1f..a507bd395cb 100644 --- a/tests/mocha/focus_manager_test.js +++ b/tests/mocha/focus_manager_test.js @@ -27,6 +27,10 @@ class FocusableNodeImpl { getFocusableTree() { return this.tree; } + + onNodeFocus() {} + + onNodeBlur() {} } class FocusableTreeImpl { @@ -46,6 +50,8 @@ class FocusableTreeImpl { return this.rootNode; } + getRestoredFocusableNode() { return null; } + getNestedTrees() { return this.nestedTrees; } @@ -53,9 +59,13 @@ class FocusableTreeImpl { lookUpFocusableNode(id) { return this.idToNodeMap[id]; } + + onTreeFocus() {} + + onTreeBlur() {} } -suite('FocusManager', function () { +suite.only('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}`; @@ -2067,7 +2077,7 @@ suite('FocusManager', function () { ); }); - test('registered tree focus()ed other tree node passively focused tree root now has active property', function () { + test('registered tree focus()ed other tree node passively focused tree node now has active property', function () { this.focusManager.registerTree(this.testFocusableTree1); this.focusManager.registerTree(this.testFocusableTree2); document.getElementById('testFocusableTree1.node1').focus(); @@ -2075,26 +2085,27 @@ suite('FocusManager', function () { document.getElementById('testFocusableTree1').focus(); - // This differs from the behavior of focusTree() since directly focusing a tree's root will - // coerce it to now have focus. + // Directly refocusing a tree's root should have functional parity with focusTree(). That + // means the tree's previous node should now have active focus again and its root should + // have no focus indication. const rootElem = this.testFocusableTree1 .getRootFocusableNode() .getFocusableElement(); const nodeElem = this.testFocusableTree1Node1.getFocusableElement(); - assert.includesClass( - rootElem.classList, + assert.includesClass( + nodeElem.classList, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME, ); assert.notIncludesClass( - rootElem.classList, + nodeElem.classList, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME, ); assert.notIncludesClass( - nodeElem.classList, + rootElem.classList, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME, ); assert.notIncludesClass( - nodeElem.classList, + rootElem.classList, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME, ); }); @@ -3879,7 +3890,7 @@ suite('FocusManager', function () { ); }); - test('registered tree focus()ed other tree node passively focused tree root now has active property', function () { + test('registered tree focus()ed other tree node passively focused tree node now has active property', function () { this.focusManager.registerTree(this.testFocusableGroup1); this.focusManager.registerTree(this.testFocusableGroup2); document.getElementById('testFocusableGroup1.node1').focus(); @@ -3887,26 +3898,27 @@ suite('FocusManager', function () { document.getElementById('testFocusableGroup1').focus(); - // This differs from the behavior of focusTree() since directly focusing a tree's root will - // coerce it to now have focus. + // Directly refocusing a tree's root should have functional parity with focusTree(). That + // means the tree's previous node should now have active focus again and its root should + // have no focus indication. const rootElem = this.testFocusableGroup1 .getRootFocusableNode() .getFocusableElement(); const nodeElem = this.testFocusableGroup1Node1.getFocusableElement(); assert.includesClass( - rootElem.classList, + nodeElem.classList, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME, ); assert.notIncludesClass( - rootElem.classList, + nodeElem.classList, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME, ); assert.notIncludesClass( - nodeElem.classList, + rootElem.classList, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME, ); assert.notIncludesClass( - nodeElem.classList, + rootElem.classList, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME, ); }); diff --git a/tests/mocha/focusable_tree_traverser_test.js b/tests/mocha/focusable_tree_traverser_test.js index b6674573ecd..8c77104d5bc 100644 --- a/tests/mocha/focusable_tree_traverser_test.js +++ b/tests/mocha/focusable_tree_traverser_test.js @@ -25,6 +25,10 @@ class FocusableNodeImpl { getFocusableTree() { return this.tree; } + + onNodeFocus() {} + + onNodeBlur() {} } class FocusableTreeImpl { @@ -44,6 +48,8 @@ class FocusableTreeImpl { return this.rootNode; } + getRestoredFocusableNode() { return null; } + getNestedTrees() { return this.nestedTrees; } @@ -51,9 +57,13 @@ class FocusableTreeImpl { lookUpFocusableNode(id) { return this.idToNodeMap[id]; } + + onTreeFocus() {} + + onTreeBlur() {} } -suite('FocusableTreeTraverser', function () { +suite.only('FocusableTreeTraverser', function () { setup(function () { sharedTestSetup.call(this); From d5b9741aff8421d85e91abc59cb3ffb2dbbeb347 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Mon, 21 Apr 2025 20:02:20 +0000 Subject: [PATCH 05/33] chore: finalize focus infra changes pt2. This finalizes the focus infrastructural changes to make ready for review (minus tests and lint fixes). It adds callback guardrails, improves documentation, and simplifies some of the focus manager implementation. --- core/focus_manager.ts | 72 ++++++++++++++------ core/interfaces/i_focusable_node.ts | 10 ++- core/interfaces/i_focusable_tree.ts | 10 ++- tests/mocha/focus_manager_test.js | 2 +- tests/mocha/focusable_tree_traverser_test.js | 2 +- 5 files changed, 72 insertions(+), 24 deletions(-) diff --git a/core/focus_manager.ts b/core/focus_manager.ts index fffcb527ee8..b5932f10d8b 100644 --- a/core/focus_manager.ts +++ b/core/focus_manager.ts @@ -60,6 +60,7 @@ export class FocusManager { registeredTrees: Array = []; private currentlyHoldsEphemeralFocus: boolean = false; + private lockFocusStateChanges: boolean = false; constructor( addGlobalEventListener: (type: string, listener: EventListener) => void, @@ -117,6 +118,7 @@ export class FocusManager { * certain whether the tree has been registered. */ registerTree(tree: IFocusableTree): void { + this.ensureManagerIsUnlocked(); if (this.isRegistered(tree)) { throw Error(`Attempted to re-register already registered tree: ${tree}.`); } @@ -142,6 +144,7 @@ export class FocusManager { * this manager. */ unregisterTree(tree: IFocusableTree): void { + this.ensureManagerIsUnlocked(); if (!this.isRegistered(tree)) { throw Error(`Attempted to unregister not registered tree: ${tree}.`); } @@ -201,6 +204,7 @@ export class FocusManager { * focus. */ focusTree(focusableTree: IFocusableTree): void { + this.ensureManagerIsUnlocked(); if (!this.isRegistered(focusableTree)) { throw Error(`Attempted to focus unregistered tree: ${focusableTree}.`); } @@ -219,6 +223,7 @@ export class FocusManager { * @param focusableNode The node that should receive active focus. */ focusNode(focusableNode: IFocusableNode): void { + this.ensureManagerIsUnlocked(); if (this.focusedNode == focusableNode) return; // State is unchanged. const nextTree = focusableNode.getFocusableTree(); @@ -234,14 +239,17 @@ export class FocusManager { nextTree, ); if (matchedNode !== focusableNode) { - throw Error(`Attempting to focus node which isn't recognized by its parent tree: ${focusableNode}.`); + throw Error( + `Attempting to focus node which isn't recognized by its parent tree: ` + + `${focusableNode}.`); } const prevNode = this.focusedNode; const prevTree = prevNode?.getFocusableTree(); if (prevNode && prevTree !== nextTree) { - this.setNodeToPassive(prevNode); + this.passivelyFocusNode(prevNode, nextTree); } + // If there's a focused node in the new node's tree, ensure it's reset. const prevNodeNextTree = FocusableTreeTraverser.findFocusedNode(nextTree); const nextTreeRoot = nextTree.getRootFocusableNode(); @@ -255,17 +263,9 @@ export class FocusManager { this.removeHighlight(nextTreeRoot); } - // TODO: Add guardrails to prevent focus methods from being invoked in callbacks. - // TODO: Make this work correctly with ephemeral focus. - // TODO: Figure out correct ordering here, but if we allow onNodeFocus to change internal display visibility then the focus() call *must* happen after it. - if (prevTree && prevTree !== nextTree) prevTree.onTreeBlur(nextTree); - nextTree.onTreeFocus(focusableNode, prevTree ?? null); - if (prevNode) prevNode.onNodeBlur(); - focusableNode.onNodeFocus(); - if (!this.currentlyHoldsEphemeralFocus) { // Only change the actively focused node if ephemeral state isn't held. - this.setNodeToActive(focusableNode); + this.activelyFocusNode(focusableNode, prevTree ?? null); } this.focusedNode = focusableNode; } @@ -291,6 +291,7 @@ export class FocusManager { takeEphemeralFocus( focusableElement: HTMLElement | SVGElement, ): ReturnEphemeralFocus { + this.ensureManagerIsUnlocked(); if (this.currentlyHoldsEphemeralFocus) { throw Error( `Attempted to take ephemeral focus when it's already held, ` + @@ -300,7 +301,7 @@ export class FocusManager { this.currentlyHoldsEphemeralFocus = true; if (this.focusedNode) { - this.setNodeToPassive(this.focusedNode); + this.passivelyFocusNode(this.focusedNode, null); } focusableElement.focus(); @@ -316,31 +317,64 @@ export class FocusManager { this.currentlyHoldsEphemeralFocus = false; if (this.focusedNode) { - this.setNodeToActive(this.focusedNode); + this.activelyFocusNode(this.focusedNode, null); } }; } + private ensureManagerIsUnlocked(): void { + if (this.lockFocusStateChanges) { + throw Error( + 'FocusManager state changes cannot happen in a tree/node focus/blur ' + + 'callback.' + ); + } + } + private defocusCurrentFocusedNode(): void { // The current node will likely be defocused while ephemeral focus is held, // but internal manager state shouldn't change since the node should be // restored upon exiting ephemeral focus mode. if (this.focusedNode && !this.currentlyHoldsEphemeralFocus) { - this.setNodeToPassive(this.focusedNode); - this.focusedNode.getFocusableTree().onTreeBlur(null); - this.focusedNode.onNodeBlur(); + this.passivelyFocusNode(this.focusedNode, null); this.focusedNode = null; } } - private setNodeToActive(node: IFocusableNode): void { + private activelyFocusNode( + node: IFocusableNode, prevTree: IFocusableTree | null + ): void { + // Note that order matters here. Focus callbacks are allowed to change + // element visibility which can influence focusability, including for a + // 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); + node.onNodeFocus(); + this.lockFocusStateChanges = false; + + this.setNodeToVisualActiveFocus(node); + node.getFocusableElement().focus(); + } + + private passivelyFocusNode( + node: IFocusableNode, nextTree: IFocusableTree | null + ): void { + this.lockFocusStateChanges = true; + node.getFocusableTree().onTreeBlur(nextTree); + node.onNodeBlur(); + this.lockFocusStateChanges = false; + + this.setNodeToVisualPassiveFocus(node); + } + + private setNodeToVisualActiveFocus(node: IFocusableNode): void { const element = node.getFocusableElement(); dom.addClass(element, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME); dom.removeClass(element, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME); - element.focus(); } - private setNodeToPassive(node: IFocusableNode): void { + private setNodeToVisualPassiveFocus(node: IFocusableNode): void { const element = node.getFocusableElement(); dom.removeClass(element, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME); dom.addClass(element, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME); diff --git a/core/interfaces/i_focusable_node.ts b/core/interfaces/i_focusable_node.ts index ed0dad2e436..20471f2873f 100644 --- a/core/interfaces/i_focusable_node.ts +++ b/core/interfaces/i_focusable_node.ts @@ -25,8 +25,14 @@ export interface IFocusableNode { * and a tab index must be present in order for the element to be focusable in * the DOM). * - * It's expected the return element will not change for the lifetime of the - * node. + * The returned element must be visible if the node is ever focused via + * FocusManager.focusNode() or FocusManager.focusTree(). It's allowed for an + * element to be hidden until onNodeFocus() is called, or become hidden with a + * call to onNodeBlur(). + * + * It's expected the actual returned element will not change for the lifetime + * of the node (that is, its properties can change but a new element should + * never be returned.) */ getFocusableElement(): HTMLElement | SVGElement; diff --git a/core/interfaces/i_focusable_tree.ts b/core/interfaces/i_focusable_tree.ts index 96c971924ae..2e3d6894eff 100644 --- a/core/interfaces/i_focusable_tree.ts +++ b/core/interfaces/i_focusable_tree.ts @@ -90,6 +90,13 @@ export interface IFocusableTree { /** * Called when a node of this tree has received active focus. * + * Note that a null previousTree does not necessarily indicate that this is + * the first time Blockly is receiving focus. In fact, few assumptions can be + * made about previous focus state as a previous null tree simply indicates + * that Blockly did not hold active focus prior to this tree becoming focused + * (which can happen due to focus exiting the Blockly injection div, or for + * other cases like ephemeral focus). + * * See IFocusableNode.onNodeFocus() as implementations have the same * restrictions as with that method. * @@ -104,7 +111,8 @@ export interface IFocusableTree { * passively focused and there is no other active node of this tree taking its * place. * - * This has the same implementation restrictions as onTreeFocus(). + * This has the same implementation restrictions and considerations as + * onTreeFocus(). * * @param nextTree The next tree receiving active focus, or null if none (such * as in the case that Blockly is entirely losing DOM focus). diff --git a/tests/mocha/focus_manager_test.js b/tests/mocha/focus_manager_test.js index a507bd395cb..26d4d991f76 100644 --- a/tests/mocha/focus_manager_test.js +++ b/tests/mocha/focus_manager_test.js @@ -65,7 +65,7 @@ class FocusableTreeImpl { onTreeBlur() {} } -suite.only('FocusManager', function () { +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}`; diff --git a/tests/mocha/focusable_tree_traverser_test.js b/tests/mocha/focusable_tree_traverser_test.js index 8c77104d5bc..de565249319 100644 --- a/tests/mocha/focusable_tree_traverser_test.js +++ b/tests/mocha/focusable_tree_traverser_test.js @@ -63,7 +63,7 @@ class FocusableTreeImpl { onTreeBlur() {} } -suite.only('FocusableTreeTraverser', function () { +suite('FocusableTreeTraverser', function () { setup(function () { sharedTestSetup.call(this); From e9a30c05eabcd7a146abae6378abe2b47cb33afe Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Mon, 21 Apr 2025 22:23:10 +0000 Subject: [PATCH 06/33] chore: another attempt to fix CSS inheritance. --- core/css.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/core/css.ts b/core/css.ts index 23d570c429f..dd59b88f594 100644 --- a/core/css.ts +++ b/core/css.ts @@ -510,4 +510,10 @@ input[type=number] { stroke-width: 3; outline-color: #ffa200; } + +.blocklyMainBackground, .blocklyActiveFocus > * { + // Reset to blocklyMainBackground's initial properties to avoid inheriting these. + stroke-width: 1; + stroke: #c6c6c6; +} `; From 41c8215465a538edd59ffefae067c920c1ce147d Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Mon, 21 Apr 2025 23:26:46 +0000 Subject: [PATCH 07/33] 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 08/33] 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 09/33] 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 82bbab6d47c3fea1441d0e323e1dc9c0d8dcd31d Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 22 Apr 2025 19:32:25 +0000 Subject: [PATCH 10/33] Fix things & add ARIA labels for demo. --- core/block_svg.ts | 1 + core/css.ts | 10 +++++----- core/field.ts | 3 ++- core/flyout_base.ts | 4 +--- core/rendered_connection.ts | 5 ++++- core/toolbox/category.ts | 1 + core/toolbox/separator.ts | 1 + core/toolbox/toolbox.ts | 1 + core/workspace_svg.ts | 3 +-- 9 files changed, 17 insertions(+), 12 deletions(-) diff --git a/core/block_svg.ts b/core/block_svg.ts index 7b11e142148..342587314ac 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -214,6 +214,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; + svgPath.setAttribute('aria-label', this.type ? '"' + this.type + '" block' : 'Block'); this.doInit_(); } diff --git a/core/css.ts b/core/css.ts index dd59b88f594..1fd03598d4e 100644 --- a/core/css.ts +++ b/core/css.ts @@ -511,9 +511,9 @@ input[type=number] { outline-color: #ffa200; } -.blocklyMainBackground, .blocklyActiveFocus > * { - // Reset to blocklyMainBackground's initial properties to avoid inheriting these. - stroke-width: 1; - stroke: #c6c6c6; -} +// .blocklyMainBackground, .blocklyActiveFocus > * { +// // Reset to blocklyMainBackground's initial properties to avoid inheriting these. +// stroke-width: 1; +// stroke: #c6c6c6; +// } `; diff --git a/core/field.ts b/core/field.ts index e4fe86095c7..e1906b5843c 100644 --- a/core/field.ts +++ b/core/field.ts @@ -309,7 +309,8 @@ export abstract class Field if (!id) throw new Error('Expected ID to be defined prior to init.'); this.fieldGroup_ = dom.createSvgElement(Svg.G, { 'tabindex': '-1', - 'id': id + 'id': id, + 'aria-label': 'Field ' + this.name, }); if (!this.isVisible()) { this.fieldGroup_.style.display = 'none'; diff --git a/core/flyout_base.ts b/core/flyout_base.ts index 945ac66f2c3..5e473ce90bc 100644 --- a/core/flyout_base.ts +++ b/core/flyout_base.ts @@ -12,8 +12,6 @@ // Former goog.module ID: Blockly.Flyout import {BlockSvg} from './block_svg.js'; -import { IFocusableNode, isFocusableNode } from './interfaces/i_focusable_node.js'; -import { IFocusableTree } from './interfaces/i_focusable_tree.js'; import * as browserEvents from './browser_events.js'; import {ComponentManager} from './component_manager.js'; import {DeleteArea} from './delete_area.js'; @@ -43,7 +41,6 @@ import {Svg} from './utils/svg.js'; import * as toolbox from './utils/toolbox.js'; import * as Variables from './variables.js'; import {WorkspaceSvg} from './workspace_svg.js'; -import { getFocusManager } from './focus_manager.js'; /** * Class for a flyout. @@ -311,6 +308,7 @@ export abstract class Flyout this.svgGroup_ = dom.createSvgElement(tagName, { 'class': 'blocklyFlyout', 'tabindex': '0', + 'aria-label': 'Flyout', }); this.svgGroup_.style.display = 'none'; this.svgBackground_ = dom.createSvgElement( diff --git a/core/rendered_connection.ts b/core/rendered_connection.ts index c32688d578a..247dc7db480 100644 --- a/core/rendered_connection.ts +++ b/core/rendered_connection.ts @@ -692,7 +692,10 @@ export class RenderedConnection getFocusableElement(): HTMLElement | SVGElement { const highlightSvg = this.findHighlightSvg(); - if (highlightSvg) return highlightSvg; + if (highlightSvg) { + highlightSvg.setAttribute('aria-label', 'Connection'); + return highlightSvg; + } throw new Error('No highlight SVG found corresponding to this connection.'); } diff --git a/core/toolbox/category.ts b/core/toolbox/category.ts index fc7d1aa03cf..f213f75b623 100644 --- a/core/toolbox/category.ts +++ b/core/toolbox/category.ts @@ -192,6 +192,7 @@ export class ToolboxCategory aria.setRole(this.htmlDiv_, aria.Role.TREEITEM); aria.setState(this.htmlDiv_, aria.State.SELECTED, false); aria.setState(this.htmlDiv_, aria.State.LEVEL, this.level_ + 1); + this.htmlDiv_.setAttribute('aria-label', 'Category ' + this.name_); this.rowDiv_ = this.createRowContainer_(); this.rowDiv_.style.pointerEvents = 'auto'; diff --git a/core/toolbox/separator.ts b/core/toolbox/separator.ts index 44ae358cf53..08602033399 100644 --- a/core/toolbox/separator.ts +++ b/core/toolbox/separator.ts @@ -61,6 +61,7 @@ export class ToolboxSeparator extends ToolboxItem { dom.addClass(container, className); } this.htmlDiv = container; + this.htmlDiv.setAttribute('aria-label', 'Separator'); return container; } diff --git a/core/toolbox/toolbox.ts b/core/toolbox/toolbox.ts index cc1f4c6831d..f3497be611f 100644 --- a/core/toolbox/toolbox.ts +++ b/core/toolbox/toolbox.ts @@ -149,6 +149,7 @@ export class Toolbox this.flyout = this.createFlyout_(); this.HtmlDiv = this.createDom_(this.workspace_); + this.HtmlDiv.setAttribute('aria-label', 'Toolbox'); const flyoutDom = this.flyout.createDom('svg'); dom.addClass(flyoutDom, 'blocklyToolboxFlyout'); dom.insertAfter(flyoutDom, svg); diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index 0512e903ec7..4c128f4266a 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -90,8 +90,6 @@ import {Workspace} from './workspace.js'; import {WorkspaceAudio} from './workspace_audio.js'; import {ZoomControls} from './zoom_controls.js'; import type {Field} from './field.js'; -import * as aria from './utils/aria.js'; -import {Msg} from './msg.js'; import { IAutoHideable } from './blockly.js'; /** Margin around the top/bottom/left/right after a zoomToFit call. */ @@ -779,6 +777,7 @@ export class WorkspaceSvg // Only the main workspace should be tabbable. 'tabindex': injectionDiv ? '0' : '-1', 'id': this.id, + 'aria-label': 'Workspace', }); if (injectionDiv) { aria.setState( From d3acbff46f7826c78fcaaff2d8d28639aaf26a3f Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 22 Apr 2025 21:15:41 +0000 Subject: [PATCH 11/33] 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 12/33] 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 13/33] 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 14/33] 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 15/33] 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 16/33] 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 17/33] 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 18/33] 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 19/33] 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 20/33] 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 21/33] 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 22/33] 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 23/33] 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 24/33] 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 25/33] 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 8df19a769d0ac501ac7e41412ac0962a802b52db Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 24 Apr 2025 21:12:06 +0000 Subject: [PATCH 26/33] chore: lint fixes for better diffing. --- core/block_svg.ts | 5 ++++- core/blockly.ts | 10 ++++++++-- core/field_input.ts | 10 +++++++--- core/flyout_base.ts | 1 - core/keyboard_nav/line_cursor.ts | 10 +++++----- core/rendered_connection.ts | 4 ++-- core/toolbox/toolbox.ts | 1 - core/workspace_svg.ts | 3 --- 8 files changed, 26 insertions(+), 18 deletions(-) diff --git a/core/block_svg.ts b/core/block_svg.ts index 342587314ac..5a39351b1e5 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -214,7 +214,10 @@ 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; - svgPath.setAttribute('aria-label', this.type ? '"' + this.type + '" block' : 'Block'); + svgPath.setAttribute( + 'aria-label', + this.type ? '"' + this.type + '" block' : 'Block', + ); this.doInit_(); } diff --git a/core/blockly.ts b/core/blockly.ts index 6774a450f24..871035e679b 100644 --- a/core/blockly.ts +++ b/core/blockly.ts @@ -145,8 +145,14 @@ import { } from './interfaces/i_draggable.js'; import {IDragger} from './interfaces/i_dragger.js'; import {IFlyout} from './interfaces/i_flyout.js'; -import {IFocusableNode, isFocusableNode} from './interfaces/i_focusable_node.js'; -import {IFocusableTree, isFocusableTree} from './interfaces/i_focusable_tree.js'; +import { + IFocusableNode, + isFocusableNode, +} from './interfaces/i_focusable_node.js'; +import { + IFocusableTree, + isFocusableTree, +} from './interfaces/i_focusable_tree.js'; import {IHasBubble, hasBubble} from './interfaces/i_has_bubble.js'; import {IIcon, isIcon} from './interfaces/i_icon.js'; import {IKeyboardAccessible} from './interfaces/i_keyboard_accessible.js'; diff --git a/core/field_input.ts b/core/field_input.ts index d13b58214cc..a612e7da50a 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. @@ -352,7 +352,9 @@ export abstract class FieldInput extends Field< * keyboards). */ private showPromptEditor() { - const returnFocusCallback = getFocusManager().takeEphemeralFocus(document.body); + const returnFocusCallback = getFocusManager().takeEphemeralFocus( + document.body, + ); dialog.prompt( Msg['CHANGE_VALUE_TITLE'], this.getText(), @@ -377,7 +379,9 @@ export abstract class FieldInput extends Field< if (!block) { throw new UnattachedFieldError(); } - const returnFocusCallback = getFocusManager().takeEphemeralFocus(document.body); + const returnFocusCallback = getFocusManager().takeEphemeralFocus( + document.body, + ); WidgetDiv.show( this, block.RTL, diff --git a/core/flyout_base.ts b/core/flyout_base.ts index 5e473ce90bc..599f750827a 100644 --- a/core/flyout_base.ts +++ b/core/flyout_base.ts @@ -35,7 +35,6 @@ import {SEPARATOR_TYPE} from './separator_flyout_inflater.js'; import * as blocks from './serialization/blocks.js'; import {Coordinate} from './utils/coordinate.js'; import * as dom from './utils/dom.js'; -import { FocusableTreeTraverser } from './utils/focusable_tree_traverser.js'; import * as idGenerator from './utils/idgenerator.js'; import {Svg} from './utils/svg.js'; import * as toolbox from './utils/toolbox.js'; diff --git a/core/keyboard_nav/line_cursor.ts b/core/keyboard_nav/line_cursor.ts index 73c1e400ed8..1812307927a 100644 --- a/core/keyboard_nav/line_cursor.ts +++ b/core/keyboard_nav/line_cursor.ts @@ -23,6 +23,8 @@ 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'; @@ -31,8 +33,6 @@ import * as dom from '../utils/dom.js'; import type {WorkspaceSvg} from '../workspace_svg.js'; import {ASTNode} from './ast_node.js'; import {Marker} from './marker.js'; -import {isFocusableNode} from '../interfaces/i_focusable_node.js'; -import {getFocusManager} from '../focus_manager.js'; /** Options object for LineCursor instances. */ export interface CursorOptions { @@ -574,7 +574,7 @@ export class LineCursor extends Marker { * @param updateSelection If true (the default) we'll update the selection * too. */ - override setCurNode(newNode: ASTNode | null, selectionUpToDate = false) { + override setCurNode(newNode: ASTNode | null, _selectionUpToDate = false) { // if (!selectionUpToDate) { // this.updateSelectionFromNode(newNode); // } @@ -649,7 +649,7 @@ export class LineCursor extends Marker { // If old node was a block, unselect it or remove fake selection. // if (oldNode?.getType() === ASTNode.types.BLOCK) { // const block = oldNode.getLocation() as BlockSvg; - // if (!block.isShadow()) { + // if (!block.isShadow()) { // // Selection should already be in sync. // } else { // block.removeSelect(); @@ -664,7 +664,7 @@ export class LineCursor extends Marker { const isZelosInputConnection = this.isZelos && curNode && this.isValueInputConnection(curNode); - // realDrawer.draw(oldNode, curNode); + // realDrawer.draw(oldNode, curNode); // If drawing can't be handled locally, just use the drawer. if (curNodeType !== ASTNode.types.BLOCK && !isZelosInputConnection) { diff --git a/core/rendered_connection.ts b/core/rendered_connection.ts index ede17733ae7..5355033ea12 100644 --- a/core/rendered_connection.ts +++ b/core/rendered_connection.ts @@ -17,13 +17,13 @@ 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'; diff --git a/core/toolbox/toolbox.ts b/core/toolbox/toolbox.ts index 6fd7e486377..2550ab08431 100644 --- a/core/toolbox/toolbox.ts +++ b/core/toolbox/toolbox.ts @@ -42,7 +42,6 @@ import type {KeyboardShortcut} from '../shortcut_registry.js'; import * as Touch from '../touch.js'; import * as aria from '../utils/aria.js'; import * as dom from '../utils/dom.js'; -import {FocusableTreeTraverser} from '../utils/focusable_tree_traverser.js'; import {Rect} from '../utils/rect.js'; import * as toolbox from '../utils/toolbox.js'; import type {WorkspaceSvg} from '../workspace_svg.js'; diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index 6f83068ab32..14782eb2e04 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -75,7 +75,6 @@ import * as arrayUtils from './utils/array.js'; import {Coordinate} from './utils/coordinate.js'; import * as dom from './utils/dom.js'; import * as drag from './utils/drag.js'; -import {FocusableTreeTraverser} from './utils/focusable_tree_traverser.js'; import type {Metrics} from './utils/metrics.js'; import {Rect} from './utils/rect.js'; import {Size} from './utils/size.js'; @@ -89,8 +88,6 @@ import * as WidgetDiv from './widgetdiv.js'; import {Workspace} from './workspace.js'; import {WorkspaceAudio} from './workspace_audio.js'; import {ZoomControls} from './zoom_controls.js'; -import type {Field} from './field.js'; -import { IAutoHideable } from './blockly.js'; /** Margin around the top/bottom/left/right after a zoomToFit call. */ const ZOOM_TO_FIT_MARGIN = 20; From 129e417e90d3b24d579ebcbe68d3794a1e7a3f55 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 24 Apr 2025 21:14:08 +0000 Subject: [PATCH 27/33] chore: remove unnecessary field ephemeral focus. --- core/field_input.ts | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/core/field_input.ts b/core/field_input.ts index a612e7da50a..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'; @@ -352,9 +351,6 @@ export abstract class FieldInput extends Field< * keyboards). */ private showPromptEditor() { - const returnFocusCallback = getFocusManager().takeEphemeralFocus( - document.body, - ); dialog.prompt( Msg['CHANGE_VALUE_TITLE'], this.getText(), @@ -364,7 +360,6 @@ export abstract class FieldInput extends Field< this.setValue(this.getValueFromEditorText_(text)); } this.onFinishEditing_(this.value_); - returnFocusCallback(); }, ); } @@ -379,16 +374,10 @@ export abstract class FieldInput extends Field< if (!block) { throw new UnattachedFieldError(); } - const returnFocusCallback = getFocusManager().takeEphemeralFocus( - document.body, - ); WidgetDiv.show( this, block.RTL, - () => { - this.widgetDispose_(); - returnFocusCallback(); - }, + this.widgetDispose_.bind(this), this.workspace_, ); this.htmlInput_ = this.widgetCreate_() as HTMLInputElement; From a346a920a900abd35da979391b88d868916e12f6 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 24 Apr 2025 21:25:16 +0000 Subject: [PATCH 28/33] 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 29/33] 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 30/33] chore: empty commit to make CI pass. Not really necessary, but I like seeing green CI before merging. From 23c1b8f482fa80fef457f20e34431f1b14c9beae Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 24 Apr 2025 23:25:08 +0000 Subject: [PATCH 31/33] fix: CSS issues. Basically puts CSS in a pretty good state for both focus & selection, and fixes a FocusManager issue (oops). There's still a lot more to do here around draw order that's going to be rather tricky to fix, but further discussion is needed. --- core/css.ts | 53 ++++++++++++++++++------------ core/focus_manager.ts | 15 ++++++--- core/renderers/common/constants.ts | 6 ---- 3 files changed, 42 insertions(+), 32 deletions(-) diff --git a/core/css.ts b/core/css.ts index 1fd03598d4e..eeda1948c3c 100644 --- a/core/css.ts +++ b/core/css.ts @@ -157,15 +157,6 @@ let content = ` stroke-width: 1; } -.blocklySelected { - stroke: #ffa200; - stroke-width: 5; -} - -.blocklySelected>.blocklyPathLight { - display: none; -} - .blocklyDraggable { cursor: grab; cursor: -webkit-grab; @@ -499,21 +490,41 @@ input[type=number] { cursor: grabbing; } -.blocklyActiveFocus { +.blocklyActiveFocus:is(.blocklyField,.blocklyPath,.blocklyHighlightedConnectionPath) { + stroke: #ffa200; + stroke-width: 3px; + outline-width: 0px; +} +.blocklyActiveFocus > .blocklyFlyoutBackground, .blocklyActiveFocus > .blocklyMainBackground { + stroke: #ffa200; + stroke-width: 3px; +} +.blocklyActiveFocus:is(.blocklyFlyout,.blocklyWorkspace) { + outline-width: 0px; +} +.blocklyActiveFocus:is(.blocklyToolbox,.blocklyToolboxCategoryContainer) { + outline: 3px solid #ffa200; +} + +.blocklyPassiveFocus:is(.blocklyField,.blocklyPath,.blocklyHighlightedConnectionPath) { stroke: #ffa200; - stroke-width: 3; - outline-color: #ffa200; + stroke-dasharray: 5px 3px; + stroke-width: 3px; } -.blocklyPassiveFocus { +.blocklyPassiveFocus > .blocklyFlyoutBackground, .blocklyPassiveFocus > .blocklyMainBackground { stroke: #ffa200; - stroke-dasharray: 5 3; - stroke-width: 3; - outline-color: #ffa200; + stroke-dasharray: 5px 3px; + stroke-width: 3px; +} +.blocklyPassiveFocus:is(.blocklyToolbox,.blocklyToolboxCategoryContainer) { + border: 3px dashed #ffa200; } -// .blocklyMainBackground, .blocklyActiveFocus > * { -// // Reset to blocklyMainBackground's initial properties to avoid inheriting these. -// stroke-width: 1; -// stroke: #c6c6c6; -// } +.blocklySelected:is(.blocklyPath) { + stroke: #ffa200; + stroke-width: 5; +} +.blocklySelected>.blocklyPathLight { + display: none; +} `; diff --git a/core/focus_manager.ts b/core/focus_manager.ts index 88eef46b530..71fb04cfd91 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,14 @@ 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); + } } /** diff --git a/core/renderers/common/constants.ts b/core/renderers/common/constants.ts index c8f770cc407..c3ac17ca30b 100644 --- a/core/renderers/common/constants.ts +++ b/core/renderers/common/constants.ts @@ -1196,12 +1196,6 @@ export class ConstantProvider { `font-weight: ${this.FIELD_TEXT_FONTWEIGHT};`, `}`, - // Selection highlight. - `${selector} .blocklySelected>.blocklyPath {`, - `stroke: #fc3;`, - `stroke-width: 3px;`, - `}`, - // Connection highlight. // `${selector} .blocklyHighlightedConnectionPath {`, // `stroke: #fc3;`, From 28d5057f77e314771a9d2d4d634a981aa30ea37b Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Fri, 25 Apr 2025 00:10:26 +0000 Subject: [PATCH 32/33] fix: ensure click & focus sync. This isn't comprehensive yet, but it covers a lot of cases. --- core/gesture.ts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/core/gesture.ts b/core/gesture.ts index fc23ba7ca15..ebe51ceaa89 100644 --- a/core/gesture.ts +++ b/core/gesture.ts @@ -36,6 +36,7 @@ import * as Touch from './touch.js'; import {Coordinate} from './utils/coordinate.js'; import {WorkspaceDragger} from './workspace_dragger.js'; import type {WorkspaceSvg} from './workspace_svg.js'; +import { getFocusManager } from './focus_manager.js'; /** * Note: In this file "start" refers to pointerdown @@ -289,7 +290,7 @@ export class Gesture { // The start block is no longer relevant, because this is a drag. this.startBlock = null; this.targetBlock = this.flyout.createBlock(this.targetBlock); - common.setSelected(this.targetBlock); + getFocusManager().focusNode(this.targetBlock); return true; } return false; @@ -726,6 +727,7 @@ export class Gesture { if (this.targetBlock) { this.bringBlockToFront(); this.targetBlock.workspace.hideChaff(!!this.flyout); + getFocusManager().focusNode(this.targetBlock); this.targetBlock.showContextMenu(e); } else if (this.startBubble) { this.startBubble.showContextMenu(e); @@ -734,6 +736,7 @@ export class Gesture { this.startComment.showContextMenu(e); } else if (this.startWorkspace_ && !this.flyout) { this.startWorkspace_.hideChaff(); + getFocusManager().focusNode(this.startWorkspace_); this.startWorkspace_.showContextMenu(e); } @@ -762,9 +765,10 @@ export class Gesture { this.mostRecentEvent = e; if (!this.startBlock && !this.startBubble && !this.startComment) { - // Selection determines what things start drags. So to drag the workspace, - // we need to deselect anything that was previously selected. - common.setSelected(null); + // Ensure the workspace is selected if nothing else should be. + getFocusManager().focusNode(ws); + } else if (this.startBlock) { + getFocusManager().focusNode(this.startBlock); } this.doStart(e); @@ -901,6 +905,7 @@ export class Gesture { const newBlock = this.flyout.createBlock(this.targetBlock); newBlock.snapToGrid(); newBlock.bumpNeighbours(); + getFocusManager().focusNode(newBlock); } } else { if (!this.startWorkspace_) { @@ -916,6 +921,9 @@ export class Gesture { 'block', ); eventUtils.fire(event); + if (this.targetBlock) { + getFocusManager().focusNode(this.targetBlock); + } } this.bringBlockToFront(); eventUtils.setGroup(false); @@ -1023,7 +1031,6 @@ export class Gesture { // If the gesture already went through a bubble, don't set the start block. if (!this.startBlock && !this.startBubble) { this.startBlock = block; - common.setSelected(this.startBlock); if (block.isInFlyout && block !== block.getRootBlock()) { this.setTargetBlock(block.getRootBlock()); } else { From 3e6a9953a124181d8bbdb35a7999ee9dce3c6a50 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Mon, 28 Apr 2025 20:15:03 +0000 Subject: [PATCH 33/33] chore: remove a bunch of unused code. Also, reenable highlights for connections. This is not yet a final solution. --- core/rendered_connection.ts | 58 +++++-------------------------- core/renderers/common/drawer.ts | 2 +- core/renderers/zelos/constants.ts | 6 ++-- 3 files changed, 12 insertions(+), 54 deletions(-) diff --git a/core/rendered_connection.ts b/core/rendered_connection.ts index 5355033ea12..78f2e9392a8 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,20 +326,20 @@ export class RenderedConnection /** Add highlighting around this connection. */ highlight() { this.highlighted = true; - const _highlightSvg = this.findHighlightSvg(); - // if (highlightSvg) { - // highlightSvg.style.display = ''; - // } + const highlightSvg = this.findHighlightSvg(); + if (highlightSvg) { + highlightSvg.style.display = ''; + } // this.getSourceBlock().queueRender(); } /** Remove the highlighting around this connection. */ unhighlight() { this.highlighted = false; - const _highlightSvg = this.findHighlightSvg(); - // if (highlightSvg) { - // highlightSvg.style.display = 'none'; - // } + const highlightSvg = this.findHighlightSvg(); + if (highlightSvg) { + highlightSvg.style.display = 'none'; + } // this.getSourceBlock().queueRender(); } diff --git a/core/renderers/common/drawer.ts b/core/renderers/common/drawer.ts index f805451a725..84766b90f57 100644 --- a/core/renderers/common/drawer.ts +++ b/core/renderers/common/drawer.ts @@ -437,7 +437,7 @@ export class Drawer { const highlightSvg = this.drawConnectionHighlightPath(elem); if (highlightSvg) { - // highlightSvg.style.display = elem.highlighted ? '' : 'none'; + highlightSvg.style.display = elem.highlighted ? '' : 'none'; } // if (elem.highlighted) { // this.drawConnectionHighlightPath(elem); 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 {`,