fix: Don't close the flyout when creating a variable using keyboard nav (#9745)

* feat: Use <dialog> instead of built-in browser alerts

* fix: Don't close the flyout when creating a variable using keyboard nav

* test: Fix tests

* fix: Take ephemeral focus for the dialog

* fix: Remove unneeded focused node check

packages/blockly/core/css.ts

@@ -8,6 +8,7 @@
 /** Has CSS already been injected? */
 const injectionSites = new WeakSet<Document | ShadowRoot>();
 const registeredStyleSheets: Array<CSSStyleSheet> = [];
+import * as userAgent from './utils/useragent.js';
 
 /**
  * Add some CSS to the blob that will be injected later.  Allows optional
@@ -659,4 +660,20 @@ input[type=number] {
   outline: var(--blockly-selection-width) solid var(--blockly-active-node-color);
   border-radius: 2px;
 }
+.blocklyDialog {
+  min-width: 300px;
+  border-radius: 16px;
+  box-shadow: 0 8px 8px rgba(0, 0, 0, 0.2);
+  border: 1px solid #999;
+}
+.blocklyDialogForm {
+  display: flex;
+  flex-direction: column;
+  row-gap: 8px;
+}
+.blocklyDialogButtonRow {
+  display: flex;
+  flex-direction: ${userAgent.MOBILE || userAgent.APPLE ? 'row-reverse' : 'row'};
+  column-gap: 8px;
+}
 `;

packages/blockly/core/dialog.ts

@@ -6,15 +6,24 @@
 
 // Former goog.module ID: Blockly.dialog
 
+import {getFocusManager} from './focus_manager.js';
+import {Msg} from './msg.js';
 import type {ToastOptions} from './toast.js';
 import {Toast} from './toast.js';
 import type {WorkspaceSvg} from './workspace_svg.js';
 
+/** Supported types of dialogs. */
+enum DialogType {
+  ALERT = 1,
+  CONFIRM,
+  PROMPT,
+}
+
+/** Tally of the number of dialogs currently on screen. */
+let activeDialogCount = 0;
+
 const defaultAlert = function (message: string, opt_callback?: () => void) {
-  window.alert(message);
-  if (opt_callback) {
-    opt_callback();
-  }
+  displayDialog(DialogType.ALERT, message, opt_callback, undefined);
 };
 
 let alertImplementation = defaultAlert;
@@ -23,7 +32,7 @@ const defaultConfirm = function (
   message: string,
   callback: (result: boolean) => void,
 ) {
-  callback(window.confirm(message));
+  displayDialog(DialogType.CONFIRM, message, callback, undefined);
 };
 
 let confirmImplementation = defaultConfirm;
@@ -33,9 +42,7 @@ const defaultPrompt = function (
   defaultValue: string,
   callback: (result: string | null) => void,
 ) {
-  // NOTE TO DEVELOPER: Ephemeral focus doesn't need to be taken for the native
-  // window prompt since it prevents focus from changing while open.
-  callback(window.prompt(message, defaultValue));
+  displayDialog(DialogType.PROMPT, message, callback, defaultValue);
 };
 
 let promptImplementation = defaultPrompt;
@@ -165,3 +172,108 @@ export function setToast(
 ) {
   toastImplementation = toastFunction;
 }
+
+/**
+ * Displays a dialog, potentially prompting for user input, and invokes the
+ * provided callback with the response.
+ */
+function displayDialog(
+  ...[type, message, callback, defaultValue]:
+    | [
+        type: DialogType.PROMPT,
+        message: string,
+        callback: (result: string | null) => void,
+        defaultValue: string | undefined,
+      ]
+    | [
+        type: DialogType.CONFIRM,
+        message: string,
+        callback: (result: boolean) => void,
+        defaultValue: undefined,
+      ]
+    | [
+        type: DialogType.ALERT,
+        message: string,
+        callback: (() => void) | undefined,
+        defaultValue: undefined,
+      ]
+): void {
+  const OK = 'ok';
+  const CANCEL = 'cancel';
+
+  const dialog = document.createElement('dialog');
+  const form = document.createElement('form');
+  const label = document.createElement('label');
+  const input = document.createElement('input');
+  const buttonRow = document.createElement('div');
+  const ok = document.createElement('button');
+
+  dialog.className = 'blocklyDialog';
+  form.className = 'blocklyDialogForm';
+  label.className = 'blocklyDialogPrompt';
+  buttonRow.className = 'blocklyDialogButtonRow';
+  ok.className = 'blocklyDialogConfirmButton';
+
+  form.setAttribute('method', 'dialog');
+
+  label.textContent = message;
+  label.setAttribute('for', 'blockly-form-input');
+
+  ok.textContent = Msg['DIALOG_OK'];
+  ok.value = OK;
+
+  dialog.appendChild(form);
+  form.appendChild(label);
+
+  if (type === DialogType.PROMPT) {
+    input.id = 'blockly-form-input';
+    input.className = 'blocklyDialogInput';
+    input.type = 'text';
+    input.name = 'input';
+    input.autofocus = true;
+    if (defaultValue) {
+      input.value = defaultValue;
+    }
+    form.appendChild(input);
+  }
+
+  buttonRow.appendChild(ok);
+
+  if (type === DialogType.CONFIRM || type === DialogType.PROMPT) {
+    const cancel = document.createElement('button');
+    cancel.className = 'blocklyDialogCancelButton';
+    cancel.textContent = Msg['DIALOG_CANCEL'];
+    cancel.value = CANCEL;
+    buttonRow.appendChild(cancel);
+  }
+
+  form.appendChild(buttonRow);
+
+  let restoreFocus: (() => void) | undefined;
+  if (activeDialogCount === 0) {
+    restoreFocus = getFocusManager().takeEphemeralFocus(dialog);
+  }
+
+  activeDialogCount++;
+
+  dialog.addEventListener('close', () => {
+    activeDialogCount--;
+    if (!activeDialogCount) {
+      restoreFocus?.();
+    }
+    dialog.remove();
+    switch (type) {
+      case DialogType.CONFIRM:
+        callback(dialog.returnValue === OK);
+        break;
+      case DialogType.PROMPT:
+        callback(dialog.returnValue === OK ? input.value : null);
+        break;
+      case DialogType.ALERT:
+        callback?.();
+    }
+  });
+  document.body.appendChild(dialog);
+
+  dialog.showModal();
+}

packages/blockly/core/flyout_base.ts

@@ -20,6 +20,7 @@ import * as eventUtils from './events/utils.js';
 import {FlyoutItem} from './flyout_item.js';
 import {FlyoutMetricsManager} from './flyout_metrics_manager.js';
 import {FlyoutSeparator, SeparatorAxis} from './flyout_separator.js';
+import {getFocusManager} from './focus_manager.js';
 import {IAutoHideable} from './interfaces/i_autohideable.js';
 import type {IFlyout} from './interfaces/i_flyout.js';
 import type {IFlyoutInflater} from './interfaces/i_flyout_inflater.js';
@@ -583,6 +584,16 @@ export abstract class Flyout
    *     toolbox definition, or a string with the name of the dynamic category.
    */
   show(flyoutDef: toolbox.FlyoutDefinition | string) {
+    // As part of showing, the existing contents of the flyout will be cleared.
+    // If the focused element is a flyout item, i.e. a child of the workspace
+    // and not the workspace itself, move focus to the workspace to prevent
+    // focus from being lost when the currently focused element is destroyed.
+    if (
+      getFocusManager().getFocusedTree() === this.workspace_ &&
+      getFocusManager().getFocusedNode() !== this.workspace_
+    ) {
+      getFocusManager().focusNode(this.workspace_);
+    }
     this.workspace_.setResizesEnabled(false);
     eventUtils.setRecordUndo(false);
     this.hide();

packages/blockly/core/variables.ts

@@ -7,8 +7,12 @@
 // Former goog.module ID: Blockly.Variables
 
 import type {Block} from './block.js';
+import type {BlockSvg} from './block_svg.js';
 import {Blocks} from './blocks.js';
 import * as dialog from './dialog.js';
+import type {BlockCreate} from './events/events.js';
+import * as Events from './events/events.js';
+import {getFocusManager} from './focus_manager.js';
 import {isLegacyProcedureDefBlock} from './interfaces/i_legacy_procedure_blocks.js';
 import {isVariableBackedParameterModel} from './interfaces/i_variable_backed_parameter_model.js';
 import {IVariableModel, IVariableState} from './interfaces/i_variable_model.js';
@@ -403,7 +407,7 @@ export function generateUniqueNameFromOptions(
  *     will default to '', which is a specific type.
  */
 export function createVariableButtonHandler(
-  workspace: Workspace,
+  workspace: WorkspaceSvg,
   opt_callback?: (p1?: string | null) => void,
   opt_type?: string,
 ) {
@@ -420,8 +424,28 @@ export function createVariableButtonHandler(
       const existing = nameUsedWithAnyType(text, workspace);
       if (!existing) {
         // No conflict
-        workspace.getVariableMap().createVariable(text, type);
+        const variable = workspace.getVariableMap().createVariable(text, type);
         if (opt_callback) opt_callback(text);
+        const flyoutWorkspace = workspace.getFlyout()?.getWorkspace();
+        if (!flyoutWorkspace) return;
+        const changeListener = (e: Events.Abstract) => {
+          // Focus the newly created variable_set block.
+          if (e.type === Events.BLOCK_CREATE) {
+            const blockId = (e as BlockCreate).blockId;
+            if (blockId) {
+              const block = flyoutWorkspace.getBlockById(blockId);
+              if (
+                block &&
+                block.type === 'variables_set' &&
+                block.getFieldValue('VAR') === variable.getId()
+              ) {
+                getFocusManager().focusNode(block as BlockSvg);
+                flyoutWorkspace.removeChangeListener(changeListener);
+              }
+            }
+          }
+        };
+        flyoutWorkspace.addChangeListener(changeListener);
         return;
       }
 

packages/blockly/tests/mocha/contextmenu_items_test.js

@@ -318,30 +318,38 @@ suite('Context Menu Items', function () {
 
       test('Deletes all blocks after confirming', function () {
         // Mocks the confirmation dialog and calls the callback with 'true' simulating ok.
-        const confirmStub = sinon.stub(window, 'confirm').returns(true);
+        let callCount = 0;
+        Blockly.dialog.setConfirm((_message, callback) => {
+          callCount++;
+          callback(true);
+        });
 
         this.workspace.newBlock('text');
         this.workspace.newBlock('text');
         this.deleteOption.callback(this.scope);
         this.clock.runAll();
-        sinon.assert.calledOnce(confirmStub);
+        assert.equal(callCount, 1);
         assert.equal(this.workspace.getTopBlocks(false).length, 0);
 
-        confirmStub.restore();
+        Blockly.dialog.setConfirm();
       });
 
       test('Does not delete blocks if not confirmed', function () {
         // Mocks the confirmation dialog and calls the callback with 'false' simulating cancel.
-        const confirmStub = sinon.stub(window, 'confirm').returns(false);
+        let callCount = 0;
+        Blockly.dialog.setConfirm((_message, callback) => {
+          callCount++;
+          callback(false);
+        });
 
         this.workspace.newBlock('text');
         this.workspace.newBlock('text');
         this.deleteOption.callback(this.scope);
         this.clock.runAll();
-        sinon.assert.calledOnce(confirmStub);
+        assert.equal(callCount, 1);
         assert.equal(this.workspace.getTopBlocks(false).length, 2);
 
-        confirmStub.restore();
+        Blockly.dialog.setConfirm();
       });
 
       test('No dialog for single block', function () {

packages/blockly/tests/mocha/dialog_test.js

@@ -24,11 +24,15 @@ suite('Dialog utilities', function () {
     Blockly.dialog.setToast();
   });
 
-  test('use the browser alert by default', function () {
-    const alert = sinon.stub(window, 'alert');
-    Blockly.dialog.alert('test');
-    assert.isTrue(alert.calledWith('test'));
-    alert.restore();
+  test('use the built in alert by default', function (done) {
+    const callback = () => {
+      done();
+    };
+    const message = 'test';
+    Blockly.dialog.alert(message, callback);
+    const dialog = document.querySelector('dialog');
+    assert.include(dialog.textContent, 'test');
+    dialog.querySelector('.blocklyDialogConfirmButton').click();
   });
 
   test('support setting a custom alert handler', function () {
@@ -40,24 +44,23 @@ suite('Dialog utilities', function () {
     assert.isTrue(alert.calledWith('test', callback));
   });
 
-  test('do not call the browser alert if a custom alert handler is set', function () {
-    const browserAlert = sinon.stub(window, 'alert');
-
+  test('do not call the built in alert if a custom alert handler is set', function () {
     const alert = sinon.spy();
     Blockly.dialog.setAlert(alert);
     Blockly.dialog.alert(test);
-    assert.isFalse(browserAlert.called);
-
-    browserAlert.restore();
+    const dialog = document.querySelector('dialog');
+    assert.isNull(dialog);
   });
 
-  test('use the browser confirm by default', function () {
-    const confirm = sinon.stub(window, 'confirm');
-    const callback = () => {};
+  test('use the built in confirm by default', function (done) {
+    const callback = () => {
+      done();
+    };
     const message = 'test';
     Blockly.dialog.confirm(message, callback);
-    assert.isTrue(confirm.calledWith(message));
-    confirm.restore();
+    const dialog = document.querySelector('dialog');
+    assert.include(dialog.textContent, 'test');
+    dialog.querySelector('.blocklyDialogCancelButton').click();
   });
 
   test('support setting a custom confirm handler', function () {
@@ -69,36 +72,39 @@ suite('Dialog utilities', function () {
     assert.isTrue(confirm.calledWith('test', callback));
   });
 
-  test('do not call the browser confirm if a custom confirm handler is set', function () {
-    const browserConfirm = sinon.stub(window, 'confirm');
-
+  test('do not call the built in confirm if a custom confirm handler is set', function () {
     const confirm = sinon.spy();
     Blockly.dialog.setConfirm(confirm);
     const callback = () => {};
     const message = 'test';
     Blockly.dialog.confirm(message, callback);
-    assert.isFalse(browserConfirm.called);
-
-    browserConfirm.restore();
+    const dialog = document.querySelector('dialog');
+    assert.isNull(dialog);
   });
 
-  test('invokes the provided callback with the confirmation response', function () {
-    const confirm = sinon.stub(window, 'confirm').returns(true);
-    const callback = sinon.spy();
+  test('invokes the provided callback with the confirmation response', function (done) {
+    const callback = (result) => {
+      assert.isTrue(result);
+      done();
+    };
     const message = 'test';
     Blockly.dialog.confirm(message, callback);
-    assert.isTrue(callback.calledWith(true));
-    confirm.restore();
+    const dialog = document.querySelector('dialog');
+    dialog.querySelector('.blocklyDialogConfirmButton').click();
   });
 
-  test('use the browser prompt by default', function () {
-    const prompt = sinon.stub(window, 'prompt');
-    const callback = () => {};
+  test('use the built in prompt by default', function (done) {
+    const callback = () => {
+      done();
+    };
     const message = 'test';
     const defaultValue = 'default';
     Blockly.dialog.prompt(message, defaultValue, callback);
-    assert.isTrue(prompt.calledWith(message, defaultValue));
-    prompt.restore();
+    const dialog = document.querySelector('dialog');
+    assert.include(dialog.textContent, 'test');
+    const input = dialog.querySelector('input');
+    assert.equal(input.value, 'default');
+    dialog.querySelector('.blocklyDialogCancelButton').click();
   });
 
   test('support setting a custom prompt handler', function () {
@@ -111,28 +117,30 @@ suite('Dialog utilities', function () {
     assert.isTrue(prompt.calledWith('test', defaultValue, callback));
   });
 
-  test('do not call the browser prompt if a custom prompt handler is set', function () {
-    const browserPrompt = sinon.stub(window, 'prompt');
-
+  test('do not call the built in prompt if a custom prompt handler is set', function () {
     const prompt = sinon.spy();
     Blockly.dialog.setPrompt(prompt);
     const callback = () => {};
     const message = 'test';
     const defaultValue = 'default';
     Blockly.dialog.prompt(message, defaultValue, callback);
-    assert.isFalse(browserPrompt.called);
-
-    browserPrompt.restore();
+    const dialog = document.querySelector('dialog');
+    assert.isNull(dialog);
   });
 
-  test('invokes the provided callback with the prompt response', function () {
-    const prompt = sinon.stub(window, 'prompt').returns('something');
-    const callback = sinon.spy();
+  test('invokes the provided callback with the prompt response', function (done) {
+    const callback = (response) => {
+      assert.equal(response, 'something');
+      done();
+    };
     const message = 'test';
     const defaultValue = 'default';
     Blockly.dialog.prompt(message, defaultValue, callback);
-    assert.isTrue(callback.calledWith('something'));
-    prompt.restore();
+    const dialog = document.querySelector('dialog');
+    assert.include(dialog.textContent, 'test');
+    const input = dialog.querySelector('input');
+    input.value = 'something';
+    dialog.querySelector('.blocklyDialogConfirmButton').click();
   });
 
   test('use the built-in toast by default', function () {

packages/blockly/tests/mocha/test_helpers/workspace.js

@@ -100,48 +100,60 @@ export function testAWorkspace() {
 
     test('deleteVariableById(id2) one usage', function () {
       // Deleting variable one usage should not trigger confirm dialog.
-      const stub = sinon.stub(window, 'confirm').returns(true);
+      let callCount = 0;
+      Blockly.dialog.setConfirm((_message, callback) => {
+        callCount++;
+        callback(true);
+      });
       const id2 = this.variableMap.getVariableById('id2');
       Blockly.Variables.deleteVariable(this.workspace, id2);
 
-      sinon.assert.notCalled(stub);
+      assert.equal(callCount, 0);
       const variable = this.variableMap.getVariableById('id2');
       assert.isNull(variable);
       assertVariableValues(this.variableMap, 'name1', 'type1', 'id1');
       assertBlockVarModelName(this.workspace, 0, 'name1');
 
-      stub.restore();
+      Blockly.dialog.setConfirm();
     });
 
     test('deleteVariableById(id1) multiple usages confirm', function () {
       // Deleting variable with multiple usages triggers confirm dialog.
-      const stub = sinon.stub(window, 'confirm').returns(true);
+      let callCount = 0;
+      Blockly.dialog.setConfirm((_message, callback) => {
+        callCount++;
+        callback(true);
+      });
       const id1 = this.variableMap.getVariableById('id1');
       Blockly.Variables.deleteVariable(this.workspace, id1);
 
-      sinon.assert.calledOnce(stub);
+      assert.equal(callCount, 1);
       const variable = this.variableMap.getVariableById('id1');
       assert.isNull(variable);
       assertVariableValues(this.variableMap, 'name2', 'type2', 'id2');
       assertBlockVarModelName(this.workspace, 0, 'name2');
 
-      stub.restore();
+      Blockly.dialog.setConfirm();
     });
 
     test('deleteVariableById(id1) multiple usages cancel', function () {
       // Deleting variable with multiple usages triggers confirm dialog.
-      const stub = sinon.stub(window, 'confirm').returns(false);
+      let callCount = 0;
+      Blockly.dialog.setConfirm((_message, callback) => {
+        callCount++;
+        callback(false);
+      });
       const id1 = this.variableMap.getVariableById('id1');
       Blockly.Variables.deleteVariable(this.workspace, id1);
 
-      sinon.assert.calledOnce(stub);
+      assert.equal(callCount, 1);
       assertVariableValues(this.variableMap, 'name1', 'type1', 'id1');
       assertVariableValues(this.variableMap, 'name2', 'type2', 'id2');
       assertBlockVarModelName(this.workspace, 0, 'name1');
       assertBlockVarModelName(this.workspace, 1, 'name1');
       assertBlockVarModelName(this.workspace, 2, 'name2');
 
-      stub.restore();
+      Blockly.dialog.setConfirm();
     });
   });