feat: consistent focus handling; remove markedNode (#326)

* feat: consistent focus handling; remove markedNode

Ensure we always focus the same element when keyboard navigation works in that
element. Previously the toolbox sometimes was and sometimes wasn't focused when
it navigated, the workspace or group or the workspace SVG could be focused and
the flyout never took the focus.

With this approach the relevant container takes focus.

Manage the passive focus indicator based on focus rather than markedNode state.

Integrate with Gesture to determine how mouse behaviours relate to focus. This
prevents issues where a gesture would cause a focus change which would close
the flyout, interrupting a normal drag. In time hopefully this code can move to
core where it will be more natural and less fragile.

Fixes #227
Fixes #200
Part of #229 (scrollbars still an issue)
Fixes #222

* fix: take care making a selection when focusing the workspace

This can interfere with in-progress gestures causing them to act in unexpected
ways. We now clear the current node when focus moves away.

Avoid altering the selection in cursor.draw to prevent similar poorly timed
selections.

If the cursor position is lost, e.g. by click in the workspace, the arrow keys
now default it if needed. Tabbing to the workspace always shows a cursor.

Fixes:
- Click in workspace blank space now doesn't select a block on mouse down
- The mini-workspace for "if" and similar now behaves correctly again (no
  keyboard nav still though)
- Also fixes #287

* fix: cope with null node

* fix: remove unintended clearing of current node

* fix: flyout-only paste fix

* fix: selection in paste in flyout case

* chore: clarify case this applies

Author: microbit-matt-hillsdon

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 [email protected] (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;
+  }
 }