From bea183d85da6465cd0511fc48f2e612b99b6c1ff Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 8 Jul 2025 16:06:24 -0700 Subject: [PATCH] fix: Auto-close widget divs on lost focus (#9216) ## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes https://github.com/google/blockly-keyboard-experimentation/issues/563 ### Proposed Changes This expands the functionality introduced in #9213 to also include widget divs. ### Reason for Changes MakeCode makes use of widget div in several field editors, so the issues described in https://github.com/google/blockly-keyboard-experimentation/issues/563 aren't fully mitigated with #9213 alone. This PR essentially adds the same support for auto-closing as drop-down divs now have, and enables this functionality by default. Note the drop-down div change: it was missed in #9123 that the API change for drop-down div's `show` function is actually API-breaking, so this updates that API to be properly backward compatible (and reverts one test change that makes use of it). The `FocusManager` change actually corrects an implementation issue from #9123: not updating the tracked focus status before calling the callback can result in focus being inadvertently restored if the callback triggers returning focus due to a lost focus situation. This was wrong for drop-down divs, too, but it's harder to notice there because the dismissal of the drop-down div happens on a timer (which means there's sufficient time for `FocusManager`'s state to correct prior to attempting to return from the ephemeral focus state). Demonstration of fixed behavior (since the inline number editor uses a widget div): [Screen recording 2025-07-08 2.12.31 PM.webm](https://github.com/user-attachments/assets/7c3c7c3c-224c-48f4-b4af-bde86feecfa8) ### Test Coverage New widget div tests have been added to verify the new parameter and auto-close functionality. The `FocusManager` test was updated to account for the new, and correct, behavior around the internal tracked ephemeral focus state. Note that some `tabindex` state has been clarified and cleaned up in the test index page and `FocusManager`. It's fine (and preferable) for ephemeral-used elements to always be focusable rather than making them dynamically so (which avoids state bleed across test runs which was happening one of the new tests). https://github.com/google/blockly-keyboard-experimentation/pull/649 includes additional tests for validating widget behaviors. ### Documentation No new documentation should be needed here--the API documentation changes should be sufficient. One documentation update was made in `dropdowndiv.ts` that corrects the documentation parameter ordering. ### Additional Information Nothing further to add. --- core/dropdowndiv.ts | 6 +-- core/focus_manager.ts | 2 +- core/widgetdiv.ts | 16 +++++++- tests/mocha/dropdowndiv_test.js | 4 +- tests/mocha/focus_manager_test.js | 39 +++++++----------- tests/mocha/index.html | 2 +- tests/mocha/widget_div_test.js | 66 +++++++++++++++++++++++++++++++ 7 files changed, 102 insertions(+), 33 deletions(-) diff --git a/core/dropdowndiv.ts b/core/dropdowndiv.ts index 608fe9b5b..f30577822 100644 --- a/core/dropdowndiv.ts +++ b/core/dropdowndiv.ts @@ -346,8 +346,8 @@ function showPositionedByRect( secondaryX, secondaryY, manageEphemeralFocus, - autoCloseOnLostFocus, opt_onHide, + autoCloseOnLostFocus, ); } @@ -366,9 +366,9 @@ function showPositionedByRect( * @param primaryY Desired origin point y, in absolute px. * @param secondaryX Secondary/alternative origin point x, in absolute px. * @param secondaryY Secondary/alternative origin point y, in absolute px. - * @param opt_onHide Optional callback for when the drop-down is hidden. * @param manageEphemeralFocus Whether ephemeral focus should be managed * according to the widget div's lifetime. + * @param opt_onHide Optional callback for when the drop-down is hidden. * @param autoCloseOnLostFocus Whether the drop-down should automatically hide * if it loses DOM focus for any reason. * @returns True if the menu rendered at the primary origin point. @@ -382,8 +382,8 @@ export function show( secondaryX: number, secondaryY: number, manageEphemeralFocus: boolean, - autoCloseOnLostFocus: boolean, opt_onHide?: () => void, + autoCloseOnLostFocus?: boolean, ): boolean { owner = newOwner as Field; onHide = opt_onHide || null; diff --git a/core/focus_manager.ts b/core/focus_manager.ts index 31453b827..1510afca0 100644 --- a/core/focus_manager.ts +++ b/core/focus_manager.ts @@ -138,10 +138,10 @@ export class FocusManager { element instanceof Node && ephemeralFocusElem.contains(element); if (hadFocus !== hasFocus) { + this.ephemerallyFocusedElementCurrentlyHasFocus = hasFocus; if (this.ephemeralDomFocusChangedCallback) { this.ephemeralDomFocusChangedCallback(hasFocus); } - this.ephemerallyFocusedElementCurrentlyHasFocus = hasFocus; } } }; diff --git a/core/widgetdiv.ts b/core/widgetdiv.ts index f9b89de56..e0d4e1229 100644 --- a/core/widgetdiv.ts +++ b/core/widgetdiv.ts @@ -99,6 +99,8 @@ export function createDom() { * passed in here then callers should manage ephemeral focus directly * otherwise focus may not properly restore when the widget closes. Defaults * to true. + * @param autoCloseOnLostFocus Whether the widget should automatically hide if + * it loses DOM focus for any reason. */ export function show( newOwner: unknown, @@ -106,6 +108,7 @@ export function show( newDispose: () => void, workspace?: WorkspaceSvg | null, manageEphemeralFocus: boolean = true, + autoCloseOnLostFocus: boolean = true, ) { hide(); owner = newOwner; @@ -131,7 +134,18 @@ export function show( dom.addClass(div, themeClassName); } if (manageEphemeralFocus) { - returnEphemeralFocus = getFocusManager().takeEphemeralFocus(div); + const autoCloseCallback = autoCloseOnLostFocus + ? (hasFocus: boolean) => { + // If focus is ever lost, close the widget. + if (!hasFocus) { + hide(); + } + } + : null; + returnEphemeralFocus = getFocusManager().takeEphemeralFocus( + div, + autoCloseCallback, + ); } } diff --git a/tests/mocha/dropdowndiv_test.js b/tests/mocha/dropdowndiv_test.js index fac8368a9..9774ed024 100644 --- a/tests/mocha/dropdowndiv_test.js +++ b/tests/mocha/dropdowndiv_test.js @@ -155,7 +155,7 @@ suite('DropDownDiv', function () { }); test('Escape dismisses DropDownDiv', function () { let hidden = false; - Blockly.DropDownDiv.show(this, false, 0, 0, 0, 0, false, false, () => { + Blockly.DropDownDiv.show(this, false, 0, 0, 0, 0, false, () => { hidden = true; }); assert.isFalse(hidden); @@ -276,7 +276,7 @@ suite('DropDownDiv', function () { // Focus an element outside of the drop-down. document.getElementById('nonTreeElementForEphemeralFocus').focus(); - // the drop-down should now be hidden since it lost focus. + // The drop-down should now be hidden since it lost focus. const dropDownDivElem = document.querySelector('.blocklyDropDownDiv'); assert.strictEqual(dropDownDivElem.style.opacity, '0'); }); diff --git a/tests/mocha/focus_manager_test.js b/tests/mocha/focus_manager_test.js index cb4a43652..2544b44db 100644 --- a/tests/mocha/focus_manager_test.js +++ b/tests/mocha/focus_manager_test.js @@ -5624,21 +5624,6 @@ suite('FocusManager', function () { /* Ephemeral focus tests. */ suite('takeEphemeralFocus()', function () { - setup(function () { - // Ensure ephemeral-specific elements are focusable. - document.getElementById('nonTreeElementForEphemeralFocus').tabIndex = -1; - document.getElementById('nonTreeGroupForEphemeralFocus').tabIndex = -1; - }); - teardown(function () { - // Ensure ephemeral-specific elements have their tab indexes reset for a clean state. - document - .getElementById('nonTreeElementForEphemeralFocus') - .removeAttribute('tabindex'); - document - .getElementById('nonTreeGroupForEphemeralFocus') - .removeAttribute('tabindex'); - }); - test('with no focused node does not change states', function () { this.focusManager.registerTree(this.testFocusableTree2); this.focusManager.registerTree(this.testFocusableGroup2); @@ -6070,7 +6055,7 @@ suite('FocusManager', function () { assert.isTrue(callback.thirdCall.calledWithExactly(true)); }); - test('with focus change callback set focus to non-ephemeral element with auto return finishes ephemeral', function () { + test('with focus change callback set focus to non-ephemeral element with auto return finishes ephemeral does not restore to focused node', function () { this.focusManager.registerTree(this.testFocusableTree2); this.focusManager.registerTree(this.testFocusableGroup2); this.focusManager.focusNode(this.testFocusableTree2Node1); @@ -6090,21 +6075,24 @@ suite('FocusManager', function () { // Force focus away, triggering the callback's automatic returning logic. ephemeralElement2.focus(); - // The original focused node should be restored. - const nodeElem = this.testFocusableTree2Node1.getFocusableElement(); + // The original node should not be focused since the ephemeral element + // lost its own DOM focus while ephemeral focus was active. Instead, the + // newly active element should still hold focus. const activeElems = Array.from( document.querySelectorAll(ACTIVE_FOCUS_NODE_CSS_SELECTOR), ); - assert.strictEqual( - this.focusManager.getFocusedNode(), - this.testFocusableTree2Node1, + const passiveElems = Array.from( + document.querySelectorAll(PASSIVE_FOCUS_NODE_CSS_SELECTOR), ); - assert.strictEqual(activeElems.length, 1); + assert.isEmpty(activeElems); + assert.strictEqual(passiveElems.length, 1); assert.includesClass( - nodeElem.classList, - FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME, + this.testFocusableTree2Node1.getFocusableElement().classList, + FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME, ); - assert.strictEqual(document.activeElement, nodeElem); + assert.isNull(this.focusManager.getFocusedNode()); + assert.strictEqual(document.activeElement, ephemeralElement2); + assert.isFalse(this.focusManager.ephemeralFocusTaken()); }); test('with focus on non-ephemeral element ephemeral ended does not restore to focused node', function () { @@ -6139,6 +6127,7 @@ suite('FocusManager', function () { this.testFocusableTree2Node1.getFocusableElement().classList, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME, ); + assert.isNull(this.focusManager.getFocusedNode()); assert.strictEqual(document.activeElement, ephemeralElement2); assert.isFalse(this.focusManager.ephemeralFocusTaken()); }); diff --git a/tests/mocha/index.html b/tests/mocha/index.html index fea0fb18e..81618a4f8 100644 --- a/tests/mocha/index.html +++ b/tests/mocha/index.html @@ -142,7 +142,7 @@ - +