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.

Author: BenHenning

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.

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.

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. */

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';

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;
   }
 

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;
   }

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();
   }
 
   /**

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) {