From 5bc83808bfe119a98de917f08840997dc9d3c324 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 24 Apr 2025 15:08:18 -0700 Subject: [PATCH 01/11] feat: Make toolbox and flyout focusable (#8920) ## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes #8918 Fixes #8919 Fixes part of #8771 ### Proposed Changes This updates several classes in order to make toolboxes and flyouts focusable: - `IFlyout` is now an `IFocusableTree` with corresponding implementations in `FlyoutBase`. - `IToolbox` is now an `IFocusableTree` with corresponding implementations in `Toolbox`. - `IToolboxItem` is now an `IFocusableNode` with corresponding implementations in `ToolboxItem`. - As the primary toolbox items, `ToolboxCategory` and `ToolboxSeparator` were updated to have -1 tab indexes and defined IDs to help `ToolboxItem` fulfill its contracted for `IFocusableNode.getFocusableElement`. Each of these two new focusable trees have specific noteworthy behaviors behaviors: - `Toolbox` will automatically indicate that its first item should be focused (if one is present), even overriding the ability to focus the toolbox's root (however there are some cases where that can still happen). - `Toolbox` will automatically synchronize its selection state with its item nodes being focused. - `FlyoutBase`, now being a focusable tree, has had a tab index of 0 added. Normally a tab index of -1 is all that's needed, but the keyboard navigation plugin specifically uses 0 for flyout so that the flyout is tabbable. This is a **new** tab stop being introduced. - `FlyoutBase` holds a workspace (for rendering blocks) and, since `WorkspaceSvg` is already set up to be a focusable tree, it's represented as a subtree to `FlyoutBase`. This does introduce some wonky behaviors: the flyout's root will have passive focus while its contents have active focus. This could be manually disabled with some CSS if it ends up being a confusing user experience. - Both `FlyoutBase` and `WorkspaceSvg` have built-in behaviors for detecting when a user tries navigating away from an open flyout to ensure that the flyout is closed when it's supposed to be. That is, the flyout is auto-hideable and a non-flyout, non-toolbox node has then been focused. This matches parity with the `T`/`Esc` flows supported in the keyboard navigation plugin playground. One other thing to note: `Toolbox` had a few tests to update that were trying to reinit a toolbox without first disposing of it (which was caught by one of `FocusManager`'s state guardrails). ### Reason for Changes This is part of an ongoing effort to ensure key components of Blockly are focusable so that they can be keyboard-navigable (with other needed changes yet both in Core Blockly and the keyboard navigation plugin). ### Test Coverage No new tests have been added. It's certainly possible to add unit tests for the focusable configurations being introduced in this PR, but it may not be highly beneficial. It's largely assumed that the individual implementations should work due to a highly tested FocusManager, and it may be the case that the interactions of the components working together is far more important to verify (that is, the end user flows). The latter is planned to be tackled as part of #8915. ### Documentation No documentation changes should be needed here. ### Additional Information This includes changes that have been pulled from #8875. --- core/flyout_base.ts | 69 ++++++++++++++++++++++++++- core/interfaces/i_flyout.ts | 3 +- core/interfaces/i_toolbox.ts | 3 +- core/interfaces/i_toolbox_item.ts | 4 +- core/toolbox/category.ts | 2 + core/toolbox/separator.ts | 2 + core/toolbox/toolbox.ts | 77 ++++++++++++++++++++++++++++++- core/toolbox/toolbox_item.ts | 24 ++++++++++ core/workspace_svg.ts | 14 +++++- tests/mocha/toolbox_test.js | 3 ++ 10 files changed, 194 insertions(+), 7 deletions(-) diff --git a/core/flyout_base.ts b/core/flyout_base.ts index e738470a606..d24ea2758a0 100644 --- a/core/flyout_base.ts +++ b/core/flyout_base.ts @@ -21,9 +21,12 @@ import * as eventUtils from './events/utils.js'; import {FlyoutItem} from './flyout_item.js'; import {FlyoutMetricsManager} from './flyout_metrics_manager.js'; import {FlyoutSeparator, SeparatorAxis} from './flyout_separator.js'; +import {getFocusManager} from './focus_manager.js'; import {IAutoHideable} from './interfaces/i_autohideable.js'; import type {IFlyout} from './interfaces/i_flyout.js'; import type {IFlyoutInflater} from './interfaces/i_flyout_inflater.js'; +import {IFocusableNode} from './interfaces/i_focusable_node.js'; +import {IFocusableTree} from './interfaces/i_focusable_tree.js'; import type {Options} from './options.js'; import * as registry from './registry.js'; import * as renderManagement from './render_management.js'; @@ -43,7 +46,7 @@ import {WorkspaceSvg} from './workspace_svg.js'; */ export abstract class Flyout extends DeleteArea - implements IAutoHideable, IFlyout + implements IAutoHideable, IFlyout, IFocusableNode { /** * Position the flyout. @@ -303,6 +306,7 @@ export abstract class Flyout // hide/show code will set up proper visibility and size later. this.svgGroup_ = dom.createSvgElement(tagName, { 'class': 'blocklyFlyout', + 'tabindex': '0', }); this.svgGroup_.style.display = 'none'; this.svgBackground_ = dom.createSvgElement( @@ -317,6 +321,9 @@ export abstract class Flyout this.workspace_ .getThemeManager() .subscribe(this.svgBackground_, 'flyoutOpacity', 'fill-opacity'); + + getFocusManager().registerTree(this); + return this.svgGroup_; } @@ -398,6 +405,7 @@ export abstract class Flyout if (this.svgGroup_) { dom.removeNode(this.svgGroup_); } + getFocusManager().unregisterTree(this); } /** @@ -961,4 +969,63 @@ export abstract class Flyout return null; } + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + if (!this.svgGroup_) throw new Error('Flyout DOM is not yet created.'); + return this.svgGroup_; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void {} + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void {} + + /** See IFocusableTree.getRootFocusableNode. */ + getRootFocusableNode(): IFocusableNode { + return this; + } + + /** See IFocusableTree.getRestoredFocusableNode. */ + getRestoredFocusableNode( + _previousNode: IFocusableNode | null, + ): IFocusableNode | null { + return null; + } + + /** See IFocusableTree.getNestedTrees. */ + getNestedTrees(): Array { + return [this.workspace_]; + } + + /** See IFocusableTree.lookUpFocusableNode. */ + lookUpFocusableNode(_id: string): IFocusableNode | null { + // No focusable node needs to be returned since the flyout's subtree is a + // workspace that will manage its own focusable state. + return null; + } + + /** See IFocusableTree.onTreeFocus. */ + onTreeFocus( + _node: IFocusableNode, + _previousTree: IFocusableTree | null, + ): void {} + + /** See IFocusableTree.onTreeBlur. */ + onTreeBlur(nextTree: IFocusableTree | null): void { + const toolbox = this.targetWorkspace.getToolbox(); + // If focus is moving to either the toolbox or the flyout's workspace, do + // not close the flyout. For anything else, do close it since the flyout is + // no longer focused. + if (toolbox && nextTree === toolbox) return; + if (nextTree == this.workspace_) return; + if (toolbox) toolbox.clearSelection(); + this.autoHide(false); + } } diff --git a/core/interfaces/i_flyout.ts b/core/interfaces/i_flyout.ts index 42204775ece..067cd5ef20d 100644 --- a/core/interfaces/i_flyout.ts +++ b/core/interfaces/i_flyout.ts @@ -12,12 +12,13 @@ import type {Coordinate} from '../utils/coordinate.js'; import type {Svg} from '../utils/svg.js'; import type {FlyoutDefinition} from '../utils/toolbox.js'; import type {WorkspaceSvg} from '../workspace_svg.js'; +import {IFocusableTree} from './i_focusable_tree.js'; import type {IRegistrable} from './i_registrable.js'; /** * Interface for a flyout. */ -export interface IFlyout extends IRegistrable { +export interface IFlyout extends IRegistrable, IFocusableTree { /** Whether the flyout is laid out horizontally or not. */ horizontalLayout: boolean; diff --git a/core/interfaces/i_toolbox.ts b/core/interfaces/i_toolbox.ts index 1780af94d8a..f5d9c9fd7c6 100644 --- a/core/interfaces/i_toolbox.ts +++ b/core/interfaces/i_toolbox.ts @@ -9,13 +9,14 @@ import type {ToolboxInfo} from '../utils/toolbox.js'; import type {WorkspaceSvg} from '../workspace_svg.js'; import type {IFlyout} from './i_flyout.js'; +import type {IFocusableTree} from './i_focusable_tree.js'; import type {IRegistrable} from './i_registrable.js'; import type {IToolboxItem} from './i_toolbox_item.js'; /** * Interface for a toolbox. */ -export interface IToolbox extends IRegistrable { +export interface IToolbox extends IRegistrable, IFocusableTree { /** Initializes the toolbox. */ init(): void; diff --git a/core/interfaces/i_toolbox_item.ts b/core/interfaces/i_toolbox_item.ts index e3c9864f0c0..661624fd7e8 100644 --- a/core/interfaces/i_toolbox_item.ts +++ b/core/interfaces/i_toolbox_item.ts @@ -6,10 +6,12 @@ // Former goog.module ID: Blockly.IToolboxItem +import type {IFocusableNode} from './i_focusable_node.js'; + /** * Interface for an item in the toolbox. */ -export interface IToolboxItem { +export interface IToolboxItem extends IFocusableNode { /** * Initializes the toolbox item. * This includes creating the DOM and updating the state of any items based diff --git a/core/toolbox/category.ts b/core/toolbox/category.ts index d8ee8736ea6..fc7d1aa03cf 100644 --- a/core/toolbox/category.ts +++ b/core/toolbox/category.ts @@ -225,6 +225,8 @@ export class ToolboxCategory */ protected createContainer_(): HTMLDivElement { const container = document.createElement('div'); + container.tabIndex = -1; + container.id = this.getId(); const className = this.cssConfig_['container']; if (className) { dom.addClass(container, className); diff --git a/core/toolbox/separator.ts b/core/toolbox/separator.ts index 31ccb7e42f3..44ae358cf53 100644 --- a/core/toolbox/separator.ts +++ b/core/toolbox/separator.ts @@ -54,6 +54,8 @@ export class ToolboxSeparator extends ToolboxItem { */ protected createDom_(): HTMLDivElement { const container = document.createElement('div'); + container.tabIndex = -1; + container.id = this.getId(); const className = this.cssConfig_['container']; if (className) { dom.addClass(container, className); diff --git a/core/toolbox/toolbox.ts b/core/toolbox/toolbox.ts index b0fd82e97f2..ceb756afbd6 100644 --- a/core/toolbox/toolbox.ts +++ b/core/toolbox/toolbox.ts @@ -22,11 +22,14 @@ import {DeleteArea} from '../delete_area.js'; import '../events/events_toolbox_item_select.js'; import {EventType} from '../events/type.js'; import * as eventUtils from '../events/utils.js'; +import {getFocusManager} from '../focus_manager.js'; import type {IAutoHideable} from '../interfaces/i_autohideable.js'; import type {ICollapsibleToolboxItem} from '../interfaces/i_collapsible_toolbox_item.js'; import {isDeletable} from '../interfaces/i_deletable.js'; import type {IDraggable} from '../interfaces/i_draggable.js'; import type {IFlyout} from '../interfaces/i_flyout.js'; +import type {IFocusableNode} from '../interfaces/i_focusable_node.js'; +import type {IFocusableTree} from '../interfaces/i_focusable_tree.js'; import type {IKeyboardAccessible} from '../interfaces/i_keyboard_accessible.js'; import type {ISelectableToolboxItem} from '../interfaces/i_selectable_toolbox_item.js'; import {isSelectableToolboxItem} from '../interfaces/i_selectable_toolbox_item.js'; @@ -51,7 +54,12 @@ import {CollapsibleToolboxCategory} from './collapsible_category.js'; */ export class Toolbox extends DeleteArea - implements IAutoHideable, IKeyboardAccessible, IStyleable, IToolbox + implements + IAutoHideable, + IKeyboardAccessible, + IStyleable, + IToolbox, + IFocusableNode { /** * The unique ID for this component that is used to register with the @@ -163,6 +171,7 @@ export class Toolbox ComponentManager.Capability.DRAG_TARGET, ], }); + getFocusManager().registerTree(this); } /** @@ -177,7 +186,6 @@ export class Toolbox const container = this.createContainer_(); this.contentsDiv_ = this.createContentsContainer_(); - this.contentsDiv_.tabIndex = 0; aria.setRole(this.contentsDiv_, aria.Role.TREE); container.appendChild(this.contentsDiv_); @@ -194,6 +202,7 @@ export class Toolbox */ protected createContainer_(): HTMLDivElement { const toolboxContainer = document.createElement('div'); + toolboxContainer.tabIndex = 0; toolboxContainer.setAttribute('layout', this.isHorizontal() ? 'h' : 'v'); dom.addClass(toolboxContainer, 'blocklyToolbox'); toolboxContainer.setAttribute('dir', this.RTL ? 'RTL' : 'LTR'); @@ -1077,7 +1086,71 @@ export class Toolbox this.workspace_.getThemeManager().unsubscribe(this.HtmlDiv); dom.removeNode(this.HtmlDiv); } + + getFocusManager().unregisterTree(this); + } + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + if (!this.HtmlDiv) throw Error('Toolbox DOM has not yet been created.'); + return this.HtmlDiv; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void {} + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void {} + + /** See IFocusableTree.getRootFocusableNode. */ + getRootFocusableNode(): IFocusableNode { + return this; + } + + /** See IFocusableTree.getRestoredFocusableNode. */ + getRestoredFocusableNode( + previousNode: IFocusableNode | null, + ): IFocusableNode | null { + // Always try to select the first selectable toolbox item rather than the + // root of the toolbox. + if (!previousNode || previousNode === this) { + return this.getToolboxItems().find((item) => item.isSelectable()) ?? null; + } + return null; } + + /** See IFocusableTree.getNestedTrees. */ + getNestedTrees(): Array { + return []; + } + + /** See IFocusableTree.lookUpFocusableNode. */ + lookUpFocusableNode(id: string): IFocusableNode | null { + return this.getToolboxItemById(id) as IFocusableNode; + } + + /** See IFocusableTree.onTreeFocus. */ + onTreeFocus( + node: IFocusableNode, + _previousTree: IFocusableTree | null, + ): void { + if (node !== this) { + // Only select the item if it isn't already selected so as to not toggle. + if (this.getSelectedItem() !== node) { + this.setSelectedItem(node as IToolboxItem); + } + } else { + this.clearSelection(); + } + } + + /** See IFocusableTree.onTreeBlur. */ + onTreeBlur(_nextTree: IFocusableTree | null): void {} } /** CSS for Toolbox. See css.js for use. */ diff --git a/core/toolbox/toolbox_item.ts b/core/toolbox/toolbox_item.ts index ef9d979ab43..0d46a5eadfd 100644 --- a/core/toolbox/toolbox_item.ts +++ b/core/toolbox/toolbox_item.ts @@ -12,6 +12,7 @@ // Former goog.module ID: Blockly.ToolboxItem import type {ICollapsibleToolboxItem} from '../interfaces/i_collapsible_toolbox_item.js'; +import type {IFocusableTree} from '../interfaces/i_focusable_tree.js'; import type {IToolbox} from '../interfaces/i_toolbox.js'; import type {IToolboxItem} from '../interfaces/i_toolbox_item.js'; import * as idGenerator from '../utils/idgenerator.js'; @@ -148,5 +149,28 @@ export class ToolboxItem implements IToolboxItem { * @param _isVisible True if category should be visible. */ setVisible_(_isVisible: boolean) {} + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + const div = this.getDiv(); + if (!div) { + throw Error('Trying to access toolbox item before DOM is initialized.'); + } + if (!(div instanceof HTMLElement)) { + throw Error('Toolbox item div is unexpectedly not an HTML element.'); + } + return div as HTMLElement; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this.parentToolbox_; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void {} + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void {} } // nop by default diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index b1c96373771..250d6cf43e2 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 8f59649956346440b417fda861e00d8375fa8be2 Mon Sep 17 00:00:00 2001 From: Grace <145345672+microbit-grace@users.noreply.github.com> Date: Fri, 25 Apr 2025 16:26:58 +0100 Subject: [PATCH 02/11] fix: LineCursor can loop forward, but not back (#8926) * fix: loop cursor when moving to prev node * chore: add loop tests for LineCursor prev and out * chore: fix out loop test for line cursor --- core/keyboard_nav/line_cursor.ts | 7 +++++-- tests/mocha/cursor_test.js | 18 ++++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/core/keyboard_nav/line_cursor.ts b/core/keyboard_nav/line_cursor.ts index b2bda39c739..2025da7bf06 100644 --- a/core/keyboard_nav/line_cursor.ts +++ b/core/keyboard_nav/line_cursor.ts @@ -146,10 +146,12 @@ export class LineCursor extends Marker { if (!curNode) { return null; } - const newNode = this.getPreviousNodeImpl( + const newNode = this.getPreviousNode( curNode, this.validLineNode.bind(this), + true, ); + if (newNode) { this.setCurNode(newNode); } @@ -168,9 +170,10 @@ export class LineCursor extends Marker { if (!curNode) { return null; } - const newNode = this.getPreviousNodeImpl( + const newNode = this.getPreviousNode( curNode, this.validInLineNode.bind(this), + true, ); if (newNode) { diff --git a/tests/mocha/cursor_test.js b/tests/mocha/cursor_test.js index 905f48c09ad..53f0714da11 100644 --- a/tests/mocha/cursor_test.js +++ b/tests/mocha/cursor_test.js @@ -125,6 +125,15 @@ suite('Cursor', function () { assert.equal(curNode.getLocation(), this.blocks.A.nextConnection); }); + test('Prev - From first connection loop to last next connection', function () { + const prevConnection = this.blocks.A.previousConnection; + const prevConnectionNode = ASTNode.createConnectionNode(prevConnection); + this.cursor.setCurNode(prevConnectionNode); + this.cursor.prev(); + const curNode = this.cursor.getCurNode(); + assert.equal(curNode.getLocation(), this.blocks.D.nextConnection); + }); + test('Out - From field does not skip over block node', function () { const field = this.blocks.E.inputList[0].fieldRow[0]; const fieldNode = ASTNode.createFieldNode(field); @@ -133,6 +142,15 @@ suite('Cursor', function () { const curNode = this.cursor.getCurNode(); assert.equal(curNode.getLocation(), this.blocks.E); }); + + test('Out - From first connection loop to last next connection', function () { + const prevConnection = this.blocks.A.previousConnection; + const prevConnectionNode = ASTNode.createConnectionNode(prevConnection); + this.cursor.setCurNode(prevConnectionNode); + this.cursor.out(); + const curNode = this.cursor.getCurNode(); + assert.equal(curNode.getLocation(), this.blocks.D.nextConnection); + }); }); suite('Searching', function () { setup(function () { From c644fe36eff50de21d485fe22127307b52a0a74a Mon Sep 17 00:00:00 2001 From: RoboErikG Date: Fri, 25 Apr 2025 15:03:32 -0700 Subject: [PATCH 03/11] Fix: Revert focus prs (#8933) * Revert "feat: Make toolbox and flyout focusable (#8920)" This reverts commit 5bc83808bfe119a98de917f08840997dc9d3c324. * Revert "feat: Make WorkspaceSvg and BlockSvg focusable (#8916)" This reverts commit d7680cf32ec0fff5b1fabf8d786f996e9e454ff2. --- core/block_svg.ts | 28 +-------- core/flyout_base.ts | 69 +-------------------- core/inject.ts | 5 ++ core/interfaces/i_flyout.ts | 3 +- core/interfaces/i_toolbox.ts | 3 +- core/interfaces/i_toolbox_item.ts | 4 +- core/renderers/common/path_object.ts | 2 +- core/toolbox/category.ts | 2 - core/toolbox/separator.ts | 2 - core/toolbox/toolbox.ts | 77 +----------------------- core/toolbox/toolbox_item.ts | 24 -------- core/workspace_svg.ts | 89 +--------------------------- tests/mocha/toolbox_test.js | 3 - 13 files changed, 15 insertions(+), 296 deletions(-) diff --git a/core/block_svg.ts b/core/block_svg.ts index 8bc0b7af3f6..b8712b01914 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -44,8 +44,6 @@ 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'; @@ -78,8 +76,7 @@ export class BlockSvg IContextMenu, ICopyable, IDraggable, - IDeletable, - IFocusableNode + IDeletable { /** * Constant for identifying rows that are to be rendered inline. @@ -213,7 +210,6 @@ 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_(); } @@ -1823,26 +1819,4 @@ export class BlockSvg ); } } - - /** See IFocusableNode.getFocusableElement. */ - getFocusableElement(): HTMLElement | SVGElement { - return this.pathObject.svgPath; - } - - /** See IFocusableNode.getFocusableTree. */ - getFocusableTree(): IFocusableTree { - return this.workspace; - } - - /** See IFocusableNode.onNodeFocus. */ - onNodeFocus(): void { - common.setSelected(this); - } - - /** See IFocusableNode.onNodeBlur. */ - onNodeBlur(): void { - if (common.getSelected() === this) { - common.setSelected(null); - } - } } diff --git a/core/flyout_base.ts b/core/flyout_base.ts index d24ea2758a0..e738470a606 100644 --- a/core/flyout_base.ts +++ b/core/flyout_base.ts @@ -21,12 +21,9 @@ 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'; @@ -46,7 +43,7 @@ import {WorkspaceSvg} from './workspace_svg.js'; */ export abstract class Flyout extends DeleteArea - implements IAutoHideable, IFlyout, IFocusableNode + implements IAutoHideable, IFlyout { /** * Position the flyout. @@ -306,7 +303,6 @@ 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( @@ -321,9 +317,6 @@ export abstract class Flyout this.workspace_ .getThemeManager() .subscribe(this.svgBackground_, 'flyoutOpacity', 'fill-opacity'); - - getFocusManager().registerTree(this); - return this.svgGroup_; } @@ -405,7 +398,6 @@ export abstract class Flyout if (this.svgGroup_) { dom.removeNode(this.svgGroup_); } - getFocusManager().unregisterTree(this); } /** @@ -969,63 +961,4 @@ 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/inject.ts b/core/inject.ts index 34d9c1795f8..de78fbfae75 100644 --- a/core/inject.ts +++ b/core/inject.ts @@ -13,11 +13,13 @@ 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'; @@ -54,6 +56,8 @@ 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); @@ -122,6 +126,7 @@ function createDom(container: HTMLElement, options: Options): SVGElement { 'xmlns:xlink': dom.XLINK_NS, 'version': '1.1', 'class': 'blocklySvg', + 'tabindex': '0', }, container, ); diff --git a/core/interfaces/i_flyout.ts b/core/interfaces/i_flyout.ts index 067cd5ef20d..42204775ece 100644 --- a/core/interfaces/i_flyout.ts +++ b/core/interfaces/i_flyout.ts @@ -12,13 +12,12 @@ 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, IFocusableTree { +export interface IFlyout extends IRegistrable { /** 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 f5d9c9fd7c6..1780af94d8a 100644 --- a/core/interfaces/i_toolbox.ts +++ b/core/interfaces/i_toolbox.ts @@ -9,14 +9,13 @@ 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, IFocusableTree { +export interface IToolbox extends IRegistrable { /** Initializes the toolbox. */ init(): void; diff --git a/core/interfaces/i_toolbox_item.ts b/core/interfaces/i_toolbox_item.ts index 661624fd7e8..e3c9864f0c0 100644 --- a/core/interfaces/i_toolbox_item.ts +++ b/core/interfaces/i_toolbox_item.ts @@ -6,12 +6,10 @@ // Former goog.module ID: Blockly.IToolboxItem -import type {IFocusableNode} from './i_focusable_node.js'; - /** * Interface for an item in the toolbox. */ -export interface IToolboxItem extends IFocusableNode { +export interface IToolboxItem { /** * Initializes the toolbox item. * This includes creating the DOM and updating the state of any items based diff --git a/core/renderers/common/path_object.ts b/core/renderers/common/path_object.ts index 72cf2a594ce..077f80bb741 100644 --- a/core/renderers/common/path_object.ts +++ b/core/renderers/common/path_object.ts @@ -62,7 +62,7 @@ export class PathObject implements IPathObject { /** The primary path of the block. */ this.svgPath = dom.createSvgElement( Svg.PATH, - {'class': 'blocklyPath', 'tabindex': '-1'}, + {'class': 'blocklyPath'}, this.svgRoot, ); diff --git a/core/toolbox/category.ts b/core/toolbox/category.ts index fc7d1aa03cf..d8ee8736ea6 100644 --- a/core/toolbox/category.ts +++ b/core/toolbox/category.ts @@ -225,8 +225,6 @@ 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 44ae358cf53..31ccb7e42f3 100644 --- a/core/toolbox/separator.ts +++ b/core/toolbox/separator.ts @@ -54,8 +54,6 @@ 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 ceb756afbd6..b0fd82e97f2 100644 --- a/core/toolbox/toolbox.ts +++ b/core/toolbox/toolbox.ts @@ -22,14 +22,11 @@ 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'; @@ -54,12 +51,7 @@ import {CollapsibleToolboxCategory} from './collapsible_category.js'; */ export class Toolbox extends DeleteArea - implements - IAutoHideable, - IKeyboardAccessible, - IStyleable, - IToolbox, - IFocusableNode + implements IAutoHideable, IKeyboardAccessible, IStyleable, IToolbox { /** * The unique ID for this component that is used to register with the @@ -171,7 +163,6 @@ export class Toolbox ComponentManager.Capability.DRAG_TARGET, ], }); - getFocusManager().registerTree(this); } /** @@ -186,6 +177,7 @@ 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_); @@ -202,7 +194,6 @@ 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'); @@ -1086,71 +1077,7 @@ export class Toolbox this.workspace_.getThemeManager().unsubscribe(this.HtmlDiv); dom.removeNode(this.HtmlDiv); } - - getFocusManager().unregisterTree(this); - } - - /** See IFocusableNode.getFocusableElement. */ - getFocusableElement(): HTMLElement | SVGElement { - if (!this.HtmlDiv) throw Error('Toolbox DOM has not yet been created.'); - return this.HtmlDiv; - } - - /** See IFocusableNode.getFocusableTree. */ - getFocusableTree(): IFocusableTree { - return this; - } - - /** See IFocusableNode.onNodeFocus. */ - onNodeFocus(): void {} - - /** See IFocusableNode.onNodeBlur. */ - onNodeBlur(): void {} - - /** See IFocusableTree.getRootFocusableNode. */ - getRootFocusableNode(): IFocusableNode { - return this; - } - - /** See IFocusableTree.getRestoredFocusableNode. */ - getRestoredFocusableNode( - previousNode: IFocusableNode | null, - ): IFocusableNode | null { - // Always try to select the first selectable toolbox item rather than the - // root of the toolbox. - if (!previousNode || previousNode === this) { - return this.getToolboxItems().find((item) => item.isSelectable()) ?? null; - } - return null; } - - /** See IFocusableTree.getNestedTrees. */ - getNestedTrees(): Array { - return []; - } - - /** See IFocusableTree.lookUpFocusableNode. */ - lookUpFocusableNode(id: string): IFocusableNode | null { - return this.getToolboxItemById(id) as IFocusableNode; - } - - /** See IFocusableTree.onTreeFocus. */ - onTreeFocus( - node: IFocusableNode, - _previousTree: IFocusableTree | null, - ): void { - if (node !== this) { - // Only select the item if it isn't already selected so as to not toggle. - if (this.getSelectedItem() !== node) { - this.setSelectedItem(node as IToolboxItem); - } - } else { - this.clearSelection(); - } - } - - /** See IFocusableTree.onTreeBlur. */ - onTreeBlur(_nextTree: IFocusableTree | null): void {} } /** CSS for Toolbox. See css.js for use. */ diff --git a/core/toolbox/toolbox_item.ts b/core/toolbox/toolbox_item.ts index 0d46a5eadfd..ef9d979ab43 100644 --- a/core/toolbox/toolbox_item.ts +++ b/core/toolbox/toolbox_item.ts @@ -12,7 +12,6 @@ // 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'; @@ -149,28 +148,5 @@ 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 250d6cf43e2..91668b744d4 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -37,7 +37,6 @@ 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'; @@ -45,8 +44,6 @@ 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 { @@ -57,7 +54,6 @@ 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'; @@ -70,7 +66,6 @@ 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'; @@ -98,7 +93,7 @@ const ZOOM_TO_FIT_MARGIN = 20; */ export class WorkspaceSvg extends Workspace - implements IASTNodeLocationSvg, IContextMenu, IFocusableNode, IFocusableTree + implements IASTNodeLocationSvg, IContextMenu { /** * A wrapper function called when a resize event occurs. @@ -769,19 +764,7 @@ export class WorkspaceSvg * * */ - this.svgGroup_ = dom.createSvgElement(Svg.G, { - 'class': 'blocklyWorkspace', - // Only the top-level workspace should be tabbable. - 'tabindex': injectionDiv ? '0' : '-1', - 'id': this.id, - }); - if (injectionDiv) { - aria.setState( - this.svgGroup_, - aria.State.LABEL, - Msg['WORKSPACE_ARIA_LABEL'], - ); - } + this.svgGroup_ = dom.createSvgElement(Svg.G, {'class': 'blocklyWorkspace'}); // 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 @@ -857,9 +840,6 @@ export class WorkspaceSvg this.getTheme(), isParentWorkspace ? this.getInjectionDiv() : undefined, ); - - getFocusManager().registerTree(this); - return this.svgGroup_; } @@ -944,10 +924,6 @@ export class WorkspaceSvg document.body.removeEventListener('wheel', this.dummyWheelListener); this.dummyWheelListener = null; } - - if (getFocusManager().isRegistered(this)) { - getFocusManager().unregisterTree(this); - } } /** @@ -2642,67 +2618,6 @@ 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 { - // 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 f32319c6779..10bfd335223 100644 --- a/tests/mocha/toolbox_test.js +++ b/tests/mocha/toolbox_test.js @@ -54,7 +54,6 @@ 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, @@ -73,14 +72,12 @@ 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 dee27b905dd4cfbae1f564192d594e401c19b468 Mon Sep 17 00:00:00 2001 From: RoboErikG Date: Mon, 28 Apr 2025 15:24:57 -0700 Subject: [PATCH 04/11] fix: Support RTL in WorkspaceSvg.scrollIntoBounds (#8936) * feat: add support for RTL to scrollBoundsIntoView * Add additional comment --- core/workspace_svg.ts | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index 91668b744d4..e8d411651c5 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -2603,15 +2603,28 @@ export class WorkspaceSvg let deltaY = 0; if (bounds.left < viewport.left) { - deltaX = viewport.left - bounds.left; + deltaX = this.RTL + ? Math.min( + viewport.left - bounds.left, + viewport.right - bounds.right, // Don't move the right side out of view + ) + : viewport.left - bounds.left; } else if (bounds.right > viewport.right) { - deltaX = viewport.right - bounds.right; + deltaX = this.RTL + ? viewport.right - bounds.right + : Math.max( + viewport.right - bounds.right, + viewport.left - bounds.left, // Don't move the left side out of view + ); } if (bounds.top < viewport.top) { deltaY = viewport.top - bounds.top; } else if (bounds.bottom > viewport.bottom) { - deltaY = viewport.bottom - bounds.bottom; + deltaY = Math.max( + viewport.bottom - bounds.bottom, + viewport.top - bounds.top, // Don't move the top out of view + ); } deltaX *= scale; From b9b40f48abff9d7d4ff352039d982639458c975f Mon Sep 17 00:00:00 2001 From: Christopher Allen Date: Tue, 29 Apr 2025 16:15:42 +0100 Subject: [PATCH 05/11] fix: Fix bug in BlockSvg.prototype.setParent (#8934) * test(BlockSvg): Add tests for setParent(null) when dragging Add tests for scenarios where block(s) unrelated to the block being disconnected has/have been marked as as being dragged. Due to a bug in BlockSvg.prototype.setParent, one of these fails in the case that the dragging block is not a top block. Also add a check to assertNonParentAndOrphan to check that the orphan block's SVG root's parent is the workspace's canvass (i.e., that orphan is a top-level block in the DOM too). * fix(BlockSvg): Fix bug in setParent re: dragging block Fix an incorrect assumption in setParent: the topmost block whose root SVG element has the blocklyDragging class may not actually be a top-level block. * refactor(BlockDragStrategy): Hide connection preview earlier * chore(BlockDragStrategy): prefer ?. to !. Per nit on PR #8934. * fix(BlockSvg): Spelling: "canvass" -> "canvas" --- core/block_svg.ts | 18 ++++++++++---- core/dragging/block_drag_strategy.ts | 18 +++++++------- tests/mocha/block_test.js | 37 ++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 14 deletions(-) diff --git a/core/block_svg.ts b/core/block_svg.ts index b8712b01914..4cef79df8e6 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -305,14 +305,22 @@ export class BlockSvg (newParent as BlockSvg).getSvgRoot().appendChild(svgRoot); } else if (oldParent) { // If we are losing a parent, we want to move our DOM element to the - // root of the workspace. - const draggingBlock = this.workspace + // root of the workspace. Try to insert it before any top-level + // block being dragged, but note that blocks can have the + // blocklyDragging class even if they're not top blocks (especially + // at start and end of a drag). + const draggingBlockElement = this.workspace .getCanvas() .querySelector('.blocklyDragging'); - if (draggingBlock) { - this.workspace.getCanvas().insertBefore(svgRoot, draggingBlock); + const draggingParentElement = draggingBlockElement?.parentElement as + | SVGElement + | null + | undefined; + const canvas = this.workspace.getCanvas(); + if (draggingParentElement === canvas) { + canvas.insertBefore(svgRoot, draggingBlockElement); } else { - this.workspace.getCanvas().appendChild(svgRoot); + canvas.appendChild(svgRoot); } this.translate(oldXY.x, oldXY.y); } diff --git a/core/dragging/block_drag_strategy.ts b/core/dragging/block_drag_strategy.ts index b53c131653b..76020f90b5b 100644 --- a/core/dragging/block_drag_strategy.ts +++ b/core/dragging/block_drag_strategy.ts @@ -238,7 +238,7 @@ export class BlockDragStrategy implements IDragStrategy { const currCandidate = this.connectionCandidate; const newCandidate = this.getConnectionCandidate(draggingBlock, delta); if (!newCandidate) { - this.connectionPreviewer!.hidePreview(); + this.connectionPreviewer?.hidePreview(); this.connectionCandidate = null; return; } @@ -254,7 +254,7 @@ export class BlockDragStrategy implements IDragStrategy { local.type === ConnectionType.OUTPUT_VALUE || local.type === ConnectionType.PREVIOUS_STATEMENT; const neighbourIsConnectedToRealBlock = - neighbour.isConnected() && !neighbour.targetBlock()!.isInsertionMarker(); + neighbour.isConnected() && !neighbour.targetBlock()?.isInsertionMarker(); if ( localIsOutputOrPrevious && neighbourIsConnectedToRealBlock && @@ -264,14 +264,14 @@ export class BlockDragStrategy implements IDragStrategy { local.type, ) ) { - this.connectionPreviewer!.previewReplacement( + this.connectionPreviewer?.previewReplacement( local, neighbour, neighbour.targetBlock()!, ); return; } - this.connectionPreviewer!.previewConnection(local, neighbour); + this.connectionPreviewer?.previewConnection(local, neighbour); } /** @@ -385,7 +385,7 @@ export class BlockDragStrategy implements IDragStrategy { dom.stopTextWidthCache(); blockAnimation.disconnectUiStop(); - this.connectionPreviewer!.hidePreview(); + this.connectionPreviewer?.hidePreview(); if (!this.block.isDeadOrDying() && this.dragging) { // These are expensive and don't need to be done if we're deleting, or @@ -413,7 +413,7 @@ export class BlockDragStrategy implements IDragStrategy { // Must dispose after connections are applied to not break the dynamic // connections plugin. See #7859 - this.connectionPreviewer!.dispose(); + this.connectionPreviewer?.dispose(); this.workspace.setResizesEnabled(true); eventUtils.setGroup(newGroup); } @@ -445,6 +445,9 @@ export class BlockDragStrategy implements IDragStrategy { return; } + this.connectionPreviewer?.hidePreview(); + this.connectionCandidate = null; + this.startChildConn?.connect(this.block.nextConnection); if (this.startParentConn) { switch (this.startParentConn.type) { @@ -471,9 +474,6 @@ export class BlockDragStrategy implements IDragStrategy { this.startChildConn = null; this.startParentConn = null; - this.connectionPreviewer!.hidePreview(); - this.connectionCandidate = null; - this.block.setDragging(false); this.dragging = false; } diff --git a/tests/mocha/block_test.js b/tests/mocha/block_test.js index 56cfba54290..85a9f29181c 100644 --- a/tests/mocha/block_test.js +++ b/tests/mocha/block_test.js @@ -1105,6 +1105,18 @@ suite('Blocks', function () { ); this.textJoinBlock = this.printBlock.getInputTargetBlock('TEXT'); this.textBlock = this.textJoinBlock.getInputTargetBlock('ADD0'); + this.extraTopBlock = Blockly.Xml.domToBlock( + Blockly.utils.xml.textToDom(` + + + + drag me + + + `), + this.workspace, + ); + this.extraNestedBlock = this.extraTopBlock.getInputTargetBlock('TEXT'); }); function assertBlockIsOnlyChild(parent, child, inputName) { @@ -1116,6 +1128,10 @@ suite('Blocks', function () { assert.equal(nonParent.getChildren().length, 0); assert.isNull(nonParent.getInputTargetBlock('TEXT')); assert.isNull(orphan.getParent()); + assert.equal( + orphan.getSvgRoot().parentElement, + orphan.workspace.getCanvas(), + ); } function assertOriginalSetup() { assertBlockIsOnlyChild(this.printBlock, this.textJoinBlock, 'TEXT'); @@ -1187,6 +1203,27 @@ suite('Blocks', function () { ); assertNonParentAndOrphan(this.textJoinBlock, this.textBlock, 'ADD0'); }); + test('Setting parent to null with dragging block', function () { + this.extraTopBlock.setDragging(true); + this.textBlock.outputConnection.disconnect(); + assert.doesNotThrow( + this.textBlock.setParent.bind(this.textBlock, null), + ); + assertNonParentAndOrphan(this.textJoinBlock, this.textBlock, 'ADD0'); + assert.equal( + this.textBlock.getSvgRoot().nextSibling, + this.extraTopBlock.getSvgRoot(), + ); + }); + test('Setting parent to null with non-top dragging block', function () { + this.extraNestedBlock.setDragging(true); + this.textBlock.outputConnection.disconnect(); + assert.doesNotThrow( + this.textBlock.setParent.bind(this.textBlock, null), + ); + assertNonParentAndOrphan(this.textJoinBlock, this.textBlock, 'ADD0'); + assert.equal(this.textBlock.getSvgRoot().nextSibling, null); + }); test('Setting parent to null without disconnecting', function () { assert.throws(this.textBlock.setParent.bind(this.textBlock, null)); assertOriginalSetup.call(this); From c75aea7a4c6cddb4bfdd7258b6cd5fb56a7eb408 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 24 Apr 2025 14:48:16 -0700 Subject: [PATCH 06/11] feat: Make WorkspaceSvg and BlockSvg focusable (#8916) ## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes #8913 Fixes #8914 Fixes part of #8771 ### Proposed Changes This updates `WorkspaceSvg` and `BlockSvg` to be focusable, that is, it makes the workspace a `IFocusableTree` and blocks `IFocusableNode`s. Some important details: - While this introduces focusable tree support for `Workspace` it doesn't include two other components that are obviously needed by the keyboard navigation plugin's playground: fields and connections. These will be introduced in subsequent PRs. - Blocks are set up to automatically synchronize their selection state with their focus state. This will eventually help to replace `LineCursor`'s responsibility for managing selection state itself. - The tabindex property for the workspace and its ARIA label have been moved down to the `.blocklyWorkspace` element itself rather than its wrapper. This helps address some tab stop issues that are already addressed in the plugin (via monkey patches), but also to ensure that the workspace's main SVG group interacts correctly with `FocusManager`. - `WorkspaceSvg` is being initially set up to default to its first top block when being focused for the first time. This is to match parity with the keyboard navigation plugin, however the latter also has functionality for defaulting to a position when no blocks are present. It's not clear how to actually support this under the new focus-based system (without adding an ephemeral element on which to focus), or if it's even necessary (since the workspace root can hold focus). ### Reason for Changes This is part of an ongoing effort to ensure key components of Blockly are focusable so that they can be keyboard-navigable (with other needed changes yet both in Core Blockly and the keyboard navigation plugin). ### Test Coverage No new tests have been added. It's certainly possible to add unit tests for the focusable configurations being introduced in this PR, but it may not be highly beneficial. It's largely assumed that the individual implementations should work due to a highly tested FocusManager, and it may be the case that the interactions of the components working together is far more important to verify (that is, the end user flows). The latter is planned to be tackled as part of #8915. ### Documentation No documentation changes should be needed here. ### Additional Information This includes changes that have been pulled from #8875. --- core/block_svg.ts | 28 +++++++++- core/inject.ts | 5 -- core/renderers/common/path_object.ts | 2 +- core/workspace_svg.ts | 77 +++++++++++++++++++++++++++- 4 files changed, 103 insertions(+), 9 deletions(-) diff --git a/core/block_svg.ts b/core/block_svg.ts index 4cef79df8e6..04b7d88d1f2 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -44,6 +44,8 @@ import {IContextMenu} from './interfaces/i_contextmenu.js'; import type {ICopyable} from './interfaces/i_copyable.js'; import {IDeletable} from './interfaces/i_deletable.js'; import type {IDragStrategy, IDraggable} from './interfaces/i_draggable.js'; +import type {IFocusableNode} from './interfaces/i_focusable_node.js'; +import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import {IIcon} from './interfaces/i_icon.js'; import * as internalConstants from './internal_constants.js'; import {MarkerManager} from './marker_manager.js'; @@ -76,7 +78,8 @@ export class BlockSvg IContextMenu, ICopyable, IDraggable, - IDeletable + IDeletable, + IFocusableNode { /** * Constant for identifying rows that are to be rendered inline. @@ -210,6 +213,7 @@ export class BlockSvg // Expose this block's ID on its top-level SVG group. this.svgGroup.setAttribute('data-id', this.id); + svgPath.id = this.id; this.doInit_(); } @@ -1827,4 +1831,26 @@ export class BlockSvg ); } } + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + return this.pathObject.svgPath; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this.workspace; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void { + common.setSelected(this); + } + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void { + if (common.getSelected() === this) { + common.setSelected(null); + } + } } diff --git a/core/inject.ts b/core/inject.ts index de78fbfae75..34d9c1795f8 100644 --- a/core/inject.ts +++ b/core/inject.ts @@ -13,13 +13,11 @@ import * as common from './common.js'; import * as Css from './css.js'; import * as dropDownDiv from './dropdowndiv.js'; import {Grid} from './grid.js'; -import {Msg} from './msg.js'; import {Options} from './options.js'; import {ScrollbarPair} from './scrollbar_pair.js'; import {ShortcutRegistry} from './shortcut_registry.js'; import * as Tooltip from './tooltip.js'; import * as Touch from './touch.js'; -import * as aria from './utils/aria.js'; import * as dom from './utils/dom.js'; import {Svg} from './utils/svg.js'; import * as WidgetDiv from './widgetdiv.js'; @@ -56,8 +54,6 @@ export function inject( if (opt_options?.rtl) { dom.addClass(subContainer, 'blocklyRTL'); } - subContainer.tabIndex = 0; - aria.setState(subContainer, aria.State.LABEL, Msg['WORKSPACE_ARIA_LABEL']); containerElement!.appendChild(subContainer); const svg = createDom(subContainer, options); @@ -126,7 +122,6 @@ function createDom(container: HTMLElement, options: Options): SVGElement { 'xmlns:xlink': dom.XLINK_NS, 'version': '1.1', 'class': 'blocklySvg', - 'tabindex': '0', }, container, ); diff --git a/core/renderers/common/path_object.ts b/core/renderers/common/path_object.ts index 077f80bb741..72cf2a594ce 100644 --- a/core/renderers/common/path_object.ts +++ b/core/renderers/common/path_object.ts @@ -62,7 +62,7 @@ export class PathObject implements IPathObject { /** The primary path of the block. */ this.svgPath = dom.createSvgElement( Svg.PATH, - {'class': 'blocklyPath'}, + {'class': 'blocklyPath', 'tabindex': '-1'}, this.svgRoot, ); diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index e8d411651c5..2648005b257 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -37,6 +37,7 @@ import {EventType} from './events/type.js'; import * as eventUtils from './events/utils.js'; import {Flyout} from './flyout_base.js'; import type {FlyoutButton} from './flyout_button.js'; +import {getFocusManager} from './focus_manager.js'; import {Gesture} from './gesture.js'; import {Grid} from './grid.js'; import type {IASTNodeLocationSvg} from './interfaces/i_ast_node_location_svg.js'; @@ -44,6 +45,8 @@ import type {IBoundedElement} from './interfaces/i_bounded_element.js'; import {IContextMenu} from './interfaces/i_contextmenu.js'; import type {IDragTarget} from './interfaces/i_drag_target.js'; import type {IFlyout} from './interfaces/i_flyout.js'; +import type {IFocusableNode} from './interfaces/i_focusable_node.js'; +import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import type {IMetricsManager} from './interfaces/i_metrics_manager.js'; import type {IToolbox} from './interfaces/i_toolbox.js'; import type { @@ -54,6 +57,7 @@ import type {LineCursor} from './keyboard_nav/line_cursor.js'; import type {Marker} from './keyboard_nav/marker.js'; import {LayerManager} from './layer_manager.js'; import {MarkerManager} from './marker_manager.js'; +import {Msg} from './msg.js'; import {Options} from './options.js'; import * as Procedures from './procedures.js'; import * as registry from './registry.js'; @@ -66,6 +70,7 @@ import {Classic} from './theme/classic.js'; import {ThemeManager} from './theme_manager.js'; import * as Tooltip from './tooltip.js'; import type {Trashcan} from './trashcan.js'; +import * as aria from './utils/aria.js'; import * as arrayUtils from './utils/array.js'; import {Coordinate} from './utils/coordinate.js'; import * as dom from './utils/dom.js'; @@ -93,7 +98,7 @@ const ZOOM_TO_FIT_MARGIN = 20; */ export class WorkspaceSvg extends Workspace - implements IASTNodeLocationSvg, IContextMenu + implements IASTNodeLocationSvg, IContextMenu, IFocusableNode, IFocusableTree { /** * A wrapper function called when a resize event occurs. @@ -764,7 +769,19 @@ export class WorkspaceSvg * * */ - this.svgGroup_ = dom.createSvgElement(Svg.G, {'class': 'blocklyWorkspace'}); + this.svgGroup_ = dom.createSvgElement(Svg.G, { + 'class': 'blocklyWorkspace', + // Only the top-level workspace should be tabbable. + 'tabindex': injectionDiv ? '0' : '-1', + 'id': this.id, + }); + if (injectionDiv) { + aria.setState( + this.svgGroup_, + aria.State.LABEL, + Msg['WORKSPACE_ARIA_LABEL'], + ); + } // Note that a alone does not receive mouse events--it must have a // valid target inside it. If no background class is specified, as in the @@ -840,6 +857,9 @@ export class WorkspaceSvg this.getTheme(), isParentWorkspace ? this.getInjectionDiv() : undefined, ); + + getFocusManager().registerTree(this); + return this.svgGroup_; } @@ -924,6 +944,10 @@ export class WorkspaceSvg document.body.removeEventListener('wheel', this.dummyWheelListener); this.dummyWheelListener = null; } + + if (getFocusManager().isRegistered(this)) { + getFocusManager().unregisterTree(this); + } } /** @@ -2631,6 +2655,55 @@ export class WorkspaceSvg deltaY *= scale; this.scroll(this.scrollX + deltaX, this.scrollY + deltaY); } + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + return this.svgGroup_; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void {} + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void {} + + /** See IFocusableTree.getRootFocusableNode. */ + getRootFocusableNode(): IFocusableNode { + return this; + } + + /** See IFocusableTree.getRestoredFocusableNode. */ + getRestoredFocusableNode( + previousNode: IFocusableNode | null, + ): IFocusableNode | null { + if (!previousNode) { + return this.getTopBlocks(true)[0] ?? null; + } else return null; + } + + /** See IFocusableTree.getNestedTrees. */ + getNestedTrees(): Array { + return []; + } + + /** See IFocusableTree.lookUpFocusableNode. */ + lookUpFocusableNode(id: string): IFocusableNode | null { + return this.getBlockById(id) as IFocusableNode; + } + + /** See IFocusableTree.onTreeFocus. */ + onTreeFocus( + _node: IFocusableNode, + _previousTree: IFocusableTree | null, + ): void {} + + /** See IFocusableTree.onTreeBlur. */ + onTreeBlur(_nextTree: IFocusableTree | null): void {} } /** From d4883f57d18ac92da2752942b15ae90cffb7cee5 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Thu, 24 Apr 2025 15:08:18 -0700 Subject: [PATCH 07/11] feat: Make toolbox and flyout focusable (#8920) ## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes #8918 Fixes #8919 Fixes part of #8771 ### Proposed Changes This updates several classes in order to make toolboxes and flyouts focusable: - `IFlyout` is now an `IFocusableTree` with corresponding implementations in `FlyoutBase`. - `IToolbox` is now an `IFocusableTree` with corresponding implementations in `Toolbox`. - `IToolboxItem` is now an `IFocusableNode` with corresponding implementations in `ToolboxItem`. - As the primary toolbox items, `ToolboxCategory` and `ToolboxSeparator` were updated to have -1 tab indexes and defined IDs to help `ToolboxItem` fulfill its contracted for `IFocusableNode.getFocusableElement`. Each of these two new focusable trees have specific noteworthy behaviors behaviors: - `Toolbox` will automatically indicate that its first item should be focused (if one is present), even overriding the ability to focus the toolbox's root (however there are some cases where that can still happen). - `Toolbox` will automatically synchronize its selection state with its item nodes being focused. - `FlyoutBase`, now being a focusable tree, has had a tab index of 0 added. Normally a tab index of -1 is all that's needed, but the keyboard navigation plugin specifically uses 0 for flyout so that the flyout is tabbable. This is a **new** tab stop being introduced. - `FlyoutBase` holds a workspace (for rendering blocks) and, since `WorkspaceSvg` is already set up to be a focusable tree, it's represented as a subtree to `FlyoutBase`. This does introduce some wonky behaviors: the flyout's root will have passive focus while its contents have active focus. This could be manually disabled with some CSS if it ends up being a confusing user experience. - Both `FlyoutBase` and `WorkspaceSvg` have built-in behaviors for detecting when a user tries navigating away from an open flyout to ensure that the flyout is closed when it's supposed to be. That is, the flyout is auto-hideable and a non-flyout, non-toolbox node has then been focused. This matches parity with the `T`/`Esc` flows supported in the keyboard navigation plugin playground. One other thing to note: `Toolbox` had a few tests to update that were trying to reinit a toolbox without first disposing of it (which was caught by one of `FocusManager`'s state guardrails). ### Reason for Changes This is part of an ongoing effort to ensure key components of Blockly are focusable so that they can be keyboard-navigable (with other needed changes yet both in Core Blockly and the keyboard navigation plugin). ### Test Coverage No new tests have been added. It's certainly possible to add unit tests for the focusable configurations being introduced in this PR, but it may not be highly beneficial. It's largely assumed that the individual implementations should work due to a highly tested FocusManager, and it may be the case that the interactions of the components working together is far more important to verify (that is, the end user flows). The latter is planned to be tackled as part of #8915. ### Documentation No documentation changes should be needed here. ### Additional Information This includes changes that have been pulled from #8875. --- core/flyout_base.ts | 69 ++++++++++++++++++++++++++- core/interfaces/i_flyout.ts | 3 +- core/interfaces/i_toolbox.ts | 3 +- core/interfaces/i_toolbox_item.ts | 4 +- core/toolbox/category.ts | 2 + core/toolbox/separator.ts | 2 + core/toolbox/toolbox.ts | 77 ++++++++++++++++++++++++++++++- core/toolbox/toolbox_item.ts | 24 ++++++++++ core/workspace_svg.ts | 14 +++++- tests/mocha/toolbox_test.js | 3 ++ 10 files changed, 194 insertions(+), 7 deletions(-) diff --git a/core/flyout_base.ts b/core/flyout_base.ts index e738470a606..d24ea2758a0 100644 --- a/core/flyout_base.ts +++ b/core/flyout_base.ts @@ -21,9 +21,12 @@ import * as eventUtils from './events/utils.js'; import {FlyoutItem} from './flyout_item.js'; import {FlyoutMetricsManager} from './flyout_metrics_manager.js'; import {FlyoutSeparator, SeparatorAxis} from './flyout_separator.js'; +import {getFocusManager} from './focus_manager.js'; import {IAutoHideable} from './interfaces/i_autohideable.js'; import type {IFlyout} from './interfaces/i_flyout.js'; import type {IFlyoutInflater} from './interfaces/i_flyout_inflater.js'; +import {IFocusableNode} from './interfaces/i_focusable_node.js'; +import {IFocusableTree} from './interfaces/i_focusable_tree.js'; import type {Options} from './options.js'; import * as registry from './registry.js'; import * as renderManagement from './render_management.js'; @@ -43,7 +46,7 @@ import {WorkspaceSvg} from './workspace_svg.js'; */ export abstract class Flyout extends DeleteArea - implements IAutoHideable, IFlyout + implements IAutoHideable, IFlyout, IFocusableNode { /** * Position the flyout. @@ -303,6 +306,7 @@ export abstract class Flyout // hide/show code will set up proper visibility and size later. this.svgGroup_ = dom.createSvgElement(tagName, { 'class': 'blocklyFlyout', + 'tabindex': '0', }); this.svgGroup_.style.display = 'none'; this.svgBackground_ = dom.createSvgElement( @@ -317,6 +321,9 @@ export abstract class Flyout this.workspace_ .getThemeManager() .subscribe(this.svgBackground_, 'flyoutOpacity', 'fill-opacity'); + + getFocusManager().registerTree(this); + return this.svgGroup_; } @@ -398,6 +405,7 @@ export abstract class Flyout if (this.svgGroup_) { dom.removeNode(this.svgGroup_); } + getFocusManager().unregisterTree(this); } /** @@ -961,4 +969,63 @@ export abstract class Flyout return null; } + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + if (!this.svgGroup_) throw new Error('Flyout DOM is not yet created.'); + return this.svgGroup_; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void {} + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void {} + + /** See IFocusableTree.getRootFocusableNode. */ + getRootFocusableNode(): IFocusableNode { + return this; + } + + /** See IFocusableTree.getRestoredFocusableNode. */ + getRestoredFocusableNode( + _previousNode: IFocusableNode | null, + ): IFocusableNode | null { + return null; + } + + /** See IFocusableTree.getNestedTrees. */ + getNestedTrees(): Array { + return [this.workspace_]; + } + + /** See IFocusableTree.lookUpFocusableNode. */ + lookUpFocusableNode(_id: string): IFocusableNode | null { + // No focusable node needs to be returned since the flyout's subtree is a + // workspace that will manage its own focusable state. + return null; + } + + /** See IFocusableTree.onTreeFocus. */ + onTreeFocus( + _node: IFocusableNode, + _previousTree: IFocusableTree | null, + ): void {} + + /** See IFocusableTree.onTreeBlur. */ + onTreeBlur(nextTree: IFocusableTree | null): void { + const toolbox = this.targetWorkspace.getToolbox(); + // If focus is moving to either the toolbox or the flyout's workspace, do + // not close the flyout. For anything else, do close it since the flyout is + // no longer focused. + if (toolbox && nextTree === toolbox) return; + if (nextTree == this.workspace_) return; + if (toolbox) toolbox.clearSelection(); + this.autoHide(false); + } } diff --git a/core/interfaces/i_flyout.ts b/core/interfaces/i_flyout.ts index 42204775ece..067cd5ef20d 100644 --- a/core/interfaces/i_flyout.ts +++ b/core/interfaces/i_flyout.ts @@ -12,12 +12,13 @@ import type {Coordinate} from '../utils/coordinate.js'; import type {Svg} from '../utils/svg.js'; import type {FlyoutDefinition} from '../utils/toolbox.js'; import type {WorkspaceSvg} from '../workspace_svg.js'; +import {IFocusableTree} from './i_focusable_tree.js'; import type {IRegistrable} from './i_registrable.js'; /** * Interface for a flyout. */ -export interface IFlyout extends IRegistrable { +export interface IFlyout extends IRegistrable, IFocusableTree { /** Whether the flyout is laid out horizontally or not. */ horizontalLayout: boolean; diff --git a/core/interfaces/i_toolbox.ts b/core/interfaces/i_toolbox.ts index 1780af94d8a..f5d9c9fd7c6 100644 --- a/core/interfaces/i_toolbox.ts +++ b/core/interfaces/i_toolbox.ts @@ -9,13 +9,14 @@ import type {ToolboxInfo} from '../utils/toolbox.js'; import type {WorkspaceSvg} from '../workspace_svg.js'; import type {IFlyout} from './i_flyout.js'; +import type {IFocusableTree} from './i_focusable_tree.js'; import type {IRegistrable} from './i_registrable.js'; import type {IToolboxItem} from './i_toolbox_item.js'; /** * Interface for a toolbox. */ -export interface IToolbox extends IRegistrable { +export interface IToolbox extends IRegistrable, IFocusableTree { /** Initializes the toolbox. */ init(): void; diff --git a/core/interfaces/i_toolbox_item.ts b/core/interfaces/i_toolbox_item.ts index e3c9864f0c0..661624fd7e8 100644 --- a/core/interfaces/i_toolbox_item.ts +++ b/core/interfaces/i_toolbox_item.ts @@ -6,10 +6,12 @@ // Former goog.module ID: Blockly.IToolboxItem +import type {IFocusableNode} from './i_focusable_node.js'; + /** * Interface for an item in the toolbox. */ -export interface IToolboxItem { +export interface IToolboxItem extends IFocusableNode { /** * Initializes the toolbox item. * This includes creating the DOM and updating the state of any items based diff --git a/core/toolbox/category.ts b/core/toolbox/category.ts index d8ee8736ea6..fc7d1aa03cf 100644 --- a/core/toolbox/category.ts +++ b/core/toolbox/category.ts @@ -225,6 +225,8 @@ export class ToolboxCategory */ protected createContainer_(): HTMLDivElement { const container = document.createElement('div'); + container.tabIndex = -1; + container.id = this.getId(); const className = this.cssConfig_['container']; if (className) { dom.addClass(container, className); diff --git a/core/toolbox/separator.ts b/core/toolbox/separator.ts index 31ccb7e42f3..44ae358cf53 100644 --- a/core/toolbox/separator.ts +++ b/core/toolbox/separator.ts @@ -54,6 +54,8 @@ export class ToolboxSeparator extends ToolboxItem { */ protected createDom_(): HTMLDivElement { const container = document.createElement('div'); + container.tabIndex = -1; + container.id = this.getId(); const className = this.cssConfig_['container']; if (className) { dom.addClass(container, className); diff --git a/core/toolbox/toolbox.ts b/core/toolbox/toolbox.ts index b0fd82e97f2..ceb756afbd6 100644 --- a/core/toolbox/toolbox.ts +++ b/core/toolbox/toolbox.ts @@ -22,11 +22,14 @@ import {DeleteArea} from '../delete_area.js'; import '../events/events_toolbox_item_select.js'; import {EventType} from '../events/type.js'; import * as eventUtils from '../events/utils.js'; +import {getFocusManager} from '../focus_manager.js'; import type {IAutoHideable} from '../interfaces/i_autohideable.js'; import type {ICollapsibleToolboxItem} from '../interfaces/i_collapsible_toolbox_item.js'; import {isDeletable} from '../interfaces/i_deletable.js'; import type {IDraggable} from '../interfaces/i_draggable.js'; import type {IFlyout} from '../interfaces/i_flyout.js'; +import type {IFocusableNode} from '../interfaces/i_focusable_node.js'; +import type {IFocusableTree} from '../interfaces/i_focusable_tree.js'; import type {IKeyboardAccessible} from '../interfaces/i_keyboard_accessible.js'; import type {ISelectableToolboxItem} from '../interfaces/i_selectable_toolbox_item.js'; import {isSelectableToolboxItem} from '../interfaces/i_selectable_toolbox_item.js'; @@ -51,7 +54,12 @@ import {CollapsibleToolboxCategory} from './collapsible_category.js'; */ export class Toolbox extends DeleteArea - implements IAutoHideable, IKeyboardAccessible, IStyleable, IToolbox + implements + IAutoHideable, + IKeyboardAccessible, + IStyleable, + IToolbox, + IFocusableNode { /** * The unique ID for this component that is used to register with the @@ -163,6 +171,7 @@ export class Toolbox ComponentManager.Capability.DRAG_TARGET, ], }); + getFocusManager().registerTree(this); } /** @@ -177,7 +186,6 @@ export class Toolbox const container = this.createContainer_(); this.contentsDiv_ = this.createContentsContainer_(); - this.contentsDiv_.tabIndex = 0; aria.setRole(this.contentsDiv_, aria.Role.TREE); container.appendChild(this.contentsDiv_); @@ -194,6 +202,7 @@ export class Toolbox */ protected createContainer_(): HTMLDivElement { const toolboxContainer = document.createElement('div'); + toolboxContainer.tabIndex = 0; toolboxContainer.setAttribute('layout', this.isHorizontal() ? 'h' : 'v'); dom.addClass(toolboxContainer, 'blocklyToolbox'); toolboxContainer.setAttribute('dir', this.RTL ? 'RTL' : 'LTR'); @@ -1077,7 +1086,71 @@ export class Toolbox this.workspace_.getThemeManager().unsubscribe(this.HtmlDiv); dom.removeNode(this.HtmlDiv); } + + getFocusManager().unregisterTree(this); + } + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + if (!this.HtmlDiv) throw Error('Toolbox DOM has not yet been created.'); + return this.HtmlDiv; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void {} + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void {} + + /** See IFocusableTree.getRootFocusableNode. */ + getRootFocusableNode(): IFocusableNode { + return this; + } + + /** See IFocusableTree.getRestoredFocusableNode. */ + getRestoredFocusableNode( + previousNode: IFocusableNode | null, + ): IFocusableNode | null { + // Always try to select the first selectable toolbox item rather than the + // root of the toolbox. + if (!previousNode || previousNode === this) { + return this.getToolboxItems().find((item) => item.isSelectable()) ?? null; + } + return null; } + + /** See IFocusableTree.getNestedTrees. */ + getNestedTrees(): Array { + return []; + } + + /** See IFocusableTree.lookUpFocusableNode. */ + lookUpFocusableNode(id: string): IFocusableNode | null { + return this.getToolboxItemById(id) as IFocusableNode; + } + + /** See IFocusableTree.onTreeFocus. */ + onTreeFocus( + node: IFocusableNode, + _previousTree: IFocusableTree | null, + ): void { + if (node !== this) { + // Only select the item if it isn't already selected so as to not toggle. + if (this.getSelectedItem() !== node) { + this.setSelectedItem(node as IToolboxItem); + } + } else { + this.clearSelection(); + } + } + + /** See IFocusableTree.onTreeBlur. */ + onTreeBlur(_nextTree: IFocusableTree | null): void {} } /** CSS for Toolbox. See css.js for use. */ diff --git a/core/toolbox/toolbox_item.ts b/core/toolbox/toolbox_item.ts index ef9d979ab43..0d46a5eadfd 100644 --- a/core/toolbox/toolbox_item.ts +++ b/core/toolbox/toolbox_item.ts @@ -12,6 +12,7 @@ // Former goog.module ID: Blockly.ToolboxItem import type {ICollapsibleToolboxItem} from '../interfaces/i_collapsible_toolbox_item.js'; +import type {IFocusableTree} from '../interfaces/i_focusable_tree.js'; import type {IToolbox} from '../interfaces/i_toolbox.js'; import type {IToolboxItem} from '../interfaces/i_toolbox_item.js'; import * as idGenerator from '../utils/idgenerator.js'; @@ -148,5 +149,28 @@ export class ToolboxItem implements IToolboxItem { * @param _isVisible True if category should be visible. */ setVisible_(_isVisible: boolean) {} + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + const div = this.getDiv(); + if (!div) { + throw Error('Trying to access toolbox item before DOM is initialized.'); + } + if (!(div instanceof HTMLElement)) { + throw Error('Toolbox item div is unexpectedly not an HTML element.'); + } + return div as HTMLElement; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this.parentToolbox_; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void {} + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void {} } // nop by default diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index 2648005b257..5f4c57a5774 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -2703,7 +2703,19 @@ export class WorkspaceSvg ): void {} /** See IFocusableTree.onTreeBlur. */ - onTreeBlur(_nextTree: IFocusableTree | null): void {} + onTreeBlur(nextTree: IFocusableTree | null): void { + // If the flyout loses focus, make sure to close it. + if (this.isFlyout && this.targetWorkspace) { + // Only hide the flyout if the flyout's workspace is losing focus and that + // focus isn't returning to the flyout itself or the toolbox. + const flyout = this.targetWorkspace.getFlyout(); + const toolbox = this.targetWorkspace.getToolbox(); + if (flyout && nextTree === flyout) return; + if (toolbox && nextTree === toolbox) return; + if (toolbox) toolbox.clearSelection(); + if (flyout && flyout instanceof Flyout) flyout.autoHide(false); + } + } } /** diff --git a/tests/mocha/toolbox_test.js b/tests/mocha/toolbox_test.js index 10bfd335223..f32319c6779 100644 --- a/tests/mocha/toolbox_test.js +++ b/tests/mocha/toolbox_test.js @@ -54,6 +54,7 @@ suite('Toolbox', function () { const themeManagerSpy = sinon.spy(themeManager, 'subscribe'); const componentManager = this.toolbox.workspace_.getComponentManager(); sinon.stub(componentManager, 'addComponent'); + this.toolbox.dispose(); // Dispose of the old toolbox so that it can be reinited. this.toolbox.init(); sinon.assert.calledWith( themeManagerSpy, @@ -72,12 +73,14 @@ suite('Toolbox', function () { const renderSpy = sinon.spy(this.toolbox, 'render'); const componentManager = this.toolbox.workspace_.getComponentManager(); sinon.stub(componentManager, 'addComponent'); + this.toolbox.dispose(); // Dispose of the old toolbox so that it can be reinited. this.toolbox.init(); sinon.assert.calledOnce(renderSpy); }); test('Init called -> Flyout is initialized', function () { const componentManager = this.toolbox.workspace_.getComponentManager(); sinon.stub(componentManager, 'addComponent'); + this.toolbox.dispose(); // Dispose of the old toolbox so that it can be reinited. this.toolbox.init(); assert.isDefined(this.toolbox.getFlyout()); }); From e5abf720c887c05ac93c6855199b1a85e547bcff Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 30 Apr 2025 00:21:52 +0000 Subject: [PATCH 08/11] fix: Remove CSS for active/passive focus. The CSS work here is far more extensive than should go in this or any of the structural branches. Instead, proper rendering will be done via the plugin and eventually merged into Core once finalized. --- core/css.ts | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/core/css.ts b/core/css.ts index 6ca262f3b25..ed47e8037ec 100644 --- a/core/css.ts +++ b/core/css.ts @@ -463,8 +463,8 @@ input[type=number] { } .blocklyMenuSeparator { - background-color: #ccc; - height: 1px; + background-color: #ccc; + height: 1px; border: 0; margin-left: 4px; margin-right: 4px; @@ -494,12 +494,4 @@ input[type=number] { cursor: grabbing; } -.blocklyActiveFocus { - outline-color: #2ae; - outline-width: 2px; -} -.blocklyPassiveFocus { - outline-color: #3fdfff; - outline-width: 1.5px; -} `; From 585c950f38428612b551761f92ca2acfb661da53 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 30 Apr 2025 00:41:26 +0000 Subject: [PATCH 09/11] feat: Make FlyoutButton focusable. --- core/flyout_button.ts | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/core/flyout_button.ts b/core/flyout_button.ts index 3b9b2fe0735..f24a3d3bc5f 100644 --- a/core/flyout_button.ts +++ b/core/flyout_button.ts @@ -15,6 +15,8 @@ import type {IASTNodeLocationSvg} from './blockly.js'; import * as browserEvents from './browser_events.js'; import * as Css from './css.js'; import type {IBoundedElement} from './interfaces/i_bounded_element.js'; +import type {IFocusableNode} from './interfaces/i_focusable_node.js'; +import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import type {IRenderedElement} from './interfaces/i_rendered_element.js'; import {Coordinate} from './utils/coordinate.js'; import * as dom from './utils/dom.js'; @@ -29,7 +31,11 @@ import type {WorkspaceSvg} from './workspace_svg.js'; * Class for a button or label in the flyout. */ export class FlyoutButton - implements IASTNodeLocationSvg, IBoundedElement, IRenderedElement + implements + IASTNodeLocationSvg, + IBoundedElement, + IRenderedElement, + IFocusableNode { /** The horizontal margin around the text in the button. */ static TEXT_MARGIN_X = 5; @@ -107,7 +113,7 @@ export class FlyoutButton this.svgGroup = dom.createSvgElement( Svg.G, - {'class': cssClass}, + {'class': cssClass, 'tabindex': '-1'}, this.workspace.getCanvas(), ); @@ -389,6 +395,22 @@ export class FlyoutButton getSvgRoot() { return this.svgGroup; } + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + return this.svgGroup; + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + return this.workspace; + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void {} + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void {} } /** CSS for buttons and labels. See css.js for use. */ From d6dcc4b42da30de1c03ef7fc1954587811b63b05 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 30 Apr 2025 01:24:32 +0000 Subject: [PATCH 10/11] fix: Actually make FlyoutButton focusable. It needed a unique ID and to be properly hooked up to node retrieval. --- core/flyout_button.ts | 7 ++++++- core/workspace_svg.ts | 15 ++++++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/core/flyout_button.ts b/core/flyout_button.ts index f24a3d3bc5f..21aa408e543 100644 --- a/core/flyout_button.ts +++ b/core/flyout_button.ts @@ -26,6 +26,7 @@ import * as style from './utils/style.js'; import {Svg} from './utils/svg.js'; import type * as toolbox from './utils/toolbox.js'; import type {WorkspaceSvg} from './workspace_svg.js'; +import {idGenerator} from './utils.js'; /** * Class for a button or label in the flyout. @@ -74,6 +75,9 @@ export class FlyoutButton */ cursorSvg: SVGElement | null = null; + /** The unique ID for this FlyoutButton. */ + private id: string; + /** * @param workspace The workspace in which to place this button. * @param targetWorkspace The flyout's target workspace. @@ -111,9 +115,10 @@ export class FlyoutButton cssClass += ' ' + this.cssClass; } + this.id = idGenerator.getNextUniqueId(); this.svgGroup = dom.createSvgElement( Svg.G, - {'class': cssClass, 'tabindex': '-1'}, + {'id': this.id, 'class': cssClass, 'tabindex': '-1'}, this.workspace.getCanvas(), ); diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index 5f4c57a5774..b99679db3fd 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -45,7 +45,7 @@ import type {IBoundedElement} from './interfaces/i_bounded_element.js'; import {IContextMenu} from './interfaces/i_contextmenu.js'; import type {IDragTarget} from './interfaces/i_drag_target.js'; import type {IFlyout} from './interfaces/i_flyout.js'; -import type {IFocusableNode} from './interfaces/i_focusable_node.js'; +import {isFocusableNode, type IFocusableNode} from './interfaces/i_focusable_node.js'; import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import type {IMetricsManager} from './interfaces/i_metrics_manager.js'; import type {IToolbox} from './interfaces/i_toolbox.js'; @@ -2693,6 +2693,19 @@ export class WorkspaceSvg /** See IFocusableTree.lookUpFocusableNode. */ lookUpFocusableNode(id: string): IFocusableNode | null { + // Check against flyout items if this workspace is part of a flyout. Note + // that blocks may match against this pass before reaching getBlockById() + // below (but only for a flyout workspace). + const flyout = this.targetWorkspace?.getFlyout(); + if (this.isFlyout && flyout) { + for (const flyoutItem of flyout.getContents()) { + const elem = flyoutItem.getElement(); + if (isFocusableNode(elem) && elem.getFocusableElement().id === id) { + return elem; + } + } + } + return this.getBlockById(id) as IFocusableNode; } From a9cf3d73990bd4e29ab24fc920057472d58f2f5c Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 30 Apr 2025 01:27:25 +0000 Subject: [PATCH 11/11] chore: lint fixes. --- core/flyout_button.ts | 2 +- core/workspace_svg.ts | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/core/flyout_button.ts b/core/flyout_button.ts index 21aa408e543..666a2e081bf 100644 --- a/core/flyout_button.ts +++ b/core/flyout_button.ts @@ -18,6 +18,7 @@ import type {IBoundedElement} from './interfaces/i_bounded_element.js'; import type {IFocusableNode} from './interfaces/i_focusable_node.js'; import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import type {IRenderedElement} from './interfaces/i_rendered_element.js'; +import {idGenerator} from './utils.js'; import {Coordinate} from './utils/coordinate.js'; import * as dom from './utils/dom.js'; import * as parsing from './utils/parsing.js'; @@ -26,7 +27,6 @@ import * as style from './utils/style.js'; import {Svg} from './utils/svg.js'; import type * as toolbox from './utils/toolbox.js'; import type {WorkspaceSvg} from './workspace_svg.js'; -import {idGenerator} from './utils.js'; /** * Class for a button or label in the flyout. diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index b99679db3fd..921fb72ecc7 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -45,7 +45,10 @@ import type {IBoundedElement} from './interfaces/i_bounded_element.js'; import {IContextMenu} from './interfaces/i_contextmenu.js'; import type {IDragTarget} from './interfaces/i_drag_target.js'; import type {IFlyout} from './interfaces/i_flyout.js'; -import {isFocusableNode, type IFocusableNode} from './interfaces/i_focusable_node.js'; +import { + isFocusableNode, + type IFocusableNode, +} from './interfaces/i_focusable_node.js'; import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import type {IMetricsManager} from './interfaces/i_metrics_manager.js'; import type {IToolbox} from './interfaces/i_toolbox.js';