fix: account for ephemeral focus in enter precondition (#619)

* chore: upgrade field colour to buggy version

This illustrates that the latest version, based on field-grid-dropdown, does
not work with keyboard navigation enabled. This is because Enter propagates
to the global handler, causing the field editor to be retriggered or block
toast messages to be shown.

* test: inverted test for colour field enter

Currently with 6.0.2 this triggers the global shortcuts.

* fix: account for ephemeral focus in enter precondition

Previously bugs could occur if the field editor contained a button but did
prevent keydown events propagating.

Author: microbit-matt-hillsdon

package-lock.json

@@ -49,7 +49,7 @@
   ],
   "devDependencies": {
     "@blockly/dev-scripts": "^4.0.8",
-    "@blockly/field-colour": "^6.0.0",
+    "@blockly/field-colour": "^6.0.2",
     "@eslint/eslintrc": "^2.1.2",
     "@eslint/js": "^8.49.0",
     "@types/chai": "^5.2.1",

package.json

@@ -64,7 +64,8 @@ export class EnterAction {
             const targetWorkspace = workspace.isFlyout
               ? workspace.targetWorkspace
               : workspace;
-            return !!targetWorkspace && !targetWorkspace.isReadOnly();
+            if (!targetWorkspace) return false;
+            return this.navigation.canCurrentlyEdit(targetWorkspace);
           }
           default:
             return false;
@@ -111,6 +112,8 @@ export class EnterAction {
    * @returns True if the enter action should be handled.
    */
   private shouldHandleEnterForWS(workspace: WorkspaceSvg): boolean {
+    if (!this.navigation.canCurrentlyNavigate(workspace)) return false;
+
     const cursor = workspace.getCursor();
     const curNode = cursor?.getCurNode();
     if (!curNode) return false;

src/actions/enter.ts

@@ -96,7 +96,12 @@ export class Navigation {
    * @returns The state of the given workspace.
    */
   getState(): Constants.STATE {
-    const focusedTree = Blockly.getFocusManager().getFocusedTree();
+    const focusManager = Blockly.getFocusManager();
+    if (focusManager.ephemeralFocusTaken()) {
+      return Constants.STATE.NOWHERE;
+    }
+
+    const focusedTree = focusManager.getFocusedTree();
     if (focusedTree instanceof Blockly.WorkspaceSvg) {
       if (focusedTree.isFlyout) {
         return Constants.STATE.FLYOUT;
@@ -837,11 +842,7 @@ export class Navigation {
       workspace.targetWorkspace ??
       workspace
     ).keyboardAccessibilityMode;
-    return (
-      !!accessibilityMode &&
-      this.getState() !== Constants.STATE.NOWHERE &&
-      !Blockly.getFocusManager().ephemeralFocusTaken()
-    );
+    return !!accessibilityMode && this.getState() !== Constants.STATE.NOWHERE;
   }
 
   /**
@@ -854,7 +855,7 @@ export class Navigation {
    * @returns whether keyboard navigation and editing is currently allowed.
    */
   canCurrentlyEdit(workspace: Blockly.WorkspaceSvg) {
-    return this.canCurrentlyNavigate(workspace) && !workspace.options.readOnly;
+    return this.canCurrentlyNavigate(workspace) && !workspace.isReadOnly();
   }
 
   /**

src/navigation.ts

@@ -363,4 +363,26 @@ suite('Keyboard navigation on Fields', function () {
     // The same field should still be focused
     chai.assert.equal(await getFocusedFieldName(this.browser), 'BOOL');
   });
+
+  test('Do not reopen field editor when handling enter to make a choice inside the editor', async function () {
+    // Open colour picker
+    await focusOnBlockField(this.browser, 'colour_picker_1', 'COLOUR');
+    await this.browser.pause(PAUSE_TIME);
+    await this.browser.keys(Key.Enter);
+    await this.browser.pause(PAUSE_TIME);
+
+    // Move right to pick a new colour.
+    await keyRight(this.browser);
+    // Enter to choose.
+    await this.browser.keys(Key.Enter);
+
+    // Focus seems to take longer than a single pause to settle.
+    await this.browser.waitUntil(
+      () =>
+        this.browser.execute(() =>
+          document.activeElement?.classList.contains('blocklyActiveFocus'),
+        ),
+      {timeout: 1000},
+    );
+  });
 });