feat: consistent focus handling; remove markedNode

src/actions/arrow_navigation.ts

@@ -64,7 +64,9 @@ export class ArrowNavigation {
             case Constants.STATE.WORKSPACE:
               isHandled = this.fieldShortcutHandler(workspace, shortcut);
               if (!isHandled && workspace) {
-                workspace.getCursor()?.in();
+                if (!this.navigation.defaultCursorPositionIfNeeded(workspace)) {
+                  workspace.getCursor()?.in();
+                }
                 isHandled = true;
               }
               return isHandled;
@@ -95,7 +97,9 @@ export class ArrowNavigation {
             case Constants.STATE.WORKSPACE:
               isHandled = this.fieldShortcutHandler(workspace, shortcut);
               if (!isHandled && workspace) {
-                workspace.getCursor()?.out();
+                if (!this.navigation.defaultCursorPositionIfNeeded(workspace)) {
+                  workspace.getCursor()?.out();
+                }
                 isHandled = true;
               }
               return isHandled;
@@ -125,7 +129,9 @@ export class ArrowNavigation {
             case Constants.STATE.WORKSPACE:
               isHandled = this.fieldShortcutHandler(workspace, shortcut);
               if (!isHandled && workspace) {
-                workspace.getCursor()?.next();
+                if (!this.navigation.defaultCursorPositionIfNeeded(workspace)) {
+                  workspace.getCursor()?.next();
+                }
                 isHandled = true;
               }
               return isHandled;
@@ -158,7 +164,14 @@ export class ArrowNavigation {
             case Constants.STATE.WORKSPACE:
               isHandled = this.fieldShortcutHandler(workspace, shortcut);
               if (!isHandled) {
-                workspace.getCursor()?.prev();
+                if (
+                  !this.navigation.defaultCursorPositionIfNeeded(
+                    workspace,
+                    'last',
+                  )
+                ) {
+                  workspace.getCursor()?.prev();
+                }
                 isHandled = true;
               }
               return isHandled;

src/actions/clipboard.ts

@@ -275,10 +275,13 @@ export class Clipboard {
       ?.getCursor()
       ?.getCurNode()
       .getSourceBlock() as BlockSvg;
-    workspace.hideChaff();
     this.copyData = sourceBlock.toCopyData();
     this.copyWorkspace = sourceBlock.workspace;
-    return !!this.copyData;
+    const copied = !!this.copyData;
+    if (copied && navigationState === Constants.STATE.FLYOUT) {
+      this.navigation.focusWorkspace(workspace);
+    }
+    return copied;
   }
 
   /**
@@ -360,8 +363,10 @@ export class Clipboard {
       ? workspace
       : this.copyWorkspace;
 
-    // Do this before clipoard.paste due to cursor/focus workaround in getCurNode.
-    const targetNode = pasteWorkspace.getCursor()?.getCurNode();
+    const targetNode = this.navigation.getStationaryNode(pasteWorkspace);
+    // If we're pasting in the flyout it still targets the workspace. Focus first
+    // so ensure correct selection handling.
+    this.navigation.focusWorkspace(workspace);
 
     Events.setGroup(true);
     const block = clipboard.paste(this.copyData, pasteWorkspace) as BlockSvg;
@@ -373,7 +378,6 @@ export class Clipboard {
           ASTNode.createBlockNode(block)!,
         );
       }
-      this.navigation.removeMark(pasteWorkspace);
       Events.setGroup(false);
       return true;
     }

src/actions/enter.ts

@@ -93,6 +93,7 @@ export class EnterAction {
     const cursor = workspace.getCursor();
     if (!cursor) return;
     const curNode = cursor.getCurNode();
+    if (!curNode) return;
     const nodeType = curNode.getType();
     if (nodeType === ASTNode.types.FIELD) {
       (curNode.getLocation() as Field).showEditor();
@@ -125,7 +126,7 @@ export class EnterAction {
    *     the block will be placed on.
    */
   private insertFromFlyout(workspace: WorkspaceSvg) {
-    const stationaryNode = workspace.getCursor()?.getCurNode();
+    const stationaryNode = this.navigation.getStationaryNode(workspace);
     const newBlock = this.createNewBlock(workspace);
     if (!newBlock) return;
     if (stationaryNode) {
@@ -144,7 +145,6 @@ export class EnterAction {
 
     this.navigation.focusWorkspace(workspace);
     workspace.getCursor()!.setCurNode(ASTNode.createBlockNode(newBlock)!);
-    this.navigation.removeMark(workspace);
   }
 
   /**

src/gesture_monkey_patch.js

@@ -5,55 +5,28 @@
  */
 
 /**
- * @fileoverview Overrides methods on Blockly.Gesture in order to allow users
- * to move the cursor to blocks or the workspace using shift click.
- * TODO(google/blockly#4584): We do not have a way to do this currently without
- * monkey patching Blockly.
+ * @fileoverview Overrides methods on Blockly.Gesture to integrate focus mangagement
+ * with the gesture handling.
  * @author aschmiedt@google.com (Abby Schmiedt)
  */
 
 import * as Blockly from 'blockly/core';
 
-const oldDoWorkspaceClick = Blockly.Gesture.prototype.doWorkspaceClick_;
+const oldDispose = Blockly.Gesture.prototype.dispose;
 
 /**
- * Execute a workspace click. When in accessibility mode shift clicking will
- * move the cursor.
- * @param {!Event} e A mouse up or touch end event.
+ * Intercept the end of a gesture and ensure the workspace is focused.
+ * See also the listener is index.ts that subverts the markFocused call
+ * in `doStart`.
  * @this {Blockly.Gesture}
  * @override
  */
-Blockly.Gesture.prototype.doWorkspaceClick_ = function (e) {
-  oldDoWorkspaceClick.call(this, e);
-  const ws = this.creatorWorkspace_;
-  if (e.shiftKey && ws.keyboardAccessibilityMode) {
-    const screenCoord = new Blockly.utils.Coordinate(e.clientX, e.clientY);
-    const wsCoord = Blockly.utils.svgMath.screenToWsCoordinates(
-      ws,
-      screenCoord,
-    );
-    const wsNode = Blockly.ASTNode.createWorkspaceNode(ws, wsCoord);
-    ws.getCursor().setCurNode(wsNode);
-  }
-};
-
-const oldDoBlockClick = Blockly.Gesture.prototype.doBlockClick_;
-
-/**
- * Execute a block click. When in accessibility mode shift clicking will move
- * the cursor to the block.
- * @this {Blockly.Gesture}
- * @override
- */
-Blockly.Gesture.prototype.doBlockClick_ = function (e) {
-  oldDoBlockClick.call(this, e);
-  if (
-    !this.targetBlock_.isInFlyout &&
-    this.mostRecentEvent_.shiftKey &&
-    this.targetBlock_.workspace.keyboardAccessibilityMode
-  ) {
-    this.creatorWorkspace_
-      .getCursor()
-      .setCurNode(Blockly.ASTNode.createTopNode(this.targetBlock_));
+Blockly.Gesture.prototype.dispose = function () {
+  // This is a bit of a cludge and focus management needs to be better
+  // integrated with Gesture. The intent is to move focus at the end of
+  // a drag from a non-auto-closing flyout.
+  if (this.isDragging()) {
+    this.creatorWorkspace?.getSvgGroup().focus();
   }
+  oldDispose.call(this);
 };

src/index.ts

@@ -7,6 +7,7 @@
 import * as Blockly from 'blockly/core';
 import {NavigationController} from './navigation_controller';
 import {CursorOptions, LineCursor} from './line_cursor';
+import {getFlyoutElement, getToolboxElement} from './workspace_utilities';
 
 /** Options object for KeyboardNavigation instances. */
 export type NavigationOptions = {
@@ -24,16 +25,22 @@ export class KeyboardNavigation {
   protected workspace: Blockly.WorkspaceSvg;
 
   /** Event handler run when the workspace gains focus. */
-  private focusListener: () => void;
+  private focusListener: (e: Event) => void;
 
   /** Event handler run when the workspace loses focus. */
-  private blurListener: () => void;
+  private blurListener: (e: Event) => void;
 
   /** Event handler run when the toolbox gains focus. */
   private toolboxFocusListener: () => void;
 
   /** Event handler run when the toolbox loses focus. */
-  private toolboxBlurListener: () => void;
+  private toolboxBlurListener: (e: Event) => void;
+
+  /** Event handler run when the flyout gains focus. */
+  private flyoutFocusListener: () => void;
+
+  /** Event handler run when the flyout loses focus. */
+  private flyoutBlurListener: (e: Event) => void;
 
   /** Keyboard navigation controller instance for the workspace. */
   private navigationController: NavigationController;
@@ -86,31 +93,85 @@ export class KeyboardNavigation {
     // We add a focus listener below so use -1 so it doesn't become focusable.
     workspace.getParentSvg().setAttribute('tabindex', '-1');
 
-    this.focusListener = () => {
-      this.navigationController.updateWorkspaceFocus(workspace, true);
+    // Move the flyout for logical tab order.
+    const flyoutElement = getFlyoutElement(workspace);
+    flyoutElement?.parentElement?.insertBefore(
+      flyoutElement,
+      workspace.getParentSvg(),
+    );
+    // Allow tab to the flyout only when there's no toolbox.
+    if (workspace.getToolbox() && flyoutElement) {
+      flyoutElement.tabIndex = -1;
+    }
+
+    this.focusListener = (e: Event) => {
+      if (e.currentTarget === this.workspace.getParentSvg()) {
+        // Starting a gesture unconditionally calls markFocused on the parent SVG
+        // but we really don't want to move to the workspace (and close the
+        // flyout) if all you did was click in a flyout, potentially on a
+        // button. See also `gesture_monkey_patch.js`.
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        const gestureInternals = this.workspace.currentGesture_ as any;
+        const gestureFlyout = gestureInternals?.flyout;
+        const gestureFlyoutAutoClose = gestureFlyout?.autoClose;
+        const gestureOnBlock = gestureInternals?.startBlock;
+        if (
+          // When clicking on flyout that cannot close.
+          (gestureFlyout && !gestureFlyoutAutoClose) ||
+          // When clicking on a block in a flyout that can close.
+          (gestureFlyout && gestureFlyoutAutoClose && !gestureOnBlock)
+        ) {
+          this.navigationController.focusFlyout(workspace);
+        } else {
+          this.navigationController.focusWorkspace(workspace);
+        }
+      } else {
+        this.navigationController.handleFocusWorkspace(workspace);
+      }
     };
-    this.blurListener = () => {
-      this.navigationController.updateWorkspaceFocus(workspace, false);
+    this.blurListener = (e: Event) => {
+      const relatedTarget = (e as FocusEvent).relatedTarget;
+      if (
+        relatedTarget !== this.workspace.getParentSvg() &&
+        relatedTarget !== this.workspace.getSvgGroup()
+      ) {
+        this.navigationController.handleBlurWorkspace(workspace);
+      }
     };
 
     workspace.getSvgGroup().addEventListener('focus', this.focusListener);
     workspace.getSvgGroup().addEventListener('blur', this.blurListener);
 
+    const toolboxElement = getToolboxElement(workspace);
     this.toolboxFocusListener = () => {
-      this.navigationController.updateToolboxFocus(workspace, true);
+      this.navigationController.handleFocusToolbox(workspace);
     };
-    this.toolboxBlurListener = () => {
-      this.navigationController.updateToolboxFocus(workspace, false);
+    this.toolboxBlurListener = (e: Event) => {
+      this.navigationController.handleBlurToolbox(
+        workspace,
+        this.shouldCloseFlyoutOnBlur(
+          (e as FocusEvent).relatedTarget,
+          flyoutElement,
+        ),
+      );
     };
+    toolboxElement?.addEventListener('focus', this.toolboxFocusListener);
+    toolboxElement?.addEventListener('blur', this.toolboxBlurListener);
 
-    const toolbox = workspace.getToolbox();
-    if (toolbox != null && toolbox instanceof Blockly.Toolbox) {
-      const contentsDiv = toolbox.HtmlDiv?.querySelector(
-        '.blocklyToolboxContents',
+    this.flyoutFocusListener = () => {
+      this.navigationController.handleFocusFlyout(workspace);
+    };
+    this.flyoutBlurListener = (e: Event) => {
+      this.navigationController.handleBlurFlyout(
+        workspace,
+        this.shouldCloseFlyoutOnBlur(
+          (e as FocusEvent).relatedTarget,
+          toolboxElement,
+        ),
       );
-      contentsDiv?.addEventListener('focus', this.toolboxFocusListener);
-      contentsDiv?.addEventListener('blur', this.toolboxBlurListener);
-    }
+    };
+    flyoutElement?.addEventListener('focus', this.flyoutFocusListener);
+    flyoutElement?.addEventListener('blur', this.flyoutBlurListener);
 
     // Temporary workaround for #136.
     // TODO(#136): fix in core.
@@ -136,14 +197,13 @@ export class KeyboardNavigation {
       .getSvgGroup()
       .removeEventListener('focus', this.focusListener);
 
-    const toolbox = this.workspace.getToolbox();
-    if (toolbox != null && toolbox instanceof Blockly.Toolbox) {
-      const contentsDiv = toolbox.HtmlDiv?.querySelector(
-        '.blocklyToolboxContents',
-      );
-      contentsDiv?.removeEventListener('focus', this.toolboxFocusListener);
-      contentsDiv?.removeEventListener('blur', this.toolboxBlurListener);
-    }
+    const toolboxElement = getToolboxElement(this.workspace);
+    toolboxElement?.removeEventListener('focus', this.toolboxFocusListener);
+    toolboxElement?.removeEventListener('blur', this.toolboxBlurListener);
+
+    const flyoutElement = getFlyoutElement(this.workspace);
+    flyoutElement?.removeEventListener('focus', this.flyoutFocusListener);
+    flyoutElement?.removeEventListener('blur', this.flyoutBlurListener);
 
     if (this.workspaceParentTabIndex) {
       this.workspace
@@ -189,4 +249,32 @@ export class KeyboardNavigation {
     });
     this.workspace.setTheme(newTheme);
   }
+
+  /**
+   * Identify whether we should close the flyout when the toolbox or flyout
+   * blurs. If a gesture is in progerss or we're moving from one the other
+   * then we leave it open.
+   *
+   * @param relatedTarget The related target from the event on the flyout or toolbox.
+   * @param container The other element of flyout or toolbox (opposite to the event).
+   * @returns true if the flyout should be closed, false otherwise.
+   */
+  private shouldCloseFlyoutOnBlur(
+    relatedTarget: EventTarget | null,
+    container: Element | null,
+  ) {
+    if (Blockly.Gesture.inProgress()) {
+      return false;
+    }
+    if (!relatedTarget) {
+      return false;
+    }
+    if (
+      relatedTarget instanceof Node &&
+      container?.contains(relatedTarget as Node)
+    ) {
+      return false;
+    }
+    return true;
+  }
 }