feat: make comment editor separately focusable from comment itself (#9154)

* feat: make comment editor separately focusable from comment itself

* feat: improve design and add styling

* chore: fix lint

* fix: add event listeners to focus parent comment

* fix: export CommentEditor

* fix: export CommentEditor

* fix: extract comment identifier to constant

core/comments.ts

@@ -4,6 +4,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+export {CommentEditor} from './comments/comment_editor.js';
 export {CommentView} from './comments/comment_view.js';
 export {RenderedWorkspaceComment} from './comments/rendered_workspace_comment.js';
 export {WorkspaceComment} from './comments/workspace_comment.js';

core/comments/comment_editor.ts

@@ -0,0 +1,188 @@
+/**
+ * @license
+ * Copyright 2024 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import * as browserEvents from '../browser_events.js';
+import {getFocusManager} from '../focus_manager.js';
+import {IFocusableNode} from '../interfaces/i_focusable_node.js';
+import {IFocusableTree} from '../interfaces/i_focusable_tree.js';
+import * as dom from '../utils/dom.js';
+import {Size} from '../utils/size.js';
+import {Svg} from '../utils/svg.js';
+import {WorkspaceSvg} from '../workspace_svg.js';
+
+/**
+ * String added to the ID of a workspace comment to identify
+ * the focusable node for the comment editor.
+ */
+export const COMMENT_EDITOR_FOCUS_IDENTIFIER = '_comment_textarea_';
+
+/** The part of a comment that can be typed into. */
+export class CommentEditor implements IFocusableNode {
+  id?: string;
+  /** The foreignObject containing the HTML text area. */
+  private foreignObject: SVGForeignObjectElement;
+
+  /** The text area where the user can type. */
+  private textArea: HTMLTextAreaElement;
+
+  /** Listeners for changes to text. */
+  private textChangeListeners: Array<
+    (oldText: string, newText: string) => void
+  > = [];
+
+  /** The current text of the comment. Updates on text area change. */
+  private text: string = '';
+
+  constructor(
+    public workspace: WorkspaceSvg,
+    commentId?: string,
+    private onFinishEditing?: () => void,
+  ) {
+    this.foreignObject = dom.createSvgElement(Svg.FOREIGNOBJECT, {
+      'class': 'blocklyCommentForeignObject',
+    });
+    const body = document.createElementNS(dom.HTML_NS, 'body');
+    body.setAttribute('xmlns', dom.HTML_NS);
+    body.className = 'blocklyMinimalBody';
+    this.textArea = document.createElementNS(
+      dom.HTML_NS,
+      'textarea',
+    ) as HTMLTextAreaElement;
+    dom.addClass(this.textArea, 'blocklyCommentText');
+    dom.addClass(this.textArea, 'blocklyTextarea');
+    dom.addClass(this.textArea, 'blocklyText');
+    body.appendChild(this.textArea);
+    this.foreignObject.appendChild(body);
+
+    if (commentId) {
+      this.id = commentId + COMMENT_EDITOR_FOCUS_IDENTIFIER;
+      this.textArea.setAttribute('id', this.id);
+    }
+
+    // Register browser event listeners for the user typing in the textarea.
+    browserEvents.conditionalBind(
+      this.textArea,
+      'change',
+      this,
+      this.onTextChange,
+    );
+
+    // Register listener for pointerdown to focus the textarea.
+    browserEvents.conditionalBind(
+      this.textArea,
+      'pointerdown',
+      this,
+      (e: PointerEvent) => {
+        // don't allow this event to bubble up
+        // and steal focus away from the editor/comment.
+        e.stopPropagation();
+        getFocusManager().focusNode(this);
+      },
+    );
+
+    // Register listener for keydown events that would finish editing.
+    browserEvents.conditionalBind(
+      this.textArea,
+      'keydown',
+      this,
+      this.handleKeyDown,
+    );
+  }
+
+  /** Gets the dom structure for this comment editor. */
+  getDom(): SVGForeignObjectElement {
+    return this.foreignObject;
+  }
+
+  /** Gets the current text of the comment. */
+  getText(): string {
+    return this.text;
+  }
+
+  /** Sets the current text of the comment and fires change listeners. */
+  setText(text: string) {
+    this.textArea.value = text;
+    this.onTextChange();
+  }
+
+  /**
+   * Triggers listeners when the text of the comment changes, either
+   * programmatically or manually by the user.
+   */
+  private onTextChange() {
+    const oldText = this.text;
+    this.text = this.textArea.value;
+    // Loop through listeners backwards in case they remove themselves.
+    for (let i = this.textChangeListeners.length - 1; i >= 0; i--) {
+      this.textChangeListeners[i](oldText, this.text);
+    }
+  }
+
+  /**
+   * Do something when the user indicates they've finished editing.
+   *
+   * @param e Keyboard event.
+   */
+  private handleKeyDown(e: KeyboardEvent) {
+    if (e.key === 'Escape' || (e.key === 'Enter' && (e.ctrlKey || e.metaKey))) {
+      if (this.onFinishEditing) this.onFinishEditing();
+      e.stopPropagation();
+    }
+  }
+
+  /** Registers a callback that listens for text changes. */
+  addTextChangeListener(listener: (oldText: string, newText: string) => void) {
+    this.textChangeListeners.push(listener);
+  }
+
+  /** Removes the given listener from the list of text change listeners. */
+  removeTextChangeListener(listener: () => void) {
+    this.textChangeListeners.splice(
+      this.textChangeListeners.indexOf(listener),
+      1,
+    );
+  }
+
+  /** Sets the placeholder text displayed for an empty comment. */
+  setPlaceholderText(text: string) {
+    this.textArea.placeholder = text;
+  }
+
+  /** Sets whether the textarea is editable. If not, the textarea will be readonly. */
+  setEditable(isEditable: boolean) {
+    if (isEditable) {
+      this.textArea.removeAttribute('readonly');
+    } else {
+      this.textArea.setAttribute('readonly', 'true');
+    }
+  }
+
+  /** Update the size of the comment editor element. */
+  updateSize(size: Size, topBarSize: Size) {
+    this.foreignObject.setAttribute(
+      'height',
+      `${size.height - topBarSize.height}`,
+    );
+    this.foreignObject.setAttribute('width', `${size.width}`);
+    this.foreignObject.setAttribute('y', `${topBarSize.height}`);
+    if (this.workspace.RTL) {
+      this.foreignObject.setAttribute('x', `${-size.width}`);
+    }
+  }
+
+  getFocusableElement(): HTMLElement | SVGElement {
+    return this.textArea;
+  }
+  getFocusableTree(): IFocusableTree {
+    return this.workspace;
+  }
+  onNodeFocus(): void {}
+  onNodeBlur(): void {}
+  canBeFocused(): boolean {
+    if (this.id) return true;
+    return false;
+  }
+}

core/comments/comment_view.ts

@@ -6,6 +6,7 @@
 
 import * as browserEvents from '../browser_events.js';
 import * as css from '../css.js';
+import type {IFocusableNode} from '../interfaces/i_focusable_node';
 import {IRenderedElement} from '../interfaces/i_rendered_element.js';
 import * as layers from '../layers.js';
 import * as touch from '../touch.js';
@@ -15,6 +16,7 @@ import * as drag from '../utils/drag.js';
 import {Size} from '../utils/size.js';
 import {Svg} from '../utils/svg.js';
 import {WorkspaceSvg} from '../workspace_svg.js';
+import {CommentEditor} from './comment_editor.js';
 
 export class CommentView implements IRenderedElement {
   /** The root group element of the comment view. */
@@ -46,11 +48,8 @@ export class CommentView implements IRenderedElement {
   /** The resize handle element. */
   private resizeHandle: SVGImageElement;
 
-  /** The foreignObject containing the HTML text area. */
-  private foreignObject: SVGForeignObjectElement;
-
-  /** The text area where the user can type. */
-  private textArea: HTMLTextAreaElement;
+  /** The part of the comment view that contains the textarea to edit the comment. */
+  private commentEditor: CommentEditor;
 
   /** The current size of the comment in workspace units. */
   private size: Size;
@@ -64,14 +63,6 @@ export class CommentView implements IRenderedElement {
   /** The current location of the comment in workspace coordinates. */
   private location: Coordinate = new Coordinate(0, 0);
 
-  /** The current text of the comment. Updates on  text area change. */
-  private text: string = '';
-
-  /** Listeners for changes to text. */
-  private textChangeListeners: Array<
-    (oldText: string, newText: string) => void
-  > = [];
-
   /** Listeners for changes to size. */
   private sizeChangeListeners: Array<(oldSize: Size, newSize: Size) => void> =
     [];
@@ -106,7 +97,10 @@ export class CommentView implements IRenderedElement {
   /** The default size of newly created comments. */
   static defaultCommentSize = new Size(120, 100);
 
-  constructor(readonly workspace: WorkspaceSvg) {
+  constructor(
+    readonly workspace: WorkspaceSvg,
+    private commentId?: string,
+  ) {
     this.svgRoot = dom.createSvgElement(Svg.G, {
       'class': 'blocklyComment blocklyEditable blocklyDraggable',
     });
@@ -122,8 +116,7 @@ export class CommentView implements IRenderedElement {
       textPreviewNode: this.textPreviewNode,
     } = this.createTopBar(this.svgRoot, workspace));
 
-    ({foreignObject: this.foreignObject, textArea: this.textArea} =
-      this.createTextArea(this.svgRoot));
+    this.commentEditor = this.createTextArea();
 
     this.resizeHandle = this.createResizeHandle(this.svgRoot, workspace);
 
@@ -236,33 +229,32 @@ export class CommentView implements IRenderedElement {
   /**
    * Creates the text area where users can type. Registers event listeners.
    */
-  private createTextArea(svgRoot: SVGGElement): {
-    foreignObject: SVGForeignObjectElement;
-    textArea: HTMLTextAreaElement;
-  } {
-    const foreignObject = dom.createSvgElement(
-      Svg.FOREIGNOBJECT,
-      {
-        'class': 'blocklyCommentForeignObject',
-      },
-      svgRoot,
+  private createTextArea() {
+    // When the user is done editing comment, focus the entire comment.
+    const onFinishEditing = () => this.svgRoot.focus();
+    const commentEditor = new CommentEditor(
+      this.workspace,
+      this.commentId,
+      onFinishEditing,
     );
-    const body = document.createElementNS(dom.HTML_NS, 'body');
-    body.setAttribute('xmlns', dom.HTML_NS);
-    body.className = 'blocklyMinimalBody';
-    const textArea = document.createElementNS(
-      dom.HTML_NS,
-      'textarea',
-    ) as HTMLTextAreaElement;
-    dom.addClass(textArea, 'blocklyCommentText');
-    dom.addClass(textArea, 'blocklyTextarea');
-    dom.addClass(textArea, 'blocklyText');
-    body.appendChild(textArea);
-    foreignObject.appendChild(body);
 
-    browserEvents.conditionalBind(textArea, 'change', this, this.onTextChange);
+    this.svgRoot.appendChild(commentEditor.getDom());
 
-    return {foreignObject, textArea};
+    commentEditor.addTextChangeListener((oldText, newText) => {
+      this.updateTextPreview(newText);
+      // Update size in case our minimum size increased.
+      this.setSize(this.size);
+    });
+
+    return commentEditor;
+  }
+
+  /**
+   *
+   * @returns The FocusableNode representing the editor portion of this comment.
+   */
+  getEditorFocusableNode(): IFocusableNode {
+    return this.commentEditor;
   }
 
   /** Creates the DOM elements for the comment resize handle. */
@@ -324,7 +316,7 @@ export class CommentView implements IRenderedElement {
 
     this.updateHighlightRect(size);
     this.updateTopBarSize(size);
-    this.updateTextAreaSize(size, topBarSize);
+    this.commentEditor.updateSize(size, topBarSize);
     this.updateDeleteIconPosition(size, topBarSize, deleteSize);
     this.updateFoldoutIconPosition(topBarSize, foldoutSize);
     this.updateTextPreviewSize(
@@ -360,7 +352,7 @@ export class CommentView implements IRenderedElement {
     foldoutSize: Size,
     deleteSize: Size,
   ): Size {
-    this.updateTextPreview(this.textArea.value ?? '');
+    this.updateTextPreview(this.commentEditor.getText() ?? '');
     const textPreviewWidth = dom.getTextWidth(this.textPreview);
 
     const foldoutMargin = this.calcFoldoutMargin(topBarSize, foldoutSize);
@@ -408,19 +400,6 @@ export class CommentView implements IRenderedElement {
     this.topBarBackground.setAttribute('width', `${size.width}`);
   }
 
-  /** Updates the size of the text area elements to reflect the new size. */
-  private updateTextAreaSize(size: Size, topBarSize: Size) {
-    this.foreignObject.setAttribute(
-      'height',
-      `${size.height - topBarSize.height}`,
-    );
-    this.foreignObject.setAttribute('width', `${size.width}`);
-    this.foreignObject.setAttribute('y', `${topBarSize.height}`);
-    if (this.workspace.RTL) {
-      this.foreignObject.setAttribute('x', `${-size.width}`);
-    }
-  }
-
   /**
    * Updates the position of the delete icon elements to reflect the new size.
    */
@@ -652,12 +631,11 @@ export class CommentView implements IRenderedElement {
     if (this.editable) {
       dom.addClass(this.svgRoot, 'blocklyEditable');
       dom.removeClass(this.svgRoot, 'blocklyReadonly');
-      this.textArea.removeAttribute('readonly');
     } else {
       dom.removeClass(this.svgRoot, 'blocklyEditable');
       dom.addClass(this.svgRoot, 'blocklyReadonly');
-      this.textArea.setAttribute('readonly', 'true');
     }
+    this.commentEditor.setEditable(editable);
   }
 
   /** Returns the current location of the comment in workspace coordinates. */
@@ -678,49 +656,29 @@ export class CommentView implements IRenderedElement {
     );
   }
 
-  /** Retursn the current text of the comment. */
+  /** Returns the current text of the comment. */
   getText() {
-    return this.text;
+    return this.commentEditor.getText();
   }
 
   /** Sets the current text of the comment. */
   setText(text: string) {
-    this.textArea.value = text;
-    this.onTextChange();
+    this.commentEditor.setText(text);
   }
 
   /** Sets the placeholder text displayed for an empty comment. */
   setPlaceholderText(text: string) {
-    this.textArea.placeholder = text;
+    this.commentEditor.setPlaceholderText(text);
   }
 
-  /** Registers a callback that listens for text changes. */
+  /** Registers a callback that listens for text changes on the comment editor. */
   addTextChangeListener(listener: (oldText: string, newText: string) => void) {
-    this.textChangeListeners.push(listener);
+    this.commentEditor.addTextChangeListener(listener);
   }
 
-  /** Removes the given listener from the list of text change listeners. */
+  /** Removes the given listener from the comment editor. */
   removeTextChangeListener(listener: () => void) {
-    this.textChangeListeners.splice(
-      this.textChangeListeners.indexOf(listener),
-      1,
-    );
-  }
-
-  /**
-   * Triggers listeners when the text of the comment changes, either
-   * programmatically or manually by the user.
-   */
-  private onTextChange() {
-    const oldText = this.text;
-    this.text = this.textArea.value;
-    this.updateTextPreview(this.text);
-    // Update size in case our minimum size increased.
-    this.setSize(this.size);
-    // Loop through listeners backwards in case they remove themselves.
-    for (let i = this.textChangeListeners.length - 1; i >= 0; i--) {
-      this.textChangeListeners[i](oldText, this.text);
-    }
+    this.commentEditor.removeTextChangeListener(listener);
   }
 
   /** Updates the preview text element to reflect the given text. */
@@ -884,6 +842,11 @@ css.register(`
   fill: none;
 }
 
+.blocklyCommentText.blocklyActiveFocus {
+  border-color: #fc3;
+  border-width: 2px;
+}
+
 .blocklySelected .blocklyCommentHighlight {
   stroke: #fc3;
   stroke-width: 3px;

core/comments/rendered_workspace_comment.ts

@@ -47,7 +47,7 @@ export class RenderedWorkspaceComment
     IFocusableNode
 {
   /** The class encompassing the svg elements making up the workspace comment. */
-  private view: CommentView;
+  view: CommentView;
 
   public readonly workspace: WorkspaceSvg;
 
@@ -59,7 +59,7 @@ export class RenderedWorkspaceComment
 
     this.workspace = workspace;
 
-    this.view = new CommentView(workspace);
+    this.view = new CommentView(workspace, this.id);
     // Set the size to the default size as defined in the superclass.
     this.view.setSize(this.getSize());
     this.view.setEditable(this.isEditable());
@@ -224,13 +224,7 @@ export class RenderedWorkspaceComment
   private startGesture(e: PointerEvent) {
     const gesture = this.workspace.getGesture(e);
     if (gesture) {
-      if (browserEvents.isTargetInput(e)) {
-        // If the text area was the focus, don't allow this event to bubble up
-        // and steal focus away from the editor/comment.
-        e.stopPropagation();
-      } else {
-        gesture.handleCommentStart(e, this);
-      }
+      gesture.handleCommentStart(e, this);
       getFocusManager().focusNode(this);
     }
   }
@@ -339,6 +333,13 @@ export class RenderedWorkspaceComment
     }
   }
 
+  /**
+   * @returns The FocusableNode representing the editor portion of this comment.
+   */
+  getEditorFocusableNode(): IFocusableNode {
+    return this.view.getEditorFocusableNode();
+  }
+
   /** See IFocusableNode.getFocusableElement. */
   getFocusableElement(): HTMLElement | SVGElement {
     return this.getSvgRoot();

core/workspace_svg.ts

@@ -22,6 +22,7 @@ import type {Block} from './block.js';
 import type {BlockSvg} from './block_svg.js';
 import type {BlocklyOptions} from './blockly_options.js';
 import * as browserEvents from './browser_events.js';
+import {COMMENT_EDITOR_FOCUS_IDENTIFIER} from './comments/comment_editor.js';
 import {RenderedWorkspaceComment} from './comments/rendered_workspace_comment.js';
 import {WorkspaceComment} from './comments/workspace_comment.js';
 import * as common from './common.js';
@@ -2729,6 +2730,26 @@ export class WorkspaceSvg
     return nestedWorkspaces;
   }
 
+  /**
+   * Used for searching for a specific workspace comment.
+   * We can't use this.getWorkspaceCommentById because the workspace
+   * comment ids might not be globally unique, but the id assigned to
+   * the focusable element for the comment should be.
+   */
+  private searchForWorkspaceComment(
+    id: string,
+  ): RenderedWorkspaceComment | undefined {
+    for (const comment of this.getTopComments()) {
+      if (
+        comment instanceof RenderedWorkspaceComment &&
+        comment.canBeFocused() &&
+        comment.getFocusableElement().id === id
+      ) {
+        return comment;
+      }
+    }
+  }
+
   /** See IFocusableTree.lookUpFocusableNode. */
   lookUpFocusableNode(id: string): IFocusableNode | null {
     // Check against flyout items if this workspace is part of a flyout. Note
@@ -2773,21 +2794,29 @@ export class WorkspaceSvg
       return null;
     }
 
+    // Search for a specific workspace comment editor
+    // (only if id seems like it is one).
+    const commentEditorIndicator = id.indexOf(COMMENT_EDITOR_FOCUS_IDENTIFIER);
+    if (commentEditorIndicator !== -1) {
+      const commentId = id.substring(0, commentEditorIndicator);
+      const comment = this.searchForWorkspaceComment(commentId);
+      if (comment) {
+        return comment.getEditorFocusableNode();
+      }
+    }
+
     // Search for a specific block.
+    // Don't use `getBlockById` because the block ID is not guaranteeed
+    // to be globally unique, but the ID on the focusable element is.
     const block = this.getAllBlocks(false).find(
       (block) => block.getFocusableElement().id === id,
     );
     if (block) return block;
 
     // Search for a workspace comment (semi-expensive).
-    for (const comment of this.getTopComments()) {
-      if (
-        comment instanceof RenderedWorkspaceComment &&
-        comment.canBeFocused() &&
-        comment.getFocusableElement().id === id
-      ) {
-        return comment;
-      }
+    const comment = this.searchForWorkspaceComment(id);
+    if (comment) {
+      return comment;
     }
 
     // Search for icons and bubbles (which requires an expensive getAllBlocks).