fix: improve fallback behavior for custom input labels (#9942)

* fix: refactor custom input labels to simplify

* fix: handle bad field image config from block factory

* chore: remove stray log

* fix: fix move mode labels

* fix: dont use numbered inputs for dummy and end row inputs

* chore: fix test

Author: maribethb

packages/blockly/core/block_aria_composer.ts

@@ -50,18 +50,18 @@ export enum ConnectionPreposition {
  * The returned label will be specialized based on whether the block is part of a
  * flyout.
  *
+ * Custom input labels (from {@link Input.setAriaLabelProvider}) are not included
+ * here; they are used only in move-mode disambiguation and parent-input context
+ * via {@link Input.getAriaLabelText}.
+ *
  * @internal
  * @param block The block for which an ARIA representation should be created.
  * @param verbosity How much detail to include in the description.
- * @param useCustomInputLabels Whether to use custom labels for inputs, if they
- *   exist. We don't want to do this when just reading a block's label, but do
- *   want to in other scenarios such as move mode.
  * @returns The ARIA representation for the specified block.
  */
 export function computeAriaLabel(
   block: BlockSvg,
   verbosity = Verbosity.STANDARD,
-  useCustomInputLabels = true,
 ) {
   if (block.isSimpleReporter()) {
     // special case for full-block field blocks.
@@ -73,7 +73,7 @@ export function computeAriaLabel(
   return [
     verbosity >= Verbosity.STANDARD && getBeginStackLabel(block),
     getParentInputLabel(block),
-    ...getInputLabels(block, verbosity, useCustomInputLabels),
+    ...getInputLabels(block, verbosity),
     verbosity === Verbosity.LOQUACIOUS && getParentToolboxCategoryLabel(block),
     verbosity >= Verbosity.STANDARD && getDisabledLabel(block),
     verbosity >= Verbosity.STANDARD && getCollapsedLabel(block),
@@ -197,6 +197,10 @@ export function computeFieldRowLabel(
  *   statement input of the parent block (in this case, the label
  *   would be redundant with the parent block's label)
  *
+ * For statement inputs without their own field labels, labels from other
+ * inputs in the same statement section are included (via
+ * {@link getInputLabelsSubset}), consistent with move-target disambiguation.
+ *
  * For statement inputs, the resolved label (whether custom or fallback) is
  * wrapped in the "Begin %1" prefix so the readout indicates that the child
  * block starts the body of the statement input.
@@ -235,7 +239,14 @@ function getParentInputLabel(block: BlockSvg) {
       return undefined;
     }
 
-    inputLabel = computeFieldRowLabel(parentInput, true);
+    const sectionLabels = getInputLabelsSubset(
+      parentBlock as BlockSvg,
+      parentInput,
+    );
+    if (!sectionLabels.length) {
+      return undefined;
+    }
+    inputLabel = sectionLabels.join(', ');
   }
 
   if (parentInput.type === inputTypes.STATEMENT) {
@@ -272,21 +283,18 @@ function getBeginStackLabel(block: BlockSvg) {
  * their contents are returned as a single item in the array per top-level
  * input.
  *
- * Generally, if a custom label for an input is provided, that is preferred.
- * However, we do not surface the custom labels when simply reading the text of
- * the block. They are used as supplementary information for situations like
- * move mode or when an input itself is focused.
+ * Uses derived labels only (field row text and connected block content via
+ * {@link Input.getLabel}). Custom input labels are not included; see
+ * {@link Input.getAriaLabelText} for move-mode and parent-input usage.
  *
  * @internal
  * @param block The block to retrieve a list of field/input labels for.
- * @param verbosity
- * @param useCustomLabels whether to use the custom label for an input, if it's present.
+ * @param verbosity How much detail to include in each input label.
  * @returns A list of field/input labels for the given block.
  */
 export function getInputLabels(
   block: BlockSvg,
   verbosity = Verbosity.STANDARD,
-  useCustomLabels = true,
 ): string[] {
   const visibleInputs = block.inputList.filter((input) => input.isVisible());
   let inputsToLabel = visibleInputs;
@@ -306,15 +314,13 @@ export function getInputLabels(
     }
   }
 
-  return inputsToLabel.map((input) => {
-    const customLabel = useCustomLabels ? input.getAriaLabelText() : null;
-    return customLabel ?? input.getLabel(verbosity, useCustomLabels);
-  });
+  return inputsToLabel.map((input) => input.getLabel(verbosity));
 }
 
 /**
- * Returns a subset of labels for inputs on the given block, ending at the
- * specified input.
+ * Returns a subset of derived labels for inputs on the given block, ending at
+ * the specified input. Used to disambiguate move targets and connection
+ * highlights when no custom label is set.
  *
  * The subset is determined based on the input type:
  * - For non-statement inputs, only the label for the given input is returned.
@@ -323,41 +329,74 @@ export function getInputLabels(
  *   begins immediately after the previous statement input, or at the start of
  *   the block if none exists.
  *
+ * Label resolution (see also {@link computeMoveConnectionLabel}):
+ * 1. Custom labels ({@link Input.getAriaLabelText}) are handled by callers, not here.
+ * 2. Derived labels from {@link Input.getLabel} (field row + child blocks).
+ * 3. Numbered fallback ({@link Msg.INPUT_LABEL_INDEX}) when tier 2 is empty.
+ *    For the statement target input, the fallback is omitted if any earlier
+ *    input in the subset already produced a label.
+ *
  * @internal
  * @param block The block to retrieve a list of field/input labels for.
  * @param input The input that defines the end of the subset.
  * @returns A list of field/input labels for the given block.
  */
-export function getInputLabelsSubset(
-  block: BlockSvg,
-  input: Input,
-  includeFallbackLabels = true,
-): string[] {
+export function getInputLabelsSubset(block: BlockSvg, input: Input): string[] {
   const inputIndex = block.inputList.indexOf(input);
   if (inputIndex === -1) {
     throw new Error(
       `Input with name "${input.name}" not found on block with id "${block.id}".`,
     );
   }
+  const isStatementTarget = input.type === inputTypes.STATEMENT;
 
-  const startIndex =
-    input.type === inputTypes.STATEMENT
-      ? findStartOfStatementSection(block.inputList, inputIndex)
-      : inputIndex;
+  const startIndex = isStatementTarget
+    ? findStartOfStatementSection(block.inputList, inputIndex)
+    : inputIndex;
 
-  return block.inputList
+  // For statement inputs, we include all visible inputs from the start
+  // of the current statement section up to and including the target input.
+  // For non-statement inputs, this will just be the target input itself.
+  const inputsInSubset = block.inputList
     .slice(startIndex, inputIndex + 1)
-    .filter((input) => input.isVisible())
-    .map(
-      (input) =>
-        input.getLabel(Verbosity.TERSE, false) ||
-        (includeFallbackLabels
-          ? Msg['INPUT_LABEL_INDEX'].replace(
-              '%1',
-              (input.getIndex() + 1).toString(),
-            )
-          : undefined),
-    )
+    .filter((subsetInput) => subsetInput.isVisible());
+
+  // The derived labels are based on the field row and any connected child
+  // blocks.
+  const derivedLabels = inputsInSubset.map((subsetInput) =>
+    subsetInput.getLabel(Verbosity.TERSE),
+  );
+
+  // For statement inputs, we only include the fallback label ("input %1")
+  // for the target input if no preceding input in the subset has a label.
+  // This prevents, e.g., "else" statement inputs from being read as "else, input 2".
+  const precedingLabelsProvideContext =
+    isStatementTarget && derivedLabels.slice(0, -1).some((label) => !!label);
+
+  return derivedLabels
+    .map((label, index) => {
+      if (label) {
+        return label;
+      }
+      const subsetInput = inputsInSubset[index];
+      // Dummy and end-row inputs are not connection inputs; getIndex() is -1
+      // and would produce a misleading "input 0" fallback label.
+      if (
+        subsetInput.type === inputTypes.DUMMY ||
+        subsetInput.type === inputTypes.END_ROW
+      ) {
+        return undefined;
+      }
+      const isStatementTargetInput =
+        isStatementTarget && index === derivedLabels.length - 1;
+      if (isStatementTargetInput && precedingLabelsProvideContext) {
+        return undefined;
+      }
+      return Msg['INPUT_LABEL_INDEX'].replace(
+        '%1',
+        (subsetInput.getIndex() + 1).toString(),
+      );
+    })
     .filter((label) => label !== undefined);
 }
 
@@ -468,7 +507,13 @@ function getAnnouncementTemplate(preposition: ConnectionPreposition): string {
 }
 
 /**
- * Returns a label for a connection includes either a block label, input label or both.
+ * Returns a label for a connection that includes either a block label, input
+ * label, or both.
+ *
+ * Input label resolution:
+ * 1. Custom label from {@link Input.getAriaLabelText} when set.
+ * 2. Otherwise derived labels from {@link getInputLabelsSubset} (field row,
+ *    child blocks, and numbered fallbacks as needed).
  *
  * @param conn The connection to generate a label for.
  * @param baseLabel An optional block label to include in the returned string.
@@ -483,8 +528,6 @@ function computeMoveConnectionLabel(
 
   let inputLabel = input.getAriaLabelText();
 
-  // If the input doesn't have a custom ARIA label, compute one using the labels from
-  // nearby fields.
   if (!inputLabel) {
     const labels = getInputLabelsSubset(conn.getSourceBlock(), input);
     if (!labels.length) return baseLabel;

packages/blockly/core/block_svg.ts

@@ -2034,7 +2034,7 @@ export class BlockSvg
    * @returns An accessibility description of this block.
    */
   getAriaLabel(verbosity: aria.Verbosity) {
-    return computeAriaLabel(this, verbosity, false);
+    return computeAriaLabel(this, verbosity);
   }
 
   /**
@@ -2051,7 +2051,7 @@ export class BlockSvg
       block = block.getNextBlock();
     }
     if (count <= 1) {
-      return computeAriaLabel(this, aria.Verbosity.TERSE, false);
+      return computeAriaLabel(this, aria.Verbosity.TERSE);
     }
 
     const labelTemplate = Msg['BLOCK_LABEL_STACK_BLOCKS'];

packages/blockly/core/dragging/block_drag_strategy.ts

@@ -1277,7 +1277,6 @@ export class BlockDragStrategy implements IDragStrategy {
    * @param block The block to re-disable, if applicable.
    */
   private redisableAllDraggedBlocks(block: BlockSvg) {
-    console.log('redisable');
     const oldUndo = eventUtils.getRecordUndo();
     eventUtils.setRecordUndo(false);
     block.getDescendants(false).forEach((descendant) => {

packages/blockly/core/field_image.ts

@@ -113,9 +113,18 @@ export class FieldImage extends Field<string> {
 
     if (config) {
       this.configure_(config);
+    } else if (isFieldImageConfig(alt)) {
+      // Block Factory and some hand-written blocks pass a config object as the
+      // fourth argument instead of using the seventh `config` parameter.
+      // This is wrong, and typescript will complain about it, but handle it
+      // for backwards compatibility.
+      this.configure_(alt);
     } else {
       this.flipRtl = !!flipRtl;
-      this.altText = parsing.replaceMessageReferences(alt) || '';
+      this.altText =
+        typeof alt === 'string'
+          ? parsing.replaceMessageReferences(alt) || ''
+          : '';
     }
     this.setValue(parsing.replaceMessageReferences(src));
   }
@@ -372,6 +381,22 @@ export class FieldImage extends Field<string> {
   }
 }
 
+/**
+ * Returns whether a value is a FieldImage config object passed in place of alt
+ * text (e.g. `{alt: '*', flipRtl: false}`). You shouldn't do this on purpose,
+ * but the block factory generates block definitions in this format.
+ *
+ * @param value The value to test.
+ */
+function isFieldImageConfig(value: unknown): value is FieldImageConfig {
+  return (
+    typeof value === 'object' &&
+    value !== null &&
+    !Array.isArray(value) &&
+    ('alt' in value || 'flipRtl' in value)
+  );
+}
+
 fieldRegistry.register('field_image', FieldImage);
 
 FieldImage.prototype.DEFAULT_VALUE = '';

packages/blockly/core/inputs/input.ts

@@ -399,13 +399,14 @@ export class Input {
   }
 
   /**
-   * Returns an accessibility label describing this input, including the labels
-   * of any fields on the input and the labels of any connected blocks, to help
-   * disambiguate this input from others on the same block.
+   * Returns a derived accessibility label for this input: field row text plus
+   * labels of any connected child blocks. Does not include custom labels from
+   * {@link getAriaLabelText}; those are used in move-mode and parent-input
+   * context only.
    *
    * @internal
    */
-  getLabel(verbosity = Verbosity.STANDARD, useCustomLabels = true): string {
+  getLabel(verbosity = Verbosity.STANDARD): string {
     if (!this.isVisible()) return '';
 
     const labels = computeFieldRowLabel(this, false, verbosity);
@@ -414,11 +415,7 @@ export class Input {
       const childBlock = this.connection.targetBlock();
       if (childBlock && !childBlock.isInsertionMarker()) {
         labels.push(
-          getInputLabels(
-            childBlock as BlockSvg,
-            verbosity,
-            useCustomLabels,
-          ).join(', '),
+          getInputLabels(childBlock as BlockSvg, verbosity).join(', '),
         );
       }
     }

packages/blockly/core/rendered_connection.ts

@@ -355,14 +355,11 @@ export class RenderedConnection
 
     // Use the custom label for an input if it exists, otherwise use the
     // "field row" approach to get the default label for the input.
-    // Don't include the "input 1" fallback for default labels, since
-    // the input is already being described as a statement or value input.
     const parentInputLabel =
       parentInput?.getAriaLabelText() ??
       getInputLabelsSubset(
         parentInput.getSourceBlock() as BlockSvg,
         parentInput,
-        false,
       ).join(', ');
     if (this.type === ConnectionType.NEXT_STATEMENT) {
       aria.setState(

packages/blockly/core/shortcut_items.ts

@@ -807,11 +807,7 @@ export function registerFocusToolbox() {
  */
 export function registerReadInformation() {
   const announceBlockInformation = (block: BlockSvg) => {
-    const description = computeAriaLabel(
-      block,
-      aria.Verbosity.LOQUACIOUS,
-      false,
-    );
+    const description = computeAriaLabel(block, aria.Verbosity.LOQUACIOUS);
     aria.announceDynamicAriaState(description);
   };
 
@@ -898,14 +894,12 @@ export function registerReadExtendedInformation() {
       }
 
       if (startBlock !== block) {
-        toAnnounce.push(
-          computeAriaLabel(startBlock, aria.Verbosity.TERSE, false),
-        );
+        toAnnounce.push(computeAriaLabel(startBlock, aria.Verbosity.TERSE));
       }
 
       let parent = startBlock.getParent();
       while (parent) {
-        toAnnounce.push(computeAriaLabel(parent, aria.Verbosity.TERSE, false));
+        toAnnounce.push(computeAriaLabel(parent, aria.Verbosity.TERSE));
         parent = parent.getParent();
       }
 
@@ -917,7 +911,7 @@ export function registerReadExtendedInformation() {
           toAnnounce.push(
             Msg['CURRENT_BLOCK_ANNOUNCEMENT'].replace(
               '%1',
-              computeAriaLabel(block, aria.Verbosity.TERSE, false),
+              computeAriaLabel(block, aria.Verbosity.TERSE),
             ),
           );
         }

packages/blockly/demos/blockfactory/factory_utils.js

@@ -458,7 +458,7 @@ FactoryUtils.getFieldsJs_ = function(block) {
           }
           break;
         case 'field_image':
-          // Result: new Blockly.FieldImage('http://...', 80, 60, '*')
+          // Result: new Blockly.FieldImage('http://...', 80, 60, {alt: '*', flipRtl: false})
           var src = JSON.stringify(block.getFieldValue('SRC'));
           var width = Number(block.getFieldValue('WIDTH'));
           var height = Number(block.getFieldValue('HEIGHT'));