From 0cbcc3144a33bf1f9be9fc00e1fa828292f4c436 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 30 Apr 2025 16:39:03 -0700 Subject: [PATCH] feat: Make connections focusable (#8928) ## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes #8930 Fixes part of #8771 ### Proposed Changes This PR introduces support for connections to be focusable (and thus navigable with keyboard navigation when paired with downstream changes to `LineCursor` and the keyboard navigation plugin). This is a largely isolated change in how it fundamentally works: - `RenderedConnection` has been updated to be an `IFocusableNode` using a new unique ID maintained by `Connection` and automatically enabling/disabling the connection highlight based on whether it's focused (per keyboard navigation). - The way that rendering works here has changed: rather than recreating the connection's highlight SVG each time, it's only created once and updated thereafter to ensure that it correctly fits block resizes or movements. Visibility of the highlight is controlled entirely through display visibility and can now be done synchronously (which was a requirement for focusability as only displayed elements can be focused). - This employs the same type of ID schema strategy as fields in #8923. ### Reason for Changes This is part of an ongoing effort to ensure key components of Blockly are focusable so that they can be keyboard-navigable (with other needed changes yet both in Core Blockly and the keyboard navigation plugin). ### Test Coverage No new tests have been added. It's certainly possible to add unit tests for the focusable configurations being introduced in this PR, but it may not be highly beneficial. It's largely assumed that the individual implementations should work due to a highly tested FocusManager, and it may be the case that the interactions of the components working together is far more important to verify (that is, the end user flows). The latter is planned to be tackled as part of #8915. ### Documentation No documentation changes should be needed here. ### Additional Information This includes changes that have been pulled from #8875. --- core/connection.ts | 5 +++ core/rendered_connection.ts | 57 ++++++++++++++++++++++++-- core/renderers/common/drawer.ts | 13 +++--- core/renderers/common/i_path_object.ts | 2 +- core/renderers/common/path_object.ts | 36 ++++++++-------- core/renderers/zelos/drawer.ts | 9 ++-- core/workspace_svg.ts | 10 +++++ 7 files changed, 96 insertions(+), 36 deletions(-) diff --git a/core/connection.ts b/core/connection.ts index 039d8822c..aed90e7c7 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,9 @@ export class Connection implements IASTNodeLocationWithBlock { /** DOM representation of a shadow block, or null if none. */ private shadowDom: Element | null = null; + /** The unique ID of this connection. */ + id: string; + /** * Horizontal location of this connection. * @@ -80,6 +84,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 168e59744..7ada1b9f6 100644 --- a/core/rendered_connection.ts +++ b/core/rendered_connection.ts @@ -22,10 +22,13 @@ 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 {Coordinate} from './utils/coordinate.js'; import * as svgMath from './utils/svg_math.js'; +import {WorkspaceSvg} from './workspace_svg.js'; /** Maximum randomness in workspace units for bumping a block. */ const BUMP_RANDOMNESS = 10; @@ -33,7 +36,10 @@ 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; @@ -320,13 +326,28 @@ export class RenderedConnection extends Connection implements IContextMenu { /** Add highlighting around this connection. */ highlight() { this.highlighted = true; - this.getSourceBlock().queueRender(); + + // Note that this needs to be done synchronously (vs. queuing a render pass) + // since only a displayed element can be focused, and this focusable node is + // implemented to make itself visible immediately prior to receiving DOM + // focus. It's expected that the connection's position should already be + // correct by this point (otherwise it will be corrected in a subsequent + // draw pass). + const highlightSvg = this.findHighlightSvg(); + if (highlightSvg) { + highlightSvg.style.display = ''; + } } /** Remove the highlighting around this connection. */ unhighlight() { this.highlighted = false; - this.getSourceBlock().queueRender(); + + // Note that this is done synchronously for parity with highlight(). + const highlightSvg = this.findHighlightSvg(); + if (highlightSvg) { + highlightSvg.style.display = 'none'; + } } /** Returns true if this connection is highlighted, false otherwise. */ @@ -626,6 +647,36 @@ 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) 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(); + } + + 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/drawer.ts b/core/renderers/common/drawer.ts index 09320710c..7046406ad 100644 --- a/core/renderers/common/drawer.ts +++ b/core/renderers/common/drawer.ts @@ -435,19 +435,16 @@ export class Drawer { for (const elem of row.elements) { if (!(elem instanceof Connection)) continue; - if (elem.highlighted) { - this.drawConnectionHighlightPath(elem); - } else { - this.block_.pathObject.removeConnectionHighlight?.( - elem.connectionModel, - ); + const highlightSvg = this.drawConnectionHighlightPath(elem); + if (highlightSvg) { + highlightSvg.style.display = elem.highlighted ? '' : 'none'; } } } } /** Returns a path to highlight the given connection. */ - drawConnectionHighlightPath(measurable: Connection) { + drawConnectionHighlightPath(measurable: Connection): SVGElement | undefined { const conn = measurable.connectionModel; let path = ''; if ( @@ -459,7 +456,7 @@ export class Drawer { path = this.getStatementConnectionHighlightPath(measurable); } const block = conn.getSourceBlock(); - block.pathObject.addConnectionHighlight?.( + return block.pathObject.addConnectionHighlight?.( conn, path, conn.getOffsetInBlock(), diff --git a/core/renderers/common/i_path_object.ts b/core/renderers/common/i_path_object.ts index 699f1d92e..776ba0067 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 72cf2a594..ed2bb7dda 100644 --- a/core/renderers/common/path_object.ts +++ b/core/renderers/common/path_object.ts @@ -268,37 +268,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 transformation = + `translate(${offset.x}, ${offset.y})` + (rtl ? ' scale(-1 1)' : ''); + + const previousHighlight = this.connectionHighlights.get(connection); + if (previousHighlight) { + // Since a connection already exists, make sure that its path and + // transform are correct. + previousHighlight.setAttribute('d', connectionPath); + previousHighlight.setAttribute('transform', transformation); + return previousHighlight; } const highlight = dom.createSvgElement( Svg.PATH, { + 'id': connection.id, 'class': 'blocklyHighlightedConnectionPath', + 'style': 'display: none;', + 'tabindex': '-1', 'd': connectionPath, - 'transform': - `translate(${offset.x}, ${offset.y})` + (rtl ? ' scale(-1 1)' : ''), + 'transform': transformation, }, this.svgRoot, ); this.connectionHighlights.set(connection, highlight); - } - - private currentHighlightMatchesNew( - connection: RenderedConnection, - newPath: string, - newOffset: Coordinate, - ): boolean { - const currPath = this.connectionHighlights - .get(connection) - ?.getAttribute('d'); - const currOffset = this.highlightOffsets.get(connection); - return currPath === newPath && Coordinate.equals(currOffset, newOffset); + return highlight; } /** diff --git a/core/renderers/zelos/drawer.ts b/core/renderers/zelos/drawer.ts index 5cc52c0cb..b38711eb6 100644 --- a/core/renderers/zelos/drawer.ts +++ b/core/renderers/zelos/drawer.ts @@ -234,15 +234,16 @@ export class Drawer extends BaseDrawer { } /** Returns a path to highlight the given connection. */ - drawConnectionHighlightPath(measurable: Connection) { + override drawConnectionHighlightPath( + measurable: Connection, + ): SVGElement | undefined { const conn = measurable.connectionModel; if ( conn.type === ConnectionType.NEXT_STATEMENT || conn.type === ConnectionType.PREVIOUS_STATEMENT || (conn.type === ConnectionType.OUTPUT_VALUE && !measurable.isDynamicShape) ) { - super.drawConnectionHighlightPath(measurable); - return; + return super.drawConnectionHighlightPath(measurable); } let path = ''; @@ -261,7 +262,7 @@ export class Drawer extends BaseDrawer { (output.shape as DynamicShape).pathDown(output.height); } const block = conn.getSourceBlock(); - block.pathObject.addConnectionHighlight?.( + return block.pathObject.addConnectionHighlight?.( conn, path, conn.getOffsetInBlock(), diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index ee526bf8b..51992fcf0 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -2710,6 +2710,7 @@ export class WorkspaceSvg } const fieldIndicatorIndex = id.indexOf('_field_'); + const connectionIndicatorIndex = id.indexOf('_connection_'); if (fieldIndicatorIndex !== -1) { const blockId = id.substring(0, fieldIndicatorIndex); const block = this.getBlockById(blockId); @@ -2719,6 +2720,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;