Skip to content

Commit d82983f

Browse files
authored
feat: Make WorkspaceSvg and BlockSvg focusable (roll forward) (#8938)
_Note: This is a roll forward of #8916 that was reverted in #8933. See Additional Information below._ ## 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). - `css.ts` was updated to remove `blocklyActiveFocus` and `blocklyPassiveFocus` since these have unintended highlighting consequences that aren't actually desirable yet. Instead, the exact styling for active/passive focus will be iterated in the keyboard navigation plugin project and moved to Core once finalized. ### 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. This was originally merged in #8916 but was reverted in #8933 due to RaspberryPiFoundation/blockly-keyboard-experimentation#481. This actually contains no differences from the original PR except for `css.ts` which are documented above. It does employ a new merge strategy: all of the necessary PRs to move both Core and the plugin over to using `FocusManager` will be staged and merged in quick succession as ensuring the plugin works for each constituent change (vs. the final one) is quite complex. Thus, this PR *does* break the plugin, and won't be merged until its subsequent PRs are approved and also ready for merging. Edit: See #8938 (comment) for why this actually is being merged a bit sooner than originally planned. Keeping the original reasoning above for context.
1 parent b8c2b73 commit d82983f

File tree

5 files changed

+105
-19
lines changed

5 files changed

+105
-19
lines changed

core/block_svg.ts

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@ import {IContextMenu} from './interfaces/i_contextmenu.js';
4444
import type {ICopyable} from './interfaces/i_copyable.js';
4545
import {IDeletable} from './interfaces/i_deletable.js';
4646
import type {IDragStrategy, IDraggable} from './interfaces/i_draggable.js';
47+
import type {IFocusableNode} from './interfaces/i_focusable_node.js';
48+
import type {IFocusableTree} from './interfaces/i_focusable_tree.js';
4749
import {IIcon} from './interfaces/i_icon.js';
4850
import * as internalConstants from './internal_constants.js';
4951
import {MarkerManager} from './marker_manager.js';
@@ -76,7 +78,8 @@ export class BlockSvg
7678
IContextMenu,
7779
ICopyable<BlockCopyData>,
7880
IDraggable,
79-
IDeletable
81+
IDeletable,
82+
IFocusableNode
8083
{
8184
/**
8285
* Constant for identifying rows that are to be rendered inline.
@@ -210,6 +213,7 @@ export class BlockSvg
210213

211214
// Expose this block's ID on its top-level SVG group.
212215
this.svgGroup.setAttribute('data-id', this.id);
216+
svgPath.id = this.id;
213217

214218
this.doInit_();
215219
}
@@ -1827,4 +1831,26 @@ export class BlockSvg
18271831
);
18281832
}
18291833
}
1834+
1835+
/** See IFocusableNode.getFocusableElement. */
1836+
getFocusableElement(): HTMLElement | SVGElement {
1837+
return this.pathObject.svgPath;
1838+
}
1839+
1840+
/** See IFocusableNode.getFocusableTree. */
1841+
getFocusableTree(): IFocusableTree {
1842+
return this.workspace;
1843+
}
1844+
1845+
/** See IFocusableNode.onNodeFocus. */
1846+
onNodeFocus(): void {
1847+
common.setSelected(this);
1848+
}
1849+
1850+
/** See IFocusableNode.onNodeBlur. */
1851+
onNodeBlur(): void {
1852+
if (common.getSelected() === this) {
1853+
common.setSelected(null);
1854+
}
1855+
}
18301856
}

core/css.ts

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -463,8 +463,8 @@ input[type=number] {
463463
}
464464
465465
.blocklyMenuSeparator {
466-
background-color: #ccc;
467-
height: 1px;
466+
background-color: #ccc;
467+
height: 1px;
468468
border: 0;
469469
margin-left: 4px;
470470
margin-right: 4px;
@@ -494,12 +494,4 @@ input[type=number] {
494494
cursor: grabbing;
495495
}
496496
497-
.blocklyActiveFocus {
498-
outline-color: #2ae;
499-
outline-width: 2px;
500-
}
501-
.blocklyPassiveFocus {
502-
outline-color: #3fdfff;
503-
outline-width: 1.5px;
504-
}
505497
`;

core/inject.ts

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,11 @@ import * as common from './common.js';
1313
import * as Css from './css.js';
1414
import * as dropDownDiv from './dropdowndiv.js';
1515
import {Grid} from './grid.js';
16-
import {Msg} from './msg.js';
1716
import {Options} from './options.js';
1817
import {ScrollbarPair} from './scrollbar_pair.js';
1918
import {ShortcutRegistry} from './shortcut_registry.js';
2019
import * as Tooltip from './tooltip.js';
2120
import * as Touch from './touch.js';
22-
import * as aria from './utils/aria.js';
2321
import * as dom from './utils/dom.js';
2422
import {Svg} from './utils/svg.js';
2523
import * as WidgetDiv from './widgetdiv.js';
@@ -56,8 +54,6 @@ export function inject(
5654
if (opt_options?.rtl) {
5755
dom.addClass(subContainer, 'blocklyRTL');
5856
}
59-
subContainer.tabIndex = 0;
60-
aria.setState(subContainer, aria.State.LABEL, Msg['WORKSPACE_ARIA_LABEL']);
6157

6258
containerElement!.appendChild(subContainer);
6359
const svg = createDom(subContainer, options);
@@ -126,7 +122,6 @@ function createDom(container: HTMLElement, options: Options): SVGElement {
126122
'xmlns:xlink': dom.XLINK_NS,
127123
'version': '1.1',
128124
'class': 'blocklySvg',
129-
'tabindex': '0',
130125
},
131126
container,
132127
);

core/renderers/common/path_object.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ export class PathObject implements IPathObject {
6262
/** The primary path of the block. */
6363
this.svgPath = dom.createSvgElement(
6464
Svg.PATH,
65-
{'class': 'blocklyPath'},
65+
{'class': 'blocklyPath', 'tabindex': '-1'},
6666
this.svgRoot,
6767
);
6868

core/workspace_svg.ts

Lines changed: 75 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,13 +37,16 @@ import {EventType} from './events/type.js';
3737
import * as eventUtils from './events/utils.js';
3838
import {Flyout} from './flyout_base.js';
3939
import type {FlyoutButton} from './flyout_button.js';
40+
import {getFocusManager} from './focus_manager.js';
4041
import {Gesture} from './gesture.js';
4142
import {Grid} from './grid.js';
4243
import type {IASTNodeLocationSvg} from './interfaces/i_ast_node_location_svg.js';
4344
import type {IBoundedElement} from './interfaces/i_bounded_element.js';
4445
import {IContextMenu} from './interfaces/i_contextmenu.js';
4546
import type {IDragTarget} from './interfaces/i_drag_target.js';
4647
import type {IFlyout} from './interfaces/i_flyout.js';
48+
import type {IFocusableNode} from './interfaces/i_focusable_node.js';
49+
import type {IFocusableTree} from './interfaces/i_focusable_tree.js';
4750
import type {IMetricsManager} from './interfaces/i_metrics_manager.js';
4851
import type {IToolbox} from './interfaces/i_toolbox.js';
4952
import type {
@@ -54,6 +57,7 @@ import type {LineCursor} from './keyboard_nav/line_cursor.js';
5457
import type {Marker} from './keyboard_nav/marker.js';
5558
import {LayerManager} from './layer_manager.js';
5659
import {MarkerManager} from './marker_manager.js';
60+
import {Msg} from './msg.js';
5761
import {Options} from './options.js';
5862
import * as Procedures from './procedures.js';
5963
import * as registry from './registry.js';
@@ -66,6 +70,7 @@ import {Classic} from './theme/classic.js';
6670
import {ThemeManager} from './theme_manager.js';
6771
import * as Tooltip from './tooltip.js';
6872
import type {Trashcan} from './trashcan.js';
73+
import * as aria from './utils/aria.js';
6974
import * as arrayUtils from './utils/array.js';
7075
import {Coordinate} from './utils/coordinate.js';
7176
import * as dom from './utils/dom.js';
@@ -93,7 +98,7 @@ const ZOOM_TO_FIT_MARGIN = 20;
9398
*/
9499
export class WorkspaceSvg
95100
extends Workspace
96-
implements IASTNodeLocationSvg, IContextMenu
101+
implements IASTNodeLocationSvg, IContextMenu, IFocusableNode, IFocusableTree
97102
{
98103
/**
99104
* A wrapper function called when a resize event occurs.
@@ -764,7 +769,19 @@ export class WorkspaceSvg
764769
* <g class="blocklyBubbleCanvas"></g>
765770
* </g>
766771
*/
767-
this.svgGroup_ = dom.createSvgElement(Svg.G, {'class': 'blocklyWorkspace'});
772+
this.svgGroup_ = dom.createSvgElement(Svg.G, {
773+
'class': 'blocklyWorkspace',
774+
// Only the top-level workspace should be tabbable.
775+
'tabindex': injectionDiv ? '0' : '-1',
776+
'id': this.id,
777+
});
778+
if (injectionDiv) {
779+
aria.setState(
780+
this.svgGroup_,
781+
aria.State.LABEL,
782+
Msg['WORKSPACE_ARIA_LABEL'],
783+
);
784+
}
768785

769786
// Note that a <g> alone does not receive mouse events--it must have a
770787
// valid target inside it. If no background class is specified, as in the
@@ -840,6 +857,9 @@ export class WorkspaceSvg
840857
this.getTheme(),
841858
isParentWorkspace ? this.getInjectionDiv() : undefined,
842859
);
860+
861+
getFocusManager().registerTree(this);
862+
843863
return this.svgGroup_;
844864
}
845865

@@ -924,6 +944,10 @@ export class WorkspaceSvg
924944
document.body.removeEventListener('wheel', this.dummyWheelListener);
925945
this.dummyWheelListener = null;
926946
}
947+
948+
if (getFocusManager().isRegistered(this)) {
949+
getFocusManager().unregisterTree(this);
950+
}
927951
}
928952

929953
/**
@@ -2631,6 +2655,55 @@ export class WorkspaceSvg
26312655
deltaY *= scale;
26322656
this.scroll(this.scrollX + deltaX, this.scrollY + deltaY);
26332657
}
2658+
2659+
/** See IFocusableNode.getFocusableElement. */
2660+
getFocusableElement(): HTMLElement | SVGElement {
2661+
return this.svgGroup_;
2662+
}
2663+
2664+
/** See IFocusableNode.getFocusableTree. */
2665+
getFocusableTree(): IFocusableTree {
2666+
return this;
2667+
}
2668+
2669+
/** See IFocusableNode.onNodeFocus. */
2670+
onNodeFocus(): void {}
2671+
2672+
/** See IFocusableNode.onNodeBlur. */
2673+
onNodeBlur(): void {}
2674+
2675+
/** See IFocusableTree.getRootFocusableNode. */
2676+
getRootFocusableNode(): IFocusableNode {
2677+
return this;
2678+
}
2679+
2680+
/** See IFocusableTree.getRestoredFocusableNode. */
2681+
getRestoredFocusableNode(
2682+
previousNode: IFocusableNode | null,
2683+
): IFocusableNode | null {
2684+
if (!previousNode) {
2685+
return this.getTopBlocks(true)[0] ?? null;
2686+
} else return null;
2687+
}
2688+
2689+
/** See IFocusableTree.getNestedTrees. */
2690+
getNestedTrees(): Array<IFocusableTree> {
2691+
return [];
2692+
}
2693+
2694+
/** See IFocusableTree.lookUpFocusableNode. */
2695+
lookUpFocusableNode(id: string): IFocusableNode | null {
2696+
return this.getBlockById(id) as IFocusableNode;
2697+
}
2698+
2699+
/** See IFocusableTree.onTreeFocus. */
2700+
onTreeFocus(
2701+
_node: IFocusableNode,
2702+
_previousTree: IFocusableTree | null,
2703+
): void {}
2704+
2705+
/** See IFocusableTree.onTreeBlur. */
2706+
onTreeBlur(_nextTree: IFocusableTree | null): void {}
26342707
}
26352708

26362709
/**

0 commit comments

Comments
 (0)