From 76c734598bfe3b4ff0cf31237d3064c528912ae0 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 1 Oct 2025 15:52:07 -0700 Subject: [PATCH] fix: Toolbox & Flyout ARIA positions (experimental) (#9394) ## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes #9386 Fixes part of #9293 ### Proposed Changes Addresses #9386 through a number of changes: - Ensures the flyout contents are reevaluated for ARIA changes whenever they themselves change (since previously `WorkspaceSvg` only recomputed its ARIA properties when one of its blocks self-registered which doesn't account for labels or buttons). - Collapsible categories are now correctly wrapped by a group (since groups of tree items must be in a parent group). - Updates toolbox ARIA computations to be on content changes rather than having items self-specify recomputing the ARIA properties. This mitigates an issue with collapsible categories not updating the toolbox contents and thus being omitted. - Introduced a separate pathway for computing tree info for flyout workspaces (vs. for the main workspace) in order to account for `FlyoutButton`s. - Updated `FlyoutButton` to use a nested structure of `treeitem` then `button` in order to actually count correctly in the tree and still be an interactive button. The readout experience is actually better now on ChromeVox, as well, since it reads out _both_ 'Button' and 'Tree item' which is interesting. It seems screen readers are designed to look for this pattern but it only works if set up in a very particular way. ### Reason for Changes Most of the changes here fixed incidental problems noticed while trying to address #9386 (such as the variables category not correctly accounting for the 'Create variable' button in the count, or not having the correct labels). Much of the count issues in the original issue were caused by a combination of missing some flyout items, and trying to compute the labels too early (i.e. before the categories were fully populated). ### Test Coverage Since this is an experimental change, no new tests were added. ### Documentation No documentation changes are directly needed here. ### Additional Information None. --- core/block_svg.ts | 2 +- core/flyout_base.ts | 2 + core/flyout_button.ts | 57 ++++++++++++++++---------- core/toolbox/category.ts | 2 - core/toolbox/collapsible_category.ts | 6 +-- core/toolbox/separator.ts | 2 - core/toolbox/toolbox.ts | 2 + core/workspace_svg.ts | 60 +++++++++++++++++++--------- 8 files changed, 84 insertions(+), 49 deletions(-) diff --git a/core/block_svg.ts b/core/block_svg.ts index a22af525c..693033090 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -262,7 +262,7 @@ export class BlockSvg collectSiblingBlocks(surroundParent: BlockSvg | null): BlockSvg[] { // NOTE TO DEVELOPERS: it's very important that these are NOT sorted. The - // returned list needs to be relatively stable for consistency block indexes + // returned list needs to be relatively stable for consistent block indexes // read out to users via screen readers. if (surroundParent) { // Start from the first sibling and iterate in navigation order. diff --git a/core/flyout_base.ts b/core/flyout_base.ts index 843d25de5..7caf98c9b 100644 --- a/core/flyout_base.ts +++ b/core/flyout_base.ts @@ -534,7 +534,9 @@ export abstract class Flyout */ setContents(contents: FlyoutItem[]): void { this.contents = contents; + this.workspace_.recomputeAriaTree(); } + /** * Update the display property of the flyout based whether it thinks it should * be visible and whether its containing workspace is visible. diff --git a/core/flyout_button.ts b/core/flyout_button.ts index ce82b5f9b..607825f05 100644 --- a/core/flyout_button.ts +++ b/core/flyout_button.ts @@ -60,7 +60,13 @@ export class FlyoutButton height = 0; /** The root SVG group for the button or label. */ - private svgGroup: SVGGElement; + private svgContainerGroup: SVGGElement; + + /** The root SVG group for the button's or label's contents. */ + private svgContentGroup: SVGGElement; + + /** The SVG group that can hold focus for this button or label. */ + private svgFocusableGroup: SVGGElement; /** The SVG element with the text of the label or button. */ private svgText: SVGTextElement | null = null; @@ -112,19 +118,27 @@ export class FlyoutButton } this.id = idGenerator.getNextUniqueId(); - this.svgGroup = dom.createSvgElement( + this.svgContainerGroup = dom.createSvgElement( Svg.G, - {'id': this.id, 'class': cssClass, 'tabindex': '-1'}, + {'class': cssClass}, this.workspace.getCanvas(), ); + this.svgContentGroup = dom.createSvgElement( + Svg.G, + {}, + this.svgContainerGroup, + ); - // Set the accessibility role. All child nodes will be set to `presentation` - // to avoid extraneous "group" narration on VoiceOver. + aria.setRole(this.svgContainerGroup, aria.Role.TREEITEM); if (this.isFlyoutLabel) { - aria.setRole(this.svgGroup, aria.Role.TREEITEM); + aria.setRole(this.svgContentGroup, aria.Role.PRESENTATION); + this.svgFocusableGroup = this.svgContainerGroup; } else { - aria.setRole(this.svgGroup, aria.Role.BUTTON); + aria.setRole(this.svgContentGroup, aria.Role.BUTTON); + this.svgFocusableGroup = this.svgContentGroup; } + this.svgFocusableGroup.id = this.id; + this.svgFocusableGroup.tabIndex = -1; let shadow; if (!this.isFlyoutLabel) { @@ -138,7 +152,7 @@ export class FlyoutButton 'x': 1, 'y': 1, }, - this.svgGroup!, + this.svgContentGroup, ); aria.setRole(shadow, aria.Role.PRESENTATION); } @@ -152,7 +166,7 @@ export class FlyoutButton 'rx': FlyoutButton.BORDER_RADIUS, 'ry': FlyoutButton.BORDER_RADIUS, }, - this.svgGroup!, + this.svgContentGroup, ); aria.setRole(rect, aria.Role.PRESENTATION); @@ -164,7 +178,7 @@ export class FlyoutButton 'y': 0, 'text-anchor': 'middle', }, - this.svgGroup!, + this.svgContentGroup, ); if (!this.isFlyoutLabel) { aria.setRole(svgText, aria.Role.PRESENTATION); @@ -181,6 +195,7 @@ export class FlyoutButton .getThemeManager() .subscribe(this.svgText, 'flyoutForegroundColour', 'fill'); } + aria.setState(this.svgFocusableGroup, aria.State.LABEL, text); const fontSize = style.getComputedStyle(svgText, 'fontSize'); const fontWeight = style.getComputedStyle(svgText, 'fontWeight'); @@ -217,13 +232,13 @@ export class FlyoutButton this.updateTransform(); this.onMouseDownWrapper = browserEvents.conditionalBind( - this.svgGroup, + this.svgContentGroup, 'pointerdown', this, this.onMouseDown, ); this.onMouseUpWrapper = browserEvents.conditionalBind( - this.svgGroup, + this.svgContentGroup, 'pointerup', this, this.onMouseUp, @@ -233,18 +248,18 @@ export class FlyoutButton createDom(): SVGElement { // No-op, now handled in constructor. Will be removed in followup refactor // PR that updates the flyout classes to use inflaters. - return this.svgGroup; + return this.svgContainerGroup; } /** Correctly position the flyout button and make it visible. */ show() { this.updateTransform(); - this.svgGroup!.setAttribute('display', 'block'); + this.svgContainerGroup!.setAttribute('display', 'block'); } /** Update SVG attributes to match internal state. */ private updateTransform() { - this.svgGroup!.setAttribute( + this.svgContainerGroup!.setAttribute( 'transform', 'translate(' + this.position.x + ',' + this.position.y + ')', ); @@ -330,8 +345,8 @@ export class FlyoutButton dispose() { browserEvents.unbind(this.onMouseDownWrapper); browserEvents.unbind(this.onMouseUpWrapper); - if (this.svgGroup) { - dom.removeNode(this.svgGroup); + if (this.svgContainerGroup) { + dom.removeNode(this.svgContainerGroup); } if (this.svgText) { this.workspace.getThemeManager().unsubscribe(this.svgText); @@ -349,8 +364,8 @@ export class FlyoutButton this.cursorSvg = null; return; } - if (this.svgGroup) { - this.svgGroup.appendChild(cursorSvg); + if (this.svgContainerGroup) { + this.svgContentGroup.appendChild(cursorSvg); this.cursorSvg = cursorSvg; } } @@ -398,12 +413,12 @@ export class FlyoutButton * @returns The root SVG element of this rendered element. */ getSvgRoot() { - return this.svgGroup; + return this.svgContainerGroup; } /** See IFocusableNode.getFocusableElement. */ getFocusableElement(): HTMLElement | SVGElement { - return this.svgGroup; + return this.svgFocusableGroup; } /** See IFocusableNode.getFocusableTree. */ diff --git a/core/toolbox/category.ts b/core/toolbox/category.ts index d86a41cb6..7b0db7b3f 100644 --- a/core/toolbox/category.ts +++ b/core/toolbox/category.ts @@ -30,7 +30,6 @@ import type { StaticCategoryInfo, } from '../utils/toolbox.js'; import * as toolbox from '../utils/toolbox.js'; -import {Toolbox} from './toolbox.js'; import {ToolboxItem} from './toolbox_item.js'; /** @@ -193,7 +192,6 @@ export class ToolboxCategory aria.setRole(this.htmlDiv_, aria.Role.TREEITEM); aria.setState(this.htmlDiv_, aria.State.SELECTED, false); aria.setState(this.htmlDiv_, aria.State.LEVEL, this.level_ + 1); - (this.parentToolbox_ as Toolbox).recomputeAriaOwners(); this.rowDiv_ = this.createRowContainer_(); this.rowDiv_.style.pointerEvents = 'auto'; diff --git a/core/toolbox/collapsible_category.ts b/core/toolbox/collapsible_category.ts index 342230fdd..7f8d8a915 100644 --- a/core/toolbox/collapsible_category.ts +++ b/core/toolbox/collapsible_category.ts @@ -20,7 +20,6 @@ import * as dom from '../utils/dom.js'; import * as toolbox from '../utils/toolbox.js'; import {ToolboxCategory} from './category.js'; import {ToolboxSeparator} from './separator.js'; -import {Toolbox} from './toolbox.js'; /** * Class for a category in a toolbox that can be collapsed. @@ -136,8 +135,7 @@ export class CollapsibleToolboxCategory this.htmlDiv_!.appendChild(this.subcategoriesDiv_); this.closeIcon_(this.iconDom_); aria.setState(this.htmlDiv_ as HTMLDivElement, aria.State.EXPANDED, false); - - aria.setRole(this.htmlDiv_!, aria.Role.GROUP); + aria.setRole(this.htmlDiv_!, aria.Role.TREEITEM); // Ensure this group has properly set children. const selectableChildren = @@ -150,7 +148,6 @@ export class CollapsibleToolboxCategory aria.State.OWNS, [...new Set(focusableChildIds)].join(' '), ); - (this.parentToolbox_ as Toolbox).recomputeAriaOwners(); return this.htmlDiv_!; } @@ -194,6 +191,7 @@ export class CollapsibleToolboxCategory newCategory.getClickTarget()?.setAttribute('id', newCategory.getId()); } } + aria.setRole(contentsContainer, aria.Role.GROUP); return contentsContainer; } diff --git a/core/toolbox/separator.ts b/core/toolbox/separator.ts index 9c2b5e6e2..517ed1601 100644 --- a/core/toolbox/separator.ts +++ b/core/toolbox/separator.ts @@ -17,7 +17,6 @@ import * as registry from '../registry.js'; import * as aria from '../utils/aria.js'; import * as dom from '../utils/dom.js'; import type * as toolbox from '../utils/toolbox.js'; -import {Toolbox} from './toolbox.js'; import {ToolboxItem} from './toolbox_item.js'; /** @@ -67,7 +66,6 @@ export class ToolboxSeparator extends ToolboxItem { this.htmlDiv = container; aria.setRole(this.htmlDiv, aria.Role.SEPARATOR); - (this.parentToolbox_ as Toolbox).recomputeAriaOwners(); return container; } diff --git a/core/toolbox/toolbox.ts b/core/toolbox/toolbox.ts index e6268b846..d006074d2 100644 --- a/core/toolbox/toolbox.ts +++ b/core/toolbox/toolbox.ts @@ -369,6 +369,7 @@ export class Toolbox this.renderContents_(toolboxDef['contents']); this.position(); this.handleToolboxItemResize(); + this.recomputeAriaOwners(); } /** @@ -445,6 +446,7 @@ export class Toolbox this.addToolboxItem_(child); } } + this.recomputeAriaOwners(); } /** diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index 2292b9b55..cc7da2fcf 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -19,7 +19,7 @@ import './events/events_theme_change.js'; import './events/events_viewport.js'; import type {Block} from './block.js'; -import type {BlockSvg} from './block_svg.js'; +import {BlockSvg} from './block_svg.js'; import type {BlocklyOptions} from './blockly_options.js'; import * as browserEvents from './browser_events.js'; import {TextInputBubble} from './bubbles/textinput_bubble.js'; @@ -42,7 +42,7 @@ import {Abstract as AbstractEvent} from './events/events.js'; 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 {FlyoutButton} from './flyout_button.js'; import {getFocusManager} from './focus_manager.js'; import {Gesture} from './gesture.js'; import {Grid} from './grid.js'; @@ -52,10 +52,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 { - isFocusableNode, - type IFocusableNode, -} from './interfaces/i_focusable_node.js'; +import {type IFocusableNode} from './interfaces/i_focusable_node.js'; import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import {hasBubble} from './interfaces/i_has_bubble.js'; import type {IMetricsManager} from './interfaces/i_metrics_manager.js'; @@ -2745,10 +2742,7 @@ export class WorkspaceSvg return ( flyout .getContents() - .find((flyoutItem) => { - const element = flyoutItem.getElement(); - return isFocusableNode(element) && element.canBeFocused(); - }) + .find((flyoutItem) => flyoutItem.getElement().canBeFocused()) ?.getElement() ?? null ); } @@ -2805,11 +2799,7 @@ export class WorkspaceSvg if (this.isFlyout && flyout) { for (const flyoutItem of flyout.getContents()) { const elem = flyoutItem.getElement(); - if ( - isFocusableNode(elem) && - elem.canBeFocused() && - elem.getFocusableElement().id === id - ) { + if (elem.canBeFocused() && elem.getFocusableElement().id === id) { return elem; } } @@ -2947,10 +2937,42 @@ export class WorkspaceSvg } recomputeAriaTree() { - // TODO: Do this efficiently (probably incrementally). - this.getTopBlocks(false).forEach((block) => - this.recomputeAriaTreeItemDetailsRecursively(block), - ); + // Flyout workspaces require special arrangement to account for items. + const flyout = this.targetWorkspace?.getFlyout(); + if (this.isFlyout && flyout) { + const focusableItems = flyout + .getContents() + .map((item) => item.getElement()) + .filter((item) => item.canBeFocused()); + focusableItems.forEach((item, index) => { + // This is rather hacky and may need more thought, but it's a + // consequence of actual button (non-label) FlyoutButtons requiring two + // distinct roles (a parent treeitem and a child button that actually + // holds focus). + // TODO: Figure out how to generalize this for arbitrary FlyoutItems + // that may require special handling like this (i.e. a treeitem wrapping + // an actual focusable element). + const treeItemElem = + item instanceof FlyoutButton + ? item.getSvgRoot() + : item.getFocusableElement(); + aria.setState(treeItemElem, aria.State.POSINSET, index + 1); + aria.setState(treeItemElem, aria.State.SETSIZE, focusableItems.length); + aria.setState(treeItemElem, aria.State.LEVEL, 1); // They are always top-level. + if (item instanceof BlockSvg) { + item + .getChildren(false) + .forEach((child) => + this.recomputeAriaTreeItemDetailsRecursively(child), + ); + } + }); + } else { + // TODO: Do this efficiently (probably incrementally). + this.getTopBlocks(false).forEach((block) => + this.recomputeAriaTreeItemDetailsRecursively(block), + ); + } } private recomputeAriaTreeItemDetailsRecursively(block: BlockSvg) {