Skip to content

feat: Automatically manage focus tree tab indexes #9079

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

Merged
merged 10 commits into from
May 29, 2025
5 changes: 2 additions & 3 deletions core/bubbles/bubble.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,8 @@ export abstract class Bubble implements IBubble, ISelectable {
* when automatically positioning.
* @param overriddenFocusableElement An optional replacement to the focusable
* element that's represented by this bubble (as a focusable node). This
* element will have its ID and tabindex overwritten. If not provided, the
* focusable element of this node will default to the bubble's SVG root.
* element will have its ID overwritten. If not provided, the focusable
* element of this node will default to the bubble's SVG root.
*/
constructor(
public readonly workspace: WorkspaceSvg,
Expand Down Expand Up @@ -138,7 +138,6 @@ export abstract class Bubble implements IBubble, ISelectable {

this.focusableElement = overriddenFocusableElement ?? this.svgRoot;
this.focusableElement.setAttribute('id', this.id);
this.focusableElement.setAttribute('tabindex', '-1');

browserEvents.conditionalBind(
this.background,
Expand Down
1 change: 0 additions & 1 deletion core/comments/rendered_workspace_comment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ export class RenderedWorkspaceComment
this.view.setEditable(this.isEditable());
this.view.getSvgRoot().setAttribute('data-id', this.id);
this.view.getSvgRoot().setAttribute('id', this.id);
this.view.getSvgRoot().setAttribute('tabindex', '-1');

this.addModelUpdateBindings();

Expand Down
1 change: 0 additions & 1 deletion core/field.ts
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,6 @@ export abstract class Field<T = any>
const id = this.id_;
if (!id) throw new Error('Expected ID to be defined prior to init.');
this.fieldGroup_ = dom.createSvgElement(Svg.G, {
'tabindex': '-1',
'id': id,
});
if (!this.isVisible()) {
Expand Down
75 changes: 44 additions & 31 deletions core/flyout_base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import {FlyoutItem} from './flyout_item.js';
import {FlyoutMetricsManager} from './flyout_metrics_manager.js';
import {FlyoutNavigator} from './flyout_navigator.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';
Expand Down Expand Up @@ -308,7 +307,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(
Expand All @@ -324,8 +322,6 @@ export abstract class Flyout
.getThemeManager()
.subscribe(this.svgBackground_, 'flyoutOpacity', 'fill-opacity');

getFocusManager().registerTree(this);

return this.svgGroup_;
}

Expand Down Expand Up @@ -407,7 +403,6 @@ export abstract class Flyout
if (this.svgGroup_) {
dom.removeNode(this.svgGroup_);
}
getFocusManager().unregisterTree(this);
}

/**
Expand Down Expand Up @@ -971,15 +966,22 @@ export abstract class Flyout
return null;
}

/** See IFocusableNode.getFocusableElement. */
/**
* See IFocusableNode.getFocusableElement.
*
* @deprecated v12: Use the Flyout's workspace for focus operations, instead.
*/
getFocusableElement(): HTMLElement | SVGElement {
if (!this.svgGroup_) throw new Error('Flyout DOM is not yet created.');
return this.svgGroup_;
throw new Error('Flyouts are not directly focusable.');
}

/** See IFocusableNode.getFocusableTree. */
/**
* See IFocusableNode.getFocusableTree.
*
* @deprecated v12: Use the Flyout's workspace for focus operations, instead.
*/
getFocusableTree(): IFocusableTree {
return this;
throw new Error('Flyouts are not directly focusable.');
}

/** See IFocusableNode.onNodeFocus. */
Expand All @@ -990,31 +992,45 @@ export abstract class Flyout

/** See IFocusableNode.canBeFocused. */
canBeFocused(): boolean {
return true;
return false;
}

/** See IFocusableTree.getRootFocusableNode. */
/**
* See IFocusableNode.getRootFocusableNode.
*
* @deprecated v12: Use the Flyout's workspace for focus operations, instead.
*/
getRootFocusableNode(): IFocusableNode {
return this;
throw new Error('Flyouts are not directly focusable.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a deprecation warning to these so we can remove them in v13?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in latest.

}

/** See IFocusableTree.getRestoredFocusableNode. */
/**
* See IFocusableNode.getRestoredFocusableNode.
*
* @deprecated v12: Use the Flyout's workspace for focus operations, instead.
*/
getRestoredFocusableNode(
_previousNode: IFocusableNode | null,
): IFocusableNode | null {
return null;
throw new Error('Flyouts are not directly focusable.');
}

/** See IFocusableTree.getNestedTrees. */
/**
* See IFocusableNode.getNestedTrees.
*
* @deprecated v12: Use the Flyout's workspace for focus operations, instead.
*/
getNestedTrees(): Array<IFocusableTree> {
return [this.workspace_];
throw new Error('Flyouts are not directly focusable.');
}

/** See IFocusableTree.lookUpFocusableNode. */
/**
* See IFocusableNode.lookUpFocusableNode.
*
* @deprecated v12: Use the Flyout's workspace for focus operations, instead.
*/
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;
throw new Error('Flyouts are not directly focusable.');
}

/** See IFocusableTree.onTreeFocus. */
Expand All @@ -1023,15 +1039,12 @@ export abstract class Flyout
_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);
/**
* See IFocusableNode.onTreeBlur.
*
* @deprecated v12: Use the Flyout's workspace for focus operations, instead.
*/
onTreeBlur(_nextTree: IFocusableTree | null): void {
throw new Error('Flyouts are not directly focusable.');
}
}
2 changes: 1 addition & 1 deletion core/flyout_button.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ export class FlyoutButton
this.id = idGenerator.getNextUniqueId();
this.svgGroup = dom.createSvgElement(
Svg.G,
{'id': this.id, 'class': cssClass, 'tabindex': '-1'},
{'id': this.id, 'class': cssClass},
this.workspace.getCanvas(),
);

Expand Down
Loading