-
Notifications
You must be signed in to change notification settings - Fork 3.8k
feat: Make WorkspaceSvg and BlockSvg focusable #8916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
41c8215
14b486e
5ef2d7e
4479b82
2637736
49192ba
a346a92
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 { | ||
|
|
@@ -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 | |
| * <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. | ||
|
||
| '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 | ||
|
|
@@ -840,6 +857,9 @@ export class WorkspaceSvg | |
| this.getTheme(), | ||
| isParentWorkspace ? this.getInjectionDiv() : undefined, | ||
| ); | ||
|
|
||
| getFocusManager().registerTree(this); | ||
|
|
||
| return this.svgGroup_; | ||
| } | ||
|
|
||
|
|
@@ -924,6 +944,10 @@ export class WorkspaceSvg | |
| document.body.removeEventListener('wheel', this.dummyWheelListener); | ||
| this.dummyWheelListener = null; | ||
| } | ||
|
|
||
| if (getFocusManager().isRegistered(this)) { | ||
| getFocusManager().unregisterTree(this); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -2618,6 +2642,55 @@ export class WorkspaceSvg | |
| deltaY *= scale; | ||
| this.scroll(this.scrollX + deltaX, this.scrollY + deltaY); | ||
| } | ||
|
|
||
| /** See IFocusableNode.getFocusableElement. */ | ||
| getFocusableElement(): HTMLElement | SVGElement { | ||
| return this.svgGroup_; | ||
| } | ||
|
|
||
| /** See IFocusableNode.getFocusableTree. */ | ||
| getFocusableTree(): IFocusableTree { | ||
| return this; | ||
| } | ||
|
|
||
| /** See IFocusableNode.onNodeFocus. */ | ||
| onNodeFocus(): void {} | ||
|
|
||
| /** See IFocusableNode.onNodeBlur. */ | ||
| onNodeBlur(): void {} | ||
|
|
||
| /** See IFocusableTree.getRootFocusableNode. */ | ||
| getRootFocusableNode(): IFocusableNode { | ||
| return this; | ||
| } | ||
|
|
||
| /** See IFocusableTree.getRestoredFocusableNode. */ | ||
| getRestoredFocusableNode( | ||
| previousNode: IFocusableNode | null, | ||
| ): IFocusableNode | null { | ||
| if (!previousNode) { | ||
| return this.getTopBlocks(true)[0] ?? null; | ||
| } else return null; | ||
| } | ||
|
|
||
| /** See IFocusableTree.getNestedTrees. */ | ||
| getNestedTrees(): Array<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 {} | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
setSelecteddirectly here and do some manual verification, then file a ticket to follow up with comprehensive tests for that case.There was a problem hiding this comment.
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.