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

Author: microbit-matt-hillsdon

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;
   }
 
   /**
@@ -373,7 +376,6 @@ export class Clipboard {
           ASTNode.createBlockNode(block)!,
         );
       }
-      this.navigation.removeMark(pasteWorkspace);
       Events.setGroup(false);
       return true;
     }

src/actions/enter.ts

@@ -144,7 +144,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,27 @@
  */
 
 /**
- * @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.
+  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,7 +25,7 @@ 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;
@@ -33,7 +34,13 @@ export class KeyboardNavigation {
   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,79 @@ 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.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 +191,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 +243,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;
+  }
 }