fix: zelos full block fields rendering badly (#7490)

* fix: text input fields * fix: colour field * chore: cleanup override keywords * chore: revert playground changes * fix: applyColour not being called * chore: swap visibility for display * chore: cleanup for PR comments * fix: PR comments
2026-03-17 10:40:10 +01:00 · 2023-09-14 15:22:40 -07:00
parent 00d870ee03
commit 25d15fd596
4 changed files with 201 additions and 78 deletions
--- a/core/block.ts
+++ b/core/block.ts
@@ -939,6 +939,19 @@ export class Block implements IASTNodeLocation, IDeletable {
    return this.disposed;
  }

+  /**
+   * @returns True if this block is a value block with a single editable field.
+   * @internal
+   */
+  isSimpleReporter(): boolean {
+    if (!this.outputConnection) return false;
+
+    for (const input of this.inputList) {
+      if (input.connection || input.fieldRow.length > 1) return false;
+    }
+    return true;
+  }
+
  /**
   * Find the connection on this block that corresponds to the given connection
   * on the other block.
--- a/core/field.ts
+++ b/core/field.ts
@@ -330,6 +330,18 @@ export abstract class Field<T = any>
   */
  initModel() {}

+  /**
+   * Defines whether this field should take up the full block or not.
+   *
+   * Be cautious when overriding this function. It may not work as you expect /
+   * intend because the behavior was kind of hacked in. If you are thinking
+   * about overriding this function, post on the forum with your intended
+   * behavior to see if there's another approach.
+   */
+  protected isFullBlockField(): boolean {
+    return !this.borderRect_;
+  }
+
  /**
   * Create a field border rect element. Not to be overridden by subclasses.
   * Instead modify the result of the function inside initView, or create a
@@ -797,7 +809,7 @@ export abstract class Field<T = any>
    const xOffset =
      margin !== undefined
        ? margin
-        : this.borderRect_
+        : !this.isFullBlockField()
        ? this.getConstants()!.FIELD_BORDER_RECT_X_PADDING
        : 0;
    let totalWidth = xOffset * 2;
@@ -813,7 +825,7 @@ export abstract class Field<T = any>
      );
      totalWidth += contentWidth;
    }
-    if (this.borderRect_) {
+    if (!this.isFullBlockField()) {
      totalHeight = Math.max(totalHeight, constants!.FIELD_BORDER_RECT_HEIGHT);
    }

@@ -922,7 +934,7 @@ export abstract class Field<T = any>
      throw new UnattachedFieldError();
    }

-    if (!this.borderRect_) {
+    if (this.isFullBlockField()) {
      // Browsers are inconsistent in what they return for a bounding box.
      // - Webkit / Blink: fill-box / object bounding box
      // - Gecko: stroke-box
@@ -940,8 +952,8 @@ export abstract class Field<T = any>
        xy.y -= 0.5 * scale;
      }
    } else {
-      const bBox = this.borderRect_.getBoundingClientRect();
-      xy = style.getPageOffset(this.borderRect_);
+      const bBox = this.borderRect_!.getBoundingClientRect();
+      xy = style.getPageOffset(this.borderRect_!);
      scaledWidth = bBox.width;
      scaledHeight = bBox.height;
    }
--- a/core/field_colour.ts
+++ b/core/field_colour.ts
@@ -19,7 +19,12 @@ import * as browserEvents from './browser_events.js';
 import * as Css from './css.js';
 import * as dom from './utils/dom.js';
 import * as dropDownDiv from './dropdowndiv.js';
-import {Field, FieldConfig, FieldValidator} from './field.js';
+import {
+  Field,
+  FieldConfig,
+  FieldValidator,
+  UnattachedFieldError,
+} from './field.js';
 import * as fieldRegistry from './field_registry.js';
 import * as aria from './utils/aria.js';
 import * as colour from './utils/colour.js';
@@ -168,29 +173,118 @@ export class FieldColour extends Field<string> {
      this.getConstants()!.FIELD_COLOUR_DEFAULT_WIDTH,
      this.getConstants()!.FIELD_COLOUR_DEFAULT_HEIGHT,
    );
-    if (!this.getConstants()!.FIELD_COLOUR_FULL_BLOCK) {
-      this.createBorderRect_();
-      this.getBorderRect().style['fillOpacity'] = '1';
-    } else if (this.sourceBlock_ instanceof BlockSvg) {
-      this.clickTarget_ = this.sourceBlock_.getSvgRoot();
+    this.createBorderRect_();
+    this.getBorderRect().style['fillOpacity'] = '1';
+    this.getBorderRect().setAttribute('stroke', '#fff');
+    if (this.isFullBlockField()) {
+      this.clickTarget_ = (this.sourceBlock_ as BlockSvg).getSvgRoot();
    }
  }

+  protected override isFullBlockField(): boolean {
+    const block = this.getSourceBlock();
+    if (!block) throw new UnattachedFieldError();
+
+    const constants = this.getConstants();
+    return block.isSimpleReporter() && !!constants?.FIELD_COLOUR_FULL_BLOCK;
+  }
+
  /**
   * Updates text field to match the colour/style of the block.
   */
  override applyColour() {
-    if (!this.getConstants()!.FIELD_COLOUR_FULL_BLOCK) {
-      if (this.borderRect_) {
-        this.borderRect_.style.fill = this.getValue() as string;
-      }
-    } else if (this.sourceBlock_ instanceof BlockSvg) {
-      this.sourceBlock_.pathObject.svgPath.setAttribute(
-        'fill',
-        this.getValue() as string,
-      );
-      this.sourceBlock_.pathObject.svgPath.setAttribute('stroke', '#fff');
+    const block = this.getSourceBlock() as BlockSvg | null;
+    if (!block) throw new UnattachedFieldError();
+
+    if (!this.fieldGroup_) return;
+
+    const borderRect = this.borderRect_;
+    if (!borderRect) {
+      throw new Error('The border rect has not been initialized');
    }
+
+    if (!this.isFullBlockField()) {
+      borderRect.style.display = 'block';
+      borderRect.style.fill = this.getValue() as string;
+    } else {
+      borderRect.style.display = 'none';
+      // In general, do *not* let fields control the color of blocks. Having the
+      // field control the color is unexpected, and could have performance
+      // impacts.
+      block.pathObject.svgPath.setAttribute('fill', this.getValue() as string);
+      block.pathObject.svgPath.setAttribute('stroke', '#fff');
+    }
+  }
+
+  /**
+   * Returns the height and width of the field.
+   *
+   * This should *in general* be the only place render_ gets called from.
+   *
+   * @returns Height and width.
+   */
+  override getSize(): Size {
+    if (this.getConstants()?.FIELD_COLOUR_FULL_BLOCK) {
+      // In general, do *not* let fields control the color of blocks. Having the
+      // field control the color is unexpected, and could have performance
+      // impacts.
+      // Full block fields have more control of the block than they should
+      // (i.e. updating fill colour). Whenever we get the size, the field may
+      // no longer be a full-block field, so we need to rerender.
+      this.render_();
+      this.isDirty_ = false;
+    }
+    return super.getSize();
+  }
+
+  /**
+   * Updates the colour of the block to reflect whether this is a full
+   * block field or not.
+   */
+  protected override render_() {
+    super.render_();
+
+    const block = this.getSourceBlock() as BlockSvg | null;
+    if (!block) throw new UnattachedFieldError();
+    // In general, do *not* let fields control the color of blocks. Having the
+    // field control the color is unexpected, and could have performance
+    // impacts.
+    // Whenever we render, the field may no longer be a full-block-field so
+    // we need to update the colour.
+    if (this.getConstants()!.FIELD_COLOUR_FULL_BLOCK) block.applyColour();
+  }
+
+  /**
+   * Updates the size of the field based on whether it is a full block field
+   * or not.
+   *
+   * @param margin margin to use when positioning the field.
+   */
+  protected updateSize_(margin?: number) {
+    const constants = this.getConstants();
+    const xOffset =
+      margin !== undefined
+        ? margin
+        : !this.isFullBlockField()
+        ? constants!.FIELD_BORDER_RECT_X_PADDING
+        : 0;
+    let totalWidth = xOffset * 2;
+    let contentWidth = 0;
+    if (!this.isFullBlockField()) {
+      contentWidth = constants!.FIELD_COLOUR_DEFAULT_WIDTH;
+      totalWidth += contentWidth;
+    }
+
+    let totalHeight = constants!.FIELD_TEXT_HEIGHT;
+    if (!this.isFullBlockField()) {
+      totalHeight = Math.max(totalHeight, constants!.FIELD_BORDER_RECT_HEIGHT);
+    }
+
+    this.size_.height = totalHeight;
+    this.size_.width = totalWidth;
+
+    this.positionTextElement_(xOffset, contentWidth);
+    this.positionBorderRect_();
  }

  /**
@@ -206,26 +300,6 @@ export class FieldColour extends Field<string> {
    return colour.parse(newValue);
  }

-  /**
-   * Update the value of this colour field, and update the displayed colour.
-   *
-   * @param newValue The value to be saved. The default validator guarantees
-   *     that this is a colour in '#rrggbb' format.
-   */
-  protected override doValueUpdate_(newValue: string) {
-    this.value_ = newValue;
-    if (this.borderRect_) {
-      this.borderRect_.style.fill = newValue;
-    } else if (
-      this.sourceBlock_ &&
-      this.sourceBlock_.rendered &&
-      this.sourceBlock_ instanceof BlockSvg
-    ) {
-      this.sourceBlock_.pathObject.svgPath.setAttribute('fill', newValue);
-      this.sourceBlock_.pathObject.svgPath.setAttribute('stroke', '#fff');
-    }
-  }
-
  /**
   * Get the text for this field.  Used when the block is collapsed.
   *
--- a/core/field_input.ts
+++ b/core/field_input.ts
@@ -34,6 +34,7 @@ import * as userAgent from './utils/useragent.js';
 import * as WidgetDiv from './widgetdiv.js';
 import type {WorkspaceSvg} from './workspace_svg.js';
 import * as renderManagement from './render_management.js';
+import {Size} from './utils/size.js';

 /**
 * Supported types for FieldInput subclasses.
@@ -88,7 +89,7 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
   * Whether the field should consider the whole parent block to be its click
   * target.
   */
-  fullBlockClickTarget_: boolean | null = false;
+  fullBlockClickTarget_: boolean = false;

  /** The workspace that this field belongs to. */
  protected workspace_: WorkspaceSvg | null = null;
@@ -142,37 +143,22 @@ export abstract class FieldInput<T extends InputTypes> extends Field<

  override initView() {
    const block = this.getSourceBlock();
-    if (!block) {
-      throw new UnattachedFieldError();
-    }
-    if (this.getConstants()!.FULL_BLOCK_FIELDS) {
-      // Step one: figure out if this is the only field on this block.
-      // Rendering is quite different in that case.
-      let nFields = 0;
-      let nConnections = 0;
-      // Count the number of fields, excluding text fields
-      for (let i = 0, input; (input = block.inputList[i]); i++) {
-        for (let j = 0; input.fieldRow[j]; j++) {
-          nFields++;
-        }
-        if (input.connection) {
-          nConnections++;
-        }
-      }
-      // The special case is when this is the only non-label field on the block
-      // and it has an output but no inputs.
-      this.fullBlockClickTarget_ =
-        nFields <= 1 && block.outputConnection && !nConnections;
-    } else {
-      this.fullBlockClickTarget_ = false;
-    }
+    if (!block) throw new UnattachedFieldError();
+    super.initView();

-    if (this.fullBlockClickTarget_) {
+    if (this.isFullBlockField()) {
      this.clickTarget_ = (this.sourceBlock_ as BlockSvg).getSvgRoot();
-    } else {
-      this.createBorderRect_();
    }
-    this.createTextElement_();
+  }
+
+  protected override isFullBlockField(): boolean {
+    const block = this.getSourceBlock();
+    if (!block) throw new UnattachedFieldError();
+
+    // Side effect for backwards compatibility.
+    this.fullBlockClickTarget_ =
+      !!this.getConstants()?.FULL_BLOCK_FIELDS && block.isSimpleReporter();
+    return this.fullBlockClickTarget_;
  }

  /**
@@ -223,23 +209,54 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
   * Updates text field to match the colour/style of the block.
   */
  override applyColour() {
-    if (!this.sourceBlock_ || !this.getConstants()!.FULL_BLOCK_FIELDS) return;
+    const block = this.getSourceBlock() as BlockSvg | null;
+    if (!block) throw new UnattachedFieldError();

-    const source = this.sourceBlock_ as BlockSvg;
+    if (!this.getConstants()!.FULL_BLOCK_FIELDS) return;
+    if (!this.fieldGroup_) return;

-    if (this.borderRect_) {
-      this.borderRect_.setAttribute('stroke', source.style.colourTertiary);
+    if (!this.isFullBlockField() && this.borderRect_) {
+      this.borderRect_!.style.display = 'block';
+      this.borderRect_.setAttribute('stroke', block.style.colourTertiary);
    } else {
-      source.pathObject.svgPath.setAttribute(
+      this.borderRect_!.style.display = 'none';
+      // In general, do *not* let fields control the color of blocks. Having the
+      // field control the color is unexpected, and could have performance
+      // impacts.
+      block.pathObject.svgPath.setAttribute(
        'fill',
        this.getConstants()!.FIELD_BORDER_RECT_COLOUR,
      );
    }
  }

+  /**
+   * Returns the height and width of the field.
+   *
+   * This should *in general* be the only place render_ gets called from.
+   *
+   * @returns Height and width.
+   */
+  override getSize(): Size {
+    if (this.getConstants()?.FULL_BLOCK_FIELDS) {
+      // In general, do *not* let fields control the color of blocks. Having the
+      // field control the color is unexpected, and could have performance
+      // impacts.
+      // Full block fields have more control of the block than they should
+      // (i.e. updating fill colour). Whenever we get the size, the field may
+      // no longer be a full-block field, so we need to rerender.
+      this.render_();
+      this.isDirty_ = false;
+    }
+    return super.getSize();
+  }
+
  /**
   * Updates the colour of the htmlInput given the current validity of the
   * field's value.
+   *
+   * Also updates the colour of the block to reflect whether this is a full
+   * block field or not.
   */
  protected override render_() {
    super.render_();
@@ -256,6 +273,15 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
        aria.setState(htmlInput, aria.State.INVALID, false);
      }
    }
+
+    const block = this.getSourceBlock() as BlockSvg | null;
+    if (!block) throw new UnattachedFieldError();
+    // In general, do *not* let fields control the color of blocks. Having the
+    // field control the color is unexpected, and could have performance
+    // impacts.
+    // Whenever we render, the field may no longer be a full-block-field so
+    // we need to update the colour.
+    if (this.getConstants()!.FULL_BLOCK_FIELDS) block.applyColour();
  }

  /**
@@ -374,7 +400,7 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
    htmlInput.style.fontSize = fontSize;
    let borderRadius = FieldInput.BORDERRADIUS * scale + 'px';

-    if (this.fullBlockClickTarget_) {
+    if (this.isFullBlockField()) {
      const bBox = this.getScaledBBox();

      // Override border radius.
@@ -462,8 +488,6 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
   * @param _value The new value of the field.
   */
  onFinishEditing_(_value: AnyDuringMigration) {}
-  // NOP by default.
-  // TODO(#2496): Support people passing a func into the field.

  /**
   * Bind handlers for user input on the text input field's editor.