Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion core/block_svg.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -76,7 +78,8 @@ export class BlockSvg
IContextMenu,
ICopyable<BlockCopyData>,
IDraggable,
IDeletable
IDeletable,
IFocusableNode
{
/**
* Constant for identifying rows that are to be rendered inline.
Expand Down Expand Up @@ -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_();
}
Expand Down Expand Up @@ -1819,4 +1823,28 @@ export class BlockSvg
);
}
}

/** See IFocusableNode.getFocusableElement. */
getFocusableElement(): HTMLElement | SVGElement {
return this.pathObject.svgPath;
}

/** See IFocusableNode.getFocusableTree. */
getFocusableTree(): IFocusableTree {
return this.workspace;
}

/** See IFocusableNode.onNodeFocus. */
onNodeFocus(): void {
if (!this.isShadow()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if it is a shadow block?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was actually copying https://github.com/google/blockly/blob/ac7fea11cffc4c445cedae77675290861ceec2f6/core/keyboard_nav/line_cursor.ts#L652-L657 but through a lot of code transforming. In retrospect, this is probably wrong.

To be fair, I'm not 100% certain how shadow blocks will behave and that will need testing. The field will receive focus, and the shadow block might be focusable if clicked on but will never otherwise be navigated to per the cursor's behavior (I think). So this should be safe to remove and just call setSelected() directly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also not sure. I think call setSelected directly here and do some manual verification, then file a ticket to follow up with comprehensive tests for that case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far I'm not seeing any issues, so I don't think there's a particular concern to address.

I've made a note in #8915 regarding this specific case.

common.setSelected(this);
}
}

/** See IFocusableNode.onNodeBlur. */
onNodeBlur(): void {
if (common.getSelected() === this) {
common.setSelected(null);
}
}
}
5 changes: 0 additions & 5 deletions core/inject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -126,7 +122,6 @@ function createDom(container: HTMLElement, options: Options): SVGElement {
'xmlns:xlink': dom.XLINK_NS,
'version': '1.1',
'class': 'blocklySvg',
'tabindex': '0',
},
container,
);
Expand Down
77 changes: 75 additions & 2 deletions core/workspace_svg.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,16 @@ import {EventType} from './events/type.js';
import * as eventUtils from './events/utils.js';
import {Flyout} from './flyout_base.js';
import type {FlyoutButton} from './flyout_button.js';
import {getFocusManager} from './focus_manager.js';
import {Gesture} from './gesture.js';
import {Grid} from './grid.js';
import type {IASTNodeLocationSvg} from './interfaces/i_ast_node_location_svg.js';
import type {IBoundedElement} from './interfaces/i_bounded_element.js';
import {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 {
Expand All @@ -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';
Expand All @@ -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';
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -764,7 +769,19 @@ export class WorkspaceSvg
* <g class="blocklyBubbleCanvas"></g>
* </g>
*/
this.svgGroup_ = dom.createSvgElement(Svg.G, {'class': 'blocklyWorkspace'});
this.svgGroup_ = dom.createSvgElement(Svg.G, {
'class': 'blocklyWorkspace',
// Only the main workspace should be tabbable.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Say "top level" rather than "main", because Blockly.getMainWorkspace() already exists and refers to the currently active workspace.

From context, I'm assuming that the distinction is between top level and flyout or mutator workspaces.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct--fixed!

'tabindex': injectionDiv ? '0' : '-1',
'id': this.id,
});
if (injectionDiv) {
aria.setState(
this.svgGroup_,
aria.State.LABEL,
Msg['WORKSPACE_ARIA_LABEL'],
);
}

// Note that a <g> alone does not receive mouse events--it must have a
// valid target inside it. If no background class is specified, as in the
Expand Down Expand Up @@ -840,6 +857,9 @@ export class WorkspaceSvg
this.getTheme(),
isParentWorkspace ? this.getInjectionDiv() : undefined,
);

getFocusManager().registerTree(this);

return this.svgGroup_;
}

Expand Down Expand Up @@ -924,6 +944,10 @@ export class WorkspaceSvg
document.body.removeEventListener('wheel', this.dummyWheelListener);
this.dummyWheelListener = null;
}

if (getFocusManager().isRegistered(this)) {
getFocusManager().unregisterTree(this);
}
}

/**
Expand Down Expand Up @@ -2618,6 +2642,55 @@ export class WorkspaceSvg
deltaY *= scale;
this.scroll(this.scrollX + deltaX, this.scrollY + deltaY);
}

/** See IFocusableNode.getFocusableElement. */
getFocusableElement(): HTMLElement | SVGElement {
return this.svgGroup_;
}

/** See IFocusableNode.getFocusableTree. */
getFocusableTree(): IFocusableTree {
return this;
}

/** See IFocusableNode.onNodeFocus. */
onNodeFocus(): void {}

/** See IFocusableNode.onNodeBlur. */
onNodeBlur(): void {}

/** See IFocusableTree.getRootFocusableNode. */
getRootFocusableNode(): IFocusableNode {
return this;
}

/** See IFocusableTree.getRestoredFocusableNode. */
getRestoredFocusableNode(
previousNode: IFocusableNode | null,
): IFocusableNode | null {
if (!previousNode) {
return this.getTopBlocks(true)[0] ?? null;
} else return null;
}

/** See IFocusableTree.getNestedTrees. */
getNestedTrees(): Array<IFocusableTree> {
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 {}
}

/**
Expand Down
Loading