fix: Fix bug that prevented keyboard navigation in flyouts. (#8687)

* fix: Fix bug that prevented keyboard navigation in flyouts.

* refactor: Add an `isFocusable()` method to FlyoutItem.

core/block_flyout_inflater.ts

@@ -10,7 +10,7 @@ import * as common from './common.js';
 import {MANUALLY_DISABLED} from './constants.js';
 import type {Abstract as AbstractEvent} from './events/events_abstract.js';
 import {EventType} from './events/type.js';
-import type {IBoundedElement} from './interfaces/i_bounded_element.js';
+import {FlyoutItem} from './flyout_item.js';
 import type {IFlyout} from './interfaces/i_flyout.js';
 import type {IFlyoutInflater} from './interfaces/i_flyout_inflater.js';
 import * as registry from './registry.js';
@@ -27,6 +27,8 @@ import * as Xml from './xml.js';
 const WORKSPACE_AT_BLOCK_CAPACITY_DISABLED_REASON =
   'WORKSPACE_AT_BLOCK_CAPACITY';
 
+const BLOCK_TYPE = 'block';
+
 /**
  * Class responsible for creating blocks for flyouts.
  */
@@ -51,7 +53,7 @@ export class BlockFlyoutInflater implements IFlyoutInflater {
    * @param flyoutWorkspace The workspace to create the block on.
    * @returns A newly created block.
    */
-  load(state: object, flyoutWorkspace: WorkspaceSvg): IBoundedElement {
+  load(state: object, flyoutWorkspace: WorkspaceSvg): FlyoutItem {
     this.setFlyoutWorkspace(flyoutWorkspace);
     this.flyout = flyoutWorkspace.targetWorkspace?.getFlyout() ?? undefined;
     const block = this.createBlock(state as BlockInfo, flyoutWorkspace);
@@ -70,7 +72,7 @@ export class BlockFlyoutInflater implements IFlyoutInflater {
     block.getDescendants(false).forEach((b) => (b.isInFlyout = true));
     this.addBlockListeners(block);
 
-    return block;
+    return new FlyoutItem(block, BLOCK_TYPE, true);
   }
 
   /**
@@ -114,7 +116,7 @@ export class BlockFlyoutInflater implements IFlyoutInflater {
    * @param defaultGap The default spacing for flyout items.
    * @returns The amount of space that should follow this block.
    */
-  gapForElement(state: object, defaultGap: number): number {
+  gapForItem(state: object, defaultGap: number): number {
     const blockState = state as BlockInfo;
     let gap;
     if (blockState['gap']) {
@@ -134,9 +136,10 @@ export class BlockFlyoutInflater implements IFlyoutInflater {
   /**
    * Disposes of the given block.
    *
-   * @param element The flyout block to dispose of.
+   * @param item The flyout block to dispose of.
    */
-  disposeElement(element: IBoundedElement): void {
+  disposeItem(item: FlyoutItem): void {
+    const element = item.getElement();
     if (!(element instanceof BlockSvg)) return;
     this.removeListeners(element.id);
     element.dispose(false, false);
@@ -257,6 +260,19 @@ export class BlockFlyoutInflater implements IFlyoutInflater {
       }
     });
   }
+
+  /**
+   * Returns the type of items this inflater is responsible for creating.
+   *
+   * @returns An identifier for the type of items this inflater creates.
+   */
+  getType() {
+    return BLOCK_TYPE;
+  }
 }
 
-registry.register(registry.Type.FLYOUT_INFLATER, 'block', BlockFlyoutInflater);
+registry.register(
+  registry.Type.FLYOUT_INFLATER,
+  BLOCK_TYPE,
+  BlockFlyoutInflater,
+);

core/blockly.ts

@@ -99,9 +99,10 @@ import {
   FieldVariableFromJsonConfig,
   FieldVariableValidator,
 } from './field_variable.js';
-import {Flyout, FlyoutItem} from './flyout_base.js';
+import {Flyout} from './flyout_base.js';
 import {FlyoutButton} from './flyout_button.js';
 import {HorizontalFlyout} from './flyout_horizontal.js';
+import {FlyoutItem} from './flyout_item.js';
 import {FlyoutMetricsManager} from './flyout_metrics_manager.js';
 import {FlyoutSeparator} from './flyout_separator.js';
 import {VerticalFlyout} from './flyout_vertical.js';

core/button_flyout_inflater.ts

@@ -5,12 +5,14 @@
  */
 
 import {FlyoutButton} from './flyout_button.js';
-import type {IBoundedElement} from './interfaces/i_bounded_element.js';
+import {FlyoutItem} from './flyout_item.js';
 import type {IFlyoutInflater} from './interfaces/i_flyout_inflater.js';
 import * as registry from './registry.js';
 import {ButtonOrLabelInfo} from './utils/toolbox.js';
 import type {WorkspaceSvg} from './workspace_svg.js';
 
+const BUTTON_TYPE = 'button';
+
 /**
  * Class responsible for creating buttons for flyouts.
  */
@@ -22,7 +24,7 @@ export class ButtonFlyoutInflater implements IFlyoutInflater {
    * @param flyoutWorkspace The workspace to create the button on.
    * @returns A newly created FlyoutButton.
    */
-  load(state: object, flyoutWorkspace: WorkspaceSvg): IBoundedElement {
+  load(state: object, flyoutWorkspace: WorkspaceSvg): FlyoutItem {
     const button = new FlyoutButton(
       flyoutWorkspace,
       flyoutWorkspace.targetWorkspace!,
@@ -30,7 +32,8 @@ export class ButtonFlyoutInflater implements IFlyoutInflater {
       false,
     );
     button.show();
-    return button;
+
+    return new FlyoutItem(button, BUTTON_TYPE, true);
   }
 
   /**
@@ -40,24 +43,34 @@ export class ButtonFlyoutInflater implements IFlyoutInflater {
    * @param defaultGap The default spacing for flyout items.
    * @returns The amount of space that should follow this button.
    */
-  gapForElement(state: object, defaultGap: number): number {
+  gapForItem(state: object, defaultGap: number): number {
     return defaultGap;
   }
 
   /**
    * Disposes of the given button.
    *
-   * @param element The flyout button to dispose of.
+   * @param item The flyout button to dispose of.
    */
-  disposeElement(element: IBoundedElement): void {
+  disposeItem(item: FlyoutItem): void {
+    const element = item.getElement();
     if (element instanceof FlyoutButton) {
       element.dispose();
     }
   }
+
+  /**
+   * Returns the type of items this inflater is responsible for creating.
+   *
+   * @returns An identifier for the type of items this inflater creates.
+   */
+  getType() {
+    return BUTTON_TYPE;
+  }
 }
 
 registry.register(
   registry.Type.FLYOUT_INFLATER,
-  'button',
+  BUTTON_TYPE,
   ButtonFlyoutInflater,
 );

core/flyout_base.ts

@@ -18,16 +18,17 @@ import {DeleteArea} from './delete_area.js';
 import type {Abstract as AbstractEvent} from './events/events_abstract.js';
 import {EventType} from './events/type.js';
 import * as eventUtils from './events/utils.js';
+import {FlyoutItem} from './flyout_item.js';
 import {FlyoutMetricsManager} from './flyout_metrics_manager.js';
 import {FlyoutSeparator, SeparatorAxis} from './flyout_separator.js';
 import {IAutoHideable} from './interfaces/i_autohideable.js';
-import type {IBoundedElement} from './interfaces/i_bounded_element.js';
 import type {IFlyout} from './interfaces/i_flyout.js';
 import type {IFlyoutInflater} from './interfaces/i_flyout_inflater.js';
 import type {Options} from './options.js';
 import * as registry from './registry.js';
 import * as renderManagement from './render_management.js';
 import {ScrollbarPair} from './scrollbar_pair.js';
+import {SEPARATOR_TYPE} from './separator_flyout_inflater.js';
 import * as blocks from './serialization/blocks.js';
 import {Coordinate} from './utils/coordinate.js';
 import * as dom from './utils/dom.js';
@@ -677,20 +678,19 @@ export abstract class Flyout
       const type = info['kind'].toLowerCase();
       const inflater = this.getInflaterForType(type);
       if (inflater) {
-        const element = inflater.load(info, this.getWorkspace());
-        contents.push({
-          type,
-          element,
-        });
-        const gap = inflater.gapForElement(info, defaultGap);
+        contents.push(inflater.load(info, this.getWorkspace()));
+        const gap = inflater.gapForItem(info, defaultGap);
         if (gap) {
-          contents.push({
-            type: 'sep',
-            element: new FlyoutSeparator(
-              gap,
-              this.horizontalLayout ? SeparatorAxis.X : SeparatorAxis.Y,
+          contents.push(
+            new FlyoutItem(
+              new FlyoutSeparator(
+                gap,
+                this.horizontalLayout ? SeparatorAxis.X : SeparatorAxis.Y,
+              ),
+              SEPARATOR_TYPE,
+              false,
             ),
-          });
+          );
         }
       }
     }
@@ -711,9 +711,12 @@ export abstract class Flyout
    */
   protected normalizeSeparators(contents: FlyoutItem[]): FlyoutItem[] {
     for (let i = contents.length - 1; i > 0; i--) {
-      const elementType = contents[i].type.toLowerCase();
-      const previousElementType = contents[i - 1].type.toLowerCase();
-      if (elementType === 'sep' && previousElementType === 'sep') {
+      const elementType = contents[i].getType().toLowerCase();
+      const previousElementType = contents[i - 1].getType().toLowerCase();
+      if (
+        elementType === SEPARATOR_TYPE &&
+        previousElementType === SEPARATOR_TYPE
+      ) {
         // Remove previousElement from the array, shifting the current element
         // forward as a result. This preserves the behavior where explicit
         // separator elements override the value of prior implicit (or explicit)
@@ -752,9 +755,9 @@ export abstract class Flyout
    * Delete elements from a previous showing of the flyout.
    */
   private clearOldBlocks() {
-    this.getContents().forEach((element) => {
-      const inflater = this.getInflaterForType(element.type);
-      inflater?.disposeElement(element.element);
+    this.getContents().forEach((item) => {
+      const inflater = this.getInflaterForType(item.getType());
+      inflater?.disposeItem(item);
     });
 
     // Clear potential variables from the previous showing.
@@ -959,11 +962,3 @@ export abstract class Flyout
     return null;
   }
 }
-
-/**
- * A flyout content item.
- */
-export interface FlyoutItem {
-  type: string;
-  element: IBoundedElement;
-}

core/flyout_horizontal.ts

@@ -13,7 +13,8 @@
 
 import * as browserEvents from './browser_events.js';
 import * as dropDownDiv from './dropdowndiv.js';
-import {Flyout, FlyoutItem} from './flyout_base.js';
+import {Flyout} from './flyout_base.js';
+import type {FlyoutItem} from './flyout_item.js';
 import type {Options} from './options.js';
 import * as registry from './registry.js';
 import {Scrollbar} from './scrollbar.js';
@@ -263,10 +264,10 @@ export class HorizontalFlyout extends Flyout {
     }
 
     for (const item of contents) {
-      const rect = item.element.getBoundingRectangle();
+      const rect = item.getElement().getBoundingRectangle();
       const moveX = this.RTL ? cursorX + rect.getWidth() : cursorX;
-      item.element.moveBy(moveX, cursorY);
-      cursorX += item.element.getBoundingRectangle().getWidth();
+      item.getElement().moveBy(moveX, cursorY);
+      cursorX += item.getElement().getBoundingRectangle().getWidth();
     }
   }
 
@@ -336,7 +337,7 @@ export class HorizontalFlyout extends Flyout {
     let flyoutHeight = this.getContents().reduce((maxHeightSoFar, item) => {
       return Math.max(
         maxHeightSoFar,
-        item.element.getBoundingRectangle().getHeight(),
+        item.getElement().getBoundingRectangle().getHeight(),
       );
     }, 0);
     flyoutHeight += this.MARGIN * 1.5;

core/flyout_item.ts

@@ -0,0 +1,42 @@
+import type {IBoundedElement} from './interfaces/i_bounded_element.js';
+
+/**
+ * Representation of an item displayed in a flyout.
+ */
+export class FlyoutItem {
+  /**
+   * Creates a new FlyoutItem.
+   *
+   * @param element The element that will be displayed in the flyout.
+   * @param type The type of element. Should correspond to the type of the
+   *     flyout inflater that created this object.
+   * @param focusable True if the element should be allowed to be focused by
+   *     e.g. keyboard navigation in the flyout.
+   */
+  constructor(
+    private element: IBoundedElement,
+    private type: string,
+    private focusable: boolean,
+  ) {}
+
+  /**
+   * Returns the element displayed in the flyout.
+   */
+  getElement() {
+    return this.element;
+  }
+
+  /**
+   * Returns the type of flyout element this item represents.
+   */
+  getType() {
+    return this.type;
+  }
+
+  /**
+   * Returns whether or not the flyout element can receive focus.
+   */
+  isFocusable() {
+    return this.focusable;
+  }
+}

core/flyout_vertical.ts

@@ -13,7 +13,8 @@
 
 import * as browserEvents from './browser_events.js';
 import * as dropDownDiv from './dropdowndiv.js';
-import {Flyout, FlyoutItem} from './flyout_base.js';
+import {Flyout} from './flyout_base.js';
+import type {FlyoutItem} from './flyout_item.js';
 import type {Options} from './options.js';
 import * as registry from './registry.js';
 import {Scrollbar} from './scrollbar.js';
@@ -229,8 +230,8 @@ export class VerticalFlyout extends Flyout {
     let cursorY = margin;
 
     for (const item of contents) {
-      item.element.moveBy(cursorX, cursorY);
-      cursorY += item.element.getBoundingRectangle().getHeight();
+      item.getElement().moveBy(cursorX, cursorY);
+      cursorY += item.getElement().getBoundingRectangle().getHeight();
     }
   }
 
@@ -301,7 +302,7 @@ export class VerticalFlyout extends Flyout {
     let flyoutWidth = this.getContents().reduce((maxWidthSoFar, item) => {
       return Math.max(
         maxWidthSoFar,
-        item.element.getBoundingRectangle().getWidth(),
+        item.getElement().getBoundingRectangle().getWidth(),
       );
     }, 0);
     flyoutWidth += this.MARGIN * 1.5 + this.tabWidth_;
@@ -312,13 +313,13 @@ export class VerticalFlyout extends Flyout {
       if (this.RTL) {
         // With the flyoutWidth known, right-align the flyout contents.
         for (const item of this.getContents()) {
-          const oldX = item.element.getBoundingRectangle().left;
+          const oldX = item.getElement().getBoundingRectangle().left;
           const newX =
             flyoutWidth / this.workspace_.scale -
-            item.element.getBoundingRectangle().getWidth() -
+            item.getElement().getBoundingRectangle().getWidth() -
             this.MARGIN -
             this.tabWidth_;
-          item.element.moveBy(newX - oldX, 0);
+          item.getElement().moveBy(newX - oldX, 0);
         }
       }
 

core/interfaces/i_flyout.ts

@@ -7,7 +7,7 @@
 // Former goog.module ID: Blockly.IFlyout
 
 import type {BlockSvg} from '../block_svg.js';
-import {FlyoutItem} from '../flyout_base.js';
+import type {FlyoutItem} from '../flyout_item.js';
 import type {Coordinate} from '../utils/coordinate.js';
 import type {Svg} from '../utils/svg.js';
 import type {FlyoutDefinition} from '../utils/toolbox.js';

core/interfaces/i_flyout_inflater.ts

@@ -1,5 +1,5 @@
+import type {FlyoutItem} from '../flyout_item.js';
 import type {WorkspaceSvg} from '../workspace_svg.js';
-import type {IBoundedElement} from './i_bounded_element.js';
 
 export interface IFlyoutInflater {
   /**
@@ -16,7 +16,7 @@ export interface IFlyoutInflater {
    *    element, however.
    * @returns The newly inflated flyout element.
    */
-  load(state: object, flyoutWorkspace: WorkspaceSvg): IBoundedElement;
+  load(state: object, flyoutWorkspace: WorkspaceSvg): FlyoutItem;
 
   /**
    * Returns the amount of spacing that should follow the element corresponding
@@ -26,7 +26,7 @@ export interface IFlyoutInflater {
    * @param defaultGap The default gap for elements in this flyout.
    * @returns The gap that should follow the given element.
    */
-  gapForElement(state: object, defaultGap: number): number;
+  gapForItem(state: object, defaultGap: number): number;
 
   /**
    * Disposes of the given element.
@@ -37,5 +37,15 @@ export interface IFlyoutInflater {
    *
    * @param element The flyout element to dispose of.
    */
-  disposeElement(element: IBoundedElement): void;
+  disposeItem(item: FlyoutItem): void;
+
+  /**
+   * Returns the type of items that this inflater is responsible for inflating.
+   * This should be the same as the name under which this inflater registers
+   * itself, as well as the value returned by `getType()` on the `FlyoutItem`
+   * objects returned by `load()`.
+   *
+   * @returns The type of items this inflater creates.
+   */
+  getType(): string;
 }

core/keyboard_nav/ast_node.ts

@@ -17,8 +17,8 @@ import {BlockSvg} from '../block_svg.js';
 import type {Connection} from '../connection.js';
 import {ConnectionType} from '../connection_type.js';
 import type {Field} from '../field.js';
-import {FlyoutItem} from '../flyout_base.js';
 import {FlyoutButton} from '../flyout_button.js';
+import type {FlyoutItem} from '../flyout_item.js';
 import type {Input} from '../inputs/input.js';
 import type {IASTNodeLocation} from '../interfaces/i_ast_node_location.js';
 import type {IASTNodeLocationWithBlock} from '../interfaces/i_ast_node_location_with_block.js';
@@ -348,10 +348,11 @@ export class ASTNode {
     );
     if (!nextItem) return null;
 
-    if (nextItem.element instanceof FlyoutButton) {
-      return ASTNode.createButtonNode(nextItem.element);
-    } else if (nextItem.element instanceof BlockSvg) {
-      return ASTNode.createStackNode(nextItem.element);
+    const element = nextItem.getElement();
+    if (element instanceof FlyoutButton) {
+      return ASTNode.createButtonNode(element);
+    } else if (element instanceof BlockSvg) {
+      return ASTNode.createStackNode(element);
     }
 
     return null;
@@ -373,13 +374,13 @@ export class ASTNode {
     const currentIndex = flyoutContents.findIndex((item: FlyoutItem) => {
       if (
         currentLocation instanceof BlockSvg &&
-        item.element === currentLocation
+        item.getElement() === currentLocation
       ) {
         return true;
       }
       if (
         currentLocation instanceof FlyoutButton &&
-        item.element === currentLocation
+        item.getElement() === currentLocation
       ) {
         return true;
       }
@@ -388,7 +389,17 @@ export class ASTNode {
 
     if (currentIndex < 0) return null;
 
-    const resultIndex = forward ? currentIndex + 1 : currentIndex - 1;
+    let resultIndex = forward ? currentIndex + 1 : currentIndex - 1;
+    // The flyout may contain non-focusable elements like spacers or custom
+    // items. If the next/previous element is one of those, keep looking until a
+    // focusable element is encountered.
+    while (
+      resultIndex >= 0 &&
+      resultIndex < flyoutContents.length &&
+      !flyoutContents[resultIndex].isFocusable()
+    ) {
+      resultIndex += forward ? 1 : -1;
+    }
     if (resultIndex === -1 || resultIndex === flyoutContents.length) {
       return null;
     }

core/label_flyout_inflater.ts

@@ -5,12 +5,14 @@
  */
 
 import {FlyoutButton} from './flyout_button.js';
-import type {IBoundedElement} from './interfaces/i_bounded_element.js';
+import {FlyoutItem} from './flyout_item.js';
 import type {IFlyoutInflater} from './interfaces/i_flyout_inflater.js';
 import * as registry from './registry.js';
 import {ButtonOrLabelInfo} from './utils/toolbox.js';
 import type {WorkspaceSvg} from './workspace_svg.js';
 
+const LABEL_TYPE = 'label';
+
 /**
  * Class responsible for creating labels for flyouts.
  */
@@ -22,7 +24,7 @@ export class LabelFlyoutInflater implements IFlyoutInflater {
    * @param flyoutWorkspace The workspace to create the label on.
    * @returns A FlyoutButton configured as a label.
    */
-  load(state: object, flyoutWorkspace: WorkspaceSvg): IBoundedElement {
+  load(state: object, flyoutWorkspace: WorkspaceSvg): FlyoutItem {
     const label = new FlyoutButton(
       flyoutWorkspace,
       flyoutWorkspace.targetWorkspace!,
@@ -30,7 +32,8 @@ export class LabelFlyoutInflater implements IFlyoutInflater {
       true,
     );
     label.show();
-    return label;
+
+    return new FlyoutItem(label, LABEL_TYPE, true);
   }
 
   /**
@@ -40,20 +43,34 @@ export class LabelFlyoutInflater implements IFlyoutInflater {
    * @param defaultGap The default spacing for flyout items.
    * @returns The amount of space that should follow this label.
    */
-  gapForElement(state: object, defaultGap: number): number {
+  gapForItem(state: object, defaultGap: number): number {
     return defaultGap;
   }
 
   /**
    * Disposes of the given label.
    *
-   * @param element The flyout label to dispose of.
+   * @param item The flyout label to dispose of.
    */
-  disposeElement(element: IBoundedElement): void {
+  disposeItem(item: FlyoutItem): void {
+    const element = item.getElement();
     if (element instanceof FlyoutButton) {
       element.dispose();
     }
   }
+
+  /**
+   * Returns the type of items this inflater is responsible for creating.
+   *
+   * @returns An identifier for the type of items this inflater creates.
+   */
+  getType() {
+    return LABEL_TYPE;
+  }
 }
 
-registry.register(registry.Type.FLYOUT_INFLATER, 'label', LabelFlyoutInflater);
+registry.register(
+  registry.Type.FLYOUT_INFLATER,
+  LABEL_TYPE,
+  LabelFlyoutInflater,
+);

core/separator_flyout_inflater.ts

@@ -4,13 +4,18 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+import {FlyoutItem} from './flyout_item.js';
 import {FlyoutSeparator, SeparatorAxis} from './flyout_separator.js';
-import type {IBoundedElement} from './interfaces/i_bounded_element.js';
 import type {IFlyoutInflater} from './interfaces/i_flyout_inflater.js';
 import * as registry from './registry.js';
 import type {SeparatorInfo} from './utils/toolbox.js';
 import type {WorkspaceSvg} from './workspace_svg.js';
 
+/**
+ * @internal
+ */
+export const SEPARATOR_TYPE = 'sep';
+
 /**
  * Class responsible for creating separators for flyouts.
  */
@@ -33,12 +38,13 @@ export class SeparatorFlyoutInflater implements IFlyoutInflater {
    * @param flyoutWorkspace The workspace the separator belongs to.
    * @returns A newly created FlyoutSeparator.
    */
-  load(_state: object, flyoutWorkspace: WorkspaceSvg): IBoundedElement {
+  load(_state: object, flyoutWorkspace: WorkspaceSvg): FlyoutItem {
     const flyoutAxis = flyoutWorkspace.targetWorkspace?.getFlyout()
       ?.horizontalLayout
       ? SeparatorAxis.X
       : SeparatorAxis.Y;
-    return new FlyoutSeparator(0, flyoutAxis);
+    const separator = new FlyoutSeparator(0, flyoutAxis);
+    return new FlyoutItem(separator, SEPARATOR_TYPE, false);
   }
 
   /**
@@ -48,7 +54,7 @@ export class SeparatorFlyoutInflater implements IFlyoutInflater {
    * @param defaultGap The default spacing for flyout items.
    * @returns The desired size of the separator.
    */
-  gapForElement(state: object, defaultGap: number): number {
+  gapForItem(state: object, defaultGap: number): number {
     const separatorState = state as SeparatorInfo;
     const newGap = parseInt(String(separatorState['gap']));
     return newGap ?? defaultGap;
@@ -57,13 +63,22 @@ export class SeparatorFlyoutInflater implements IFlyoutInflater {
   /**
    * Disposes of the given separator. Intentional no-op.
    *
-   * @param _element The flyout separator to dispose of.
+   * @param _item The flyout separator to dispose of.
    */
-  disposeElement(_element: IBoundedElement): void {}
+  disposeItem(_item: FlyoutItem): void {}
+
+  /**
+   * Returns the type of items this inflater is responsible for creating.
+   *
+   * @returns An identifier for the type of items this inflater creates.
+   */
+  getType() {
+    return SEPARATOR_TYPE;
+  }
 }
 
 registry.register(
   registry.Type.FLYOUT_INFLATER,
-  'sep',
+  SEPARATOR_TYPE,
   SeparatorFlyoutInflater,
 );